[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
|
|
||
| // 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
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Sending out the super tiny nitpicks while I review the changes to FlutterPlatformViews*.
|
|
||
| // 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
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Second round of comments. Reviewing the tests, then I'll be done.
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
| // 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
cbracken
left a comment
cbracken
left a comment
There was a problem hiding this comment.
Just a few more small comments. Other than that:
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.
hellohuanlin
left a comment
hellohuanlin
left a 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.
| [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
| 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
|
|
||
| // 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!
6 participants
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