Reland "[Windows] Move to FlutterCompositor for rendering" #49726

loic-sharma · 2024-01-11T19:42:52Z

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

Reland (again)

#49262 was reverted as it regressed package:material_floating_search_bar_2. See: flutter/flutter#140828

This pull request is split into the following commits:

d337378 is the previous reland pull request, unchanged
e866af0 disables the scissor test before blitting the framebuffer, allowing us to "force" copy the framebuffer's contents by ignoring scissoring values

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.

shell/platform/windows/compositor_opengl.cc

loic-sharma · 2024-01-11T20:25:37Z

shell/platform/windows/compositor_opengl.cc

+  gl_->Disable(GL_SCISSOR_TEST);
+  gl_->Disable(GL_DEPTH_TEST);
+  gl_->Disable(GL_STENCIL_TEST);


This is new: disable the scissor test

EDIT: Updated after addressing feedback

I don't think you need to worry about the depth/stencil state here. Just the scissor. The 4.6 spec says:

When values are written to the draw buffers, blit operations bypass most of the fragment pipeline. The only fragment operations which affect a blit are the pixel ownership test, the scissor test, and sRGB conversion (see section 17.3.7). Color, depth, and stencil masks (see section 17.4.2) are ignored.

Looks like you're right: the fix still works if we disable only the scissor test. Updated!

jonahwilliams

Not that it should block this change, but does Impeller still work with this configuration?

loic-sharma · 2024-01-12T00:58:20Z

@jonahwilliams It does not. Impeller invokes FlutterOpenGLRendererConfig.fbo_with_frame_info_callback, which now immediately exits on Windows since the engine should be rendering to a backing store instead of the window's surface

jonahwilliams · 2024-01-12T01:17:47Z

Thanks for checking! Can you file a bug for us with some information on what we need to update to make it work again?

cbracken

cbracken · 2024-01-12T02:18:50Z

impeller/renderer/backend/gles/blit_command_gles.cc

@@ -48,7 +48,7 @@ static std::optional<GLuint> ConfigureFBO(
  }

  if (gl.CheckFramebufferStatus(fbo_type) != GL_FRAMEBUFFER_COMPLETE) {
-    VALIDATION_LOG << "Could not create a complete frambuffer.";
+    VALIDATION_LOG << "Could not create a complete framebuffer.";


frambuffer: most, but not quite all, of a framebuffer. 🤣

shell/platform/linux/fl_backing_store_provider.cc

azchohfi · 2024-01-12T21:40:06Z

Would this break Windows 7 compatibility, or is this being used only if Win8+?
We might need to wait for flutter/flutter#140830

loic-sharma · 2024-01-12T21:48:34Z

@azchohfi For now we still fully support Windows 7. This PR doesn't use DirectComposition - is that your concern?

loic-sharma · 2024-01-12T22:05:26Z

@jonahwilliams Sure! Opened: flutter/flutter#141482.

azchohfi · 2024-01-12T22:16:35Z

@azchohfi For now we still fully support Windows 7. This PR doesn't use DirectComposition - is that your concern?

I completely misread the PR, ignore my comment. My bad.

This reverts commit e95e123.

…lutter/engine#49726)

…141651) flutter/engine@d4b6b7e...021a5ff 2024-01-16 ditman@gmail.com [web] Leave blob URLs untouched in TT policy. (flutter/engine#49782) 2024-01-16 jason-simmons@users.noreply.github.com [Impeller] Fix a race between SwapchainImplVK::Present and WaitForFence (flutter/engine#49777) 2024-01-16 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49726) 2024-01-16 jason-simmons@users.noreply.github.com [Impeller] Add an API for sampling strictly within the bounds of the source rect in DrawImageRect (flutter/engine#49696) 2024-01-16 jonahwilliams@google.com [Impeller] Encode directly to command buffer for Vulkan. (flutter/engine#49780) 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,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

…isions)" (#141659) Reverts #141651 Initiated by: jonahwilliams This change reverts the following previous change: Original Description: flutter/engine@d4b6b7e...021a5ff 2024-01-16 ditman@gmail.com [web] Leave blob URLs untouched in TT policy. (flutter/engine#49782) 2024-01-16 jason-simmons@users.noreply.github.com [Impeller] Fix a race between SwapchainImplVK::Present and WaitForFence (flutter/engine#49777) 2024-01-16 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49726) 2024-01-16 jason-simmons@users.noreply.github.com [Impeller] Add an API for sampling strictly within the bounds of the source rect in DrawImageRect (flutter/engine#49696) 2024-01-16 jonahwilliams@google.com [Impeller] Encode directly to command buffer for Vulkan. (flutter/engine#49780) 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,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

