Implement support for non-scale/translate CTM in image filters.

Implement support for non-scale/translate CTM in image filters.

Implemented by extracting out the non-scale/translate components
and applying that post-filter as an SkMatrixImageFilter.

[Stephen White, 2015-03-06 20:20:24]

senorblanco@chromium.org changed reviewers:
+ reed@google.com

[Stephen White, 2015-03-16 18:19:11]

New version up: stash the original matrix on the DeviceCM, instead of trying to
use internalSave()/internalRestore().

[Stephen White, 2015-03-16 19:02:03]

In order to eliminate the extra blit incurred by the non-scale/translate case, I
think we'dhave to stash the other "half" of the matrix (the non-scale/translate
bits) in DeviceCM as well, and then apply it in place of the drawSprite() in
internalDrawDevice(). In order to get the matrix at that point, we'd either have
to pass in the DeviceCM into internalDrawDevice(), or perhaps just the matrix
(maybe internalDrawDevice could take a matrix instead of a position? That would
make it different from drawDevice(), though.)

We'd have to make these same changes in SkGpuDevice I think, since it does its
own filter application in its drawDevice() and drawSprite(), using different
code paths.

We'd also have to modify clipRectBounds() to take the "extra" matrix, and bring
in parts of SkMatrixImageFilter::onFilterBounds().

I think this optimization is pretty intrusive, so I still recommend landing the
above as-is (since it's now very self-contained), and working on the
optimization in a separate patch.

[reed1, 2015-03-16 21:18:11]

not sure I'm convinced we need to land an intermediate cl, but that aside...

1. I think making some form of MatrixImageFilter be in core is fine. I would
most prefer just a static factory on SkImageFilter (see SkColorFilter.h or
SkShader.h for exposing special subclass just via factories).

2. I'd really like to share code somehow in decomposing the matrix, with other
places in skia that do this...

3. We are directly wacking the ctm in fMCRec, but not notifying devices that it
has changed. This may be fine, but I need to look more into to be sure. If we
had a way to affect the ctm using existing calls (e.g. whatever canvas::concat
does) that might be cleaner.

4. Before we land anything like this, we should be sure we have GMs that already
exercise affine matrices and imagefilters... so we can see the effect.

5. This cl passes matrices by value instead of const&, which is more efficient.

[reed1, 2015-03-16 21:18:33]

reed@google.com changed reviewers:
+ robertphillips@google.com

[Stephen White, 2015-03-16 22:03:41]

On 2015/03/16 21:18:11, reed1 wrote:
> not sure I'm convinced we need to land an intermediate cl, but that aside...
> 
> 1. I think making some form of MatrixImageFilter be in core is fine. I would
> most prefer just a static factory on SkImageFilter (see SkColorFilter.h or
> SkShader.h for exposing special subclass just via factories).

Done. Unfortunately, Blink still needs SkMatrixImageFilter exposed as API to
handle
the filterRes parameter, even after this change. We can change Blink to use the
factory function after this lands, though. Then we can move the header
to src/core.

> 2. I'd really like to share code somehow in decomposing the matrix, with other
> places in skia that do this...

OK; moved to SkMatrix.h, and refactored with the version in SkPictureShader. Let
me know if you'd prefer somewhere else.

> 3. We are directly wacking the ctm in fMCRec, but not notifying devices that
it
> has changed. This may be fine, but I need to look more into to be sure. If we
> had a way to affect the ctm using existing calls (e.g. whatever canvas::concat
> does) that might be cleaner.

Changed to use SkCanvas::setMatrix().

> 4. Before we land anything like this, we should be sure we have GMs that
already
> exercise affine matrices and imagefilters... so we can see the effect.

We used to have bots that ran all GMs with a perspective matrix. Are they still
around?

> 5. This cl passes matrices by value instead of const&, which is more
efficient.

Fixed.

[Stephen White, 2015-03-16 22:05:13]

BTW, I have an attempt at the blit optimization here:
https://codereview.chromium.org/1016483003/

Raster-only, and it doesn't work, though. The non-scale/translate parts seem to
be applied correctly, but the offset is wrong (e.g., imagefiltersclipped GM
w/rotate in SampleApp).

[reed1, 2015-03-19 17:47:30]

Seems much better, but I'd still like to split out the parts that are not
actually affecting savelayer for the moment...

1. Make matriximagefilter core -- just moving the impl and adding the factory
(with dox)

2. Add the matrix helper and use it in (3) places (including
SkBitmapProcState.cpp) -- I may propose a different signature for this if you're
ok w/ me doing this part.

3. whatever is left

[Stephen White, 2015-03-19 18:45:20]

On 2015/03/19 17:47:30, reed1 wrote:
> Seems much better, but I'd still like to split out the parts that are not
> actually affecting savelayer for the moment...
> 
> 1. Make matriximagefilter core -- just moving the impl and adding the factory
> (with dox)

Done:  https://codereview.chromium.org/1011273003

> 2. Add the matrix helper and use it in (3) places (including
> SkBitmapProcState.cpp) -- I may propose a different signature for this if
you're
> ok w/ me doing this part.

Sure, I'll wait.

> 3. whatever is left

[Stephen White, 2015-03-20 13:34:41]

Ok this patch now has just:

> > 3. whatever is left

[robertphillips, 2015-03-20 14:01:10]

first pass

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
File gm/imagefilterstransformed.cpp (right):

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:20: 
// This GM checks that the scale portion of the applied matrices is correctly
extracted and applied to the image inputs separate from the non-scale portion

?

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:28: 
one line ?

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:32: 
one line ?

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:36: 
makeGradientCircle ?

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:56: void onDraw(SkCanvas* canvas) SK_OVERRIDE {
Can we move this stuff to a onOnceBeforeDraw method and get rid of fInitialized
?

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:82: 
Can we make these two guys const ?

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:123: 
Use DEF_GM macro ?

https://codereview.chromium.org/986623003/diff/340001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/986623003/diff/340001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:115: SkPaint*            fPaint; // may be null (in the
future)
// The stashed matrix is used for ... ?

https://codereview.chromium.org/986623003/diff/340001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:442: 
overlength ?

https://codereview.chromium.org/986623003/diff/340001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:905: SkImageFilter* imageFilter = paint ?
paint->getImageFilter() : NULL;
Can matrix be a "const&" rather than a copy ?

[reed1, 2015-03-20 14:09:42]

Its great that its down to one file, but its still tricky :)

