[Impeller] fix drawPoints scaling factors. #54368

jonahwilliams · 2024-08-06T16:33:14Z

Fixes flutter/flutter#152780
Fixes flutter/flutter#152794

Problems:

If point scale is < 1, we discarded it too early by computing circle divisions with the rounded scale. Since we multiply the scale by the transform after rounding, we may end up generating too many points in some circumstances.
Tessellator used max basis XYZ instead of max basis XY. The latter will never allow scaling factors less than 1 as Flutter canvas scale does not impact Z, only transform layers do.
Computation of max basis required squaring the scaling factor, which would cause us to hit float::inf too early. For translate scale just take the max of m[0] and m[5].
Draw points minimum circumference is 1, so the min radius should be 0.5

flutter-dashboard · 2024-08-06T18:43:15Z

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 #54368 at sha ec28758

jonahwilliams · 2024-08-06T19:07:33Z

This shows diffs in the ClipsUseCurrentTransform test. The test was measuring the impact of scaling factors on clips:

    builder.Scale(0.8, 0.8);

    paint.setColor(colors[i % colors.size()]);
    builder.ClipPath(CreateCircle(0, 0, 300));
    builder.DrawRect(SkRect::MakeXYWH(-300, -300, 600, 600), paint);

However since the tessellator was using MaxBasis XYZ, we'd treat the tessellation as using a scaling factor of 1.0 instead of 0.8

gaaclarke

Code looks good to me, logic is sound, I think we should have a test that asserts the new logic we've added.

gaaclarke · 2024-08-06T20:03:13Z

impeller/tessellator/tessellator_unittests.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+#include <cmath>


Stray include, did you forget to include some tessellator tests?

Yeah I had some but I removed them. The only way to test that we're using masBasisXY instead of XYZ is to assert the subdivision count - but these are supposed to be arbitrary so it doesnt seem like a good route. Will stick with goldens for now.

gaaclarke · 2024-08-06T20:08:23Z

impeller/entity/geometry/point_field_geometry.cc

    auto generator =
-        renderer.GetTessellator()->FilledCircle(transform, {}, radius);
+        renderer.GetTessellator()->FilledCircle(transform, {}, radius_);


We should have a test that asserts this. Probably a golden test that is similar to the reproduction code on the linked issue is easiest.

gaaclarke

lgtm!

gaaclarke · 2024-08-06T20:47:22Z

impeller/display_list/aiks_dl_unittests.cc

@@ -467,5 +467,45 @@ TEST_P(AiksTest, CanDrawPointsWithTextureMap) {
  ASSERT_TRUE(OpenPlaygroundHere(builder.Build()));
 }

