[Impeller] Fix mipmap generation for Vulkan render target textures. #49848

jonahwilliams · 2024-01-18T00:12:21Z

Potentially fixes flutter/flutter#141495

The texture used as a render target attachment can have miplevels, but the image view cannot. Unconditionally create two image views per texture (this could be optimized in some cases), where the specific "render target texture view" always sets a mipcount of 1.

In theory this should allow us to generate mipmaps for textures that are used as render target attachments, including toImage and toImageSync textures which are currently missing this functionality.

This new blur should perform faster since it scales down the image before blurring it. Jonah did early testing of it and found it to be faster. Scrolling around with the blur perf bug it seems faster. It also has a wider test bed and is hopefully easier to maintain since it contains all of its logic for both directions. testing: There are existing blur tests and we've backfilled more as we've added features to this blur. fixes flutter/flutter#131580 fixes flutter/flutter#138259 - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I added new tests to check the change I am making or feature I am adding, or the PR is [test-exempt]. See [testing the engine] for instructions on writing and running engine tests. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I signed the [CLA]. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/wiki/Tree-hygiene#overview [Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene [test-exempt]: https://github.com/flutter/flutter/wiki/Tree-hygiene#tests [Flutter Style Guide]: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style [testing the engine]: https://github.com/flutter/flutter/wiki/Testing-the-engine [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/wiki/Chat

This reverts commit cae5c50.

This reverts commit 590a245.

This reverts commit 3d8682e.

gaaclarke · 2024-01-22T19:43:50Z

(I rebased this onto upstream/main)

gaaclarke

@jonahwilliams I ran this locally and found that it passed our tests and rendered everything correctly. Now that we have our golden's setup correctly you can test this yourself with impeller_golden_tests --working_dir=<path> --gtest_filter="*GaussianBlur*Vulkan".

We have tests that are asserting that the blur is being executed with mipmaps (example AiksTest.GaussianBlurMipMapImageFilter).

The only thing I wasn't able to verify is to visually check to make sure it looks good when animating the blur. Given that it passes the aforementioned checks though I have no reason to believe it looks different than the metal rendering.

I also gave the PR a review and this approach sounds good to me.

jonahwilliams · 2024-01-22T21:40:40Z

Thanks @gaaclarke , I'll get this cleaned up. I think I can verify the flicker on some of the blur examples we have

gaaclarke · 2024-01-22T21:42:06Z

Thanks @gaaclarke , I'll get this cleaned up. I think I can verify the flicker on some of the blur examples we have

Ah yea, just running it on an Android device, sounds good.

gaaclarke · 2024-01-22T21:42:33Z

impeller/entity/entity_pass.cc

-    // generation on opengles.
-    mip_count = 1;
-  }
+  // if (context->GetBackendType() != Context::BackendType::kMetal) {


friendly note, switch this to mip_count 1 for opengles until we can verify that is working.

…to hacky_image_view

jonahwilliams · 2024-01-22T23:27:50Z

Bad news: this still has more validation errors:

E/flutter (32222): -----------------------------------------------------------------
E/flutter (32222): [ERROR:flutter/impeller/base/validation.cc(49)] Break on 'ImpellerValidationBreak' to inspect point of failure: 
E/flutter (32222): --- Vulkan Debug Report  ----------------------------------------
E/flutter (32222): |                Severity: Error
E/flutter (32222): |                    Type: { Validation }
E/flutter (32222): |                 ID Name: UNASSIGNED-CoreValidation-DrawState-InvalidImageLayout
E/flutter (32222): |               ID Number: 1303270965
E/flutter (32222): |       Queue Breadcrumbs: [NONE]
E/flutter (32222): |  CMD Buffer Breadcrumbs: TextFrame, Solid Fill, RRect Shadow, Restore Clip, Texture Fill: Subpass, Texture Fill: MSAA backdrop, Intersect Clip, QueueSubmit
E/flutter (32222): |         Related Objects: CommandBuffer [12970367425478379120] [EntityPass Command Buffer: Depth=0 Count=18]
E/flutter (32222): |                 Trigger: Validation Error: [ UNASSIGNED-CoreValidation-DrawState-InvalidImageLayout ] Object 0: handle = 0xb400007419efc670, name = EntityPass Command Buffer: Depth=0 Count=18, type = VK_OBJECT_TYPE_COMMAND_BUFFER; | MessageID = 0x4dae5635 | vkQueueSubmit(): pSubmits[0].pCommandBuffers[0] command buffer VkCommandBuffer 0xb400007419efc670[EntityPass Command Buffer: Depth=0 Count=18] expects VkImage 0x1550000000155[EntityPass Color Texture] (subresource: aspectMask 0x1 array layer 0, mip level 2) to be in layout VK_IMAGE_LAYOUT_GENERAL--instead, current layout is VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL.
E/flutter (32222): -----------------------------------------------------------------

jonahwilliams · 2024-01-22T23:28:16Z

It looks like the miplevels aren't getting set to the correct state

gaaclarke · 2024-01-22T23:29:38Z

