[iOS] Create Explicit Transation Avoid Implicit Checker

[houhuayong]

fix： flutter/flutter#103390

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
  writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[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.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

[chinmaygarde]

cc @cyanglaz . I think this might be related to the platform views stuff you are working on right now.

[cyanglaz]

We begin transactions here: https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L287 and commit here: https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L561

These 2 locations are called for every frame that rasterizer runs on platform thread.

Essentially, this change would make an additional CATransaction in 2 scenarios:

1. When there is no thread merging, no platform view on the screen, a new CATransaction is created on the raster thread
2. When threads are merged and there are platform view(s) on the screen. An additional CATransaction begin-commit pair is created around an existing CATransaction begin-commit pair.

I'm not sure how either of the scenario fixed the issue mentioned in the PR.
It does look like we have some issue with CATransaction not paired correctly though. This might be related: #33262 Do you want to try the PR and see if it is fixed for you?

[houhuayong]

We begin transactions here: https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L287 and commit here: https://github.com/flutter/engine/blob/main/shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm#L561

These 2 locations are called for every frame that rasterizer runs on platform thread.

Essentially, this change would make an additional CATransaction in 2 scenarios:

1. When there is no thread merging, no platform view on the screen, a new CATransaction is created on the raster thread
2. When threads are merged and there are platform view(s) on the screen. An additional CATransaction begin-commit pair is created around an existing CATransaction begin-commit pair.

I'm not sure how either of the scenario fixed the issue mentioned in the PR. It does look like we have some issue with CATransaction not paired correctly though. This might be related: #33262 Do you want to try the PR and see if it is fixed for you?

thanks for your reply, I've tried #33262 and is not fixed for me.

From Apple

Core Animation supports two types of transactions: implicit transactions and explicit transactions. Implicit transactions are created automatically when the layer tree is modified by a thread without an active transaction and are committed automatically when the thread's runloop next iterates. Explicit transactions occur when the the application sends the CATransaction class a begin() message before modifying the layer tree, and a commit() message afterwards.

when the value layer.presentsWithTransaction changed, in some case, It will trigger layer tree modified and so Implicit transaction is created. and in flutter, it is on raster thread. If we set CA_ASSERT_MAIN_THREAD_TRANSACTIONS = 1 in add-to-app, It's not allowed to create Implicit transaction unless on main thread. This is why this code can fix my issue.

[cyanglaz]

I don't understand the full problem here, it looks like CA_ASSERT_MAIN_THREAD_TRANSACTIONS is a debugging flag, which indicates we were adding transactions (implicit) in the background thread (raster) when we are not suppose to.

Do you know what causes the layer tree being modified? Is it because in Add-to-app, a non-flutter UIView is changed while the Flutter rasterizer is also running?

[houhuayong]

I don't understand the full problem here, it looks like CA_ASSERT_MAIN_THREAD_TRANSACTIONS is a debugging flag, which indicates we were adding transactions (implicit) in the background thread (raster) when we are not suppose to.

Do you know what causes the layer tree being modified? Is it because in Add-to-app, a non-flutter UIView is changed while the Flutter rasterizer is also running?

In my case , It's a flutter app not Add-to-app. I mentioned Add-to-app just because I can set CA_ASSERT_MAIN_THREAD_TRANSACTIONS = 0 in flutter app to avoid this problem, but in Add-to-app I can't do that.

From what I know flutter will set layer.presentsWithTransaction in ios_surface_metal.mm, and the value depend on current running thread. when platform thread and raster thread merged it will be YES, otherwise NO. It seems when the value changed from YES to NO will make layer tree modified. And implicit transaction is created, at this time it is on background thread (raster).

[cyanglaz]

It seems wrong that just by changing presentsWithTransaction from YES to NO sets the layer tree as modified and automatically triggers an implicit transaction.

Maybe @chinmaygarde knows more about this.

Though, I have been seeing some issue with threads unmerging. Sometimes, when threads are unmerging, a frame on the platform thread hasn't finished before a new frame is created on the raster thread, which hits some DChecks in the engine. It is only reproducible in local engine and flaky. It might be related. (I'm trying to find a more consistent way to reproduce it and document the issue)

[houhuayong]

Maybe this issue is different with what you have met before. I have tried a native demo without flutter by create
CAMetalLayer which presentsWithTransaction set to YES. And I change the property to NO after a while, It triggers an implicit transaction too. If it has no explicit transaction wrapped, It behavior same as my app case.

[chinmaygarde]

@cyanglaz I doing think I am the right person to review this. Are you able to reason about this?

[cyanglaz]

@cyanglaz I doing think I am the right person to review this. Are you able to reason about this?

Sure! I can test it out.

@houhuayong I remember we had some issue with CATransactions in the background thread: flutter/flutter#66647, we fixed it by only adding explicit CATransactions on the Platform Thread. #21526

I think the change in this PR will be a regression for flutter/flutter#66647. Can we find a different way to fix it? Or even better, maybe our diagnostic of flutter/flutter#66647 wasn't accurate and there is a way to avoid the crash while having explicit transactions on the background thread?

[chinmaygarde]

From @cyanglaz s last comment it seems like we should not land this patch. Is that right?

[cyanglaz]

From @cyanglaz s last comment it seems like we should not land this patch. Is that right?

Correct.

@houhuayong I think we should find a different way to support this. I'm going to close this PR. Please feel empowered to open a new PR if you know a different fix.