+TEST_P(AiksTest, CanDrawScaledPointsLargeScaleSmallRadius) {


Add link to issue.

flar · 2024-08-06T21:00:00Z

Tessellator used max basis XYZ instead of max basis XY. The latter will never allow scaling factors less than 1 as Flutter canvas scale does not impact Z, only transform layers do.

I think you meant former? XYZ has the problem, but not XY...

flar · 2024-08-06T21:01:15Z

Computation of max basis required squaring the scaling factor, which would cause us to hit float::inf too early. For translate scale just take the max of m[0] and m[5].

~~Why not handle that in GetMaxBasisXY so no other code has the same problem?~~

[Ignore - I missed one of the changes...]

flar

I'm not sure handing the raw radius to the tessellator is the best move since the tessellator doesn't enforce minimum sizes.

flar · 2024-08-06T21:09:30Z

impeller/entity/geometry/point_field_geometry.cc

  if (round_) {
    // Get triangulation relative to {0, 0} so we can translate it to each
-    // point in turn.
+    // point in turn. Note: we intentionally used the original radius here
+    // to capture any scaling < 1.0.


But the minimum size of stroked primitives is (somewhat arbitrarily chosen as) 1.0 device pixels which means you are allowing the transform to scale the size of the dots to (near) 0.

I will change this so we enforce the min size in the tessellator logic.

Right now we're rounding too early

The rounding was supposed to be reversible, but I think the issue here is that the rounding is 1-dimensional and the use of the rounded radius was 2-dimensional?

Also, tessellator is also used for filled shapes and those are allowed to scale to 0, so it minimally needs a flag indicating if the operation is a stroke or a fill in order to know when to apply the rounding.

I just realized that since the rounding code in the points code is using sqrt(determinant), it is probably suffering from the same issue that GetMaxBasisXYZ was suffering from. If the rouding in the points code uses GetMaxBasisXY instead, does that also solve the issue?

hmm good point. maybe then Tessellator::FilledCircle should have separate radius and pixel_radius arguments and we can let the caller manage.

I just realized that since the rounding code in the points code is using sqrt(determinant), it is probably suffering from the same issue that GetMaxBasisXYZ was suffering from. If the rouding in the points code uses GetMaxBasisXY instead, does that also solve the issue?

Ahh good catch

If the new radius works, then we should just use it in the call get get the generator below.

flutter-dashboard · 2024-08-07T03:44:12Z

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

Changes reported for pull request #54368 at sha 1ce6a7b

jonahwilliams · 2024-08-08T18:05:25Z

impeller/entity/geometry/point_field_geometry.cc

@@ -28,17 +28,17 @@ GeometryResult PointFieldGeometry::GetPositionBuffer(
    return {};
  }

-  Scalar min_size = 1.0f / sqrt(std::abs(determinant));
+  Scalar min_size = 0.5f / sqrt(std::abs(determinant));


I'm having a brain fart, should this actually be 0.5 / transform.GetMaxBasisLengthXY(); ?

I think so. Determinant of 2D matrices is the area of the (transformed) unit square which is pretty much the same thing, but it combines them geometrically so 2x in one direction balances 0.5 in the other. But matrix is a 3D thing so it becomes volume of the unit cube which might not be what we want. Or maybe it's OK since the volume of the cube is the area of the square times the height which is 1.0 - but still it will combine scales between the directions which is not what we want.

So, I think MaxBasisXY is a better value to use.

Really in all of these cases we should be doing separated 2D measurements rather than using the "max" or determinant. We should independently clamp each dimension separately.

And if they've imposed a Z scale then determinant would be not at all what we want. Another reason to go with MaxBasisXY

flutter-dashboard · 2024-08-08T20:04:31Z

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

Changes reported for pull request #54368 at sha f372355

flar

I'm wondering if it is just enough to use MaxBasisLengthXY() everywhere and leave it at that?

flar · 2024-08-08T21:27:26Z

impeller/entity/geometry/point_field_geometry.cc


-  Scalar min_size = 0.5f / sqrt(std::abs(determinant));
+  Scalar min_size = 0.5f / transform.GetMaxBasisLengthXY();


We lost our protection against a zero scale here. Is it checked anywhere else?

Added back.

flar · 2024-08-08T21:31:08Z

impeller/entity/geometry/point_field_geometry.cc

  if (round_) {
    // Get triangulation relative to {0, 0} so we can translate it to each
-    // point in turn.
+    // point in turn. Note: we intentionally used the original radius here
+    // to capture any scaling < 1.0.


If the new radius works, then we should just use it in the call get get the generator below.

flar · 2024-08-08T21:32:25Z

impeller/geometry/matrix.h

+    // for translate/scale only matrices. This substantially limits the range of
+    // precision for small and large scales. Instead, check for the common cases
+    // and directly return the max scaling factor.
+    if (IsTranslationScaleOnly()) {


Technically, we only care if [0,1] and [1,0] are 0s. The rest of the entries don't matter since we're not planning on consulting them.

need to do this still.

Another thing to consider, probably in a follow-on PR, there are only a couple of other uses of the non XY version of this method and they should probably all be converted to the XY version and then the non-XY method removed so it doesn't cause more problems. All of our rendering ops are 2D geometry so I don't think there is any code in the system that would want to use the regular version of this method.

YEah that sounds like a good idea. I can take a pass after this change.

(Done with this one though)

flar · 2024-08-08T21:37:56Z

impeller/tessellator/tessellator.cc

-  auto divisions =
-      ComputeQuadrantDivisions(view_transform.GetMaxBasisLength() * radius);
+  size_t divisions = ComputeQuadrantDivisions(
+      view_transform.GetMaxBasisLengthXY() * raw_radius);


If the scaled radius is very small then we choose very few points to represent it, but the actual dimensions of what is drawn might be large enough to see the loss of precision.

On the other hand, these are all 1-pixel in size at this point? Is it worth it either way? Since we've fixed the issue with the determinant, how much does any of this matter any more?

This is a pretty critical part of the bug fix, the scenario:

GetMaxBasisXY = 1e20
raw_radius = 1e-20

If we use the rounded up radius, we compute divisions as if we had a radius of 1e20, if we use the raw radius we get the appropriate divisions of ~1.

How so? The calling code would have done:

min_size = 1/MaxBasis = 1/1e20 = 1e-20
radius = max(radius, min_size) == max(1e-20, 1e-20) == 1e-20

Calling the tessellator it would have then computed 1e-20 * MaxBasis == 1e-20 * 1e20 == 1.0 and computed divisions for that.

The "rounded up radius" should never be greater than 1.0 when multiplied by MaxBasisXY.

Basically, the calling code is saying "When we ask for the tesselation it will base it on (radius * MaxBasis). Let's make sure that product is >= 1.0. To do that the radius can never be smaller than (1/MaxBasis)."

Another way of doing this is:

scaled_radius = radius * MaxBasis
if (scaled_radius < 1.0) scaled_radius = 1.0
scaled_radius /= MaxBasis.

Then hand that value into the tesselator and it will compute "passed_radius * MaxBasis" and that value can never be less than 1.0.

The code in the function is basically the same thing: max(radius, 1/MaxBasis) computes the same value.

The actual problems were that it was using the determinant which is "similar", but not the same value (it is the area of the unit square, not the largest scale factor), and then the code in the function was using the 3D version of MaxBasis. Does it not work if you replace both with MaxBasisXY?

You're right! agh, total facepalm on my end.

flar

Approved assuming the test case can be fixed.

flar · 2024-08-09T20:50:35Z

impeller/geometry/matrix_unittests.cc

+TEST(MatrixTest, GetMaxBasisXYWithLargeAndSmallScalingFactorNonScaleTranslate) {
+  Matrix m = Matrix::MakeScale({2.625e+20, 2.625e+20, 1}) *
+             Matrix::MakeRotationX(Radians(kPi / 2));
+  EXPECT_TRUE(std::isinf(m.GetMaxBasisLengthXY()));


This is failing, perhaps that isn't enough to create infinities on some platforms?

Could this be a case of a platform treating results that are too big for single precision as large, but not infinite values? I looked for answers and discovered that a compliant implementation is allowed to round to ::max if it is close enough. I think they said if it is closer to ::max than ::nextafter(::max). Perhaps use a larger exponent here to be sure (one that doesn't overflow ::max but squaring it overflows by a larger amount?

oh, I think its just not tripping on the scale/translate check anymore. I should just set one of matrix entires that will trigger it directly.

Is the math done in doubles and so the sqrt reduces it back into 32-bit float before the return?

oh, so the matrix is something like:

[ERROR:flutter/impeller/geometry/matrix_unittests.cc(176)] ( 2.000000, 0.000000, 0.000000, 0.000000, 0.000000, 0.000000, -2.000000, 0.000000, 0.000000, 1.000000, 0.000000, 0.000000, 0.000000, 0.000000, 0.000000, 1.000000, )

am I making the right check here?

if (e[0][1] == 0 && e[1][0] == 0) { return std::max(m[0], m[5]); } ```

You rotated around the X axis which moved it into the 3rd dimension. We're seeing it end-on, so the Y scale went to zero as everything is in the Y=0 plane.

Another way to look at this is that if the optimization doesn't trigger then only these 4 entries will be used to compute the value. So, if 2 of them are 0 (2 that happen to be 0 a lot), then you can simplify the calculation.

One question is why you are mixing e and m representations? e[0][0] should be optimized by the compiler to "base of union + 0" and e[1][1] should be optimized to "base of union + 5" so m[0] and m[5] aren't any more efficient, are they?

No Its not an efficiency thing, I was just being a bit lazy. So otherwise this code is correct then?

"optimization" doesn't necessarily mean an efficiency thing. You did it to do less work because more work was causing an overflow. It's still a more direct path to the answer.

The new test code is one way to force the old computation to occur. Yes.

flar

Looks good to go. Re-approving to confirm the point.

…_scaling

flutter-dashboard · 2024-08-13T15:43:49Z

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

Changes reported for pull request #54368 at sha ce54247

…54368)

…153394) flutter/engine@4246f15...019f9e3 2024-08-13 jonahwilliams@google.com [Impeller] fix drawPoints scaling factors. (flutter/engine#54368) 2024-08-13 zanderso@users.noreply.github.com Move API level 34 scenario app tests back to prod (flutter/engine#54539) 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 rmistry@google.com,zra@google.com 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

…lutter#153394) flutter/engine@4246f15...019f9e3 2024-08-13 jonahwilliams@google.com [Impeller] fix drawPoints scaling factors. (flutter/engine#54368) 2024-08-13 zanderso@users.noreply.github.com Move API level 34 scenario app tests back to prod (flutter/engine#54539) 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 rmistry@google.com,zra@google.com 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

[Impeller] fix drawPoints scaling factors.

b897f3f

github-actions bot added the e: impeller label Aug 6, 2024

jonahwilliams added 2 commits August 6, 2024 10:10

use big number.

7e6fdbf

++

ec28758

flutter-dashboard bot added the will affect goldens label Aug 6, 2024

jonahwilliams requested review from flar and gaaclarke August 6, 2024 19:07

gaaclarke reviewed Aug 6, 2024

View reviewed changes

jonahwilliams added 2 commits August 6, 2024 13:24

add goldens.

badfb0c

++

c174ab2

gaaclarke approved these changes Aug 6, 2024

View reviewed changes

flar requested changes Aug 6, 2024

View reviewed changes

tessellation adjustments.

1ce6a7b

jonahwilliams commented Aug 8, 2024

View reviewed changes

max basis XYZ

f372355

flar requested changes Aug 8, 2024

View reviewed changes

jonahwilliams added 2 commits August 9, 2024 10:24

remove extra radius.

f6ca941

update matrix.

fcce4aa

jonahwilliams requested a review from flar August 9, 2024 19:59

flar approved these changes Aug 9, 2024

View reviewed changes

flar reviewed Aug 9, 2024

View reviewed changes

set property.

16ad4f5

flar approved these changes Aug 13, 2024

View reviewed changes

Merge branch 'main' of github.com:flutter/engine into fix_draw_points…

ce54247

…_scaling

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 13, 2024

auto-submit bot merged commit 019f9e3 into flutter:main Aug 13, 2024
29 checks passed

engine-flutter-autoroll mentioned this pull request Aug 13, 2024

Roll Flutter Engine from 4246f1536c5d to 019f9e3f0744 (2 revisions) flutter/flutter#153394

Merged

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

019f9e3f0 [Impeller] fix drawPoints scaling factors. (flutter/engine#…

92250c9

…54368)

jonahwilliams mentioned this pull request Aug 14, 2024

[Impeller] switch all scaling computations to use maxBasisXY instead of determinant. flutter/flutter#153451

Closed


		Scalar min_size = 0.5f / sqrt(std::abs(determinant));
		Scalar min_size = 0.5f / transform.GetMaxBasisLengthXY();

[Impeller] fix drawPoints scaling factors. #54368

[Impeller] fix drawPoints scaling factors. #54368

Conversation

jonahwilliams commented Aug 6, 2024 • edited Loading

flutter-dashboard bot commented Aug 6, 2024

jonahwilliams commented Aug 6, 2024

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar commented Aug 6, 2024

flar commented Aug 6, 2024 • edited Loading

flar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Aug 8, 2024

flar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flar left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Aug 13, 2024

jonahwilliams commented Aug 6, 2024 •

edited

Loading

flar commented Aug 6, 2024 •

edited

Loading

flar Aug 10, 2024 •

edited

Loading