[Impeller] Implement OpenGL to Vulkan texture trampolining. #53966

chinmaygarde · 2024-07-17T17:54:12Z

This decouples the Impeller on-by-default effort from the release schedule and plugin migrations.

The plugin migration documented in go/impeller-plugin-migration is still recommended and facilitates zero-copy texture transfers between OpenGL and Vulkan. To recap, the plugin migration is to move away from the OpenGL-only SurfaceTexture APIs in the plugin interface.

This patch facilitates rendering OpenGL textures in a Vulkan renderer using texture trampolining using a single device-device transfer on all devices that support Impeller using the Vulkan renderer.

The performance of this approach is more than acceptable but at the cost of an additional texture allocation and will serve as a fallback to the for any remaining unmigrated plugins (all first-party plugins will already be migrated when the Impeller is on by default and we are following up on the migration of the major third-party plugins as well).

This is a straight improvement to the current state of things were unmigrated plugins will render an empty quad.

chinmaygarde · 2024-07-19T22:48:06Z

The FD based approach has been replaced with the AHB based one.

chinmaygarde · 2024-07-19T22:49:17Z

I have a couple of more improvements planned around the synchronization and some more testing with landscape natural devices, but this should work fine and is good for a review now.

impeller/toolkit/glvk/proc_table.h

jonahwilliams · 2024-07-21T03:47:52Z

impeller/toolkit/glvk/trampoline.cc

+precision mediump float;
+
+uniform samplerExternalOES uTexture;
+uniform mat4 uSTTransformation;


dumb question, what does ST stand for here?

I am pretty sure I have used (u, v) and (s, t) interchangeably for a long long time. But, I think there is a very subtle difference between the two. The spec. mentions (s, t) when using coordinates that sample from a texture. But these need not be in the 0.0 to 1.0 range. Depending on the wrap mode, the conversion between (s, t) and (u, v) may end up differently. For instance, 1.25 as s may end up being 0.25 u in repeat-wrap or 0.75 u in mirror-wrap. However, if you use (s, t) for say deriving the min or mag filter in the implementation, things might go very wrong. Hence, I think, the distinction. While for us, needing to be this specific is usually being pedantic.

The docs for getTransformMatrix mentioned (s, t) which I am using directly and am just depending on the wrap modes to do the rest. So I just use the same terminology. But, I am never giving the operation coordinates outside the unit range pre-transformation and the docs say the contents post transformation outside the range are undefined. So I am pretty sure the coordinates post transformation won't be outside the unit range. So perhaps UV is also a fine terminology. Would you like me to use that instead to avoid confusion?

Speaking of wrap modes, I realized I should at least default it to something instead of depending on whatever Android specified on the bind. Will fix that bit.

I renamed it to UV since that seems be our convention.

jonahwilliams · 2024-07-21T03:49:40Z

impeller/toolkit/glvk/trampoline.cc

+  // Theoretically, this does nothing because the surface is a 1x1 pbuffer
+  // surface. But frame capture tools use this to denote a frame boundary in
+  // OpenGL. So add this as a debugging aid anyway.
+  eglSwapBuffers(egl_display_->GetHandle(), egl_surface_->GetHandle());


Just curious, how does this end up helping debugging?

Back when I was trying to debug texture coordinates, I wanted to see if my render-to-texture was bad in OpenGL or the sampling was in Vulkan. Mali Graphics Analyzer only allows displaying the contents of textures and intermediate framebuffers at frame boundaries. And while rendering to a texture in OpenGL, without an eglSwapBuffers, the tool thought that the first frame was never done because there was no swap. So thousands of commands kept getting captured as being part of the "frame". The tool was just waiting indefinitely.

Adding this innocuous call made the captures work and nicely separated the OpenGL workloads from the Vulkan ones. But, in terms of actual work done, this is a no-op.

impeller/toolkit/glvk/trampoline.h

shell/platform/android/surface_texture_external_texture.h

jonahwilliams · 2024-07-21T03:57:29Z

shell/platform/android/surface_texture_external_texture_vk_impeller.cc

