Support videos with JPEG color range in GPU YUV convert path.

Support videos with JPEG color range in GPU YUV convert path.

It's worth noting that the media_browsertests Yuv* tests compare video pixels with a reference image, but whether or not the hardware path is used, copying the pixels from the video into a canvas goes through the software path, so despite looking correct onscreen when GPU-accelerated, the JPEG color range video tests will still fail until the software path properly supports the JPEG color range.

BUG=172898
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268010

[rileya (GONE FROM CHROMIUM), 2013-12-03 00:23:04]

Note that this depends on: https://codereview.chromium.org/88403004/

[danakj, 2013-12-03 00:31:47]

https://codereview.chromium.org/92703003/diff/80001/cc/quads/yuv_video_draw_q...
File cc/quads/yuv_video_draw_quad.h (right):

https://codereview.chromium.org/92703003/diff/80001/cc/quads/yuv_video_draw_q...
cc/quads/yuv_video_draw_quad.h:30: bool jpeg_color_range);
This change should be represented in cc_messages.h and cc_messages_unittest.cc

[rileya (GONE FROM CHROMIUM), 2013-12-03 01:22:58]

> This change should be represented in cc_messages.h and cc_messages_unittest.cc

Alright, done.

+tsepez: please have a look at content/common/cc_messages*

[Tom Sepez, 2013-12-03 02:21:58]

On 2013/12/03 01:22:58, rileya wrote:
> > This change should be represented in cc_messages.h and
cc_messages_unittest.cc
> 
> Alright, done.
> 
> +tsepez: please have a look at content/common/cc_messages*

Messages LGTM, although I might have called it has_jpeg_color_range or
is_jpeg_color_range to emphasize its boolness.  No matter.

[rileya (GONE FROM CHROMIUM), 2013-12-03 19:08:54]

On 2013/12/03 02:21:58, Tom Sepez wrote:
> On 2013/12/03 01:22:58, rileya wrote:
> > > This change should be represented in cc_messages.h and
> cc_messages_unittest.cc
> > 
> > Alright, done.
> > 
> > +tsepez: please have a look at content/common/cc_messages*
> 
> Messages LGTM, although I might have called it has_jpeg_color_range or
> is_jpeg_color_range to emphasize its boolness.  No matter.

Good point, I renamed it to "has_jpeg_color_range".

[danakj, 2013-12-03 19:15:47]

Given the discussion on https://codereview.chromium.org/88403004/ I'll wait for
that to resolve before reviewing this, but overall it looks fine.

[scherkus (not reviewing), 2013-12-03 21:29:12]

lgtm

danakj: I believe you'll want to take a look at
https://codereview.chromium.org/88403004/ and ell-gee-tee-emm the cc/ code

[danakj, 2013-12-03 21:35:33]

https://codereview.chromium.org/92703003/diff/120001/cc/layers/video_layer_im...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/92703003/diff/120001/cc/layers/video_layer_im...
cc/layers/video_layer_impl.cc:176: bool use_jpeg_range = frame_->format() ==
media::VideoFrame::YV12J;
change this name to match the quad variable name?

https://codereview.chromium.org/92703003/diff/120001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/92703003/diff/120001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:470: y_resource, u_resource, v_resource,
a_resource, false);
use a temp variable here to give the boolean literal a name

https://codereview.chromium.org/92703003/diff/120001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:475: TEST_F(VideoGLRendererPixelTest,
SimpleYUVRect) {
Can you make a test like this that uses your new code?

https://codereview.chromium.org/92703003/diff/120001/cc/quads/draw_quad_unitt...
File cc/quads/draw_quad_unittest.cc (right):

https://codereview.chromium.org/92703003/diff/120001/cc/quads/draw_quad_unitt...
cc/quads/draw_quad_unittest.cc:554: bool has_jpeg_color_range = false;
use a non-0 value for this in case copying just sets things to 0, so true.

https://codereview.chromium.org/92703003/diff/120001/cc/quads/draw_quad_unitt...
cc/quads/draw_quad_unittest.cc:793: bool has_jpeg_color_range = false;
same here

https://codereview.chromium.org/92703003/diff/120001/cc/test/render_pass_test...
File cc/test/render_pass_test_common.cc (right):

https://codereview.chromium.org/92703003/diff/120001/cc/test/render_pass_test...
cc/test/render_pass_test_common.cc:246: false);
use a temp variable to give this boolean literal a name

