Disabled YUV decoding for subsets

Disabled YUV decoding for subsets

Image subsets were always showing the same tile and were causing an image to be fully YUV decoded once per tile, which was both slow and wrong.

BUG=413001, 419718
Committed: https://skia.googlesource.com/skia/+/ff58e4679b9473ce19abb81a3e50fc6a18f0981d

[sugoi1, 2014-10-15 15:05:58]

sugoi@chromium.org changed reviewers:
+ bsalomon@google.com, reed@google.com

[bsalomon, 2014-10-15 15:34:42]

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp#newcode232
src/gpu/SkGr.cpp:232: if ((NULL == pixelRef) ||
!pixelRef->getYUV8Planes(yuvSizes, NULL, NULL, NULL) ||
Should the failure happen in getYUV8Planes rather than here? It seems like it is
kind of broken as is. It will "succeed" but give the wrong result for subset
bitmaps.

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp#newcode234
src/gpu/SkGr.cpp:234: (yuvSizes[1].fWidth > maxTextureSize) ||
(yuvSizes[1].fHeight > maxTextureSize) ||
Are these checks needed?

[sugoi1, 2014-10-15 18:13:28]

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp#newcode232
src/gpu/SkGr.cpp:232: if ((NULL == pixelRef) ||
!pixelRef->getYUV8Planes(yuvSizes, NULL, NULL, NULL) ||
On 2014/10/15 15:34:42, bsalomon wrote:
> Should the failure happen in getYUV8Planes rather than here? It seems like it
is
> kind of broken as is. It will "succeed" but give the wrong result for subset
> bitmaps.

getYUV8Planes() doesn't know anything about the bitmap. It is perfectly capable
of decoding that pixelRef properly and yielding the YUV planes, no matter the
size (that part doesn't use the GPU). It doesn't know that we are planning on
only using a subset of the YUV planes from the pixelRef itself. I don't think
there's any difference between the pixelRef of a complete bitmap or of a bitmap
subset (in fact, I think they can share the same pixelRef).

In fact, the jpeg images causing the issue I'm trying to fix could be decoded
properly using YUV decoding, if it wasn't for the fact that shouldTileBitmap()
returns true when the size is large as compared to the GrResourceCache size.

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp#newcode234
src/gpu/SkGr.cpp:234: (yuvSizes[1].fWidth > maxTextureSize) ||
(yuvSizes[1].fHeight > maxTextureSize) ||
On 2014/10/15 15:34:42, bsalomon wrote:
> Are these checks needed?

Well, it's not impossible that these would be required, but it's unlikely. Let
me explain: yuvSizes are the sizes that we need to allocate and these sizes are
expressed in (number of blocks) * (size of a block), so the size of a JPEG image
is rounded up to the nearest block to produce the yuvSizes. So even if a
bitmap's dimensions fit into maxTextureSize at the moment where we call
shouldTileBitmap(), yuvSizes' dimensions may exceed that number and no longer
fit into maxTextureSize.

That's all theoretical, though, as in practice, block sizes are usually 8, 16 or
32, and as long as maxTextureSize is an exact multiple of 8, 16 or 32 (which is
most likely always the case), than these checks can probably be safely removed.

[bsalomon, 2014-10-15 18:31:54]

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp#newcode232
src/gpu/SkGr.cpp:232: if ((NULL == pixelRef) ||
!pixelRef->getYUV8Planes(yuvSizes, NULL, NULL, NULL) ||
On 2014/10/15 18:13:28, sugoi1 wrote:
> On 2014/10/15 15:34:42, bsalomon wrote:
> > Should the failure happen in getYUV8Planes rather than here? It seems like
it
> is
> > kind of broken as is. It will "succeed" but give the wrong result for subset
> > bitmaps.
> 
> getYUV8Planes() doesn't know anything about the bitmap. It is perfectly
capable
> of decoding that pixelRef properly and yielding the YUV planes, no matter the
> size (that part doesn't use the GPU). It doesn't know that we are planning on
> only using a subset of the YUV planes from the pixelRef itself. I don't think
> there's any difference between the pixelRef of a complete bitmap or of a
bitmap
> subset (in fact, I think they can share the same pixelRef).
> 
> In fact, the jpeg images causing the issue I'm trying to fix could be decoded
> properly using YUV decoding, if it wasn't for the fact that shouldTileBitmap()
> returns true when the size is large as compared to the GrResourceCache size.

Acknowledged.

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp#newcode234
src/gpu/SkGr.cpp:234: (yuvSizes[1].fWidth > maxTextureSize) ||
(yuvSizes[1].fHeight > maxTextureSize) ||
On 2014/10/15 18:13:28, sugoi1 wrote:
> On 2014/10/15 15:34:42, bsalomon wrote:
> > Are these checks needed?
> 
> Well, it's not impossible that these would be required, but it's unlikely. Let
> me explain: yuvSizes are the sizes that we need to allocate and these sizes
are
> expressed in (number of blocks) * (size of a block), so the size of a JPEG
image
> is rounded up to the nearest block to produce the yuvSizes. So even if a
> bitmap's dimensions fit into maxTextureSize at the moment where we call
> shouldTileBitmap(), yuvSizes' dimensions may exceed that number and no longer
> fit into maxTextureSize.
> 
> That's all theoretical, though, as in practice, block sizes are usually 8, 16
or
> 32, and as long as maxTextureSize is an exact multiple of 8, 16 or 32 (which
is
> most likely always the case), than these checks can probably be safely
removed.

In any case, wouldn't we just fail down below in refScratchTexture() and fail
gracefully?

[sugoi1, 2014-10-15 19:08:56]

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/661483002/diff/1/src/gpu/SkGr.cpp#newcode234
src/gpu/SkGr.cpp:234: (yuvSizes[1].fWidth > maxTextureSize) ||
(yuvSizes[1].fHeight > maxTextureSize) ||
On 2014/10/15 18:31:54, bsalomon wrote:
> On 2014/10/15 18:13:28, sugoi1 wrote:
> > On 2014/10/15 15:34:42, bsalomon wrote:
> > > Are these checks needed?
> > 
> > Well, it's not impossible that these would be required, but it's unlikely.
Let
> > me explain: yuvSizes are the sizes that we need to allocate and these sizes
> are
> > expressed in (number of blocks) * (size of a block), so the size of a JPEG
> image
> > is rounded up to the nearest block to produce the yuvSizes. So even if a
> > bitmap's dimensions fit into maxTextureSize at the moment where we call
> > shouldTileBitmap(), yuvSizes' dimensions may exceed that number and no
longer
> > fit into maxTextureSize.
> > 
> > That's all theoretical, though, as in practice, block sizes are usually 8,
16
> or
> > 32, and as long as maxTextureSize is an exact multiple of 8, 16 or 32 (which
> is
> > most likely always the case), than these checks can probably be safely
> removed.
> 
> In any case, wouldn't we just fail down below in refScratchTexture() and fail
> gracefully?

Ah, well, if that's the case I'll just remove these. Thanks.

[bsalomon, 2014-10-16 01:41:31]

lgtm

[sugoi1, 2014-10-16 12:13:35]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2014-10-16 12:13:47]

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

[commit-bot: I haz the power, 2014-10-16 12:19:36]

Committed patchset #2 (id:20001) as ff58e4679b9473ce19abb81a3e50fc6a18f0981d