Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Windows] Refactor logic when window resize completes #49872

Merged
merged 2 commits into from
Jan 18, 2024
Merged

[Windows] Refactor logic when window resize completes #49872

merged 2 commits into from
Jan 18, 2024

Conversation

loic-sharma
Copy link
Member

Changes:

  1. Moves surface buffer swapping from FlutterWindowsView to CompositorOpenGL
  2. Renames FlutterWindowsView::SwapBuffers to FlutterWindowsView::OnFramePresented
  3. Previously, if a resize was pending and the window was not visible Windows would unblock the platform thread before swapping buffers. This trick aimed to reduce the time the platform thread was blocked as swapping buffers previously waited until the v-blank. This logic was removed as swapping buffers no longer waits until the v-blank.

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 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] Refactor resize complete logic [Windows] Refactor logic when window resize completes Jan 18, 2024
yaakovschectman
yaakovschectman reviewed Jan 18, 2024
View reviewed changes
shell/platform/windows/angle_surface_manager.cc
@@ -329,7 +329,7 @@ bool AngleSurfaceManager::MakeResourceCurrent() {
egl_resource_context_) == EGL_TRUE);
}

EGLBoolean AngleSurfaceManager::SwapBuffers() {
bool AngleSurfaceManager::SwapBuffers() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this change?

Copy link
Member

Choose a reason for hiding this comment

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

Anytime we can avoid leaking implementation details (including types) from behind interfaces, it's a nice plus. Users of our surface managers shouldn't need to know about the underlying usage of GL types.

shell/platform/windows/compositor_opengl.cc
Comment on lines +157 to +162
if (!engine_->surface_manager()->SwapBuffers()) {
return false;
}

engine_->view()->OnFramePresented();
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be refactored into a separate function? It looks like this and the below addition to this file are identical.

Copy link
Member Author

@loic-sharma loic-sharma Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You are right that they're identical, but I think the logic is short enough that it's okay to repeat. I don't feel super strongly about this though, let me know if you do and I'd be happy to update this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought. I don't think it's too important either way.

cbracken
cbracken approved these changes Jan 18, 2024
View reviewed changes
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.

LGTM stamp from a Japanese personal seal

I love it! Super since!

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 18, 2024
@auto-submit auto-submit bot merged commit dde3ebf into flutter:main Jan 18, 2024
@loic-sharma loic-sharma deleted the windows_refactor_present branch January 18, 2024 22:18
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 18, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2024
@engine-flutter-autoroll
dde3ebf65 [Windows] Refactor logic when window resize completes (flut…
d5e988b 
…ter/engine#49872)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 18, 2024
@engine-flutter-autoroll
Roll Flutter Engine from 9dded186bcff to dde3ebf6551a (2 revisions) (#…
cdf82b2 
…141811)

flutter/engine@9dded18...dde3ebf

2024-01-18 [email protected] [Windows] Refactor logic when window resize completes (flutter/engine#49872)
2024-01-18 [email protected] Roll Skia from 40200ceca00e to da3bfb25fb84 (1 revision) (flutter/engine#49873)

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 [email protected],[email protected],[email protected] 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://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@loic-sharma loic-sharma added the revert Label used to revert changes in a closed and merged pull request. label Jan 22, 2024
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Jan 22, 2024

Time to revert pull request flutter/engine/49872 has elapsed.
You need to open the revert manually and process as a regular pull request.

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jan 22, 2024
This was referenced Jan 22, 2024
Closed
Merged
auto-submit bot pushed a commit that referenced this pull request Jan 22, 2024
@loic-sharma
[Windows] Fix resize crash (#49935)
d87e742 
#49872 introduced a crash in debug mode if the platform thread starts a window resize in between `OnFrameGenerated` and `OnFramePresented`.

Fixes flutter/flutter#141855.

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cbracken cbracken cbracken approved these changes

@yaakovschectman yaakovschectman yaakovschectman approved these changes

Assignees
No one assigned
Labels
affects: desktop autosubmit Merge PR when tree becomes green via auto submit App platform-windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@loic-sharma @cbracken @yaakovschectman