Smooth window resizing on macOS #21525
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| #import <Cocoa/Cocoa.h> | ||
|
|
||
| @class FlutterResizeSynchronizer; | ||
|
|
||
| @protocol FlutterResizeSynchronizerDelegate | ||
|
There was a problem hiding this comment. All protocols should have declaration comments explaining their role, per style guide. |
||
|
|
||
| // Invoked on raster thread; Delegate should flush the OpenGL context | ||
|
There was a problem hiding this comment. All of these comments need a |
||
| - (void)resizeSynchronizerFlush:(FlutterResizeSynchronizer*)synchronizer; | ||
|
There was a problem hiding this comment. All of the macOS embedding's headers should have nullability annotations. |
||
|
|
||
| // Invoked on platform thread; Delegate should flip the surfaces | ||
| - (void)resizeSynchronizerCommit:(FlutterResizeSynchronizer*)synchronizer; | ||
|
|
||
| @end | ||
|
|
||
| // Encapsulates the logic for blocking platform thread during window resize as | ||
| // well as synchronizing the raster and platform thread during commit (presenting frame) | ||
| // | ||
| // Flow during window resize | ||
| // | ||
| // 1. Platform thread calls [synchronizer beginResize:notify:] | ||
| // This will hold the platform thread until we're ready to display contents. | ||
| // 2. Raster thread calls [synchronizer shouldEnsureSurfaceForSize:] with target size | ||
| // This will return false for any size other than target size | ||
| // 3. Raster thread calls [synchronizer requestCommit] | ||
| // Any commit calls before shouldEnsureSurfaceForSize: is called with the right | ||
| // size are simply ignored; There's no point rasterizing and displaying frames | ||
| // with wrong size. | ||
| // Both delegate methods (flush/commit) will be invoked before beginResize returns | ||
| // | ||
| // Flow during regular operation (no resizing) | ||
| // | ||
| // 1. Raster thread calls [synchronizer requestCommit] | ||
| // This will invoke [delegate flush:] on raster thread and | ||
| // [delegate commit:] on platform thread. The requestCommit call will be blocked | ||
| // until this is done. This is necessary to ensure that rasterizer won't start | ||
| // rasterizing next frame before we flipped the surface, which must be performed | ||
| // on platform thread | ||
| @interface FlutterResizeSynchronizer : NSObject | ||
|
|
||
| - (instancetype)initWithDelegate:(id<FlutterResizeSynchronizerDelegate>)delegate; | ||
|
|
||
| // Blocks the platform thread until | ||
| // - shouldEnsureSurfaceForSize is called with proper size and | ||
| // - requestCommit is called | ||
| // All requestCommit calls before `shouldEnsureSurfaceForSize` is called with | ||
| // expected size are ignored; | ||
| // The notify block is invoked immediately after synchronizer mutex is acquired | ||
| - (void)beginResize:(CGSize)size notify:(dispatch_block_t)notify; | ||
|
|
||
| // Returns whether the view should ensure surfaces with given size; | ||
| // This will be false during resizing for any size other than size specified | ||
| // during beginResize | ||
| - (bool)shouldEnsureSurfaceForSize:(CGSize)size; | ||
|
|
||
| // Called from rasterizer thread, will block until delegate resizeSynchronizerCommit: | ||
| // method is called (on platform thread) | ||
| - (void)requestCommit; | ||
|
|
||
| @end | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| #import "flutter/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h" | ||
|
|
||
| #import <mutex> | ||
|
|
||
|
|
||
| @interface FlutterResizeSynchronizer () { | ||
| uint32_t cookie; // counter to detect stale callbacks | ||
|
There was a problem hiding this comment. None of these ivars are following the style guide's naming requirements. They all need to start with There was a problem hiding this comment. Please follow the style used in all the rest of the macOS code and put comments on the line before the ivar, not after it. |
||
|
|
||
| std::mutex mutex; | ||
| std::condition_variable condBlockBeginResize; // used to block [beginResize:] | ||
| std::condition_variable condBlockRequestCommit; // used to block [requestCommit] | ||
|
|
||
| bool acceptingCommit; // if false, requestCommit calls are ignored until | ||
|
There was a problem hiding this comment. Please use (And please remember to change There was a problem hiding this comment. Will do. This was a c++ class in the first PR, I guess bool is a leftover. |
||
| // shouldEnsureSurfaceForSize is called with proper size | ||
| bool waiting; // waiting for resize to finish | ||
| bool pendingCommit; // requestCommit was called and [delegate commit:] must be performed on | ||
| // platform thread | ||
| CGSize newSize; // target size for resizing | ||
|
|
||
| __weak id<FlutterResizeSynchronizerDelegate> delegate; | ||
| } | ||
| @end | ||
|
|
||
| @implementation FlutterResizeSynchronizer | ||
|
|
||
| - (instancetype)initWithDelegate:(id<FlutterResizeSynchronizerDelegate>)delegate_ { | ||
|
There was a problem hiding this comment. A parameter name should not end with an (I assume this was done to distinguish from the ivar, but the correct way to handle that is to name the ivar according to the style guide.) |
||
| if (self = [super init]) { | ||
iskakaushik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| acceptingCommit = true; | ||
| delegate = delegate_; | ||
| } | ||
| return self; | ||
| } | ||
|
|
||
| - (void)beginResize:(CGSize)size notify:(dispatch_block_t)notify { | ||
| std::unique_lock<std::mutex> lock(mutex); | ||
| if (!delegate) { | ||
| return; | ||
| } | ||
|
|
||
| ++cookie; | ||
|
|
||
| // from now on, ignore all incoming commits until the block below gets | ||
| // scheduled on raster thread | ||
| acceptingCommit = false; | ||
|
|
||
| // let pending commits finish to unblock the raster thread | ||
| pendingCommit = false; | ||
| condBlockBeginResize.notify_all(); | ||
|
|
||
| // let the engine send resize notification | ||
| notify(); | ||
|
|
||
| newSize = size; | ||
|
|
||
| waiting = true; | ||
|
|
||
| condBlockRequestCommit.wait(lock, [&] { return pendingCommit; }); | ||
|
|
||
| [delegate resizeSynchronizerFlush:self]; | ||
| [delegate resizeSynchronizerCommit:self]; | ||
| pendingCommit = false; | ||
| condBlockBeginResize.notify_all(); | ||
|
|
||
| waiting = false; | ||
| } | ||
|
|
||
| - (bool)shouldEnsureSurfaceForSize:(CGSize)size { | ||
| std::unique_lock<std::mutex> lock(mutex); | ||
| if (!acceptingCommit) { | ||
| if (CGSizeEqualToSize(newSize, size)) { | ||
| acceptingCommit = true; | ||
| } | ||
| } | ||
| return acceptingCommit; | ||
| } | ||
|
|
||
| - (void)requestCommit { | ||
| std::unique_lock<std::mutex> lock(mutex); | ||
| if (!acceptingCommit) { | ||
| return; | ||
| } | ||
|
|
||
| pendingCommit = true; | ||
| if (waiting) { // BeginResize is in progress, interrupt it and schedule commit call | ||
| condBlockRequestCommit.notify_all(); | ||
| condBlockBeginResize.wait(lock, [&]() { return !pendingCommit; }); | ||
| } 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] { | ||
| std::unique_lock<std::mutex> lock(mutex); | ||
| if (cookie_ == cookie) { | ||
| if (delegate) { | ||
| [delegate resizeSynchronizerCommit:self]; | ||
| } | ||
| pendingCommit = false; | ||
| condBlockBeginResize.notify_all(); | ||
| } | ||
| }); | ||
| condBlockBeginResize.wait(lock, [&]() { return !pendingCommit; }); | ||
| } | ||
| } | ||
|
|
||
| @end | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| #import <Cocoa/Cocoa.h> | ||
|
|
||
| // Manages the IOSurfaces for FlutterView | ||
| @interface FlutterSurfaceManager : NSObject | ||
|
|
||
| - (instancetype)initWithLayer:(CALayer*)layer openGLContext:(NSOpenGLContext*)opengLContext; | ||
|
|
||
| - (void)ensureSurfaceSize:(CGSize)size; | ||
| - (void)swapBuffers; | ||
|
|
||
| - (uint32_t)glFrameBufferId; | ||
|
|
||
| @end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update this to follow the format used in all of the other functions, where it forwards directly, with no logic, to an engine method? That structure was intentional.