-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] Implement compose image filter #35991
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. |
7600b55 to
d2ed1c1
Compare
bdero
left a comment
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.
Looking good so far! Just some comments about testing and where stuff should live.
| auto contents = inner_proc(input, effect_transform); | ||
| contents = outer_proc(FilterInput::Make(contents), effect_transform); | ||
| return contents; | ||
| } |
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.
Since chaining filters is already supported as part of the public API via FilterInput, let's just remove this factory function and do this chaining directly in the dispatcher.
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.
Done
| using MaskFilterProc = std::function<std::shared_ptr<FilterContents>( | ||
| FilterInput::Ref, | ||
| bool is_solid_color, | ||
| const Matrix& effect_transform)>; |
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.
These type aliases are closely tied to the Paint state -- I'd rather we just make new ones if convenient elsewhere rather than moving them.
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.
Done.
| return FilterContents::MakeComposeImageFilter( | ||
| input, outer_proc.value(), inner_proc.value(), effect_transform); | ||
| }; | ||
| break; |
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.
Please add a test that uses a DisplayList to chain filters together in impeller/display_list/display_list_unittests.cc.
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.
Done.
flar
left a comment
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.
Thanks for the review request. I don't have anything to add that @bdero isn't already on top of...
bdero
left a comment
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, thanks!
* 4e9d1f4 Roll Skia from 595f51fe1ac9 to 89ff483bccbe (4 revisions) (flutter/engine#36065) * a21f534 [Impeller] Implement compose image filter (flutter/engine#35991) * 0d8ebbb [Impeller] Implement color filter image filter (flutter/engine#36043) * 51ca930 Roll Fuchsia Mac SDK from thzQ3Eamr4bpD4qXS... to T-LhIRl7A7MOniA01... (flutter/engine#36068) * f4e4f02 Roll Dart SDK from be96b5476852 to 86d283205b1d (1 revision) (flutter/engine#36069) * 6ee5d1d Roll Skia from 89ff483bccbe to 82f833ccc0a0 (1 revision) (flutter/engine#36070) * 293a9f4 Roll Fuchsia Linux SDK from S5y_wLZJBzalYADeY... to NxOIrMXIoDdeHU6S5... (flutter/engine#36071) * 0882c93 Roll Skia from 82f833ccc0a0 to 943e9a1f6d4d (1 revision) (flutter/engine#36073) * 757bd3a Roll Dart SDK from 86d283205b1d to 708893986cd0 (1 revision) (flutter/engine#36072) * adb9fd8 Roll Skia from 943e9a1f6d4d to 1efc9a18c5aa (1 revision) (flutter/engine#36075) * 18b4d8a Roll Dart SDK from 708893986cd0 to e859710c87ff (1 revision) (flutter/engine#36076) * f01919a Roll Fuchsia Mac SDK from T-LhIRl7A7MOniA01... to bi6RYiiYQspjFGDD5... (flutter/engine#36077) * 4fc3486 Windows: Cleanup: Eliminate ExternalTexturePixelBufferState (flutter/engine#36074) * 5bfda22 Roll Fuchsia Linux SDK from NxOIrMXIoDdeHU6S5... to qFA4XAWsiceDJRgmL... (flutter/engine#36078) * 175f673 Roll Dart SDK from e859710c87ff to 33d0a3b8a0ef (1 revision) (flutter/engine#36079) * 639dec4 Roll Dart SDK from 33d0a3b8a0ef to bb232d1f3a26 (1 revision) (flutter/engine#36081) * 16640f9 Roll Fuchsia Mac SDK from bi6RYiiYQspjFGDD5... to pNjLhqLsohBUBCSRa... (flutter/engine#36080) * 1df1c02 Roll Skia from 1efc9a18c5aa to 5452714cb4a1 (1 revision) (flutter/engine#36082) * f9c202c Roll Fuchsia Linux SDK from qFA4XAWsiceDJRgmL... to GBgVvIAWMvO4q1aMh... (flutter/engine#36083) * 971bf4f Roll Dart SDK from bb232d1f3a26 to 5ae14d5bf7f8 (1 revision) (flutter/engine#36085) * 74a939c Roll Fuchsia Linux SDK from GBgVvIAWMvO4q1aMh... to zsFwMWukLDug0qmn1... (flutter/engine#36086) * 5a9c6b6 Roll Skia from 5452714cb4a1 to 4b7eb38d3cbd (1 revision) (flutter/engine#36087) * c54f105 Roll Skia from 4b7eb38d3cbd to a1a9d1ca7ece (2 revisions) (flutter/engine#36088) * 08b5652 Bump ossf/scorecard-action from 1.1.2 to 2.0.2 (flutter/engine#36089) * 51f1987 Roll Skia from a1a9d1ca7ece to 1828e4d636b2 (1 revision) (flutter/engine#36090)
fix flutter/flutter#109734
Pre-launch Checklist
writing and running engine tests.
///).