From 78e30202ca9a6ee7c04386ea8e45a702ff8d3344 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Thu, 10 Nov 2022 13:55:13 +0100 Subject: [PATCH] macOS: Fix platform view threading issues --- .../framework/Source/FlutterCompositor.h | 12 +++++-- .../framework/Source/FlutterCompositor.mm | 32 +++++++++++++++---- .../framework/Source/FlutterCompositorTest.mm | 2 +- .../macos/framework/Source/FlutterEngine.mm | 25 ++++++++------- .../framework/Source/FlutterEngineTest.mm | 2 +- .../macos/framework/Source/FlutterRenderer.h | 2 +- .../macos/framework/Source/FlutterRenderer.mm | 6 ++-- .../framework/Source/FlutterRendererTest.mm | 4 +-- .../Source/FlutterResizeSynchronizer.h | 9 ++++++ .../Source/FlutterResizeSynchronizer.mm | 25 ++++++++++++++- .../macos/framework/Source/FlutterView.h | 8 +++-- .../macos/framework/Source/FlutterView.mm | 4 +-- 12 files changed, 96 insertions(+), 35 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h b/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h index 66cf7b8939d37..e9df38d07b6b3 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterCompositor.h @@ -60,7 +60,9 @@ class FlutterCompositor { // Callback triggered at the end of the Present function. has_flutter_content // is true when Flutter content was rendered, otherwise false. - using PresentCallback = std::function; + using PresentCallback = + std::function; // Registers a callback to be triggered at the end of the Present function. // If a callback was previously registered, it will be replaced. @@ -85,18 +87,22 @@ class FlutterCompositor { void SetFrameStatus(FrameStatus frame_status); FrameStatus GetFrameStatus(); - // Clears the previous CALayers and updates the frame status to frame started. + // Sets frame status to frame started. void StartFrame(); + // Clears the previous CALayers. + void RemoveOldLayers(); + // Calls the present callback and ensures the frame status is updated // to frame ended, returning whether the present was successful or not. - bool EndFrame(bool has_flutter_content); + bool EndFrame(bool has_flutter_content, dispatch_block_t on_notify); // Creates a CALayer object which is backed by the supplied IOSurface, and // adds it to the root CALayer for the given view. void InsertCALayerForIOSurface( FlutterView* view, const IOSurfaceRef& io_surface, + size_t layer_position, CATransform3D transform = CATransform3DIdentity); private: diff --git a/shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm b/shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm index ccdfaec180c2a..6693eaa3abe30 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterCompositor.mm @@ -90,6 +90,12 @@ SetFrameStatus(FrameStatus::kPresenting); bool has_flutter_content = false; + + std::vector actions; + actions.push_back(^{ + RemoveOldLayers(); + }); + for (size_t i = 0; i < layers_count; ++i) { const auto* layer = layers[i]; FlutterBackingStore* backing_store = const_cast(layer->backing_store); @@ -100,19 +106,27 @@ FlutterIOSurfaceHolder* io_surface_holder = (__bridge FlutterIOSurfaceHolder*)backing_store->metal.texture.user_data; IOSurfaceRef io_surface = [io_surface_holder ioSurface]; - InsertCALayerForIOSurface(view, io_surface); + actions.push_back(^{ + InsertCALayerForIOSurface(view, io_surface, i); + }); } has_flutter_content = true; break; } case kFlutterLayerContentTypePlatformView: { - PresentPlatformView(view, layer, i); + actions.push_back(^{ + PresentPlatformView(view, layer, i); + }); break; } }; } - return EndFrame(has_flutter_content); + return EndFrame(has_flutter_content, ^{ + for (auto& action : actions) { + action(); + } + }); } void FlutterCompositor::PresentPlatformView(FlutterView* default_base_view, @@ -143,6 +157,10 @@ } void FlutterCompositor::StartFrame() { + SetFrameStatus(FrameStatus::kStarted); +} + +void FlutterCompositor::RemoveOldLayers() { // First remove all CALayers from the superlayer. for (auto layer : active_ca_layers_) { [layer removeFromSuperlayer]; @@ -150,11 +168,10 @@ // Reset active layers. active_ca_layers_.clear(); - SetFrameStatus(FrameStatus::kStarted); } -bool FlutterCompositor::EndFrame(bool has_flutter_content) { - bool status = present_callback_(has_flutter_content); +bool FlutterCompositor::EndFrame(bool has_flutter_content, dispatch_block_t on_notify) { + bool status = present_callback_(has_flutter_content, on_notify); SetFrameStatus(FrameStatus::kEnded); return status; } @@ -173,6 +190,7 @@ void FlutterCompositor::InsertCALayerForIOSurface(FlutterView* view, const IOSurfaceRef& io_surface, + size_t layer_position, CATransform3D transform) { // FlutterCompositor manages the lifecycle of CALayers. CALayer* content_layer = [[CALayer alloc] init]; @@ -180,7 +198,7 @@ content_layer.frame = view.layer.bounds; [content_layer setContents:(__bridge id)io_surface]; [view.layer addSublayer:content_layer]; - + content_layer.zPosition = layer_position; active_ca_layers_.push_back(content_layer); } diff --git a/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm index 576af2bbac152..9a2a4e8a0836e 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterCompositorTest.mm @@ -67,7 +67,7 @@ - (nullable FlutterView*)getView:(uint64_t)viewId { /*mtl_device*/ nullptr); bool flag = false; - macos_compositor->SetPresentCallback([f = &flag](bool has_flutter_content) { + macos_compositor->SetPresentCallback([f = &flag](bool has_flutter_content, dispatch_block_t) { *f = true; return true; }); diff --git a/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm b/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm index 47f9ca39aed73..c79a6dba0a614 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterEngine.mm @@ -423,18 +423,19 @@ - (FlutterCompositor*)createFlutterCompositor { _macOSCompositor = std::make_unique( _viewProvider, _platformViewController, _renderer.device); - _macOSCompositor->SetPresentCallback([weakSelf](bool has_flutter_content) { - // TODO(dkwingsmt): The compositor only supports single-view for now. As - // more classes are gradually converted to multi-view, it should get the - // view ID from somewhere. - uint64_t viewId = kFlutterDefaultViewId; - if (has_flutter_content) { - return [weakSelf.renderer present:viewId] == YES; - } else { - [weakSelf.renderer presentWithoutContent:viewId]; - return true; - } - }); + _macOSCompositor->SetPresentCallback( + [weakSelf](bool has_flutter_content, dispatch_block_t on_notify) { + // TODO(dkwingsmt): The compositor only supports single-view for now. As + // more classes are gradually converted to multi-view, it should get the + // view ID from somewhere. + uint64_t viewId = kFlutterDefaultViewId; + if (has_flutter_content) { + return [weakSelf.renderer present:viewId withBlock:on_notify] == YES; + } else { + [weakSelf.renderer presentWithoutContent:viewId]; + return true; + } + }); _compositor = {}; _compositor.struct_size = sizeof(FlutterCompositor); diff --git a/shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm index e6a96d9adf275..904cb7fd0cbcf 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterEngineTest.mm @@ -465,7 +465,7 @@ @interface FlutterEngine (Test) // Latch to ensure the entire layer tree has been generated and presented. fml::AutoResetWaitableEvent latch; auto compositor = engine.macOSCompositor; - compositor->SetPresentCallback([&](bool has_flutter_content) { + compositor->SetPresentCallback([&](bool has_flutter_content, dispatch_block_t completion) { latch.Signal(); return true; }); diff --git a/shell/platform/darwin/macos/framework/Source/FlutterRenderer.h b/shell/platform/darwin/macos/framework/Source/FlutterRenderer.h index d9cbe1da5abb0..82c7e88b752f4 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterRenderer.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterRenderer.h @@ -38,7 +38,7 @@ /** * Called by the engine when the given view's buffers should be swapped. */ -- (BOOL)present:(uint64_t)viewId; +- (BOOL)present:(uint64_t)viewId withBlock:(nullable dispatch_block_t)block; /** * Tells the renderer that there is no Flutter content available for this frame. diff --git a/shell/platform/darwin/macos/framework/Source/FlutterRenderer.mm b/shell/platform/darwin/macos/framework/Source/FlutterRenderer.mm index 996e613dba926..ecf083fd0965b 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterRenderer.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterRenderer.mm @@ -28,7 +28,7 @@ static bool OnPresentDrawableOfDefaultView(FlutterEngine* engine, // operates on the default view. To support multi-view, we need a new callback // that also receives a view ID. uint64_t viewId = kFlutterDefaultViewId; - return [engine.renderer present:viewId]; + return [engine.renderer present:viewId withBlock:nil]; } static bool OnAcquireExternalTexture(FlutterEngine* engine, @@ -104,12 +104,12 @@ - (FlutterMetalTexture)createTextureForView:(uint64_t)viewId size:(CGSize)size { return embedderTexture; } -- (BOOL)present:(uint64_t)viewId { +- (BOOL)present:(uint64_t)viewId withBlock:(dispatch_block_t)block { FlutterView* view = [_viewProvider getView:viewId]; if (view == nil) { return NO; } - [view present]; + [view presentWithBlock:block]; return YES; } diff --git a/shell/platform/darwin/macos/framework/Source/FlutterRendererTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterRendererTest.mm index 6cd57e33d48f9..fcf534ae9a9c6 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterRendererTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterRendererTest.mm @@ -39,8 +39,8 @@ void SetEngineDefaultView(FlutterEngine* engine, id flutterView) { FlutterRenderer* renderer = [[FlutterRenderer alloc] initWithFlutterEngine:engine]; id mockFlutterView = OCMClassMock([FlutterView class]); SetEngineDefaultView(engine, mockFlutterView); - [(FlutterView*)[mockFlutterView expect] present]; - [renderer present:kFlutterDefaultViewId]; + [(FlutterView*)[mockFlutterView expect] presentWithBlock:nil]; + [renderer present:kFlutterDefaultViewId withBlock:nil]; } TEST(FlutterRenderer, TextureReturnedByFlutterView) { diff --git a/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h b/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h index 556856ed3684d..b3211fdd16278 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h @@ -75,6 +75,15 @@ */ - (void)requestCommit; +/** + * Called from rasterizer thread, will block until delegate resizeSynchronizerCommit: + * method is called (on platform thread). + * + * The onCommit block will be invoked on platform thread during the core + * animation transaction. + */ +- (void)requestCommitWithBlock:(nullable dispatch_block_t)onCommit; + /** * Called from view to notify the synchronizer that there are no Flutter frames * coming. Synchronizer must unblock main thread and not block until another diff --git a/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm b/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm index 7681aa3385779..3d77998da39ee 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm @@ -4,6 +4,7 @@ #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h" +#import #include @interface FlutterResizeSynchronizer () { @@ -38,6 +39,8 @@ @interface FlutterResizeSynchronizer () { BOOL _shuttingDown; __weak id _delegate; + + dispatch_block_t _onCommit; } @end @@ -83,7 +86,16 @@ - (void)beginResize:(CGSize)size notify:(dispatch_block_t)notify { _condBlockRequestCommit.wait(lock, [&] { return _pendingCommit || _shuttingDown; }); [_delegate resizeSynchronizerFlush:self]; + + [CATransaction begin]; + [CATransaction setDisableActions:YES]; [_delegate resizeSynchronizerCommit:self]; + if (_onCommit) { + _onCommit(); + } + _onCommit = nil; + [CATransaction commit]; + _pendingCommit = NO; _condBlockBeginResize.notify_all(); @@ -106,6 +118,10 @@ - (BOOL)shouldEnsureSurfaceForSize:(CGSize)size { } - (void)requestCommit { + [self requestCommitWithBlock:nil]; +} + +- (void)requestCommitWithBlock:(dispatch_block_t)onCommit { std::unique_lock lock(_mutex); if (!_acceptingCommit || _shuttingDown) { @@ -116,18 +132,25 @@ - (void)requestCommit { _pendingCommit = YES; if (_waiting) { // BeginResize is in progress, interrupt it and schedule commit call + _onCommit = onCommit; _condBlockRequestCommit.notify_all(); _condBlockBeginResize.wait(lock, [&]() { return !_pendingCommit || _shuttingDown; }); } else { // No resize, schedule commit on platform thread and wait until either done // or interrupted by incoming BeginResize [_delegate resizeSynchronizerFlush:self]; - dispatch_async(dispatch_get_main_queue(), [self, cookie = _cookie] { + dispatch_async(dispatch_get_main_queue(), [self, cookie = _cookie, onCommit] { std::unique_lock lock(_mutex); if (cookie == _cookie) { + [CATransaction begin]; + [CATransaction setDisableActions:YES]; if (_delegate) { [_delegate resizeSynchronizerCommit:self]; } + if (onCommit) { + onCommit(); + } + [CATransaction commit]; _pendingCommit = NO; _condBlockBeginResize.notify_all(); } diff --git a/shell/platform/darwin/macos/framework/Source/FlutterView.h b/shell/platform/darwin/macos/framework/Source/FlutterView.h index 2da2f48f34963..35d11867d85a9 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterView.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterView.h @@ -49,9 +49,13 @@ constexpr uint64_t kFlutterDefaultViewId = 0; - (nonnull instancetype)init NS_UNAVAILABLE; /** - * Flushes the graphics context and flips the surfaces. Expected to be called on raster thread. + * Flushes the OpenGL context and flips the surfaces. Expected to be called + * on raster thread. Blocks until the operation is complete. + * + * The onCommit block will be invoked on platform thread during the core + * animation transaction; */ -- (void)present; +- (void)presentWithBlock:(nullable dispatch_block_t)onCommit; /** * Called when there is no Flutter content available to render. This must be passed to resize diff --git a/shell/platform/darwin/macos/framework/Source/FlutterView.mm b/shell/platform/darwin/macos/framework/Source/FlutterView.mm index 8a92b9f5fbb59..b67777a3a528b 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterView.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterView.mm @@ -45,8 +45,8 @@ - (FlutterRenderBackingStore*)backingStoreForSize:(CGSize)size { return [_resizableBackingStoreProvider backingStore]; } -- (void)present { - [_resizeSynchronizer requestCommit]; +- (void)presentWithBlock:(dispatch_block_t)onCommit { + [_resizeSynchronizer requestCommitWithBlock:onCommit]; } - (void)presentWithoutContent {