I vote to land the GM right away, so we can see before/afters and experiment
with variations of the proposed solution.

[Stephen White, 2015-03-20 14:19:08]

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
File gm/imagefilterstransformed.cpp (right):

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:20: 
On 2015/03/20 14:01:09, robertphillips wrote:
> // This GM checks that the scale portion of the applied matrices is correctly
> extracted and applied to the image inputs separate from the non-scale portion
> 
> ?

Done (with tweaks).

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:28: 
On 2015/03/20 14:01:09, robertphillips wrote:
> one line ?

Done.

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:32: 
On 2015/03/20 14:01:09, robertphillips wrote:
> one line ?

Done.

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:36: 
On 2015/03/20 14:01:09, robertphillips wrote:
> makeGradientCircle ?

Done.

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:56: void onDraw(SkCanvas* canvas) SK_OVERRIDE {
On 2015/03/20 14:01:09, robertphillips wrote:
> Can we move this stuff to a onOnceBeforeDraw method and get rid of
fInitialized
> ?

Done.

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:82: 
On 2015/03/20 14:01:09, robertphillips wrote:
> Can we make these two guys const ?

Done.

https://codereview.chromium.org/986623003/diff/340001/gm/imagefilterstransfor...
gm/imagefilterstransformed.cpp:123: 
On 2015/03/20 14:01:09, robertphillips wrote:
> Use DEF_GM macro ?

Done.

[Stephen White, 2015-03-20 14:28:02]

Patchset #19 (id:360001) has been deleted

[Stephen White, 2015-03-20 14:31:42]

On 2015/03/20 14:09:42, reed1 wrote:
> Its great that its down to one file, but its still tricky :)
> 
> I vote to land the GM right away, so we can see before/afters and experiment
> with variations of the proposed solution.

Done: https://codereview.chromium.org/1028663002/

[Stephen White, 2015-03-20 14:47:21]

https://codereview.chromium.org/986623003/diff/340001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/986623003/diff/340001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:442: 
On 2015/03/20 14:01:09, robertphillips wrote:
> overlength ?

Done.

https://codereview.chromium.org/986623003/diff/340001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:905: SkImageFilter* imageFilter = paint ?
paint->getImageFilter() : NULL;
On 2015/03/20 14:01:09, robertphillips wrote:
> Can matrix be a "const&" rather than a copy ?

No. fMCRec->fMatrix will be modified by resetMatrix()/scale() below, which will
hose the ref (I think).

[Stephen White, 2015-03-20 14:55:11]

BTW, one thing that occurred to me: if we implement the SkShader optimization
where the last image filter returns its result as an SkShader, the blit
optimization (to apply the matrix during blit) will naturally fall out of
implementing that for SkMatrixImageFilter. So that's one argument for landing
this.

[robertphillips, 2015-03-20 15:02:26]

https://codereview.chromium.org/986623003/diff/420001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/986623003/diff/420001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:920: // that would invoke a possibly overridden virtual
So, internalSave pushes the MCRec stack. Why don't we munge his matrix rather
than the matrix of the prior stack frame? That way we wouldn't have to stash a
matrix since the restore would just pop of the munged matrix.

