Add color metadata info to VideoDecoderConfig.

media/base/video_decoder_config.h

Index: media/base/video_decoder_config.h
diff --git a/media/base/video_decoder_config.h b/media/base/video_decoder_config.h
index 50dee06534c3cab8c22ed0b208d69a8f3412e270..5e2122cc36bf3951e9bc8619c2f216b396a51fa0 100644
--- a/media/base/video_decoder_config.h
+++ b/media/base/video_decoder_config.h
@@ -11,10 +11,13 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "media/base/encryption_scheme.h"
+#include "media/base/hdr_metadata.h"
 #include "media/base/media_export.h"
 #include "media/base/video_codecs.h"
 #include "media/base/video_types.h"
+#include "ui/gfx/color_space.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 
@@ -95,7 +98,7 @@ class MEDIA_EXPORT VideoDecoderConfig {
   gfx::Size natural_size() const { return natural_size_; }
 
   // Optional byte data required to initialize video decoders, such as H.264
-  // AAVC data.
+  // AVCC data.
   const std::vector<uint8_t>& extra_data() const { return extra_data_; }
 
   // Whether the video stream is potentially encrypted.
@@ -108,11 +111,19 @@ class MEDIA_EXPORT VideoDecoderConfig {
     return encryption_scheme_;
   }
 
+  void set_color_space_info(const gfx::ColorSpace& color_space_info);
+  base::Optional<gfx::ColorSpace> color_space_info() const;
+
+  void set_hdr_metadata(const HDRMetadata& hdr_metadata);
+  base::Optional<HDRMetadata> hdr_metadata() const;
+
  private:
   VideoCodec codec_;
   VideoCodecProfile profile_;
 
   VideoPixelFormat format_;
+
+  // TODO(servolk): Deprecated, use color_space_info_ instead.
   ColorSpace color_space_;
 
   gfx::Size coded_size_;
@@ -123,6 +134,9 @@ class MEDIA_EXPORT VideoDecoderConfig {
 
   EncryptionScheme encryption_scheme_;
 
+  base::Optional<gfx::ColorSpace> color_space_info_;

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

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:03]

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

[hubbe, 2016/09/15 20:42:53]

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.

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

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.

+  base::Optional<HDRMetadata> hdr_metadata_;
+
   // Not using DISALLOW_COPY_AND_ASSIGN here intentionally to allow the compiler
   // generated copy constructor and assignment operator. Since the extra data is
   // typically small, the performance impact is minimal.