-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Impeller] Implement YUV texture import and sampling for video player frames. #50730
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). 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. |
I'm working with the video_player example in the linked issue with a local patch in flutter/flutter#142082 (comment). |
This is good for a review. I've editing the original comment with the commit message and rounded out documentation and cleaned up logs. There are no validation errors but I need to manually check more applications than just the video player still. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like to make the LRU cache effective for video streams we need to increase the cache size, for me 36 did it. Not important to handle right now.
Overall LGTM!
@jonahwilliams @johnmccutchan An issue that I haven't investigated yet is the following log emitted every frame. It doesn't seem to cause issues but it was confusing.
After backgrounding, the log changes to:
over and over. LMK if you have ideas, but this was happening before and after my patch so I'm treating it as unrelated. |
I noticed that too: flutter/flutter#143720 . We should follow up with Android folks to figure out if we're misusing the aquireFence API, because its actually surprisingly expensive too. |
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. |
b6b69d0
to
f40adec
Compare
Golden file changes are available for triage from new commit, Click here to view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, we should have tests though. At a minimum a golden test that renders a YUV texture would be good. I was wondering if just writing out a camera frame as raw bytes might be the easiest thing to get the input?
#include "impeller/renderer/pipeline.h" | ||
|
||
namespace impeller { | ||
|
||
class SamplerVK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -16,53 +16,114 @@ | |||
|
|||
namespace impeller { | |||
|
|||
/// Abstract base class that represents a vkImage and an vkImageView. | |||
class YUVConversionVK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
//---------------------------------------------------------------------------- | ||
/// @brief When sampling from textures whose formats are not known to | ||
/// Vulkan, a custom conversion is necessary to setup custom | ||
/// samplers. This accessor provides this conversion is one is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// samplers. This accessor provides this conversion is one is | |
/// samplers. This accessor provides this conversion if one is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
/// @brief Determines if swapchain image. That is, an image used as the | ||
/// root render target. | ||
/// | ||
/// @return Whether or not this is a swapchain image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// @return Whether or not this is a swapchain image. | |
/// @return `true` if this is a swapchain image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't author these docstrings. Just move them to use headerdoc annotations. We can clean up the actual strings in a later patch.
@@ -15,6 +15,8 @@ | |||
|
|||
namespace impeller { | |||
|
|||
class SamplerVK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
YUVConversionVK& operator=(const YUVConversionVK&) = delete; | ||
|
||
//---------------------------------------------------------------------------- | ||
/// @return If this conversion is valid for use with images and samplers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// @return If this conversion is valid for use with images and samplers. | |
/// @return `true` if this conversion is valid for use with images and samplers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
/// @return The descriptor. | ||
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// @return The descriptor. | |
/// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
const YUVConversionDescriptorVK& rhs) const; | ||
}; | ||
|
||
class SamplerVK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I also noticed that the Vulkan backend specific object ImmutableSamplerKey was missing the VK suffix. Patched that up too.
That isn't possible because the golden test harness doesn't support I wanted to bring testing this during the weekly to see if the Android Plugins Test/Test-Infra workstream has ideas on this. Because if I were to rely on existing tests for verifying camera feeds, I suspect there aren't any. |
We'll need to test this with the scenario app harness. |
FYI- You can create AHBs using the NDK: |
This seems specific to video decoders as other producers do signal the fence. I've asked Android folks about this.
By backgrounding do you mean backgrounding the app or ? |
Yeah. Just backgrounding the app. |
You're not causing the stall. Though I still can't explain the results of the logs pulled from the bots as described in flutter/flutter#144352. Going to investigate this further today. |
My mid-day update of struggles with the scenarios app.
My plan of action for the rest of the day.
|
d6e52bb
to
dd87020
Compare
Golden file changes are available for triage from new commit, Click here to view. |
This is now fixed! The latest golden file changes now show the differences @matanlurey was running into when he manually pulled the images. @dnfield I trying to run this on his emulator. |
Woohoo! |
I was able to get this running on an emulator by disabling vulkan validation layers locally. I'm reproducing the color incorrectness on my emulator, but not on a physical Pixel 7 device. |
End of day update:
I am completely out of ideas as even non-Flutter applications and reduced test cases are not running correctly on the emulators. In the interest of making progress, I would like us to merge this patch with either with a gold instance running the tests on an actual device with a followup based on the resolution of b/327501571, or triaging the current digests as positive (ugh). If there is a non-Flutter Vulkan app running with AHB running on the emulators, or if we can reproduce this issue on any device, I'd love to see it. But I recommend now we treat the emulator issue separately. |
So, strictly speaking, even the current digest is better than a blank image, in terms of correctness. I don't think it's feasible for us to have a pre-submit with an actual device, or if it is, we'll need to divorce that option from landing this PR. It's clear to me you put a lot of time into this, and this gets us to a better place, so I'm OK with merging as positive - I can make a note in the README that the emulator output of the Vulkan tests doesn't (quite) match reality. Wdut @gaaclarke @jonahwilliams? |
As long as we have issues tracking the follow up work and some coverage of this (even if partially working), then landing SGTM. |
Yeah good point, +1 tracking issue - and I do think it should be a P1, it will be hard to catch regressions on Vulkan if we can't trust the output image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for all the work to get this tested, lgtm
I marked the digests as "positive". Filed an issue to track the emulator instability. Dan closed in as a duplicate of his tracking issue. It is P1 and cross-referenced with the internal bug. Separately, I filed flutter/flutter#144632 to track backing out the digests marked incorrectly positive. Landing this. |
Hmm, I marked the digests as positive 35 mins ago but the gold check is still pending. Anyway, the triage page is clean. I'm landing now. |
Auto submit won't work. I'd just hit the update branch button and let it go again. |
… frames. This patch rounds out support for importing Android Hardware Buffers as Vulkan images and sampling from them. Not Android Hardware Buffers are in formats that Vulkan (let alone Impeller) understands. Some YUV textures have formats with no equivalent `vk::Format` enum and need extra information on how to sample from them. This patch adds support for correctly importing and sampling from such formats. There are severe restrictions on how sampling from such external formats works. For one, it isn’t possible to assign a combined image sampler in the render pass. The pipeline itself needs to be rebuilt to reference a specially created immutable sampler. This immutable sampler is a combination of the usual information present in an Impeller SamplerDescriptor as well as a custom “conversion” object obtained while importing the Android Hardware Buffer. There is no way to predict what conversion object will be necessary ahead of time as this will depend on the source of the Android Hardware Buffers and is likely different for different video feeds, camera sources, and other Android Hardware Buffer texture sources. To handle this uncertainty, a new pipeline variant with a JIT determined immutable sampler will be hashed and cached before being used in a render pass. The number of pipeline variants created just-in-time will depend on the number of sampler variants used in the render pass to sample from wrapped Image. For instance, specifying a sampler with a different address mode will likely result in a new pipeline variant being created. In most cases however, there will just be one or two additional pipeline variants per application. Impellers sampler diversity is very low with most samplers being the usual NN samplers. It may be possible to preload even this pipeline by trying known conversions. As said previously, there can only be a handful of these conversions. More restrictions on sampling from such images includes being limited to `VK_FILTER_LINEAR` without additional format and extension wrangling and performance penalties. Fixes flutter/flutter#142082.
dd87020
to
6ef67ae
Compare
Golden file changes are available for triage from new commit, Click here to view. |
Thats odd, I wonder why Gold missed this one in the earlier run. |
…144653) flutter/engine@49bc157...effcf97 2024-03-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Enable depth buffer clipping & Stencil-then-Cover path rendering. (#51209)" (flutter/engine#51217) 2024-03-05 30870216+gaaclarke@users.noreply.github.com [Impeller] implement blur style solid (flutter/engine#50892) 2024-03-05 xilaizhang@google.com [g3 roll] Revert "Remove unused drone_dimension field" (flutter/engine#51214) 2024-03-05 skia-flutter-autoroll@skia.org Roll Skia from 17677914dabf to 37947aec8c5c (5 revisions) (flutter/engine#51211) 2024-03-05 737941+loic-sharma@users.noreply.github.com Fix git hooks on Windows (flutter/engine#51203) 2024-03-05 bdero@google.com [Impeller] Enable depth buffer clipping & Stencil-then-Cover path rendering. (flutter/engine#51209) 2024-03-05 bdero@google.com [Impeller] Fix convex triangulation winding bug for multi-contour paths. (flutter/engine#51198) 2024-03-05 skia-flutter-autoroll@skia.org Roll Skia from 875b356e4d96 to 17677914dabf (2 revisions) (flutter/engine#51207) 2024-03-05 gspencergoog@users.noreply.github.com Skip configuration dependency if unit tests are disabled (flutter/engine#51179) 2024-03-05 chinmaygarde@google.com [Impeller] Implement YUV texture import and sampling for video player frames. (flutter/engine#50730) 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 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
This patch rounds out support for importing Android Hardware Buffers as Vulkan
images and sampling from them.
Not all Android Hardware Buffers are in formats that Vulkan (let alone Impeller)
understands. Some YUV textures have formats with no equivalent
vk::Format
enumand need extra information on how to sample from them.
This patch adds support for correctly importing and sampling from such formats.
There are severe restrictions on how sampling from such external formats works.
For one, it isn’t possible to assign a combined image sampler in the render
pass. The pipeline itself needs to be rebuilt to reference a specially created
immutable sampler. This immutable sampler is a combination of the usual
information present in an Impeller SamplerDescriptor as well as a custom
“conversion” object obtained while importing the Android Hardware Buffer.
There is no way to predict what conversion object will be necessary ahead of
time as this will depend on the source of the Android Hardware Buffers and is
likely different for different video feeds, camera sources, and other Android
Hardware Buffer texture sources.
To handle this uncertainty, a new pipeline variant with a JIT determined
immutable sampler will be hashed and cached before being used in a render pass.
The number of pipeline variants created just-in-time will depend on the number
of sampler variants used in the render pass to sample from wrapped Image. For
instance, specifying a sampler with a different address mode will likely result
in a new pipeline variant being created.
In most cases however, there will just be one or two additional pipeline
variants per application. Impellers sampler diversity is very low with most
samplers being the usual NN samplers. It may be possible to preload even this
pipeline by trying known conversions. As said previously, there can only be a
handful of these conversions.
More restrictions on sampling from such images includes being limited to
VK_FILTER_LINEAR
without additional format and extension wrangling andperformance penalties.
Fixes flutter/flutter#142082.