color: Clarify default behaviors

ui/gfx/color_space.cc

Index: ui/gfx/color_space.cc
diff --git a/ui/gfx/color_space.cc b/ui/gfx/color_space.cc
index 0f5a42b601841ca529d42c456df504512c708914..3c7b294e4446ad4e10004ab1a5e16f2d34764909 100644
--- a/ui/gfx/color_space.cc
+++ b/ui/gfx/color_space.cc
@@ -86,7 +86,9 @@ ColorSpace::ColorSpace(const ColorSpace& other)
 ColorSpace::~ColorSpace() = default;
 
 bool ColorSpace::IsValid() const {
-  return *this != gfx::ColorSpace();
+  return primaries_ != PrimaryID::UNSPECIFIED &&
+         transfer_ != TransferID::UNSPECIFIED &&
+         matrix_ != MatrixID::UNSPECIFIED;

[hubbe, 2017/02/15 08:48:56]

What if a video specifies the range but nothing else?

[ccameron, 2017/02/15 09:34:28]

Errr... I meant to put UNKNOWN here.

So, UNSPECIFIED implies "default to rec709" and UNKNOWN implies "default to
sRGB".

 }
 
 // static
@@ -96,6 +98,27 @@ ColorSpace ColorSpace::CreateSRGB() {
 }
 
 // static
+ColorSpace ColorSpace::CreateCustom(const SkMatrix44& to_XYZD50,
+                                    const SkColorSpaceTransferFn& fn) {
+  ColorSpace result(ColorSpace::PrimaryID::CUSTOM,
+                    ColorSpace::TransferID::CUSTOM, ColorSpace::MatrixID::RGB,
+                    ColorSpace::RangeID::FULL);
+  for (int row = 0; row < 3; ++row) {
+    for (int col = 0; col < 3; ++col) {
+      result.custom_primary_matrix_[3 * row + col] = to_XYZD50.get(row, col);
+    }
+  }
+  result.custom_transfer_params_[0] = fn.fA;
+  result.custom_transfer_params_[1] = fn.fB;
+  result.custom_transfer_params_[2] = fn.fC;
+  result.custom_transfer_params_[3] = fn.fD;
+  result.custom_transfer_params_[4] = fn.fE;
+  result.custom_transfer_params_[5] = fn.fF;
+  result.custom_transfer_params_[6] = fn.fG;
+  return result;
+}
+
+// static
 ColorSpace ColorSpace::CreateSCRGBLinear() {
   return ColorSpace(PrimaryID::BT709, TransferID::LINEAR_HDR, MatrixID::RGB,
                     RangeID::FULL);
@@ -197,10 +220,8 @@ sk_sp<SkColorSpace> ColorSpace::ToSkColorSpace() const {
     return icc_profile_sk_color_space_;
 
   // Unspecified color spaces correspond to the null SkColorSpace.
-  if (primaries_ == PrimaryID::UNSPECIFIED ||

[hubbe, 2017/02/15 08:48:56]

Not sure I understand why this was the way it was before, or why it should be
the way it is now...

[ccameron, 2017/02/15 09:34:28]

This is the same as before, just expressed in a way consistent with the rest of
the file (IsValid also checks MatrixID, but that's done below, so the effect is
the same).

-      transfer_ == TransferID::UNSPECIFIED) {
+  if (!IsValid())
     return nullptr;
-  }
 
   // Handle only full-range RGB spaces.
   if (matrix_ != MatrixID::RGB) {
@@ -416,7 +437,6 @@ bool ColorSpace::GetTransferFunction(SkColorSpaceTransferFn* fn) const {
       return true;
     case ColorSpace::TransferID::RESERVED0:
     case ColorSpace::TransferID::RESERVED:
-    case ColorSpace::TransferID::UNSPECIFIED:
     case ColorSpace::TransferID::UNKNOWN:
     // All unknown values default to BT709
     case ColorSpace::TransferID::BT709:
@@ -436,6 +456,8 @@ bool ColorSpace::GetTransferFunction(SkColorSpaceTransferFn* fn) const {
       fn->fD = 0.091286342118f;
       fn->fG = 2.222222222222f;
       return true;
+    // Unspecified values are given the sRGB transfer function.

[hubbe, 2017/02/15 08:48:56]

The comment above says otherwise.

[ccameron, 2017/02/15 09:34:28]

This is the UNSPECIFIED versus UNKNOWN deal ... that said, the two should be
flipped (uploaded a patch that flipped them).

+    case ColorSpace::TransferID::UNSPECIFIED:
     case ColorSpace::TransferID::IEC61966_2_1:
       fn->fA = 0.947867345704f;
       fn->fB = 0.052132654296f;
@@ -475,12 +497,12 @@ void ColorSpace::GetTransferMatrix(SkMatrix44* matrix) const {
   float Kr = 0;
   float Kb = 0;
   switch (matrix_) {

[hubbe, 2017/02/15 08:48:56]

I'm worried that changing all these defaults will affect video rendering. LOTS
of videos have unspecified values in them.

I'm thinking that we need a method for filling in default values explicitly,
like:

   ColorSpace fully_specified = color_space.Merge(ColorSpace::BT709);

then we can add DCHECKS to all the unspecified/unknown values to make sure that
we don't forget to call Merge() and what the switch statements defaults to won't
matter anymore.

[ccameron, 2017/02/15 09:34:28]

Should have been UNKNOWN ... does this fix it?

[hubbe, 2017/02/15 18:47:45]

I don't think so. For video you we want unknown values to default to BT709 as
well I think.

+    case ColorSpace::MatrixID::UNSPECIFIED:
     case ColorSpace::MatrixID::RGB:
       matrix->setIdentity();
       return;
 
     case ColorSpace::MatrixID::BT709:
-    case ColorSpace::MatrixID::UNSPECIFIED:
     case ColorSpace::MatrixID::RESERVED:
     case ColorSpace::MatrixID::UNKNOWN:
       Kr = 0.2126f;