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.

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

[Windows] Move to FlutterCompositor for rendering #48849

Merged
merged 11 commits into from
Dec 13, 2023

Conversation

loic-sharma
Copy link
Member

@loic-sharma loic-sharma commented Dec 8, 2023

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.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The CompositorOpenGL mirrors Linux's backing store provider:

static void fl_backing_store_provider_dispose(GObject* object) {
FlBackingStoreProvider* self = FL_BACKING_STORE_PROVIDER(object);
glDeleteFramebuffers(1, &self->framebuffer_id);
glDeleteTextures(1, &self->texture_id);
G_OBJECT_CLASS(fl_backing_store_provider_parent_class)->dispose(object);
}
static void fl_backing_store_provider_class_init(
FlBackingStoreProviderClass* klass) {
G_OBJECT_CLASS(klass)->dispose = fl_backing_store_provider_dispose;
}
static void fl_backing_store_provider_init(FlBackingStoreProvider* self) {}
FlBackingStoreProvider* fl_backing_store_provider_new(int width, int height) {
FlBackingStoreProvider* provider = FL_BACKING_STORE_PROVIDER(
g_object_new(fl_backing_store_provider_get_type(), nullptr));
provider->geometry = {
.x = 0,
.y = 0,
.width = width,
.height = height,
};
glGenTextures(1, &provider->texture_id);
glGenFramebuffers(1, &provider->framebuffer_id);
glBindFramebuffer(GL_FRAMEBUFFER, provider->framebuffer_id);
glBindTexture(GL_TEXTURE_2D, provider->texture_id);
glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, width, height, 0, GL_RGBA,
GL_UNSIGNED_BYTE, NULL);
glBindTexture(GL_TEXTURE_2D, 0);
glFramebufferTexture2D(GL_FRAMEBUFFER_EXT, GL_COLOR_ATTACHMENT0_EXT,
GL_TEXTURE_2D, provider->texture_id, 0);
return provider;
}
uint32_t fl_backing_store_provider_get_gl_framebuffer_id(
FlBackingStoreProvider* self) {
return self->framebuffer_id;
}
uint32_t fl_backing_store_provider_get_gl_texture_id(
FlBackingStoreProvider* self) {
return self->texture_id;
}
uint32_t fl_backing_store_provider_get_gl_target(FlBackingStoreProvider* self) {
return GL_TEXTURE_2D;
}
uint32_t fl_backing_store_provider_get_gl_format(FlBackingStoreProvider* self) {
// Flutter defines SK_R32_SHIFT=16, so SK_PMCOLOR_BYTE_ORDER should be BGRA.
// In Linux kN32_SkColorType is assumed to be kBGRA_8888_SkColorType.
// So we must choose a valid gl format to be compatible with surface format
// BGRA8.
// Following logics are copied from Skia GrGLCaps.cpp.
if (epoxy_is_desktop_gl()) {
// For OpenGL.
if (epoxy_gl_version() >= 12 || epoxy_has_gl_extension("GL_EXT_bgra")) {
return GL_RGBA8;
}
} else {
// For OpenGL ES.
if (epoxy_has_gl_extension("GL_EXT_texture_format_BGRA8888") ||
(epoxy_has_gl_extension("GL_APPLE_texture_format_BGRA8888") &&
epoxy_gl_version() >= 30)) {
return GL_BGRA8_EXT;
}
}
g_critical("Failed to determine valid GL format for Flutter rendering");
return GL_RGBA8;
}

@@ -134,6 +139,7 @@ source_set("flutter_windows_source") {
deps = [
":flutter_windows_headers",
"//flutter/fml:fml",
"//flutter/impeller/renderer/backend/gles",
Copy link
Member Author

@loic-sharma loic-sharma Dec 9, 2023

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@loic-sharma loic-sharma Dec 13, 2023

Choose a reason for hiding this comment

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

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.

@loic-sharma loic-sharma marked this pull request as ready for review December 9, 2023 02:15
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 as long as we're going with the C stdlib memory management


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

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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.

Overall looks great -- just a few nits!

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

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

@loic-sharma loic-sharma force-pushed the windows_renderer branch 2 times, most recently from 4bea4a5 to aea1725 Compare December 13, 2023 18:24
@loic-sharma loic-sharma added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 13, 2023
@auto-submit auto-submit bot merged commit 45b95f2 into flutter:main Dec 13, 2023
28 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 14, 2023
…140108)

flutter/engine@9f7004e...45b95f2

2023-12-13 737941+loic-sharma@users.noreply.github.com [Windows] Move to `FlutterCompositor` for rendering (flutter/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
Copy link
Member Author

loic-sharma commented Dec 14, 2023

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

@loic-sharma loic-sharma added the revert Label used to revert changes in a closed and merged pull request. label Dec 14, 2023
auto-submit bot pushed a commit that referenced this pull request Dec 14, 2023
@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Dec 14, 2023
auto-submit bot added a commit that referenced this pull request Dec 14, 2023
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
auto-submit bot added a commit to flutter/flutter that referenced this pull request Dec 14, 2023
…ision)" (#140123)

Reverts #140108
Initiated by: loic-sharma
This change reverts the following previous change:
Original Description:

flutter/engine@9f7004e...45b95f2

2023-12-13 737941+loic-sharma@users.noreply.github.com [Windows] Move to `FlutterCompositor` for rendering (flutter/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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 14, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Dec 14, 2023
…140130)

flutter/engine@9f7004e...923f9e2

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

wanjm commented Dec 16, 2023

why it is reverted?

auto-submit bot pushed a commit that referenced this pull request Dec 21, 2023
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants