Fix tile bitmap code in SkGpuDevice to compute correct local coords

Fix tile bitmap code in SkGpuDevice to compute correct local coords.

This fixes a bug when using an alpha bitmap/image with a SkShader where the SkShader's GrFragmentProcessor would receive incorrect coordinates.

BUG=skia:5643
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249163004

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

[bsalomon, 2016-08-16 19:41:03]

Description was changed from

==========
Fix tile bitmap code in SkGpuDevice to compute correct local coords

BUG=skia:
==========

to

==========
Fix tile bitmap code in SkGpuDevice to compute correct local coords

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249163004
==========

[bsalomon, 2016-08-17 03:07:12]

Description was changed from

==========
Fix tile bitmap code in SkGpuDevice to compute correct local coords

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249163004
==========

to

==========
Fix tile bitmap code in SkGpuDevice to compute correct local coords.

This fixes a bug when using an alpha bitmap/image with a SkShader where the
SkShader's GrFragmentProcessor would receive incorrect coordinates.

BUG=skia:5643

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249163004
==========

[bsalomon, 2016-08-17 03:07:12]

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

[bsalomon, 2016-08-17 03:11:57]

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

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:943: SkMatrix viewM = viewMatrix;
See note below. This modification of the view matrix isn't legal if there is a
shader.

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1030: texMatrix.setScale(wInv, hInv);
see comment at bottom of the function about why this branch is removed.

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1080: if (kAlpha_8_SkColorType == bitmap.colorType() &&
paint.getShader()) {
This trickery here is to try to avoid using a coord transform in the the
fragment processor and instead use a localRect. Different coord transforms block
batching whereas rect batches turn local coords into vertex attrs. That mattered
when this function was also called in the non-tiled case. However, now that it
is only called for tiled bitmaps, so we know we are also using different
textures. Since the texture change breaks batching it doesn't actually matter.

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1254: viewMatrix.preTranslate(dst->fLeft, dst->fTop);
Here (along with the matrix mod noted above) is the src of the problem. Once we
modify the viewMatrix the SkShader will no longer see the correct coords.

Moreover, the way this matrix is computed it is relying on drawTiledBitmap
further modifying view matrix (site of a previous comment). This means this
isn't really the right matrix to pass to shouldTileImageID. The new code
reflects the correct set of mappings between device space, local space, and
image space.

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1396: SkMatrix totalMatrix = *draw.fMatrix;
Again, this isn't really the right matrix.

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

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.h#o...
src/gpu/SkGpuDevice.h:200: void internalDrawBitmap(const SkBitmap&,
This is only being called by drawTiledBitmap. As a follow up I'd like to inline
it into that function which may yield more simplification.

[robertphillips, 2016-08-17 13:55:08]

It looks good to me but I would like to take another pass.

Is there already a test case or GM that tests this?

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

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:675: const SkMatrix& viewMatrix,
the -Matrix suffix is a bit overkill here

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

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.h#n...
src/gpu/SkGpuDevice.h:202: const SkMatrix& viewMatrix,
add param name here (since it's not obvious what it is) ?

[bsalomon, 2016-08-17 14:06:45]

The bleed_alpha_[image|bitmap]_shader GMs show the bug.

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

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:675: const SkMatrix& viewMatrix,
On 2016/08/17 13:55:08, robertphillips wrote:
> the -Matrix suffix is a bit overkill here

Done.

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

https://codereview.chromium.org/2249163004/diff/80001/src/gpu/SkGpuDevice.h#n...
src/gpu/SkGpuDevice.h:202: const SkMatrix& viewMatrix,
On 2016/08/17 13:55:08, robertphillips wrote:
> add param name here (since it's not obvious what it is) ?

Done.

[robertphillips, 2016-08-17 15:27:52]

lgtm

[robertphillips, 2016-08-17 15:28:00]

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

[commit-bot: I haz the power, 2016-08-17 15:28:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 15:35:30]

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

[commit-bot: I haz the power, 2016-08-17 15:35:31]

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

[bsalomon, 2016-08-17 15:41:00]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-08-17 15:41:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/2249163004/#ps120001
(title: "fix int->scalar warning on windows")

[commit-bot: I haz the power, 2016-08-17 15:41:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 16:02:11]

Description was changed from

==========
Fix tile bitmap code in SkGpuDevice to compute correct local coords.

This fixes a bug when using an alpha bitmap/image with a SkShader where the
SkShader's GrFragmentProcessor would receive incorrect coordinates.

BUG=skia:5643

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249163004
==========

to

==========
Fix tile bitmap code in SkGpuDevice to compute correct local coords.

This fixes a bug when using an alpha bitmap/image with a SkShader where the
SkShader's GrFragmentProcessor would receive incorrect coordinates.

BUG=skia:5643

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2249163004

Committed:
https://skia.googlesource.com/skia/+/e553b64bca57723db2b8da1c015a5ebf467eade9
==========

[commit-bot: I haz the power, 2016-08-17 16:02:12]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/e553b64bca57723db2b8da1c015a5ebf467eade9