Add support for the Rec601 YUV color space to GrYUVtoRGBEffect.

Add support for the Rec601 YUV color space to GrYUVtoRGBEffect.

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

[rileya (GONE FROM CHROMIUM), 2014-08-28 01:03:28]

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

[rileya (GONE FROM CHROMIUM), 2014-08-28 01:03:28]

This is somewhat speculative at this point, but I'm looking at using Ganesh for
chromium's video YUV conversion in some cases. Copying frames from video ->
canvas/WebGL currently uses a very slow software YUV conversion path,
GrYUVtoRGBEffect speeds things up ~3-4x.

This patch adds support for the most common video YUV color space: Rec.601. This
is very similar to the JPEG color space, but with a constrained color range.

I have no intention of landing this unless the Blink/chromium components go
through, since I doubt skia has a use for this otherwise... But I wanted to get
this up for review now to get some thoughts.

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
File src/gpu/effects/GrYUVtoRGBEffect.cpp (right):

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
src/gpu/effects/GrYUVtoRGBEffect.cpp:70: "1.0, -0.34414, -0.71414, 
0.529,\n\t\t\t"
I added a couple decimal points to the original JPEG version so that it matches
the YUV conversion implementation in chromium's compositor:

https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...

which is based on libjpeg itself's numbers:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjpe...

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
File src/gpu/effects/GrYUVtoRGBEffect.h (right):

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
src/gpu/effects/GrYUVtoRGBEffect.h:15: enum YUVColorSpace {
Is there a better place for this enum to live?

[bsalomon, 2014-08-28 13:33:26]

Hi, Riley! :)

It seems fine to me. I'm curious how the video frame will make it to the yuv
effect? That is, what skcanvas API is used? Is it already in textures or is this
sw-decoded video?

If we actually land this, the GM test should be updated to exercise the YUV code
path.

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
File src/gpu/effects/GrYUVtoRGBEffect.h (right):

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
src/gpu/effects/GrYUVtoRGBEffect.h:15: enum YUVColorSpace {
On 2014/08/28 01:03:28, rileya wrote:
> Is there a better place for this enum to live?

Not sure about whether there is a more natural place, but the enum values should
be suffixed with "_YUVColorSpace".

[Stephen White, 2014-08-28 15:11:06]

Seems ok. My comments are more just annoying/tangential questions. :)

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
File src/gpu/effects/GrYUVtoRGBEffect.cpp (right):

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
src/gpu/effects/GrYUVtoRGBEffect.cpp:69: "mat4(1.0,  0.0,     1.402,   
-0.701,\n\t\t\t"
Not new to this patch, but I wonder if it's worth using a uniform mat4 here,
instead of a constant. It would make for one fewer shader permutation, at the
cost of a matrix upload.

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
File src/gpu/effects/GrYUVtoRGBEffect.h (right):

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
src/gpu/effects/GrYUVtoRGBEffect.h:17: kJPEG,
This should be kJPEG_YUVColorSpace according to Skia coding style.

Maybe we should just call it ColorSpace, since the YUV is implied by the scoping
class? It would make the enum values a little less ungainly.

Out of curiosity, will we need Rec.709 as well? The coefficients are slightly
different for HD video (for reasons that escape me).

[bsalomon, 2014-08-28 15:18:07]

On 2014/08/28 13:33:26, bsalomon wrote:
> 
> If we actually land this, the GM test should be updated to exercise the YUV
code
> path.
> 

I meant the Rec601 code path.

[rileya (GONE FROM CHROMIUM), 2014-08-28 21:05:57]

> Hi, Riley! :)

Hey Brian! :)

> It seems fine to me. I'm curious how the video frame will make it to the yuv
> effect? That is, what skcanvas API is used? Is it already in textures or is
this
> sw-decoded video?

I'm focusing on SW-decoded video here. The HW path needs some attention too
though...

