[Impeller] Share vulkan playground across goldens #49981
Conversation
|
This feels like a @jason-simmons and @chinmaygarde PR review :) |
| @@ -95,9 +110,20 @@ bool SaveScreenshot(std::unique_ptr<testing::Screenshot> screenshot) { | |||
| return screenshot->WriteToPNG( | |||
| testing::WorkingDirectory::Instance()->GetFilenamePath(filename)); | |||
| } | |||
|
|
|||
| bool ShouldTestHaveVulkanValidations() { | |||
| bool enable_vulkan_validations = false; | |||
There was a problem hiding this comment.
This variable is not needed. The function can return the result of the std::find comparison.
| pimpl_->screenshotter = std::make_unique<testing::VulkanScreenshotter>( | ||
| enable_vulkan_validations); | ||
| if (enable_vulkan_validations) { | ||
| static absl::NoDestructor<std::unique_ptr<PlaygroundImpl>> |
There was a problem hiding this comment.
Move the validated and non-validated playground instances into GoldenPlaygroundTestImpl and access them through static methods.
I think that will make it clearer that these playground instances are used across all golden playground test cases.
There was a problem hiding this comment.
I want to make sure the playgrounds are only created if they are used. The rules for static fields in classes are a bit fuzzy. I've split them out into a method called GetSharedVulkanPlayground. I think that addresses your concern about making these more clear. Let me know.
| @@ -29,7 +30,7 @@ class VulkanScreenshotter : public Screenshotter { | |||
| PlaygroundImpl& GetPlayground() override { return *playground_; } | |||
|
|
|||
| private: | |||
| std::unique_ptr<PlaygroundImpl> playground_; | |||
| const std::unique_ptr<PlaygroundImpl>& playground_; | |||
There was a problem hiding this comment.
Hold this as a PlaygroundImpl& instead of a reference to a unique_ptr
There was a problem hiding this comment.
I don't want to do that because that would require converting from a pointer to a reference which is removing validation for null pointers.
| enable_vulkan_validations = true; | ||
| } | ||
|
|
||
| bool enable_vulkan_validations = ShouldTestHaveVulkanValidations(); |
There was a problem hiding this comment.
Move this into the if GetParam() == kVulkan block
With the other changes below, the Vulkan block can be reduced to something like:
PlaygroundImpl& playground = ShouldTestHaveVulkanValidations()
? GoldenPlaygroundTestImpl::GetVulkanValidationPlayground()
: GoldenPlaygroundTestImpl::GetVulkanPlayground();
pimpl_->screenshotter = std::make_unique<testing::VulkanScreenshotter>(playground);
There was a problem hiding this comment.
This mostly looks like this now, the branch is in the GetSharedVulkanPlayground function though.
…142167) flutter/engine@4bc368e...ed498f1 2024-01-24 flar@google.com [Impeller] allow non-square corner radii for fast blurs (flutter/engine#49994) 2024-01-24 103135467+sealesj@users.noreply.github.com Fuchsia + ocmock mirror migration (flutter/engine#50003) 2024-01-24 skia-flutter-autoroll@skia.org Roll Dart SDK from 0f7c49da26da to e0bf6a261895 (1 revision) (flutter/engine#50009) 2024-01-24 dkwingsmt@users.noreply.github.com [macOS] Fix: Memory sanitizer violated when encoding indirect strings (flutter/engine#49995) 2024-01-24 30870216+gaaclarke@users.noreply.github.com [Impeller] Share vulkan playground across goldens (flutter/engine#49981) 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 jacksongardner@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
fixes flutter/flutter#142052
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.