Add color metadata info to VideoDecoderConfig.

ui/gfx/color_space.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef UI_GFX_COLOR_SPACE_H_
 #define UI_GFX_COLOR_SPACE_H_
 
 #include <stdint.h>
 
 #include "base/gtest_prod_util.h"
@@ skipping 98 matching lines @@
     BT2020_CL = 10,
     YDZDX = 11,
 
     LAST_STANDARD_VALUE = YDZDX,
 
     // Chrome-specific values start at 1000
     UNKNOWN = 1000,
     LAST = UNKNOWN,
   };
 
-  // The h264 spec declares this as bool, so only the the first two values
-  // correspond to the h264 spec. Chrome-specific values can start at 2.
+  // The h264 spec declares this as bool, so only the the LIMITED/FULL values
+  // correspond to the h264 spec. Chrome-specific values can start at 3.

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

Doesn't seem to be true. 0 is chrome-specific too.

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

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]

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).

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

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?

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

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

   // We use an enum instead of a bool becuase different bit depths may have
   // different definitions of what "limited" means.
   enum class RangeID : int8_t {
-    FULL = 0,
+    UNSPECIFIED = 0,
     LIMITED = 1,
+    FULL = 2,
 
-    LAST = LIMITED
+    // Defined by TransferID/MatrixID

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

please use punctuation for comments, they are sentences.

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

Done.

+    DERIVED = 3,
+
+    LAST = DERIVED
   };
 
   ColorSpace();
   ColorSpace(PrimaryID primaries,
              TransferID transfer,
              MatrixID matrix,
              RangeID full_range);
   ColorSpace(int primaries, int transfer, int matrix, RangeID full_range);
 
   static PrimaryID PrimaryIDFromInt(int primary_id);
@@ skipping 30 matching lines @@
 
   friend class ICCProfile;
   friend class ColorSpaceToColorSpaceTransform;
   friend struct IPC::ParamTraits<gfx::ColorSpace>;
   FRIEND_TEST_ALL_PREFIXES(SimpleColorSpace, GetColorSpace);
 };
 
 }  // namespace gfx
 
 #endif  // UI_GFX_COLOR_SPACE_H_