[iOS] Supported rendering platform views without merging the raster thread. #53826
Conversation
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Outdated
Show resolved
Hide resolved
| return nullptr; | ||
| } | ||
|
|
||
| void FlutterPlatformViewLayerPool::CreateLayer(GrDirectContext* gr_context, |
There was a problem hiding this comment.
This is probably the biggest PITA. Because I can only construct these on the platform thread, right now frame 0 with platform views will skip some number of overlay layers. Its possible that the FlutterMetalLayer solves this partially
There was a problem hiding this comment.
If we move away from CAMetalLayer (or even FlutterMetalLayer) and adopt custom composition, we can solve this easily because you can allocate IOSurfaces on any thread, when they are not tied to particular CALayer. We can also have much cheaper overlays. Right now we use up to 2 CAMetaLayers for each FlutterView overlay, which is 6 full screen surfaces. With custom composition it is possible to set a single iOSurface as content of as many CALayers as needed each showing different part, so you can many unobstructed platform view overlays with single layer.
On macOS surfaces are allocated on raster thread and the overlay sublayers are crated here.
Here are relevant files:
flow/compositor_context.cc
Outdated
| if (canvas() && clip_rect) { | ||
| canvas()->Translate(-clip_rect->x(), -clip_rect->y()); | ||
| } | ||
| // if (canvas() && clip_rect) { |
There was a problem hiding this comment.
Need to figure out why this was firing. Possibly we're turning off partial repaint based on thread merging when it should be based on platform view state.
There was a problem hiding this comment.
We need better way to disable partial repaint. I think instead of
bool force_full_repaint =
external_view_embedder_ &&
(!raster_thread_merger_ || raster_thread_merger_->IsMerged());we should reset damage and clipRect inside CompositorContext::ScopedFrame::Raster after Preroll when we know that there are platform view present.
There was a problem hiding this comment.
That would be after we preroll with damage based cull rect though?
There was a problem hiding this comment.
Yeah, fair point. Though I'm not 100% sure where the cullrect is used during preroll, the actual culling is done through quickReject during Paint. I think long time ago it was done in preroll setting a flag on layer.
Maybe better to mark entire frame dirty in PlatformViewLayer::Diff. Just need to make sure it behaves correctly when diff is skipped for retained layers. I'll look into this as a separate PR.
shell/platform/android/external_view_embedder/external_view_embedder.cc
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Outdated
Show resolved
Hide resolved
| @@ -685,8 +638,11 @@ static bool ClipRRectContainsPlatformViewBoundingRect(const SkRRect& clip_rrect, | |||
| auto did_submit = true; | |||
| auto num_platform_views = composition_order_.size(); | |||
|
|
|||
| // TODO(hellohuanlin) this double for-loop is expensive with wasted computations. | |||
| // See: https://github.com/flutter/flutter/issues/145802 | |||
| size_t missing_layer_count = 0; | |||
There was a problem hiding this comment.
With this design, if we're missing an overlay layer then it won't render until the next frame. I think we can handle that by forcing resubmission of the frame but I'll have to follow up on it.
shell/platform/darwin/ios/framework/Source/FlutterPlatformViewsTest.mm
Outdated
Show resolved
Hide resolved
shell/platform/android/external_view_embedder/external_view_embedder.h
Outdated
Show resolved
Hide resolved
| @@ -286,7 +286,7 @@ - (void)flutterPrepareForPresent:(nonnull id<MTLCommandBuffer>)commandBuffer; | |||
| // If the threads have been merged, or there is a pending frame capture, | |||
| // then block on cmd buffer scheduling to ensure that the | |||
| // transaction/capture work correctly. | |||
There was a problem hiding this comment.
The above comment isn't adding much and rather than making it even longer to cover the new case, we could probably just yank out a local and the code would do the exact same amount of documenting as the comment currently does:
BOOL threads_merged = [[NSThread currentThread] isMainThread];Or maybe extract the whole blobby conditional to a bool named transactions_enabled or something.
There was a problem hiding this comment.
Maybe I can move this to the surface_frame doc comment?
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Outdated
Show resolved
Hide resolved
|
|
||
| // If a layer was allocated in the previous frame, but it's not used in the current frame, | ||
| // then it can be removed from the scene. | ||
| RemoveUnusedLayers(unused_layers, composition_order, active_composition_order); | ||
|
|
||
| // If the frame is submitted with embedded platform views, |
There was a problem hiding this comment.
Yep -- I'd be tempted to just delete the whole comment.
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews.mm
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h
Outdated
Show resolved
Hide resolved
shell/platform/darwin/ios/framework/Source/FlutterPlatformViews_Internal.h
Outdated
Show resolved
Hide resolved
| // is a non-Flutter UIView active, such as in add2app or a | ||
| // presentViewController page transition, then this will cause CoreAnimation | ||
| // assertion errors and exit the app. | ||
| bool present_with_transaction = false; |
There was a problem hiding this comment.
I assume this is necessary for things to correctly work with FlutterMetalLayer disabled?
There was a problem hiding this comment.
FlutterMetalLayer is always presenting with a CATransaction, right?
There was a problem hiding this comment.
Yes, it always makes the jump to platform thread (can't set CALayer content on background thread).
There was a problem hiding this comment.
Makes sense. I ported this over from the existing logic, but once we remove the old metal layer usage we could remove this as well.
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm not familiar with this part of the code, so more for me to ask and learn.
My suggestion: It'd be very helpful to read if we have a comment on each function about which thread it runs on. Otherwise I would have to jump back and forth in the code to figure it out.
| // to run on the platform thread after no platform view is rendered. | ||
| // | ||
| // Note: this is an arbitrary number. | ||
| static const int kDefaultMergedLeaseDuration = 10; |
There was a problem hiding this comment.
ask to learn - are we still merging the thread (given this variable name?)
There was a problem hiding this comment.
We are merging threads on the iOS simulator (technically we only need to when using the software backend but that is a harder condition to test). Once we remove skia and the software backend we can remove this condition.
There was a problem hiding this comment.
May I ask what's the difference between the simulator and device that we have to merge the thread on simulator?
There was a problem hiding this comment.
its not the simulator, its the software backend. The software backend surfaces cannot be used from multiple threads, which this change requires.
| ChildClippingView* clipping_view = [[ChildClippingView alloc] initWithFrame:CGRectZero]; | ||
| [clipping_view addSubview:touch_interceptor]; | ||
| root_views_[viewId] = fml::scoped_nsobject<UIView>(clipping_view); | ||
|
|
||
| platform_views_.emplace( |
There was a problem hiding this comment.
nice clean up with the struct
| slices_.clear(); | ||
| current_composition_params_.clear(); | ||
| clip_count_.clear(); | ||
| views_to_recomposite_.clear(); | ||
| layer_pool_->RecycleLayers(); | ||
| visited_platform_views_.clear(); |
There was a problem hiding this comment.
Are these variables ever mutated on main thread as well? Or only on raster thread?
There was a problem hiding this comment.
Only on the raster thread
There was a problem hiding this comment.
Gotcha. Are all of the embedder callback APIs (e.g. SubmitFrame etc) now run on raster thread?
There was a problem hiding this comment.
Thanks! I think it's indeed a simpler (and better) design
| size_t required_overlay_layers) { | ||
| TRACE_EVENT0("flutter", "FlutterPlatformViewsController::CreateMissingLayers"); | ||
|
|
||
| if (required_overlay_layers <= layer_pool_->size()) { |
There was a problem hiding this comment.
it looks like the layer_pool is accessed here on raster thread, but mutated below in CreateLayer on main thread (if im reading it right). I'm a bit concerned about race condition here
There was a problem hiding this comment.
Because we block the raster thread on completion via a latch, there is no race
|
Added some more comments describing the intended thread and removed some more dead code. |
jonahwilliams
added
the
autosubmit
| void Reset(); | ||
|
|
||
| // Encode rendering for the Flutter overlay views and queue up perform platform view mutations. | ||
| // | ||
| // Runs on the raster thread. |
There was a problem hiding this comment.
Thanks for adding these!
…152707) flutter/engine@17e3c7d...1cbe88e 2024-08-01 jonahwilliams@google.com [iOS] Supported rendering platform views without merging the raster thread. (flutter/engine#53826) 2024-08-01 jhy03261997@gmail.com Set deep linking flag to true by default (flutter/engine#52350) 2024-08-01 leroux_bruno@yahoo.fr [Android] Revert "Reset IME state on clear text input client" (flutter/engine#54277) 2024-08-01 skia-flutter-autoroll@skia.org Roll Skia from 03732b9f885e to ddb6901e6141 (5 revisions) (flutter/engine#54287) 2024-08-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 5acd806b6dad to acbbbe73b5eb (1 revision) (flutter/engine#54286) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#152707) flutter/engine@17e3c7d...1cbe88e 2024-08-01 jonahwilliams@google.com [iOS] Supported rendering platform views without merging the raster thread. (flutter/engine#53826) 2024-08-01 jhy03261997@gmail.com Set deep linking flag to true by default (flutter/engine#52350) 2024-08-01 leroux_bruno@yahoo.fr [Android] Revert "Reset IME state on clear text input client" (flutter/engine#54277) 2024-08-01 skia-flutter-autoroll@skia.org Roll Skia from 03732b9f885e to ddb6901e6141 (5 revisions) (flutter/engine#54287) 2024-08-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 5acd806b6dad to acbbbe73b5eb (1 revision) (flutter/engine#54286) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…lutter#152707) flutter/engine@17e3c7d...1cbe88e 2024-08-01 jonahwilliams@google.com [iOS] Supported rendering platform views without merging the raster thread. (flutter/engine#53826) 2024-08-01 jhy03261997@gmail.com Set deep linking flag to true by default (flutter/engine#52350) 2024-08-01 leroux_bruno@yahoo.fr [Android] Revert "Reset IME state on clear text input client" (flutter/engine#54277) 2024-08-01 skia-flutter-autoroll@skia.org Roll Skia from 03732b9f885e to ddb6901e6141 (5 revisions) (flutter/engine#54287) 2024-08-01 skia-flutter-autoroll@skia.org Roll Dart SDK from 5acd806b6dad to acbbbe73b5eb (1 revision) (flutter/engine#54286) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jacksongardner@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…4820) It turns out that when there are multiple paths to clip, they are unioned together, rather than intersected. But clipping paths need to be intersected. There isn't any good way to intersect arbitrary paths. However, it is easy to intersect rect paths, which is the most common use case. Then we simply fallback to software rendering if we have to intersect non-rect paths. That is: **Case 1** Only 1 clipping path (either rect path or arbitrary path): Hardware rendering. This should be the most common use case **Case 2** Multiple rect clipping path: Hardware rendering. This is also common, and it's the linked issue that we are fixing. **Case 3** Other complex case (multiple non-rect clipping path) Fallback to software rendering. This should be rare. After #53826, we don't have a working benchmark that measures the main thread anymore. However, this PR shouldn't impact our ad benchmark, since it only has 1 clipping path. I will verify manually by checking Instruments and make sure no software rendering is happening. But we really should make the benchmark working again, not just for performance improvement, but also for monitoring regression. *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#153904 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
…utter#54820) It turns out that when there are multiple paths to clip, they are unioned together, rather than intersected. But clipping paths need to be intersected. There isn't any good way to intersect arbitrary paths. However, it is easy to intersect rect paths, which is the most common use case. Then we simply fallback to software rendering if we have to intersect non-rect paths. That is: **Case 1** Only 1 clipping path (either rect path or arbitrary path): Hardware rendering. This should be the most common use case **Case 2** Multiple rect clipping path: Hardware rendering. This is also common, and it's the linked issue that we are fixing. **Case 3** Other complex case (multiple non-rect clipping path) Fallback to software rendering. This should be rare. After flutter#53826, we don't have a working benchmark that measures the main thread anymore. However, this PR shouldn't impact our ad benchmark, since it only has 1 clipping path. I will verify manually by checking Instruments and make sure no software rendering is happening. But we really should make the benchmark working again, not just for performance improvement, but also for monitoring regression. *List which issues are fixed by this PR. You must list at least one issue.* Fixes flutter/flutter#153904 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Remove the need to merge raster and platform thread in the presence of platform views by defering UIView mutation and presentation of flutter views into separate platform thread task. Fixes priority inversion problem cause by platform thread blocking on drawable aquisition.
Open questions:
What is a better interface for handling the partial submit with impeller. (TBD)Update: We Don't | How do we fix this for SkiaFixedUpdate: Done, we post a task to the platform thread. Is there a shorter term solution for creating overlay layers on the raster thread.FixedUpdate: seems to. Does this perform well enough (independent of platform/ui thread merge and w/ thread merge).FixedFixes flutter/flutter#142841
part of flutter/flutter#150525