Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Windows] Add force redraw to the C++ client wrapper #42061

Merged
merged 2 commits into from May 16, 2023

Conversation

loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented May 15, 2023

This change adds a C++ client wrapper to Windows's "force redraw" C API. This API can be used to schedule a frame.

Part of flutter/flutter#119415

Background

The Windows runner has a race at startup:

  1. Platform thread: creates a hidden window
  2. Platform thread: launches the Flutter engine
  3. UI/Raster threads: renders the first frame
  4. Platform thread: Registers a callback to show the window once the next frame has been rendered.

Steps 3 and 4 happen in parallel and it is possible for step 3 to complete before step 4 starts. In this scenario, the next frame callback is never called and the window is never shown.

The Windows runner will be updated to call the "force redraw" API after it registers the next frame callback. If step 3 hasn't completed yet, the "force redraw" will no-op as a frame is already scheduled. If step 3 has already completed, the "force redraw" will schedule another frame, which will call the next frame callback and show the window.

See this discussion below on why we cannot avoid changing the Windows runner to fix this issue: #42061 (comment)

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@loic-sharma loic-sharma changed the title [Windows] Add force redraw to C++ client wrapper [Windows] Add force redraw to the C++ client wrapper May 16, 2023
@loic-sharma loic-sharma marked this pull request as ready for review May 16, 2023 16:19
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself seems technically correct and tested, but I wonder if there's a way to do this without adding public API -- i.e. trigger this from the internal window.cc when it goes from hidden to shown?

I don't think there's necessarily anything wrong with adding public API to support this -- we expose frame scheduling to the Dart side app too -- but it would be nice to just do the right thing automatically with no new code in the runner, if we can.

@loic-sharma
Copy link
Member Author

loic-sharma commented May 16, 2023

but it would be nice to just do the right thing automatically with no new code in the runner, if we can.

I don't think we can avoid changing the runner. The "correct" solution would be to add the view and register the next frame callback atomically. This would require non-trivial embedder API changes. Currently both of these operations work by enqueueing work to the raster thread's task runner - nothing prevents these two tasks from being interleaved with raster operations submitted by the UI thread. In other words, we cannot avoid this race simply by moving/reordering when we add the view or the next frame callback.

Another potential solution would be to register the "show window" callback after we've created the view but before we've sent the window metrics:

if (!state->view->GetEngine()->running()) {
if (!state->view->GetEngine()->Run()) {
return nullptr;
}
}
// Must happen after engine is running.
state->view->SendInitialBounds();
state->view->SendInitialAccessibilityFeatures();
return state.release();

This would require changes to the Windows embedder's runner API: the FlutterViewController's constructor would need to accept a "show window" callback that is passed to the Windows embedder's FlutterDesktopViewControllerCreate (or some fork to avoid breaking changes). The runner would still need to be modified to something like:

bool FlutterWindow::OnCreate() {
  // ...
  flutter_controller_ = std::make_unique<flutter::FlutterViewController>(
-     frame.right - frame.left, frame.bottom - frame.top, project_);
+     frame.right - frame.left,
+     frame.bottom - frame.top,
+     project_,
+     [&]() {
+       this->Show();
+     });
  // Ensure that basic setup of the controller was successful.
  if (!flutter_controller_->engine() || !flutter_controller_->view()) {
    return false;
  }
  RegisterPlugins(flutter_controller_->engine());
  SetChildContent(flutter_controller_->view()->GetNativeWindow());

- flutter_controller_->engine()->SetNextFrameCallback([&]() {
-   this->Show();
- });

  return true;
}

The "force frame" approach requires the following runner modification:

