[Impeller] Incorporate filters in subpass coverage. #45778

bdero · 2023-09-13T17:50:43Z

I adjusted the size of the matrix filtered circle in both SaveLayer Matrix filter test variants (post-filter and backdrop filter) to move the transformed circle outside the coverage area that would be computed if subpass filters aren't being accounted for. Further, we need to avoid intersecting with the pass coverage hint when the transformation being applied by the paint's filter graph causes the Entity's space basis to deviate from the parent pass basis. We're still able to apply it for simple translations, though.

Before:

After:

flutter-dashboard · 2023-09-13T18:22:01Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #45778 at sha 1a09663

jonahwilliams

LGTM

jonahwilliams · 2023-09-13T18:16:08Z

impeller/entity/contents/filters/filter_contents.h

  virtual Matrix GetLocalTransform(const Matrix& parent_transform) const;

  Matrix GetTransform(const Matrix& parent_transform) const;

+  /// @brief  Returns true of this filter graph performs basis transformations


Suggested change

/// @brief Returns true of this filter graph performs basis transformations

/// @brief Returns true if this filter graph performs basis transformations

jonahwilliams · 2023-09-13T18:17:13Z

impeller/entity/contents/filters/filter_contents.h

  virtual Matrix GetLocalTransform(const Matrix& parent_transform) const;

  Matrix GetTransform(const Matrix& parent_transform) const;

+  /// @brief  Returns true of this filter graph performs basis transformations
+  ///         to the filtered content. For example: Rotating, scaling, and
+  ///         skewing are all basis transformations, but translating is not.


Is translating the only kind of non-basis transform? Could we rephrase this as "TranslateOnly"?

Yeah, inverted this.

jonahwilliams · 2023-09-13T18:21:02Z

impeller/entity/entity_pass.cc

      if (coverage.has_value() && coverage_limit.has_value()) {
-        coverage = coverage->Intersection(coverage_limit.value());
+        const auto* filter = entity->GetContents()->AsFilter();
+        if (!filter || !filter->HasBasisTransformations()) {
+          coverage = coverage->Intersection(coverage_limit.value());
+        }
      }