…lutter/engine#49726)

…141664) flutter/engine@d4b6b7e...1382ff7 2024-01-16 magder@google.com Remove iOS 12 availability checks (flutter/engine#49771) 2024-01-16 30870216+gaaclarke@users.noreply.github.com [Impeller] generate mipmaps for imagefilters (flutter/engine#49794) 2024-01-16 ditman@gmail.com [web] Leave blob URLs untouched in TT policy. (flutter/engine#49782) 2024-01-16 jason-simmons@users.noreply.github.com [Impeller] Fix a race between SwapchainImplVK::Present and WaitForFence (flutter/engine#49777) 2024-01-16 737941+loic-sharma@users.noreply.github.com Reland "[Windows] Move to FlutterCompositor for rendering" (flutter/engine#49726) 2024-01-16 jason-simmons@users.noreply.github.com [Impeller] Add an API for sampling strictly within the bounds of the source rect in DrawImageRect (flutter/engine#49696) 2024-01-16 jonahwilliams@google.com [Impeller] Encode directly to command buffer for Vulkan. (flutter/engine#49780) 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,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

knopp · 2024-03-21T16:29:06Z

Just to be clear, this change introduced an extra blit that's always there? What exactly is the reason for it? Why not use the back framebuffer?

loic-sharma · 2024-03-25T15:20:56Z

@knopp and I talked on Discord, but I'll capture our conversation here as well in case other folks have the same question.

Currently the engine's backing stores are interchangeable if they have the same size. The intermediary textures plus extra blit allow us to render to multiple views while keeping this interchangeable model, with the trade-off of higher memory usage.

We plan to have a conversation with the engine team to see if we can "tie" backing stores to a view. We expect this would require some minor embedder API changes. See: http://flutter.dev/go/backing-stores-for-multi-view-partial-repaint

github-actions bot added affects: desktop platform-linux platform-windows e: impeller labels Jan 11, 2024

loic-sharma force-pushed the windows_compositor_v3 branch from 934fa88 to 0b7d8b8 Compare January 11, 2024 20:12

loic-sharma commented Jan 11, 2024

View reviewed changes

shell/platform/windows/compositor_opengl.cc Outdated Show resolved Hide resolved

loic-sharma commented Jan 11, 2024

View reviewed changes

jonahwilliams reviewed Jan 11, 2024

View reviewed changes

loic-sharma force-pushed the windows_compositor_v3 branch from 096c847 to fb075db Compare January 11, 2024 21:27

loic-sharma marked this pull request as ready for review January 12, 2024 00:59

loic-sharma requested review from cbracken, dkwingsmt and yaakovschectman January 12, 2024 00:59

cbracken approved these changes Jan 12, 2024

View reviewed changes

yaakovschectman approved these changes Jan 12, 2024

View reviewed changes

loic-sharma mentioned this pull request Jan 12, 2024

[Impeller] Fix Windows support (broken by flutter/engine#49726) flutter/flutter#141482

Closed

loic-sharma force-pushed the windows_compositor_v3 branch from ff7d3b2 to 7c8da79 Compare January 16, 2024 21:22

Reland "[Windows] Move to FlutterCompositor for rendering"

d337378

This reverts commit e95e123.

loic-sharma force-pushed the windows_compositor_v3 branch from 7c8da79 to e866af0 Compare January 16, 2024 21:28

Add fix: disable scissor test before blit

e866af0

loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 16, 2024

auto-submit bot merged commit 9525c1a into flutter:main Jan 16, 2024
29 checks passed

engine-flutter-autoroll mentioned this pull request Jan 16, 2024

Roll Flutter Engine from d4b6b7ec8e48 to 021a5ff5eae6 (5 revisions) flutter/flutter#141651

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 16, 2024

9525c1a1b Reland "[Windows] Move to FlutterCompositor for rendering" (f…

21c3fe5

…lutter/engine#49726)

auto-submit bot mentioned this pull request Jan 17, 2024

Reverts "Roll Flutter Engine from d4b6b7ec8e48 to 021a5ff5eae6 (5 revisions)" flutter/flutter#141659

Merged

engine-flutter-autoroll mentioned this pull request Jan 17, 2024

Roll Flutter Engine from d4b6b7ec8e48 to 1382ff79dd6d (7 revisions) flutter/flutter#141663

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 17, 2024

9525c1a1b Reland "[Windows] Move to FlutterCompositor for rendering" (f…

0fb5239

…lutter/engine#49726)

engine-flutter-autoroll mentioned this pull request Jan 17, 2024

Roll Flutter Engine from d4b6b7ec8e48 to 1382ff79dd6d (7 revisions) flutter/flutter#141664

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 17, 2024

9525c1a1b Reland "[Windows] Move to FlutterCompositor for rendering" (f…

59ef12c

…lutter/engine#49726)

This was referenced Jan 17, 2024

[Windows] Compositor rendering regresses apps with faded backgrounds flutter/flutter#140828

Closed

[Windows] Move to FlutterCompositor for rendering flutter/flutter#128904

Closed

loic-sharma mentioned this pull request Jun 3, 2024

Use glBlitFramebuffer when rendering #53080

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland "[Windows] Move to FlutterCompositor for rendering" #49726

Reland "[Windows] Move to FlutterCompositor for rendering" #49726

loic-sharma commented Jan 11, 2024 •

edited

Loading

loic-sharma Jan 11, 2024 •

edited

Loading

bdero Jan 12, 2024 •

edited

Loading

loic-sharma Jan 16, 2024 •

edited

Loading

jonahwilliams left a comment

loic-sharma commented Jan 12, 2024 •

edited

Loading

jonahwilliams commented Jan 12, 2024

cbracken left a comment

cbracken Jan 12, 2024

azchohfi commented Jan 12, 2024

loic-sharma commented Jan 12, 2024

loic-sharma commented Jan 12, 2024

azchohfi commented Jan 12, 2024

knopp commented Mar 21, 2024

loic-sharma commented Mar 25, 2024 •

edited

Loading

Reland "[Windows] Move to FlutterCompositor for rendering" #49726

Reland "[Windows] Move to FlutterCompositor for rendering" #49726

Conversation

loic-sharma commented Jan 11, 2024 • edited Loading

Original pull request description

Reland (again)

Pre-launch Checklist

loic-sharma Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

bdero Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

loic-sharma Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

loic-sharma commented Jan 12, 2024 • edited Loading

jonahwilliams commented Jan 12, 2024

cbracken left a comment

Choose a reason for hiding this comment

cbracken Jan 12, 2024

Choose a reason for hiding this comment

azchohfi commented Jan 12, 2024

loic-sharma commented Jan 12, 2024

loic-sharma commented Jan 12, 2024

azchohfi commented Jan 12, 2024

knopp commented Mar 21, 2024

loic-sharma commented Mar 25, 2024 • edited Loading

loic-sharma commented Jan 11, 2024 •

edited

Loading

loic-sharma Jan 11, 2024 •

edited

Loading

bdero Jan 12, 2024 •

edited

Loading

loic-sharma Jan 16, 2024 •

edited

Loading

loic-sharma commented Jan 12, 2024 •

edited

Loading

loic-sharma commented Mar 25, 2024 •

edited

Loading