[windows] Enable smooth resizing on windows #23701

iskakaushik · 2021-01-14T22:56:11Z

Brings #22279 up-to date and some minor modifications.

Related Issues

flutter/flutter#44136
flutter/flutter#66475

flutter-dashboard · 2021-01-14T22:56:14Z

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.

shell/platform/windows/BUILD.gn

shell/platform/windows/flutter_windows_view.h

shell/platform/windows/flutter_windows_view.cc

stuartmorgan · 2021-01-15T17:06:25Z

shell/platform/windows/flutter_windows_view.cc

+
+  if (resize_target_width_ == width && resize_target_height_ == height) {
+    surface_manager_->ResizeSurface(GetRenderTarget(), width, height);
+    surface_manager_->MakeCurrent();


Do we actually need these two steps to done within the lock? It's unclear to me if the mutex is also intended to protect the actual buffer swap, or just the ivars.

Relatedly, it would be helpful to annotate all these lock-using methods with the thread we expect them to be called on so it's easier to understand the flow.

surface_manager_ methods here don't need to be protected by the mutex, the line immediately after this needs to be protected. That being said having these be protected by the mutex shouldn't cause any contention (for the flow as it stands now) given that all the surface manager methods will be invoked on the raster task runner.

I had a question about validating the task runner, i see this pattern a lot: GetTaskRunner()->RunsTasksOnCurrentThread(), but it looks like in the case of windows the emedding only has a handle to the platform task runner / thread. I added comments to the methods with the task runner they run on, but i suspect theres something better we can do. Any ideas?

Yes, we only have access to the task runners of threads we provide custom runners for. Comments are fine here; my concern wasn't enforcing invariants, just helping readers understand the threading flow (the assertion version is nice because then you have confidence that the comments aren't out of sync with reality, but I don't think that's a significant concern here.)

That being said having these be protected by the mutex shouldn't cause any contention (for the flow as it stands now) given that all the surface manager methods will be invoked on the raster task runner.

Just to make sure I'm understanding: the reason we don't care about the lock blocking the platform thread unnecessarily during these steps is that if this block is running the platform thread is blocked on this whole step completing regardless, right?

It wouldn't hurt to put a comment here to that effect so that it doesn't look like a locking mistake (which tends to be my default concern when I see non-trivial function calls happening inside a lock).

Added a comment clarifying that platform thread is blocked for the entirety of the step.

shell/platform/windows/flutter_windows_view.cc

shell/platform/windows/flutter_windows_view.h

clarkezone

LGTM with one suggestion

clarkezone · 2021-01-16T14:11:26Z

shell/platform/windows/flutter_windows_view.cc

