[Windows] Move to `FlutterCompositor` for rendering by loic-sharma · Pull Request #48849 · flutter-team-archive/engine

loic-sharma · 2023-12-08T21:37:54Z

This migrates the Windows embedder to FlutterCompositor so that the engine renders off-screen to a framebuffer instead of directly onto the window's surface. This will allow us to support platform views and multiple views on Windows.

Tests...

Verify OpenGL compositor's raster time isn't regressed and memory increase is reasonable
Software compositor's raster time and memory isn't regressed

Test device configurations

Windows 11 (hardware acceleration enabled/disabled)
Windows Arm64 (hardware acceleration enabled/disabled)
Windows 7 (hardware acceleration enabled/disabled, DWM enabled/disabled)

Addresses flutter/flutter#128904

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 · 2023-12-09T00:13:56Z

+CompositorOpenGL::CompositorOpenGL(FlutterWindowsEngine* engine,
+                                   impeller::ProcTableGLES::Resolver resolver)
+    : engine_(engine), resolver_(resolver) {}
+


The CompositorOpenGL mirrors Linux's backing store provider: https://github.com/flutter/engine/blob/101396fd3b8216acea95f09fa7c3fc8e97b38ab8/shell/platform/linux/fl_backing_store_provider.cc#L19-L102

loic-sharma · 2023-12-09T00:18:09Z

  deps = [
    ":flutter_windows_headers",
    "//flutter/fml:fml",
+    "//flutter/impeller/renderer/backend/gles",


The Windows embedder uses Impeller's ProcTableGLES and DescriptionGLES to resolve OpenGLES functions and determine the GL version and extensions. This was the approach recommended by Impeller folks: https://discord.com/channels/608014603317936148/1173336353187041380/1182110838849536040

Also note that Windows has a GlProcTable which is a worse version of Impeller's ProcTableGLES. I was hoping to converge everything to ProcTableGLES, however, ProcTableGLES's initialization requires that the GL context is current. This prevents us from using ProcTableGLES on the platform thread as it ideally shouldn't make the context current. We'll want to clean this up somehow in the future...

why does impeller require the context to be current - do we combine creation of the proc table with looking up gl state? Could we split that functionality out of proc table so you could use it exclusively?

do we combine creation of the proc table with looking up gl state?

Yes. The proc table uses the DescriptionGLES to determine capabilities and determine supported extension functions, and the DescriptionGLES constructor fails if the context isn't current.

Could we split that functionality out of proc table so you could use it exclusively?

I think so. We could have a "raw" proc table that can be initialized without a GL context, does not have the description/capabilities, and includes potentially unsupported extension functions. The "safe" proc table would load the description/capabilities and remove unsupported functions. This might require a little experimentation to get the API right.

yaakovschectman

LGTM as long as we're going with the C stdlib memory management

yaakovschectman · 2023-12-12T18:52:01Z

+
+bool CompositorSoftware::Present(const FlutterLayer** layers,
+                                 size_t layers_count) {
+  FML_DCHECK(layers_count == 1);


This reminds me:
This wouldn't be in this PR, but how about iterating through however many backing store layers there are and blending them per-pixel-alpha so we can composite an arbitrary number of layers? I'm doing something similar on my local branch working with platform views. If that sounds good, I can start a PR once this lands to add that here.

That sounds reasonable but before that PR we'd first want a design doc shared and reviewed by everyone :)

cbracken

Overall looks great -- just a few nits!

jonahwilliams

Usage looks fine to me. But if we can make an adjustment to proc table to make it more useful we should do that.

See: #48849 (comment)

…team-archive/engine#48849)

flutter-team-archive/engine@9f7004e...45b95f2 2023-12-13 737941+loic-sharma@users.noreply.github.com [Windows] Move to `FlutterCompositor` for rendering (flutter-team-archive/engine#48849) 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 jsimmons@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

loic-sharma · 2023-12-14T04:59:26Z

Reverting. The framework tree became red on Windows run_debug_test_windows and Windows_arm64 run_debug_test_windows once this commit rolled to the framework.

Example failure: https://ci.chromium.org/ui/p/flutter/builders/prod/Windows%20run_debug_test_windows/5826/overview

This reverts commit 45b95f2.

Reverts #48849 Initiated by: loic-sharma This change reverts the following previous change: Original Description: This migrates the Windows embedder to `FlutterCompositor` so that the engine renders off-screen to a framebuffer instead of directly onto the window's surface. This will allow us to support platform views and multiple views on Windows. <details> <summary>Tests...</summary> * Verify OpenGL compositor's raster time isn't regressed and memory increase is reasonable * Software compositor's raster time and memory isn't regressed Test device configurations * [x] Windows 11 (hardware acceleration enabled/disabled) * [x] Windows Arm64 (hardware acceleration enabled/disabled) * [x] Windows 7 (hardware acceleration enabled/disabled, DWM enabled/disabled) </details> Addresses flutter/flutter#128904 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…140123) Reverts #140108 Initiated by: loic-sharma This change reverts the following previous change: Original Description: flutter-team-archive/engine@9f7004e...45b95f2 2023-12-13 737941+loic-sharma@users.noreply.github.com [Windows] Move to `FlutterCompositor` for rendering (flutter-team-archive/engine#48849) 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 jsimmons@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

…team-archive/engine#48849)

flutter-team-archive/engine@9f7004e...923f9e2 2023-12-14 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Windows] Move to `FlutterCompositor` for rendering" (flutter-team-archive/engine#49015) 2023-12-14 magder@google.com Add xcprivacy privacy manifest to iOS framework (flutter-team-archive/engine#48951) 2023-12-14 30870216+gaaclarke@users.noreply.github.com [Impeller] Made the new blur support 1D blurs (flutter-team-archive/engine#49001) 2023-12-14 skia-flutter-autoroll@skia.org Roll Skia from 69c02c9d56b2 to 188515347032 (1 revision) (flutter-team-archive/engine#49005) 2023-12-14 bdero@google.com [Impeller] Add golden for clipped+transformed blur. (flutter-team-archive/engine#48886) 2023-12-14 bdero@google.com [Flutter GPU] Runtime shader import. (flutter-team-archive/engine#48875) 2023-12-13 737941+loic-sharma@users.noreply.github.com [Windows] Move to `FlutterCompositor` for rendering (flutter-team-archive/engine#48849) 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 jsimmons@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

wanjm · 2023-12-16T05:59:15Z

why it is reverted?

## Reland #48849 was reverted as it incorrectly expected to receive always 1 layer. However, the engine will present 0 layers on an ["empty" app](https://github.com/flutter/flutter/blob/6981fe6fd3aacb95bfc4a6c427ee5d493f43c5dc/dev/integration_tests/ui/lib/empty.dart#L8-L19). This pull request is split into two commits: 1. df604a1 is the original pull request, unchanged 2. c30b369 adds the ability to "clear" the view if the engine presents 0 layers ## Original pull request description This migrates the Windows embedder to `FlutterCompositor` so that the engine renders off-screen to a framebuffer instead of directly onto the window's surface. This will allow us to support platform views and multiple views on Windows. Addresses flutter/flutter#128904 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

github-actions Bot added affects: desktop platform-linux platform-windows e: impeller labels Dec 8, 2023

loic-sharma force-pushed the windows_renderer branch from b589838 to 9b70e4b Compare December 8, 2023 23:56

loic-sharma commented Dec 9, 2023

View reviewed changes

loic-sharma requested review from cbracken and yaakovschectman December 9, 2023 02:15

loic-sharma marked this pull request as ready for review December 9, 2023 02:15

yaakovschectman reviewed Dec 11, 2023

View reviewed changes

Comment thread shell/platform/windows/compositor_software.cc Outdated

yaakovschectman approved these changes Dec 11, 2023

View reviewed changes

yaakovschectman reviewed Dec 12, 2023

View reviewed changes

loic-sharma requested a review from dkwingsmt December 12, 2023 18:58