Smooth window resizing on macOS #21525

knopp · 2020-09-30T20:58:20Z

Description

Replaces #21252

Make window resizing on macOS smooth, responsive and synchronous with windows size.

Related design doc: https://flutter.dev/go/desktop-resize-macos

Notable change: It seems that double buffered IOSurface backed NSOpenGLContext is not all that double buffered after all and I was able to create a test case which resulted in partial drawn frame visible. This version uses single buffered NSOpenGLContext with two IOSurfaces (front and back) which eliminates the problem.

Depends on: #21108

Related Issues

flutter/flutter#44136

Tests

I added the following tests:

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the contributor guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the C++, Objective-C, Java style guides for the engine.
I read the tree hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

flutter-dashboard · 2020-09-30T20:58:23Z

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.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

iskakaushik

Left some initial feedback, still need to look at FlutterResizeSynchronizer and the cond_var notification mechanism.

iskakaushik · 2020-09-30T23:30:53Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h

+
+@end
+
+// Encapsulates the logic for blocking platform thread during window resize


It will be document how the member functions of this interface need to be run. Some of this is mentioned in beginResize, but some questions that would likely need to be answered by the docs are:

What if beginResize gets called and only one of shouldEnsureSurfaceForSize or requestCommit is called? Should one of them be called before the other?

When the platform thread is blocked, is it blocked with a timeout? What happens when we exceed the said timeout?

beginResize seems to indicate that there is an endResize but instead we have requestCommit, should we call it endResize instead?

I'll document it in class comment

There is no timeout. No sure if there needs to be one. It seems that if you rely on timeout here, something must have gone wrong already. In which case it's probably better to fail early be locking up, than "sort-of work".

requestCommit is called out of resize as well. There is another thing FlutterResizeSynchronizer does - it ensures that delegate commit method is called on platform thread, while raster thread waits in requestCommit. It could be that the class needs more generic name, since it's not just resizing that it synchronizes.

Documented here.

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm

iskakaushik · 2020-10-01T16:18:55Z

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

+    glBindFramebuffer(GL_FRAMEBUFFER, _frameBufferId[0]);
+    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, _backingTexture[0]);
+    glTexParameterf(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+    glTexParameterf(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0);


Create a private method createTextureAndBindToFBO and re-use for both front and back buffers.

iskakaushik · 2020-10-01T16:20:54Z

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

+    layer = layer_;
+    openGLContext = opengLContext_;
+
+    NSOpenGLContext* prev = [NSOpenGLContext currentContext];


We are doing this pattern of

Save old context.

Make context current.

Reset old context or clear it.

This is being done at a couple of places in this class. This is very reminiscent of GLContextSwitch. Can we use that wrapper here or create an RAII wrapper?

Added MacOSSwitchableOpenGLContext.

iskakaushik · 2020-10-01T16:27:07Z

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

+    glBindFramebuffer(GL_FRAMEBUFFER, _frameBufferId[0]);
+    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, _backingTexture[0]);
+    glTexParameterf(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+    glTexParameterf(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MAG_FILTER, GL_NEAREST);


It might also be worth doing:

glTexParameteri(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); glTexParameteri(GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);

To avoid having the default which is GL_REPEAT.

I'm not sure this does anything give that we don't actually use these as textures anywhere, but I suppose it's not going to harm.

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

iskakaushik · 2020-10-01T16:30:24Z

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

+    glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0);
+
+    glBindFramebuffer(GL_FRAMEBUFFER, _frameBufferId[i]);
+    glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_RECTANGLE_ARB,


Check the status after this is done glCheckFramebufferStatus. Maybe even have an FML_CHECK to ensure it succeeds.

Good catch. Added.

iskakaushik · 2020-10-01T16:44:31Z

shell/platform/darwin/macos/framework/Source/FlutterView.mm

