Add color metadata info to VideoDecoderConfig.

Add color metadata info to VideoDecoderConfig.

Also implemented reading color metadata from the Colour element in WebM containers.

BUG=645626
Committed: https://crrev.com/e21ae6e132cd646d0fc8f3aeda071e0d2e5bc4f3
Cr-Commit-Position: refs/heads/master@{#420511}

[servolk, 2016-09-12 17:34:15]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 17:35:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 22:30:11]

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

[commit-bot: I haz the power, 2016-09-12 22:30:11]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-09-13 00:35:57]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 00:36:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 00:50:40]

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

[commit-bot: I haz the power, 2016-09-13 00:50:40]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[servolk, 2016-09-13 02:15:59]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 02:16:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-09-13 02:18:08]

Description was changed from

==========
Implement parsing WebM Colour element

BUG=645626
==========

to

==========
Implement parsing WebM Colour element

Also implemented reading color metadata from the Colour element in WebM
containers.

BUG=645626
==========

[servolk, 2016-09-13 02:18:08]

servolk@chromium.org changed reviewers:
+ erickung@chromium.org, halliwell@chromium.org, lcwu@chromium.org

[commit-bot: I haz the power, 2016-09-13 04:36:13]

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

[commit-bot: I haz the power, 2016-09-13 04:36:14]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-09-13 22:08:35]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 22:09:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-09-13 22:10:19]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 22:11:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 22:21:50]

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

[commit-bot: I haz the power, 2016-09-13 22:21:50]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[servolk, 2016-09-13 22:59:08]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 22:59:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-09-13 23:00:04]

Description was changed from

==========
Implement parsing WebM Colour element

Also implemented reading color metadata from the Colour element in WebM
containers.

BUG=645626
==========

to

==========
Add color metadata info to VideoDecoderConfig.

Also implemented reading color metadata from the Colour element in WebM
containers.

BUG=645626
==========

[servolk, 2016-09-13 23:00:26]

servolk@chromium.org changed reviewers:
+ dalecurtis@chromium.org, hubbe@chromium.org

[commit-bot: I haz the power, 2016-09-13 23:34:13]

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

[commit-bot: I haz the power, 2016-09-13 23:34:14]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[servolk, 2016-09-13 23:58:09]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 23:58:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-09-14 00:04:23]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 00:04:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 01:54:04]

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

[commit-bot: I haz the power, 2016-09-14 01:54:05]

Dry run: This issue passed the CQ dry run.

