[Impeller] implement experimental canvas in snapshot controller. #53750
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| @@ -12,6 +12,9 @@ | |||
| #include "flutter/impeller/display_list/dl_image_impeller.h" | |||
| #include "flutter/impeller/geometry/size.h" | |||
| #include "flutter/shell/common/snapshot_controller.h" | |||
| #include "impeller/renderer/render_target.h" | |||
|
|
|||
| #define EXPERIMENTAL_CANVAS false | |||
There was a problem hiding this comment.
Is there some context on why this isn't in a GN rule somewhere and applied globally instead of on a per TU basis?
There was a problem hiding this comment.
factoring out to a gn rule seems like a good idea, will make it easier to flip on/off to see if there are any golden diffs or test failures.
There was a problem hiding this comment.
Don't mean to block this patch though. We can do that later.
| static_cast<double>(max_size.width) / static_cast<double>(size.width()); | ||
| double scale_factor_y = | ||
| static_cast<double>(max_size.height) / static_cast<double>(size.height()); | ||
| double scale_factor = std::min(1.0, std::min(scale_factor_x, scale_factor_y)); |
There was a problem hiding this comment.
IIRC, std::min has an overload that takes an initializer list. So you should be able do do just std::min({a, b, c}) instead of std::min(a, std::min(b, c)).
| static_cast<double>(max_size.height) / static_cast<double>(size.height()); | ||
| double scale_factor = std::min(1.0, std::min(scale_factor_x, scale_factor_y)); | ||
|
|
||
| auto render_target_size = impeller::ISize(size.width(), size.height()); |
There was a problem hiding this comment.
Nit: TSize has a * operator overload that takes a scalar. So something like size * std::min<Scalar>({1.0, max.width / static_cast<Scalar>(size.width), max.height / static_cast<Scalar>(size.height)) might be more readable.
| impeller_size, // size | ||
| /*mip_count=*/1, | ||
| "Picture Snapshot MSAA", // label | ||
| impeller::RenderTarget:: |
There was a problem hiding this comment.
Nit: This could perhaps be dried up using a ternary since only the last argument differs based on SupportsOffscreenMSAA and everything else is repeated.
There was a problem hiding this comment.
Do you mean adding a new CreateOffscreen that differentiates MSAA via an argument?
There was a problem hiding this comment.
Nothing as involved as all that. Just context->GetContext()->GetCapabilities()->SupportsOffscreenMSAA() ? kDefaultColorAttachmentConfigMSAA : kDefaultColorAttachmentConfig
There was a problem hiding this comment.
But the methods are different
jonahwilliams
added
the
autosubmit
…151495) flutter/engine@d3269d5...9d943eb 2024-07-09 [email protected] Avoid using a private GTest macro to skip tests. (flutter/engine#53782) 2024-07-09 [email protected] [Impeller] Use downsample shader for blur instead of mip levels. (flutter/engine#53760) 2024-07-09 [email protected] [engine] support combined UI/Platform thread for iOS/Android. (flutter/engine#53656) 2024-07-09 [email protected] [Impeller] Enable framebuffer fetch tests disabled on OpenGL ES. (flutter/engine#53766) 2024-07-09 [email protected] [Impeller] implement experimental canvas in snapshot controller. (flutter/engine#53750) 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
Fixes flutter/flutter#150994
This ensures that the dart:ui API toImage and toImageSync use the experimental canvas API when the define is set. No other changes.