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

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Sep 8, 2022

fix flutter/flutter#109734

NZNTZ0iyn0

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@flutter-dashboard
Copy link

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.

Copy link
Member

@bdero bdero left a 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;
}
Copy link
Member

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.

Copy link
Member Author

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)>;
Copy link
Member

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.

Copy link
Member Author

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;
Copy link
Member

@bdero bdero Sep 8, 2022

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@bdero bdero requested a review from flar September 8, 2022 20:40
@bdero bdero changed the title [Impeller] Implement compose image fiter [Impeller] Implement compose image filter Sep 8, 2022
Copy link
Contributor

@flar flar left a 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...

@ColdPaleLight ColdPaleLight requested review from bdero and flar September 9, 2022 11:35
Copy link
Member

@bdero bdero left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ColdPaleLight ColdPaleLight added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 10, 2022
@auto-submit auto-submit bot merged commit a21f534 into flutter:main Sep 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 12, 2022
zanderso pushed a commit to flutter/flutter that referenced this pull request Sep 12, 2022
* 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)
cfontas pushed a commit to cfontas/engine that referenced this pull request Sep 14, 2022
Oleh-Sv pushed a commit to Oleh-Sv/engine that referenced this pull request Sep 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

[Impeller] Compose image filter is unimplemented.

3 participants