[hubbe, 2016-09-14 17:34:17]

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadata.h
File media/base/hdr_metadata.h (right):

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
media/base/hdr_metadata.h:24: enum class MatrixCoefficients : std::uint64_t {
Plase use gfx::ColorSpace

[halliwell, 2016-09-14 18:58:41]

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
File media/base/hdr_metadata.cc (right):

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
media/base/hdr_metadata.cc:7: #include "base/logging.h"
nit, unnecessary

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadata.h
File media/base/hdr_metadata.h (right):

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
media/base/hdr_metadata.h:260: float PrimaryRChromaticityX = 0;
nit: here and below, not following standard for member naming.

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
media/base/hdr_metadata.h:287: struct MEDIA_EXPORT ColorMetadata {
nit, shouldn't the file be called color_metadata?  as this seems like the
top-level struct here.

https://codereview.chromium.org/2333663003/diff/100001/media/formats/webm/web...
File media/formats/webm/webm_colour_parser.cc (right):

https://codereview.chromium.org/2333663003/diff/100001/media/formats/webm/web...
media/formats/webm/webm_colour_parser.cc:158: if (chroma_subsampling_horz_ !=
-1)
is it expected that this data could be missing?  Looks like the defaults could
be strange if so ... what would a decoder be expected to do with that?

[servolk, 2016-09-14 22:34:35]

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
File media/base/hdr_metadata.cc (right):

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
media/base/hdr_metadata.cc:7: #include "base/logging.h"
On 2016/09/14 18:58:40, halliwell wrote:
> nit, unnecessary

Done.

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadata.h
File media/base/hdr_metadata.h (right):

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
media/base/hdr_metadata.h:24: enum class MatrixCoefficients : std::uint64_t {
On 2016/09/14 17:34:16, hubbe wrote:
> Plase use gfx::ColorSpace

Ah, ok, thank for the pointer I didn't know about gfx::ColorSpace enums. I'll
switch to using gfx::ColorSpace.
Btw, I have noticed that while most of the values can be mapped directly, there
are some missing values. E.g. I don't see any gfx::ColorSpace::TransferID value
corresponding to ARIB STD B67 transfer function (aka hybrid-log gamma, HLG) we
should probably add a new value for this.
Also atm there are only two possible values for gfx::ColorSpace::Range enum,
while there are 4 values in the WebM definitions. This one is less clear to me -
should we also add the UNDEFINED and DERIVED values to gfx::ColorSpace::Range,
or remap somehow?

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
media/base/hdr_metadata.h:287: struct MEDIA_EXPORT ColorMetadata {
On 2016/09/14 18:58:40, halliwell wrote:
> nit, shouldn't the file be called color_metadata?  as this seems like the
> top-level struct here.

I'm actually not sure about it yes, since this ColorMetadata in WebM specific
and probably doesn't belong here. Now that hubbe@ pointed out that we already
have similar enums defined in gfx::ColorSpace I'm planning to move this
definition back to WebM parser, which is probably a better place for it.
HDRMetadata (i.e. SMPTE 2086 MasteringMetadata + MaxCLL + MaxFALL) seems to be
generic enough to stay here, since it can be shared between WebM HDR and HDR10.

https://codereview.chromium.org/2333663003/diff/100001/media/formats/webm/web...
File media/formats/webm/webm_colour_parser.cc (right):

https://codereview.chromium.org/2333663003/diff/100001/media/formats/webm/web...
media/formats/webm/webm_colour_parser.cc:158: if (chroma_subsampling_horz_ !=
-1)
On 2016/09/14 18:58:40, halliwell wrote:
> is it expected that this data could be missing?  Looks like the defaults could
> be strange if so ... what would a decoder be expected to do with that?

No, it's not expected to be missing, but this read from the input stream, so I
guess it could be missing anyway, e.g. a malformed/non-compliant stream. So I
figured it'd be better to not fail in this case and try to decode with default
subsampling assumptions. It might look slight worse, but it should still be
watchable.

[hubbe, 2016-09-14 22:41:25]

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadata.h
File media/base/hdr_metadata.h (right):

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
media/base/hdr_metadata.h:24: enum class MatrixCoefficients : std::uint64_t {
On 2016/09/14 22:34:35, servolk wrote:
> On 2016/09/14 17:34:16, hubbe wrote:
> > Plase use gfx::ColorSpace
> 
> Ah, ok, thank for the pointer I didn't know about gfx::ColorSpace enums. I'll
> switch to using gfx::ColorSpace.
> Btw, I have noticed that while most of the values can be mapped directly,
there
> are some missing values. E.g. I don't see any gfx::ColorSpace::TransferID
value
> corresponding to ARIB STD B67 transfer function (aka hybrid-log gamma, HLG) we
> should probably add a new value for this.
> Also atm there are only two possible values for gfx::ColorSpace::Range enum,
> while there are 4 values in the WebM definitions. This one is less clear to me
-
> should we also add the UNDEFINED and DERIVED values to gfx::ColorSpace::Range,
> or remap somehow?

We made the range an enum specifically so that we can add new values, so please
go ahead and add new values as needed. :)
Just make sure you update gfx::ColorTransform as well.

At some point we'll need to make gfx::ColorTransform aware of HDR mapping as
well, which means that some of the other parameters below will need to be moved
over to gfx:: as well, but one step at a time is fine by me... :)

[servolk, 2016-09-14 23:23:28]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 23:24:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-09-15 01:49:32]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[servolk, 2016-09-15 01:49:42]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 01:49:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-09-15 01:50:03]

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadata.h
File media/base/hdr_metadata.h (right):

https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
media/base/hdr_metadata.h:260: float PrimaryRChromaticityX = 0;
On 2016/09/14 18:58:40, halliwell wrote:
> nit: here and below, not following standard for member naming.

Done.

[servolk, 2016-09-15 01:50:25]

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

[servolk, 2016-09-15 01:53:46]

On 2016/09/14 22:41:25, hubbe wrote:
>
https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadata.h
> File media/base/hdr_metadata.h (right):
> 
>
https://codereview.chromium.org/2333663003/diff/100001/media/base/hdr_metadat...
> media/base/hdr_metadata.h:24: enum class MatrixCoefficients : std::uint64_t {
> On 2016/09/14 22:34:35, servolk wrote:
> > On 2016/09/14 17:34:16, hubbe wrote:
> > > Plase use gfx::ColorSpace
> > 
> > Ah, ok, thank for the pointer I didn't know about gfx::ColorSpace enums.
I'll
> > switch to using gfx::ColorSpace.
> > Btw, I have noticed that while most of the values can be mapped directly,
> there
> > are some missing values. E.g. I don't see any gfx::ColorSpace::TransferID
> value
> > corresponding to ARIB STD B67 transfer function (aka hybrid-log gamma, HLG)
we
> > should probably add a new value for this.
> > Also atm there are only two possible values for gfx::ColorSpace::Range enum,
> > while there are 4 values in the WebM definitions. This one is less clear to
me
> -
> > should we also add the UNDEFINED and DERIVED values to
gfx::ColorSpace::Range,
> > or remap somehow?
> 
> We made the range an enum specifically so that we can add new values, so
please
> go ahead and add new values as needed. :)
> Just make sure you update gfx::ColorTransform as well.
> 
> At some point we'll need to make gfx::ColorTransform aware of HDR mapping as
> well, which means that some of the other parameters below will need to be
moved
> over to gfx:: as well, but one step at a time is fine by me... :)

