[DisplayList] migrate DlImageFilter code to Impeller geometry classes #56720

flar · 2024-11-19T22:54:33Z

The DlImageFilter code uses Skia geometry classes for its internal computations. This PR switches those implementations to use the Impeller geometry classes for consistency and 3rd party header file independence.

flar · 2024-11-20T00:59:20Z

There are just a few lines of code in dl_image_filter.cc that still use the Skia classes for bounds computation as they seem to pass the perspective transform tests better, but otherwise the ImageFilter code is 99.5% migrated to the Impeller classes including what should be 100% of the API (not including the use of SkShader by the runtime effect class). See flutter/flutter#159175 for more information.

flutter-dashboard · 2024-11-20T01:04:12Z

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 #56720 at sha 199970f

flar · 2024-11-20T01:06:33Z

I find the golden diffs interesting. They are very tiny, but they don't appear (by name) to involve image filters.

gaaclarke

Woo, that was a lot to look at. Mostly looking good, here's some notes and questions.

gaaclarke · 2024-11-20T01:14:15Z

display_list/dl_builder.cc

  // A backdrop will affect up to the entire surface, bounded by the clip
  bool will_be_unbounded = (backdrop != nullptr);
-  std::shared_ptr<const DlImageFilter> filter;
+  std::shared_ptr<DlImageFilter> filter;


Why change this?

The classes are immutable and I was having trouble with the fact that you can't mix and match shared_ptr with different "const" modifiers.

https://google.github.io/styleguide/cppguide.html#Use_of_const

Immutable classes should have const methods so having a const reference should still be the appropriate usage in c++.

gaaclarke · 2024-11-20T01:15:16Z

display_list/dl_builder.cc

-        if (global_bounds.intersect(clip)) {
-          parent_layer().global_space_accumulator.accumulate(global_bounds);
+        global_bounds = DlRect::Make(global_ibounds);
+        auto clipped_bounds = global_bounds.Intersection(clip);


https://google.github.io/styleguide/cppguide.html#Type_deduction

gaaclarke · 2024-11-20T01:18:21Z

display_list/dl_builder.h


  // |DlCanvas|
-  void SaveLayer(std::optional<const DlRect>& bounds,
+  void SaveLayer(const std::optional<DlRect>& bounds,


Why was this changed?

const in an optional doesn't really prevent you from changing it, the optional itself has to be const. And at that point, why have the const inside the template type?

why have the const inside the template type?

To make sure no one modifies the DlRect. I would have expected const std::optional<const DlRect>& bounds

gaaclarke · 2024-11-20T01:26:12Z

flow/layers/image_filter_layer.cc

 namespace flutter {

-ImageFilterLayer::ImageFilterLayer(std::shared_ptr<const DlImageFilter> filter,
+ImageFilterLayer::ImageFilterLayer(const std::shared_ptr<DlImageFilter>& filter,


I don't think switching these to const references is an improvement. With the std::move there ways to avoid a copy, the const reference forces a copy to happen.

These are always called from a reference source that cannot be moved so a copy must happen at some point. It would be better if the copy happened into the layer's local field rather than to the value handed in to the constructor.

You are considering just the code that is written today. There is no way to enforce that this is only called with references.

gaaclarke · 2024-11-20T01:29:14Z

flow/layers/layer_state_stack.h

+  // The transform from the local coordinates to the device coordinates
+  // in 4x4 DlMatrix representation. This matrix provides all information
+  // needed to compute bounds for a 2D rendering primitive, and it will
+  // accurately concatenate with other 4x4 matrices without losing information.


Please write as a docstring (ie ///)

None of the methods in this file are docstring. This is an issue with a scope outside of this change - flutter/flutter#159179

gaaclarke · 2024-11-20T01:32:36Z

impeller/geometry/rect.h

    return TRect(0.0, 0.0, size.width, size.height);
  }

+  template <class U, class FT = T>


docstring would help here, it's not immediately clear to me what this is for. it's copying a rect, but why the enable_if_t? If this potentially throwing away precision implicitly?

Done. It's only defined for float destinations. If you want to convert to an integer rect you have to use a method like RoundOut to specify the rounding behavior.

jonahwilliams · 2024-11-20T01:53:02Z

impeller/geometry/matrix.h


  Scalar GetDeterminant() const;

+  bool IsInvertible() const { return GetDeterminant() != 0; }


can you put these in the implementation?

I'm not sure what you are asking?

I forget the proper terminology :). Can you put the method bodies in the .cc file?

These geometry files seem to prefer inline implementations unless the method is very complicated.

flutter/flutter#159220 - we should reconsider the decisions made in that module separately.

jonahwilliams

LGTM!

…ry classes (flutter/engine#56720)

…159226) flutter/engine@3828681...2d32cf3 2024-11-20 [email protected] [DisplayList] migrate DlImageFilter code to Impeller geometry classes (flutter/engine#56720) 2024-11-20 [email protected] Split channel messaging out of handlers (flutter/engine#56667) 2024-11-20 [email protected] Revert "Added assert for opengles thread safety (#56585)" (flutter/engine#56730) 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…flutter/engine#56720) The DlImageFilter code uses Skia geometry classes for its internal computations. This PR switches those implementations to use the Impeller geometry classes for consistency and 3rd party header file independence.

github-actions bot added platform-ios e: impeller labels Nov 19, 2024

flar mentioned this pull request Nov 19, 2024

[DisplayList] The ImageFilter inset/outset_bounds methods are not migrated to Impeller classes flutter/flutter#159175

Open

flar added 3 commits November 19, 2024 15:38

[DisplayList] migrate DlImageFilter code to Impeller geometry classes

42567b1

workaround perspective math failures in ImageFilter bounds unit tests

bea2a89

fix ios includes

199970f

flar force-pushed the dl-image-filter-on-impeller-geometry branch from 981ba0f to 199970f Compare November 20, 2024 00:05

flar marked this pull request as ready for review November 20, 2024 01:02

flar requested review from gaaclarke and jonahwilliams November 20, 2024 01:03

flutter-dashboard bot added the will affect goldens label Nov 20, 2024

gaaclarke reviewed Nov 20, 2024

View reviewed changes

jonahwilliams reviewed Nov 20, 2024

View reviewed changes

review feedback

f7fb622

jonahwilliams approved these changes Nov 20, 2024

View reviewed changes

flar added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 20, 2024

auto-submit bot merged commit 2d32cf3 into flutter:main Nov 20, 2024
36 checks passed

engine-flutter-autoroll mentioned this pull request Nov 20, 2024

Roll Flutter Engine from 3828681d1f86 to 2d32cf3a7971 (3 revisions) flutter/flutter#159226

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 20, 2024

2d32cf3a7 [DisplayList] migrate DlImageFilter code to Impeller geomet…

aa863be

…ry classes (flutter/engine#56720)


		Scalar GetDeterminant() const;

		bool IsInvertible() const { return GetDeterminant() != 0; }

[DisplayList] migrate DlImageFilter code to Impeller geometry classes #56720

[DisplayList] migrate DlImageFilter code to Impeller geometry classes #56720

Uh oh!

Conversation

flar commented Nov 19, 2024

Uh oh!

flar commented Nov 20, 2024

Uh oh!

flutter-dashboard bot commented Nov 20, 2024

Uh oh!

flar commented Nov 20, 2024

Uh oh!

gaaclarke 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!

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!

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!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants