iOS: only add explicit transactions when there are platform views (only on main threads)

[cyanglaz]

Description

We only need to add explicit CATransactions on the main thread since platform views are only going to be on the main thread.
Also, adding explicit transactions seem to crash in certain image picker scenarios on iOS 14, so remove these transactions on the background thread will fix those crashes (on metal only)

Related Issues

flutter/flutter#66647

Tests

Not sure what we can do to test the rendering synchronization. I added a bunch of FML_DCHECKs so at least, the scenario app tests in the engine should catch un-balanced CATransactions or CATransactions on the background thread.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

• I read the contributor guide and followed the process outlined there for submitting PRs.
• I signed the CLA.
• I read and followed the C++, Objective-C, Java style guides for the engine.
• I read the tree hygiene wiki page, which explains my responsibilities.
• I updated/added relevant documentation.
• All existing and new tests are passing.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

• No, no existing tests failed, so this is not a breaking change.
• Yes, this is a breaking change. If not, delete the remainder of this section.
  • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
  • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
  • I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[gaaclarke]

This is going to be somewhat costly. Couldn't we just do something like:

PlatformViews.mm

io_surface->AddCATransaction();
layer.transform = newTransform;

io_surface.mm:

void IOSurface::AddCATransaction() {
  assert(IsRunningOnMainThread());
  if (!has_ca_transaction_) {
    [CATransaction begin];
    has_ca_transaction_ = true;
  }
}

void IOSSurface::SubmitFrame() {
  if (has_ca_transaction) {
    [CATransaction commit];
    has_ca_transaction_ = false;
  }
}

[cyanglaz]

It is not true that the platform views code runs entirely on the main thread. The platform view's code will run on the raster thread until PostPrerollAction, then it determines if there are platform views, if so, it will merge the threads, then the rasterizer run son the main thread in the next 10 frames.

So even we move the transaction code into the platformviews.mm, we would sill need to check if ([[NSThread currentThread] isMainThread])

I'm not sure how costly the code for checking main thread is (I thought it is just a bit check). If it is costly, we can just check once and set a flag like what you did, and only check our flag in the same frame.

[gaaclarke]

It's not a bit check because there are 2 different objc_msgsend's in order to even get to the implementation. It isn't just the cost, the proposal I'm making makes the logic more clear. We are adding a CATransaction only when one is needed. The addition of the transaction is close to the location that needs it, too, so it is easier to maintain and understand.

The way the transaction is now, it isn't obvious why we even need it.

[gaaclarke]

This is a lot better from a code perspective. I would wrap up the begin and the commit into small helper methods. I do have concerns that we are changing the location of the [CATransaction commit]. Did you test it out? I just want to make sure it doesn't cause synchronization problems. That's why I suggested keeping the commit in the same place.

[cyanglaz]

@gaaclarke Yes, I have tested with local engine and it works.
We don't have automated tests set up for this and I'm not sure how to automated test this.
I have added some DChecks for tests to catch tho.

Also updated with your suggestions.

[gaaclarke]

Code looks good, as long as manual testing checks out for synchronization in platformviews (which it has) LGTM.

I tested this on iPhone SE 2 with the image_picker example and the crash is gone on device.

[Hixie]

tests: this PR can land without tests since our repro involves a specific plugin, on the condition that we prioritise the work to add an end-to-end test to the plugin itself that tests this ASAP. Thanks.

[flutter-dashboard]

This pull request was opened against a branch other than main. Since Flutter pull requests should not normally be opened against branches other than main, I have changed the base to main. If this was intended, you may modify the base back to master. See the Release Process for information about how other branches get updated.

Reviewers: Use caution before merging pull requests to branches other than main, unless this is an intentional hotfix/cherrypick.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.