YUV planes cache

YUV planes cache

- Added new classes to contain YUV planes of memory, along with the associated data.
- Used these classes in load_yuv_texture() to enable YUV planes caching
- Added a unit test for the new cache

BUG=450021
Committed: https://skia.googlesource.com/skia/+/692135f9689d4dcb5ba91ff8f4899e268c0bfe11

[sugoi1, 2015-01-16 19:04:09]

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

[sugoi1, 2015-01-16 19:04:10]

I took at SkMaskCache as an example and came up with this. Hopefully, this is
close to what you had in mind.

[reed1, 2015-01-18 19:02:18]

Seems like a great start.

https://codereview.chromium.org/851273003/diff/1/src/core/SkYUVPlanesCache.h
File src/core/SkYUVPlanesCache.h (right):

https://codereview.chromium.org/851273003/diff/1/src/core/SkYUVPlanesCache.h#...
src/core/SkYUVPlanesCache.h:18: size_t  fSizeInMemory[3];
I'm guessing size-in-memory is so the client can find each of the 3 pointers
given a base-ptr (cacheddata)...? If so, perhaps we should add dox to describe
that (also I presume we are assuming that the planes of data are contiguous and
in the same order as YUV?)

https://codereview.chromium.org/851273003/diff/1/src/core/SkYUVPlanesCache.h#...
src/core/SkYUVPlanesCache.h:32: static void Add(uint32_t genID, SkCachedData*
data, Info* info);
To really make these testable, we'll need something like the optional Cache
parameter the other Caches have (otherwise we can get spurious failures when we
run the tests in multi-threaded mode.

[sugoi1, 2015-01-19 13:28:01]

https://codereview.chromium.org/851273003/diff/1/src/core/SkYUVPlanesCache.h
File src/core/SkYUVPlanesCache.h (right):

https://codereview.chromium.org/851273003/diff/1/src/core/SkYUVPlanesCache.h#...
src/core/SkYUVPlanesCache.h:18: size_t  fSizeInMemory[3];
On 2015/01/18 19:02:18, reed1 wrote:
> I'm guessing size-in-memory is so the client can find each of the 3 pointers
> given a base-ptr (cacheddata)...? If so, perhaps we should add dox to describe
> that (also I presume we are assuming that the planes of data are contiguous
and
> in the same order as YUV?)

Done.

https://codereview.chromium.org/851273003/diff/1/src/core/SkYUVPlanesCache.h#...
src/core/SkYUVPlanesCache.h:32: static void Add(uint32_t genID, SkCachedData*
data, Info* info);
On 2015/01/18 19:02:18, reed1 wrote:
> To really make these testable, we'll need something like the optional Cache
> parameter the other Caches have (otherwise we can get spurious failures when
we
> run the tests in multi-threaded mode.

Done.

[Stephen White, 2015-01-19 15:14:36]

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

[Stephen White, 2015-01-19 15:14:37]

https://codereview.chromium.org/851273003/diff/20001/src/core/SkYUVPlanesCach...
File src/core/SkYUVPlanesCache.cpp (right):

https://codereview.chromium.org/851273003/diff/20001/src/core/SkYUVPlanesCach...
src/core/SkYUVPlanesCache.cpp:52: YUVValue* result = (YUVValue*)contextData;
OCD nit: previous line has static_cast, this one is c-style cast.

https://codereview.chromium.org/851273003/diff/20001/src/core/SkYUVPlanesCache.h
File src/core/SkYUVPlanesCache.h (right):

https://codereview.chromium.org/851273003/diff/20001/src/core/SkYUVPlanesCach...
src/core/SkYUVPlanesCache.h:22: * fSize: Width and height of each of the 3
planes (in pixels).
Nit: I think Skia uses the doxygen @param notation for these type of comments,
although it doesn't seem to be consistently applied.

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

https://codereview.chromium.org/851273003/diff/20001/src/gpu/SkGr.cpp#newcode252
src/gpu/SkGr.cpp:252: yuvInfo.fSizeInMemory[i] = yuvInfo.fRowBytes[i] *
yuvInfo.fSize[i].fHeight;
Not new to this patch, but it might be nice to document that these sizes are
rounded up to JPEG block size, and may be larger than the actual width / height
of the image (maybe just expand the "for memory allocation" comment above).

[sugoi1, 2015-01-19 15:36:25]

https://codereview.chromium.org/851273003/diff/20001/src/core/SkYUVPlanesCach...
File src/core/SkYUVPlanesCache.cpp (right):

https://codereview.chromium.org/851273003/diff/20001/src/core/SkYUVPlanesCach...
src/core/SkYUVPlanesCache.cpp:52: YUVValue* result = (YUVValue*)contextData;
On 2015/01/19 15:14:36, Stephen White wrote:
> OCD nit: previous line has static_cast, this one is c-style cast.

Done.

https://codereview.chromium.org/851273003/diff/20001/src/core/SkYUVPlanesCache.h
File src/core/SkYUVPlanesCache.h (right):

https://codereview.chromium.org/851273003/diff/20001/src/core/SkYUVPlanesCach...
src/core/SkYUVPlanesCache.h:22: * fSize: Width and height of each of the 3
planes (in pixels).
On 2015/01/19 15:14:36, Stephen White wrote:
> Nit: I think Skia uses the doxygen @param notation for these type of comments,
although it doesn't seem to be consistently applied.

As discussed, those are struct members, not function params, so unless skia has
a notation for struct members, I'll leave this as is.

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

https://codereview.chromium.org/851273003/diff/20001/src/gpu/SkGr.cpp#newcode252
src/gpu/SkGr.cpp:252: yuvInfo.fSizeInMemory[i] = yuvInfo.fRowBytes[i] *
yuvInfo.fSize[i].fHeight;
On 2015/01/19 15:14:36, Stephen White wrote:
> Not new to this patch, but it might be nice to document that these sizes are
> rounded up to JPEG block size, and may be larger than the actual width /
height
> of the image (maybe just expand the "for memory allocation" comment above).

Done.

[Stephen White, 2015-01-19 15:43:11]

https://codereview.chromium.org/851273003/diff/40001/src/core/SkYUVPlanesCach...
File src/core/SkYUVPlanesCache.cpp (right):

https://codereview.chromium.org/851273003/diff/40001/src/core/SkYUVPlanesCach...
src/core/SkYUVPlanesCache.cpp:52: YUVValue* result = (YUVValue*)contextData;
This cast is still C-style.

Looks like you fixed the original offender in Masks that you copy-pasted. :)

[sugoi1, 2015-01-19 15:51:58]

https://codereview.chromium.org/851273003/diff/40001/src/core/SkYUVPlanesCach...
File src/core/SkYUVPlanesCache.cpp (right):

https://codereview.chromium.org/851273003/diff/40001/src/core/SkYUVPlanesCach...
src/core/SkYUVPlanesCache.cpp:52: YUVValue* result = (YUVValue*)contextData;
On 2015/01/19 15:43:11, Stephen White wrote:
> This cast is still C-style.
> 
> Looks like you fixed the original offender in Masks that you copy-pasted. :)

