antialias matrix-image-filter to get smooth diagonals

antialias matrix-image-filter to get smooth diagonals

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/fa33f5a6b770130acdc55f2ffe19dd545665726a

[reed1, 2015-05-14 19:50:25]

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

[reed1, 2015-05-14 19:50:26]

aa makes the edges look nice, but wanks out when on GPU :(

see gm:matriximagefilter for the wank

[Stephen White, 2015-05-14 20:09:09]

On 2015/05/14 19:50:26, reed1 wrote:
> aa makes the edges look nice, but wanks out when on GPU :(
> 
> see gm:matriximagefilter for the wank

Strange. It's making pretty generic Skia calls. I wonder why we don't see this
in other GPU tests that draw bitmaps w/AA.

[reed1, 2015-05-14 21:13:41]

seems to be related to the exact-size -vs- sloppy/fast size for the texture.

[Stephen White, 2015-05-14 21:31:16]

On 2015/05/14 20:09:09, Stephen White wrote:
> On 2015/05/14 19:50:26, reed1 wrote:
> > aa makes the edges look nice, but wanks out when on GPU :(
> > 
> > see gm:matriximagefilter for the wank
> 
> Strange. It's making pretty generic Skia calls. I wonder why we don't see this
> in other GPU tests that draw bitmaps w/AA.

At a guess, it looks like an issue with drawing a scratch texture at texture
size instead of bitmap size (ie., it's drawing a whole scratch texture including
garbage).

BTW, since this filter is already being used in Chrome without AA, I think we
could live without AA for the time being if necessary.

[Stephen White, 2015-05-14 22:06:35]

This fix seems to work for the GPU side. Brian, does this look correct?

Note also that there may be a similar bug in the maskfilter case w/scratch
textures (see the other half of the
if-stmt above this); I'm not sure.

diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
index 7b36363..9e95601 100644
--- a/src/gpu/SkGpuDevice.cpp
+++ b/src/gpu/SkGpuDevice.cpp
@@ -1235,7 +1235,8 @@ void SkGpuDevice::drawBitmapCommon(const SkDraw& draw,
                 localM.preTranslate(offset.fX, offset.fY);
             }
         } else {
-            localM.reset();
+            localM.setScale(SkIntToScalar(width) / bitmap.width(),
+                            SkIntToScalar(height) / bitmap.height());
         }

         SkPaint paintWithShader(paint);

[bsalomon, 2015-05-15 13:51:02]

On 2015/05/14 22:06:35, Stephen White wrote:
> This fix seems to work for the GPU side. Brian, does this look correct?
> 
> Note also that there may be a similar bug in the maskfilter case w/scratch
> textures (see the other half of the
> if-stmt above this); I'm not sure.
> 
> diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
> index 7b36363..9e95601 100644
> --- a/src/gpu/SkGpuDevice.cpp
> +++ b/src/gpu/SkGpuDevice.cpp
> @@ -1235,7 +1235,8 @@ void SkGpuDevice::drawBitmapCommon(const SkDraw& draw,
>                  localM.preTranslate(offset.fX, offset.fY);
>              }
>          } else {
> -            localM.reset();
> +            localM.setScale(SkIntToScalar(width) / bitmap.width(),
> +                            SkIntToScalar(height) / bitmap.height());
>          }
> 
>          SkPaint paintWithShader(paint);

I believe that it is more correct.

We generally do not really support scratch texture in bitmaps very well. It's
hard to catch all the cases where we can read outside of the content area,
(bilerp, mipmaps, filtering, tiling, ...) and I'd rather stop trying to get all
the corner cases right. I hope as we move away from SkBitmap inputs to SkImage
we can create a pinch point in Ganesh that goes from image to texture and makes
a copy to a tight texture whenever the caller can't promise it is doing a 1
texel to 1 pixel draw.

[reed1, 2015-05-15 15:50:57]

New approach : tweak saveLayer to know when it can't cheat on the layer's
surface

[reed1, 2015-05-15 15:51:22]

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

[reed1, 2015-05-15 16:05:44]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-15 16:06:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134743003/60001

[commit-bot: I haz the power, 2015-05-15 16:12:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-15 16:12:23]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-05-15 17:05:19]

ptal -- may rename the enum, but it will affect a lot of files incidentally, so
I'd like to to that in a follow-on CL

[bsalomon, 2015-05-15 17:08:33]

lgtm

[robertphillips, 2015-05-15 17:11:08]

https://codereview.chromium.org/1134743003/diff/60001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/1134743003/diff/60001/include/core/SkImageFil...
include/core/SkImageFilter.h:348: bool mayDrawWithMatrix() const { return
this->onMayDrawWithMatrix(); }
"may Ganesh" ?

https://codereview.chromium.org/1134743003/diff/60001/src/core/SkMatrixImageF...
File src/core/SkMatrixImageFilter.cpp (right):

https://codereview.chromium.org/1134743003/diff/60001/src/core/SkMatrixImageF...
src/core/SkMatrixImageFilter.cpp:86: 
Do we still need this clear ?

[reed1, 2015-05-15 17:24:25]

https://codereview.chromium.org/1134743003/diff/60001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/1134743003/diff/60001/include/core/SkImageFil...
include/core/SkImageFilter.h:348: bool mayDrawWithMatrix() const { return
this->onMayDrawWithMatrix(); }
On 2015/05/15 17:11:07, robertphillips wrote:
> "may Ganesh" ?

Done.

https://codereview.chromium.org/1134743003/diff/60001/src/core/SkMatrixImageF...
File src/core/SkMatrixImageFilter.cpp (right):