https://codereview.chromium.org/92703003/diff/120001/content/common/cc_messag...
File content/common/cc_messages_unittest.cc (right):

https://codereview.chromium.org/92703003/diff/120001/content/common/cc_messag...
content/common/cc_messages_unittest.cc:407: false);
arbitrary_bool1

[rileya (GONE FROM CHROMIUM), 2013-12-03 23:33:41]

Alright, I think I addressed everything.

https://codereview.chromium.org/92703003/diff/120001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/92703003/diff/120001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:470: y_resource, u_resource, v_resource,
a_resource, false);
On 2013/12/03 21:35:34, danakj wrote:
> use a temp variable here to give the boolean literal a name

Made into a param actually, in order to add the test you suggested below.

https://codereview.chromium.org/92703003/diff/160001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/92703003/diff/160001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:529:
base::FilePath(FILE_PATH_LITERAL("green.png")),
Conveniently the test YUV values evaluate to the same RGB color in both MPEG and
JPEG color ranges (I was surprised... I worked the math out by hand to be sure).

[danakj, 2013-12-03 23:39:04]

https://codereview.chromium.org/92703003/diff/160001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/92703003/diff/160001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:529:
base::FilePath(FILE_PATH_LITERAL("green.png")),
On 2013/12/03 23:33:41, rileya wrote:
> Conveniently the test YUV values evaluate to the same RGB color in both MPEG
and
> JPEG color ranges (I was surprised... I worked the math out by hand to be
sure).

Oh.. that's unfortunate as it means the test would pass even if we stopped using
the right code path in gl_renderer. Can you adjust the test so that it would
fail? Bonus points if you can make use of another existing .png in cc/test/data/

[rileya (GONE FROM CHROMIUM), 2013-12-04 20:37:47]

On 2013/12/03 23:39:04, danakj wrote:
>
https://codereview.chromium.org/92703003/diff/160001/cc/output/renderer_pixel...
> File cc/output/renderer_pixeltest.cc (right):
> 
>
https://codereview.chromium.org/92703003/diff/160001/cc/output/renderer_pixel...
> cc/output/renderer_pixeltest.cc:529:
> base::FilePath(FILE_PATH_LITERAL("green.png")),
> On 2013/12/03 23:33:41, rileya wrote:
> > Conveniently the test YUV values evaluate to the same RGB color in both MPEG
> and
> > JPEG color ranges (I was surprised... I worked the math out by hand to be
> sure).
> 
> Oh.. that's unfortunate as it means the test would pass even if we stopped
using
> the right code path in gl_renderer. Can you adjust the test so that it would
> fail? Bonus points if you can make use of another existing .png in
cc/test/data/

Okay, I ended up adding two new tests, one using the MPEG color range and
expecting a YUV of 15,128,128 to match black.png. And another in JPEG color
range with the same YUV values (those values actually yield dark grey in the
JPEG range) and expecting it to be brighter than black.png (I made a slightly
ugly one-off PixelComparator to do this check).

[danakj, 2013-12-04 20:41:21]

On Wed, Dec 4, 2013 at 3:37 PM, <rileya@chromium.org> wrote:

