Add ability to extract YUV planes from SkImage

Add ability to extract YUV planes from SkImage

GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1513393002

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

[bsalomon, 2016-01-25 19:56:05]

Description was changed from

==========
[WIP] Initial version of RGBA->YUV for SkImage
==========

to

==========
[WIP] Initial version of RGBA->YUV for SkImage
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bsalomon, 2016-01-25 20:16:16]

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

[bsalomon, 2016-01-25 20:16:17]

This isn't quite done. The GPU version works. I wasn't sure what approach to
take for the CPU implementation. What I have doesn't quite work for reasons I
don't yet understand, but it also relies on pulling in SkColorMatrixFilter from
effects.

[bsalomon, 2016-01-25 20:16:45]

Description was changed from

==========
[WIP] Initial version of RGBA->YUV for SkImage
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add ability to extract YUV planes from SkImage

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[reed1, 2016-01-26 16:06:45]

what is driving the public api?

https://codereview.chromium.org/1513393002/diff/160001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1513393002/diff/160001/include/core/SkImage.h...
include/core/SkImage.h:157: bool asYUV8Planes(const SkISize[3], void* const
planes[3], const size_t rowBytes[3],
Is this intended to always succeed, even if the underlying form is not
intrinsically YUV? (e.g. rgb or picture)

Matt and Leon have gone to great lengths to allow the codec to specify the
sizes/rowbytes for each plane (including padding sometimes). How does the caller
arrive at the specified sizes here?

[bsalomon, 2016-01-26 16:23:48]

https://codereview.chromium.org/1513393002/diff/180001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1513393002/diff/180001/include/core/SkImage.h...
include/core/SkImage.h:157: bool asYUV8Planes(const SkISize[3], void* const
planes[3], const size_t rowBytes[3],
One thing I wondered is whether this should take a SkFilterQuality for scaling
the planes. Currently I use bilerp which should be OK for the common case of a
scale factor of 2. Our reverse conversions don't take a quality so maybe
assuming bilerp is alright.

[bsalomon, 2016-01-26 16:32:50]

https://codereview.chromium.org/1513393002/diff/160001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1513393002/diff/160001/include/core/SkImage.h...
include/core/SkImage.h:157: bool asYUV8Planes(const SkISize[3], void* const
planes[3], const size_t rowBytes[3],
On 2016/01/26 16:06:45, reed1 wrote:
> Is this intended to always succeed, even if the underlying form is not
> intrinsically YUV? (e.g. rgb or picture)

Yes, it is for turning a SkImage into a video frame. The motivating use case is
for taking rendered canvas2D frames and turning them into a video stream. So
typically the content behind the image is a newImageSnapshot ()from a GPU
(though probably could be CPU) SkSurface. I hadn't thought about cases where the
original data is YUV. That would accelerate that use but isn't really the target
use case.

> 
> Matt and Leon have gone to great lengths to allow the codec to specify the
> sizes/rowbytes for each plane (including padding sometimes). How does the
caller
> arrive at the specified sizes here?

That would based on what the video encoder can ingest, probably 4:2:0 (y full
sized, u and v half sized in both dimensions).

[bsalomon, 2016-01-27 18:57:19]

The hacky cpu implementation now works (didn't realize SkColorMatrix's 5th
column was unnormalized[!?])

[reed1, 2016-01-28 22:12:45]

I wouldn't fret optimizing til later

lgtm /w some comment/rowbytes suggestions

https://codereview.chromium.org/1513393002/diff/220001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1513393002/diff/220001/src/image/SkImage.cpp#...
src/image/SkImage.cpp:277: {0.256951f,  0.504421f,  0.0977346f, 0.f, 0.0625f *
255.f,
// I'm this colorspace

https://codereview.chromium.org/1513393002/diff/220001/src/image/SkImage.cpp#...
src/image/SkImage.cpp:307: pixmap.reset(info, pixels.get(), 0);
pass the explicit rowBYtes (width() * 4)

[bsalomon, 2016-01-29 16:52:55]

Please take another look.

[reed1, 2016-01-29 17:17:00]

I like the single-row helper. makes things more compartmentalized.

where is the SkRGBToYUV code implemented?

https://codereview.chromium.org/1513393002/diff/300001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1513393002/diff/300001/include/core/SkImage.h...
include/core/SkImage.h:161: bool asYUV8Planes(const SkISize[3], void* const
planes[3], const size_t rowBytes[3],
bikeshed: readYUV8Planes -- to be suggestive of readPixels(), which also 1.
converts to the required config, 2. always copies into the buffer allocated by
the caller.

https://codereview.chromium.org/1513393002/diff/300001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1513393002/diff/300001/src/image/SkImage.cpp#...
src/image/SkImage.cpp:276: SkRGBAToYUV(this, sizes, planes, rowBytes,
colorSpace);
Hmmm, I thought the "utility" function would take a pixmap (simple), rather than
an image (complex) as its src.

[bsalomon, 2016-01-29 17:36:42]

Oops, forgot to add the new h/cpp to git. Updated.

https://codereview.chromium.org/1513393002/diff/300001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/1513393002/diff/300001/include/core/SkImage.h...
include/core/SkImage.h:161: bool asYUV8Planes(const SkISize[3], void* const
planes[3], const size_t rowBytes[3],
On 2016/01/29 17:17:00, reed1 wrote:
> bikeshed: readYUV8Planes -- to be suggestive of readPixels(), which also 1.
> converts to the required config, 2. always copies into the buffer allocated by
> the caller.

Done.

https://codereview.chromium.org/1513393002/diff/300001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1513393002/diff/300001/src/image/SkImage.cpp#...
src/image/SkImage.cpp:276: SkRGBAToYUV(this, sizes, planes, rowBytes,
colorSpace);
On 2016/01/29 17:17:00, reed1 wrote:
> Hmmm, I thought the "utility" function would take a pixmap (simple), rather
than
> an image (complex) as its src.

I originally typed it that way and then realized I needed to convert it back to
something complex in the implementation in order to draw it.

[reed1, 2016-01-29 20:14:55]

lgtm

https://codereview.chromium.org/1513393002/diff/340001/src/utils/SkRGBAToYUV.h
File src/utils/SkRGBAToYUV.h (right):

https://codereview.chromium.org/1513393002/diff/340001/src/utils/SkRGBAToYUV....
src/utils/SkRGBAToYUV.h:16: void SkRGBAToYUV(const SkImage*, const SkISize [3],
void* const planes[3],
// Works with any image type at the moment, but in the future it may only work
with raster-backed images...

[robertphillips, 2016-01-29 20:33:13]

https://codereview.chromium.org/1513393002/diff/340001/gm/imagetoyuvplanes.cpp
File gm/imagetoyuvplanes.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/gm/imagetoyuvplanes.cp...
gm/imagetoyuvplanes.cpp:49: canvas->drawImage(rgbImage, kPad, kPad);
// Test out 444, 422 and 42(1.333) YUV formats 

?

If that comment is correct why are we testing 42(1.333) and not 421 ?

https://codereview.chromium.org/1513393002/diff/340001/gm/imagetoyuvplanes.cp...
gm/imagetoyuvplanes.cpp:65: void *planes[3] = {yPlane.get(), uPlane.get(),
vPlane.get()};
Should we exercise the rowBytes ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
File src/gpu/GrTextureToYUVPlanes.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
src/gpu/GrTextureToYUVPlanes.cpp:68: if (sizes[0] == sizes[1] && sizes[1] ==
sizes[2]) {
// YUV444 - only 1 draw/readback

?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
src/gpu/GrTextureToYUVPlanes.cpp:88: if (sizes[1] == sizes[2]) {
// Something like 422 or 411 - only 2 draws/readbacks (y & uv)

?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
src/gpu/GrTextureToYUVPlanes.cpp:99: } else {
// Each channel is different sized - 3 draws/readbacks 

?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
src/gpu/GrTextureToYUVPlanes.cpp:166: if (yuvTex) {
The use of sizes[0], [1] & [2] mildly bothers me here. Can we just assert
they're all the same and just use one ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
src/gpu/GrTextureToYUVPlanes.cpp:203: }
SkASSERT(sizes[1].fWidth == sizes[2].fWidth); ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
File src/gpu/effects/GrYUVEffect.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:67: SkYUVColorSpace colorSpace) {
Don't all the 'sizes' need to be the same in this case ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:94: SkYUVColorSpace getColorSpace() const {
1 line ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:104: 
rm virtual ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:107: 
yuvMatrix -> colorSpaceMat ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:112: "YUVMatrix", &yuvMatrix);
rm formatting control characters ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:125: protected:
rm virtual ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:171: 
rm virtual ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:232: 
rm virtual ?

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
File src/gpu/effects/GrYUVEffect.h (right):

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.h:7: 
GrYUVEffect_DEFINED ?

https://codereview.chromium.org/1513393002/diff/340001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/src/image/SkImage.cpp#...
src/image/SkImage.cpp:266: 
overlength ?

https://codereview.chromium.org/1513393002/diff/340001/src/utils/SkRGBAToYUV.cpp
File src/utils/SkRGBAToYUV.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/src/utils/SkRGBAToYUV....
src/utils/SkRGBAToYUV.cpp:43: SkPaint paint;
Always low ?

[bsalomon, 2016-02-01 20:27:45]

https://codereview.chromium.org/1513393002/diff/340001/gm/imagetoyuvplanes.cpp
File gm/imagetoyuvplanes.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/gm/imagetoyuvplanes.cp...
gm/imagetoyuvplanes.cpp:49: canvas->drawImage(rgbImage, kPad, kPad);
On 2016/01/29 20:33:13, robertphillips wrote:
> // Test out 444, 422 and 42(1.333) YUV formats 
> 
> ?
> 
> If that comment is correct why are we testing 42(1.333) and not 421 ?


I wasn't necessarily thinking about particular YUV formats, but wanted to
exercise three code paths - all planes the same size, uv the same size, and all
different. Added comment.

https://codereview.chromium.org/1513393002/diff/340001/gm/imagetoyuvplanes.cp...
gm/imagetoyuvplanes.cpp:65: void *planes[3] = {yPlane.get(), uPlane.get(),
vPlane.get()};
On 2016/01/29 20:33:13, robertphillips wrote:
> Should we exercise the rowBytes ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
File src/gpu/GrTextureToYUVPlanes.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
src/gpu/GrTextureToYUVPlanes.cpp:68: if (sizes[0] == sizes[1] && sizes[1] ==
sizes[2]) {
On 2016/01/29 20:33:13, robertphillips wrote:
> // YUV444 - only 1 draw/readback
> 
> ?

I put a comment above the first if about the three cases.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
src/gpu/GrTextureToYUVPlanes.cpp:166: if (yuvTex) {
On 2016/01/29 20:33:13, robertphillips wrote:
> The use of sizes[0], [1] & [2] mildly bothers me here. Can we just assert
> they're all the same and just use one ?

Compromise - assert and then use another scoped var: yuvSize.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/GrTextureToYUV...
src/gpu/GrTextureToYUVPlanes.cpp:203: }
On 2016/01/29 20:33:13, robertphillips wrote:
> SkASSERT(sizes[1].fWidth == sizes[2].fWidth); ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
File src/gpu/effects/GrYUVEffect.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:67: SkYUVColorSpace colorSpace) {
On 2016/01/29 20:33:13, robertphillips wrote:
> Don't all the 'sizes' need to be the same in this case ?

Most of these comments are about YUVtoRGBEffect. I didn't edit anything in
YUVtoRGBEffect, just renamed this file and added RBGToYUVEffect.

This class supports a mix of sizes and can draw to a dest with a matrix. It just
bilerps the three textures, runs the yuv through the matrix and outputs a color.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:94: SkYUVColorSpace getColorSpace() const {
On 2016/01/29 20:33:13, robertphillips wrote:
> 1 line ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:104: 
On 2016/01/29 20:33:13, robertphillips wrote:
> rm virtual ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:107: 
On 2016/01/29 20:33:13, robertphillips wrote:
> yuvMatrix -> colorSpaceMat ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:112: "YUVMatrix", &yuvMatrix);
On 2016/01/29 20:33:13, robertphillips wrote:
> rm formatting control characters ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:125: protected:
On 2016/01/29 20:33:13, robertphillips wrote:
> rm virtual ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:171: 
On 2016/01/29 20:33:13, robertphillips wrote:
> rm virtual ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.cpp:232: 
On 2016/01/29 20:33:13, robertphillips wrote:
> rm virtual ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
File src/gpu/effects/GrYUVEffect.h (right):

https://codereview.chromium.org/1513393002/diff/340001/src/gpu/effects/GrYUVE...
src/gpu/effects/GrYUVEffect.h:7: 
On 2016/01/29 20:33:13, robertphillips wrote:
> GrYUVEffect_DEFINED ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/image/SkImage.cpp
File src/image/SkImage.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/src/image/SkImage.cpp#...
src/image/SkImage.cpp:266: 
On 2016/01/29 20:33:13, robertphillips wrote:
> overlength ?

Done.

https://codereview.chromium.org/1513393002/diff/340001/src/utils/SkRGBAToYUV.cpp
File src/utils/SkRGBAToYUV.cpp (right):

https://codereview.chromium.org/1513393002/diff/340001/src/utils/SkRGBAToYUV....
src/utils/SkRGBAToYUV.cpp:43: SkPaint paint;
On 2016/01/29 20:33:13, robertphillips wrote:
> Always low ?

I thought about having the filter quality be part of the public API. However,
our other similar conversion all just implement bilerp. The common cases all
should use bilerp. It's hard to imagine that a caller would want us to generate
MIPs or a high quality scale for this. If the caller really wants this control
they can always by draw the RGBA image to a RGBA surface with the desired filter
quality and then call readYUV8Planes.

https://codereview.chromium.org/1513393002/diff/340001/src/utils/SkRGBAToYUV.h
File src/utils/SkRGBAToYUV.h (right):

https://codereview.chromium.org/1513393002/diff/340001/src/utils/SkRGBAToYUV....
src/utils/SkRGBAToYUV.h:16: void SkRGBAToYUV(const SkImage*, const SkISize [3],
void* const planes[3],
On 2016/01/29 20:14:55, reed1 wrote:
> // Works with any image type at the moment, but in the future it may only work
> with raster-backed images...

Done.

[robertphillips, 2016-02-01 20:45:43]

lgtm

[bsalomon, 2016-02-01 20:58:55]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2016-02-01 20:58:56]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com,
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1513393002/#ps420001
(title: "gm cleanup")

[commit-bot: I haz the power, 2016-02-01 20:59:08]

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

[commit-bot: I haz the power, 2016-02-01 21:16:20]

Description was changed from

==========
Add ability to extract YUV planes from SkImage

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add ability to extract YUV planes from SkImage

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

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

[commit-bot: I haz the power, 2016-02-01 21:16:20]

Committed patchset #22 (id:420001) as
https://skia.googlesource.com/skia/+/f267c1efe7de7a8e71404afde6cbf93c3808d267