[Flutter GPU] Get the GLES backend/Windows working. #55694

bdero · 2024-10-07T10:29:37Z

Resolves flutter/flutter#156305.
Resolves flutter/flutter#145012.

Resolve pipelines and submit command buffers on the raster thread.
Don't use desktop GL shader variation on Windows.
Fix interpretation of array_elements.
Fix texture binding metadata.

Gets Flutter GPU working on Windows!

flutter-dashboard · 2024-10-07T10:29:41Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

jonahwilliams · 2024-10-07T16:15:31Z

lib/gpu/command_buffer.cc

    encodable->EncodeCommands();
  }
+
+  if (context_->GetBackendType() == impeller::Context::BackendType::kOpenGLES) {


I thought the whole point of the GLES backend's task reactor design was that we could call these methods from any thread?

blocking on the raster task runner for like this seems pretty bad, you're essentially de-pipeling .

It's funky that the submit fails if done outside the raster thread. I need to take a closer look at this. If I'm not holding it wrong then fixing the API to defer the submit in the reactor shouldn't be difficult.

Okay, so submitting the command buffer on GLES just flushes the reactor, which makes sense. I changed this to not synchronize with the raster task. So this doesn't block the UI thread anymore.

jonahwilliams · 2024-10-07T16:17:09Z

lib/gpu/render_pass.cc

-      context.GetPipelineLibrary()->GetPipeline(pipeline_desc).Get();
+  std::shared_ptr<impeller::Pipeline<impeller::PipelineDescriptor>> pipeline;
+
+  if (context.GetBackendType() == impeller::Context::BackendType::kOpenGLES) {


Similar question here, are we misssing some thread handling in the gles shader library

I think the reactor was WAI here. When attempting to create a pipeline from the UI thread, the pipeline creation task was getting stored in the reactor because GL calls need to be done on the raster thread. The problem is I need the pipeline right away, and so I need to post a raster task.

It's excessive to post a raster task every time we do a pipeline lookup like this. We only need to do it when we know a new pipeline needs to be created, but the API is lacking at the moment.

Do you actually need the pipeline object, or could we store the pipeline future?

Alternatively, consider making this API future returning so we don't end up with weird thread pinning issues.

Yeah we need the pipeline object immediately. This happens during RenderPass.draw. I don't think we should make it async. But we could prevent this raster task in cases where a new pipeline doesn't actually need to be created.

I changed this to not post a raster task unless we know a new pipeline actually needs to be created.

jonahwilliams

I'd like @chinmaygarde to take a look since he's more familiar with the threading issues on GLES. Ideally we'd have them fixed such that we don't need to post messages to different threads to use this backend.

…asks for creating pipelines when a pipeline needs to be created

jonahwilliams · 2024-10-08T23:08:28Z

lib/gpu/render_pass.cc

+
+    // Note that this branch is only called if a new pipeline actually needs to
+    // be built.
    auto dart_state = flutter::UIDartState::Current();


So I think this is a problem in the HAL design. We expect to be able to have a pipeline synchronously for encoding but 1) we can only get the pipeline on a particular thread for GLES and 2) we don't actually need the pipeline until we render - which always happens on the react thread.

I think it would be something to punt to a new PR, but I would consider change the HAL design such that SetPipeline accepted the pipleine future. The Vulkan/Metal implementation would call waitandget while the GLES implementation would defer that until encoding.

Yeah I completely agree with this. Started filing an issue for it but you beat me to it. :)

jonahwilliams

LGTM

Filled flutter/flutter#156441

…r/engine#55694)

…156478) flutter/engine@db0c0b7...4a97943 2024-10-09 [email protected] Roll Fuchsia Linux SDK from TlU-It6X_ZLrNqMjW... to xGr5ZkxX3CajAY1xu... (flutter/engine#55770) 2024-10-09 [email protected] Roll Skia from a077e78e531f to e0bb55353b27 (3 revisions) (flutter/engine#55768) 2024-10-09 [email protected] [Impeller] libImpeller: Allow fetching OpenGL texture handle. (flutter/engine#55753) 2024-10-09 [email protected] Roll Fuchsia Test Scripts from jCde9sMKJ3YAdG2DH... to _fkA2GjLQH4bc_n2p... (flutter/engine#55762) 2024-10-09 [email protected] Roll Skia from 701b6e4b4bc4 to a077e78e531f (3 revisions) (flutter/engine#55761) 2024-10-09 [email protected] Release`onTrimMemoryListener` after `ImageReaderSurfaceProducer` released. (flutter/engine#55760) 2024-10-09 [email protected] [Flutter GPU] Get the GLES backend/Windows working. (flutter/engine#55694) 2024-10-08 [email protected] [canvaskit] Fix incorrect clipping with Opacity scene layer (flutter/engine#55751) 2024-10-08 [email protected] Refactor multi-file build parsing into a single `BuildPlan` class. (flutter/engine#55720) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from TlU-It6X_ZLr to xGr5ZkxX3Caj 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 [email protected],[email protected] 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

…5694) Resolves flutter#156305. * Resolve pipelines and submit command buffers on the raster thread. * Don't use desktop GL shader variation on Windows. * Fix interpretation of `array_elements`. * Fix texture binding metadata. Gets Flutter GPU working on Windows! ![image](https://github.com/user-attachments/assets/9eecb67f-a980-4556-8060-b0c947713534) ![image](https://github.com/user-attachments/assets/c8e2071f-e7c0-411c-8f37-e1f3037916f4)

[Flutter GPU] Get the GLES backend/Windows working.

a2c60a0

bdero added the flutter-gpu label Oct 7, 2024

bdero requested review from chinmaygarde and jonahwilliams October 7, 2024 10:29

jonahwilliams reviewed Oct 7, 2024

View reviewed changes

bdero mentioned this pull request Oct 8, 2024

Bug: compiling vsproj file bdero/flutter_scene#30

Closed

Don't sync with the raster task on submission, and only post raster t…

347bbda

…asks for creating pipelines when a pipeline needs to be created

github-actions bot added the e: impeller label Oct 8, 2024

bdero requested a review from jonahwilliams October 8, 2024 23:03

jonahwilliams reviewed Oct 8, 2024

View reviewed changes

jonahwilliams mentioned this pull request Oct 8, 2024

[Impeller] RenderPass::SetPipeline should accept a pipeline future and not the actual pipleine flutter/flutter#156441

Open

jonahwilliams approved these changes Oct 8, 2024

View reviewed changes

bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 9, 2024

auto-submit bot merged commit db49445 into flutter:main Oct 9, 2024

engine-flutter-autoroll mentioned this pull request Oct 9, 2024

Roll Flutter Engine from db0c0b7ca428 to 4a97943e0556 (9 revisions) flutter/flutter#156478

Merged

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

db4944505 [Flutter GPU] Get the GLES backend/Windows working. (flutte…

a24bd31

…r/engine#55694)

bdero mentioned this pull request Oct 16, 2024

[Flutter GPU] OpenGLES support. flutter/flutter#145012

Closed

[Flutter GPU] Get the GLES backend/Windows working. #55694

[Flutter GPU] Get the GLES backend/Windows working. #55694

Uh oh!

Conversation

bdero commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Oct 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdero Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bdero commented Oct 7, 2024 •

edited

Loading

bdero Oct 7, 2024 •

edited

Loading