Convert SkGpuDevice::drawTextureAdjuster to SkGpuDevice::drawTextureProducer

Convert SkGpuDevice::drawTextureAdjuster to SkGpuDevice::drawTextureProducer

Move createFragmentProcessor to GrTextureProducer base class.

Make non-tiled sw-bitmap draws go through drawTextureProducer.

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

[bsalomon, 2015-11-17 23:15:56]

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

[bsalomon, 2015-11-17 23:15:57]

The next steps would be:

1) rewrite tiling code to use GrTextureMaker as a layer on top of
drawTextureProducer

2) figure out how to make a GrTextureMaker exposed for the cacherator-backed
SkImages so that drawImage never devolves to drawBitmap but goes straight to
drawTextureProducer or drawTiledTextureMaker.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
File src/gpu/GrImageIDTextureAdjuster.h (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
src/gpu/GrImageIDTextureAdjuster.h:49: class GrBitmapTextureMaker : public
GrTextureMaker {
Moved this from SkGr.cpp to here and the corresponding cpp, renamed to be
Gr-prefixed, and updated to take GrContext in its constructor rather than in
overrides to reflect changes to base class.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.cpp (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:240: if (restrictFilterToRect ||
textureContentArea) {
this check should have been here all along.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.h (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.h:37: enum FilterConstraint {
this block moved up from subclass and made virtual

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

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:861: if (!skipTileCheck) {
this stuff is lifted from the now-deleted drawBitmapCommon to handle detecting
tiling and calling drawTiledBitmap if tiling. Eventually I plan to make a
drawTiledTextureProducer and combine this with the similar code added to
drawBitmapRect below.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1008: // de-optimized this determination
decided this wasn't worth keeping since we're already in the case of tiling here
and plan to rewrite all this on top of GrTextureMaker anyway.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1282: bool drawAA =
!fRenderTarget->isUnifiedMultisampled() &&
Didn't share all this with drawBitmap() since I plan to write a common version
on top of GrTextureMaker and it this version has added complexities because of
non identity mapping between src and dst rects.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice_dra...
File src/gpu/SkGpuDevice_drawTexture.cpp (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice_dra...
src/gpu/SkGpuDevice_drawTexture.cpp:60: static bool has_aligned_samples(const
SkRect& srcRect, const SkRect& transformedRect) {
these functions are lifted and simplified from SkGpuDevice.cpp.

Modifications are noted inline.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice_dra...
src/gpu/SkGpuDevice_drawTexture.cpp:63:
SkScalarAbs(SkScalarFraction(transformedRect.top())) < kColorBleedTolerance &&
Changed this to use abs(fraction(x)) instead of abs(round(x) - x), but that is
incorrect. Will change back before landing.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice_dra...
src/gpu/SkGpuDevice_drawTexture.cpp:71: static bool may_color_bleed(const
SkRect& srcRect,
this function is unchanged.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice_dra...
src/gpu/SkGpuDevice_drawTexture.cpp:100: static bool
can_ignore_bilerp_constraint(const GrTextureProducer& producer,
was needs_texture_domain() in the original. That function really assumed bilerp
for its optimizations. It also looked to see if the srcrect contains the whole
texture bounds but GrTextureProducer already checks that.

[robertphillips, 2015-11-18 13:58:36]

lgtm + nits & questions

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
File src/gpu/GrImageIDTextureAdjuster.cpp (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
src/gpu/GrImageIDTextureAdjuster.cpp:62: , fBitmap(bitmap)
prev line for the '{' ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
File src/gpu/GrImageIDTextureAdjuster.h (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
src/gpu/GrImageIDTextureAdjuster.h:48: /** Implementation for sw-backed
SkBitmaps. */
Soup up this comment since it is now separated from its base class ?

// This class manages the conversion of SW-backed bitmaps to GrTextures.
// If the input bitmap is non-volatile the texture is cached using a key created
from the bitmap's unique id and the subset of the texture specified by the
bitmap.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.cpp (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:240: if (restrictFilterToRect ||
textureContentArea) {
use -> us ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:327: return kDomain_DomainMode;
rm ';' ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:329: 
static ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:427: if (copyKey.isValid()) {
Why not fContext ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:440: 
// The filter mode used to determine the domain differs from the filter mode
used in the texture params because ...

?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.h (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.h:42: /**
Aren't these all supposed to be one pixel right ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.h:48: *                                        
be in texel units (rather than normalized) with
r' -> r's ?

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

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:851: 
pain -> paint ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:852: // The tile code path doesn't currently support AA,
so if the pain asked for aa and we could
untiles -> untiled ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:862: SkRect srcRect = SkRect::MakeIWH(bitmap.width(),
bitmap.height());
rm this copy of 'maxTileSize' ?
rename it 'maxTileSizeWithPadding' ?
If we rename it can we move its declaration down to where we compute it ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:891: }
Can we really use kFast here rather than kStrict ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1254: const SkRect* dst = &origDst;
const SkRect bmpBounds ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1265: SkRect tmpSrc, tmpDst;
fix spaces ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1279: 
pain -> paint; untiles -> untiled ?

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1289: if (!skipTileCheck) {
delete this copy of 'maxTileSize' ?
rename it 'maxTileSizeWithPadding' ?
If we rename it can we move its declaration down to where we compute it ?

[bsalomon, 2015-11-18 18:05:38]

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
File src/gpu/GrImageIDTextureAdjuster.cpp (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
src/gpu/GrImageIDTextureAdjuster.cpp:62: , fBitmap(bitmap)
On 2015/11/18 13:58:35, robertphillips wrote:
> prev line for the '{' ?

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
File src/gpu/GrImageIDTextureAdjuster.h (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrImageIDTextur...
src/gpu/GrImageIDTextureAdjuster.h:48: /** Implementation for sw-backed
SkBitmaps. */
On 2015/11/18 at 13:58:35, robertphillips wrote:
> Soup up this comment since it is now separated from its base class ?
> 
> // This class manages the conversion of SW-backed bitmaps to GrTextures.
> // If the input bitmap is non-volatile the texture is cached using a key
created from the bitmap's unique id and the subset of the texture specified by
the bitmap.

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.cpp (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:240: if (restrictFilterToRect ||
textureContentArea) {
On 2015/11/18 at 13:58:35, robertphillips wrote:
> use -> us ?

Done

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:327: return kDomain_DomainMode;
On 2015/11/18 13:58:35, robertphillips wrote:
> rm ';' ?

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:329: 
On 2015/11/18 13:58:35, robertphillips wrote:
> static ?

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:427: if (copyKey.isValid()) {
On 2015/11/18 13:58:35, robertphillips wrote:
> Why not fContext ?

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.cpp:440: 
On 2015/11/18 13:58:35, robertphillips wrote:
> // The filter mode used to determine the domain differs from the filter mode
> used in the texture params because ...
> 
> ?

The explanation is two lines below

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
File src/gpu/GrTextureParamsAdjuster.h (right):

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.h:42: /**
On 2015/11/18 at 13:58:35, robertphillips wrote:
> Aren't these all supposed to be one pixel right ?

Done

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/GrTextureParams...
src/gpu/GrTextureParamsAdjuster.h:48: *                                        
be in texel units (rather than normalized) with
On 2015/11/18 at 13:58:35, robertphillips wrote:
> r' -> r's ?

Done

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

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:851: 
On 2015/11/18 13:58:35, robertphillips wrote:
> pain -> paint ?

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:852: // The tile code path doesn't currently support AA,
so if the pain asked for aa and we could
On 2015/11/18 13:58:35, robertphillips wrote:
> untiles -> untiled ?

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:862: SkRect srcRect = SkRect::MakeIWH(bitmap.width(),
bitmap.height());
On 2015/11/18 13:58:35, robertphillips wrote:
> rm this copy of 'maxTileSize' ?
> rename it 'maxTileSizeWithPadding' ?
> If we rename it can we move its declaration down to where we compute it ?

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:891: }
On 2015/11/18 13:58:35, robertphillips wrote:
> Can we really use kFast here rather than kStrict ?

I guess not for the extract subset case. fixed

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1254: const SkRect* dst = &origDst;
On 2015/11/18 13:58:35, robertphillips wrote:
> const SkRect bmpBounds ?

sure

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1265: SkRect tmpSrc, tmpDst;
On 2015/11/18 13:58:35, robertphillips wrote:
> fix spaces ?

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1279: 
On 2015/11/18 13:58:35, robertphillips wrote:
> pain -> paint; untiles -> untiled ?

Done.

https://codereview.chromium.org/1459433002/diff/40001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1289: if (!skipTileCheck) {
On 2015/11/18 13:58:35, robertphillips wrote:
> delete this copy of 'maxTileSize' ?
> rename it 'maxTileSizeWithPadding' ?
> If we rename it can we move its declaration down to where we compute it ?

Done.

[bsalomon, 2015-11-18 18:05:48]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-11-18 18:05:49]

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/1459433002/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2015-11-18 18:06:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459433002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459433002/60001

[commit-bot: I haz the power, 2015-11-18 18:07:04]

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

[commit-bot: I haz the power, 2015-11-18 18:07:05]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[bsalomon, 2015-11-18 18:18:57]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-11-18 18:18:57]

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/1459433002/#ps100001
(title: "fix comment")

[commit-bot: I haz the power, 2015-11-18 18:19:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459433002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459433002/100001

[commit-bot: I haz the power, 2015-11-18 18:20:16]

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

[commit-bot: I haz the power, 2015-11-18 18:20:17]

Try jobs failed on following builders:
  Build-Mac10.8-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac10.8-Clang-...)

[bsalomon, 2015-11-18 18:43:59]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-11-18 18:43:59]

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/1459433002/#ps120001
(title: "remove redudant virtual decl")

[commit-bot: I haz the power, 2015-11-18 18:44:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1459433002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1459433002/120001

[commit-bot: I haz the power, 2015-11-18 18:56:11]

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