@jonahwilliams how did you get those? It seems like we aren't getting validation errors in our golden test runner?

jonahwilliams · 2024-01-22T23:48:12Z

This with my patching running the example in flutter/flutter#132735

It looks like the problem might be the image layout is per-mip level? I need to do more research, that would blow my mind.

jonahwilliams · 2024-01-24T01:40:34Z

Added a bit of a hack to get to zero validation errors. Right now the usage flags for our render targes are incorrect for the backdrop filters. If I fix that I can make this behavior less manual.

gaaclarke · 2024-01-24T18:48:05Z

Hey @jonahwilliams, I'm just circling back to this today. Where are we at, how can I help?

Now that we have the allow list for validations, I think we should turn on validations for one of the tests where we had problems as part of this PR, for example AiksTest.GaussianBlurMipMapNestedLayer. I don't know if I'll get validation landed as turned on across the board before we land this.

jonahwilliams · 2024-01-24T18:52:05Z

I have all the validation errors fixed locally, but I'm re-arranging this patch a bit to remove the blit pass changes. ETA EOD.

jonahwilliams · 2024-01-24T20:35:39Z

I think I found the best example of the jittering. The macrobenchmarks app --> animated backdrop filter.

Before

video.mp4

After

patch.mp4

jonahwilliams · 2024-01-24T20:36:26Z

I think there is less shaking? but its still pretty shaky. If this is caused by the misalignment, then fixing that would be the next thing to do.

jonahwilliams · 2024-01-24T20:38:32Z

on iOS there is still some shaking but its less bad.

gaaclarke · 2024-01-24T20:59:43Z

Yea, I don't see much difference there. I think the more important case is shaky when not animating the sigma, but contents under the blur.

gaaclarke

Can you please turn on the validation for GaussianBlurMipMapNestedLayer in golden_playground_test_mac.cc as part of the PR?

gaaclarke · 2024-01-24T23:42:50Z

