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

Conversation

@bdero
Copy link
Member

@bdero bdero commented Sep 7, 2023

These are API improvements on the way to solving flutter/flutter#131182.

  • Remove effect_transform and is_subpass from the static FilterContents factories. These are for internal use only.
  • Replace impeller::Paint filter procs with Aiks ImageFilter factories similar to ColorFilter. This gives Aiks a simple interface for constructing filters and prevents possible state cloning bugs when copying filters into EntityPass.
  • Allow for setting filter inputs, setting the effect transform, and enabling subpass mode on a filter chain after it has been instantiated. This will allow us to sample coverage in EntityPass without rendering any snapshots using rect filter inputs.

@bdero bdero self-assigned this Sep 7, 2023
@bdero bdero force-pushed the bdero/image-filters branch from 226a752 to 46cdb4d Compare September 7, 2023 08:18
@flutter-dashboard

This comment was marked as outdated.

@bdero bdero changed the title [Impeller] Copyable Aiks image filters. [Impeller] Aiks image filters; allow setting effect transforms after FilterContents instantiation. Sep 7, 2023
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

virtual void SetEffectTransform(const Matrix& matrix);

/// @brief Turns on subpass mode for filter inputs.
virtual void SetIsForSubpass(bool is_for_subpass);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can you add more context on what subpass mode implies?

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.

subpass_texture,
Matrix::MakeTranslation(Vector3{-global_pass_position}) *
subpass->xformation_);
subpass_texture, subpass->xformation_.Basis());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a different bug or a new bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Investigating this a bit to remind myself about how we got here. It's kind of peculiar that this was working before.

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted this little change -- it turns out the test was actually behaving correctly and just needed to be adjusted. Before this PR, the effect_transform wasn't being set by the ImageFilter proc defined in the test.

@bdero bdero force-pushed the bdero/image-filters branch from e4b2cd7 to 4b876f9 Compare September 8, 2023 05:51
@bdero bdero merged commit 932027f into flutter:main Sep 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 8, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 8, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 8, 2023
…134288)

flutter/engine@ea1d0d2...a140ab4

2023-09-08 [email protected] Roll Fuchsia Mac SDK from EuBfOtm5TZIdgNaQe... to h_bQLstuirD8blCZW... (flutter/engine#45571)
2023-09-08 [email protected] Roll ANGLE from 99d39241ad4d to e60556f75b68 (1 revision) (flutter/engine#45569)
2023-09-08 [email protected] [Impeller] Aiks image filters; allow setting effect transforms after FilterContents instantiation. (flutter/engine#45530)
2023-09-08 [email protected] Roll Skia from ce3ec572ae77 to ece9f3a15b08 (1 revision) (flutter/engine#45568)
2023-09-08 [email protected] Roll Skia from fb4faa8646f8 to ce3ec572ae77 (1 revision) (flutter/engine#45567)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from EuBfOtm5TZId to h_bQLstuirD8

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] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants