[Impeller] Wire up Android Hardware Buffer backed swapchains. #51213

chinmaygarde · 2024-03-05T22:11:18Z

This avoids the use of VK_KHR_swapchain and VK_KHR_surface for swapchains
on Android.
Instead, a pool of Android Hardware Buffers allocated by Impeller is used as
swapchain images.
The hardware buffers are presented as contents on surface controls via surface
transactions.
The total number of buffers either being used on the client side or in the
compositor is capped.
The swapchain caches a certain (configurable) number of buffer on the client
side but these can expire.
All swapchain images are lazily allocated. This is different from the KHR
swapchains where all swapchain images are eagerly created when the swapchain
size is updated.
The number of swapchain images used should generally be less than the KHR
swapchain except in cases where there either compositor backpressure (where
the client is able to wait on the semaphore till the internal count is
reached), and when the backpressure subsides (where there are more recyclables
generated but they haven't yet expired). To ameliorate the memory usage in
case of the latter, the recyclables count will be less than the total number
of allowable images in flight.
The format of the swapchain buffers is always RGBA8888. This may allow for
earlier creation of the pipeline state objects since we no longer have to wait
for surface acquisition to find the root surface format.
In terms of code organization, the old swapchains have been renamed to
KHRSwapchainVK while the replacements are called AHBSwapchainVK.
Corresponding utilities and dependencies have been renamed.
In order to make working with Android handles easier, a new toolkit has been
introduced in //impeller/toolkit/android. This adds type-safe,
reference-counted wrappers around the Android handles with lazy proc table
setup.
These new swapchains may only be used on Android API 29 and above. This is the
baseline for Impeller Vulkan on Android. So the KHR swapchains should never be
used on Android. Instead, non-Android platforms will use this.
There is some recycling of device transient textures in the KHR swapchains
that is being reproduced by caching the entire render target in the AHB
swapchains.

chinmaygarde · 2024-03-05T22:13:53Z

Still WIP but good for an initial review. The current swapchains create a new impeller::Surface around each image per acquire. I am attempting to reuse the surface as well as the render target so the device transient textures are also saved (I think @jonahwilliams was trying to do something similar). But I believe I am running into the same issues as #51208. The recycling is not done yet because of this issue.

jonahwilliams · 2024-03-05T22:30:33Z

On the current swapchain implementation, the MSAA texture is cached on the swapchain implementation and not the surface. I have #51206 which adds the depth stencil to that list too.

chinmaygarde · 2024-03-05T22:33:40Z

You are referring to the HasMSAATexture sidecar texture right? I must admit I don't remember why I setup the surface to be created each time a swapchain image is acquired. Wouldn't keeping it around and its rendertarget solve all caching issues?

jonahwilliams · 2024-03-05T22:43:21Z

Yeah something like that would work. I think we can actually make all surfaces share the same MSAA/Depth+Stencil attachment, which should reduce memory usage for the devices without lazily allocated memory support.

chinmaygarde · 2024-03-05T22:45:24Z

I think we can actually make all surfaces share the same MSAA/Depth+Stencil attachment, which should reduce memory usage for the devices without lazily allocated memory support.

Oh, yeah. That's a good idea.

I am also wondering if I am trying to boil the ocean here. I can just do the same thing the current swapchain is doing but that was causing code repetition. Can clean it up in a later attempt though.

jonahwilliams · 2024-03-06T00:05:09Z

I'm definitely partial to smaller changes, ideally separating out renaming/moving with adding the new code.

chinmaygarde · 2024-03-06T00:17:29Z

I'm going to leave all the caching stuff in the existing swapchains alone.

jonahwilliams · 2024-03-06T00:21:27Z

FWIW this crashes on a Pixel 4 immediately:

A Dart VM Service on Pixel 4 XL is available at: http://127.0.0.1:64174/DN2SNbWi1nw=/
The Flutter DevTools debugger and profiler on Pixel 4 XL is available at: http://127.0.0.1:9102?uri=http://127.0.0.1:64174/DN2SNbWi1nw=/
I/AdrenoGLES-0(28633): QUALCOMM build                   : 85da404, I46ff5fc46f
I/AdrenoGLES-0(28633): Build Date                       : 11/30/20
I/AdrenoGLES-0(28633): OpenGL ES Shader Compiler Version: EV031.31.04.01
I/AdrenoGLES-0(28633): Local Branch                     : promo490_3_Google
I/AdrenoGLES-0(28633): Remote Branch                    : 
I/AdrenoGLES-0(28633): Remote Branch                    : 
I/AdrenoGLES-0(28633): Reconstruct Branch               : 
I/AdrenoGLES-0(28633): Build Config                     : S P 10.0.4 AArch64
I/AdrenoGLES-0(28633): Driver Path                      : /vendor/lib64/egl/libGLESv2_adreno.so
I/AdrenoGLES-0(28633): PFP: 0x016ee190, ME: 0x00000000
W/AdrenoUtils(28633): <ReadGpuID_from_sysfs:197>: Failed to open /sys/class/kgsl/kgsl-3d0/gpu_model
W/AdrenoUtils(28633): <ReadGpuID:221>: Failed to read chip ID from gpu_model. Fallback to use the GSL path
I/Choreographer(28633): Skipped 38 frames!  The application may be doing too much work on its main thread.
I/flutter (28633): [IMPORTANT:flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_vk.cc(155)] Created surface.
I/flutter (28633): [IMPORTANT:flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_vk.cc(155)] Created surface.
I/flutter (28633): [IMPORTANT:flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_vk.cc(214)] There are 1 recyclables.
I/flutter (28633): [IMPORTANT:flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_vk.cc(150)] Recycled surface.
E/flutter (28633): [ERROR:flutter/impeller/base/validation.cc(57)] Break on 'ImpellerValidationBreak' to inspect point of failure: 
E/flutter (28633): --- Vulkan Debug Report  ----------------------------------------
E/flutter (28633): |                Severity: Error
E/flutter (28633): |                    Type: { Validation }
E/flutter (28633): |                 ID Name: VUID-VkRenderPassBeginInfo-framebuffer-parameter
E/flutter (28633): |               ID Number: -533747612
E/flutter (28633): |       Queue Breadcrumbs: [NONE]
E/flutter (28633): |  CMD Buffer Breadcrumbs: [NONE]
E/flutter (28633): |         Related Objects: RenderPass [452998790644124] [EntityPass Render Pass: Depth=0 Count=0], Framebuffer [454098302271901] [UNNAMED], ImageView [451899279016347] [UNNAMED]
E/flutter (28633): |                 Trigger: Validation Error: [ VUID-VkRenderPassBeginInfo-framebuffer-parameter ] Object 0: handle = 0x19c000000019c, name = EntityPass Render Pass: Depth=0 Count=0, type = VK_OBJECT_TYPE_RENDER_PASS; Object 1: handle = 0x19d000000019d, type = VK_OBJECT_TYPE_FRAMEBUFFER; Object 2: handle = 0x19b000000019b, type = VK_OBJECT_TYPE_IMAGE_VIEW; | MessageID = 0xe02fa864 | vkCmdBeginRenderPass(): pCreateInfo->pAttachments[2] VkImageView 0x19b000000019b[] is invalid. The Vulkan spec states: framebuffer must be a valid VkFramebuffer handle (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkRenderPassBeginInfo-framebuffer-parameter)

chinmaygarde · 2024-03-06T00:26:13Z

That was the render target caching. I’ve backed out the recycling. Try now. It will be slow though because no recycling.

…

On Tue, Mar 5, 2024 at 4:21 PM Jonah Williams ***@***.***> wrote: FWIW this crashes on a Pixel 4 immediately: A Dart VM Service on Pixel 4 XL is available at: http://127.0.0.1:64174/DN2SNbWi1nw=/ The Flutter DevTools debugger and profiler on Pixel 4 XL is available at: http://127.0.0.1:9102?uri=http://127.0.0.1:64174/DN2SNbWi1nw=/ I/AdrenoGLES-0(28633) <http://127.0.0.1:9102?uri=http://127.0.0.1:64174/DN2SNbWi1nw=/I/AdrenoGLES-0(28633)>: QUALCOMM build : 85da404, I46ff5fc46f I/AdrenoGLES-0(28633): Build Date : 11/30/20 I/AdrenoGLES-0(28633): OpenGL ES Shader Compiler Version: EV031.31.04.01 I/AdrenoGLES-0(28633): Local Branch : promo490_3_Google I/AdrenoGLES-0(28633): Remote Branch : I/AdrenoGLES-0(28633): Remote Branch : I/AdrenoGLES-0(28633): Reconstruct Branch : I/AdrenoGLES-0(28633): Build Config : S P 10.0.4 AArch64 I/AdrenoGLES-0(28633): Driver Path : /vendor/lib64/egl/libGLESv2_adreno.so I/AdrenoGLES-0(28633): PFP: 0x016ee190, ME: 0x00000000 W/AdrenoUtils(28633): <ReadGpuID_from_sysfs:197>: Failed to open /sys/class/kgsl/kgsl-3d0/gpu_model W/AdrenoUtils(28633): <ReadGpuID:221>: Failed to read chip ID from gpu_model. Fallback to use the GSL path I/Choreographer(28633): Skipped 38 frames! The application may be doing too much work on its main thread. I/flutter (28633): [IMPORTANT:flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_vk.cc(155)] Created surface. I/flutter (28633): [IMPORTANT:flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_vk.cc(155)] Created surface. I/flutter (28633): [IMPORTANT:flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_vk.cc(214)] There are 1 recyclables. I/flutter (28633): [IMPORTANT:flutter/impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_vk.cc(150)] Recycled surface. E/flutter (28633): [ERROR:flutter/impeller/base/validation.cc(57)] Break on 'ImpellerValidationBreak' to inspect point of failure: E/flutter (28633): --- Vulkan Debug Report ---------------------------------------- E/flutter (28633): | Severity: Error E/flutter (28633): | Type: { Validation } E/flutter (28633): | ID Name: VUID-VkRenderPassBeginInfo-framebuffer-parameter E/flutter (28633): | ID Number: -533747612 E/flutter (28633): | Queue Breadcrumbs: [NONE] E/flutter (28633): | CMD Buffer Breadcrumbs: [NONE] E/flutter (28633): | Related Objects: RenderPass [452998790644124] [EntityPass Render Pass: Depth=0 Count=0], Framebuffer [454098302271901] [UNNAMED], ImageView [451899279016347] [UNNAMED] E/flutter (28633): | Trigger: Validation Error: [ VUID-VkRenderPassBeginInfo-framebuffer-parameter ] Object 0: handle = 0x19c000000019c, name = EntityPass Render Pass: Depth=0 Count=0, type = VK_OBJECT_TYPE_RENDER_PASS; Object 1: handle = 0x19d000000019d, type = VK_OBJECT_TYPE_FRAMEBUFFER; Object 2: handle = 0x19b000000019b, type = VK_OBJECT_TYPE_IMAGE_VIEW; | MessageID = 0xe02fa864 | vkCmdBeginRenderPass(): pCreateInfo->pAttachments[2] VkImageView 0x19b000000019b[] is invalid. The Vulkan spec states: framebuffer must be a valid VkFramebuffer handle (https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkRenderPassBeginInfo-framebuffer-parameter) — Reply to this email directly, view it on GitHub <#51213 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAKYNNNHWGULD2YWOK3UZ3YWZORHAVCNFSM6AAAAABEH6FJZSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZZHA2TMOJQHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

jonahwilliams · 2024-03-06T00:27:25Z

Now it occassionally freezes on the "backpressure" step or flashes black:

video.mp4

jonahwilliams · 2024-03-06T00:29:00Z

Also if I rotate I get:

E/flutter (29648): [ERROR:flutter/impeller/base/validation.cc(57)] Break on 'ImpellerValidationBreak' to inspect point of failure: New surface size is not allocatable.

chinmaygarde · 2024-03-06T19:47:59Z

Yeah, both should be fixed. I still haven't figure out if I can depend on Android to always fire the OnComplete callback too. If not, there is a chance of deadlock and our semaphores don't allow for timeouts. I'm just going to patch that in for safety.

screen-20240306-114226.mp4

chinmaygarde · 2024-03-06T21:30:04Z

if I can depend on Android to always fire the OnComplete

This isn't documented. But following along the Android code seems to indicate that it does.

chinmaygarde · 2024-03-07T21:21:25Z

This will not land as-is. I am splitting it up into smaller patches.

* This avoids the use of `VK_KHR_swapchain` and `VK_KHR_surface` for swapchain on Android. * Instead, a pool of Android Hardware Buffers allocated by Impeller is used as swapchain images. * The hardware buffers are presented as contents on surface controls via surface transactions. * The total number of buffers either being used on the client side or in the compositor is capped. * The swapchain caches a certain (configurable) number of buffer on the client side but these can expire. * All swapchain images are lazily allocated. This is different from the KHR swapchains where all swapchain images are eagerly created when the swapchain size is updated. * The number of swapchain images used should generally be less than the KHR swapchain except in cases where there either compositor backpressure (where the client is able to wait on the semaphore till the internal count is reached), and when the backpressure subsides (where there are more recyclables generated but they haven't yet expired). To ameliorate the memory usage in case of the latter, the recyclables count will be less than the total number of allowable images in flight. * The format of the swapchain buffers is always RGBA8888. This may allow for earlier creation of the pipeline state objects since we no longer have to wait for surface acquisition to find the root surface format. * In terms of code organization, the old swapchains have been renamed to `KHRSwapchainVK` while the replacements are called `AHBSwapchainVK`. Corresponding utilities and dependencies have been renamed. * In order to make working with Android handles easier, a new toolkit has been introduced in `//impeller/toolkit/android`. This adds type-safe, reference-counted wrappers around the Android handles with lazy proc table setup. * These new swapchains may only be used on Android API 29 and above. This is the baseline for Impeller Vulkan on Android. So the KHR swapchains should never be used on Android. Instead, non-Android platforms will use this. * There is some recycling of device transient textures in the KHR swapchains that is being reproduced by caching the entire render target in the AHB swapchains.

This is part of blowing up flutter#51213 into smaller commits. Our current swapchain is based on the VK_KHR_swapchain extension. When this was the only swapchain that could be created, the KHR prefix was dropped. Since we are going to be having multiple swapchain types, add the KHR prefix and make room for other swapchains. No change in functionality. Just renames classes and files.

This is part of blowing up #51213 into smaller commits. Our current swapchain is based on the VK_KHR_swapchain extension. When this was the only swapchain that could be created, the KHR prefix was dropped. Since we are going to be having multiple swapchain types, add the KHR prefix and make room for other swapchains. No change in functionality. Just renames classes and files.

…re Buffers. Also doesn't compile the TU on non-Android platforms. Part of flutter#51213 being chopped up. No change in functionality. Just renames and moves stuff around.

…re Buffers. (#51298) Also doesn't compile the TU on non-Android platforms. Part of #51213 being chopped up. No change in functionality. Just renames and moves stuff around.

jonahwilliams · 2024-03-11T20:14:23Z

Not crashing anymore! That said, we need to have at least recyclying of the underlying AHBs implemented, otherwise re-allocation of these starts to dominate the per frame costs:

chinmaygarde · 2024-03-11T21:54:52Z

Yeah. Recycling is table stakes. I'm landing bits and pieces of this patch at the moment and I still expect to have a flag for comparison.

Only available on Android device API levels >= 29. Proc table is setup has versioning checks. All handles are type safe. Collection of handles takes into account cleanup tasks (like reparenting surface controls). The proc table contains code duplicated in ndk_helpers and I will remove that in favor of this in a subsequent patch. Part of flutter#51213 being chopped up.

jonahwilliams · 2024-03-11T22:36:34Z

I would also start thinking about how we test this - what it means to test it. We have some capability to unit test android code on CI too.

chinmaygarde · 2024-03-11T22:39:49Z

I added a unit-test for just the toolkit in this patch. I am able to create hardware buffers in a unit-test just fine. We can test the minutia in that harness or another just like it. Presenting them in a window would be an integration test I suppose and would be covered by everything else.

chinmaygarde · 2024-03-12T06:48:20Z

I just realized we can also move the binder transaction (SurfaceTransaction::Apply) to a background thread.

Only available on Android device API levels >= 29. Proc table is setup has versioning checks. All handles are type safe. Collection of handles takes into account cleanup tasks (like reparenting surface controls). The proc table contains code duplicated in ndk_helpers and I will remove that in favor of this in a subsequent patch. Part of flutter#51213 being chopped up.

…s. (#51334) Only available on Android device API levels >= 29. Proc table is setup has versioning checks. All handles are type safe. Collection of handles takes into account cleanup tasks (like reparenting surface controls). The proc table contains code duplicated in ndk_helpers and I will remove that in favor of this in a subsequent patch. Part of #51213 being chopped up.

…ed objects. (#51334)" (#51457) Reverts: #51334 Initiated by: matanlurey Reason for reverting: Broke engine post-submit, see https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8753367119442265873/+/u/test:_Android_Unit_Tests__API_28_/stdout. Original PR Author: chinmaygarde Reviewed By: {dnfield} This change reverts the following previous change: Only available on Android device API levels >= 29. Proc table is setup has versioning checks. All handles are type safe. Collection of handles takes into account cleanup tasks (like reparenting surface controls). The proc table contains code duplicated in ndk_helpers and I will remove that in favor of this in a subsequent patch. Part of #51213 being chopped up.

flutter-dashboard · 2024-04-01T06:50:55Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

chinmaygarde · 2024-04-04T20:14:26Z

Closing this as most of it has landed and the new patch isn't going to build on this.

chinmaygarde · 2024-04-16T18:49:45Z

xref #52087

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

chinmaygarde requested review from dnfield and johnmccutchan March 5, 2024 22:11

github-actions bot added platform-android e: impeller labels Mar 5, 2024

chinmaygarde force-pushed the ahb_swapchain branch from 5f58e4d to 545f97c Compare March 6, 2024 20:16

chinmaygarde force-pushed the ahb_swapchain branch from 7bebef6 to d6504f5 Compare March 8, 2024 20:12

chinmaygarde mentioned this pull request Mar 8, 2024

[Impeller] Add the KHR prefix to existing swapchain utilities. #51295

Merged

chinmaygarde mentioned this pull request Mar 8, 2024

[Impeller] Use the AHB prefix for utils that work with Android Hardware Buffers. #51298

Merged

chinmaygarde mentioned this pull request Mar 11, 2024

[Impeller] A toolkit for managed handles to Android NDK vended objects. #51334

Merged

auto-submit bot mentioned this pull request Mar 15, 2024

Reverts "[Impeller] A toolkit for managed handles to Android NDK vended objects. (#51334)" #51457

Merged

jonahwilliams mentioned this pull request Mar 29, 2024

[Impeller] Set RGBA8888 as the default Vulkan color format before the app acquires a surface #51770

Merged

chinmaygarde closed this Apr 4, 2024

chinmaygarde deleted the ahb_swapchain branch April 16, 2024 18:49

[Impeller] Wire up Android Hardware Buffer backed swapchains. #51213

[Impeller] Wire up Android Hardware Buffer backed swapchains. #51213

Uh oh!

Conversation

chinmaygarde commented Mar 5, 2024

Uh oh!

chinmaygarde commented Mar 5, 2024

Uh oh!

jonahwilliams commented Mar 5, 2024

Uh oh!

chinmaygarde commented Mar 5, 2024

Uh oh!

jonahwilliams commented Mar 5, 2024

Uh oh!

chinmaygarde commented Mar 5, 2024

Uh oh!

jonahwilliams commented Mar 6, 2024

Uh oh!

chinmaygarde commented Mar 6, 2024

Uh oh!

jonahwilliams commented Mar 6, 2024

Uh oh!

chinmaygarde commented Mar 6, 2024 via email

Uh oh!

jonahwilliams commented Mar 6, 2024

Uh oh!

jonahwilliams commented Mar 6, 2024

Uh oh!

chinmaygarde commented Mar 6, 2024

Uh oh!

chinmaygarde commented Mar 6, 2024

Uh oh!

chinmaygarde commented Mar 7, 2024

Uh oh!

jonahwilliams commented Mar 11, 2024

Uh oh!

chinmaygarde commented Mar 11, 2024

Uh oh!

jonahwilliams commented Mar 11, 2024

Uh oh!

chinmaygarde commented Mar 11, 2024

Uh oh!

chinmaygarde commented Mar 12, 2024

Uh oh!

flutter-dashboard bot commented Apr 1, 2024

Uh oh!

chinmaygarde commented Apr 4, 2024

Uh oh!

chinmaygarde commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants