-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Only apply the rrect blur fast path for solid Colors #37594
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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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. |
@@ -36,6 +36,16 @@ struct Paint { | |||
kStroke, | |||
}; | |||
|
|||
enum class ColorSourceType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like with the enum + closure that the current color source abstraction is stretched a little thin. Perhaps we should collect both into a new object on paint?
I'm not sure where we are on re-using DLtypes but that seems like the best candidate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and have some ideas about this -- mind if I resolve it in a followup patch? I think it's time we wrap up all these factories in setColorSource and ditch the two-step conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…115335) * 6976132e5 [web] Switch to doubles (flutter/engine#37336) * c3d7c5967 Roll Skia from a434f9b69660 to 33c62dafffc9 (9 revisions) (flutter/engine#37598) * 91656a5ff Avoid segfault when converting no-op ColorFilter to ImageFilter (flutter/engine#37596) * 965f87d31 Archive windows gen_snapshot.exe. (flutter/engine#35414) * 16dba68e5 [Impeller] Only apply the rrect blur fast path for solid Colors (flutter/engine#37594) * d366183c0 Roll Dart SDK from 7cbcf48157cf to 6f5478a58387 (2 revisions) (flutter/engine#37597) * 88f56870b Roll Fuchsia Linux SDK from dRHIZSishiboEHMdw... to B0OuUvWOY24LI1WoF... (flutter/engine#37603) * 28eeba518 [Multiwindow, macOS] FlutterCompositor::Present receives view_id (flutter/engine#37391) * 8d815657c Roll Skia from 33c62dafffc9 to b474a43dcc34 (6 revisions) (flutter/engine#37606) * 78ae24140 Revert "[web] Switch to doubles (#37336)" (flutter/engine#37609)
…lutter#115335) * 6976132e5 [web] Switch to doubles (flutter/engine#37336) * c3d7c5967 Roll Skia from a434f9b69660 to 33c62dafffc9 (9 revisions) (flutter/engine#37598) * 91656a5ff Avoid segfault when converting no-op ColorFilter to ImageFilter (flutter/engine#37596) * 965f87d31 Archive windows gen_snapshot.exe. (flutter/engine#35414) * 16dba68e5 [Impeller] Only apply the rrect blur fast path for solid Colors (flutter/engine#37594) * d366183c0 Roll Dart SDK from 7cbcf48157cf to 6f5478a58387 (2 revisions) (flutter/engine#37597) * 88f56870b Roll Fuchsia Linux SDK from dRHIZSishiboEHMdw... to B0OuUvWOY24LI1WoF... (flutter/engine#37603) * 28eeba518 [Multiwindow, macOS] FlutterCompositor::Present receives view_id (flutter/engine#37391) * 8d815657c Roll Skia from 33c62dafffc9 to b474a43dcc34 (6 revisions) (flutter/engine#37606) * 78ae24140 Revert "[web] Switch to doubles (flutter#37336)" (flutter/engine#37609)
…lutter#115335) * 6976132e5 [web] Switch to doubles (flutter/engine#37336) * c3d7c5967 Roll Skia from a434f9b69660 to 33c62dafffc9 (9 revisions) (flutter/engine#37598) * 91656a5ff Avoid segfault when converting no-op ColorFilter to ImageFilter (flutter/engine#37596) * 965f87d31 Archive windows gen_snapshot.exe. (flutter/engine#35414) * 16dba68e5 [Impeller] Only apply the rrect blur fast path for solid Colors (flutter/engine#37594) * d366183c0 Roll Dart SDK from 7cbcf48157cf to 6f5478a58387 (2 revisions) (flutter/engine#37597) * 88f56870b Roll Fuchsia Linux SDK from dRHIZSishiboEHMdw... to B0OuUvWOY24LI1WoF... (flutter/engine#37603) * 28eeba518 [Multiwindow, macOS] FlutterCompositor::Present receives view_id (flutter/engine#37391) * 8d815657c Roll Skia from 33c62dafffc9 to b474a43dcc34 (6 revisions) (flutter/engine#37606) * 78ae24140 Revert "[web] Switch to doubles (flutter#37336)" (flutter/engine#37609)
Resolves flutter/flutter#114184.
This also reveals a different bug: It appears that the fast BorderMaskBlur filter is being used instead of the regular Gaussian blur.I think this is also a regression but there hasn't been a playground that reproduces it until now.Actually, this isn't a regression -- we always apply the BorderMaskBlur filter for non-solid color sources. This is correct behavior for images, but I need to check over how Skia behaves with the rest of the color sources now that they're all mostly implemented.
Before

After