Ok, I've added ARIB_STD_B67 in the TransferID enum and a couple of values in the
RangeID to match what we have in WebM. I've also reorganized the data structures
slightly to keep HDR metadata separate from the color space info. Also we should
probably deprecate the old VideoDecoderConfig::color_space enum in favor of the
new gfx::ColorSpace member, the old color_space is currently used in a few
different places and it's not immediately clear to me how to replace all those
places, I guess it can be done in a separate CL.

[commit-bot: I haz the power, 2016-09-15 04:50:21]

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

[commit-bot: I haz the power, 2016-09-15 04:50:22]

Dry run: This issue passed the CQ dry run.

[hubbe, 2016-09-15 17:25:52]

I'm currently in the middle of the work to move from the old colorspace enum to
use gfx::ColorSpace for all video things, including proper color mapping, which
takes monitor ICC profiles into account. It's slow work, but the media
colorspace enum should be gone when I'm done.

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:73: ARIB_STD_B67 = 18,  // AKA hybrid-log gamma, HLG
Also need to add this to ToLinear/FromLinear in color_transform.cc and
associated unit tests.

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_transform...
ui/gfx/color_transform.cc:498: case ColorSpace::RangeID::DERIVED:
Also update color_transform_unittests.cc

[hubbe, 2016-09-15 17:30:10]

https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
media/base/video_decoder_config.h:137: base::Optional<gfx::ColorSpace>
color_space_info_;
I don't think this needs to be optional.
gfx::ColorSpace is meant to be small and lightweight and defaults to UNDEFINED
values. In most places in the code we just use the class directly. (including
media::VideoFrame)

[servolk, 2016-09-15 20:29:04]

https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
media/base/video_decoder_config.h:137: base::Optional<gfx::ColorSpace>
color_space_info_;
On 2016/09/15 17:30:10, hubbe wrote:
> I don't think this needs to be optional.
> gfx::ColorSpace is meant to be small and lightweight and defaults to UNDEFINED
> values. In most places in the code we just use the class directly. (including
> media::VideoFrame)

I've made it optional mostly because I wasn't sure that we'll be able to
initialize this properly in all cases and I assumed that not having color space
info in those cases would be clearer than having UNDEFINED. But I don't have a
strong preference here, so converted to non-optional for now. Perhaps we should
even make it the constructor/Initialize parameter? I didn't do that yet to avoid
confusion, since the old color space enum is already there, we need to get rid
of it first.
Also I guess we could use Rec. 601 for SD video by default and Rec. 709 for HD
video, like we do e.g. at
https://cs.chromium.org/chromium/src/media/ffmpeg/ffmpeg_common.cc?rcl=0&l=516

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:73: ARIB_STD_B67 = 18,  // AKA hybrid-log gamma, HLG
On 2016/09/15 17:25:52, hubbe wrote:
> Also need to add this to ToLinear/FromLinear in color_transform.cc and
> associated unit tests.
> 