[Stephen White, 2015-03-20 15:07:33]

https://codereview.chromium.org/986623003/diff/420001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/986623003/diff/420001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:920: // that would invoke a possibly overridden virtual
On 2015/03/20 15:02:26, robertphillips wrote:
> So, internalSave pushes the MCRec stack. Why don't we munge his matrix rather
> than the matrix of the prior stack frame? That way we wouldn't have to stash a
> matrix since the restore would just pop of the munged matrix.

Because the pop happens too early: we need the munged matrix in effect when we
call internalDrawDevice() from internalRestore(), which happens after the matrix
is popped.

[Stephen White, 2015-03-25 22:54:21]

Patchset #23 (id:460001) has been deleted

[Stephen White, 2015-03-25 22:56:19]

Ping?

Are there any better ideas for solving this problem?

[reed1, 2015-03-26 15:54:03]

https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:911: this->resetMatrix();
these two calls for matrix changes are virtual, which we don't want to make
(esp. if we're in a picture-recorder).

https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:913: SkAutoTUnref<SkImageFilter>
matrixFilter(SkImageFilter::CreateMatrixFilter(remainder,
SkFilterQuality::kHigh_SkFilterQuality, imageFilter));
kLow might be more honest, since by definition remainder has rotation/skew which
we always leave to kLow (bilerp).

https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1025: if (layer->fPaint && layer->fPaint->fImageFilter) {
the check seems a little fragile, since it somehow knows the predicate being
used in internalSave. I think an explicit flag in the layer, or just always
copying it, would be clearer.

[Stephen White, 2015-03-26 16:18:34]

Thanks for your review. New patch up.

https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:911: this->resetMatrix();
On 2015/03/26 15:54:03, reed1 wrote:
> these two calls for matrix changes are virtual, which we don't want to make
> (esp. if we're in a picture-recorder).

Hmm -- they don't seem to be virtual on SkCanvas? At any rate, I was messing the
fMCRec->fMatrix directly before, and you asked me to change it. I must've
misunderstood -- what should I change it to?

https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:913: SkAutoTUnref<SkImageFilter>
matrixFilter(SkImageFilter::CreateMatrixFilter(remainder,
SkFilterQuality::kHigh_SkFilterQuality, imageFilter));
On 2015/03/26 15:54:03, reed1 wrote:
> kLow might be more honest, since by definition remainder has rotation/skew
which
> we always leave to kLow (bilerp).

Done.

https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1025: if (layer->fPaint && layer->fPaint->fImageFilter) {
On 2015/03/26 15:54:03, reed1 wrote:
> the check seems a little fragile, since it somehow knows the predicate being
> used in internalSave. I think an explicit flag in the layer, or just always
> copying it, would be clearer.

Done.

[reed1, 2015-03-26 16:23:22]

On 2015/03/26 16:18:34, Stephen White wrote:
> Thanks for your review. New patch up.
> 
> https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp
> File src/core/SkCanvas.cpp (right):
> 
>
https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp#n...
> src/core/SkCanvas.cpp:911: this->resetMatrix();
> On 2015/03/26 15:54:03, reed1 wrote:
> > these two calls for matrix changes are virtual, which we don't want to make
> > (esp. if we're in a picture-recorder).
> 
> Hmm -- they don't seem to be virtual on SkCanvas? At any rate, I was messing
the
> fMCRec->fMatrix directly before, and you asked me to change it. I must've
> misunderstood -- what should I change it to?

resetMatrix -> setMatrix -> virtual didSetMatrix

> 
>
https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp#n...
> src/core/SkCanvas.cpp:913: SkAutoTUnref<SkImageFilter>
> matrixFilter(SkImageFilter::CreateMatrixFilter(remainder,
> SkFilterQuality::kHigh_SkFilterQuality, imageFilter));
> On 2015/03/26 15:54:03, reed1 wrote:
> > kLow might be more honest, since by definition remainder has rotation/skew
> which
> > we always leave to kLow (bilerp).
> 
> Done.
> 
>
https://codereview.chromium.org/986623003/diff/480001/src/core/SkCanvas.cpp#n...
> src/core/SkCanvas.cpp:1025: if (layer->fPaint && layer->fPaint->fImageFilter)
{
> On 2015/03/26 15:54:03, reed1 wrote:
> > the check seems a little fragile, since it somehow knows the predicate being
> > used in internalSave. I think an explicit flag in the layer, or just always
> > copying it, would be clearer.
> 
> Done.

[Stephen White, 2015-03-26 16:33:19]

Ok new patch up: back to setting fMCRec->fMatrix directly, but we also set the
dirty flags as concat() and friends do.