[Impeller] adds vulkan golden images #49849

gaaclarke · 2024-01-18T00:37:49Z

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
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.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

gaaclarke · 2024-01-18T23:51:39Z

making it not a draft so we can get golden image tests

flutter-dashboard · 2024-01-19T20:34:54Z

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 #49849 at sha d76241d

gaaclarke · 2024-01-19T21:22:16Z

The colors for the golden images are different than the ones we are generating. I verified that the colors are correct by looking at the colors inside the generated png for impeller_Play_AiksTest_CanRenderSweepGradientManyColorsMirror_Vulkan and comparing them against the colors in the source code and they match.

matanlurey

Seems reasonable to me. A few questions, most non-blocking

matanlurey · 2024-01-19T21:28:32Z

impeller/golden_tests/metal_screenshot.mm

 }

-const UInt8* MetalScreenshot::GetBytes() const {
+const uint8_t* MetalScreenshot::GetBytes() const {


(non-blocking) for my own curiosity, what is the difference between UInt8 and uint8_t here?

UInt8 comes from Apple's c headers and used in Apple code, uint8_t comes from stdint.h.

matanlurey · 2024-01-19T21:29:32Z

impeller/golden_tests/vulkan_screenshotter.mm

+  FML_CHECK(success);
+  latch.Wait();
+
+  // TODO(gaaclarke): Replace CoreImage requirement with something


Just checking I understand - this is OSX-only code for MoltenVK?

In this case we are using swiftshader on macos. The tests should fail if you use moltenvk, there's a check in the TearDown somewhere that makes sure libMoltenVK.dylib isn't loaded.

matanlurey · 2024-01-19T21:30:07Z

impeller/aiks/aiks_unittests.cc

  }
  EXPECT_EQ(max_mip_count, blur_required_mip_count);
+  // The log is FML_DLOG, so only check in debug builds.
+#ifndef NDEBUG


Do we build the unittests outside of debug mode?

Yea, we run these in release build when we do golden image testing. The reason why was trying to keep the visual result we are looking at as close as possible to what users would see. It may be necessary now too that swiftshader goldens take >3s to generate on debug builds.

matanlurey · 2024-01-19T21:30:59Z

impeller/golden_tests/golden_playground_test_mac.cc

 }

-bool SaveScreenshot(std::unique_ptr<testing::MetalScreenshot> screenshot) {
+bool SaveScreenshot(std::unique_ptr<testing::Screenshot> screenshot) {


(non-blocking) Could this just be a reference to a screenshot instead of a pointer?

Yep, it could have been. In this case taking ownership of the screenshot or not is moot since no one ever uses it after it's saved. The minor downside of that is that in c++ you don't know from the call site if you are working with references or not like you do with rust. So keeping things as smart pointers avoids the situation where you have to make sure you aren't accidentally introducing an implicit copy.

(if I didn't want to take ownership, I would probably have made a reference to a unique_ptr instead of using a reference to a Screenshot for the above reason.)

matanlurey · 2024-01-19T21:32:13Z

impeller/golden_tests/screenshot.h

+ public:
+  virtual ~Screenshot() = default;
+
+  virtual const uint8_t* GetBytes() const = 0;


Now that we have this neatly seperated it might be nice to ad 1-line comments at least for the non-obvious APIs (like I'm not sure what GetBytesPerRow() does for example, though maybe it's obvious to graphics engineers?)

Done, added docstrings.

flutter-dashboard · 2024-01-19T22:30:46Z

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

Changes reported for pull request #49849 at sha 19ffc9d

gaaclarke · 2024-01-19T22:43:12Z

Note for future archeology:

These golden images were previously being generated with metal. This fixes the implementation to actually use vulkan to render the golden images. So if it seems like this PR changed a golden image test, it didn't since the old ones were a lie. I looked at all the diffs and they looked good despite some issue with gamma in the old tests that is now resolved.

…141914) flutter/engine@d00e55f...a9b87c6 2024-01-20 [email protected] [Impeller] null check vertex buffer. (flutter/engine#49915) 2024-01-19 [email protected] [Impeller] adds vulkan golden images (flutter/engine#49849) 2024-01-19 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Fuchsia] Redo - Use chromium test-scripts to download images and execute tests" (flutter/engine#49908) 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

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

gaaclarke marked this pull request as ready for review January 18, 2024 23:51

gaaclarke added 11 commits January 19, 2024 09:43

[Impeller] got goldens working for vulkan

2be86e9

pulled out the screenshoter to vulkan_screenshoter

d2e1b13

started creating the vulkan screenshotter

7cbabb1

added screenshot class

ea29458

fixed calls to the vulkan device

71637bb

it's actually generating images now

e1b7e02

cleanup

2bd429a

fixed color component order

88a1b17

added missing build directive

73484c9

added back the swiftshader assert

57b3b18

lint fix

96b8d97

gaaclarke force-pushed the vulkan_goldens branch from c639534 to 96b8d97 Compare January 19, 2024 17:43

gaaclarke added 4 commits January 19, 2024 09:55

turned on vulkan

8d60d47

fixed dl tests

0808edf

license update

32bb763

disabled crashing test

7f20c8e

gaaclarke mentioned this pull request Jan 19, 2024

[Impeller] Play/AiksTest.DrawPaintTransformsBounds/Vulkan crashes flutter/flutter#141891

Closed

++

d76241d

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

gaaclarke requested review from dnfield and matanlurey January 19, 2024 21:20

matanlurey reviewed Jan 19, 2024

View reviewed changes

added docstrings

19ffc9d

gaaclarke requested a review from matanlurey January 19, 2024 21:53

matanlurey approved these changes Jan 19, 2024

View reviewed changes

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

auto-submit bot merged commit 83076ac into flutter:main Jan 19, 2024

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

Roll Flutter Engine from d00e55f7f8ed to a9b87c6288d2 (3 revisions) flutter/flutter#141914

Merged

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

83076ac70 [Impeller] adds vulkan golden images (flutter/engine#49849)

4f9e203

[Impeller] adds vulkan golden images #49849

[Impeller] adds vulkan golden images #49849

Uh oh!

Conversation

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

Uh oh!

Pre-launch Checklist

Uh oh!

gaaclarke commented Jan 18, 2024

Uh oh!

flutter-dashboard bot commented Jan 19, 2024

Uh oh!

gaaclarke commented Jan 19, 2024

Uh oh!

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

flutter-dashboard bot commented Jan 19, 2024

Uh oh!

gaaclarke commented Jan 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gaaclarke commented Jan 18, 2024 •

edited

Loading