Done.
I've found some conversion formulas in Wikipedia
(https://en.wikipedia.org/wiki/Hybrid_Log-Gamma) and at
https://github.com/videovillage/Lattice-Issues/issues/52. Adding Steven to check
these are correct. IIUC we should use the formulas for the values normalized to
0..1 range here, right?
I think I might have done something wrong, since ColorSpaceTests start failing
when I add ARIB_STD_B67 into the all_transfers in color_transform_unittest.cc

PS: I've extracted the changes to add the new ARIB/HLG transfer function in a
separate CL so that it's easier to review and address gfx_unittest failures in
isolation from the rest of this CL:
https://codereview.chromium.org/2342963003/

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_transform.cc
File ui/gfx/color_transform.cc (right):

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_transform...
ui/gfx/color_transform.cc:498: case ColorSpace::RangeID::DERIVED:
On 2016/09/15 17:25:52, hubbe wrote:
> Also update color_transform_unittests.cc

Done.

[hubbe, 2016-09-15 20:42:54]

https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
media/base/video_decoder_config.h:137: base::Optional<gfx::ColorSpace>
color_space_info_;
On 2016/09/15 20:29:03, servolk wrote:
> On 2016/09/15 17:30:10, hubbe wrote:
> > I don't think this needs to be optional.
> > gfx::ColorSpace is meant to be small and lightweight and defaults to
UNDEFINED
> > values. In most places in the code we just use the class directly.
(including
> > media::VideoFrame)
> 
> I've made it optional mostly because I wasn't sure that we'll be able to
> initialize this properly in all cases and I assumed that not having color
space
> info in those cases would be clearer than having UNDEFINED. But I don't have a
> strong preference here, so converted to non-optional for now. Perhaps we
should
> even make it the constructor/Initialize parameter? I didn't do that yet to
avoid
> confusion, since the old color space enum is already there, we need to get rid
> of it first.
> Also I guess we could use Rec. 601 for SD video by default and Rec. 709 for HD
> video, like we do e.g. at
> https://cs.chromium.org/chromium/src/media/ffmpeg/ffmpeg_common.cc?rcl=0&l=516

I'm not actually sure this is what we want to do going forward.
Changing defaults based on resolution can lead to some really ugly things when
the resolution changes dynamically.  My current thinking is that defaulting to
709 in all cases is the right thing to do, and if you want something else, make
sure to tag your content correctly.

[hubbe, 2016-09-15 20:46:12]

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/140001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:73: ARIB_STD_B67 = 18,  // AKA hybrid-log gamma, HLG
On 2016/09/15 20:29:04, servolk wrote:
> On 2016/09/15 17:25:52, hubbe wrote:
> > Also need to add this to ToLinear/FromLinear in color_transform.cc and
> > associated unit tests.
> > 
> 
> Done.
> I've found some conversion formulas in Wikipedia
> (https://en.wikipedia.org/wiki/Hybrid_Log-Gamma) and at
> https://github.com/videovillage/Lattice-Issues/issues/52. Adding Steven to
check
> these are correct. IIUC we should use the formulas for the values normalized
to
> 0..1 range here, right?
> I think I might have done something wrong, since ColorSpaceTests start failing
> when I add ARIB_STD_B67 into the all_transfers in color_transform_unittest.cc
> 
> PS: I've extracted the changes to add the new ARIB/HLG transfer function in a
> separate CL so that it's easier to review and address gfx_unittest failures in
> isolation from the rest of this CL:
> https://codereview.chromium.org/2342963003/

+1 for moving to a separate CL.
The 0-1 normalization is not clear. The specs that I followed for the other
transfer functions doesn't do that in all cases.

[servolk, 2016-09-15 22:28:38]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 22:29:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[servolk, 2016-09-15 23:12:41]

https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
media/base/video_decoder_config.h:137: base::Optional<gfx::ColorSpace>
color_space_info_;
On 2016/09/15 20:42:53, hubbe wrote:
> On 2016/09/15 20:29:03, servolk wrote:
> > On 2016/09/15 17:30:10, hubbe wrote:
> > > I don't think this needs to be optional.
> > > gfx::ColorSpace is meant to be small and lightweight and defaults to
> UNDEFINED
> > > values. In most places in the code we just use the class directly.
> (including
> > > media::VideoFrame)
> > 
> > I've made it optional mostly because I wasn't sure that we'll be able to
> > initialize this properly in all cases and I assumed that not having color
> space
> > info in those cases would be clearer than having UNDEFINED. But I don't have
a
> > strong preference here, so converted to non-optional for now. Perhaps we
> should
> > even make it the constructor/Initialize parameter? I didn't do that yet to
> avoid
> > confusion, since the old color space enum is already there, we need to get
rid
> > of it first.
> > Also I guess we could use Rec. 601 for SD video by default and Rec. 709 for
HD
> > video, like we do e.g. at
> >
https://cs.chromium.org/chromium/src/media/ffmpeg/ffmpeg_common.cc?rcl=0&l=516
> 
> I'm not actually sure this is what we want to do going forward.
> Changing defaults based on resolution can lead to some really ugly things when
> the resolution changes dynamically.  My current thinking is that defaulting to
> 709 in all cases is the right thing to do, and if you want something else,
make
> sure to tag your content correctly.

Ok, I've looked at all code paths that invoke VideoDecoderConfig::Initialize and
found only 4 places (excluding test code) that use actual input values for the
color_space, that are not copied from another VideoDecoderConfig struct:
1. VideoDecoderShim::Initialize
(https://cs.chromium.org/chromium/src/content/renderer/pepper/video_decoder_sh...)
uses COLOR_SPACE_UNSPECIFIED. IIUC this is mostly for Flash video, so Rec. 709
should be ok here.
2. AVStreamToVideoDecoderConfig for src=/FFmpegDemuxer playback uses 601 for SD
and 709 for HD
(https://cs.chromium.org/chromium/src/media/ffmpeg/ffmpeg_common.cc?rcl=147393...).
3,4. WebM parser and MP4 parser for MSE, both use 709 unconditionally, which is
probably the right choice for MSE, even for lower resolutions:
https://cs.chromium.org/chromium/src/media/formats/webm/webm_video_client.cc?...
https://cs.chromium.org/chromium/src/media/formats/mp4/mp4_stream_parser.cc?r...

So yes, I agree, we probably don't need to select based on resolution here,
since the only place where it makes sense (FFmpegDemuxer) already does that.
I'll remove that code.

[servolk, 2016-09-15 23:13:19]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 23:13:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 01:11:58]

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

[commit-bot: I haz the power, 2016-09-16 01:12:04]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-09-21 20:11:52]

On 2016/09/15 23:12:41, servolk wrote:
>
https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
> File media/base/video_decoder_config.h (right):
> 
>
https://codereview.chromium.org/2333663003/diff/140001/media/base/video_decod...
> media/base/video_decoder_config.h:137: base::Optional<gfx::ColorSpace>
> color_space_info_;
> On 2016/09/15 20:42:53, hubbe wrote:
> > On 2016/09/15 20:29:03, servolk wrote:
> > > On 2016/09/15 17:30:10, hubbe wrote:
> > > > I don't think this needs to be optional.
> > > > gfx::ColorSpace is meant to be small and lightweight and defaults to
> > UNDEFINED
> > > > values. In most places in the code we just use the class directly.
> > (including
> > > > media::VideoFrame)
> > > 
> > > I've made it optional mostly because I wasn't sure that we'll be able to
> > > initialize this properly in all cases and I assumed that not having color
> > space
> > > info in those cases would be clearer than having UNDEFINED. But I don't
have
> a
> > > strong preference here, so converted to non-optional for now. Perhaps we
> > should
> > > even make it the constructor/Initialize parameter? I didn't do that yet to
> > avoid
> > > confusion, since the old color space enum is already there, we need to get
> rid
> > > of it first.
> > > Also I guess we could use Rec. 601 for SD video by default and Rec. 709
for
> HD
> > > video, like we do e.g. at
> > >
> https://cs.chromium.org/chromium/src/media/ffmpeg/ffmpeg_common.cc?rcl=0&l=516
> > 
> > I'm not actually sure this is what we want to do going forward.
> > Changing defaults based on resolution can lead to some really ugly things
when
> > the resolution changes dynamically.  My current thinking is that defaulting
to
> > 709 in all cases is the right thing to do, and if you want something else,
> make
> > sure to tag your content correctly.
> 
> Ok, I've looked at all code paths that invoke VideoDecoderConfig::Initialize
and
> found only 4 places (excluding test code) that use actual input values for the
> color_space, that are not copied from another VideoDecoderConfig struct:
> 1. VideoDecoderShim::Initialize
>
(https://cs.chromium.org/chromium/src/content/renderer/pepper/video_decoder_sh...)
> uses COLOR_SPACE_UNSPECIFIED. IIUC this is mostly for Flash video, so Rec. 709
> should be ok here.
> 2. AVStreamToVideoDecoderConfig for src=/FFmpegDemuxer playback uses 601 for
SD
> and 709 for HD
>
(https://cs.chromium.org/chromium/src/media/ffmpeg/ffmpeg_common.cc?rcl=147393...).
> 3,4. WebM parser and MP4 parser for MSE, both use 709 unconditionally, which
is
> probably the right choice for MSE, even for lower resolutions:
>
https://cs.chromium.org/chromium/src/media/formats/webm/webm_video_client.cc?...
>
https://cs.chromium.org/chromium/src/media/formats/mp4/mp4_stream_parser.cc?r...
> 
> So yes, I agree, we probably don't need to select based on resolution here,
> since the only place where it makes sense (FFmpegDemuxer) already does that.
> I'll remove that code.

ping. Any further comments on this CL?
We can separately discuss a somewhat controversial issue of whether
gfx::ColorSpace needs getters (https://codereview.chromium.org/2349923002/), but
I'd like to land this CL in order to be able to access WebM color metadata in
Chromecast.

[hubbe, 2016-09-21 20:25:44]

LGTM, although I have not verified that the parser code actually follows the
standards..

https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_colour_parser.cc (right):

https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
media/formats/webm/webm_colour_parser.cc:298: 
Since these should line up exactly in all cases, it's probably more efficient to
just have a cast in here and verify that the numbers line up with a unit test.
(Same fore primaries and ranges.)

[servolk, 2016-09-21 20:53:22]

https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_colour_parser.cc (right):

https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
media/formats/webm/webm_colour_parser.cc:298: 
On 2016/09/21 20:25:44, hubbe wrote:
> Since these should line up exactly in all cases, it's probably more efficient
to
> just have a cast in here and verify that the numbers line up with a unit test.
> (Same fore primaries and ranges.)

Yeah, I was thinking about whether it's better to use switches or just cast. We
could, probably, just use casts, at least for the lower, 'standard' ranges of
these enums. But I think having switches is useful to ensure those are kept in
sync, since we'll get compile-time warnings when a new valud is added to WebM
enums which is unhandled in those switches - that's actually the primary reason
why I omitted default cases in each switch.
Perhaps it would be a little cleaner to use casts for conversions but add a
number of static_casts to ensure enum values are kept in sync. WDYT?

[hubbe, 2016-09-21 20:59:53]

On 2016/09/21 20:53:22, servolk wrote:
>
https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
> File media/formats/webm/webm_colour_parser.cc (right):
> 
>
https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
> media/formats/webm/webm_colour_parser.cc:298: 
> On 2016/09/21 20:25:44, hubbe wrote:
> > Since these should line up exactly in all cases, it's probably more
efficient
> to
> > just have a cast in here and verify that the numbers line up with a unit
test.
> > (Same fore primaries and ranges.)
> 
> Yeah, I was thinking about whether it's better to use switches or just cast.
We
> could, probably, just use casts, at least for the lower, 'standard' ranges of
> these enums. But I think having switches is useful to ensure those are kept in
> sync, since we'll get compile-time warnings when a new valud is added to WebM
> enums which is unhandled in those switches - that's actually the primary
reason
> why I omitted default cases in each switch.
> Perhaps it would be a little cleaner to use casts for conversions but add a
> number of static_casts to ensure enum values are kept in sync. WDYT?

Assuming that you meant static_assert at the end there, SGTM.

[servolk, 2016-09-21 21:11:27]

https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_colour_parser.cc (right):

https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
media/formats/webm/webm_colour_parser.cc:298: 
On 2016/09/21 20:53:22, servolk wrote:
> On 2016/09/21 20:25:44, hubbe wrote:
> > Since these should line up exactly in all cases, it's probably more
efficient
> to
> > just have a cast in here and verify that the numbers line up with a unit
test.
> > (Same fore primaries and ranges.)
> 
> Yeah, I was thinking about whether it's better to use switches or just cast.
We
> could, probably, just use casts, at least for the lower, 'standard' ranges of
> these enums. But I think having switches is useful to ensure those are kept in
> sync, since we'll get compile-time warnings when a new valud is added to WebM
> enums which is unhandled in those switches - that's actually the primary
reason
> why I omitted default cases in each switch.
> Perhaps it would be a little cleaner to use casts for conversions but add a
> number of static_casts to ensure enum values are kept in sync. WDYT?

Yup, sorry, I meant static_assert. I'll convert this in the next patchset
shortly.

[servolk, 2016-09-21 22:05:06]

https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
File media/formats/webm/webm_colour_parser.cc (right):

https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
media/formats/webm/webm_colour_parser.cc:298: 
On 2016/09/21 21:11:27, servolk wrote:
> On 2016/09/21 20:53:22, servolk wrote:
> > On 2016/09/21 20:25:44, hubbe wrote:
> > > Since these should line up exactly in all cases, it's probably more
> efficient
> > to
> > > just have a cast in here and verify that the numbers line up with a unit
> test.
> > > (Same fore primaries and ranges.)
> > 
> > Yeah, I was thinking about whether it's better to use switches or just cast.
> We
> > could, probably, just use casts, at least for the lower, 'standard' ranges
of
> > these enums. But I think having switches is useful to ensure those are kept
in
> > sync, since we'll get compile-time warnings when a new valud is added to
WebM
> > enums which is unhandled in those switches - that's actually the primary
> reason
> > why I omitted default cases in each switch.
> > Perhaps it would be a little cleaner to use casts for conversions but add a
> > number of static_casts to ensure enum values are kept in sync. WDYT?
> 
> Yup, sorry, I meant static_assert. I'll convert this in the next patchset
> shortly.

Done.

[servolk, 2016-09-21 22:06:53]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 22:07:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 23:54:17]

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

[commit-bot: I haz the power, 2016-09-21 23:54:18]

Dry run: This issue passed the CQ dry run.

[servolk, 2016-09-22 19:18:10]

On 2016/09/21 22:05:06, servolk wrote:
>
https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
> File media/formats/webm/webm_colour_parser.cc (right):
> 
>
https://codereview.chromium.org/2333663003/diff/220001/media/formats/webm/web...
> media/formats/webm/webm_colour_parser.cc:298: 
> On 2016/09/21 21:11:27, servolk wrote:
> > On 2016/09/21 20:53:22, servolk wrote:
> > > On 2016/09/21 20:25:44, hubbe wrote:
> > > > Since these should line up exactly in all cases, it's probably more
> > efficient
> > > to
> > > > just have a cast in here and verify that the numbers line up with a unit
> > test.
> > > > (Same fore primaries and ranges.)
> > > 
> > > Yeah, I was thinking about whether it's better to use switches or just
cast.
> > We
> > > could, probably, just use casts, at least for the lower, 'standard' ranges
> of
> > > these enums. But I think having switches is useful to ensure those are
kept
> in
> > > sync, since we'll get compile-time warnings when a new valud is added to
> WebM
> > > enums which is unhandled in those switches - that's actually the primary
> > reason
> > > why I omitted default cases in each switch.
> > > Perhaps it would be a little cleaner to use casts for conversions but add
a
> > > number of static_casts to ensure enum values are kept in sync. WDYT?
> > 
> > Yup, sorry, I meant static_assert. I'll convert this in the next patchset
> > shortly.
> 
> Done.

danakj@ could you please take a quick look at ui/gfx/ ?

[danakj, 2016-09-22 20:03:49]

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:120: // correspond to the h264 spec. Chrome-specific values
can start at 3.
Doesn't seem to be true. 0 is chrome-specific too.

[servolk, 2016-09-22 20:07:56]

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:120: // correspond to the h264 spec. Chrome-specific values
can start at 3.
On 2016/09/22 20:03:49, danakj wrote:
> Doesn't seem to be true. 0 is chrome-specific too.

I'd say UNSPECIFIED is a special case that is not really Chrome-specific + the
first part of the comment makes it sufficiently clear that only LIMITED/FULL are
used for h264. But I'm open to changing this, if you disagree.

[danakj, 2016-09-22 20:30:03]

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:120: // correspond to the h264 spec. Chrome-specific values
can start at 3.
On 2016/09/22 20:07:56, servolk wrote:
> On 2016/09/22 20:03:49, danakj wrote:
> > Doesn't seem to be true. 0 is chrome-specific too.
> 
> I'd say UNSPECIFIED is a special case that is not really Chrome-specific + the
> first part of the comment makes it sufficiently clear that only LIMITED/FULL
are
> used for h264. But I'm open to changing this, if you disagree.

Id suggest instaed to drop the first 2 sentences of this comment, and instead
put a comment on each of the values. 2 of them will mention being part of the
h264 spec, the others will explain why they exist while not being in the spec
(the comment on 3 does not really explain why it is there, I'm an outsider but I
have no idea what the comment is meant to convey).

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:128: // Defined by TransferID/MatrixID
please use punctuation for comments, they are sentences.

[servolk, 2016-09-22 20:59:47]

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:120: // correspond to the h264 spec. Chrome-specific values
can start at 3.
On 2016/09/22 20:30:03, danakj wrote:
> On 2016/09/22 20:07:56, servolk wrote:
> > On 2016/09/22 20:03:49, danakj wrote:
> > > Doesn't seem to be true. 0 is chrome-specific too.
> > 
> > I'd say UNSPECIFIED is a special case that is not really Chrome-specific +
the
> > first part of the comment makes it sufficiently clear that only LIMITED/FULL
> are
> > used for h264. But I'm open to changing this, if you disagree.
> 
> Id suggest instaed to drop the first 2 sentences of this comment, and instead
> put a comment on each of the values. 2 of them will mention being part of the
> h264 spec, the others will explain why they exist while not being in the spec
> (the comment on 3 does not really explain why it is there, I'm an outsider but
I
> have no idea what the comment is meant to convey).

If feel like the current comment is rightly there, as it explains why we are
using an enum instead of bool for indicating range, even though somebody who is
only familiar with h264 might assume that bool would be enough. So I don't know
if removing it is a good idea. The two values that I'm adding are necessary to
ensure that we can map those values directly to the information that we have in
the WebM container. I agree that probably providing a better explanation for
each enum value in a separate comment is a good idea, but in addition to the
general comment about the whole enum at the top, not as a replacement for it.
hubbe@ WDYT?

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:128: // Defined by TransferID/MatrixID
On 2016/09/22 20:30:03, danakj wrote:
> please use punctuation for comments, they are sentences.

Done.

[servolk, 2016-09-22 21:09:55]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[servolk, 2016-09-22 21:09:56]

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/240001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:120: // correspond to the h264 spec. Chrome-specific values
can start at 3.
On 2016/09/22 20:59:47, servolk wrote:
> On 2016/09/22 20:30:03, danakj wrote:
> > On 2016/09/22 20:07:56, servolk wrote:
> > > On 2016/09/22 20:03:49, danakj wrote:
> > > > Doesn't seem to be true. 0 is chrome-specific too.
> > > 
> > > I'd say UNSPECIFIED is a special case that is not really Chrome-specific +
> the
> > > first part of the comment makes it sufficiently clear that only
LIMITED/FULL
> > are
> > > used for h264. But I'm open to changing this, if you disagree.
> > 
> > Id suggest instaed to drop the first 2 sentences of this comment, and
instead
> > put a comment on each of the values. 2 of them will mention being part of
the
> > h264 spec, the others will explain why they exist while not being in the
spec
> > (the comment on 3 does not really explain why it is there, I'm an outsider
but
> I
> > have no idea what the comment is meant to convey).
> 
> If feel like the current comment is rightly there, as it explains why we are
> using an enum instead of bool for indicating range, even though somebody who
is
> only familiar with h264 might assume that bool would be enough. So I don't
know
> if removing it is a good idea. The two values that I'm adding are necessary to
> ensure that we can map those values directly to the information that we have
in
> the WebM container. I agree that probably providing a better explanation for
> each enum value in a separate comment is a good idea, but in addition to the
> general comment about the whole enum at the top, not as a replacement for it.
> hubbe@ WDYT?

Ok, I've tried to improve comments here in the latest patchset. PTAL.

[danakj, 2016-09-22 21:11:50]

LGTM % hubbe

[servolk, 2016-09-22 21:26:42]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 21:27:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hubbe, 2016-09-22 22:14:18]

https://codereview.chromium.org/2333663003/diff/280001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/280001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:120: // correspond to the h264 spec. Chrome-specific values
can start at 3.
I think this should read something like:

This corresponds to the webm range <link to doc or standard>
h264 only uses a bool, which corresponds to the LIMITED/FULL values.
Chrome-specific values start at 1000.

(Because none of these values are chrome-specific, as they were taken from the
webm standard, right?)

[servolk, 2016-09-22 22:34:34]

The CQ bit was checked by servolk@chromium.org to run a CQ dry run

[servolk, 2016-09-22 22:34:39]

https://codereview.chromium.org/2333663003/diff/280001/ui/gfx/color_space.h
File ui/gfx/color_space.h (right):

https://codereview.chromium.org/2333663003/diff/280001/ui/gfx/color_space.h#n...
ui/gfx/color_space.h:120: // correspond to the h264 spec. Chrome-specific values
can start at 3.
On 2016/09/22 22:14:17, hubbe wrote:
> I think this should read something like:
> 
> This corresponds to the webm range <link to doc or standard>
> h264 only uses a bool, which corresponds to the LIMITED/FULL values.
> Chrome-specific values start at 1000.
> 
> (Because none of these values are chrome-specific, as they were taken from the
> webm standard, right?)
> 

Done.

[commit-bot: I haz the power, 2016-09-22 22:35:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hubbe, 2016-09-22 22:37:57]

lgtm

[servolk, 2016-09-22 22:50:28]

The CQ bit was unchecked by servolk@chromium.org

[servolk, 2016-09-22 22:50:31]

The CQ bit was checked by servolk@chromium.org

[servolk, 2016-09-22 22:50:32]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2333663003/#ps300001
(title: "Commented the RangeID comment as suggested")

[commit-bot: I haz the power, 2016-09-22 22:51:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 00:53:21]

Description was changed from

==========
Add color metadata info to VideoDecoderConfig.

Also implemented reading color metadata from the Colour element in WebM
containers.

BUG=645626
==========

to

==========
Add color metadata info to VideoDecoderConfig.

Also implemented reading color metadata from the Colour element in WebM
containers.

BUG=645626
==========

[commit-bot: I haz the power, 2016-09-23 00:53:22]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-09-23 00:55:11]

Description was changed from

==========
Add color metadata info to VideoDecoderConfig.

Also implemented reading color metadata from the Colour element in WebM
containers.

BUG=645626
==========

to

==========
Add color metadata info to VideoDecoderConfig.

Also implemented reading color metadata from the Colour element in WebM
containers.

BUG=645626

Committed: https://crrev.com/e21ae6e132cd646d0fc8f3aeda071e0d2e5bc4f3
Cr-Commit-Position: refs/heads/master@{#420511}
==========

[commit-bot: I haz the power, 2016-09-23 00:55:13]

Patchset 16 (id:??) landed as
https://crrev.com/e21ae6e132cd646d0fc8f3aeda071e0d2e5bc4f3
Cr-Commit-Position: refs/heads/master@{#420511}