Add support for Rec709 color space videos in software YUV convert path.

Add support for Rec709 color space videos in software YUV convert path.

BUG=333619
Committed: https://crrev.com/4e30d62669c312dd6788ba23376b1e8181be0a0c
Cr-Commit-Position: refs/heads/master@{#312454}

[rileya (GONE FROM CHROMIUM), 2014-09-29 18:15:13]

rileya@chromium.org changed reviewers:
+ scherkus@chromium.org

[rileya (GONE FROM CHROMIUM), 2014-09-29 18:15:14]

Not quite sure about this, it's probably going to be best in the long run to
introduce a color space/range member to VideoFrame (someday we'll want to
support Rec2020, etc...), but that'll be a bit of a yak shave that I'm not sure
I have the bandwidth for currently. Thoughts?

https://codereview.chromium.org/591313008/diff/1/media/base/simd/yuv_to_rgb_t...
File media/base/simd/yuv_to_rgb_table.cc (right):

https://codereview.chromium.org/591313008/diff/1/media/base/simd/yuv_to_rgb_t...
media/base/simd/yuv_to_rgb_table.cc:17: #define
DEFINE_YUV_CONVERSION_TABLE(TABLE_NAME)\
yo dawg, I heard you like macros...

Not sure how I feel about nesting macros like this, but it saves a bunch of
duplicate code...

https://codereview.chromium.org/591313008/diff/1/media/base/simd/yuv_to_rgb_t...
media/base/simd/yuv_to_rgb_table.cc:428: static_cast<int16>(2.12798 * 64 * (i -
128) + 0.5), \
I don't think I have the numbers here quite right yet, documentation online is a
bit scattered. This will need to be sorted out before this lands.

https://codereview.chromium.org/591313008/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/591313008/diff/1/media/base/video_frame.h#new...
media/base/video_frame.h:66: YV12HD = 10,  // Rec709 "HD" color space version of
YV12
Is it worth making this another one-off, or is it about time to give VideoFrame
separate color space (maybe color range too) enums?

[scherkus (not reviewing), 2014-10-01 16:39:34]

sorry -- this fell off my radar

we should also add a YUV test in content_browsertests

https://codereview.chromium.org/591313008/diff/1/media/base/simd/yuv_to_rgb_t...
File media/base/simd/yuv_to_rgb_table.cc (right):

https://codereview.chromium.org/591313008/diff/1/media/base/simd/yuv_to_rgb_t...
media/base/simd/yuv_to_rgb_table.cc:17: #define
DEFINE_YUV_CONVERSION_TABLE(TABLE_NAME)\
On 2014/09/29 18:15:14, rileya wrote:
> yo dawg, I heard you like macros...
> 
> Not sure how I feel about nesting macros like this, but it saves a bunch of
> duplicate code...

it's fine, although we typically have a space before the \


if you ever get really bored this is a prime candidate for having python
generate the code considering this macro counts from 0x00 to 0xFF 4 separate
times (seems like a good candidate for two for loops!)

https://codereview.chromium.org/591313008/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/591313008/diff/1/media/base/video_frame.h#new...
media/base/video_frame.h:66: YV12HD = 10,  // Rec709 "HD" color space version of
YV12
On 2014/09/29 18:15:14, rileya wrote:
> Is it worth making this another one-off, or is it about time to give
VideoFrame
> separate color space (maybe color range too) enums?

I'm OK with this for now.

[rileya (GONE FROM CHROMIUM), 2014-12-13 01:36:27]

Add test

[rileya (GONE FROM CHROMIUM), 2014-12-13 01:41:09]

Clean up some bad rebasing...

[rileya (GONE FROM CHROMIUM), 2014-12-13 01:48:45]

rileya@chromium.org changed reviewers:
+ danakj@chromium.org

[rileya (GONE FROM CHROMIUM), 2014-12-13 01:48:46]

Bringing this CL back to life, rebased and added a test. PTAL.

+danakj: please have a look at cc/resources/video_resource_updater.cc

[danakj, 2014-12-15 16:44:37]

cc/ LGTM

[DaleCurtis, 2014-12-15 18:47:33]

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

[DaleCurtis, 2014-12-15 18:47:34]

https://codereview.chromium.org/591313008/diff/60001/media/base/simd/yuv_to_r...
File media/base/simd/yuv_to_rgb_table.cc (right):

https://codereview.chromium.org/591313008/diff/60001/media/base/simd/yuv_to_r...
media/base/simd/yuv_to_rgb_table.cc:404: // Rec709 color space version:
Are you able to provide some sources for these values?

https://codereview.chromium.org/591313008/diff/60001/media/base/simd/yuv_to_r...
media/base/simd/yuv_to_rgb_table.cc:459:
DEFINE_YUV_CONVERSION_TABLE(kCoefficientsRgbY_Rec709);
For anyone else following along, each one of these tables is ~8k which results
in ~24k total space for these 3 tables.  We should probably be generating these
in InitializeCPUSpecificYUVConversions().

https://codereview.chromium.org/591313008/diff/60001/media/ffmpeg/ffmpeg_comm...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/591313008/diff/60001/media/ffmpeg/ffmpeg_comm...
media/ffmpeg/ffmpeg_common.cc:405: // TODO(tomfinegan): libavcodec doesn't know
about VP9.
Can you check with fgalligan@ to see if VP9 needs YV12HD color spaces?

https://codereview.chromium.org/591313008/diff/60001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/591313008/diff/60001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:276: // TODO(rileya): Add support for
HD color space in Skia's YUV conversions.
Doesn't this just need the right matrix below? Without this what happens to
canvas?

[rileya (GONE FROM CHROMIUM), 2014-12-16 22:05:59]

https://codereview.chromium.org/591313008/diff/60001/media/base/simd/yuv_to_r...
File media/base/simd/yuv_to_rgb_table.cc (right):

https://codereview.chromium.org/591313008/diff/60001/media/base/simd/yuv_to_r...
media/base/simd/yuv_to_rgb_table.cc:404: // Rec709 color space version:
On 2014/12/15 18:47:34, DaleCurtis wrote:
> Are you able to provide some sources for these values? 

These values were actually wrong, corrected and added a source
(http://www.equasys.de/colorconversion.html)

https://codereview.chromium.org/591313008/diff/60001/media/base/simd/yuv_to_r...
media/base/simd/yuv_to_rgb_table.cc:459:
DEFINE_YUV_CONVERSION_TABLE(kCoefficientsRgbY_Rec709);
On 2014/12/15 18:47:34, DaleCurtis wrote:
> For anyone else following along, each one of these tables is ~8k which results
> in ~24k total space for these 3 tables.  We should probably be generating
these
> in InitializeCPUSpecificYUVConversions().

I'll look into doing this, do we have a way of guaranteeing that they get
allocated in a SIMD-friendly alignment if we're heap-allocating?

https://codereview.chromium.org/591313008/diff/60001/media/ffmpeg/ffmpeg_comm...
File media/ffmpeg/ffmpeg_common.cc (right):

https://codereview.chromium.org/591313008/diff/60001/media/ffmpeg/ffmpeg_comm...
media/ffmpeg/ffmpeg_common.cc:405: // TODO(tomfinegan): libavcodec doesn't know
about VP9.
On 2014/12/15 18:47:34, DaleCurtis wrote:
> Can you check with fgalligan@ to see if VP9 needs YV12HD color spaces?

Will do.

https://codereview.chromium.org/591313008/diff/60001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/591313008/diff/60001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:276: // TODO(rileya): Add support for
HD color space in Skia's YUV conversions.
On 2014/12/15 18:47:34, DaleCurtis wrote:
> Doesn't this just need the right matrix below? Without this what happens to
> canvas?

Skia needs the 709 matrix to get plumbed into its YUV convert shader, that's an
easy change, but one that requires skia changes to happen and roll in.

Without this, we just fall back to our usual SW YUV convert. This path is purely
an optimization, where if the destination canvas is accelerated, skia can take
YUV data directly, instead of asking us for RGB.

[DaleCurtis, 2014-12-16 22:20:44]

+watk: Can you patch this in to your hw decode fallback and see if the colors
match more closely?

lgtm % whatever Frank says about vp9 support, but that's something that can be
done in a follow up patch.

https://codereview.chromium.org/591313008/diff/60001/media/base/simd/yuv_to_r...
File media/base/simd/yuv_to_rgb_table.cc (right):

https://codereview.chromium.org/591313008/diff/60001/media/base/simd/yuv_to_r...
media/base/simd/yuv_to_rgb_table.cc:459:
DEFINE_YUV_CONVERSION_TABLE(kCoefficientsRgbY_Rec709);
On 2014/12/16 22:05:59, rileya wrote:
> On 2014/12/15 18:47:34, DaleCurtis wrote:
> > For anyone else following along, each one of these tables is ~8k which
results
> > in ~24k total space for these 3 tables.  We should probably be generating
> these
> > in InitializeCPUSpecificYUVConversions().
> 
> I'll look into doing this, do we have a way of guaranteeing that they get
> allocated in a SIMD-friendly alignment if we're heap-allocating?

Yes, see base::AlignedAlloc() 

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/aligne...

https://codereview.chromium.org/591313008/diff/80001/media/base/simd/yuv_to_r...
File media/base/simd/yuv_to_rgb_table.cc (right):

https://codereview.chromium.org/591313008/diff/80001/media/base/simd/yuv_to_r...
media/base/simd/yuv_to_rgb_table.cc:454: 
Extra space.

[scherkus (not reviewing), 2014-12-16 22:32:31]

scherkus@chromium.org changed reviewers:
- scherkus@chromium.org

[scherkus (not reviewing), 2014-12-16 22:32:32]

[ removing myself from reviewers as Mr. Curtis has this on lockdown ]

[watk, 2014-12-17 21:10:22]

On 2014/12/16 22:20:44, DaleCurtis wrote:
> +watk: Can you patch this in to your hw decode fallback and see if the colors
> match more closely?

This doesn't seem to make a difference.

[watk, 2014-12-17 21:21:41]

On 2014/12/17 21:10:22, watk wrote:
> On 2014/12/16 22:20:44, DaleCurtis wrote:
> > +watk: Can you patch this in to your hw decode fallback and see if the
colors
> > match more closely?
> 
> This doesn't seem to make a difference.

Talked with Dale offline about why. We don't parse the .colr box for MSE
streams. It also looks like gpu video decoder just uses a heuristic always color
convert for 720p content.

[rileya (GONE FROM CHROMIUM), 2015-01-07 21:37:45]

dalecurtis@, mind having a quick peek again? I replaced the static YUV->RGB
lookup tables with heap-allocated ones that are populated at the same time as
the function pointers for the CPU-specific convert fns.

I assume it's okay to just let the tables leak? Or is there someplace they
should be cleaned up?

[DaleCurtis, 2015-01-08 01:36:57]

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert....
media/base/yuv_convert.cc:115: static const int16* g_table_rec601 = 0;
NULL here and below.

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert....
media/base/yuv_convert.cc:169: int16* table =
static_cast<int16*>(base::AlignedAlloc(
You'll either need to use a LazyInstance::Leaky which contains
base::AlignedMemory<16, #size> member variables and point the global variables
at that or modify the valgrind/memcheck/lsan suppressions to let them know this
is an intentional leak.

If you go the suppression route you can submit this CL as is to the lsan,
drmemory, and valgrind bots to figure out what suppression to use and then add
them to the appropriate files under tools/valgrind as well as adding
ANNOTATE_LEAKING_OBJECT_PTR() for the allocations to keep lsan happy.

Using a LazyInstance::Leaky already has all of that suppressed.

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert....
media/base/yuv_convert.cc:270: static const double kRec601ConvertMatrix[3][3] =
{
const is fine for all of these. Typically we want to avoid using static const
within function scope;

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert....
media/base/yuv_convert.cc:463: GetLookupTable(yuv_type));
Call this one outside of the loops? Ditto for all the other users.

https://codereview.chromium.org/591313008/diff/240001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/591313008/diff/240001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:271: frame_->format() ==
VideoFrame::YV12HD) {
Is this a TODO?

[rileya (GONE FROM CHROMIUM), 2015-01-12 21:46:21]

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert....
media/base/yuv_convert.cc:115: static const int16* g_table_rec601 = 0;
On 2015/01/08 01:36:57, DaleCurtis wrote:
> NULL here and below.

I made each table a lazy instance, so no longer relevant.

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert....
media/base/yuv_convert.cc:169: int16* table =
static_cast<int16*>(base::AlignedAlloc(
On 2015/01/08 01:36:57, DaleCurtis wrote:
> You'll either need to use a LazyInstance::Leaky which contains
> base::AlignedMemory<16, #size> member variables and point the global variables
> at that or modify the valgrind/memcheck/lsan suppressions to let them know
this
> is an intentional leak.
> 
> If you go the suppression route you can submit this CL as is to the lsan,
> drmemory, and valgrind bots to figure out what suppression to use and then add
> them to the appropriate files under tools/valgrind as well as adding
> ANNOTATE_LEAKING_OBJECT_PTR() for the allocations to keep lsan happy.
> 
> Using a LazyInstance::Leaky already has all of that suppressed.

I made each table a LazyInstance::Leaky of a struct that wraps over
base::AlignedMemory (it lacks a new() operator, so LazyInstance doesn't accept
it as a template arg). Does this look about right?

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert....
media/base/yuv_convert.cc:270: static const double kRec601ConvertMatrix[3][3] =
{
On 2015/01/08 01:36:57, DaleCurtis wrote:
> const is fine for all of these. Typically we want to avoid using static const
> within function scope;

Done.

https://codereview.chromium.org/591313008/diff/240001/media/base/yuv_convert....
media/base/yuv_convert.cc:463: GetLookupTable(yuv_type));
On 2015/01/08 01:36:57, DaleCurtis wrote:
> Call this one outside of the loops? Ditto for all the other users.

Done.

https://codereview.chromium.org/591313008/diff/240001/media/filters/skcanvas_...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/591313008/diff/240001/media/filters/skcanvas_...
media/filters/skcanvas_video_renderer.cc:271: frame_->format() ==
VideoFrame::YV12HD) {
On 2015/01/08 01:36:57, DaleCurtis wrote:
> Is this a TODO?

Yep! Lost it somewhere in the rebase-ing process... Fixed.

[DaleCurtis, 2015-01-12 22:00:01]

lgtm

https://codereview.chromium.org/591313008/diff/260001/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/591313008/diff/260001/media/base/yuv_convert....
media/base/yuv_convert.cc:157: return
g_table_rec601.Get().table.data_as<int16>();
Each .Get() requires some atomic operations, so you could initialize some raw
pointers to .Get().table.data_as<int16>() instead post-initialization if you're
so inclined.

[rileya (GONE FROM CHROMIUM), 2015-01-20 23:23:04]

https://codereview.chromium.org/591313008/diff/260001/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

https://codereview.chromium.org/591313008/diff/260001/media/base/yuv_convert....
media/base/yuv_convert.cc:157: return
g_table_rec601.Get().table.data_as<int16>();
On 2015/01/12 22:00:01, DaleCurtis wrote:
> Each .Get() requires some atomic operations, so you could initialize some raw
> pointers to .Get().table.data_as<int16>() instead post-initialization if
you're
> so inclined.

Done.

[DaleCurtis, 2015-01-20 23:26:50]

lgtm

[rileya (GONE FROM CHROMIUM), 2015-01-21 00:19:18]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2015-01-21 00:19:58]

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

[rileya (GONE FROM CHROMIUM), 2015-01-21 00:29:56]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2015-01-21 00:31:04]

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

[commit-bot: I haz the power, 2015-01-21 00:43:15]

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

[commit-bot: I haz the power, 2015-01-21 00:43:16]

Try jobs failed on following builders:
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[rileya (GONE FROM CHROMIUM), 2015-01-21 01:24:52]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2015-01-21 01:26:14]

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

[rileya (GONE FROM CHROMIUM), 2015-01-21 01:39:42]

The CQ bit was checked by rileya@chromium.org

[rileya (GONE FROM CHROMIUM), 2015-01-21 01:40:09]

The CQ bit was unchecked by rileya@chromium.org

[rileya (GONE FROM CHROMIUM), 2015-01-21 18:36:25]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2015-01-21 18:37:58]

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

[commit-bot: I haz the power, 2015-01-21 18:51:42]

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

[commit-bot: I haz the power, 2015-01-21 18:51:44]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[rileya (GONE FROM CHROMIUM), 2015-01-21 19:07:05]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2015-01-21 19:08:17]

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

[commit-bot: I haz the power, 2015-01-21 19:19:14]

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

[commit-bot: I haz the power, 2015-01-21 19:19:15]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rileya (GONE FROM CHROMIUM), 2015-01-21 20:05:32]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2015-01-21 20:07:42]

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

[commit-bot: I haz the power, 2015-01-21 21:18:08]

Committed patchset #24 (id:430001)

[commit-bot: I haz the power, 2015-01-21 21:19:13]

Patchset 24 (id:??) landed as
https://crrev.com/4e30d62669c312dd6788ba23376b1e8181be0a0c
Cr-Commit-Position: refs/heads/master@{#312454}