```diff
bool FlutterWindow::OnCreate() {
  // ...
  flutter_controller_ = std::make_unique<flutter::FlutterViewController>(
     frame.right - frame.left, frame.bottom - frame.top, project_);
  // Ensure that basic setup of the controller was successful.
  if (!flutter_controller_->engine() || !flutter_controller_->view()) {
    return false;
  }
  RegisterPlugins(flutter_controller_->engine());
  SetChildContent(flutter_controller_->view()->GetNativeWindow());

  flutter_controller_->engine()->SetNextFrameCallback([&]() {
    this->Show();
  });

+ // It is possible Flutter rendered the first frame before the "show window"
+ // callback was registered. In case this happened, schedule another frame to
+ // ensure the window is shown. This no-ops if the first frame hasn't completed yet.
+ flutter_controller_->ForceFrame();

  return true;
}

In my mind the "force frame" approach is the least intrusive change to the Windows runner.

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed write-up; agreed the required embedder API changes to work around this aren't worth it, plus we already expose this to the Dart bits of the app and it's in the C API anyway.

lgtm

Copy link
Contributor

@yaakovschectman yaakovschectman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if we want this in the API

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label May 16, 2023
@auto-submit auto-submit bot merged commit 2feb085 into flutter:main May 16, 2023
28 checks passed
@loic-sharma loic-sharma deleted the windows_cpp_force_redraw branch May 16, 2023 18:14
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 16, 2023
zanderso pushed a commit to flutter/flutter that referenced this pull request May 16, 2023
…sions) (#126961)

Manual roll requested by zra@google.com

flutter/engine@fe24767...1c775e3

2023-05-16 zanderso@users.noreply.github.com Revert "[ios_platform_view]
only recycle maskView when the view is applying mutators"
(flutter/engine#42080)
2023-05-16 gspencergoog@users.noreply.github.com [macOS] Wait for
binding to be ready before requesting exits from framework
(flutter/engine#41753)
2023-05-16 gspencergoog@users.noreply.github.com [linux] Wait for
binding to be ready before requesting exits from framework
(flutter/engine#41782)
2023-05-16 jacksongardner@google.com Initial support for images in
Skwasm (flutter/engine#42019)
2023-05-16 jacksongardner@google.com Use new `unresolvedCodePoints` API
from skia. (flutter/engine#41991)
2023-05-16 jason-simmons@users.noreply.github.com Convert public API
NativeFieldWrapper classes to abstract interfaces (flutter/engine#41945)
2023-05-16 737941+loic-sharma@users.noreply.github.com [Windows] Add
force redraw to the C++ client wrapper (flutter/engine#42061)
2023-05-16 godofredoc@google.com Fix drone_dimension
host_engine_builder. (flutter/engine#42068)
2023-05-16 godofredoc@google.com Add linux_clang_tidy builder.
(flutter/engine#41990)
2023-05-16 ychris@google.com [ios_platform_view] only recycle maskView
when the view is applying mutators (flutter/engine#41573)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that
a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 19, 2023
## Background

The Windows runner has a race at startup:

1. **Platform thread**: creates a hidden window
2. **Platform thread**: launches the Flutter engine
3. **UI/Raster threads**: renders the first frame
4. **Platform thread**: Registers a callback to show the window once the next frame has been rendered.

Steps 3 and 4 happen in parallel and it is possible for step 3 to complete before step 4 starts. In this scenario, the next frame callback is never called and the window is never shown.

As a result the `windows_startup_test`'s test, which [verifies that the "show window" callback is called](https://github.com/flutter/flutter/blob/1f09a8662dad3bb1959b24e9124e05e2b9dbff1d/dev/integration_tests/windows_startup_test/windows/runner/flutter_window.cpp#L60-L64), can flake if the first frame is rendered before the show window callback has been registered.

## Solution

This change makes the runner schedule a frame after it registers the next frame callback. If step 3 hasn't completed yet, this no-ops as a frame is already scheduled. If step 3 has already completed, a new frame will be rendered, which will call the next frame callback and show the window.

Part of #119415

See this thread for alternatives that were considered: flutter/engine#42061 (comment)
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
…sions) (flutter#126961)

Manual roll requested by zra@google.com

flutter/engine@fe24767...1c775e3

2023-05-16 zanderso@users.noreply.github.com Revert "[ios_platform_view]
only recycle maskView when the view is applying mutators"
(flutter/engine#42080)
2023-05-16 gspencergoog@users.noreply.github.com [macOS] Wait for
binding to be ready before requesting exits from framework
(flutter/engine#41753)
2023-05-16 gspencergoog@users.noreply.github.com [linux] Wait for
binding to be ready before requesting exits from framework
(flutter/engine#41782)
2023-05-16 jacksongardner@google.com Initial support for images in
Skwasm (flutter/engine#42019)
2023-05-16 jacksongardner@google.com Use new `unresolvedCodePoints` API
from skia. (flutter/engine#41991)
2023-05-16 jason-simmons@users.noreply.github.com Convert public API
NativeFieldWrapper classes to abstract interfaces (flutter/engine#41945)
2023-05-16 737941+loic-sharma@users.noreply.github.com [Windows] Add
force redraw to the C++ client wrapper (flutter/engine#42061)
2023-05-16 godofredoc@google.com Fix drone_dimension
host_engine_builder. (flutter/engine#42068)
2023-05-16 godofredoc@google.com Add linux_clang_tidy builder.
(flutter/engine#41990)
2023-05-16 ychris@google.com [ios_platform_view] only recycle maskView
when the view is applying mutators (flutter/engine#41573)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that
a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
CaseyHillers pushed a commit to CaseyHillers/flutter that referenced this pull request May 24, 2023
## Background

The Windows runner has a race at startup:

1. **Platform thread**: creates a hidden window
2. **Platform thread**: launches the Flutter engine
3. **UI/Raster threads**: renders the first frame
4. **Platform thread**: Registers a callback to show the window once the next frame has been rendered.

Steps 3 and 4 happen in parallel and it is possible for step 3 to complete before step 4 starts. In this scenario, the next frame callback is never called and the window is never shown.

As a result the `windows_startup_test`'s test, which [verifies that the "show window" callback is called](https://github.com/flutter/flutter/blob/1f09a8662dad3bb1959b24e9124e05e2b9dbff1d/dev/integration_tests/windows_startup_test/windows/runner/flutter_window.cpp#L60-L64), can flake if the first frame is rendered before the show window callback has been registered.

## Solution

This change makes the runner schedule a frame after it registers the next frame callback. If step 3 hasn't completed yet, this no-ops as a frame is already scheduled. If step 3 has already completed, a new frame will be rendered, which will call the next frame callback and show the window.

Part of flutter#119415

See this thread for alternatives that were considered: flutter/engine#42061 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-windows
Projects
None yet
3 participants