Implement approx-match support in image filter saveLayer() offscreen.

Implement approx-match support in image filter saveLayer() offscreen.

Currently, the GPU-side image filter implementation creates
exact-match textures for the offscreen backing stores for
saveLayer().  This is because several filters have GPU
implementations which depend on the texture coordinates
being 0..1.

The fix is three-fold:

1) Store the actual requested size in the SkGpuDevice, so
that when wrapping it in an SkBitmap for passing to 
filterImage(), we can give it the original size.
2) Fix the filters (SkMagnifierImageFilter,
SkLightingImageFilter, SkMatrixConvolutionImageFilter,
SkMatrixImageFilter) whose GPU implementation depends on 
0..1 texture coordinates.
3) Remove the exception for GPU-side image filters in
SkCanvas::internalSaveLayer().

For the lighting filters, there were two bugs which were 
cancelling each other out: the sobel filter matrix was
being computed upside down, but then we'd negate the 
resulting normal. This worked fine in the exact-match case,
but in the approx-match case we'd sample garbage along
the edge pixels. Also, we never implemented the edge pixels
according to spec in the GPU case. It requires a
different fragment shader for each edge of the nine-patch,
which meant we couldn't use asFragmentProcessor(), and had 
to implement the drawing via a filterImageGPU() override. 
In order to avoid polluting the public API, I inserted a 
new base class, SkLightingImageFilterInternal above 
Sk[Diffuse|Specular]LightingImageFilter to handle the 
implementation.

For the SkMatrixConvolutionImageFilter, it seems the
GLSL clamp() function occasionally returns values outside 
the clamped range, resulting in access of garbage 
texels even in GL_NEAREST. The fix here is to clamp to a
rect inset by half a texel. There was also a bug in 
the unpremultiply step when fConvolveAlpha is false.

For SkMatrixImageFilter, the fix was to make the generic
draw path be more careful about when to use texture domain.
If the bitmap already has a texture, use texture domain
if the srcRect is smaller than the entire texture (not
the entire bitmap).

N.B.: this change will cause some minor pixel diffs in the 
GPU results of the following GMs (and possibly more):
matriximagefilter, matrixconvolution, imagefiltersscaled,
lighting, imagemagnifier, filterfastbounds,
complexclip_aa_Layer_invert, complexclip_aa_layer,
complexclip_bw_layer_invert, complexclip_bw_layer.

BUG=skia:3532
Committed: https://skia.googlesource.com/skia/+/b97dafefe63ea0a1bbce8e8b209f4920983fb8b9

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

Committed: https://skia.googlesource.com/skia/+/46112cf2a7c7307f1c9eebb5f881cbda15aa460c

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

[Stephen White, 2015-03-25 21:37:57]

senorblanco@chromium.org changed reviewers:
+ bsalomon@chromium.org

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

senorblanco@chromium.org changed reviewers:
+ bsalomon@google.com
- bsalomon@chromium.org

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

Brian: PTAL. Thanks!