haha! wow... Mondays... Fixed it in both spots now.

[Stephen White, 2015-01-19 16:19:31]

LGTM

[sugoi1, 2015-01-19 18:02:46]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2015-01-19 18:03:02]

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

[commit-bot: I haz the power, 2015-01-19 18:10:32]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/692135f9689d4dcb5ba91ff8f4899e268c0bfe11

[reed2, 2015-01-19 19:09:09]

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

[reed2, 2015-01-19 19:09:09]

its ok this landed, but I wasn't really finished reviewing.

https://codereview.chromium.org/851273003/diff/60001/src/core/SkYUVPlanesCache.h
File src/core/SkYUVPlanesCache.h (right):

https://codereview.chromium.org/851273003/diff/60001/src/core/SkYUVPlanesCach...
src/core/SkYUVPlanesCache.h:20: * contiguously, in that order, as a single block
of memory within SkYUVPlanesCache.
Well, I thought it was contiguous, but now perhaps there are gaps, given the
comment about sizesinmemory?

How big are these "gaps" in practice? Its seems like perhaps we could just
require that the blocks really are contiguous, in which case we don't even need
the fSizeInMemory array at all.

[reed2, 2015-01-19 19:09:51]

might also have been good to have robert or brian take a look at the gpu
changes.

[sugoi1, 2015-01-19 19:28:56]

https://codereview.chromium.org/851273003/diff/60001/src/core/SkYUVPlanesCache.h
File src/core/SkYUVPlanesCache.h (right):

https://codereview.chromium.org/851273003/diff/60001/src/core/SkYUVPlanesCach...
src/core/SkYUVPlanesCache.h:20: * contiguously, in that order, as a single block
of memory within SkYUVPlanesCache.
On 2015/01/19 19:09:09, reed2 wrote:
> Well, I thought it was contiguous, but now perhaps there are gaps, given the
> comment about sizesinmemory?
> 
> How big are these "gaps" in practice? Its seems like perhaps we could just
> require that the blocks really are contiguous, in which case we don't even
need
> the fSizeInMemory array at all.

Typically, block sizes are 8 and width and height are also multiples of 8, so in
that case, there would be no gap, but, for an extreme example, a 9x9 JPEG would
be read into a 16x16 memory buffer (rounded up to the next multiple of the JPEG
block size). In that case, assuming 8 bit data, rowBytes could be 16, rowBytes *
height would be 144 and sizeInMemory would be 256. We can allocate less, the
price to pay is an extra temp buffer allocation at decode time, to read the last
row of blocks, and some added memcpy() calls to copy the memory back into the
YUV plane memory buffer.

[sugoi1, 2015-01-19 19:38:25]

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

[sugoi1, 2015-01-19 19:38:26]

Adding bsalomon@

Sorry for adding you after the cl has landed. I just want you to review the code
in SkGr.cpp. If there are any problems, I'll address them in a follow-up ASAP.

[bsalomon, 2015-01-20 14:19:30]

On 2015/01/19 19:38:26, sugoi1 wrote:
> Adding bsalomon@
> 
> Sorry for adding you after the cl has landed. I just want you to review the
code
> in SkGr.cpp. If there are any problems, I'll address them in a follow-up ASAP.

thanks, lgtm.