Convert ImageFilter.matrix with scale/translate into blit with sampling options #33562

jonahwilliams · 2022-05-23T20:25:32Z

While we don't have a generic way to draw a transformed image quickly with the existing canvas APIs, we don't need to do anything fancy for scale/translations. If an ImageFilter is used with a matrix translation that is only scale + translate, then we can apply this directly to the dest rect that we draw the raster cache image with.

This came up again since a commit of flutter that used filter quality for the zoom page transition rolled into google3 and it improved a number of customer:money's benchmarks.

To solve this for a generic image, an API like drawImageQuad would be ideal (TBD)

flutter/flutter#100972

flow/BUILD.gn

jonahwilliams · 2022-05-23T20:26:16Z

flow/raster_cache.cc

+    canvas.drawImage(image_, bounds.fLeft, bounds.fTop, SkSamplingOptions(),
+                     paint);
+  } else {
+    FML_DCHECK(matrix->isScaleTranslate());


yeah, yeah... I know

(I think this is fine - if someone misuses this it'll draw something kind of weird, but it'll still draw something, and having a DCHECK here to make sure we don't misuse it in our own test cases of the API we expose is valid)

Oh I meant more that I couldn't figure out how to multiply the matrices so I just did mapRect twice 😢

How would they misuse this? Why would it draw something kinda weird?

Oh, I see, because the rendering code below doesn't handle an arbitrary matrix. That can be fixed...

The problem here is that the code for the untransformed cache draw is not a very straightforward way to accomplish the goals here and it is largely unnecessary except that it has a very small potential to have slight roundoff errors that spook the underlying graphics engine (Skia) from using a straight pixel-for-pixel blit which was very fast on slow hardware into using a (degenerately unnecessary) scaling GPU shader. As a result, they explicitly dumbed down the straight cache blit to a hard-coded identity-transform blit to avoid any potential for round-off mistakes.

But the code you are adding is already intending to invoke a transformed blit operation anyway, so rather than try to code it to match the identity-space clamped blit operation that cache::draws normally use, just draw the cache entry with the appropriate transform.

jonahwilliams · 2022-05-23T21:04:31Z

This is very much a draft right now. Open questions are what the best way to support filter quality is - are the drawImage filter qualities comparable to the image filter filter quality?

cc @flar

dnfield · 2022-05-23T21:26:12Z

flow/raster_cache.cc

+    matrix->mapRect(&dest_rect, logical_rect_);
+    SkRect dest_rect_2;
+    device_total_matrix.mapRect(&dest_rect_2, dest_rect);
+    canvas.drawImageRect(image_, dest_rect_2, SkSamplingOptions(), paint);


You'd want to pump through the sampling options from the original matrix filter here I think.

Also, don't try to do this variant of the cache blit with bounding rectangles. Supply the rendering call with the transform you need and let the graphics engine do the work. There's some transform math to be done here (the original matrix produced this cache image and now there's a new matrix to render under), but you could allow for any kind of transform matrix here.

Basically I think we can assert that the entry was cached with the same matrix we are rendering with or we wouldn't have gotten this far. The only difference are the translation components. Then we want to apply the given matrix parameter on top of that, and translate it to the current canvas's translation component and draw.

The ideal would be for there to be a drawImageQuad() where you specify the 4 points of the quad into which to render the image, but there is drawAtlas that lets you specify a scaled parallelogram into which to render the image. It would work for any scaled and rotated transform. Beyond that there is drawVertices which allows you to specify 2 triangles into which to map the bitmap.

flar · 2022-05-23T22:09:30Z

I was thinking of moving the implementation from ImageFilterLayer to TransformLayer with the same hint, or BitmapTransformLayer.

Part of the reasoning is that Skia doesn't seem to optimize a MatrixImageFilter quite as much as just doing the transform ourselves, and partly because nested ImageFilterLayers with the Matrix filter don't seem to concatenate correctly and I need to find out why. If we did the caching ourselves, then we could handle the nesting better.

jonahwilliams · 2022-05-23T22:12:24Z

I was thinking of moving the implementation from ImageFilterLayer to TransformLayer with the same hint, or BitmapTransformLayer.

I believe @dnfield brought this up earlier and I had a similar question: If swap this out with a transform layer, don't we end up invalidating our raster cached child as we animate it? Or does this essentially become "implement raster cache for transform layer", but more restricted?

jonahwilliams · 2022-05-23T22:12:53Z

Besides that question, splitting into a separate layer class SGTM

flar · 2022-05-23T22:12:26Z

flow/layers/image_filter_layer.cc


  const SkIRect filter_input_bounds = child_bounds.roundOut();
-  SkIRect filter_output_bounds = filter_->filterBounds(
+  SkIRect filter_output_bounds = filter_->skia_object()->filterBounds(


DlImageFilter provides this bounds operation directly, no need to invoke it on skia_object.

Which one is that?

flar · 2022-05-23T22:14:20Z

flow/layers/image_filter_layer.cc

+    // transition.
+    auto matrix_filter = filter_->asMatrix();
+    if (matrix_filter != nullptr &&
+        matrix_filter->matrix().isScaleTranslate() &&


Why only scale translate? If you fall through because it is not a scale translate you are going to get a bitmap transform anyway.

flar · 2022-05-23T22:17:09Z

flow/raster_cache.cc

+    canvas.drawImage(image_, bounds.fLeft, bounds.fTop, SkSamplingOptions(),
+                     paint);
+  } else {
+    FML_DCHECK(matrix->isScaleTranslate());


How would they misuse this? Why would it draw something kinda weird?

Oh, I see, because the rendering code below doesn't handle an arbitrary matrix. That can be fixed...

flar · 2022-05-23T22:17:52Z

flow/raster_cache.cc

+    matrix->mapRect(&dest_rect, logical_rect_);
+    SkRect dest_rect_2;
+    device_total_matrix.mapRect(&dest_rect_2, dest_rect);
+    canvas.drawImageRect(image_, dest_rect_2, SkSamplingOptions(), paint);


flar · 2022-05-23T22:21:39Z

flow/raster_cache.cc

+    canvas.drawImage(image_, bounds.fLeft, bounds.fTop, SkSamplingOptions(),
+                     paint);
+  } else {
+    FML_DCHECK(matrix->isScaleTranslate());


The problem here is that the code for the untransformed cache draw is not a very straightforward way to accomplish the goals here and it is largely unnecessary except that it has a very small potential to have slight roundoff errors that spook the underlying graphics engine (Skia) from using a straight pixel-for-pixel blit which was very fast on slow hardware into using a (degenerately unnecessary) scaling GPU shader. As a result, they explicitly dumbed down the straight cache blit to a hard-coded identity-transform blit to avoid any potential for round-off mistakes.

But the code you are adding is already intending to invoke a transformed blit operation anyway, so rather than try to code it to match the identity-space clamped blit operation that cache::draws normally use, just draw the cache entry with the appropriate transform.

flar · 2022-05-23T22:35:48Z

flow/raster_cache.cc

+    matrix->mapRect(&dest_rect, logical_rect_);
+    SkRect dest_rect_2;
+    device_total_matrix.mapRect(&dest_rect_2, dest_rect);
+    canvas.drawImageRect(image_, dest_rect_2, SkSamplingOptions(), paint);


Also, don't try to do this variant of the cache blit with bounding rectangles. Supply the rendering call with the transform you need and let the graphics engine do the work. There's some transform math to be done here (the original matrix produced this cache image and now there's a new matrix to render under), but you could allow for any kind of transform matrix here.

Basically I think we can assert that the entry was cached with the same matrix we are rendering with or we wouldn't have gotten this far. The only difference are the translation components. Then we want to apply the given matrix parameter on top of that, and translate it to the current canvas's translation component and draw.

flar · 2022-05-23T22:40:35Z

flow/raster_cache.cc

+    matrix->mapRect(&dest_rect, logical_rect_);
+    SkRect dest_rect_2;
+    device_total_matrix.mapRect(&dest_rect_2, dest_rect);
+    canvas.drawImageRect(image_, dest_rect_2, SkSamplingOptions(), paint);


The ideal would be for there to be a drawImageQuad() where you specify the 4 points of the quad into which to render the image, but there is drawAtlas that lets you specify a scaled parallelogram into which to render the image. It would work for any scaled and rotated transform. Beyond that there is drawVertices which allows you to specify 2 triangles into which to map the bitmap.

flar · 2022-05-23T22:43:28Z

I was thinking of moving the implementation from ImageFilterLayer to TransformLayer with the same hint, or BitmapTransformLayer.

I believe @dnfield brought this up earlier and I had a similar question: If swap this out with a transform layer, don't we end up invalidating our raster cached child as we animate it? Or does this essentially become "implement raster cache for transform layer", but more restricted?

Not any more than the current implementation that uses an ImageFilterLayer...?

Basically, in the code we replace if (filterQuality) then IFL(matrix, quality, child) with if (filterQuality) then TransformLayer(matrix, quality, child) or if (filterQuality) then BitmapTransformLayer(matrix, quality, child)

None of those are any more likely to invalidate than the others.

jonahwilliams · 2022-05-23T23:11:20Z

Yeah perspective is ... I don't know how to deal with that here, which is why I took the easy way out and only implemented scale/translate :)

flar · 2022-05-23T23:16:33Z

Yeah perspective is ... I don't know how to deal with that here, which is why I took the easy way out and only implemented scale/translate :)

I deleted a previous comment because the cached image bounds are the bounds of the transformed logical bounds.

jonahwilliams · 2022-05-23T23:22:08Z

Would drawVertices be sufficient to cover the perspective case?

jonahwilliams · 2022-05-23T23:24:00Z

The problem here is that the code for the untransformed cache draw is not a very straightforward way to accomplish the goals here and it is largely unnecessary except that it has a very small potential to have slight roundoff errors that spook the underlying graphics engine (Skia) from using a straight pixel-for-pixel blit which was very fast on slow hardware into using a (degenerately unnecessary) scaling GPU shader. As a result, they explicitly dumbed down the straight cache blit to a hard-coded identity-transform blit to avoid any potential for round-off mistakes.

I'm sort of following you here - this is essentially the pixel snap that everything interacting with the raster cache performs. if this is unnecessary for correctness, in the case of having an arbitrary matrix, what is the best way to handle this correctly? Could we avoid resetting the canvas total matrix, or am I misunderstanding that code?

jonahwilliams · 2022-05-23T23:29:55Z

actually, if we're just giving the transform to the canvas, do we just call drawImage just like before?

jonahwilliams · 2022-05-24T17:14:51Z

So for vertices, I think in theory something like the following works:

    FML_DCHECK(sampling != nullptr);

    auto left = logical_rect_.left();
    auto top = logical_rect_.top();
    auto right = logical_rect_.right();
    auto bottom = logical_rect_.bottom();
    SkPoint triangles[6] = {
        SkPoint::Make(left, top),     SkPoint::Make(right, top),
        SkPoint::Make(right, bottom), SkPoint::Make(left, top),
        SkPoint::Make(left, bottom),  SkPoint::Make(right, bottom),
    };
    SkPoint input[6] = {
        SkPoint::Make(left, top),     SkPoint::Make(right, top),
        SkPoint::Make(right, bottom), SkPoint::Make(left, top),
        SkPoint::Make(left, bottom),  SkPoint::Make(right, bottom),
    };
    matrix->mapPoints(triangles, 6);
    SkPaint paint_copy;
    paint_copy.setShader(
        image_->makeShader(SkTileMode::kClamp, SkTileMode::kClamp, *sampling));
    auto vertices =
        SkVertices::MakeCopy(SkVertices::VertexMode::kTriangles_VertexMode, 6,
                             triangles, input, nullptr);
    canvas.drawVertices(vertices, SkBlendMode::kSrcIn, paint_copy);

Except that that it doesn't quite look right (I know that i'm not correctly merging the paints)

…scale_optimization

flar · 2022-05-24T19:10:00Z

This is why I deleted my previous comment. The problem is that you are transforming the corners of the original logical rect, but what is in the cache entry is the bounds of those transformed corners. So, imagine if there is a 45 degree rotation. You would transform the bounds of the logical rect and then have to take the bounds and those bounds would represent 4 points that are outside the bounds of the original logical rect.

flar · 2022-05-24T19:14:34Z

I don't think Vertices or Atlas can deal with perspective, but let me think about it. They only offer x,y coordinates for instance.

flar · 2022-05-24T19:15:11Z

I was leaning towards Atlas because it didn't require turning the image into a shader. I'm not sure if that makes it any faster or slower, though.

jonahwilliams · 2022-05-24T19:16:21Z

I think special casing scale/translate is OK FWIW - though I'd love to hear others thoughts @dnfield @zanderso . This sort of transform is used by the ZoomPageTransition which is the default on Android, so it is very heavily used. Rotations and perspective are not so common

flar · 2022-05-24T20:43:05Z

I'm saying that because we cache those, you need to check that the transform that was cached was only a scale/translate since

Which matrix are you referring to? The image filter matrix, the canvas total matrix? .. et cetera

Any of them.

flar · 2022-05-24T20:47:53Z

flow/raster_cache.cc

+    FML_DCHECK(matrix->isScaleTranslate());
+
+    SkRect dest_rect;
+    matrix->mapRect(&dest_rect, logical_rect_);


mapRect sorts the points so if you have negative scales this will fail to produce the results you are looking for.

Added a check for negative scales

flar · 2022-05-24T20:49:21Z

I'm saying that because we cache those, you need to check that the transform that was cached was only a scale/translate since

Which matrix are you referring to? The image filter matrix, the canvas total matrix? .. et cetera

It's even worse because isScaleTranslate() is true when there are negative scales, but the logic you use in RCE::Draw is not compatible with negative scales. So, not only do you need to check both transforms, you also need to ensure that they don't have negative scales.

jonahwilliams · 2022-05-24T21:04:59Z

ToT: current zoom page transition using FilterQuality.low, wrapped in an AnimatedRotation:

flutter_gallery.2022-05-24.14-02-38.mp4

This change:

flutter_gallery.2022-05-24.14-00-43.mp4

jonahwilliams · 2022-05-24T21:06:04Z

I'm not resetting the canvas total matrix so that I don't have to multiply it with the image filter transform, I think is helping here?

flar · 2022-05-25T08:36:34Z

flow/layers/image_filter_layer.cc

+ImageFilterLayer::ImageFilterLayer(sk_sp<SkImageFilter> filter)
+    : filter_(filter == nullptr
+                  ? nullptr
+                  : std::make_shared<DlUnknownImageFilter>(filter)),


AKA DlImageFilter::From(filter)

flar · 2022-05-25T08:39:54Z

flow/layers/image_filter_layer.cc

+      auto matrix = &matrix_filter->matrix();
+      auto sampling = &matrix_filter->sampling();
+      if (matrix->isScaleTranslate() && matrix->getScaleX() >= 0.0 &&
+          matrix->getScaleY() >= 0.0 &&


And leaf_nodes_canvas has to be ScaleTranslate with non-negative scales.

What is a test case I could use for this? a non-composited transform drawn in the same canvas?

added a few...

Try a 45 degree rotation parent transform followed by a non-uniform scale in the bitmap scaling child. That should produce interesting results. Try it with both being regular transform layers and then with one of them being a bitmap scaling layer.

What should happen is that the child should be stretched in its rotated space - getting longer to the LR and LL in the screen space - but I think you'll see it distort being scaled to the right and down instead of LR and LL.

If either transform is not a non-negative scaletranslate then the math involved is a lot more complicated than what is written here. A non-ST parent transform means that the scales and translates in the child transform that comes from the IFL should be acting on different axes. A non-ST child matrix in the IFL would distort the axis aligned cached image box into a non-rect. Either can cause problems.

Ahh, I used uniform scaling above so perhaps I didn't notice this.

ds84182 · 2022-05-25T18:05:08Z

To chime in a bit, #31443 (which built off of RasterCache work from #22045) both properly handle rotation. Not sure about perspective transforms though.

jonahwilliams · 2022-05-25T19:43:57Z

I've updated ImageFilter layer to only use this optimization for scaleTranslate on both the image filter matrix and the ctm.

dnfield

Lgtm if lgt @flar

flar · 2022-05-25T20:55:20Z

To chime in a bit, #31443 (which built off of RasterCache work from #22045) both properly handle rotation. Not sure about perspective transforms though.

In general, I'd rather see an approach like #31443 than this PR, but 31443 has a number of problems with it as well...

Why does #31443 add a scale?

~~The scale would appear to break its caching as it is not factored into the multiple transforms and so the cache hit will miss.~~ (It looks like the scale is added to the matrix in Preroll so this shouldn't be an issue)

The wrong canvas is used to draw the cache entry - leaf nodes canvas is used for drawing, internal nodes is used for clip/save/transform ops that will persist to the children and those operations only.

I haven't read it closely for other issues, but since IFL is performing poorly and this PR is introducing a bunch of code to only solve some of its issues, I think I'd prefer to just go with a dedicated solution that solves more transform cases (even if neither can do perspective).

flar · 2022-05-25T20:59:29Z

@jonahwilliams @dnfield how do you feel about reopening and exploring #31443 alongside this?

A third option is to add the RTL code from 31443 to the regular TransformLayer as an "if sampling != null" option.

jonahwilliams · 2022-05-25T21:01:56Z

I'm not against the idea of making this more general, but it doesn't really solve the immediate problem of the zoom page transition being quite slow for customer money

jonahwilliams · 2022-05-25T21:02:57Z

would the idea be to completely replace image filter+matrix with RasterTransformLayer? if we could make it work for any transform that would simplify things signficantly - though we might still need a fast path for scale+translate

flar · 2022-05-25T21:12:13Z

I'm not against the idea of making this more general, but it doesn't really solve the immediate problem of the zoom page transition being quite slow for customer money

How does it not solve the problem? (Other than it needs a round of reviews)

flar · 2022-05-25T21:15:10Z

would the idea be to completely replace image filter+matrix with RasterTransformLayer? if we could make it work for any transform that would simplify things signficantly - though we might still need a fast path for scale+translate

First, the fast path for scale+translate isn't fast because it is scale/translate. It is fast because it is bypassing the ImageFilter. Both solutions bypass the ImageFilter so once that is done it doesn't matter if the matrix is scale/translate or not - all of the matrices will be fast.

The idea would be that if a Flutter widget adds the raster flag, we currently funnel that into ImageFilterLayer which is faster than just doing the transform on every frame, but it is slower than a direct solution that just controls the matrix of the cache draw.

So, at the point in Flutter framework where we decide "Should we use TransformLayer here or ImageFilterLayer?" we would change that to "Should we use TL or RTL"?

jonahwilliams · 2022-05-25T21:19:00Z

TBH I'm a bit skeptical about introducing yet another method to the framework to transform children. Optimizing image filter layer behind the scenes feels like a better short term solution

flar · 2022-05-25T21:26:46Z

TBH I'm a bit skeptical about introducing yet another method to the framework to transform children. Optimizing image filter layer behind the scenes feels like a better short term solution

We could at least have your solution adopt the general solution from 31443.

At that point RTL vs IFL is just whether it is better to have a specific layer for this, or side effect it off of IFL as we are doing. You'll have introduced all of the code that would be in RTL into IFL but wrapped it in checks for a Matrix filter. So, we can have:

IFL with isMatrix checks
TL with hasSampling checks
RTL that just does it

Perhaps the optimization for "this is an IFL with a matrix" could be done at the native scene_builder level which creates an IFL - it could check and create an RTL without the framework even knowing.

jonahwilliams · 2022-05-25T21:28:10Z

doing it at the scene builder level SGTM.

jonahwilliams · 2022-05-25T21:53:55Z

@flar I think the only question I'd have is if you're okay with me (or someone else) handling that in a follow up or if you think that needs to be done in this PR. I thought that @dnfield and @zanderso agreed that this was worth special casing as well.

flar · 2022-05-25T22:23:01Z

Right now my focus is on getting #31892 landed before any other changes to RasterCache.

jonahwilliams · 2022-05-25T22:27:25Z

ack, we'll circle back to this another time

make zoom page transition fast

742b10d

jonahwilliams commented May 23, 2022

View reviewed changes

flow/BUILD.gn Outdated Show resolved Hide resolved

jonahwilliams commented May 23, 2022

View reviewed changes

dnfield reviewed May 23, 2022

View reviewed changes

plumb SkSamplingOptions

f3a56e0

flar reviewed May 23, 2022

View reviewed changes

Jonah Williams added 2 commits May 23, 2022 23:13

layer cache, cleanup

9fc9bd5

Remove debugging

4563a99

Jonah Williams added 4 commits May 24, 2022 10:41

fix unittests

9a54b3f

fix docs

e6c4a9d

Merge branch 'master' of github.com:flutter/engine into image_filter_…

5b21b60

…scale_optimization

update tests that assert on SkImageFilter

bb2a284

jonahwilliams changed the title ~~make zoom page transition fast~~ Convert ImageFilter.matrix with scale/translate into blit with sampling options May 24, 2022

++

ba25962

jonahwilliams marked this pull request as ready for review May 24, 2022 19:09

flar reviewed May 24, 2022

View reviewed changes

Jonah Williams added 2 commits May 24, 2022 14:46

positive scale and doc comment fix

6ded8f2

Format fix

a5b5cfe

flar reviewed May 25, 2022

View reviewed changes

flar suggested changes May 25, 2022

View reviewed changes

add unittests

76a3d21

jonahwilliams requested review from dnfield and flar May 25, 2022 19:08

jonahwilliams mentioned this pull request May 25, 2022

[framework] reland FilterQuality for zoom page transition flutter/flutter#104649

Closed

dnfield approved these changes May 25, 2022

View reviewed changes

jonahwilliams closed this May 25, 2022

jonahwilliams mentioned this pull request May 26, 2022

Determine a fix for the zoom page transition performance flutter/flutter#104680

Closed

Convert ImageFilter.matrix with scale/translate into blit with sampling options #33562

Convert ImageFilter.matrix with scale/translate into blit with sampling options #33562

Uh oh!

Conversation

jonahwilliams commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented May 23, 2022

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 May 23, 2022

Uh oh!

jonahwilliams commented May 23, 2022

Uh oh!

jonahwilliams commented May 23, 2022

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!

flar commented May 23, 2022

Uh oh!

jonahwilliams commented May 23, 2022

Uh oh!

flar commented May 23, 2022

Uh oh!

jonahwilliams commented May 23, 2022

Uh oh!

jonahwilliams commented May 23, 2022

Uh oh!

jonahwilliams commented May 23, 2022

Uh oh!

jonahwilliams commented May 24, 2022

Uh oh!

flar commented May 24, 2022

Uh oh!

flar commented May 24, 2022

Uh oh!

flar commented May 24, 2022

Uh oh!

jonahwilliams commented May 24, 2022

Uh oh!

flar commented May 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar commented May 24, 2022

Uh oh!

jonahwilliams commented May 23, 2022 •

edited

Loading

flar commented May 25, 2022 •

edited

Loading