+SurfaceTextureExternalTextureVKImpeller::
+    ~SurfaceTextureExternalTextureVKImpeller() = default;
+
+static bool SetTextureLayoutSync(const ContextVK& context,


For my understanding: we're blocking on the layout transitions because the Vulkan/OpenGL interop does not include an ability to sequence these barriers, so we must do it on the CPU?

Vulkan/OpenGL interop does not include an ability to sequence these barriers

Am a bit skill-checked here ATM and reading up. An unrelated item in the spec. only mentioned EGLSync as a possible means of synchronization. And I believe you can create sync objects from FDs that I can import into Vulkan.

So there may be a way and I can wire up in a later patch as I read up more. But this works and works without an extension. So I figured this is good enough for a first pass.

The second layout update doesn't need to be have a sync wait either BTW as our barriers later can do the same.

Figured I'd submit what I have that works. But, the wait is pretty icky I agree.

jonahwilliams · 2024-07-21T03:59:02Z

impeller/toolkit/glvk/trampoline.cc

+
+precision mediump float;
+
+uniform samplerExternalOES uTexture;


Is it correct/possible to bind a texture that isn't an OES texture to this uniform?

My understanding was that surface textures would never give us non external textures. Is that not the case?

Oh, you meant in terms of the trampoline being a general purpose utility. Thats a good point, I can patch it to work with both. It will probably require another shader but thats fine.

Its possible to do something like create a SurfaceTexture, grab the surface from that, lock canvas and draw with the Android canvas, then give that SurfaceTexture to flutter. That would not be an oes texture.

TBH, I was just following what we did with Skia in GL and that is OES only.

engine/shell/platform/android/surface_texture_external_texture_gl_skia.cc

Line 43 in 207f088

GrGLTextureInfo textureInfo = {GL_TEXTURE_EXTERNAL_OES, texture_name_,

Its not OES only though, whatever that configuration happens to say. Perhaps the oes texture binding is just very permissive?

jonahwilliams · 2024-07-21T04:02:05Z

Amazing, it works great on the camera plugin. I need to try a non-oes backed texture example too

chinmaygarde · 2024-07-21T18:32:20Z

I may have missed how you'd get a non-oes-backed texture using the current API. But if you show me how, the fix is easy to apply and verify.

chinmaygarde · 2024-07-21T21:52:38Z

Ok, I got rid of the second sync wait and also patched the sampling modes to match Skia (the defaults were good but now they are explicitly specified).

jonahwilliams · 2024-07-22T15:48:32Z

We should have the ability to turn on some of the previously disabled scenario app tests. There should be an option to force surface texture w/ "forceSurfaceProducerSurfaceTexture"

chinmaygarde · 2024-07-22T17:10:40Z

turn on some of the previously disabled scenario app tests

Good idea. Wilco. Getting rid of

engine/testing/scenario_app/bin/utils/options.dart

Line 49 in e7b5998

// Cannot use forceSurfaceProducerSurfaceTexture with Impeller+Vulkan.

and checking.

chinmaygarde · 2024-07-22T17:29:48Z

I removed the implicit defaults and flag gates on surface producer. Does it now even make sense to have that flag? Since we have tests for both and Impeller should work in either case.

jonahwilliams · 2024-07-22T17:31:41Z

I think we need the flag to ensure we can test all combinations regardless of the actual API level.

chinmaygarde · 2024-07-22T18:00:13Z

I added a variant that uses surface texture to the Vulkan scenario app tests with Impeller. I think //flutter/ci/builders/linux_android_emulator.json is the right spot. But not sure. The tests runs locally.

flutter-dashboard · 2024-07-22T18:53:10Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #53966 at sha b090740

This decouples the Impeller on-by-default effort from the release schedule and plugin migrations. The plugin migration documented in [go/impeller-plugin-migration][plugin-migration] is still recommended and facilitates zero-copy texture transfers between OpenGL and Vulkan. To recap, the plugin migration is to move away from the OpenGL-only SurfaceTexture APIs in the plugin interface. This patch facilitates rendering OpenGL textures in a Vulkan renderer using texture trampolining using a single device-device transfer on all devices that support Impeller using the Vulkan renderer. The performance of this approach is more than acceptable but at the cost of an additional texture allocation and will serve as a fallback to the for any remaining unmigrated plugins (all first-party plugins will already be migrated when the Impeller is on by default and we are following up on the migration of the major third-party plugins as well). [plugin-migration]: https://docs.flutter.dev/release/breaking-changes/android-surface-plugins

flutter-dashboard · 2024-07-22T20:23:12Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53966 at sha 4073775

chinmaygarde · 2024-07-22T20:56:09Z

Just bizarre behavior with the goldens. The first report said multiple platform views tests were failing. But I could not reproduce them on my machine. I was testing on my emulators but they kept crashing (possibly because of 147533). In the meantime, I updated to ToT and the goldens bot says there were changes but the triage list was empty. So I don't know where there is an actual issue or not.

chinmaygarde · 2024-07-22T21:00:53Z

Ok, I force pushed. Thats what broke gold.

flutter-dashboard · 2024-07-22T22:11:10Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53966 at sha 4b5be4f

chinmaygarde · 2024-07-22T23:36:23Z

then give that SurfaceTexture to flutter. That would not be an oes texture

@jonahwilliams Other than in the embedder API context, I haven't been able to get surface texture to give me a regular texture as confirmed by GL_TEXTURE_BINDING_EXTERNAL_OES with glGet. So I think we are good? There doesn't seem to be any special handling in Skia for this either. So I think Impeller is doing just as much.

flutter-dashboard · 2024-07-23T00:19:56Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53966 at sha be0d4e3

jonahwilliams

LGTM

Could you file a bug to find a way to enable these tests? Either running them on real devices or updating emulator version.

chinmaygarde · 2024-07-23T16:39:39Z

Filed flutter/flutter#152173. I am going to timebox looking into the AHB situation on Android emulators. I had promised @johnmccutchan a reduced test case for the AHB swapchains issue but that sort of fizzled out when we decided not to switch to those swapchains. Since we are revisiting that decision and now this, its likely time to thaw that effort.

jonahwilliams · 2024-07-23T16:47:52Z

I'm able to get the AHB swapchains working locally, I would consider trying to update the CI AVD manager to see if that "fixes" it.

chinmaygarde · 2024-07-23T18:08:20Z

What AVD version are you seeing success on?

jonahwilliams · 2024-07-23T18:13:26Z

34.2.16 , mac m1. Make sure VVL are disabled (they should be by default, unless you are running unopt).

jonahwilliams · 2024-07-23T18:13:39Z

Wait, are these tests running on unopt engine builds?

…flutter/engine#53966)

…152203) flutter/engine@f5ec4ab...1572635 2024-07-24 [email protected] Change JSArray.length to return int (flutter/engine#54051) 2024-07-24 [email protected] Roll Dart SDK from 17131486a2f7 to e53beb039093 (1 revision) (flutter/engine#54066) 2024-07-24 [email protected] Roll Fuchsia Linux SDK from ZcBsXDojTYbriHD7_... to qA7S-DZ5FyMtcM7_J... (flutter/engine#54064) 2024-07-24 [email protected] [DisplayList] Fix assertions on DisplayList verbose comparison tests (flutter/engine#54065) 2024-07-24 [email protected] Roll Fuchsia Test Scripts from 5bzzKaW7fCp_No_w_... to clqtZA8cx4GEXwcOe... (flutter/engine#54063) 2024-07-23 [email protected] [iOS] Flush layer pool after platform view dispose (flutter/engine#54056) 2024-07-23 [email protected] [iOS] Mark EmbeddedViewCount const (flutter/engine#54062) 2024-07-23 [email protected] Roll Skia from f4355cf73508 to 3f1b4e98f65a (1 revision) (flutter/engine#54060) 2024-07-23 [email protected] Roll Skia from 2d518b6a793a to f4355cf73508 (9 revisions) (flutter/engine#54058) 2024-07-23 [email protected] Roll Dart SDK from eeb2e4e409bf to 17131486a2f7 (1 revision) (flutter/engine#54055) 2024-07-23 [email protected] Roll Skia from a9019fddac28 to 2d518b6a793a (4 revisions) (flutter/engine#54052) 2024-07-23 [email protected] [Impeller] add emulated advanced blend support for exp canvas. (flutter/engine#54020) 2024-07-23 [email protected] Roll Skia from 1cda2a7b0ee4 to a9019fddac28 (3 revisions) (flutter/engine#54050) 2024-07-23 [email protected] [DisplayList] track unbounded state on save layers and DisplayLists (flutter/engine#54032) 2024-07-23 [email protected] Temporarily disable use of glBlitFramebuffer on NVIDIA (flutter/engine#54040) 2024-07-23 [email protected] Set the view ID for FlView (flutter/engine#54043) 2024-07-23 [email protected] [Impeller] Implement OpenGL to Vulkan texture trampolining. (flutter/engine#53966) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from ZcBsXDojTYbr to qA7S-DZ5FyMt 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],[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

…lutter#152203) flutter/engine@f5ec4ab...1572635 2024-07-24 [email protected] Change JSArray.length to return int (flutter/engine#54051) 2024-07-24 [email protected] Roll Dart SDK from 17131486a2f7 to e53beb039093 (1 revision) (flutter/engine#54066) 2024-07-24 [email protected] Roll Fuchsia Linux SDK from ZcBsXDojTYbriHD7_... to qA7S-DZ5FyMtcM7_J... (flutter/engine#54064) 2024-07-24 [email protected] [DisplayList] Fix assertions on DisplayList verbose comparison tests (flutter/engine#54065) 2024-07-24 [email protected] Roll Fuchsia Test Scripts from 5bzzKaW7fCp_No_w_... to clqtZA8cx4GEXwcOe... (flutter/engine#54063) 2024-07-23 [email protected] [iOS] Flush layer pool after platform view dispose (flutter/engine#54056) 2024-07-23 [email protected] [iOS] Mark EmbeddedViewCount const (flutter/engine#54062) 2024-07-23 [email protected] Roll Skia from f4355cf73508 to 3f1b4e98f65a (1 revision) (flutter/engine#54060) 2024-07-23 [email protected] Roll Skia from 2d518b6a793a to f4355cf73508 (9 revisions) (flutter/engine#54058) 2024-07-23 [email protected] Roll Dart SDK from eeb2e4e409bf to 17131486a2f7 (1 revision) (flutter/engine#54055) 2024-07-23 [email protected] Roll Skia from a9019fddac28 to 2d518b6a793a (4 revisions) (flutter/engine#54052) 2024-07-23 [email protected] [Impeller] add emulated advanced blend support for exp canvas. (flutter/engine#54020) 2024-07-23 [email protected] Roll Skia from 1cda2a7b0ee4 to a9019fddac28 (3 revisions) (flutter/engine#54050) 2024-07-23 [email protected] [DisplayList] track unbounded state on save layers and DisplayLists (flutter/engine#54032) 2024-07-23 [email protected] Temporarily disable use of glBlitFramebuffer on NVIDIA (flutter/engine#54040) 2024-07-23 [email protected] Set the view ID for FlView (flutter/engine#54043) 2024-07-23 [email protected] [Impeller] Implement OpenGL to Vulkan texture trampolining. (flutter/engine#53966) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from ZcBsXDojTYbr to qA7S-DZ5FyMt 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],[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

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Jul 17, 2024

This comment was marked as outdated.

Sign in to view

github-actions bot added platform-android e: impeller labels Jul 17, 2024

This comment was marked as outdated.

Sign in to view

chinmaygarde force-pushed the trampoline branch 3 times, most recently from 462f0fa to 29b9db1 Compare July 19, 2024 22:45

chinmaygarde requested a review from jonahwilliams July 19, 2024 22:48

chinmaygarde removed the Work in progress (WIP) Not ready (yet) for review! label Jul 19, 2024

chinmaygarde mentioned this pull request Jul 20, 2024

☂️: 1P rendering plugins should use the suggested migration path flutter/flutter#151018

Closed

4 tasks

jonahwilliams reviewed Jul 21, 2024

View reviewed changes

flutter-dashboard bot added the will affect goldens label Jul 22, 2024

chinmaygarde force-pushed the trampoline branch from b090740 to 4073775 Compare July 22, 2024 19:41

Nothing change for gold.

4b5be4f

Merge branch 'main' into trampoline

be0d4e3

Back out CI

7589376

jonahwilliams approved these changes Jul 23, 2024

View reviewed changes

chinmaygarde mentioned this pull request Jul 23, 2024

[Impeller] Test surface texture based external textures with Vulkan. flutter/flutter#152173

Closed

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 23, 2024

auto-submit bot merged commit f3b053e into flutter:main Jul 23, 2024

chinmaygarde deleted the trampoline branch July 23, 2024 16:43

engine-flutter-autoroll mentioned this pull request Jul 24, 2024

Roll Flutter Engine from f5ec4ab0bf94 to 1572635432c0 (17 revisions) flutter/flutter#152203

Merged

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

f3b053e63 [Impeller] Implement OpenGL to Vulkan texture trampolining. (…

7604682

…flutter/engine#53966)


		precision mediump float;

		uniform samplerExternalOES uTexture;

[Impeller] Implement OpenGL to Vulkan texture trampolining. #53966

[Impeller] Implement OpenGL to Vulkan texture trampolining. #53966

Uh oh!

Conversation

chinmaygarde commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

chinmaygarde commented Jul 19, 2024

Uh oh!

chinmaygarde commented Jul 19, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Jul 21, 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!

Uh oh!

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!

chinmaygarde Jul 21, 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!

chinmaygarde Jul 21, 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!

jonahwilliams commented Jul 21, 2024

Uh oh!

chinmaygarde commented Jul 21, 2024

Uh oh!

chinmaygarde commented Jul 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Jul 22, 2024

Uh oh!

chinmaygarde commented Jul 22, 2024

Uh oh!

chinmaygarde commented Jul 22, 2024

Uh oh!

jonahwilliams commented Jul 22, 2024

Uh oh!

chinmaygarde commented Jul 22, 2024

Uh oh!

flutter-dashboard bot commented Jul 22, 2024

Uh oh!

flutter-dashboard bot commented Jul 22, 2024

Uh oh!

chinmaygarde commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chinmaygarde commented Jul 22, 2024

Uh oh!

flutter-dashboard bot commented Jul 22, 2024

Uh oh!

chinmaygarde commented Jul 22, 2024

Uh oh!

chinmaygarde commented Jul 17, 2024 •

edited

Loading

chinmaygarde Jul 21, 2024 •

edited

Loading

chinmaygarde Jul 21, 2024 •

edited

Loading

chinmaygarde Jul 21, 2024 •

edited

Loading

chinmaygarde commented Jul 21, 2024 •

edited

Loading

chinmaygarde commented Jul 22, 2024 •

edited

Loading