color: Towards ColorTransform optimizations and code generation

ui/gfx/color_transform.cc

 // Copyright (c) 2016 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_transform.h"
 
 #include <algorithm>
+#include <cmath>

[hubbe, 2017/02/13 22:03:18]

verbose :)

[ccameron, 2017/02/13 23:18:42]

Agree :/

...but I got a whole bunch of presubmit errors errors for using the C math
functions recently (IIRC in cc/), so I've started mass-switching things.

 #include <vector>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "third_party/qcms/src/qcms.h"
 #include "ui/gfx/color_space.h"
 #include "ui/gfx/icc_profile.h"
+#include "ui/gfx/skia_color_space_util.h"
 #include "ui/gfx/transform.h"
-#include "third_party/qcms/src/qcms.h"
 
 #ifndef THIS_MUST_BE_INCLUDED_AFTER_QCMS_H
 extern "C" {
 #include "third_party/qcms/src/chain.h"
 };
 #endif
 
 namespace gfx {
 
-float EvalSkTransferFn(const SkColorSpaceTransferFn& fn, float x) {
-  if (x < 0)
-    return 0;
-  if (x < fn.fD)
-    return fn.fC * x + fn.fF;
-  return powf(fn.fA * x + fn.fB, fn.fG) + fn.fE;
-}
+namespace {
+
+// Helper for scoped QCMS profiles.
+struct QcmsProfileDeleter {
+  void operator()(qcms_profile* p) {
+    if (p) {
+      qcms_profile_release(p);
+    }
+  }
+};
+using ScopedQcmsProfile = std::unique_ptr<qcms_profile, QcmsProfileDeleter>;
 
 Transform Invert(const Transform& t) {
   Transform ret = t;
   if (!t.GetInverse(&ret)) {
     LOG(ERROR) << "Inverse should alsways be possible.";
   }
   return ret;
 }
 
 float FromLinear(ColorSpace::TransferID id, float v) {
   switch (id) {
     case ColorSpace::TransferID::SMPTEST2084_NON_HDR:
       // Should already be handled.
       break;
 
     case ColorSpace::TransferID::LOG:
       if (v < 0.01f)
         return 0.0f;
-      return 1.0f + log(v) / log(10.0f) / 2.0f;
+      return 1.0f + std::log(v) / std::log(10.0f) / 2.0f;
 
     case ColorSpace::TransferID::LOG_SQRT:
-      if (v < sqrt(10.0f) / 1000.0f)
+      if (v < std::sqrt(10.0f) / 1000.0f)
         return 0.0f;
-      return 1.0f + log(v) / log(10.0f) / 2.5f;
+      return 1.0f + std::log(v) / std::log(10.0f) / 2.5f;
 
     case ColorSpace::TransferID::IEC61966_2_4: {
       float a = 1.099296826809442f;
       float b = 0.018053968510807f;
       if (v < -b) {
-        return -a * powf(-v, 0.45f) + (a - 1.0f);
+        return -a * std::pow(-v, 0.45f) + (a - 1.0f);
       } else if (v <= b) {
         return 4.5f * v;
       } else {
-        return a * powf(v, 0.45f) - (a - 1.0f);
+        return a * std::pow(v, 0.45f) - (a - 1.0f);
       }
     }
 
     case ColorSpace::TransferID::BT1361_ECG: {
       float a = 1.099f;
       float b = 0.018f;
       float l = 0.0045f;
       if (v < -l) {
-        return -(a * powf(-4.0f * v, 0.45f) + (a - 1.0f)) / 4.0f;
+        return -(a * std::pow(-4.0f * v, 0.45f) + (a - 1.0f)) / 4.0f;
       } else if (v <= b) {
         return 4.5f * v;
       } else {
-        return a * powf(v, 0.45f) - (a - 1.0f);
+        return a * std::pow(v, 0.45f) - (a - 1.0f);
       }
     }
 
     case ColorSpace::TransferID::SMPTEST2084: {
       // Go from scRGB levels to 0-1.
       v *= 80.0f / 10000.0f;
-      v = fmax(0.0f, v);
+      v = std::max(0.0f, v);
       float m1 = (2610.0f / 4096.0f) / 4.0f;
       float m2 = (2523.0f / 4096.0f) * 128.0f;
       float c1 = 3424.0f / 4096.0f;
       float c2 = (2413.0f / 4096.0f) * 32.0f;
       float c3 = (2392.0f / 4096.0f) * 32.0f;
-      return powf((c1 + c2 * powf(v, m1)) / (1.0f + c3 * powf(v, m1)), m2);
+      return std::pow(
+          (c1 + c2 * std::pow(v, m1)) / (1.0f + c3 * std::pow(v, m1)), m2);
     }
 
     // Spec: http://www.arib.or.jp/english/html/overview/doc/2-STD-B67v1_0.pdf
     case ColorSpace::TransferID::ARIB_STD_B67: {
       const float a = 0.17883277f;
       const float b = 0.28466892f;
       const float c = 0.55991073f;
-      v = fmax(0.0f, v);
+      v = std::max(0.0f, v);
       if (v <= 1)
         return 0.5f * sqrtf(v);
       else
-        return a * log(v - b) + c;
+        return a * std::log(v - b) + c;
     }
 
     default:
       // Handled by SkColorSpaceTransferFn.
       break;
   }
   NOTREACHED();
   return 0;
 }
 
 float ToLinear(ColorSpace::TransferID id, float v) {
   switch (id) {
     case ColorSpace::TransferID::LOG:
       if (v < 0.0f)
         return 0.0f;
-      return powf(10.0f, (v - 1.0f) * 2.0f);
+      return std::pow(10.0f, (v - 1.0f) * 2.0f);
 
     case ColorSpace::TransferID::LOG_SQRT:
       if (v < 0.0f)
         return 0.0f;
-      return powf(10.0f, (v - 1.0f) * 2.5f);
+      return std::pow(10.0f, (v - 1.0f) * 2.5f);
 
     case ColorSpace::TransferID::IEC61966_2_4: {
       float a = 1.099296826809442f;
-      float b = 0.018053968510807f;
-      if (v < FromLinear(ColorSpace::TransferID::IEC61966_2_4, -a)) {
-        return -powf((a - 1.0f - v) / a, 1.0f / 0.45f);
-      } else if (v <= FromLinear(ColorSpace::TransferID::IEC61966_2_4, b)) {
+      float c = -1.047844f;  //  FromLinear(IEC61966_2_4, -a)
+      float d = 0.081243f;   //  FromLinear(IEC61966_2_4, b));
+      if (v < c) {
+        return -std::pow((a - 1.0f - v) / a, 1.0f / 0.45f);
+      } else if (v <= d) {
         return v / 4.5f;
       } else {
-        return powf((v + a - 1.0f) / a, 1.0f / 0.45f);
+        return std::pow((v + a - 1.0f) / a, 1.0f / 0.45f);
       }
     }
 
     case ColorSpace::TransferID::BT1361_ECG: {
       float a = 1.099f;
-      float b = 0.018f;
-      float l = 0.0045f;
-      if (v < FromLinear(ColorSpace::TransferID::BT1361_ECG, -l)) {
-        return -powf((1.0f - a - v * 4.0f) / a, 1.0f / 0.45f) / 4.0f;
-      } else if (v <= FromLinear(ColorSpace::TransferID::BT1361_ECG, b)) {
+      float c = -0.02025f;  // FromLinear(BT1361_ECG, -l)

[hubbe, 2017/02/13 22:03:18]

Not a fan of refactoring and adding slight modifications to functionality in the
same CL.

[ccameron, 2017/02/13 23:18:42]

This should be functionally identical -- it just rolls the constant in, so that
we don't have the dependency between functions and so that we don't re-compute
the same value repeatedly.

I've removed this for now.

+      float d = 0.081f;     // FromLinear(BT1361_ECG, b)
+      if (v < c) {
+        return -std::pow((1.0f - a - v * 4.0f) / a, 1.0f / 0.45f) / 4.0f;
+      } else if (v <= d) {
         return v / 4.5f;
       } else {
-        return powf((v + a - 1.0f) / a, 1.0f / 0.45f);
+        return std::pow((v + a - 1.0f) / a, 1.0f / 0.45f);
       }
     }
 
     case ColorSpace::TransferID::SMPTEST2084: {
-      v = fmax(0.0f, v);
+      v = std::max(0.0f, v);
       float m1 = (2610.0f / 4096.0f) / 4.0f;
       float m2 = (2523.0f / 4096.0f) * 128.0f;
       float c1 = 3424.0f / 4096.0f;
       float c2 = (2413.0f / 4096.0f) * 32.0f;
       float c3 = (2392.0f / 4096.0f) * 32.0f;
-      v = powf(
-          fmax(powf(v, 1.0f / m2) - c1, 0) / (c2 - c3 * powf(v, 1.0f / m2)),
-          1.0f / m1);
+      v = std::pow(std::max(std::pow(v, 1.0f / m2) - c1, 0.0f) /
+                       (c2 - c3 * std::pow(v, 1.0f / m2)),
+                   1.0f / m1);
       // This matches the scRGB definition that 1.0 means 80 nits.
       // TODO(hubbe): It would be *nice* if 1.0 meant more than that, but
       // that might be difficult to do right now.
       v *= 10000.0f / 80.0f;
       return v;
     }
 
     case ColorSpace::TransferID::SMPTEST2084_NON_HDR:
-      v = fmax(0.0f, v);
-      return fmin(2.3f * pow(v, 2.8f), v / 5.0f + 0.8f);
+      v = std::max(0.0f, v);
+      return std::min(2.3f * pow(v, 2.8f), v / 5.0f + 0.8f);
 
     // Spec: http://www.arib.or.jp/english/html/overview/doc/2-STD-B67v1_0.pdf
     case ColorSpace::TransferID::ARIB_STD_B67: {
-      v = fmax(0.0f, v);
+      v = std::max(0.0f, v);
       const float a = 0.17883277f;
       const float b = 0.28466892f;
       const float c = 0.55991073f;
       float v_ = 0.0f;
       if (v <= 0.5f) {
         v_ = (v * 2.0f) * (v * 2.0f);
       } else {
-        v_ = exp((v - c) / a) + b;
+        v_ = std::exp((v - c) / a) + b;
       }
       return v_;
     }
 
     default:
       // Handled by SkColorSpaceTransferFn.
       break;
   }
   NOTREACHED();
   return 0;
 }
 
 Transform GetTransferMatrix(const gfx::ColorSpace& color_space) {
   SkMatrix44 transfer_matrix;
   color_space.GetTransferMatrix(&transfer_matrix);
   return Transform(transfer_matrix);
 }
 
 Transform GetRangeAdjustMatrix(const gfx::ColorSpace& color_space) {
   SkMatrix44 range_adjust_matrix;
   color_space.GetRangeAdjustMatrix(&range_adjust_matrix);
   return Transform(range_adjust_matrix);
 }
 
+Transform GetPrimaryTransform(const gfx::ColorSpace& color_space) {
+  SkMatrix44 primary_matrix;
+  color_space.GetPrimaryMatrix(&primary_matrix);
+  return Transform(primary_matrix);
+}
+
+}  // namespace
+
 class ColorTransformMatrix;
-class ColorTransformToLinear;
 class ColorTransformFromLinear;
 class ColorTransformToBT2020CL;
 class ColorTransformFromBT2020CL;
 class ColorTransformNull;
+class QCMSColorTransform;
 
-class ColorTransformInternal : public ColorTransform {
+class ColorTransform::Step {
  public:
-  // Visitor pattern, Prepend() calls return prev->Join(this).
-  virtual bool Prepend(ColorTransformInternal* prev) = 0;
+  virtual ~Step() {}
+  virtual ColorTransformFromLinear* GetFromLinear() { return nullptr; }
+  virtual ColorTransformToBT2020CL* GetToBT2020CL() { return nullptr; }
+  virtual ColorTransformFromBT2020CL* GetFromBT2020CL() { return nullptr; }
+  virtual ColorTransformMatrix* GetMatrix() { return nullptr; }
+  virtual ColorTransformNull* GetNull() { return nullptr; }
+  virtual QCMSColorTransform* GetQCMS() { return nullptr; }
 
   // Join methods, returns true if the |next| transform was successfully
   // assimilated into |this|.
   // If Join() returns true, |next| is no longer needed and can be deleted.
-  virtual bool Join(const ColorTransformToLinear& next) { return false; }
-  virtual bool Join(const ColorTransformFromLinear& next) { return false; }
-  virtual bool Join(const ColorTransformToBT2020CL& next) { return false; }
-  virtual bool Join(const ColorTransformFromBT2020CL& next) { return false; }
-  virtual bool Join(const ColorTransformMatrix& next) { return false; }
-  virtual bool Join(const ColorTransformNull& next) { return true; }
+  virtual bool Join(Step* next) { return false; }
 
   // Return true if this is a null transform.
   virtual bool IsNull() { return false; }
+
+  virtual void transform(ColorTransform::TriStim* color, size_t num) = 0;
 };
 
-class ColorTransformNull : public ColorTransformInternal {
+class ColorTransformNull : public ColorTransform::Step {
  public:
-  bool Prepend(ColorTransformInternal* prev) override {
-    return prev->Join(*this);
-  }
+  ColorTransformNull* GetNull() override { return this; }
   bool IsNull() override { return true; }
   void transform(ColorTransform::TriStim* color, size_t num) override {}
 };
 
-class ColorTransformMatrix : public ColorTransformInternal {
+class ColorTransformMatrix : public ColorTransform::Step {
  public:
   explicit ColorTransformMatrix(const Transform& matrix) : matrix_(matrix) {}
-
-  bool Prepend(ColorTransformInternal* prev) override {
-    return prev->Join(*this);
-  }
-
-  bool Join(const ColorTransformMatrix& next) override {
-    Transform tmp = next.matrix_;
+  ColorTransformMatrix* GetMatrix() override { return this; }
+  bool Join(ColorTransform::Step* next_untyped) override {
+    ColorTransformMatrix* next = next_untyped->GetMatrix();
+    if (!next)
+      return false;
+    Transform tmp = next->matrix_;
     tmp *= matrix_;
     matrix_ = tmp;
     return true;
   }
 
   bool IsNull() override {
-    // Returns true if we're very close to an identity matrix.
-    for (int i = 0; i < 4; i++) {
-      for (int j = 0; j < 4; j++) {
-        float expected = i == j ? 1.0f : 0.0f;
-        if (fabs(matrix_.matrix().get(i, j) - expected) > 0.00001f) {
-          return false;
-        }
-      }
-    }
-    return true;
+    return SkMatrixIsApproximatelyIdentity(matrix_.matrix());
   }
 
   void transform(ColorTransform::TriStim* colors, size_t num) override {
     for (size_t i = 0; i < num; i++)
       matrix_.TransformPoint(colors + i);
   }
 
  private:
   Transform matrix_;
 };
 
-class ColorTransformFromLinear : public ColorTransformInternal {
+class ColorTransformFromLinear : public ColorTransform::Step {
  public:
   explicit ColorTransformFromLinear(ColorSpace::TransferID transfer,
                                     const SkColorSpaceTransferFn& fn,
                                     bool fn_valid)
       : transfer_(transfer), fn_(fn), fn_valid_(fn_valid) {
     if (transfer_ == ColorSpace::TransferID::LINEAR_HDR)
       transfer_ = ColorSpace::TransferID::LINEAR;
   }
-  bool Prepend(ColorTransformInternal* prev) override {
-    return prev->Join(*this);
-  }
-
+  ColorTransformFromLinear* GetFromLinear() override { return this; }
   bool IsNull() override { return transfer_ == ColorSpace::TransferID::LINEAR; }
-
   void transform(ColorTransform::TriStim* colors, size_t num) override {
     if (fn_valid_) {
       for (size_t i = 0; i < num; i++) {
         colors[i].set_x(EvalSkTransferFn(fn_, colors[i].x()));
         colors[i].set_y(EvalSkTransferFn(fn_, colors[i].y()));
         colors[i].set_z(EvalSkTransferFn(fn_, colors[i].z()));
       }
     } else {
       for (size_t i = 0; i < num; i++) {
         colors[i].set_x(FromLinear(transfer_, colors[i].x()));
         colors[i].set_y(FromLinear(transfer_, colors[i].y()));
         colors[i].set_z(FromLinear(transfer_, colors[i].z()));
       }
     }
   }
 
  private:
   friend class ColorTransformToLinear;
   ColorSpace::TransferID transfer_;
   SkColorSpaceTransferFn fn_;
   bool fn_valid_ = false;
 };
 
-class ColorTransformToLinear : public ColorTransformInternal {
+class ColorTransformToLinear : public ColorTransform::Step {
  public:
   explicit ColorTransformToLinear(ColorSpace::TransferID transfer,
                                   const SkColorSpaceTransferFn& fn,
                                   bool fn_valid)
       : transfer_(transfer), fn_(fn), fn_valid_(fn_valid) {
     if (transfer_ == ColorSpace::TransferID::LINEAR_HDR)
       transfer_ = ColorSpace::TransferID::LINEAR;
   }
 
-  bool Prepend(ColorTransformInternal* prev) override {
-    return prev->Join(*this);
-  }
-
   static bool IsGamma22(ColorSpace::TransferID transfer) {
     switch (transfer) {
       // We don't need to check BT709 here because it's been translated into
       // SRGB in ColorSpaceToColorSpaceTransform::ColorSpaceToLinear below.
       case ColorSpace::TransferID::GAMMA22:
       case ColorSpace::TransferID::IEC61966_2_1:  // SRGB
         return true;
 
       default:
         return false;
     }
   }
 
-  bool Join(const ColorTransformFromLinear& next) override {
-    if (transfer_ == next.transfer_ ||
-        (IsGamma22(transfer_) && IsGamma22(next.transfer_))) {
+  bool Join(ColorTransform::Step* next_untyped) override {
+    ColorTransformFromLinear* next = next_untyped->GetFromLinear();
+    if (!next)
+      return false;
+    // TODO(ccameron): Use SkTransferFnsApproximatelyCancel and
+    // SkTransferFnIsApproximatelyIdentity to merge parametric transfer
+    // functions.
+    if (transfer_ == next->transfer_ ||
+        (IsGamma22(transfer_) && IsGamma22(next->transfer_))) {
       transfer_ = ColorSpace::TransferID::LINEAR;
       return true;
     }
     return false;
   }
 
   bool IsNull() override { return transfer_ == ColorSpace::TransferID::LINEAR; }
 
   // Assumes BT2020 primaries.
   static float Luma(const ColorTransform::TriStim& c) {
@@ skipping 56 matching lines @@
 // on the RGB values. However, running the transfer function
 // on the U and V values doesn't make any sense since they
 // are centered at 0.5. To work around this, the transfer function
 // is applied to the Y, R and B values, and then the U and V
 // values are calculated from that.
 // In our implementation, the YUV->RGB matrix is used to
 // convert YUV to RYB (the G value is replaced with an Y value.)
 // Then we run the transfer function like normal, and finally
 // this class is inserted as an extra step which takes calculates
 // the U and V values.
-class ColorTransformToBT2020CL : public ColorTransformInternal {
+class ColorTransformToBT2020CL : public ColorTransform::Step {
  public:
-  bool Prepend(ColorTransformInternal* prev) override {
-    return prev->Join(*this);
-  }
-
-  bool Join(const ColorTransformFromBT2020CL& next) override {
+  bool Join(ColorTransform::Step* next_untyped) override {
+    ColorTransformFromBT2020CL* next = next_untyped->GetFromBT2020CL();
+    if (!next)
+      return false;
     if (null_)
       return false;
     null_ = true;
     return true;
   }
 
   bool IsNull() override { return null_; }
 
   void transform(ColorTransform::TriStim* RYB, size_t num) override {
     for (size_t i = 0; i < num; i++) {
@@ skipping 12 matching lines @@
       }
       RYB[i] = ColorTransform::TriStim(RYB[i].y(), U, V);
     }
   }
 
  private:
   bool null_ = false;
 };
 
 // Inverse of ColorTransformToBT2020CL, see comment above for more info.
-class ColorTransformFromBT2020CL : public ColorTransformInternal {
+class ColorTransformFromBT2020CL : public ColorTransform::Step {
  public:
-  bool Prepend(ColorTransformInternal* prev) override {
-    return prev->Join(*this);
-  }
-
-  bool Join(const ColorTransformToBT2020CL& next) override {
+  bool Join(ColorTransform::Step* next_untyped) override {
+    ColorTransformToBT2020CL* next = next_untyped->GetToBT2020CL();
+    if (!next)
+      return false;
     if (null_)
       return false;
     null_ = true;
     return true;
   }
 
   bool IsNull() override { return null_; }
 
   void transform(ColorTransform::TriStim* YUV, size_t num) override {
     if (null_)
@@ skipping 15 matching lines @@
       }
       // Return an RYB value, later steps will fix it.
       YUV[i] = ColorTransform::TriStim(R_Y + Y, YUV[i].x(), B_Y + Y);
     }
   }
 
  private:
   bool null_ = false;
 };
 
-class ChainColorTransform : public ColorTransform {
- public:
-  ChainColorTransform(std::unique_ptr<ColorTransform> a,
-                      std::unique_ptr<ColorTransform> b)
-      : a_(std::move(a)), b_(std::move(b)) {}
+// static
+void ColorTransform::Append(ColorSpace from,
+                            const ColorSpace& to,
+                            ColorTransform::Intent intent,
+                            StepList* builder) {
+  if (intent == ColorTransform::Intent::INTENT_PERCEPTUAL) {
+    switch (from.transfer_) {
+      case ColorSpace::TransferID::UNSPECIFIED:
+      case ColorSpace::TransferID::BT709:
+      case ColorSpace::TransferID::SMPTE170M:
+        // SMPTE 1886 suggests that we should be using gamma 2.4 for BT709
+        // content. However, most displays actually use a gamma of 2.2, and
+        // user studies shows that users don't really care. Using the same
+        // gamma as the display will let us optimize a lot more, so lets stick
+        // with using the SRGB transfer function.
+        from.transfer_ = ColorSpace::TransferID::IEC61966_2_1;
+        break;
 
- private:
-  void transform(TriStim* colors, size_t num) override {
-    a_->transform(colors, num);
-    b_->transform(colors, num);
-  }
-  std::unique_ptr<ColorTransform> a_;
-  std::unique_ptr<ColorTransform> b_;
-};
+      case ColorSpace::TransferID::SMPTEST2084:
+        if (!to.IsHDR()) {
+          // We don't have an HDR display, so replace SMPTE 2084 with
+          // something that returns ranges more or less suitable for a normal
+          // display.
+          from.transfer_ = ColorSpace::TransferID::SMPTEST2084_NON_HDR;
+        }
+        break;
 
-class TransformBuilder {
- public:
-  void Append(std::unique_ptr<ColorTransformInternal> transform) {
-    if (!disable_optimizations_ && transform->IsNull())
-      return;  // Null transform
-    transforms_.push_back(std::move(transform));
-    if (disable_optimizations_)
-      return;
-    while (transforms_.size() >= 2 &&
-           transforms_.back()->Prepend(
-               transforms_[transforms_.size() - 2].get())) {
-      transforms_.pop_back();
-      if (transforms_.back()->IsNull()) {
-        transforms_.pop_back();
+      case ColorSpace::TransferID::ARIB_STD_B67:
+        if (!to.IsHDR()) {
+          // Interpreting HLG using a gamma 2.4 works reasonably well for SDR
+          // displays.
+          from.transfer_ = ColorSpace::TransferID::GAMMA24;
+        }
         break;
-      }
+
+      default:  // Do nothing
+        break;
     }
+
+    // TODO(hubbe): shrink gamuts here (never stretch gamuts)
   }
 
-  std::unique_ptr<ColorTransform> GetTransform() {
-    if (transforms_.empty())
-      return base::MakeUnique<ColorTransformNull>();
-    std::unique_ptr<ColorTransform> ret(std::move(transforms_.back()));
-    transforms_.pop_back();
+  builder->push_back(
+      base::MakeUnique<ColorTransformMatrix>(GetRangeAdjustMatrix(from)));
 
-    while (!transforms_.empty()) {
-      ret = std::unique_ptr<ColorTransform>(new ChainColorTransform(
-          std::move(transforms_.back()), std::move(ret)));
-      transforms_.pop_back();
-    }
+  builder->push_back(
+      base::MakeUnique<ColorTransformMatrix>(Invert(GetTransferMatrix(from))));
 
-    return ret;
+  SkColorSpaceTransferFn to_linear_fn;
+  bool to_linear_fn_valid = from.GetTransferFunction(&to_linear_fn);
+  builder->push_back(base::MakeUnique<ColorTransformToLinear>(
+      from.transfer_, to_linear_fn, to_linear_fn_valid));
+
+  if (from.matrix_ == ColorSpace::MatrixID::BT2020_CL) {
+    // BT2020 CL is a special case.
+    builder->push_back(base::MakeUnique<ColorTransformFromBT2020CL>());
+  }
+  builder->push_back(
+      base::MakeUnique<ColorTransformMatrix>(GetPrimaryTransform(from)));
+
+  builder->push_back(
+      base::MakeUnique<ColorTransformMatrix>(Invert(GetPrimaryTransform(to))));
+  if (to.matrix_ == ColorSpace::MatrixID::BT2020_CL) {
+    // BT2020 CL is a special case.
+    builder->push_back(base::MakeUnique<ColorTransformToBT2020CL>());
   }
 
-  void disable_optimizations() { disable_optimizations_ = true; }
+  SkColorSpaceTransferFn from_linear_fn;
+  bool from_linear_fn_valid = to.GetInverseTransferFunction(&from_linear_fn);
+  builder->push_back(base::MakeUnique<ColorTransformFromLinear>(
+      to.transfer_, from_linear_fn, from_linear_fn_valid));
 
- private:
-  bool disable_optimizations_ = false;
-  std::vector<std::unique_ptr<ColorTransformInternal>> transforms_;
-};
+  builder->push_back(
+      base::MakeUnique<ColorTransformMatrix>(GetTransferMatrix(to)));
 
-class ColorSpaceToColorSpaceTransform {
- public:
-  static Transform GetPrimaryTransform(const ColorSpace& c) {
-    SkMatrix44 sk_matrix;
-    c.GetPrimaryMatrix(&sk_matrix);
-    return Transform(sk_matrix);
-  }
+  builder->push_back(
+      base::MakeUnique<ColorTransformMatrix>(Invert(GetRangeAdjustMatrix(to))));
+}
 
-  static void ColorSpaceToColorSpace(ColorSpace from,
-                                     ColorSpace to,
-                                     ColorTransform::Intent intent,
-                                     TransformBuilder* builder) {
-    if (intent == ColorTransform::Intent::INTENT_PERCEPTUAL) {
-      switch (from.transfer_) {
-        case ColorSpace::TransferID::UNSPECIFIED:
-        case ColorSpace::TransferID::BT709:
-        case ColorSpace::TransferID::SMPTE170M:
-          // SMPTE 1886 suggests that we should be using gamma 2.4 for BT709
-          // content. However, most displays actually use a gamma of 2.2, and
-          // user studies shows that users don't really care. Using the same
-          // gamma as the display will let us optimize a lot more, so lets stick
-          // with using the SRGB transfer function.
-          from.transfer_ = ColorSpace::TransferID::IEC61966_2_1;
-          break;
-
-        case ColorSpace::TransferID::SMPTEST2084:
-          if (!to.IsHDR()) {
-            // We don't have an HDR display, so replace SMPTE 2084 with
-            // something that returns ranges more or less suitable for a normal
-            // display.
-            from.transfer_ = ColorSpace::TransferID::SMPTEST2084_NON_HDR;
-          }
-          break;
-
-        case ColorSpace::TransferID::ARIB_STD_B67:
-          if (!to.IsHDR()) {
-            // Interpreting HLG using a gamma 2.4 works reasonably well for SDR
-            // displays.
-            from.transfer_ = ColorSpace::TransferID::GAMMA24;
-          }
-          break;
-
-        default:  // Do nothing
-          break;
-      }
-
-      // TODO(hubbe): shrink gamuts here (never stretch gamuts)
-    }
-
-    builder->Append(base::MakeUnique<ColorTransformMatrix>(
-        GetRangeAdjustMatrix(from)));
-
-    builder->Append(base::MakeUnique<ColorTransformMatrix>(
-        Invert(GetTransferMatrix(from))));
-
-    SkColorSpaceTransferFn to_linear_fn;
-    bool to_linear_fn_valid = from.GetTransferFunction(&to_linear_fn);
-    builder->Append(base::MakeUnique<ColorTransformToLinear>(
-        from.transfer_, to_linear_fn, to_linear_fn_valid));
-
-    if (from.matrix_ == ColorSpace::MatrixID::BT2020_CL) {
-      // BT2020 CL is a special case.
-      builder->Append(base::MakeUnique<ColorTransformFromBT2020CL>());
-    }
-    builder->Append(
-        base::MakeUnique<ColorTransformMatrix>(GetPrimaryTransform(from)));
-
-    builder->Append(base::MakeUnique<ColorTransformMatrix>(
-        Invert(GetPrimaryTransform(to))));
-    if (to.matrix_ == ColorSpace::MatrixID::BT2020_CL) {
-      // BT2020 CL is a special case.
-      builder->Append(base::MakeUnique<ColorTransformToBT2020CL>());
-    }
-
-    SkColorSpaceTransferFn from_linear_fn;
-    bool from_linear_fn_valid = to.GetInverseTransferFunction(&from_linear_fn);
-    builder->Append(base::MakeUnique<ColorTransformFromLinear>(
-        to.transfer_, from_linear_fn, from_linear_fn_valid));
-
-    builder->Append(
-        base::MakeUnique<ColorTransformMatrix>(GetTransferMatrix(to)));
-
-    builder->Append(base::MakeUnique<ColorTransformMatrix>(
-        Invert(GetRangeAdjustMatrix(to))));
-  }
-};
-
-class QCMSColorTransform : public ColorTransformInternal {
+class QCMSColorTransform : public ColorTransform::Step {
  public:
   // Takes ownership of the profiles
-  QCMSColorTransform(qcms_profile* from, qcms_profile* to)
-      : from_(from), to_(to) {}
-  ~QCMSColorTransform() override {
-    qcms_profile_release(from_);
-    qcms_profile_release(to_);
-  }
-  bool Prepend(ColorTransformInternal* prev) override {
-    // Not currently optimizable.
+  QCMSColorTransform(ScopedQcmsProfile from, ScopedQcmsProfile to)
+      : from_(std::move(from)), to_(std::move(to)) {}
+  ~QCMSColorTransform() override {}
+  QCMSColorTransform* GetQCMS() override { return this; }
+  bool Join(ColorTransform::Step* next_untyped) override {
+    QCMSColorTransform* next = next_untyped->GetQCMS();
+    if (!next)
+      return false;
+    if (qcms_profile_match(to_.get(), next->from_.get())) {
+      to_ = std::move(next->to_);
+      return true;
+    }
     return false;
   }
-  bool IsNull() override { return from_ == to_; }
-  void transform(TriStim* colors, size_t num) override {
-    CHECK(sizeof(TriStim) == sizeof(float[3]));
+  bool IsNull() override {
+    if (qcms_profile_match(from_.get(), to_.get()))
+      return true;
+    return false;
+  }
+  void transform(ColorTransform::TriStim* colors, size_t num) override {
+    CHECK(sizeof(ColorTransform::TriStim) == sizeof(float[3]));
     // QCMS doesn't like numbers outside 0..1
     for (size_t i = 0; i < num; i++) {
-      colors[i].set_x(fmin(1.0f, fmax(0.0f, colors[i].x())));
-      colors[i].set_y(fmin(1.0f, fmax(0.0f, colors[i].y())));
-      colors[i].set_z(fmin(1.0f, fmax(0.0f, colors[i].z())));
+      colors[i].set_x(std::min(1.0f, std::max(0.0f, colors[i].x())));
+      colors[i].set_y(std::min(1.0f, std::max(0.0f, colors[i].y())));
+      colors[i].set_z(std::min(1.0f, std::max(0.0f, colors[i].z())));
     }
-    qcms_chain_transform(from_, to_, reinterpret_cast<float*>(colors),
+    qcms_chain_transform(from_.get(), to_.get(),
+                         reinterpret_cast<float*>(colors),
                          reinterpret_cast<float*>(colors), num * 3);
   }
 
  private:
-  qcms_profile *from_, *to_;
+  ScopedQcmsProfile from_;
+  ScopedQcmsProfile to_;
 };
 
-qcms_profile* GetQCMSProfileIfAvailable(const ColorSpace& color_space) {
+ScopedQcmsProfile GetQCMSProfileIfAvailable(const ColorSpace& color_space) {
   ICCProfile icc_profile = ICCProfile::FromColorSpace(color_space);
   if (icc_profile.GetData().empty())
     return nullptr;
-  return qcms_profile_from_memory(icc_profile.GetData().data(),
-                                  icc_profile.GetData().size());
+  return ScopedQcmsProfile(qcms_profile_from_memory(
+      icc_profile.GetData().data(), icc_profile.GetData().size()));
 }
 
-qcms_profile* GetXYZD50Profile() {
+ScopedQcmsProfile GetXYZD50Profile() {
   // QCMS is trixy, it has a datatype called qcms_CIE_xyY, but what it expects
   // is in fact not xyY color coordinates, it just wants the x/y values of the
   // primaries with Y equal to 1.0.
   qcms_CIE_xyYTRIPLE xyz;
   qcms_CIE_xyY w;
   xyz.red.x = 1.0f;
   xyz.red.y = 0.0f;
   xyz.red.Y = 1.0f;
   xyz.green.x = 0.0f;
   xyz.green.y = 1.0f;
   xyz.green.Y = 1.0f;
   xyz.blue.x = 0.0f;
   xyz.blue.y = 0.0f;
   xyz.blue.Y = 1.0f;
   w.x = 0.34567f;
   w.y = 0.35850f;
   w.Y = 1.0f;
-  return qcms_profile_create_rgb_with_gamma(w, xyz, 1.0f);
+  return ScopedQcmsProfile(qcms_profile_create_rgb_with_gamma(w, xyz, 1.0f));
 }
 
 std::unique_ptr<ColorTransform> ColorTransform::NewColorTransform(
     const ColorSpace& from,
     const ColorSpace& to,
     Intent intent) {
-  TransformBuilder builder;
-  if (intent == Intent::TEST_NO_OPT) {
-    builder.disable_optimizations();
+  std::list<std::unique_ptr<Step>> builder;
+
+  ScopedQcmsProfile from_profile = GetQCMSProfileIfAvailable(from);
+  ScopedQcmsProfile to_profile = GetQCMSProfileIfAvailable(to);
+  bool has_from_profile = !!from_profile;
+  bool has_to_profile = !!to_profile;
+
+  if (from_profile) {
+    builder.push_back(base::MakeUnique<QCMSColorTransform>(
+        std::move(from_profile), GetXYZD50Profile()));
   }
 
-  qcms_profile* from_profile = GetQCMSProfileIfAvailable(from);
-  qcms_profile* to_profile = GetQCMSProfileIfAvailable(to);
+  Append(has_from_profile ? ColorSpace::CreateXYZD50() : from,
+         has_to_profile ? ColorSpace::CreateXYZD50() : to, intent, &builder);
 
-  if (from_profile && to_profile) {
-    return std::unique_ptr<ColorTransform>(
-        new QCMSColorTransform(from_profile, to_profile));
-  }
-  if (from_profile) {
-    builder.Append(std::unique_ptr<ColorTransformInternal>(
-        new QCMSColorTransform(from_profile, GetXYZD50Profile())));
-  }
-  ColorSpaceToColorSpaceTransform::ColorSpaceToColorSpace(
-      from_profile ? ColorSpace::CreateXYZD50() : from,
-      to_profile ? ColorSpace::CreateXYZD50() : to, intent, &builder);
   if (to_profile) {
-    builder.Append(std::unique_ptr<ColorTransformInternal>(
-        new QCMSColorTransform(GetXYZD50Profile(), to_profile)));
+    builder.push_back(base::MakeUnique<QCMSColorTransform>(
+        GetXYZD50Profile(), std::move(to_profile)));
   }
 
-  return builder.GetTransform();
+  if (intent != Intent::TEST_NO_OPT)
+    Simplify(&builder);
+
+  return std::unique_ptr<ColorTransform>(
+      new ColorTransform(std::move(builder)));
 }
 
 // static
-float ColorTransform::ToLinearForTesting(ColorSpace::TransferID transfer,
-                                         float v) {
-  ColorSpace space(ColorSpace::PrimaryID::BT709, transfer,
-                   ColorSpace::MatrixID::RGB, ColorSpace::RangeID::FULL);
-  SkColorSpaceTransferFn to_linear_fn;
-  bool to_linear_fn_valid = space.GetTransferFunction(&to_linear_fn);
-  ColorTransformToLinear to_linear_transform(transfer, to_linear_fn,
-                                             to_linear_fn_valid);
-  TriStim color(v, v, v);
-  to_linear_transform.transform(&color, 1);
-  return color.x();
+void ColorTransform::Simplify(StepList* steps) {
+  for (auto iter = steps->begin(); iter != steps->end();) {
+    std::unique_ptr<Step>& this_step = *iter;
+
+    // Try to Join |next_step| into |this_step|. If successful, re-visit the
+    // step before |this_step|.
+    auto iter_next = iter;
+    iter_next++;
+    if (iter_next != steps->end()) {
+      std::unique_ptr<Step>& next_step = *iter_next;
+      if (this_step->Join(next_step.get())) {
+        steps->erase(iter_next);
+        if (iter != steps->begin())
+          --iter;
+        continue;
+      }
+    }
+
+    // If |this_step| step is a no-op, remove it, and re-visit the step before
+    // |this_step|.
+    if (this_step->IsNull()) {
+      iter = steps->erase(iter);
+      if (iter != steps->begin())
+        --iter;
+      continue;
+    }
+
+    ++iter;
+  }
 }
 
-// static
-float ColorTransform::FromLinearForTesting(ColorSpace::TransferID transfer,
-                                           float v) {
-  ColorSpace space(ColorSpace::PrimaryID::BT709, transfer,
-                   ColorSpace::MatrixID::RGB, ColorSpace::RangeID::FULL);
-  SkColorSpaceTransferFn from_linear_fn;
-  bool from_linear_fn_valid = space.GetInverseTransferFunction(&from_linear_fn);
-
-  ColorTransformFromLinear from_linear_transform(transfer, from_linear_fn,
-                                                 from_linear_fn_valid);
-  TriStim color(v, v, v);
-  from_linear_transform.transform(&color, 1);
-  return color.x();
+void ColorTransform::transform(ColorTransform::TriStim* colors, size_t num) {
+  for (const auto& step : steps_)
+    step->transform(colors, num);
 }
 
+ColorTransform::ColorTransform(ColorTransform::StepList steps)
+    : steps_(std::move(steps)) {}
+
+ColorTransform::~ColorTransform() {}
+
 }  // namespace gfx