impeller/entity/inline_pass_context.cc


  if (command_buffer_) {
-    if (!command_buffer_->EncodeAndSubmit(pass_)) {
+    if (!pass_->EncodeCommands()) {


This is kind of weird now, it isn't obvious why there is a if (command_buffer_) check here. Is that still necessary?

Should be handled by isActive check, added D_CHECK

gaaclarke · 2024-01-24T23:45:40Z

impeller/golden_tests/golden_playground_test_mac.cc

-  if (GetBackend() != PlaygroundBackend::kMetal &&
-      GetBackend() != PlaygroundBackend::kVulkan) {


This should be turning off vulkan golden images, which isn't what we want.

gaaclarke · 2024-01-24T23:47:11Z

impeller/renderer/backend/vulkan/allocator_vk.cc

    }
+    // Create a specialized view for render target attachments.
+    view_info.subresourceRange.levelCount = 1u;
+    auto [result_2, rt_image_view] = device.createImageViewUnique(view_info);


Can you give result_2 a more meaningful name? rt_image_view_result? It looks like you are n't checking it below, you are checking result.

gaaclarke · 2024-01-24T23:49:28Z

impeller/renderer/backend/vulkan/allocator_vk.cc

    ImageResource(ImageResource&& o) {
      std::swap(image, o.image);
      std::swap(image_view, o.image_view);
+      std::swap(rt_image_view, o.rt_image_view);


Can't this just be ImageResource(ImageResource&&) = default?

gaaclarke · 2024-01-24T23:52:17Z

impeller/renderer/backend/vulkan/blit_command_vk.cc

  );

-  return true;
+  // If this is not an onscreen, convert back to shader read.


This comment doesn't match the code (it's the negation of what's written). It's more correct to say "If it's on screen, don't convert it to shader read".

gaaclarke · 2024-01-24T23:53:38Z

impeller/renderer/backend/vulkan/texture_source_vk.h

+  /// @brief Retrieve the image view used for render target attachments
+  ///        with this texture source.
+  ///
+  /// image views used as render target attachments cannot have any mip levels.


Suggested change

/// image views used as render target attachments cannot have any mip levels.

/// ImageViews used as render target attachments cannot have any mip levels.

gaaclarke · 2024-01-24T23:54:34Z

impeller/renderer/backend/vulkan/texture_source_vk.h

  vk::ImageLayout GetLayout() const;

+  /// Whether or not this is a swapchain image.
+  virtual bool IsOnscreen() const = 0;


Maybe this should read "IsSwapchainImage()"?

jonahwilliams · 2024-01-25T16:37:56Z

I think I turned on validation correctly, lets see if it blends

gaaclarke

LGTM, the test name just needs to be fixed. Once this becomes a deny list or is on across the board it will be better.

impeller/golden_tests/golden_playground_test_mac.cc

Co-authored-by: gaaclarke <[email protected]>

gaaclarke

LGTM, thanks for looking into this. This would have taken me quite a while to work out.

flutter-dashboard · 2024-01-25T17:52:26Z

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 #49848 at sha b0c7ab6

gaaclarke · 2024-01-25T17:54:53Z

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

Yay, further validation that we got this right. It's weird it didn't show up in more tests but it's hard to draw conclusions with a fuzzy matcher though.

jonahwilliams · 2024-01-25T17:58:57Z

Something is definitely different!

…extures. (flutter/engine#49848)

…142256) flutter/engine@b2167a9...4b145d0 2024-01-25 [email protected] [Impeller] Fix mipmap generation for Vulkan render target textures. (flutter/engine#49848) 2024-01-25 [email protected] [Impeller] Do not emit metadata for structs that are not part of the shader's interface (flutter/engine#50029) 2024-01-25 [email protected] Roll Skia from de46a989e0ca to 29b545e4356b (1 revision) (flutter/engine#50047) 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

gaaclarke and others added 12 commits January 17, 2024 11:39

[Impeller] disabled misleading vulkan golden image tests

74a559b

chinmays vulkan patch

888fdae

started setting TextureVK::mipmaps_generated_

f4cb738

made tests only want metal mips

e15a7b7

turned on for macos ios only

6922642

Revert "started setting TextureVK::mipmaps_generated_"

f64cd5d

This reverts commit cae5c50.

Revert "chinmays vulkan patch"

f621cc6

This reverts commit 590a245.

Revert "turned on for macos ios only"

00c0f43

This reverts commit 3d8682e.

disabled mipmap generation for vulkan

118d428

fixed vulkan/opengles test

8f13cdf

[Impeller] maintain two image views per texture.

4a0b890

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

jonahwilliams changed the title ~~[IMpeller] Fix mipmap generation for Vulkan render target textures.~~ [Impeller] Fix mipmap generation for Vulkan render target textures. Jan 18, 2024

[Impeller] maintain two image views per texture.

84f6856

gaaclarke force-pushed the hacky_image_view branch from 4a0b890 to 84f6856 Compare January 22, 2024 19:43

updated tests

6a56680

gaaclarke reviewed Jan 22, 2024

View reviewed changes

jonahwilliams added 3 commits January 22, 2024 14:43

Merge branch 'hacky_image_view' of github.com:jonahwilliams/engine in…

7d24b71

…to hacky_image_view

more cleanups

693b8a0

more conflict marker removal.

03834c3

add mipmap gen to existing buffer.

e6a89d2

jonahwilliams added 2 commits January 23, 2024 17:37

add API for image layout transition.

b814ea2

fix image layouts.

06f7520

remove blit pass stuff.

c72c7ca

cleanup.

3102976

++

9d8650b

jonahwilliams requested a review from gaaclarke January 24, 2024 21:57

gaaclarke suggested changes Jan 24, 2024

View reviewed changes

aaron review.

defaa06

gaaclarke reviewed Jan 25, 2024

View reviewed changes

impeller/golden_tests/golden_playground_test_mac.cc Outdated Show resolved Hide resolved

Update impeller/golden_tests/golden_playground_test_mac.cc

b0c7ab6

Co-authored-by: gaaclarke <[email protected]>

gaaclarke approved these changes Jan 25, 2024

View reviewed changes

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

jonahwilliams added autosubmit Merge PR when tree becomes green via auto submit App and removed autosubmit Merge PR when tree becomes green via auto submit App labels Jan 25, 2024

Merge branch 'main' into hacky_image_view

edc8ff4

gaaclarke merged commit 4b145d0 into flutter:main Jan 25, 2024

engine-flutter-autoroll mentioned this pull request Jan 25, 2024

Roll Flutter Engine from b2167a93c1a0 to 4b145d041560 (3 revisions) flutter/flutter#142256

Merged

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

4b145d041 [Impeller] Fix mipmap generation for Vulkan render target t…

c011e37

…extures. (flutter/engine#49848)

		if (GetBackend() != PlaygroundBackend::kMetal &&
		GetBackend() != PlaygroundBackend::kVulkan) {

	/// image views used as render target attachments cannot have any mip levels.
	/// ImageViews used as render target attachments cannot have any mip levels.

[Impeller] Fix mipmap generation for Vulkan render target textures. #49848

[Impeller] Fix mipmap generation for Vulkan render target textures. #49848

Uh oh!

Conversation

jonahwilliams commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke commented Jan 22, 2024

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jan 22, 2024

Uh oh!

gaaclarke commented Jan 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Jan 22, 2024

Uh oh!

jonahwilliams commented Jan 22, 2024

Uh oh!

gaaclarke commented Jan 22, 2024

Uh oh!

jonahwilliams commented Jan 22, 2024

Uh oh!

jonahwilliams commented Jan 24, 2024

Uh oh!

gaaclarke commented Jan 24, 2024

Uh oh!

jonahwilliams commented Jan 24, 2024

Uh oh!

jonahwilliams commented Jan 24, 2024

Before

After

Uh oh!

jonahwilliams commented Jan 24, 2024

Uh oh!

jonahwilliams commented Jan 24, 2024

Uh oh!

gaaclarke commented Jan 24, 2024

Uh oh!

gaaclarke 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!

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!

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 commented Jan 25, 2024

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaaclarke left a comment

jonahwilliams commented Jan 18, 2024 •

edited

Loading