> On 2013/12/03 23:39:04, danakj wrote:
>
> https://codereview.chromium.org/92703003/diff/160001/cc/
> output/renderer_pixeltest.cc
>
>> File cc/output/renderer_pixeltest.cc (right):
>>
>
>
> https://codereview.chromium.org/92703003/diff/160001/cc/
> output/renderer_pixeltest.cc#newcode529
>
>> cc/output/renderer_pixeltest.cc:529:
>> base::FilePath(FILE_PATH_LITERAL("green.png")),
>> On 2013/12/03 23:33:41, rileya wrote:
>> > Conveniently the test YUV values evaluate to the same RGB color in both
>> MPEG
>> and
>> > JPEG color ranges (I was surprised... I worked the math out by hand to
>> be
>> sure).
>>
>
>  Oh.. that's unfortunate as it means the test would pass even if we stopped
>>
> using
>
>> the right code path in gl_renderer. Can you adjust the test so that it
>> would
>> fail? Bonus points if you can make use of another existing .png in
>>
> cc/test/data/
>
> Okay, I ended up adding two new tests, one using the MPEG color range and
> expecting a YUV of 15,128,128 to match black.png. And another in JPEG color
> range with the same YUV values (those values actually yield dark grey in
> the
> JPEG range) and expecting it to be brighter than black.png (I made a
> slightly
> ugly one-off PixelComparator to do this check).
>

Ah, just adding a new PNG file would be preferable then for the latter. Can
we do that instead? In general, non-expect pixel comparators have just been
to deal with differences between platforms.


> https://codereview.chromium.org/92703003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rileya (GONE FROM CHROMIUM), 2013-12-04 20:54:42]

On 2013/12/04 20:41:21, danakj wrote:
> On Wed, Dec 4, 2013 at 3:37 PM, <mailto:rileya@chromium.org> wrote:
> 
> > On 2013/12/03 23:39:04, danakj wrote:
> >
> > https://codereview.chromium.org/92703003/diff/160001/cc/
> > output/renderer_pixeltest.cc
> >
> >> File cc/output/renderer_pixeltest.cc (right):
> >>
> >
> >
> > https://codereview.chromium.org/92703003/diff/160001/cc/
> > output/renderer_pixeltest.cc#newcode529
> >
> >> cc/output/renderer_pixeltest.cc:529:
> >> base::FilePath(FILE_PATH_LITERAL("green.png")),
> >> On 2013/12/03 23:33:41, rileya wrote:
> >> > Conveniently the test YUV values evaluate to the same RGB color in both
> >> MPEG
> >> and
> >> > JPEG color ranges (I was surprised... I worked the math out by hand to
> >> be
> >> sure).
> >>
> >
> >  Oh.. that's unfortunate as it means the test would pass even if we stopped
> >>
> > using
> >
> >> the right code path in gl_renderer. Can you adjust the test so that it
> >> would
> >> fail? Bonus points if you can make use of another existing .png in
> >>
> > cc/test/data/
> >
> > Okay, I ended up adding two new tests, one using the MPEG color range and
> > expecting a YUV of 15,128,128 to match black.png. And another in JPEG color
> > range with the same YUV values (those values actually yield dark grey in
> > the
> > JPEG range) and expecting it to be brighter than black.png (I made a
> > slightly
> > ugly one-off PixelComparator to do this check).
> >
> 
> Ah, just adding a new PNG file would be preferable then for the latter. Can
> we do that instead? In general, non-expect pixel comparators have just been
> to deal with differences between platforms.
> 
> 
> > https://codereview.chromium.org/92703003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Okay, I added a dark_grey.png image for that test to compare against.

[danakj, 2013-12-04 21:00:12]

LGTM