https://codereview.chromium.org/1134743003/diff/60001/src/core/SkMatrixImageF...
src/core/SkMatrixImageFilter.cpp:86: 
On 2015/05/15 17:11:07, robertphillips wrote:
> Do we still need this clear ?

Done.

[reed1, 2015-05-15 17:24:32]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2015-05-15 17:24:32]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1134743003/#ps80002
(title: " ")

[commit-bot: I haz the power, 2015-05-15 17:24:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134743003/80002

[robertphillips, 2015-05-15 17:25:51]

lgtm

[commit-bot: I haz the power, 2015-05-15 17:30:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-15 17:30:59]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-05-15 17:33:01]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-05-15 17:33:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1134743003/80002

[commit-bot: I haz the power, 2015-05-15 17:33:41]

Committed patchset #6 (id:80002) as
https://skia.googlesource.com/skia/+/fa33f5a6b770130acdc55f2ffe19dd545665726a

[reed1, 2015-05-15 17:34:15]

crap -- this may generate layouttests changes, so I may need to revert and add a
guard

[reed1, 2015-05-15 17:38:48]

A revert of this CL (patchset #6 id:80002) has been created in
https://codereview.chromium.org/1128823010/ by reed@google.com.

The reason for reverting is: likely affect layouttests, so need to add a guard.

[Stephen White, 2015-05-15 17:44:02]

On 2015/05/15 17:38:48, reed1 wrote:
> A revert of this CL (patchset #6 id:80002) has been created in
> https://codereview.chromium.org/1128823010/ by mailto:reed@google.com.
> 
> The reason for reverting is: likely affect layouttests, so need to add a
guard.

Just my 2c: I'd much prefer the small Ganesh patch above to adding this new API
to SkImageFilter.

[bsalomon, 2015-05-15 17:55:16]

On 2015/05/15 17:44:02, Stephen White wrote:
> On 2015/05/15 17:38:48, reed1 wrote:
> > A revert of this CL (patchset #6 id:80002) has been created in
> > https://codereview.chromium.org/1128823010/ by mailto:reed@google.com.
> > 
> > The reason for reverting is: likely affect layouttests, so need to add a
> guard.
> 
> Just my 2c: I'd much prefer the small Ganesh patch above to adding this new
API
> to SkImageFilter.

I really don't want to make SkGpuDevice promise to implement drawBitmap
correctly for all cases where the bitmap is approx-texture-backed. That patch
fixes one issue with them, but there are a host of other problems.

I think the real answer here is to make formalize the CTM contract between
canvas and IFs so the canvas can apply the residual matrix and make the right
determination about whether it is a 1-to-1 pixel matrix without having to query
the IF.

[Stephen White, 2015-05-15 18:00:22]

On 2015/05/15 17:55:16, bsalomon wrote:
> On 2015/05/15 17:44:02, Stephen White wrote:
> > On 2015/05/15 17:38:48, reed1 wrote:
> > > A revert of this CL (patchset #6 id:80002) has been created in
> > > https://codereview.chromium.org/1128823010/ by mailto:reed@google.com.
> > > 
> > > The reason for reverting is: likely affect layouttests, so need to add a
> > guard.
> > 
> > Just my 2c: I'd much prefer the small Ganesh patch above to adding this new
> API
> > to SkImageFilter.
> 
> I really don't want to make SkGpuDevice promise to implement drawBitmap
> correctly for all cases where the bitmap is approx-texture-backed. That patch
> fixes one issue with them, but there are a host of other problems.
> 
> I think the real answer here is to make formalize the CTM contract between
> canvas and IFs so the canvas can apply the residual matrix and make the right
> determination about whether it is a 1-to-1 pixel matrix without having to
query
> the IF.

Hmm, ok. It seems like the "pinch point" you referred to above (going from
scratch to
tight-bounds textures) is going to cost us performance with an extra draw, but
maybe 
I'm missing something.

The formalization I'd prefer is your suggestion of returning an SkShader from
the
last filter stage. That seems like a generalization that would take care of
residual 
matrix application, as well as any other single-pass effects that are required.
At any 
rate, let's talk about this more offline.

[bsalomon, 2015-05-15 18:02:48]

On 2015/05/15 18:00:22, Stephen White wrote:
> On 2015/05/15 17:55:16, bsalomon wrote:
> > On 2015/05/15 17:44:02, Stephen White wrote:
> > > On 2015/05/15 17:38:48, reed1 wrote:
> > > > A revert of this CL (patchset #6 id:80002) has been created in
> > > > https://codereview.chromium.org/1128823010/ by mailto:reed@google.com.
> > > > 
> > > > The reason for reverting is: likely affect layouttests, so need to add a
> > > guard.
> > > 
> > > Just my 2c: I'd much prefer the small Ganesh patch above to adding this
new
> > API
> > > to SkImageFilter.
> > 
> > I really don't want to make SkGpuDevice promise to implement drawBitmap
> > correctly for all cases where the bitmap is approx-texture-backed. That
patch
> > fixes one issue with them, but there are a host of other problems.
> > 
> > I think the real answer here is to make formalize the CTM contract between
> > canvas and IFs so the canvas can apply the residual matrix and make the
right
> > determination about whether it is a 1-to-1 pixel matrix without having to
> query
> > the IF.
> 
> Hmm, ok. It seems like the "pinch point" you referred to above (going from
> scratch to
> tight-bounds textures) is going to cost us performance with an extra draw, but
> maybe 
> I'm missing something.
> 

The idea is we'd only do the copy when the caller can't promise that the draw is
1-to-1.