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.

Sign up for GitHub

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

Jump to bottom

[Windows] Improve logic to update swap intervals #46172

Merged
merged 2 commits into from
Sep 27, 2023
Merged

[Windows] Improve logic to update swap intervals #46172

merged 2 commits into from
Sep 27, 2023

Conversation

loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Sep 21, 2023 •
edited
Loading

This relands #45310 to unblock the ANGLE roll with the fix for flutter/flutter#134262.

Background

Swap interval

If the Windows system compositor is enabled, the Windows embedder disables the swap interval so that presenting to a surface does not block until the v-blank. If the Windows system compositor is disabled (which is possible on Windows 7), the Windows embedder enables swap interval to prevent screen tearing.

GL context threading

Our current version of ANGLE allows making a GL context current on multiple threads. However, the latest version of ANGLE errors if a GL context is made current on multiple threads. This is causing the ANGLE roll to fail (example).

The Windows embedder has two GL context threading issues:

  1. At startup, the platform thread creates and binds the GL context. This change ensures the GL context is released from the platform thread so that the raster thread can use the GL context for rendering.
  2. When the system compositor updates, the GL context is bound to the platform thread to update the swap interval. This change ensures the swap interval update happens on the raster thread.

Window resizing

Resizing the window recreates the GL surface and resets the swap interval.

