[Impeller] Share vulkan playground across goldens #49981
Conversation
|
This feels like a @jason-simmons and @chinmaygarde PR review :) |
gaaclarke
requested review from
chinmaygarde
and removed request for
jonahwilliams
| } | ||
|
|
||
| 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.
|
|
||
| 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.
gaaclarke
added
the
autosubmit
…142167) flutter/engine@4bc368e...ed498f1 2024-01-24 [email protected] [Impeller] allow non-square corner radii for fast blurs (flutter/engine#49994) 2024-01-24 [email protected] Fuchsia + ocmock mirror migration (flutter/engine#50003) 2024-01-24 [email protected] Roll Dart SDK from 0f7c49da26da to e0bf6a261895 (1 revision) (flutter/engine#50009) 2024-01-24 [email protected] [macOS] Fix: Memory sanitizer violated when encoding indirect strings (flutter/engine#49995) 2024-01-24 [email protected] [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 [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
3 participants
fixes flutter/flutter#142052
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.