SkCanvasVideoRenderer::Paint (src/media/filters/SkCanvasVideoRenderer.[cc|h])
does YUV conversion and paints the result into a SkCanvas that is passed in. The
current solution converts to RGB on the CPU before drawBitmapRect-ing the result
into the destination canvas. In the accelerated <canvas> case this seems silly,
since the GPU is way better suited to color conversion, and the result is
getting uploaded to the GPU anyway (as an added bonus, YUV is usually 12bpp, so
there's less to copy).

Currently I'm wrapping the Y, U and V planes in SkBitmaps and making GrTextures
from those, which I then use to paint the frame using the yuv effect.

My WIP patch is here: https://codereview.chromium.org/512733003/

^ I'm unsure on the caching story there though, since ideally reusing the YUV
textures for multiple paints of a single frame is desirable, but I'm unsure if
the Gr texture cache will handle video frame data well (unless the video is
paused, an individual texture is only useful for a very short period time...).

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
File src/gpu/effects/GrYUVtoRGBEffect.cpp (right):

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
src/gpu/effects/GrYUVtoRGBEffect.cpp:69: "mat4(1.0,  0.0,     1.402,   
-0.701,\n\t\t\t"
On 2014/08/28 15:11:06, Stephen White wrote:
> Not new to this patch, but I wonder if it's worth using a uniform mat4 here,
> instead of a constant. It would make for one fewer shader permutation, at the
> cost of a matrix upload.

I'll need to refresh on how exactly uniforms get plumbed around in Gr, but I'll
take a look.

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
File src/gpu/effects/GrYUVtoRGBEffect.h (right):

https://codereview.chromium.org/516463005/diff/20001/src/gpu/effects/GrYUVtoR...
src/gpu/effects/GrYUVtoRGBEffect.h:17: kJPEG,
On 2014/08/28 15:11:06, Stephen White wrote:
> This should be kJPEG_YUVColorSpace according to Skia coding style.
> 
> Maybe we should just call it ColorSpace, since the YUV is implied by the
scoping
> class? It would make the enum values a little less ungainly.
> 
> Out of curiosity, will we need Rec.709 as well? The coefficients are slightly
> different for HD video (for reasons that escape me).

Yeah, in the long term rec709 should be supported. However, none of the existing
YUV paths in chromium actually support it yet, so that'll need to be a separate
effort that touches cc/, libyuv, etc.

[rileya (GONE FROM CHROMIUM), 2014-09-11 06:59:30]

Alright, looks like things work chromium-side, so giving this some more
attention.

It now uses uniforms, rather than generating different code.

I plumbed the color space into the ImageGenerator/PixelRef *YUV8Planes
interfaces.

I also added a gm for the new color space.

https://codereview.chromium.org/516463005/diff/100001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/516463005/diff/100001/include/core/SkImageGen...
include/core/SkImageGenerator.h:134: virtual bool onGetYUV8Planes(SkISize
sizes[3], void* planes[3], size_t rowBytes[3]);
I'm retaining the original interface, but if Blink's usage is changed can we
safely remove this afterwards?

https://codereview.chromium.org/516463005/diff/100001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.chromium.org/516463005/diff/100001/include/core/SkPixelRef...
include/core/SkPixelRef.h:223: enum YUVColorSpace {
This would probably be better off somewhere else, any thoughts on where?

[reed1, 2014-09-11 14:58:54]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-09-11 14:58:56]

enum SkYUVColorSpace(Type) {
   ...
};

in SkImageInfo... ? (that's where we have some other "global" enums for color
types.

https://codereview.chromium.org/516463005/diff/100001/src/core/SkImageGenerat...
File src/core/SkImageGenerator.cpp (right):

https://codereview.chromium.org/516463005/diff/100001/src/core/SkImageGenerat...
src/core/SkImageGenerator.cpp:103: *colorSpace =
SkPixelRef::kJPEG_YUVColorSpace;
do we know its JPEG at this point? Isn't that the job of onGetYUV8Planes?

[rileya (GONE FROM CHROMIUM), 2014-09-11 17:38:38]

No new patchset yet, but one comment response:

https://codereview.chromium.org/516463005/diff/100001/src/core/SkImageGenerat...
File src/core/SkImageGenerator.cpp (right):

https://codereview.chromium.org/516463005/diff/100001/src/core/SkImageGenerat...
src/core/SkImageGenerator.cpp:103: *colorSpace =
SkPixelRef::kJPEG_YUVColorSpace;
On 2014/09/11 14:58:55, reed1 wrote:
> do we know its JPEG at this point? Isn't that the job of onGetYUV8Planes?

For the sake of working with the old (sans color space) interface, the default
impl for this assumes jpeg. Once chromium/blink override the new version of this
virtual, I plan on nuking the old one entirely and removing this (assuming
nobody anywhere else uses it?).

I'll add a comment explaining this.

[reed1, 2014-09-11 17:47:06]

On 2014/09/11 17:38:38, rileya wrote:
> No new patchset yet, but one comment response:
> 
>
https://codereview.chromium.org/516463005/diff/100001/src/core/SkImageGenerat...
> File src/core/SkImageGenerator.cpp (right):
> 
>
https://codereview.chromium.org/516463005/diff/100001/src/core/SkImageGenerat...
> src/core/SkImageGenerator.cpp:103: *colorSpace =
> SkPixelRef::kJPEG_YUVColorSpace;
> On 2014/09/11 14:58:55, reed1 wrote:
> > do we know its JPEG at this point? Isn't that the job of onGetYUV8Planes?
> 
> For the sake of working with the old (sans color space) interface, the default
> impl for this assumes jpeg. Once chromium/blink override the new version of
this
> virtual, I plan on nuking the old one entirely and removing this (assuming
> nobody anywhere else uses it?).
> 
> I'll add a comment explaining this.

Ah yes, the evils of client overriding

[sugoi1, 2014-09-11 17:57:23]

https://codereview.chromium.org/516463005/diff/100001/src/gpu/effects/GrYUVto...
File src/gpu/effects/GrYUVtoRGBEffect.cpp (right):

https://codereview.chromium.org/516463005/diff/100001/src/gpu/effects/GrYUVto...
src/gpu/effects/GrYUVtoRGBEffect.cpp:65: fMatrixUni =
builder->addUniform(GrGLProgramBuilder::kFragment_Visibility,
Hi, sorry for the late comment:

Instead of adding a uniform, couldn't we add a key and produce 2 shaders with
hardcoded matrices inside them, since we already know what the matrices are. I
doubt we'd have more than 255 color spaces any time soon. Otherwise, this shader
would just become a variant of SkColorMatrixFilter, with 3 inputs, no?

[sugoi1, 2014-09-11 17:58:12]

On 2014/09/11 17:57:23, sugoi1 wrote:
>
https://codereview.chromium.org/516463005/diff/100001/src/gpu/effects/GrYUVto...
> File src/gpu/effects/GrYUVtoRGBEffect.cpp (right):
> 
>
https://codereview.chromium.org/516463005/diff/100001/src/gpu/effects/GrYUVto...
> src/gpu/effects/GrYUVtoRGBEffect.cpp:65: fMatrixUni =
> builder->addUniform(GrGLProgramBuilder::kFragment_Visibility,
> Hi, sorry for the late comment:
> 
> Instead of adding a uniform, couldn't we add a key and produce 2 shaders with
> hardcoded matrices inside them, since we already know what the matrices are. I
> doubt we'd have more than 255 color spaces any time soon. Otherwise, this
shader
> would just become a variant of SkColorMatrixFilter, with 3 inputs, no?

Never mind, I see this was discussed.

[rileya (GONE FROM CHROMIUM), 2014-09-11 18:45:05]

Okay, I moved the color space enum to SkImageInfo.

https://codereview.chromium.org/516463005/diff/100001/src/core/SkImageGenerat...
File src/core/SkImageGenerator.cpp (right):

https://codereview.chromium.org/516463005/diff/100001/src/core/SkImageGenerat...
src/core/SkImageGenerator.cpp:103: *colorSpace =
SkPixelRef::kJPEG_YUVColorSpace;
On 2014/09/11 17:38:38, rileya wrote:
> On 2014/09/11 14:58:55, reed1 wrote:
> > do we know its JPEG at this point? Isn't that the job of onGetYUV8Planes?
> 
> For the sake of working with the old (sans color space) interface, the default
> impl for this assumes jpeg. Once chromium/blink override the new version of
this
> virtual, I plan on nuking the old one entirely and removing this (assuming
> nobody anywhere else uses it?).
> 
> I'll add a comment explaining this.

Added a comment.

[reed1, 2014-09-11 19:04:32]

API lgtm

[bsalomon, 2014-09-11 20:17:17]

lgtm w/ a couple small comments.

https://codereview.chromium.org/516463005/diff/120001/gm/yuvtorgbeffect.cpp
File gm/yuvtorgbeffect.cpp (right):

https://codereview.chromium.org/516463005/diff/120001/gm/yuvtorgbeffect.cpp#n...
gm/yuvtorgbeffect.cpp:146: DEF_GM( return SkNEW_ARGS(YUVtoRGBEffect,
(kJPEG_SkYUVColorSpace)); )
Is it possible to make the existing GM draw twice? I know it's a bit of a pain
to rebase an existing GM, but I'd like to avoid exploding the number of GM
images.

https://codereview.chromium.org/516463005/diff/120001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/516463005/diff/120001/include/core/SkImageGen...
include/core/SkImageGenerator.h:135: virtual bool onGetYUV8Planes(SkISize
sizes[3], void* planes[3], size_t rowBytes[3],
Is the plan to remove the virtual that doesn't take the color space on Chromium
code no longer overrides it?

https://codereview.chromium.org/516463005/diff/120001/src/core/SkImageGenerat...
File src/core/SkImageGenerator.cpp (right):

https://codereview.chromium.org/516463005/diff/120001/src/core/SkImageGenerat...
src/core/SkImageGenerator.cpp:109: return onGetYUV8Planes(sizes, planes,
rowBytes);
this->

[rileya (GONE FROM CHROMIUM), 2014-09-11 20:43:40]

https://codereview.chromium.org/516463005/diff/120001/gm/yuvtorgbeffect.cpp
File gm/yuvtorgbeffect.cpp (right):

https://codereview.chromium.org/516463005/diff/120001/gm/yuvtorgbeffect.cpp#n...
gm/yuvtorgbeffect.cpp:146: DEF_GM( return SkNEW_ARGS(YUVtoRGBEffect,
(kJPEG_SkYUVColorSpace)); )
On 2014/09/11 20:17:17, bsalomon wrote:
> Is it possible to make the existing GM draw twice? I know it's a bit of a pain
> to rebase an existing GM, but I'd like to avoid exploding the number of GM
> images.

Done. Is the rebasing process described here still the way to go?:
https://sites.google.com/site/skiadocs/developer-documentation/testing/using-...

https://codereview.chromium.org/516463005/diff/120001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/516463005/diff/120001/include/core/SkImageGen...
include/core/SkImageGenerator.h:135: virtual bool onGetYUV8Planes(SkISize
sizes[3], void* planes[3], size_t rowBytes[3],
On 2014/09/11 20:17:17, bsalomon wrote:
> Is the plan to remove the virtual that doesn't take the color space on
Chromium
> code no longer overrides it?

Exactly, assuming we're okay changing the interface (I imagine Chromium/Blink
are the only users of the interface at this point...).

https://codereview.chromium.org/516463005/diff/120001/src/core/SkImageGenerat...
File src/core/SkImageGenerator.cpp (right):

https://codereview.chromium.org/516463005/diff/120001/src/core/SkImageGenerat...
src/core/SkImageGenerator.cpp:109: return onGetYUV8Planes(sizes, planes,
rowBytes);
On 2014/09/11 20:17:17, bsalomon wrote:
> this->

Done.

[Stephen White, 2014-09-11 22:25:10]

https://codereview.chromium.org/516463005/diff/140001/include/core/SkImageGen...
File include/core/SkImageGenerator.h (right):

https://codereview.chromium.org/516463005/diff/140001/include/core/SkImageGen...
include/core/SkImageGenerator.h:13: #include "SkPixelRef.h"
Is this necessary? It looks like the new dependency in this file is the
SkYUVColorSpace enum, which is in SkColor.h. Am I missing something?

[rileya (GONE FROM CHROMIUM), 2014-09-11 22:38:03]

On 2014/09/11 22:25:10, Stephen White wrote:
>
https://codereview.chromium.org/516463005/diff/140001/include/core/SkImageGen...
> File include/core/SkImageGenerator.h (right):
> 
>
https://codereview.chromium.org/516463005/diff/140001/include/core/SkImageGen...
> include/core/SkImageGenerator.h:13: #include "SkPixelRef.h"
> Is this necessary? It looks like the new dependency in this file is the
> SkYUVColorSpace enum, which is in SkColor.h. Am I missing something?

The SkYUVColorSpace enum is in SkImageInfo.h (this CL adds it), per Mike's
suggestion (comment #9).

[Stephen White, 2014-09-11 23:06:08]

On 2014/09/11 22:38:03, rileya wrote:
> On 2014/09/11 22:25:10, Stephen White wrote:
> >
>
https://codereview.chromium.org/516463005/diff/140001/include/core/SkImageGen...
> > File include/core/SkImageGenerator.h (right):
> > 
> >
>
https://codereview.chromium.org/516463005/diff/140001/include/core/SkImageGen...
> > include/core/SkImageGenerator.h:13: #include "SkPixelRef.h"
> > Is this necessary? It looks like the new dependency in this file is the
> > SkYUVColorSpace enum, which is in SkColor.h. Am I missing something?
> 
> The SkYUVColorSpace enum is in SkImageInfo.h (this CL adds it), per Mike's
> suggestion (comment #9).

Sorry my mistake. The question stands, though: why include SkPixelRef.h?

[rileya (GONE FROM CHROMIUM), 2014-09-11 23:09:39]

> Sorry my mistake. The question stands, though: why include SkPixelRef.h?

Whoops, *my* mistake, haha. I'd originally stuck the enum in SkPixelRef.h, but
neglected to remove the #include when I moved it to SkImageInfo.h.

Fixed in patchset 9

[bsalomon, 2014-09-12 00:19:05]

https://codereview.chromium.org/516463005/diff/120001/gm/yuvtorgbeffect.cpp
File gm/yuvtorgbeffect.cpp (right):

https://codereview.chromium.org/516463005/diff/120001/gm/yuvtorgbeffect.cpp#n...
gm/yuvtorgbeffect.cpp:146: DEF_GM( return SkNEW_ARGS(YUVtoRGBEffect,
(kJPEG_SkYUVColorSpace)); )
On 2014/09/11 20:43:40, rileya wrote:
> On 2014/09/11 20:17:17, bsalomon wrote:
> > Is it possible to make the existing GM draw twice? I know it's a bit of a
pain
> > to rebase an existing GM, but I'd like to avoid exploding the number of GM
> > images.
> 
> Done. Is the rebasing process described here still the way to go?:
>
https://sites.google.com/site/skiadocs/developer-documentation/testing/using-...

Yup, you'll want to add the test name to expectations/gm/ignored-tests.txt as
part of the land. That way the bots won't go red. After all the bots have cycled
you rebaseline the tests you and remove the test name from the file.

[Stephen White, 2014-09-12 19:20:02]

LGTM, but I'd like sugoi@ to take a look too. Alexis?

[sugoi1, 2014-09-12 20:12:24]

On 2014/09/12 19:20:02, Stephen White wrote:
> LGTM, but I'd like sugoi@ to take a look too. Alexis?

It's ok as an intermediate step, as long as the next step follows quickly after
this one. LGTM.

[rileya (GONE FROM CHROMIUM), 2014-09-12 23:53:24]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-12 23:54:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/516463005/180001

[commit-bot: I haz the power, 2014-09-13 00:02:11]

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

[commit-bot: I haz the power, 2014-09-13 00:02:12]

Try jobs failed on following builders:
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)

[rileya (GONE FROM CHROMIUM), 2014-09-13 00:12:14]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-13 00:12:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/516463005/200001

[commit-bot: I haz the power, 2014-09-13 00:33:33]

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

[commit-bot: I haz the power, 2014-09-13 00:33:34]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[rileya (GONE FROM CHROMIUM), 2014-09-13 00:36:36]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-09-13 00:37:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/516463005/220001

[commit-bot: I haz the power, 2014-09-13 00:46:02]

Committed patchset #12 (id:220001) as abaef86f2b37d8a939506a2076da07f6db456951