The previous fix released the current GL context after updating the swap interval (this ensured the platform thread released the GL context at startup). This broke window resizing as it caused the engine to "lose" its GL context during rendering (see flutter/flutter#134262). This reland releases the GL context only if on the startup case.

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 marked this pull request as ready for review September 22, 2023 21:40
yaakovschectman
yaakovschectman approved these changes Sep 25, 2023
View reviewed changes
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

shell/platform/windows/flutter_windows_view.cc Outdated Show resolved Hide resolved
cbracken
cbracken approved these changes Sep 26, 2023
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 with one suggestion -- thanks for getting to the bottom of this!

shell/platform/windows/flutter_windows_view_unittests.cc Show resolved Hide resolved
@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2023
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 26, 2023
@auto-submit
Copy link
Contributor

auto-submit bot commented Sep 26, 2023

auto label is removed for flutter/engine/46172, due to - The status or check suite Linux linux_web_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 27, 2023
@auto-submit auto-submit bot merged commit 9d28c72 into flutter:main Sep 27, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 27, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 27, 2023
@engine-flutter-autoroll
9d28c7270 [Windows] Improve logic to update swap intervals (flutter/e…
90c062b 
…ngine#46172)
@loic-sharma loic-sharma deleted the reland_windows_vsync_fixes branch September 27, 2023 22:01
@loic-sharma loic-sharma mentioned this pull request Sep 27, 2023
Closed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 28, 2023
Merged
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 28, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 28, 2023
@engine-flutter-autoroll
9d28c7270 [Windows] Improve logic to update swap intervals (flutter/e…
a667450 
…ngine#46172)
zanderso pushed a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 28, 2023
@engine-flutter-autoroll @zanderso
9d28c7270 [Windows] Improve logic to update swap intervals (flutter/e…
4d34802 
…ngine#46172)
zanderso pushed a commit to flutter/flutter that referenced this pull request Sep 28, 2023
@engine-flutter-autoroll
Manual roll Flutter Engine from f70f65f7a622 to be32dcc9117a (31 revi…
f2cf0cc 
…sions) (#135637)

Manual roll Flutter Engine from f70f65f7a622 to be32dcc9117a (31
revisions)

Manual roll requested by jacksongardner@google.com

Cannot build log URL because revision "be32dcc9117a" is invalid: Luci
builds of "Linux linux_unopt" for
be32dcc9117a462760cb52c18d0d050f067f2463 was STARTED

2023-09-28 jacksongardner@google.com Rollback Dart SDK to 3.2.0-119.
(flutter/engine#46339)
2023-09-28 1961493+harryterkelsen@users.noreply.github.com [canvaskit]
Do not double-apply ImageFilter transform to children
(flutter/engine#46336)
2023-09-28 ychris@google.com Reland "Reverts "[ios] Fix app extension
not able to find assets from… (flutter/engine#46329)
2023-09-27 47866232+chunhtai@users.noreply.github.com Revert "[Impeller]
Fixes stroke path geometry that can draw outside o…
(flutter/engine#46334)
2023-09-27 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
jQHACBU4Fi2wMElkm... to U334SygIkffMJVmdu... (flutter/engine#46333)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 952e8dd66560 to
b048b468d641 (1 revision) (flutter/engine#46332)
2023-09-27 jason-simmons@users.noreply.github.com [Impeller] Destroy all
per-thread command pools tied to a context before deleting the context
(flutter/engine#46286)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from d78aba2524b3 to
952e8dd66560 (1 revision) (flutter/engine#46331)
2023-09-27 jonahwilliams@google.com [Impeller] Match Skia gradient
clamping behavior (and document). (flutter/engine#44825)
2023-09-27 skia-flutter-autoroll@skia.org Roll Dart SDK from
97647bb1666b to 80a965ee48ab (1 revision) (flutter/engine#46330)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from ff5474eed6b4 to
d78aba2524b3 (3 revisions) (flutter/engine#46327)
2023-09-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"[ios] Fix app extension not able to find assets from unloaded bundle"
(flutter/engine#46328)
2023-09-27 737941+loic-sharma@users.noreply.github.com [Windows] Improve
logic to update swap intervals (flutter/engine#46172)
2023-09-27 jonahwilliams@google.com [Impeller] Fallback to no index
buffer when tesselation count is large, split up nonZero contours.
(flutter/engine#46282)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 4731ccd6342c to
ff5474eed6b4 (1 revision) (flutter/engine#46325)
2023-09-27 jason-simmons@users.noreply.github.com Declare native wrapper
classes in Fuchsia packages as base classes (flutter/engine#46305)
2023-09-27 godofredoc@google.com Remove fuchsia from recipes cq.
(flutter/engine#46324)
2023-09-27 skia-flutter-autoroll@skia.org Roll Dart SDK from
c2a455113e39 to 97647bb1666b (1 revision) (flutter/engine#46323)
2023-09-27 matej.knopp@gmail.com [macOS] Synchronise modifiers from
mouse events for RawKeyboard (flutter/engine#46230)
2023-09-27 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from
Lg6FR6iDnZeV6y-E8... to 6Y22MutFhgL7ua18F... (flutter/engine#46322)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 2991bb799d3f to
4731ccd6342c (1 revision) (flutter/engine#46321)
2023-09-27 jason-simmons@users.noreply.github.com Declare native wrapper
classes in the GPU package as base classes (flutter/engine#46304)
2023-09-27 jason-simmons@users.noreply.github.com Do not call
DrawTextBlob for performance overlay text when using Impeller
(flutter/engine#46307)
2023-09-27 kjlubick@users.noreply.github.com Update to use
GrDirectContexts::MakeGL (flutter/engine#46308)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 0f4f31127ac5 to
2991bb799d3f (1 revision) (flutter/engine#46319)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from fc629215398f to
0f4f31127ac5 (1 revision) (flutter/engine#46318)
2023-09-27 skia-flutter-autoroll@skia.org Roll Dart SDK from
6c4eb86ecd25 to c2a455113e39 (1 revision) (flutter/engine#46317)
2023-09-27 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
OMrTgAfDg9PKXTzq0... to jQHACBU4Fi2wMElkm... (flutter/engine#46314)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 960325c13009 to
fc629215398f (1 revision) (flutter/engine#46313)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 76aecbaea259 to
960325c13009 (2 revisions) (flutter/engine#46311)
2023-09-27 skia-flutter-autoroll@skia.org Roll Dart SDK from
7c3588c05f87 to 6c4eb86ecd25 (3 revisions) (flutter/engine#46310)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Lg6FR6iDnZeV to 6Y22MutFhgL7
  fuchsia/sdk/core/mac-amd64 from OMrTgAfDg9PK to U334SygIkffM

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 jacksongardner@google.com,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

...
Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
@engine-flutter-autoroll @Mairramer
Manual roll Flutter Engine from f70f65f7a622 to be32dcc9117a (31 revi…
730a17c 
…sions) (flutter#135637)

Manual roll Flutter Engine from f70f65f7a622 to be32dcc9117a (31
revisions)

Manual roll requested by jacksongardner@google.com

Cannot build log URL because revision "be32dcc9117a" is invalid: Luci
builds of "Linux linux_unopt" for
be32dcc9117a462760cb52c18d0d050f067f2463 was STARTED

2023-09-28 jacksongardner@google.com Rollback Dart SDK to 3.2.0-119.
(flutter/engine#46339)
2023-09-28 1961493+harryterkelsen@users.noreply.github.com [canvaskit]
Do not double-apply ImageFilter transform to children
(flutter/engine#46336)
2023-09-28 ychris@google.com Reland "Reverts "[ios] Fix app extension
not able to find assets from… (flutter/engine#46329)
2023-09-27 47866232+chunhtai@users.noreply.github.com Revert "[Impeller]
Fixes stroke path geometry that can draw outside o…
(flutter/engine#46334)
2023-09-27 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
jQHACBU4Fi2wMElkm... to U334SygIkffMJVmdu... (flutter/engine#46333)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 952e8dd66560 to
b048b468d641 (1 revision) (flutter/engine#46332)
2023-09-27 jason-simmons@users.noreply.github.com [Impeller] Destroy all
per-thread command pools tied to a context before deleting the context
(flutter/engine#46286)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from d78aba2524b3 to
952e8dd66560 (1 revision) (flutter/engine#46331)
2023-09-27 jonahwilliams@google.com [Impeller] Match Skia gradient
clamping behavior (and document). (flutter/engine#44825)
2023-09-27 skia-flutter-autoroll@skia.org Roll Dart SDK from
97647bb1666b to 80a965ee48ab (1 revision) (flutter/engine#46330)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from ff5474eed6b4 to
d78aba2524b3 (3 revisions) (flutter/engine#46327)
2023-09-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts
"[ios] Fix app extension not able to find assets from unloaded bundle"
(flutter/engine#46328)
2023-09-27 737941+loic-sharma@users.noreply.github.com [Windows] Improve
logic to update swap intervals (flutter/engine#46172)
2023-09-27 jonahwilliams@google.com [Impeller] Fallback to no index
buffer when tesselation count is large, split up nonZero contours.
(flutter/engine#46282)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 4731ccd6342c to
ff5474eed6b4 (1 revision) (flutter/engine#46325)
2023-09-27 jason-simmons@users.noreply.github.com Declare native wrapper
classes in Fuchsia packages as base classes (flutter/engine#46305)
2023-09-27 godofredoc@google.com Remove fuchsia from recipes cq.
(flutter/engine#46324)
2023-09-27 skia-flutter-autoroll@skia.org Roll Dart SDK from
c2a455113e39 to 97647bb1666b (1 revision) (flutter/engine#46323)
2023-09-27 matej.knopp@gmail.com [macOS] Synchronise modifiers from
mouse events for RawKeyboard (flutter/engine#46230)
2023-09-27 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from
Lg6FR6iDnZeV6y-E8... to 6Y22MutFhgL7ua18F... (flutter/engine#46322)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 2991bb799d3f to
4731ccd6342c (1 revision) (flutter/engine#46321)
2023-09-27 jason-simmons@users.noreply.github.com Declare native wrapper
classes in the GPU package as base classes (flutter/engine#46304)
2023-09-27 jason-simmons@users.noreply.github.com Do not call
DrawTextBlob for performance overlay text when using Impeller
(flutter/engine#46307)
2023-09-27 kjlubick@users.noreply.github.com Update to use
GrDirectContexts::MakeGL (flutter/engine#46308)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 0f4f31127ac5 to
2991bb799d3f (1 revision) (flutter/engine#46319)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from fc629215398f to
0f4f31127ac5 (1 revision) (flutter/engine#46318)
2023-09-27 skia-flutter-autoroll@skia.org Roll Dart SDK from
6c4eb86ecd25 to c2a455113e39 (1 revision) (flutter/engine#46317)
2023-09-27 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
OMrTgAfDg9PKXTzq0... to jQHACBU4Fi2wMElkm... (flutter/engine#46314)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 960325c13009 to
fc629215398f (1 revision) (flutter/engine#46313)
2023-09-27 skia-flutter-autoroll@skia.org Roll Skia from 76aecbaea259 to
960325c13009 (2 revisions) (flutter/engine#46311)
2023-09-27 skia-flutter-autoroll@skia.org Roll Dart SDK from
7c3588c05f87 to 6c4eb86ecd25 (3 revisions) (flutter/engine#46310)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Lg6FR6iDnZeV to 6Y22MutFhgL7
  fuchsia/sdk/core/mac-amd64 from OMrTgAfDg9PK to U334SygIkffM

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 jacksongardner@google.com,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

...
harryterkelsen pushed a commit that referenced this pull request Oct 23, 2023
@loic-sharma @harryterkelsen
[Windows] Improve logic to update swap intervals (#46172)
88575ed 
This relands #45310 to unblock the ANGLE roll with the fix for flutter/flutter#134262.

## Background

### Swap interval

If the Windows system compositor is enabled, the Windows embedder disables the swap interval so that presenting to a surface does not block until the v-blank. If the Windows system compositor is disabled (which is possible on Windows 7), the Windows embedder enables swap interval to prevent screen tearing.

### GL context threading

Our current version of ANGLE allows making a GL context current on multiple threads. However, the latest version of ANGLE errors if a GL context is made current on multiple threads. This is causing the ANGLE roll to fail ([example](https://ci.chromium.org/ui/p/flutter/builders/try/Windows%20Engine%20Drone/203788/overview)).

The Windows embedder has two GL context threading issues:

1. At startup, the platform thread creates and binds the GL context. This change ensures the GL context is released from the platform thread so that the raster thread can use the GL context for rendering.
2. When the system compositor updates, the GL context is bound to the platform thread to update the swap interval. This change ensures the swap interval update happens on the raster thread.

### Window resizing

Resizing the window recreates the GL surface and resets the swap interval.

The previous fix released the current GL context after updating the swap interval (this ensured the platform thread released the GL context at startup). This broke window resizing as it caused the engine to "lose" its GL context during rendering (see flutter/flutter#134262). This reland releases the GL context only if on the startup case.

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

@yaakovschectman yaakovschectman yaakovschectman approved these changes

@cbracken cbracken cbracken 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