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] Make the view own its EGL surface #50421

Merged
merged 1 commit into from
Feb 12, 2024
Merged

[Windows] Make the view own its EGL surface #50421

merged 1 commit into from
Feb 12, 2024

Conversation

loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Feb 6, 2024 •
edited
Loading

This makes the view own its EGL surface. This will allow us to support multiple EGL surfaces once the engine supports having multiple views.

Some notable changes:

  1. EGL surface resizing logic is now entirely in FlutterWindowsView. Previously some resizing logic was in the egl::Manager, however, the view has to handle resizing failures so this unifies the logic in one place.
  2. The OnEmptyFrameGenerated and OnFrameGenerated now return false (aka "don't present") if the surface is invalid. This simplifies the compositor as it no longer needs to check for invalid surfaces
  3. This introduces a ViewModifier testing helper to allow overriding a view's surface. This isn't strictly necessary, tests can setup a surface by mocking several EGL methods and calling FlutterWindowsView::CreateRenderSurface(). However, this is verbose & heavily tied to implementation details. The ViewModifier avoids this boilerplate.
  4. CompositorOpenGL's initialization now makes the render context current without any render surfaces. Previously it also made the view's surface current, which was unnecessary.

Part of flutter/flutter#137267
Part of flutter/flutter#142845

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 commented Feb 7, 2024
View reviewed changes
shell/platform/windows/flutter_windows_view.cc
void FlutterWindowsView::DestroyRenderSurface() {
if (!engine_) {
return;
bool FlutterWindowsView::ResizeRenderSurface(size_t width, size_t height) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was moved from egl::Manager::ResizeWindowSurface

@loic-sharma loic-sharma marked this pull request as ready for review February 7, 2024 19:13
yaakovschectman
yaakovschectman approved these changes Feb 7, 2024
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

@chinmaygarde
Copy link
Member

Can we land this?

@loic-sharma
Copy link
Member Author

This change is a bit gnarly so I'd also like a review from @cbracken if possible. I'll land this soon!

@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 12, 2024
@auto-submit auto-submit bot merged commit a190775 into flutter:main Feb 12, 2024
29 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 12, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 12, 2024
@engine-flutter-autoroll
a19077503 [Windows] Make the view own its EGL surface (flutter/engine…
d103c27 
…#50421)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 12, 2024
@engine-flutter-autoroll
Roll Flutter Engine from 1c3ecee77350 to a19077503a0c (8 revisions) (#…
1e6a902 
…143322)

flutter/engine@1c3ecee...a190775

2024-02-12 737941+loic-sharma@users.noreply.github.com [Windows] Make the view own its EGL surface (flutter/engine#50421)
2024-02-12 skia-flutter-autoroll@skia.org Roll Dart SDK from 5c9e79e66364 to dca13675e65e (1 revision) (flutter/engine#50559)
2024-02-12 1889916+miguelfrde@users.noreply.github.com [fuchsia] Update Inspect library usage (flutter/engine#50467)
2024-02-12 zanderso@users.noreply.github.com Initializes RBE in the build config runner (flutter/engine#50543)
2024-02-12 tugorez@users.noreply.github.com Rename [ViewFocusDirection.backwards] to [ViewFocusDirection.backward] (flutter/engine#50533)
2024-02-12 dnfield@google.com Refactor NDK helpers some more, add methods for SurfaceControl/Transaction, tests (flutter/engine#50540)
2024-02-12 30870216+gaaclarke@users.noreply.github.com [Impeller] deleted the old blur (flutter/engine#50470)
2024-02-12 kjlubick@users.noreply.github.com Update codec defines for Skia (flutter/engine#50554)

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

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
@auto-submit auto-submit bot mentioned this pull request Feb 12, 2024
Merged
auto-submit bot added a commit to flutter/flutter that referenced this pull request Feb 12, 2024
@auto-submit
Reverts "Roll Flutter Engine from 1c3ecee77350 to a19077503a0c (8 rev…
0377bc3 
…isions) (#143322)" (#143338)

Reverts #143322

Initiated by: zanderso

Reason for reverting: The tree is red.

Original PR Author: engine-flutter-autoroll

Reviewed By: {fluttergithubbot}

This change reverts the following previous change:
Original Description:

flutter/engine@1c3ecee...a190775

2024-02-12 737941+loic-sharma@users.noreply.github.com [Windows] Make the view own its EGL surface (flutter/engine#50421)
2024-02-12 skia-flutter-autoroll@skia.org Roll Dart SDK from 5c9e79e66364 to dca13675e65e (1 revision) (flutter/engine#50559)
2024-02-12 1889916+miguelfrde@users.noreply.github.com [fuchsia] Update Inspect library usage (flutter/engine#50467)
2024-02-12 zanderso@users.noreply.github.com Initializes RBE in the build config runner (flutter/engine#50543)
2024-02-12 tugorez@users.noreply.github.com Rename [ViewFocusDirection.backwards] to [ViewFocusDirection.backward] (flutter/engine#50533)
2024-02-12 dnfield@google.com Refactor NDK helpers some more, add methods for SurfaceControl/Transaction, tests (flutter/engine#50540)
2024-02-12 30870216+gaaclarke@users.noreply.github.com [Impeller] deleted the old blur (flutter/engine#50470)
2024-02-12 kjlubick@users.noreply.github.com Update codec defines for Skia (flutter/engine#50554)

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

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
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 13, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 13, 2024
@engine-flutter-autoroll
a19077503 [Windows] Make the view own its EGL surface (flutter/engine…
d39c7c6 
…#50421)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 13, 2024
@engine-flutter-autoroll
Manual roll Flutter Engine from 1c3ecee77350 to 920874bf2c1a (34 revi…
e94e406 
…sions) (#143380)

Manual roll Flutter Engine from 1c3ecee77350 to 920874bf2c1a (34 revisions)

Manual roll requested by jonahwilliams@google.com

Cannot build log URL because revision "920874bf2c1a" is invalid: Luci builds of "Linux Fuchsia FEMU" for 920874bf2c1a9ba2e9b7260b79ede8f1e26ea4f0 was FAILURE

2024-02-13 dnfield@google.com Do not use AChoreographer on 32 bit devices (flutter/engine#50586)
2024-02-13 zanderso@users.noreply.github.com Move libpng to //flutter/third_party/libpng (flutter/engine#50571)
2024-02-13 skia-flutter-autoroll@skia.org Roll Skia from b150107d536d to aaed347b2dbb (2 revisions) (flutter/engine#50595)
2024-02-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 8d3a8d85c883 to 105312b80125 (2 revisions) (flutter/engine#50593)
2024-02-13 skia-flutter-autoroll@skia.org Roll Skia from d503bc9c6e46 to b150107d536d (1 revision) (flutter/engine#50591)
2024-02-13 skia-flutter-autoroll@skia.org Roll Skia from a1d9b83034ac to d503bc9c6e46 (3 revisions) (flutter/engine#50587)
2024-02-13 30870216+gaaclarke@users.noreply.github.com [Impeller] cleaned up and removed golden test exceptions (flutter/engine#50572)
2024-02-13 skia-flutter-autoroll@skia.org Roll Skia from 17d00f9241b3 to a1d9b83034ac (2 revisions) (flutter/engine#50582)
2024-02-13 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from RVHAIKU50ogkiV-Lj... to l6mWjvlO1xJg5ZFKK... (flutter/engine#50579)
2024-02-13 skia-flutter-autoroll@skia.org Roll Dart SDK from 94f7dec34cef to 8d3a8d85c883 (1 revision) (flutter/engine#50577)
2024-02-13 matanlurey@users.noreply.github.com Add Impeller runtime options to Android `scenario_app` (flutter/engine#50487)
2024-02-13 skia-flutter-autoroll@skia.org Roll Skia from c464143dfaab to 17d00f9241b3 (1 revision) (flutter/engine#50576)
2024-02-13 skia-flutter-autoroll@skia.org Roll Skia from ecf2f622d0b3 to c464143dfaab (2 revisions) (flutter/engine#50574)
2024-02-12 jonahwilliams@google.com [Impeller] CommandPoolVK recycles command buffers too. (flutter/engine#50468)
2024-02-12 matanlurey@users.noreply.github.com Make `dart:ui#Paint` `final` to allow changes/disallow implementing. (flutter/engine#50557)
2024-02-12 robert.ancell@canonical.com Replace usage of an integer for GdkModifierType (flutter/engine#50481)
2024-02-12 robert.ancell@canonical.com Remove FlKeyEvent.dispose_origin and use GdkEvent type for origin (flutter/engine#50483)
2024-02-12 zanderso@users.noreply.github.com Move Linux impeller_unittests to linux_unopt (flutter/engine#50558)
2024-02-12 chinmaygarde@google.com [Impeller] Don't suppress outdated validation error. (flutter/engine#50568)
2024-02-12 chinmaygarde@google.com [Impeller] Remove references to moved impeller/image from README. (flutter/engine#50513)
2024-02-12 skia-flutter-autoroll@skia.org Roll Dart SDK from dca13675e65e to 94f7dec34cef (1 revision) (flutter/engine#50567)
2024-02-12 skia-flutter-autoroll@skia.org Roll Skia from f88eeb658358 to ecf2f622d0b3 (1 revision) (flutter/engine#50566)
2024-02-12 skia-flutter-autoroll@skia.org Roll Skia from 867c1e8390e2 to f88eeb658358 (5 revisions) (flutter/engine#50564)
2024-02-12 jason-simmons@users.noreply.github.com Roll vulkan-deps to 014f44e134a1de387791bffacc32ff9d8db71176 (flutter/engine#50515)
2024-02-12 skia-flutter-autoroll@skia.org Roll Skia from 4081496b7693 to 867c1e8390e2 (20 revisions) (flutter/engine#50562)
2024-02-12 jonahwilliams@google.com [Impeller] force render pass construction on all backends to ensure image layout is transitioned/render pass state setup. (flutter/engine#50539)
2024-02-12 737941+loic-sharma@users.noreply.github.com [Windows] Make the view own its EGL surface (flutter/engine#50421)
2024-02-12 skia-flutter-autoroll@skia.org Roll Dart SDK from 5c9e79e66364 to dca13675e65e (1 revision) (flutter/engine#50559)
2024-02-12 1889916+miguelfrde@users.noreply.github.com [fuchsia] Update Inspect library usage (flutter/engine#50467)
2024-02-12 zanderso@users.noreply.github.com Initializes RBE in the build config runner (flutter/engine#50543)
2024-02-12 tugorez@users.noreply.github.com Rename [ViewFocusDirection.backwards] to [ViewFocusDirection.backward] (flutter/engine#50533)
2024-02-12 dnfield@google.com Refactor NDK helpers some more, add methods for SurfaceControl/Transaction, tests (flutter/engine#50540)
2024-02-12 30870216+gaaclarke@users.noreply.github.com [Impeller] deleted the old blur (flutter/engine#50470)
2024-02-12 kjlubick@users.noreply.github.com Update codec defines for Skia (flutter/engine#50554)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from RVHAIKU50ogk to l6mWjvlO1xJg

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 jonahwilliams@google.com,jsimmons@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chinmaygarde chinmaygarde chinmaygarde left review comments

@yaakovschectman yaakovschectman yaakovschectman approved these changes

@cbracken cbracken Awaiting requested review from cbracken

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 @chinmaygarde @yaakovschectman