-    } else if (auto subpass =
+    } else if (auto subpass_ptr =
                   std::get_if<std::unique_ptr<EntityPass>>(&element)) {
-      coverage = GetSubpassCoverage(*subpass->get(), coverage_limit);
+      auto& subpass = *subpass_ptr->get();
+
+      std::optional<Rect> unfiltered_coverage =
+          GetSubpassCoverage(subpass, std::nullopt);
+      if (!unfiltered_coverage.has_value()) {
+        continue;
+      }
+
+      std::shared_ptr<FilterContents> image_filter =
+          subpass.delegate_->WithImageFilter(*unfiltered_coverage,
+                                             subpass.xformation_);
+      if (image_filter) {
+        Entity subpass_entity;
+        subpass_entity.SetTransformation(subpass.xformation_);
+        coverage = image_filter->GetCoverage(subpass_entity);
+      } else {
+        coverage = unfiltered_coverage;
+      }
+
+      if (coverage.has_value() && coverage_limit.has_value() &&
+          (!image_filter || !image_filter->HasBasisTransformations())) {
+        coverage = coverage->Intersection(coverage_limit.value());
+      }


nit: this code feels complicated enough that you might want to pull it into a helper method.

chinmaygarde · 2023-09-13T18:24:39Z

impeller/aiks/paint.cc

@@ -60,14 +60,21 @@ std::shared_ptr<Contents> Paint::WithFilters(
    std::shared_ptr<Contents> input) const {
  input = WithColorFilter(input, /*absorb_opacity=*/true);


Dry by comment on an unrelated item: Instead of booleans for absorb_opacity and is_subpass, I'd rather have a typed enum with AbsorbOpacity::kYes and AbsorbOpacity::kNo and the like.

Added an issue about this: flutter/flutter#134681

flutter-dashboard · 2023-09-13T19:50:03Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #45778 at sha 61d3746

…r/engine#45778)

…134755) flutter/engine@2cd34d2...e0b5b6c 2023-09-14 [email protected] Roll Skia from 54253f58533d to 87025d1e162c (1 revision) (flutter/engine#45833) 2023-09-14 [email protected] [Impeller] Incorporate filters in subpass coverage. (flutter/engine#45778) 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],[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

flar · 2023-09-29T22:54:46Z

impeller/entity/entity_pass.cc

+  // has deviated too much from the parent pass to safely intersect with the
+  // pass coverage limit.
+  coverage_limit =
+      (image_filter && image_filter->IsTranslationOnly() ? std::nullopt


Should this be !IsTranslationOnly()?

…lutter#134755) flutter/engine@2cd34d2...e0b5b6c 2023-09-14 [email protected] Roll Skia from 54253f58533d to 87025d1e162c (1 revision) (flutter/engine#45833) 2023-09-14 [email protected] [Impeller] Incorporate filters in subpass coverage. (flutter/engine#45778) 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],[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

I adjusted the size of the matrix filtered circle in both SaveLayer Matrix filter test variants (post-filter and backdrop filter) to move the transformed circle outside the coverage area that would be computed if subpass filters aren't being accounted for. Further, we need to avoid intersecting with the pass coverage hint when the transformation being applied by the paint's filter graph causes the Entity's space basis to deviate from the parent pass basis. We're still able to apply it for simple translations, though.

[Impeller] Incorporate filters in subpass coverage.

1a09663

bdero added the e: impeller label Sep 13, 2023

bdero requested review from chinmaygarde and jonahwilliams September 13, 2023 17:50

bdero self-assigned this Sep 13, 2023

flutter-dashboard bot added the will affect goldens label Sep 13, 2023

jonahwilliams approved these changes Sep 13, 2023

View reviewed changes

chinmaygarde reviewed Sep 13, 2023

View reviewed changes

Address comments

61d3746

bdero merged commit c39eb69 into flutter:main Sep 14, 2023

engine-flutter-autoroll mentioned this pull request Sep 14, 2023

Roll Flutter Engine from 2cd34d23c1a2 to e0b5b6c4eb76 (2 revisions) flutter/flutter#134755

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 14, 2023

c39eb69fc [Impeller] Incorporate filters in subpass coverage. (flutte…

2719951

…r/engine#45778)

flar reviewed Sep 29, 2023

View reviewed changes

flar mentioned this pull request Sep 29, 2023

[Impeller] ColorFiltered on top of ImageFiltered Widget with Blur causes UI glitches. flutter/flutter#135212

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] Incorporate filters in subpass coverage. #45778

[Impeller] Incorporate filters in subpass coverage. #45778

Uh oh!

bdero commented Sep 13, 2023 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Sep 13, 2023

Uh oh!

jonahwilliams left a comment

Uh oh!

jonahwilliams Sep 13, 2023

Uh oh!

bdero Sep 13, 2023

Uh oh!

jonahwilliams Sep 13, 2023

Uh oh!

bdero Sep 13, 2023

Uh oh!

jonahwilliams Sep 13, 2023

Uh oh!

bdero Sep 13, 2023

Uh oh!

chinmaygarde Sep 13, 2023

Uh oh!

bdero Sep 13, 2023

Uh oh!

flutter-dashboard bot commented Sep 13, 2023

Uh oh!

flar Sep 29, 2023

Uh oh!

bdero Sep 29, 2023

Uh oh!

Uh oh!

	/// @brief Returns true of this filter graph performs basis transformations
	/// @brief Returns true if this filter graph performs basis transformations

		@@ -60,14 +60,21 @@ std::shared_ptr<Contents> Paint::WithFilters(
		std::shared_ptr<Contents> input) const {
		input = WithColorFilter(input, /absorb_opacity=/true);

[Impeller] Incorporate filters in subpass coverage. #45778

[Impeller] Incorporate filters in subpass coverage. #45778

Uh oh!

Conversation

bdero commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Sep 13, 2023

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Sep 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bdero commented Sep 13, 2023 •

edited

Loading