+}
+
+void FlutterWindowsView::OnWindowSizeChanged(size_t width, size_t height) {
+  // platform task runner


Does this comment mean this method must be called from the platform thread? If that is true would be great to flesh out the comment and / or maybe also express this in the header comment for this function since callers (UWP or win32 FlutterWindow implementations) have different UI threading models and that contract may have material impact to their implementations.

+1; please use style-conformant (i.e., full-sentence) comments. E.g., "Called on the platform thread."

Converted these to full comments, and also amended the headers.

stuartmorgan

LGTM with some small comments.

stuartmorgan · 2021-01-19T14:09:59Z

shell/platform/windows/BUILD.gn

@@ -98,6 +98,8 @@ source_set("flutter_windows_source") {
      "win32_window_proc_delegate_manager.cc",
      "win32_window_proc_delegate_manager.h",
    ]
+
+    libs = [ "dwmapi.lib" ]


Shouldn't this still be in a !winuwp condition?

This is in an else block for if (target_os == "winuwp") already.

stuartmorgan · 2021-01-19T14:13:29Z

shell/platform/windows/flutter_windows_view.cc

+    std::unique_ptr<WindowBindingHandler> window_binding)
+    : resize_status_(ResizeState::kDone),
+      resize_target_width_(0),
+      resize_target_height_(0) {


Initializer list is an improvement over the previous version, but I was referring to the C++11 feature of giving the initial value in the header where the value is declared: https://abseil.io/tips/61

stuartmorgan · 2021-01-19T14:22:06Z

shell/platform/windows/flutter_windows_view.cc

+}
+
+void FlutterWindowsView::OnWindowSizeChanged(size_t width, size_t height) {
+  // platform task runner


+1; please use style-conformant (i.e., full-sentence) comments. E.g., "Called on the platform thread."

stuartmorgan · 2021-01-19T14:26:41Z

shell/platform/windows/flutter_windows_view.cc

+
+  if (resize_target_width_ == width && resize_target_height_ == height) {
+    surface_manager_->ResizeSurface(GetRenderTarget(), width, height);
+    surface_manager_->MakeCurrent();


Yes, we only have access to the task runners of threads we provide custom runners for. Comments are fine here; my concern wasn't enforcing invariants, just helping readers understand the threading flow (the assertion version is nice because then you have confidence that the comments aren't out of sync with reality, but I don't think that's a significant concern here.)

stuartmorgan · 2021-01-19T14:27:38Z

shell/platform/windows/flutter_windows_view.cc

@@ -255,7 +287,27 @@ bool FlutterWindowsView::ClearContext() {
 }

 bool FlutterWindowsView::SwapBuffers() {
-  return surface_manager_->SwapBuffers();
+  // raster task runner


I wouldn't necessarily name the specific thread for the raster thread cases since that's up to the engine; instead you could say something like "Called on an engine-controlled (non-platform) thread."

(It's particularly confusing to mention in an embedding built on the API, because the API never mentions the raster thread.)

stuartmorgan · 2021-01-19T14:32:22Z

shell/platform/windows/flutter_windows_view.cc

+
+  if (resize_target_width_ == width && resize_target_height_ == height) {
+    surface_manager_->ResizeSurface(GetRenderTarget(), width, height);
+    surface_manager_->MakeCurrent();


That being said having these be protected by the mutex shouldn't cause any contention (for the flow as it stands now) given that all the surface manager methods will be invoked on the raster task runner.

Just to make sure I'm understanding: the reason we don't care about the lock blocking the platform thread unnecessarily during these steps is that if this block is running the platform thread is blocked on this whole step completing regardless, right?

It wouldn't hurt to put a comment here to that effect so that it doesn't look like a locking mistake (which tends to be my default concern when I see non-trivial function calls happening inside a lock).

fluttergithubbot · 2021-01-19T17:34:01Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite Linux Arm Host Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

This reverts commit f6162dc.

…3701)

iskakaushik requested review from knopp, stuartmorgan and clarkezone January 14, 2021 22:56

googlebot added the cla: yes label Jan 14, 2021

[windows] Enable smooth resizing on windows

946ffd5

iskakaushik force-pushed the windows_resize_fixes branch from f36188b to 946ffd5 Compare January 14, 2021 23:04

stuartmorgan requested changes Jan 15, 2021

View reviewed changes

address review comments

8425fd0

clarkezone approved these changes Jan 16, 2021

View reviewed changes

clarkezone mentioned this pull request Jan 17, 2021

Complete drawing for UWP embedding flutter/flutter#70198

Closed

4 tasks

stuartmorgan approved these changes Jan 19, 2021

View reviewed changes

fix comments about task runner affinity

2b2b37e

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 19, 2021

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 19, 2021

iskakaushik merged commit f6162dc into flutter:master Jan 19, 2021

iskakaushik deleted the windows_resize_fixes branch January 19, 2021 19:17

iskakaushik mentioned this pull request Jan 19, 2021

Smooth resizing on windows (Win32) flutter/flutter#66475

Closed

iskakaushik added a commit to iskakaushik/engine that referenced this pull request Jan 19, 2021

Revert "[windows] Enable smooth resizing on windows (flutter#23701)"

5f51e46

This reverts commit f6162dc.

engine-flutter-autoroll mentioned this pull request Jan 19, 2021

Roll Engine from 609036f2bf7c to 915e2ca178e5 (13 revisions) flutter/flutter#74266

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 19, 2021

f6162dc [windows] Enable smooth resizing on windows (flutter/engine#2…

0a51226

…3701)

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 20, 2021

[windows] Enable smooth resizing on windows (flutter#23701)

ff45a2c

hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021

[windows] Enable smooth resizing on windows (flutter#23701)

1ad2c1b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[windows] Enable smooth resizing on windows #23701

[windows] Enable smooth resizing on windows #23701

iskakaushik commented Jan 14, 2021 •

edited by stuartmorgan

flutter-dashboard bot commented Jan 14, 2021

stuartmorgan Jan 15, 2021

iskakaushik Jan 15, 2021

stuartmorgan Jan 19, 2021

stuartmorgan Jan 19, 2021 •

edited

iskakaushik Jan 19, 2021

clarkezone left a comment

clarkezone Jan 16, 2021

stuartmorgan Jan 19, 2021

iskakaushik Jan 19, 2021

stuartmorgan left a comment

stuartmorgan Jan 19, 2021

iskakaushik Jan 19, 2021

stuartmorgan Jan 19, 2021

iskakaushik Jan 19, 2021

stuartmorgan Jan 19, 2021

stuartmorgan Jan 19, 2021

stuartmorgan Jan 19, 2021

stuartmorgan Jan 19, 2021 •

edited

fluttergithubbot commented Jan 19, 2021

[windows] Enable smooth resizing on windows #23701

[windows] Enable smooth resizing on windows #23701

Conversation

iskakaushik commented Jan 14, 2021 • edited by stuartmorgan

Related Issues

flutter-dashboard bot commented Jan 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan Jan 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarkezone 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

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

stuartmorgan Jan 19, 2021 • edited

Choose a reason for hiding this comment

fluttergithubbot commented Jan 19, 2021

iskakaushik commented Jan 14, 2021 •

edited by stuartmorgan

stuartmorgan Jan 19, 2021 •

edited

stuartmorgan Jan 19, 2021 •

edited