[Impeller] Implement framebuffer-fetch via subpasses in Vulkan without extensions. #50154

chinmaygarde · 2024-01-29T22:28:03Z

Subpasses are not exposed in the HAL and the need for subpasses in Vulkan can
be determined based on the presence and use of input-attachments in the
shaders. This information is already reflected by the compiler. Because of
this, all references to subpasses have been removed from APIs above the HAL.
RenderPassBuilderVK is a lightweight object used to generate render passes
to use either with the pipelines (compat, base, or per-subpass) or during
rendering along with the framebuffer. Using the builder also sets up the
right subpass dependencies. As long as the builder contains compatible
attachments and subpass counts, different subpasses stamped by the builder
(via the Build method) are guaranteed to be compatible per the rules in the
spec.
Pass attachments are now in the eGeneral layout. There was no observable
difference in performance when manually inserting the right transitions.
Except, a lot of transitions needed to be inserted. If we need it, we can add
it back in short order. I wouldn't be averse to adding it if reviewers
insist.
Additional pipeline state objects need not be created as the sub-pass
self-dependencies are sufficient to setup the render-pass.
Speaking of the rasterization_order_attachment_access extension, its use has
been removed in this patch. I am prototyping adding it back to measure the
overhead introduced by manual subpass management. If the overhead is
measurable, we can use the extension on devices that have it as an added
optimization.
The complexity of command encoding remains linear (to the number of commands)
per pass.
This patch only works on a single color attachment being used as an input
attachment. While this is sufficient for current use cases, the Metal
implementation is significantly more capable since the multiple attachments
and attachment types (depth) are already supported. Rounding out support for
this is in progress.

chinmaygarde · 2024-01-29T22:30:44Z

I was able to reproduce and fix the clips breaking as reported in #49787 (comment). It was due to a stencil being cleared that should no longer be an issue in this patch. I haven't been able to reproduce the issue reported on a Samsung Android phone because I don't have access to one.

jonahwilliams · 2024-01-29T23:05:26Z

Awesome, checking this out - quick pass with a few questions.

impeller/renderer/backend/vulkan/render_pass_vk.cc

jonahwilliams · 2024-01-29T22:59:55Z

impeller/renderer/backend/vulkan/allocator_vk.cc

-    if (supports_framebuffer_fetch) {
-      vk_usage |= vk::ImageUsageFlagBits::eInputAttachment;
-    }
+    vk_usage |= vk::ImageUsageFlagBits::eInputAttachment;


Isn't this only for color attachments?

You can use subpassLoad on any image. If a depth-stencil image is set in the descriptor, that will work too as long as there is sufficient synchronization. So, no. Though I just removed this code since supports_framebuffer_fetch will always be true in this code.

impeller/renderer/backend/vulkan/formats_vk.h

jonahwilliams · 2024-01-30T01:13:58Z

This is working for me locally with a pixel and an older galaxy

impeller/renderer/backend/vulkan/render_pass_builder_vk.h

impeller/renderer/backend/vulkan/render_pass_vk.cc

jonahwilliams · 2024-01-30T01:27:50Z

impeller/renderer/renderer_unittests.cc