https://codereview.chromium.org/92703003/diff/220001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/92703003/diff/220001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:629: scoped_ptr<YUVVideoDrawQuad> yuv_quad =
CreateTestYUVVideoDrawQuad(
I like your // YUV of (x,x,x) should be green (y,y,y) in RGB. comments. add it
here too?

[scherkus (not reviewing), 2014-01-06 19:33:59]

rileya: checking in ... is this CL waiting on anything before landing?

[rileya (GONE FROM CHROMIUM), 2014-01-06 19:49:11]

On 2014/01/06 19:33:59, scherkus wrote:
> rileya: checking in ... is this CL waiting on anything before landing?

I wanted to wait until the software path is all set before landing both at about
the same time (and the software path is still pending some performance testing
on Windows that I don't have a machine set up for).

I discovered when writing tests for color conversion that if you copy pixels
from a <video> into a <canvas> a software YUV conversion is always done. So if
the two paths behave differently, the displayed pixels using the hardware path
and the pixels you get from doing a software-path copy into a canvas wouldn't
match up, which seems incorrect/unintuitive.

[fbarchard, 2014-01-17 20:58:52]

cool!  full range 601 makes alot more sense than 'video' range of 16..240.  but
its still a color space designed for cathode ray tubes :-)

https://codereview.chromium.org/92703003/diff/240001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/92703003/diff/240001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1728: 0.0f, -.34414f, 1.772f,
prefer you use the jpeg coefficients from jdcolor.c
 *	R = Y                + 1.40200 * Cr
 *	G = Y - 0.34414 * Cb - 0.71414 * Cr
 *	B = Y + 1.77200 * Cb

[fbarchard, 2014-01-17 23:33:07]

https://codereview.chromium.org/92703003/diff/240001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/92703003/diff/240001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1728: 0.0f, -.34414f, 1.772f,
matrix is transposed compared to the C code, so I think we're good here,
assuming jpeg is correct.

[rileya (GONE FROM CHROMIUM), 2014-05-01 21:45:01]

Software path is finally ready to go, so I just rebased this.

danakj: Please give cc/* a quick once-over. Everything should be about the same
as before, but that was a good 4ish months-worth rebase, so perhaps some changes
are now necessary. Also, cc/output/renderer_pixeltest.cc changed a fair bit.

+piman for OWNER review of content/common/cc_messages_unittest.cc

[danakj, 2014-05-01 21:54:16]

LGTMstill

[Tom Sepez, 2014-05-01 21:56:48]

Still LGTM.

[piman, 2014-05-01 21:59:51]

https://codereview.chromium.org/92703003/diff/340001/cc/quads/yuv_video_draw_...
File cc/quads/yuv_video_draw_quad.h (right):

https://codereview.chromium.org/92703003/diff/340001/cc/quads/yuv_video_draw_...
cc/quads/yuv_video_draw_quad.h:50: bool has_jpeg_color_range;
It sounds like we could want to support other standard color spaces in the
future. Should we make this an enum?

[rileya (GONE FROM CHROMIUM), 2014-05-01 23:25:09]

Okay, switched to an enum for the color space, to allow for easier addition of
more color spaces in the future.

Not much change from the previous patchset, but I'm not super-familiar with the
IPC side of things, so a sanity check to my addition of the enum there would be
much appreciated (tsepez?).

https://codereview.chromium.org/92703003/diff/340001/cc/quads/yuv_video_draw_...
File cc/quads/yuv_video_draw_quad.h (right):

https://codereview.chromium.org/92703003/diff/340001/cc/quads/yuv_video_draw_...
cc/quads/yuv_video_draw_quad.h:50: bool has_jpeg_color_range;
On 2014/05/01 21:59:51, piman wrote:
> It sounds like we could want to support other standard color spaces in the
> future. Should we make this an enum?

Good point. In fact, there's an outstanding issue to support Rec709's subtly
different 'HDTV' color space... Switched to an enum.

https://codereview.chromium.org/92703003/diff/360001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/92703003/diff/360001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1780: float yuv_to_rgb_rec601[9] = {
Is there a header these might be better off being moved to?

[Tom Sepez, 2014-05-02 00:25:36]

https://codereview.chromium.org/92703003/diff/360001/cc/quads/yuv_video_draw_...
File cc/quads/yuv_video_draw_quad.h (right):

https://codereview.chromium.org/92703003/diff/360001/cc/quads/yuv_video_draw_...
cc/quads/yuv_video_draw_quad.h:21: };
Generally, the way we do this is by adding a constant:
ColorSpace_LAST = kRec601_jpeg
to the enum duplicating the highest value (naming this to suit your preferences
is ceratianly ok). Then, in your *_messages.h file, your
IPC_ENUM_TRAITS_MAX_VALUE() declaration would use ColorSpace_LAST, rather than
kRec601_jpeg, so that down the road, if someone adds a new case, the messages
file need not be changed.

Otherwise, its fine.

[piman, 2014-05-02 00:37:28]

some nits, otherwise LGTM.

https://codereview.chromium.org/92703003/diff/360001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/92703003/diff/360001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1780: float yuv_to_rgb_rec601[9] = {
On 2014/05/01 23:25:09, rileya wrote:
> Is there a header these might be better off being moved to?

I don't think one exists. We could create one, possibly in a follow-up.

https://codereview.chromium.org/92703003/diff/360001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1803: float* yuv_adjust;
nit: initialize both.

https://codereview.chromium.org/92703003/diff/360001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1814: default:
nit: skip the default case, relying instead on compiler warnings, so that if one
adds an enum case they'll know they need to change this code.

https://codereview.chromium.org/92703003/diff/360001/cc/quads/yuv_video_draw_...
File cc/quads/yuv_video_draw_quad.h (right):

https://codereview.chromium.org/92703003/diff/360001/cc/quads/yuv_video_draw_...
cc/quads/yuv_video_draw_quad.h:21: };
nit: we use ALL_CAPS style for enum values in chromium, for consistency.

[fbarchard, 2014-05-02 02:39:28]

https://codereview.chromium.org/92703003/diff/360001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/92703003/diff/360001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1784: 1.f, 1.f, 1.f, 0.0f, -.34414f, 1.772f, 1.402f,
-.71414, 0.0f,
FYI I found fourcc sometimes inconsistent with other sources of format
conversion - in this case i would trust the jpeg source code more.  I dont have
that handy, so here is what Intel IPP says, and it matches your code:
Y = 0.299*R + 0.587*G + 0.114*B
Cb = -0.16874*R - 0.33126*G + 0.5*B + 128
Cr = 0.5*R - 0.41869*G - 0.08131*B + 128
R = Y + 1.402*Cr - 179,456
G = Y - 0.34414*Cb - 0.71414*Cr + 135.45984
B = Y + 1.772*Cb - 226.816

[rileya (GONE FROM CHROMIUM), 2014-05-02 21:40:10]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-05-02 21:41:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/92703003/380001

[commit-bot: I haz the power, 2014-05-02 21:49:08]

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

[commit-bot: I haz the power, 2014-05-02 21:49:09]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium

[rileya (GONE FROM CHROMIUM), 2014-05-02 21:59:06]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-05-02 21:59:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/92703003/400001

[rileya (GONE FROM CHROMIUM), 2014-05-02 22:45:24]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-05-02 22:45:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/92703003/420001

[commit-bot: I haz the power, 2014-05-02 22:56:58]

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

[commit-bot: I haz the power, 2014-05-02 22:56:59]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium

[rileya1, 2014-05-02 23:12:21]

The CQ bit was checked by rileya@google.com

[commit-bot: I haz the power, 2014-05-02 23:13:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/92703003/420001

[commit-bot: I haz the power, 2014-05-02 23:54:08]

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

[commit-bot: I haz the power, 2014-05-02 23:54:08]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[rileya1, 2014-05-02 23:56:45]

The CQ bit was checked by rileya@google.com

[commit-bot: I haz the power, 2014-05-03 00:00:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rileya@chromium.org/92703003/420001

[commit-bot: I haz the power, 2014-05-03 02:26:02]

Change committed as 268010