Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Reland ImageFiltered bounds fix #17077

Merged
merged 5 commits into from
Mar 13, 2020
Merged

Conversation

flar
Copy link
Contributor

@flar flar commented Mar 10, 2020

The original fix in #16960 was passing the filtered bounds to the child savelayer which means 2 things:

  • when the filter grew the bounds, the child was painting into an unnecessarily large temporary surface before the filter grew the result again - which means a lot of transparent pixels would be in the source of the filter and the filter would have more useless work to do.

  • when the filter shrunk the bounds, the child was given a temporary surface that was too small to draw itself into and so the output was cropped.

Fixes flutter/flutter#51978

@flar flar requested review from yjbanov and liyuqian March 10, 2020 21:19
@auto-assign auto-assign bot requested a review from chinmaygarde March 10, 2020 21:19
@flar flar changed the title Reland Imagefiltered bounds fix Reland ImageFiltered bounds fix Mar 10, 2020
@liyuqian
Copy link
Contributor

It would be nice to add a unit test that would fail if 2f971c8 isn't patched.

@flar
Copy link
Contributor Author

flar commented Mar 10, 2020

Forgot to update the unit tests in the engine. On it...

@flar
Copy link
Contributor Author

flar commented Mar 10, 2020

And, for the record, I also reran the framework golden tests for this widget with this local engine build and they passed.

@flar
Copy link
Contributor Author

flar commented Mar 10, 2020

It would be nice to add a unit test that would fail if 2f971c8 isn't patched.

The existing unit tests that were landed in the original fix started failing when this was patched. I had to update them to reflect the new relationship between filtered and child bounds. So they are essentially asserting that the child and parent bounds meet the constraints now implemented.

@flar
Copy link
Contributor Author

flar commented Mar 10, 2020

For added test coverage I updated the new SimpleFilterBounds test with the code we use to verify the mock drawing calls.

Note that this created what looks like a strange diff because the tail end of the brand new SimpleFilterBounds test is character by character identical to the tail end of the old existing SimpleFilter test and so it confused which function owned the last 10-12 lines.

@flar
Copy link
Contributor Author

flar commented Mar 12, 2020

Ping

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ImageFiltered with matrix filter clips its output
3 participants