Note that I have another version of this patch which removes the old flavour of
SkGpuDevice::Create() as well (the one that doesn't take width/height). It just
changes some tests and a few more call sites. Let me know if you'd prefer that.

[bsalomon, 2015-03-26 19:04:00]

great to see this! lgtm

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:127: SkGpuDevice* SkGpuDevice::Create(GrRenderTarget*
rt, int width, int height, const SkSurfaceProps* props, unsigned flags) {
nit, line wrap

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1645: if (this->filterTexture(fContext, devTex,
device->width(), device->height(), filter, ctx, &filteredBitmap,
nit: line wrap

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1699: return this->filterTexture(fContext, texture,
src.width(), src.height(), filter, ctx, result, offset);
nit: line wrap

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1909: return
SkGpuDevice::Create(texture->asRenderTarget(), cinfo.fInfo.width(),
cinfo.fInfo.height(), &props, flags);
nit line wrap

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.h
File src/gpu/SkGpuDevice.h (right):

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.h#n...
src/gpu/SkGpuDevice.h:47: 
nit, line wrap

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

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:127: SkGpuDevice* SkGpuDevice::Create(GrRenderTarget*
rt, int width, int height, const SkSurfaceProps* props, unsigned flags) {
On 2015/03/26 19:03:59, bsalomon wrote:
> nit, line wrap

Done.

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1645: if (this->filterTexture(fContext, devTex,
device->width(), device->height(), filter, ctx, &filteredBitmap,
On 2015/03/26 19:03:59, bsalomon wrote:
> nit: line wrap

Done.

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1699: return this->filterTexture(fContext, texture,
src.width(), src.height(), filter, ctx, result, offset);
On 2015/03/26 19:03:59, bsalomon wrote:
> nit: line wrap

Done.

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1909: return
SkGpuDevice::Create(texture->asRenderTarget(), cinfo.fInfo.width(),
cinfo.fInfo.height(), &props, flags);
On 2015/03/26 19:03:59, bsalomon wrote:
> nit line wrap

Done.

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.h
File src/gpu/SkGpuDevice.h (right):

https://codereview.chromium.org/1034733002/diff/80001/src/gpu/SkGpuDevice.h#n...
src/gpu/SkGpuDevice.h:47: 
On 2015/03/26 19:04:00, bsalomon wrote:
> nit, line wrap

Done.

[Stephen White, 2015-03-26 19:42:30]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-03-26 19:42:31]

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/1034733002/#ps140001
(title: "Fix 100-col issues")

[commit-bot: I haz the power, 2015-03-26 19:42:43]

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

[commit-bot: I haz the power, 2015-03-26 19:51:23]

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

[commit-bot: I haz the power, 2015-03-26 19:51:25]

Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[Stephen White, 2015-03-28 20:37:49]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-03-28 20:37:49]

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/1034733002/#ps220001
(title: "Fix comment")

[commit-bot: I haz the power, 2015-03-28 20:37:52]

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

[commit-bot: I haz the power, 2015-03-28 20:43:19]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/b97dafefe63ea0a1bbce8e8b209f4920983fb8b9

[reed2, 2015-03-28 21:55:26]

reed@chromium.org changed reviewers:
+ reed@chromium.org

[reed2, 2015-03-28 21:55:27]

https://codereview.chromium.org/1034733002/diff/220001/src/effects/SkMagnifie...
File src/effects/SkMagnifierImageFilter.cpp (right):

https://codereview.chromium.org/1034733002/diff/220001/src/effects/SkMagnifie...
src/effects/SkMagnifierImageFilter.cpp:28: const SkRect& bounds,
If we try to re-land this, lets add some dox for this (and perhaps the other)
parameters.

[Stephen White, 2015-03-30 15:22:54]

On 2015/03/28 21:55:27, reed2 wrote:
>
https://codereview.chromium.org/1034733002/diff/220001/src/effects/SkMagnifie...
> File src/effects/SkMagnifierImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/1034733002/diff/220001/src/effects/SkMagnifie...
> src/effects/SkMagnifierImageFilter.cpp:28: const SkRect& bounds,
> If we try to re-land this, lets add some dox for this (and perhaps the other)
> parameters.

Done.

[Stephen White, 2015-03-30 22:12:35]

New patch up: implemented fixes for SkLightingImageFilter.

Dirty little secret about the GPU path: it never correctly implemented the
boundary conditions for the spec, while the CPU path did.

Investigation revealed two bugs which cancelled each other out in most cases:
the matrix of
sobel coefficients was being built upside down, but then the normals were
flipped in Y after 
computation. This works fine in the interior, but causes us to sample outside
the texture 
bounds on the edge.

Fixing this makes the GPU version of pointToNormal() look more like the CPU
version.

[bsalomon, 2015-03-31 14:50:24]

lgtm

https://codereview.chromium.org/1034733002/diff/410001/src/effects/SkLighting...
File src/effects/SkLightingImageFilter.cpp (right):

https://codereview.chromium.org/1034733002/diff/410001/src/effects/SkLighting...
src/effects/SkLightingImageFilter.cpp:387: drawRect(context, srcTexture, dst,
matrix, clip, topLeft, kTopLeft_BoundaryMode, bounds);
this->

https://codereview.chromium.org/1034733002/diff/410001/src/effects/SkLighting...
src/effects/SkLightingImageFilter.cpp:445: virtual GrFragmentProcessor*
getFragmentProcessor(GrTexture*, const SkMatrix&,
could get rid of the "virtual" here and elsewhere

[Stephen White, 2015-03-31 15:00:14]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-03-31 15:00:14]

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/1034733002/#ps450001
(title: "Fix this->, resulting 100-col issues, extraneous "virtual"")

[commit-bot: I haz the power, 2015-03-31 15:00:30]

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

[Stephen White, 2015-03-31 15:16:55]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-03-31 15:16:55]

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/1034733002/#ps510001
(title: "Offset the bounds by srcOffset (as the CPU path does).")

[commit-bot: I haz the power, 2015-03-31 15:17:10]

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

[commit-bot: I haz the power, 2015-03-31 15:35:20]

Committed patchset #27 (id:510001) as
https://skia.googlesource.com/skia/+/f5f8518fe0bbd2703e4ffc1b11ad7b4312ff7641

[rmistry, 2015-04-01 11:19:23]

A revert of this CL (patchset #27 id:510001) has been created in
https://codereview.chromium.org/1057443003/ by rmistry@google.com.

The reason for reverting is: Looks like this change is causing layout test
failures which is blocking Skia's DEPS roll into Chromium:
https://codereview.chromium.org/1050563002/
https://codereview.chromium.org/1043133005/
https://codereview.chromium.org/1048273002/

Reverting to see if this fixes the DEPS roll..

[Stephen White, 2015-04-01 19:41:05]

I've added fixes for SkMatrixConvolutionImageFilter.

PTAL. Thanks!

[bsalomon, 2015-04-01 21:04:29]

lgtm

[Stephen White, 2015-04-01 21:11:38]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2015-04-01 21:12:00]

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

[commit-bot: I haz the power, 2015-04-01 21:17:18]

Committed patchset #31 (id:590001) as
https://skia.googlesource.com/skia/+/46112cf2a7c7307f1c9eebb5f881cbda15aa460c

[rmistry, 2015-04-01 22:00:27]

rmistry@google.com changed reviewers:
+ rmistry@google.com

[rmistry, 2015-04-01 22:00:29]

Looks like this is causing failed assertions in some debug builds:

https://build.chromium.org/p/client.skia.android/builders/Test-Android-GCC-Ne...
https://build.chromium.org/p/client.skia/builders/Test-Mac10.9-Clang-MacMini6...
https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GP...
https://build.chromium.org/p/client.skia/builders/Test-Win7-MSVC-ShuttleA-GPU...

Could you please take a look.

[Stephen White, 2015-04-01 22:04:30]

On 2015/04/01 22:00:29, rmistry wrote:
> Looks like this is causing failed assertions in some debug builds:
> 
>
https://build.chromium.org/p/client.skia.android/builders/Test-Android-GCC-Ne...
>
https://build.chromium.org/p/client.skia/builders/Test-Mac10.9-Clang-MacMini6...
>
https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GP...
>
https://build.chromium.org/p/client.skia/builders/Test-Win7-MSVC-ShuttleA-GPU...
> 
> Could you please take a look.

I'm on it: https://codereview.chromium.org/1053873002/

[rmistry, 2015-04-01 22:52:49]

A revert of this CL (patchset #31 id:590001) has been created in
https://codereview.chromium.org/1057693002/ by rmistry@google.com.

The reason for reverting is: Spoke to Stephen about this. Reverting because
failing debug builds:

https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Mac10.9-Clan...
https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Ubuntu-GCC-S....

[Stephen White, 2015-04-02 10:52:27]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-04-02 10:52:28]

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/1034733002/#ps610001
(title: "Fix empty/out-of-range rects in GrTextureDomain Clamp mode")

[commit-bot: I haz the power, 2015-04-02 10:52:42]

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

[rmistry, 2015-04-02 10:54:50]

The CQ bit was unchecked by rmistry@google.com

[rmistry, 2015-04-02 10:56:50]

On 2015/04/02 10:52:42, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1034733002/610001

Unchecked CQ box because wanted to put a note here.

When this CL landed yesterday afternoon, there was a DEPs roll that was kicked
off with it:
https://codereview.chromium.org/1053703003/

The DEPs roll had some layout test failures and thus could not land:
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...
Maybe further suppresions are required?

The DEPs roll with the 2 reverts landed successfully.

[Stephen White, 2015-04-02 11:25:57]

On 2015/04/02 10:56:50, rmistry wrote:
> On 2015/04/02 10:52:42, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1034733002/610001
> 
> Unchecked CQ box because wanted to put a note here.
> 
> When this CL landed yesterday afternoon, there was a DEPs roll that was kicked
> off with it:
> https://codereview.chromium.org/1053703003/
> 
> The DEPs roll had some layout test failures and thus could not land:
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...
> Maybe further suppresions are required?
> 
> The DEPs roll with the 2 reverts landed successfully.

Thanks for catching that, Ravi! The layout test results are an improvement
(matrix convolution looks more like sosftware path now). I'm landing more
suppressions in Chrome.

[Stephen White, 2015-04-02 11:54:32]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2015-04-02 11:54:45]

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

[commit-bot: I haz the power, 2015-04-02 11:55:02]

Committed patchset #32 (id:610001) as
https://skia.googlesource.com/skia/+/d0d37cace08f12abf8d316e6949e947551d418e6

[rmistry, 2015-04-02 12:49:58]

On 2015/04/02 11:55:02, I haz the power (commit-bot) wrote:
> Committed patchset #32 (id:610001) as
> https://skia.googlesource.com/skia/+/d0d37cace08f12abf8d316e6949e947551d418e6

Looks like this caused a failed assertion in the
Test-Win8-MSVC-ShuttleA-GPU-GTX660-x86_64-Debug build:
https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Win8-MSVC-Sh...

Snippet:
( 347MB     7) 56.3ms	unit test
GpuColorFilterc:\0\build\slave\test-win8-msvc-shuttlea-gpu-gtx660-x86_64-debug\build\skia\src\gpu\grinvariantoutput.cpp:16:
failed assertion "this->colorComponentsAllEqual()"

[Stephen White, 2015-04-02 15:30:45]

On 2015/04/02 12:49:58, rmistry wrote:
> On 2015/04/02 11:55:02, I haz the power (commit-bot) wrote:
> > Committed patchset #32 (id:610001) as
> >
https://skia.googlesource.com/skia/+/d0d37cace08f12abf8d316e6949e947551d418e6
> 
> Looks like this caused a failed assertion in the
> Test-Win8-MSVC-ShuttleA-GPU-GTX660-x86_64-Debug build:
>
https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Win8-MSVC-Sh...
> 
> Snippet:
> ( 347MB     7) 56.3ms	unit test
>
GpuColorFilterc:\0\build\slave\test-win8-msvc-shuttlea-gpu-gtx660-x86_64-debug\build\skia\src\gpu\grinvariantoutput.cpp:16:
> failed assertion "this->colorComponentsAllEqual()"

Note that that test seems to be failing on other builders, even without my
change:
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...

(occurred between the revert and my re-land).

Something in my change is tickling it, but I don't think it's the cause.

[rmistry, 2015-04-02 17:49:01]

On 2015/04/02 15:30:45, Stephen White wrote:
> On 2015/04/02 12:49:58, rmistry wrote:
> > On 2015/04/02 11:55:02, I haz the power (commit-bot) wrote:
> > > Committed patchset #32 (id:610001) as
> > >
> https://skia.googlesource.com/skia/+/d0d37cace08f12abf8d316e6949e947551d418e6
> > 
> > Looks like this caused a failed assertion in the
> > Test-Win8-MSVC-ShuttleA-GPU-GTX660-x86_64-Debug build:
> >
>
https://uberchromegw.corp.google.com/i/client.skia/builders/Test-Win8-MSVC-Sh...
> > 
> > Snippet:
> > ( 347MB     7) 56.3ms	unit test
> >
>
GpuColorFilterc:\0\build\slave\test-win8-msvc-shuttlea-gpu-gtx660-x86_64-debug\build\skia\src\gpu\grinvariantoutput.cpp:16:
> > failed assertion "this->colorComponentsAllEqual()"
> 
> Note that that test seems to be failing on other builders, even without my
> change:
>
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...
> 
> (occurred between the revert and my re-land).
> 
> Something in my change is tickling it, but I don't think it's the cause.

Looks like Brian's revert ( https://codereview.chromium.org/1032593003 ) made
this go away.