+  [resizeSynchronizer requestCommit];
+}
+
+- (void)start {


I'm still not sure if start is something we want outside of the init method. Are there situations where we will want to create the FlutterView and not start it right away?

The problem here is that we block the platform thread every time the view is resized, which is detected by overriding [NSView setFrameSize]. It is normal to get some calls to setFrameSize before the shell is running and producing frames. If we block the thread too soon, it will deadlock. So [FlutterView start] basically tells the view that the shell is ready and running and that if it block the platform thread, a frame will come.

So is this call more like: readyForSynchronousResizing?

Well, perhaps, but it also blocks the platform thread until it gets proper sized frame. I don't think that's necessary though. readyForSynchronousResizing sounds good.

I changed it here. [FlutterView start] is gone, instead there is synchronousResizing property, which when true means the view will block the platform thread during resizing. If false, resizing still works in a non blocking way.

iskakaushik · 2020-10-01T16:46:39Z

shell/platform/darwin/macos/framework/Source/FlutterView.mm

+  if (active) {
+    CGSize scaledSize = [self convertSizeToBacking:self.bounds.size];
+    [resizeSynchronizer beginResize:scaledSize
+                             notify:^{


Rather than notify what do you think of onResizeCommit?

Seems confusing. The notify block is basically invoked as soon as we get the mutex (and unblock raster thread). That's at the beginning of resizing. requestCommit (which triggers delegate's resizeSynchronizerCommit) is when raster thread wants to present frame.

So presenting frame is called commit here, not sure why starting the resizing process (which notify does) would also be called commit.

iskakaushik · 2020-10-01T16:47:56Z

shell/platform/darwin/macos/framework/Source/FlutterView.mm

+  [CATransaction begin];
+  [CATransaction setDisableActions:YES];
+  self.layer.frame = self.bounds;
+  self.layer.sublayerTransform = CATransform3DTranslate(CATransform3DMakeScale(1, -1, 1), 0,


Maybe add a comment explaining why this transform is needed?

I thought about this, and the transform is basically needed because we're rendering to an OpenGL texture, which are flipped. The fact, that we're rendering to an OpenGL texture / IOSurface is an implementation detail of FlutterSurfaceManager, and thus the surface manager should take care of flipping the layer, just like it takes care of settings layer contents. I updated the behavior here.

iskakaushik · 2020-10-01T16:57:41Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm

+
+  waiting = true;
+
+  condBlock.wait(lock);


Better to wait with a condition here. Maybe the condition here is []() { return pendingCommit; }.

Lets do it for all wait calls in this class.

The version before that was running the thread loop didn't have this problem. Definitely need to check for spurious wake-up.

Should be fixed here: d046e86

iskakaushik · 2020-10-01T16:58:49Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm

+
+  condBlock.wait(lock);
+
+  if (pendingCommit) {


This shouldn't ever be false.

iskakaushik · 2020-10-01T17:07:14Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm

+@interface FlutterResizeSynchronizer () {
+  uint32_t cookie;  // counter to detect stale callbacks
+  std::mutex mutex;
+  std::condition_variable condRun;


What about renaming:

condBlock -> pendingCommits

condRun -> noPendingCommits

shell/platform/darwin/macos/framework/Source/FlutterView.mm

knopp · 2020-10-01T18:00:43Z

I added another commit that separates flush and commit. There's no reason to flush the OpenGL context on platform tread, so this commit moves it to raster thread.

iskakaushik · 2020-10-01T18:26:48Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm

+  if (waiting) {  // BeginResize is in progress, interrupt it and schedule commit call
+    pendingCommit = true;
+    condPendingCommit.notify_all();
+    condNoPendingCommit.wait(lock, [&]() { return !pendingCommit; });


move this outside of the if {} else {} block given that we always have to wait for pending commits to be done in requestCommit.

iskakaushik

LGTM modulo the some minor comments.

iskakaushik · 2020-10-23T17:12:06Z

shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm

+- (void)swapBuffers {
+  contentLayer.frame = layer.bounds;
+
+  contentLayer.transform = CATransform3DMakeScale(1, -1, 1);


Add a comment here for why we need this transform

iskakaushik · 2020-10-23T17:30:37Z

shell/platform/darwin/macos/framework/Source/FlutterView.mm

+- (void)resizeSynchronizerCommit:(FlutterResizeSynchronizer*)synchronizer {
+  [CATransaction begin];
+  [CATransaction setDisableActions:YES];
+  self.layer.frame = self.bounds;


Can you add a comment explaining why this is needed? Specifically why setFrameSize: doesn't do this.

Actually, the root layer should be auto-resized with the view, I think it is safe to remove the self.layer.frame = self.bounds; call. I'll look into it.

I removed the assignment.

iskakaushik · 2020-10-23T17:32:45Z

shell/platform/darwin/macos/framework/Source/FlutterView.mm

+
+- (void)resizeSynchronizerCommit:(FlutterResizeSynchronizer*)synchronizer {
+  [CATransaction begin];
+  [CATransaction setDisableActions:YES];


Why do we need to disable the actions here?

Without this every change will have implicit animation added to it. So if you set bounds for example the bounds change will be animated, which is not something you want during resizing.

iskakaushik · 2020-10-27T16:50:12Z

Can you please amend the licenses file:

+++ out/license_script_output/licenses_flutter	2020-10-26 22:03:02.085013907 +0000
@@ -1031,21 +1031,27 @@
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterEngineUnittests.mm
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterEngine_Internal.h
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterExternalTextureGL.h
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterExternalTextureGL.mm
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterMouseCursorPlugin.h
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterMouseCursorPlugin.mm
+FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h
+FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm
+FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h
+FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputModel.h
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputModel.mm
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.h
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterTextInputPlugin.mm
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterView.h
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterView.mm
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController.mm
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterViewControllerTest.mm
 FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/FlutterViewController_Internal.h
+FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/MacOSSwitchableGLContext.h
+FILE: ../../../flutter/shell/platform/darwin/macos/framework/Source/MacOSSwitchableGLContext.mm```

iskakaushik · 2020-10-27T18:48:12Z

@knopp there are some failures still. I think some of them will be fixed by a rebase. Can you rebase on-top of master? Thanks!

Flush can be performed on raster thread

* 53d5d68 Add dart-lang/sdk's new package:clock dependency (flutter/engine#22142) * c32e3d8 Roll Skia from 7737a5bd2510 to 5567a6091ceb (8 revisions) (flutter/engine#22146) * 376045c Roll Fuchsia Linux SDK from XYN02FThN... to UKgKCjxSA... (flutter/engine#22147) * 03395de Roll Fuchsia Mac SDK from GKPwGj1Ux... to xHjtLQPQ5... (flutter/engine#22151) * 0c7e952 Manual Dart SDK roll 6e015bd9cddb to 9c6e76468ca4 (6 revisions (flutter/engine#22153) * e5f168a Update constraint to latest integration test (flutter/engine#22169) * e61e8c2 Smooth window resizing on macOS (flutter/engine#21525) * acece00 Allow parse_and_send to use access tokens (flutter/engine#22019) * 0270632 Includes roles for links, checkboxes, and radio buttons in semantics (flutter/engine#22061) * 632354d Roll Fuchsia Linux SDK from UKgKCjxSA... to PY5hNI-wY... (flutter/engine#22175) * 62d50af Add plumbing for command line arguments on Windows (flutter/engine#22094) * 06b0910 Fix possible use of std::moved value in Rasterizer (flutter/engine#22125) * 005dec4 [web] Clean up unused previousStyle logic (flutter/engine#22150) * ca05bdc Roll Skia from 5567a6091ceb to f548a028ce70 (7 revisions) (flutter/engine#22155) * 5b07ac4 Roll Fuchsia Mac SDK from xHjtLQPQ5... to ICK_JlnKJ... (flutter/engine#22188) * d615a97 Roll Fuchsia Linux SDK from PY5hNI-wY... to Usec4YBzR... (flutter/engine#22197) * 11ed711 Invalidate the cached SkParagraph font collection when a new font manager is installed (flutter/engine#22157) * 07c780b [web] Assign default natural width/height for svgs that report 0,0 on firefox and ie11 (flutter/engine#22184) * b54bb88 Migrate runZoned to runZonedGuarded (flutter/engine#22198) * 247139a [web] Fix transform not invalidating path bounds causing debugValidate failure (flutter/engine#22172) * e4dffc1 [web] Fix scroll wheel line delta on Firefox. (flutter/engine#21928) * d6627c6 Reland [ios] Refactor IOSSurface factory and unify surface creation (flutter/engine#22016) * ce1dd11 Standardize macro names for dartdoc macros (flutter/engine#22180) * f81bc37 [profiling] Handle thread_info to account for killed threads (flutter/engine#22170) * fd94c86 Fix for firefox custom clipping (flutter/engine#22182) * 9ccf9f1 bring back build_test to ensure we validate licenses (flutter/engine#22201) * 38f6665 Set strut font to Roboto if the given font hasn't been registered (flutter/engine#22129) * caf32d5 Add a proc table version of embedder API (flutter/engine#21813) * ed0f477 Use preTranslate when applying offset to matrix (flutter/engine#21648) * b457e2d Refactor make_mock_engine into fl_test (flutter/engine#21585) * 28497c8 Fix typos in FlValue docs (flutter/engine#21875) * bb32446 Fix FlTextInputPlugin tear down (flutter/engine#22007) * 7a7804b Add "input shield" to capture pointer input for reinjection (flutter/engine#22067) * fe85f94 Update painting.dart (flutter/engine#22195) * 99cc50d [ios] Surface factory holds the canonical reference to the external view embedder (flutter/engine#22206)

stuartmorgan

Thanks for this work; I only just saw that this had landed.

I've done a quick post-submit review pass since I was looking through it out of curiousity, and found a lot of style issues. Could you do a follow-up to address them?

There is also one substantive issue, which is the new use of code that isn't in embedder.h. I wish that had been raised with me before adding new dependencies that are not consistent with how any of the desktop embeddings have been written. How extensive is that code? Do we need to share it?

stuartmorgan · 2020-11-02T17:40:32Z

shell/platform/darwin/macos/framework/Source/FlutterEngine.mm

-  return 0;
+static uint32_t OnFBO(FlutterEngine* engine, const FlutterFrameInfo* info) {
+  CGSize size = CGSizeMake(info->size.width, info->size.height);
+  return [engine.viewController.flutterView getFrameBufferIdForSize:size];


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.

stuartmorgan · 2020-11-02T17:41:32Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h

+
+@class FlutterResizeSynchronizer;
+
+@protocol FlutterResizeSynchronizerDelegate


All protocols should have declaration comments explaining their role, per style guide.

stuartmorgan · 2020-11-02T17:42:11Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h

+
+@protocol FlutterResizeSynchronizerDelegate
+
+// Invoked on raster thread; Delegate should flush the OpenGL context


All of these comments need a . at the end, per style guide.

stuartmorgan · 2020-11-02T17:45:09Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h

+@protocol FlutterResizeSynchronizerDelegate
+
+// Invoked on raster thread; Delegate should flush the OpenGL context
+- (void)resizeSynchronizerFlush:(FlutterResizeSynchronizer*)synchronizer;


All of the macOS embedding's headers should have nullability annotations.

stuartmorgan · 2020-11-02T17:46:06Z

shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.mm

@@ -0,0 +1,104 @@
+#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h"
+
+#import <mutex>


stuartmorgan · 2020-11-02T17:59:49Z

shell/platform/darwin/macos/framework/Source/MacOSSwitchableGLContext.h

@@ -0,0 +1,21 @@
+#include "flutter/flow/gl_context_switch.h"


This file is missing its license.

stuartmorgan · 2020-11-02T18:00:05Z

shell/platform/darwin/macos/framework/Source/MacOSSwitchableGLContext.h

+
+#import <Cocoa/Cocoa.h>
+
+class MacOSSwitchableGLContext final : public flutter::SwitchableGLContext {


All classes need declaration comments.

stuartmorgan · 2020-11-02T18:00:15Z

shell/platform/darwin/macos/framework/Source/MacOSSwitchableGLContext.h

+ public:
+  explicit MacOSSwitchableGLContext(NSOpenGLContext* context);
+
+  bool SetCurrent() override;


Please put the class name this is coming from in a comment, as is done throughout the engine codebase.

stuartmorgan · 2020-11-02T18:01:26Z

shell/platform/darwin/macos/framework/Source/MacOSSwitchableGLContext.h

+
+  FML_DECLARE_THREAD_CHECKER(checker);
+
+  FML_DISALLOW_COPY_AND_ASSIGN(MacOSSwitchableGLContext);


New code should not use this macro; it violates style. Delete the relevant methods directly.

stuartmorgan · 2020-11-02T18:01:39Z

shell/platform/darwin/macos/framework/Source/MacOSSwitchableGLContext.mm

@@ -0,0 +1,19 @@
+#import "flutter/shell/platform/darwin/macos/framework/Source/MacOSSwitchableGLContext.h"


Missing license again.

knopp · 2020-11-02T18:50:41Z

I've done a quick post-submit review pass since I was looking through it out of curiousity, and found a lot of style issues. Could you do a follow-up to address them?

No problem, I'll submit a PR for this

There is also one substantive issue, which is the new use of code that isn't in embedder.h. I wish that had been raised with me before adding new dependencies that are not consistent with how any of the desktop embeddings have been written. How extensive is that code? Do we need to share it?

I think this refers to GLContextSwitch? It was suggested by @iskakaushik during review (similar to how iOS embedder does it). It's only used in handful of places, it should be farily easy to get rid of and track the previous context manually.

stuartmorgan · 2020-11-02T19:16:50Z

Right, that was referring to my comment on the flutter/flow/gl_context_switch.h include. The desktop embeddings deliberately don't reach past the embedder.h API into internal engine code. We should either replace that, or look into setting up a specific, clearly documented location for code that can be used in embeddings (that does not in turn depend on other engine internals, so it doesn't slowly spread to include more and more code.)

/cc @chinmaygarde for thoughts on the latter; I think it's probably worth considering having a place for that kind of code.

stuartmorgan · 2020-11-02T21:26:07Z

(I just ran with this fix for the first time, by the way, and it feels great 😃 )

knopp · 2020-11-03T17:29:17Z

@stuartmorgan, I have created #22270 for the code style issues. It should address all your points (unless I missed something) except for the enum not being enum class (the values are meant to be used as indices).

Previously, FlutterSurfaceManager was a protocol with two concrete implementations: FlutterGLSurfaceManager and FlutterMetalSurfaceManager. Because much of the surface manager code was shared, they had a shared superclass FlutterIOSurfaceManager that implemented the bulk of the logic and called into the subclasses when OpenGL or Metal-specific code was required. It did this via a delegate pattern where each of the surface manager subclasses implemented the FlutterIOSurfaceManagerDelegate protocol that exposed operations that required backend-specific code. Now that only the Metal implementation remains, the delegate code can be inlined into the calling functions, and the class hierarchy can be squashed into a single concrete implementation class, FlutterSurfaceManager, similar to how it was originally implemented in flutter#21525 before we had two backends. Issue: flutter/flutter#108304 Issue: flutter/flutter#114445

Previously, FlutterSurfaceManager was a protocol with two concrete implementations: FlutterGLSurfaceManager and FlutterMetalSurfaceManager. Most of the implementation was in a shared superclass, FlutterIOSurfaceManager, which called into the OpenGL or Metal-specific subclass when backend-specific operations (such as allocating textures) was required. It did so via a delegate pattern, wherein the subclasses both implemented the FlutterIOSurfaceManagerDelegate protocol that exposed the backend-specific functionality. Now that only the Metal implementation remains, the delegate code can be inlined into the calling functions, and the class hierarchy can be squashed into a single concrete implementation class, FlutterSurfaceManager, similar to how it was originally implemented in flutter#21525 before we had two backends. Issue: flutter/flutter#108304 Issue: flutter/flutter#114445

Previously, FlutterSurfaceManager was a protocol with two concrete implementations: FlutterGLSurfaceManager and FlutterMetalSurfaceManager. Most of the implementation was in a shared superclass, FlutterIOSurfaceManager, which called into the OpenGL or Metal-specific subclass when backend-specific operations (such as allocating textures) was required. It did so via a delegate pattern, wherein the subclasses both implemented the FlutterIOSurfaceManagerDelegate protocol that exposed the backend-specific functionality. Now that only the Metal implementation remains, the delegate code can be inlined into the calling functions, and the class hierarchy can be squashed into a single concrete implementation class, FlutterSurfaceManager, similar to how it was originally implemented in #21525 before we had two backends. Issue: flutter/flutter#108304 Issue: flutter/flutter#114445

googlebot added the cla: yes label Sep 30, 2020

knopp mentioned this pull request Sep 30, 2020

WIP: Smooth window resizing on macOS #21252

Closed

12 tasks

knopp force-pushed the macos_smooth_resizing_pr branch from d28d64b to d97ebaa Compare September 30, 2020 21:01

iskakaushik self-requested a review September 30, 2020 22:02

iskakaushik suggested changes Oct 1, 2020

View reviewed changes

iskakaushik reviewed Oct 1, 2020

View reviewed changes

knopp force-pushed the macos_smooth_resizing_pr branch from 28895d2 to d046e86 Compare October 1, 2020 17:24

iskakaushik reviewed Oct 1, 2020

View reviewed changes

shell/platform/darwin/macos/framework/Source/FlutterView.mm Show resolved Hide resolved

iskakaushik reviewed Oct 1, 2020

View reviewed changes

knopp force-pushed the macos_smooth_resizing_pr branch from 07dbce0 to c32f0cd Compare October 1, 2020 18:41

iskakaushik approved these changes Oct 26, 2020

View reviewed changes

knopp changed the title ~~WIP: Smooth window resizing on macOS~~ Smooth window resizing on macOS Oct 27, 2020

knopp added 9 commits October 27, 2020 20:04

Move raster task runner away from platform thread

586738a

Smooth resizing for macOS embedder

4f27eaf

Consistency

78121e5

Fix possible spurious wake-ups

1376e96

Separate flush and commit

3211670

Flush can be performed on raster thread

move assignment out of block

f49de06

Change [FlutterView start] to synchronousResizing property

6a5b14e

Restore the previously removed updateWindowMetrics call

e890a08

Improve comments and variable names

9a13f68

GaryQian mentioned this pull request Oct 29, 2020

Roll engine from 073263e39d1d to fe85f946db04 (34 revisions) manual flutter/flutter#69347

Closed

engine-flutter-autoroll mentioned this pull request Oct 29, 2020

Roll Engine from 073263e39d1d to fe85f946db04 (34 revisions) flutter/flutter#69349

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 29, 2020

e61e8c2 Smooth window resizing on macOS (flutter/engine#21525)

e9b14af

engine-flutter-autoroll mentioned this pull request Oct 30, 2020

Roll Engine from 073263e39d1d to 99cc50dfffab (35 revisions) flutter/flutter#69353

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020

e61e8c2 Smooth window resizing on macOS (flutter/engine#21525)

71e298c

engine-flutter-autoroll mentioned this pull request Oct 30, 2020

Roll Engine from 073263e39d1d to 7d6180c3014a (37 revisions) flutter/flutter#69357

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020

e61e8c2 Smooth window resizing on macOS (flutter/engine#21525)

97cb8c5

engine-flutter-autoroll mentioned this pull request Oct 30, 2020

Roll Engine from 99cc50dfffab1 to 7d6180c3014a (2 revisions) flutter/flutter#69365

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 30, 2020

e61e8c2 Smooth window resizing on macOS (flutter/engine#21525)

082d334

tvolkert mentioned this pull request Oct 30, 2020

☂️ Desktop resize is janky flutter/flutter#44136

Closed

3 tasks

stuartmorgan reviewed Nov 2, 2020

View reviewed changes

knopp mentioned this pull request Nov 3, 2020

Fix code style issues in MacOS embedder #22270

Merged

13 tasks

gw280 added affects: desktop needs tests platform-macos labels Nov 24, 2020

chaselatta pushed a commit to chaselatta/engine that referenced this pull request Nov 30, 2020

Smooth window resizing on macOS (flutter#21525)

3c883a8

cbracken mentioned this pull request Nov 17, 2022

[macOS] Merge FlutterSurfaceManager classes #37701

Merged

8 tasks

pedro-w mentioned this pull request Feb 24, 2023

al_draw_line coordinates are off when resizing a window on MacOS liballeg/allegro5#1410

Closed


		@end

		// Encapsulates the logic for blocking platform thread during window resize


		@class FlutterResizeSynchronizer;

		@protocol FlutterResizeSynchronizerDelegate


		@protocol FlutterResizeSynchronizerDelegate

		// Invoked on raster thread; Delegate should flush the OpenGL context

		@@ -0,0 +1,104 @@
		#import "flutter/shell/platform/darwin/macos/framework/Source/FlutterResizeSynchronizer.h"

		#import <mutex>


		#import <Cocoa/Cocoa.h>

		class MacOSSwitchableGLContext final : public flutter::SwitchableGLContext {


		FML_DECLARE_THREAD_CHECKER(checker);

		FML_DISALLOW_COPY_AND_ASSIGN(MacOSSwitchableGLContext);

		@@ -0,0 +1,19 @@
		#import "flutter/shell/platform/darwin/macos/framework/Source/MacOSSwitchableGLContext.h"

Smooth window resizing on macOS #21525

Smooth window resizing on macOS #21525

Conversation

knopp commented Sep 30, 2020 • edited Loading

Description

Related Issues

Tests

Checklist

Breaking Change

flutter-dashboard bot commented Sep 30, 2020

iskakaushik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knopp Oct 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knopp commented Oct 1, 2020

Choose a reason for hiding this comment

iskakaushik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iskakaushik commented Oct 27, 2020

iskakaushik commented Oct 27, 2020

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knopp commented Nov 2, 2020

stuartmorgan commented Nov 2, 2020

stuartmorgan commented Nov 2, 2020

knopp commented Nov 3, 2020

knopp commented Sep 30, 2020 •

edited

Loading

knopp Oct 1, 2020 •

edited

Loading