[DisplayList] implement shadow bounds without relying on Skia utilities #172572

flar · 2025-07-22T20:14:16Z

Right now DisplayList defers to Skia to compute the bounds of shadow operations, but we really need to own this code now that we are doing our own rendering so I ported it over and simplified it to just deal with the cases we care about.

Eliminates a bullet item in #161456

engine/src/flutter/impeller/geometry/matrix.h

flar · 2025-07-22T20:18:27Z

engine/src/flutter/display_list/skia/dl_sk_canvas_unittests.cc

+
+}  // namespace
+
+TEST(DlSkCanvas, ShadowBoundsCompatibilityTranslateScale) {


Note: These tests make sure we return the same answers as the previous code, but they will be deleted summarily when we delete the Skia backend (this entire directory will be deleted in whole).

So, there are also some additional tests in dl_canvas_unittests.cc that test the raw answers without comparing to Skia and those will live on at that point...

@gaaclarke here are the droi...tests you were looking for...

engine/src/flutter/display_list/dl_canvas.cc

engine/src/flutter/impeller/geometry/matrix.cc

chinmaygarde · 2025-07-22T20:30:57Z

engine/src/flutter/impeller/geometry/matrix.cc

+  //  need -B for the final quadratic equation computations anyway.)
+  double negB = a2 + d2;
+  double C = a2 * d2 - b2 * c2;
+  double B2minus4AC = negB * negB - 4 * 1.0f * C;


Here and elsewhere, use underscore_case

That gets tricky for these values that represent known variables in a common formula, so these would need to have capital A,B,C in them - but otherwise I can make them snake case.

How about the new names?

gaaclarke

I'm assuming we have a lot of existing test coverage in the existing goldens, right?

engine/src/flutter/display_list/dl_canvas.cc

gaaclarke · 2025-07-22T20:38:33Z

engine/src/flutter/display_list/dl_canvas_unittests.cc

+  EXPECT_FLOAT_EQ(shadow_bounds.GetLeft(), 96.333336f);
+  EXPECT_FLOAT_EQ(shadow_bounds.GetTop(), 97.761909f);
+  EXPECT_FLOAT_EQ(shadow_bounds.GetRight(), 203.66667f);
+  EXPECT_FLOAT_EQ(shadow_bounds.GetBottom(), 205.09525f);


Can we compare this with the skia output since the intention is to replace it?

That is done in the other file that will get deleted when the Skia source dependencies are deleted...

(see display_list/skia/dl_sk_canvas_unittests.cc)

flar · 2025-07-22T21:02:24Z

I'm assuming we have a lot of existing test coverage in the existing goldens, right?

The shadow bounds get used to compute bounds for DLs and we do have some tests that make sure that DL bounds are reasonable (by comparing to Skia and by ensuring that we don't overdraw them in the rendertests).

See

flutter/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc

Line 3909 in be9526f

TEST_F(DisplayListRendering, DrawShadow) {

And https://github.com/flutter/flutter/blob/be9526fbaaaab9474e95d196b70c41297eeda2d0/engine/src/flutter/display_list/testing/dl_rendering_unittests.cc#L2435C14-L2435C23

The only direct use of the method I see in the existing unit tests is

flutter/engine/src/flutter/display_list/display_list_unittests.cc

Line 5530 in be9526f

auto shadow_bounds =

which is used to confirm that we chose appropriately for a DrawShadow call to fit within the bounds our test is designed for.

gaaclarke

This looks good to me. I think the only thing it needs is a docstring for GetLocalBounds.

gaaclarke · 2025-07-22T22:48:01Z

engine/src/flutter/display_list/dl_canvas.cc

+                         -zRatio * params.light_position.y);
+}
+
+DlRect GetLocalBounds(DlRect ambient_bounds,


Can you add a docstring here. It's not immediately easy to understand what this is doing. Here is the one for skia: https://github.com/google/skia/blob/d43c7d1ea21a80982b926be4f72f48b64fa4bfa6/include/utils/SkShadowUtils.h#L66

The name isn't quite obvious like the skia version either IMO since it's laking the "shadow" context.

Well, that's the implementation function which never has a doc string. The doc string should be in the header file and that didn't change so it isn't in the diffs.

But, if you go look there you'll notice that it is also missing a doc comment, so the point is still valid. ;)

None of the methods in DlCanvas are documented as they are assumed to be just duplicates of the SkCanvas methods, but we need to fix that at some point. For now, I'm also adding a doc comment to DrawShadow as well as the ComputeBounds function to help fully document what we mean by shadows (ComputeBounds refers to DrawShadow as "I'm the bounds for that guy" so it helps to nail down what "that guy" does). The rest of the methods in DlCanvas should be documented later as a low priority task in the de-skiafication mini-project.

gaaclarke · 2025-07-22T22:52:53Z

engine/src/flutter/impeller/geometry/matrix.h

+  /// @brief   Return the smaller of the two non-negative scales that will
+  ///          be applied to 2D coordinates by this matrix. If the matrix
+  ///          has perspective components or the value cannot be computed,
+  ///          the method will return -1.


Why not use std::optional? It seems like accidentally using -1 isn't going to give the results you want, right?

Technically this method would not be involved in any computation inside a perspective coordinate system, so this method shouldn't even be called on one. Any code that called this without knowing up front that the result would be successful would be considered poorly written (most likely didn't bother to have a plan for what to do when the matrix is perspective) and this method would not likely be called until well after the perspective condition was noted and branched away.

But there is no good way to "enforce" that with our API, so while using optional is more of a nuisance than informative for well-written code, its use would raise the visibility of the inappropriateness of using these methods with a perspective transform. Hopefully someone modifying code in Impeller to use this method would be motivated to think more about how their code should work in a perspective situation.

gaaclarke · 2025-07-22T22:54:48Z

engine/src/flutter/impeller/geometry/matrix.cc

@@ -358,6 +360,76 @@ std::optional<MatrixDecomposition> Matrix::Decompose() const {
  return result;
 }

+std::optional<std::pair<Scalar, Scalar>> Matrix::GetScales2D() const {


Couldn't you just transform 2 unit vectors to calculate this?

No, this isn't the scale factor for the unit vectors - that's what the basis methods return. This is the expansion factor in any direction. For skewed transforms, the unit axis vectors are not scaled, but the 45-degree unit vector is scaled by more than 1, and the perpendicular to that vector is scaled by less than 1.

I'm creating a doc comment for the GetMaxBasis method to clarify its limitation.

I note that there is a GetScale() method also, which doesn't return a true scale and is only used in one location:

flutter/engine/src/flutter/impeller/display_list/dl_dispatcher.cc

Line 926 in afba7d7

GetCanvas().GetCurrentTransform().GetScale().y},

I'll file a bug for the latter and leave it for now. See #172611

flar · 2025-07-23T07:14:34Z

I assume that I'll have to merge with the post-license-cpp revert and do the appropriate manual licensing stuff, but the code itself should be in a hopefully final form.

gaaclarke

lgtm, thanks

implement shadow bounds without relying on Skia utilities

bb6aa8d

flar requested a review from bdero July 22, 2025 20:14

github-actions bot added engine flutter/engine repository. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Jul 22, 2025

flar requested review from gaaclarke and chinmaygarde July 22, 2025 20:14

flar commented Jul 22, 2025

View reviewed changes

engine/src/flutter/impeller/geometry/matrix.h Show resolved Hide resolved

flar commented Jul 22, 2025

View reviewed changes

engine/src/flutter/impeller/geometry/matrix.h Outdated Show resolved Hide resolved

flar commented Jul 22, 2025

View reviewed changes

chinmaygarde reviewed Jul 22, 2025

View reviewed changes

engine/src/flutter/display_list/dl_canvas.cc Show resolved Hide resolved

chinmaygarde reviewed Jul 22, 2025

View reviewed changes

doc comments and remove redundant std::abs

2e44227

gaaclarke reviewed Jul 22, 2025

View reviewed changes

review feedback

af649cc

flar requested review from chinmaygarde and gaaclarke July 22, 2025 21:02

gaaclarke requested changes Jul 22, 2025

View reviewed changes

gaaclarke enabled auto-merge July 22, 2025 23:43

review feedback, more doc comments and use of std::optional

61d79d3

flar requested a review from gaaclarke July 23, 2025 07:09

gaaclarke approved these changes Jul 23, 2025

View reviewed changes

gaaclarke added this pull request to the merge queue Jul 23, 2025

Merged via the queue into flutter:master with commit 3deedd2 Jul 23, 2025
178 checks passed

flar mentioned this pull request Jul 23, 2025

DL/Geometry engine migration tracking issue #161456

Open

33 tasks


		} // namespace

		TEST(DlSkCanvas, ShadowBoundsCompatibilityTranslateScale) {

[DisplayList] implement shadow bounds without relying on Skia utilities #172572

[DisplayList] implement shadow bounds without relying on Skia utilities #172572

Conversation

flar commented Jul 22, 2025

Uh oh!

Uh oh!

Uh oh!

flar Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

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!

flar commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

flar Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

flar commented Jul 23, 2025

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flar Jul 22, 2025 •

edited

Loading

flar commented Jul 22, 2025 •

edited

Loading

flar Jul 22, 2025 •

edited

Loading