Fix deferred filter application with shearing or rotating CTM.

Fix deferred filter application with shearing or rotating CTM.

NOTE: this requires Skia change
https://codereview.chromium.org/211103006/ to land and roll into
Chromium.

Pixel-moving filters (e.g., blur, morphology) don't work well with
matrices applying anything more than scale and translate. (We do a
best-effort, but it's really wrong.) To fix, we separate out only
the scaling and translation components from the CTM, process the filters
with that, and apply the remainder of the CTM as a final matrix
transform image filter.

Since the matrix image filter is a generalization of the resize image
filter, we switch the filterRes implementation over to use that as well.

Covered by svg/batik/filters/feTile.svg and svg/filters/filterRes.svg
when run with --enable-deferred-filters.

R=fmalita@chromium.org
BUG=353630
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170104

[Stephen White, 2014-03-25 20:02:18]

Florin: PTAL. Thanks!

[f(malita), 2014-03-26 13:28:25]

It's unfortunate we still care about the record-time CTM with deferred
filters... will this still work if the playback rotation is different (not that
I think it's possible today)?

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:245:
scaleAndTranslate.scale(ctm.xScale(), ctm.yScale());
Why not scale(ctm.a(), ctm.d())?

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:246:
scaleAndTranslate.translate(ctm.e(), ctm.f());
Translate first to preserve the original translation?

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:247: AffineTransform
shearAndRotate = scaleAndTranslate.inverse();
Is scaleAndTranslate guaranteed to be invertible? I think so, but we should
probably assert so.

[f(malita), 2014-03-26 13:30:46]

Also, is fixing the Skia filters to handle arbitrary transforms not feasible?

[Stephen White, 2014-03-26 19:09:51]

> It's unfortunate we still care about the record-time CTM with deferred
filters... will this still work if the playback rotation is different (not that
I think it's possible today)?

You're right that it won't handle rotation at playback time, but that AFAIK
doesn't happen in practice: device pixel ratio and pinch-zoom are only scales.

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:245:
scaleAndTranslate.scale(ctm.xScale(), ctm.yScale());
On 2014/03/26 13:28:26, Florin Malita wrote:
> Why not scale(ctm.a(), ctm.d())?

xScale() and yScale() handle the rotated case better: if you have a rotation of
89 degrees, for example, ctm.a() will be very small, and the resized filter will
have low resolution in X. By using xScale() and yScale(), the scaling lengths
always stay at 1 for a rotating-only filter.

Note that this is what the old path uses, above.

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:246:
scaleAndTranslate.translate(ctm.e(), ctm.f());
On 2014/03/26 13:28:26, Florin Malita wrote:
> Translate first to preserve the original translation?

For some reason, it was causing incorrect results when I did it this way. I'll
look at it again to see what's going on.

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:247: AffineTransform
shearAndRotate = scaleAndTranslate.inverse();
On 2014/03/26 13:28:26, Florin Malita wrote:
> Is scaleAndTranslate guaranteed to be invertible?

Yes, it's guaranteed.

> I think so, but we should probably assert so.

Good idea; will do.

[Stephen White, 2014-03-26 20:26:08]

New patch up.

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGResourceFilter.cpp (right):

https://codereview.chromium.org/208683008/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGResourceFilter.cpp:246:
scaleAndTranslate.translate(ctm.e(), ctm.f());
On 2014/03/26 19:09:52, Stephen White wrote:
> On 2014/03/26 13:28:26, Florin Malita wrote:
> > Translate first to preserve the original translation?
> 
> For some reason, it was causing incorrect results when I did it this way. I'll
> look at it again to see what's going on.

Fixed; you were absolutely correct.

(Note that it doesn't actually matter what translation we apply (even none at
all!), since we apply its inverse below. Still, I'll leave it this way since it
seems clearer to apply CTM translation in the saveLayer() call than in the
SkMatrixImageFilter.)

[f(malita), 2014-03-26 20:28:32]

lgtm

[Stephen White, 2014-03-26 20:38:25]

On 2014/03/26 13:30:46, Florin Malita wrote:
> Also, is fixing the Skia filters to handle arbitrary transforms not feasible?

I'd like to do that longer-term, but there are some architectural and code
placement issues that I need to resolve.

[Stephen White, 2014-03-26 20:47:42]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-03-26 20:47:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/senorblanco@chromium.org/208683008/110001

[commit-bot: I haz the power, 2014-03-26 21:32:43]

Change committed as 170104