+  return pipeline;
+}
+
+TEST_P(RendererTest, CanSepiaToneWithSubpasses) {


AFAIK (@gaaclarke correct me) but this test file doesn't have goldens right? Do we have sufficient golden coverage in aiks?

Nope, the goldens happen in AiksTest, not here.

Good call. Do we have goldens coverage for any advanced blend already?

Since we depend on host testing, I'm not sure if anything in our matrix supports fb fetch. Metal desktop only supports it on arm.

Maybe since this path is unconditional swiftshader will still use it and the advanced blends will be covered?

The unconditionality of these tests mean that these will only run on devices that support framebuffer-fetch. I forgot to add a skip otherwise. But yeah, on CI, these tests will only work on Vulkan and ARM Macs.

but these tests as written are playground tests that aren't golden, so they don't run on CI AFAIK

It may run, but the test is more like ["does it compile", "does it run without crashing"]. I'm not sure about "passes validation" and we know "results are correct" isn't checked. Making sure that impeller_golden_tests is executing it makes sure we have the later 2 checks.

jonahwilliams

This is really exciting!

jonahwilliams · 2024-01-30T01:49:45Z

Clips still appear broken when I run the wonderous app on either a Pixel 7 or a Samsung

…t extensions. * Subpasses are not exposed in the HAL and the need for subpasses in Vulkan can be determined based on the presence and use of input-attachments in the shaders. This information is already reflected by the compiler. Because of this, all references to subpasses have been removed from APIs above the HAL. * `RenderPassBuilderVK` is a lightweight object used to generate render passes to use either with the pipelines (compat, base, or per-subpass) or during rendering along with the framebuffer. Using the builder also sets up the right subpass dependencies. As long as the builder contains compatible attachments and subpass counts, different subpasses stamped by the builder (via the `Build` method) are guaranteed to be compatible per the rules in the spec. * Pass attachments are now in the `eGeneral` layout. There was no observable difference in performance when manually inserting the right transitions. Except, a lot of transitions needed to be inserted. If we need it, we can add it back in short order. I wouldn't be averse to adding it if reviewers insist. * Additional pipeline state objects need not be created as the sub-pass self-dependencies are sufficient to setup the render-pass. * Speaking of the `rasterization_order_attachment_access` extension, its use has been removed in this patch. I am prototyping adding it back to measure the overhead introduced by manual subpass management. If the overhead is measurable, we can use the extension on devices that have it as an added optimization. * The complexity of command encoding remains linear (to the number of commands) per pass. * This patch only works on a single color attachment being used as an input attachment. While this is sufficient for current use cases, the Metal implementation is significantly more capable since the multiple attachments and attachment types (depth) are already supported. Rounding out support for this is in progress.

chinmaygarde · 2024-01-30T23:25:22Z

okie dokie, the test failures have been addressed, the clips work now too and are covered by the golden tests and I think I've addressed all PR comments. PTAL.

jonahwilliams

Overall LGTM

I would prefer it if the additional test coverage either added some goldens or something that we could catch on CI.

jonahwilliams · 2024-01-31T00:23:56Z

Work towards flutter/flutter#120223

jonahwilliams · 2024-01-31T02:13:39Z

Also word of warning, if you force push skia gold won't rerun. I would probably update branch at least once and let it run to verify that there are no regressions.

chinmaygarde · 2024-01-31T23:26:10Z

I updated the branch and the goldens seem to be happy. Assuming everything is good. Landing this and working on the new goldens next.

…kan without extensions. (flutter/engine#50154)

…142675) flutter/engine@c4247c5...f4fbabf 2024-01-31 chinmaygarde@google.com [Impeller] Implement framebuffer-fetch via subpasses in Vulkan without extensions. (flutter/engine#50154) 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 matanl@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

chinmaygarde requested review from bdero and jonahwilliams January 29, 2024 22:28

github-actions bot added the e: impeller label Jan 29, 2024

chinmaygarde mentioned this pull request Jan 29, 2024

[Impeller] Implement framebuffer-fetch via subpasses in Vulkan without extensions. #49787

Closed

jonahwilliams reviewed Jan 29, 2024

View reviewed changes

jonahwilliams reviewed Jan 30, 2024

View reviewed changes

impeller/renderer/backend/vulkan/render_pass_builder_vk.h Show resolved Hide resolved

jonahwilliams reviewed Jan 30, 2024

View reviewed changes

impeller/renderer/backend/vulkan/render_pass_vk.cc Show resolved Hide resolved

jonahwilliams reviewed Jan 30, 2024

View reviewed changes

impeller/renderer/backend/vulkan/render_pass_vk.cc Show resolved Hide resolved

jonahwilliams reviewed Jan 30, 2024

View reviewed changes

chinmaygarde force-pushed the pass6 branch from ed0b701 to 3c240d3 Compare January 30, 2024 18:54

This comment was marked as outdated.

Sign in to view

flutter-dashboard bot added the will affect goldens label Jan 30, 2024

chinmaygarde force-pushed the pass6 branch from d937190 to 941474a Compare January 30, 2024 23:21

jonahwilliams approved these changes Jan 31, 2024

View reviewed changes

Merge branch 'main' into pass6

8ac851b

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

auto-submit bot merged commit f4fbabf into flutter:main Jan 31, 2024
29 checks passed

chinmaygarde deleted the pass6 branch January 31, 2024 23:32

engine-flutter-autoroll mentioned this pull request Feb 1, 2024

Roll Flutter Engine from c4247c5e31ba to f4fbabf1eb9f (1 revision) flutter/flutter#142675

Merged

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

f4fbabf1e [Impeller] Implement framebuffer-fetch via subpasses in Vul…

dd0873e

…kan without extensions. (flutter/engine#50154)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Implement framebuffer-fetch via subpasses in Vulkan without extensions. #50154

[Impeller] Implement framebuffer-fetch via subpasses in Vulkan without extensions. #50154

chinmaygarde commented Jan 29, 2024 •

edited

chinmaygarde commented Jan 29, 2024

jonahwilliams commented Jan 29, 2024

jonahwilliams Jan 29, 2024

chinmaygarde Jan 30, 2024

jonahwilliams commented Jan 30, 2024

jonahwilliams Jan 30, 2024

gaaclarke Jan 30, 2024

chinmaygarde Jan 30, 2024

jonahwilliams Jan 30, 2024

chinmaygarde Jan 30, 2024

jonahwilliams Jan 30, 2024

gaaclarke Jan 30, 2024

jonahwilliams left a comment

jonahwilliams commented Jan 30, 2024

This comment was marked as outdated.

chinmaygarde commented Jan 30, 2024

jonahwilliams left a comment

jonahwilliams commented Jan 31, 2024

jonahwilliams commented Jan 31, 2024

chinmaygarde commented Jan 31, 2024

[Impeller] Implement framebuffer-fetch via subpasses in Vulkan without extensions. #50154

[Impeller] Implement framebuffer-fetch via subpasses in Vulkan without extensions. #50154

Conversation

chinmaygarde commented Jan 29, 2024 • edited

chinmaygarde commented Jan 29, 2024

jonahwilliams commented Jan 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Jan 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams left a comment

Choose a reason for hiding this comment

jonahwilliams commented Jan 30, 2024

This comment was marked as outdated.

chinmaygarde commented Jan 30, 2024

jonahwilliams left a comment

Choose a reason for hiding this comment

jonahwilliams commented Jan 31, 2024

jonahwilliams commented Jan 31, 2024

chinmaygarde commented Jan 31, 2024

chinmaygarde commented Jan 29, 2024 •

edited