color: Clarify default behaviors

ui/gfx/color_space.cc

 // 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.
 
 #include "ui/gfx/color_space.h"
 
 #include <map>
 
 #include "base/lazy_instance.h"
 #include "base/synchronization/lock.h"
@@ skipping 68 matching lines @@
   }
   if (primaries_ == PrimaryID::CUSTOM) {
     memcpy(custom_primary_matrix_, other.custom_primary_matrix_,
            sizeof(custom_primary_matrix_));
   }
 }
 
 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
 ColorSpace ColorSpace::CreateSRGB() {
   return ColorSpace(PrimaryID::BT709, TransferID::IEC61966_2_1, MatrixID::RGB,
                     RangeID::FULL);
 }
 
 // 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);
 }
 
 // Static
 ColorSpace ColorSpace::CreateXYZD50() {
   return ColorSpace(PrimaryID::XYZ_D50, TransferID::LINEAR, MatrixID::RGB,
                     RangeID::FULL);
 }
@@ skipping 81 matching lines @@
   return false;
 }
 
 sk_sp<SkColorSpace> ColorSpace::ToSkColorSpace() const {
   // If we got a specific SkColorSpace from the ICCProfile that this color space
   // was created from, use that.
   if (icc_profile_sk_color_space_)
     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) {
     DLOG(ERROR) << "Not creating non-RGB SkColorSpace";
     return nullptr;
   }
   if (range_ != RangeID::FULL) {
     DLOG(ERROR) << "Not creating non-full-range SkColorSpace";
     return nullptr;
   }
@@ skipping 195 matching lines @@
       fn->fG = 2.2f;
       return true;
     case ColorSpace::TransferID::GAMMA24:
       fn->fG = 2.4f;
       return true;
     case ColorSpace::TransferID::GAMMA28:
       fn->fG = 2.8f;
       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:
     case ColorSpace::TransferID::SMPTE170M:
     case ColorSpace::TransferID::BT2020_10:
     case ColorSpace::TransferID::BT2020_12:
       fn->fA = 0.909672431050f;
       fn->fB = 0.090327568950f;
       fn->fC = 0.222222222222f;
       fn->fD = 0.081242862158f;
       fn->fG = 2.222222222222f;
       return true;
     case ColorSpace::TransferID::SMPTE240M:
       fn->fA = 0.899626676224f;
       fn->fB = 0.100373323776f;
       fn->fC = 0.250000000000f;
       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;
       fn->fC = 0.077399380805f;
       fn->fD = 0.040449937172f;
       fn->fG = 2.400000000000f;
       return true;
     case ColorSpace::TransferID::SMPTEST428_1:
       fn->fA = 0.225615407568f;
       fn->fE = -1.091041666667f;
@@ skipping 18 matching lines @@
 bool ColorSpace::GetInverseTransferFunction(SkColorSpaceTransferFn* fn) const {
   if (!GetTransferFunction(fn))
     return false;
   *fn = SkTransferFnInverse(*fn);
   return true;
 }
 
 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;
       Kb = 0.0722f;
       break;
 
     case ColorSpace::MatrixID::FCC:
       Kr = 0.30f;
       Kb = 0.11f;
       break;
@@ skipping 94 matching lines @@
     case MatrixID::BT2020_CL:
     case MatrixID::YDZDX:
     case MatrixID::UNKNOWN:
       matrix->setScale(255.0f/219.0f, 255.0f/224.0f, 255.0f/224.0f);
       matrix->postTranslate(-16.0f/219.0f, -15.5f/224.0f, -15.5f/224.0f);
       break;
   }
 }
 
 }  // namespace gfx