color: Add analytic transfer functions in shaders

ui/gfx/color_transform.cc

Index: ui/gfx/color_transform.cc
diff --git a/ui/gfx/color_transform.cc b/ui/gfx/color_transform.cc
index 15c7ac4e3df90af437215b41a968b8d8367f414c..043515a62573c4de4d4b2f94137addceecfcfa48 100644
--- a/ui/gfx/color_transform.cc
+++ b/ui/gfx/color_transform.cc
@@ -35,6 +35,10 @@ namespace gfx {
 
 namespace {
 
+std::string Str(float f) {
+  return base::StringPrintf("%f", f);
+}
+
 // Helper for scoped QCMS profiles.
 struct QcmsProfileDeleter {
   void operator()(qcms_profile* p) {
@@ -227,6 +231,7 @@ Transform GetPrimaryTransform(const gfx::ColorSpace& color_space) {
 }  // namespace
 
 class ColorTransformMatrix;
+class ColorTransformSkTransferFn;
 class ColorTransformFromLinear;
 class ColorTransformToBT2020CL;
 class ColorTransformFromBT2020CL;
@@ -240,6 +245,7 @@ class ColorTransformStep {
   virtual ColorTransformFromLinear* GetFromLinear() { return nullptr; }
   virtual ColorTransformToBT2020CL* GetToBT2020CL() { return nullptr; }
   virtual ColorTransformFromBT2020CL* GetFromBT2020CL() { return nullptr; }
+  virtual ColorTransformSkTransferFn* GetSkTransferFn() { return nullptr; }
   virtual ColorTransformMatrix* GetMatrix() { return nullptr; }
   virtual ColorTransformNull* GetNull() { return nullptr; }
   virtual QCMSColorTransform* GetQCMS() { return nullptr; }
@@ -348,72 +354,120 @@ class ColorTransformMatrix : public ColorTransformStep {
   class Transform matrix_;
 };
 
-class ColorTransformFromLinear : public ColorTransformStep {
+class ColorTransformSkTransferFn : public ColorTransformStep {
  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;
+  explicit ColorTransformSkTransferFn(const SkColorSpaceTransferFn& fn)
+      : fn_(fn) {}
+  ColorTransformSkTransferFn* GetSkTransferFn() override { return this; }
+
+  bool Join(ColorTransformStep* next_untyped) override {
+    ColorTransformSkTransferFn* next = next_untyped->GetSkTransferFn();
+    if (!next)
+      return false;
+    if (SkTransferFnsApproximatelyCancel(fn_, next->fn_)) {
+      // Set to be the identity.
+      fn_.fA = 1;
+      fn_.fB = 0;
+      fn_.fC = 1;
+      fn_.fD = 0;
+      fn_.fE = 0;
+      fn_.fF = 0;
+      fn_.fG = 1;
+      return true;
+    }
+    return false;
   }
-  ColorTransformFromLinear* GetFromLinear() override { return this; }
-  bool IsNull() override { return transfer_ == ColorSpace::TransferID::LINEAR; }
+
+  bool IsNull() override { return SkTransferFnIsApproximatelyIdentity(fn_); }
+
   void Transform(ColorTransform::TriStim* colors, size_t num) const 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()));
-      }
+    for (size_t i = 0; i < num; i++) {
+      colors[i].set_x(SkTransferFnEval(fn_, colors[i].x()));
+      colors[i].set_y(SkTransferFnEval(fn_, colors[i].y()));
+      colors[i].set_z(SkTransferFnEval(fn_, colors[i].z()));
+    }
+  }
+
+  bool CanAppendShaderSource() override { return true; }
+
+  void AppendShaderSourceChannel(std::string* result, const char* value) {

[hubbe, 2017/02/16 23:06:38]

Please add some tests for this.
It would also help me review the generated code, as I can see what the code
would look like in the test. :)

[ccameron, 2017/02/16 23:51:58]

Mmh, good point -- this was hit a little bit by the GLRenderer tests, but it
didn't get all of the cases (where some things are 1 and others are 0).

I added a test that goes through a variety of SkColorSpaceTransferFns, to
generate the various combinations of elided operations.

[hubbe, 2017/02/17 00:06:00]

Could I get one of two tests on the form:

string str;
trasnform->AppendShaderSource(&str);
EXPECT_EQ(str, "LITERAL SHADER SOURCE HERE"); ?

[ccameron, 2017/02/19 22:28:07]

I don't feel that that sort of test is particularly helpful -- it will fail
every time we change the shader source, but will give no signal as to whether or
not the shader source is correct.

I added a test similar to this, which verifies that the shader source doesn't
emit an "if" or a "pow" if they aren't needed -- is that closer to what you were
looking for?

We currently have 3 other sets of tests for this
1. GLRenderer tests that verify that all versions of this compile
2. GLRenderer pixel tests that verify that the output pixels did not change
3. Blink pixel layout tests that verify that the output pixels did not change

IMO we need to add more of category [2] tests (and I'm working on that, but it
has lots of unrelated plumbing that is needed first).

[hubbe, 2017/02/20 05:01:02]

That is exactly the point. I don't want a lot of these kind of tests, because
that would be really annoying, but one or two will force people to explicitly
show how their changes affect the generated code, which will make it much easier
for the person reviewing the changes. (Which is likely to be you or me.)

[ccameron, 2017/02/21 22:32:42]

Ah, that's a good idea.

I've added one test which does that.

+    const float kEpsilon = 1.f / 1024.f;
+
+    // Construct the linear segment
+    //   linear = C * x + F
+    // Elide operations that will be close to the identity.
+    std::string linear = value;
+    if (std::abs(fn_.fC - 1.f) > kEpsilon)
+      linear = Str(fn_.fC) + " * " + linear;
+    if (std::abs(fn_.fF) > kEpsilon)
+      linear = linear + "+ " + Str(fn_.fF);
+
+    // Construct the nonlinear segment.
+    //   nonlinear = pow(A * x + B, G) + E
+    // Elide operations (especially the pow) that will be close to the
+    // identity.
+    std::string nonlinear = value;
+    if (std::abs(fn_.fA - 1.f) > kEpsilon)
+      nonlinear = Str(fn_.fA) + " * " + nonlinear;
+    if (std::abs(fn_.fB) > kEpsilon)
+      nonlinear = nonlinear + " + " + Str(fn_.fB);
+    if (std::abs(fn_.fG - 1.f) > kEpsilon)
+      nonlinear = "pow(" + nonlinear + ", " + Str(fn_.fG) + ")";
+    if (std::abs(fn_.fE) > kEpsilon)
+      nonlinear = nonlinear + " + " + Str(fn_.fE);
+
+    // Add both parts, as needed.
+    SRC("// Apply transfer function.");
+    if (fn_.fD > kEpsilon) {
+      SRC("if (%s < %f)", value, fn_.fD);
+      SRC("  %s = %s;", value, linear.c_str());
+      SRC("else");
+      SRC("  %s = %s;", value, nonlinear.c_str());
     } 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()));
-      }
+      SRC("%s = %s;", value, nonlinear.c_str());
     }
   }
 
+  void AppendShaderSource(std::string* result) override {
+    // Append the transfer function for each channel.
+    AppendShaderSourceChannel(result, "color.r");

[hubbe, 2017/02/16 23:06:38]

Is the compiler smart enough to vectorize these?
I just wonder if it would be better to work on all channels simultaneously?

[ccameron, 2017/02/16 23:51:58]

I can't speak to anything but NVIDIA GPUs, but I'm pretty sure they operate the
same.

Since the ~Tesla generation, GPU shader cores are actually scalar units -- when
you do matrix-vector multiplies, they're executed one-float-at-a-time. At least
at a per-pixel level.

The place where GPU shader cores get parallelism is that they're executing the
same scalar program for each pixel. So, the compiler can be super-lazy and not
try to vectorize anything, and just rely on the fact that we're running the same
program on thousands of pixels in parallel to get the parallelism. In
particular, vectorizing in the compiler is a losing game.

In NVIDIA parlance, the parallel execution units are called "warps" of 16 or 32
"threads", where "thread" is sort of a misnomer (it's more like 1 lane of a
SIMD). Within this warp
- if the shader programs diverge (like have an if-statement), the shader will
execute both paths of the if-statement 
- if one of the threads is delayed cause of a texture cache miss, every thread
is delayed

This is all a mental relief for me, cause it means I don't have to think about
"is this vectorize-able".

[hubbe, 2017/02/17 00:06:00]

That's very interesting. It's my impression that some GPUs operate more
efficiently on quads. I'd like to get a second opinion on on this, perhaps
piman@?

+    AppendShaderSourceChannel(result, "color.g");
+    AppendShaderSourceChannel(result, "color.b");
+  }
+
  private:
-  friend class ColorTransformToLinear;
-  ColorSpace::TransferID transfer_;
   SkColorSpaceTransferFn fn_;
-  bool fn_valid_ = false;
 };
 
-class ColorTransformToLinear : public ColorTransformStep {
+class ColorTransformFromLinear : public ColorTransformStep {
  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;
+  explicit ColorTransformFromLinear(ColorSpace::TransferID transfer)
+      : transfer_(transfer) {}
+  ColorTransformFromLinear* GetFromLinear() override { return this; }
+  bool IsNull() override { return transfer_ == ColorSpace::TransferID::LINEAR; }
+  void Transform(ColorTransform::TriStim* colors, size_t num) const override {
+    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()));
+    }
   }
 
-  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;
+ private:
+  friend class ColorTransformToLinear;
+  ColorSpace::TransferID transfer_;
+};
 
-      default:
-        return false;
-    }
-  }
+class ColorTransformToLinear : public ColorTransformStep {
+ public:
+  explicit ColorTransformToLinear(ColorSpace::TransferID transfer)
+      : transfer_(transfer) {}
 
   bool Join(ColorTransformStep* 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_))) {
+    if (transfer_ == next->transfer_) {
       transfer_ = ColorSpace::TransferID::LINEAR;
       return true;
     }
@@ -441,13 +495,7 @@ class ColorTransformToLinear : public ColorTransformStep {
   }
 
   void Transform(ColorTransform::TriStim* colors, size_t num) const 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 if (transfer_ == ColorSpace::TransferID::SMPTEST2084_NON_HDR) {
+    if (transfer_ == ColorSpace::TransferID::SMPTEST2084_NON_HDR) {
       for (size_t i = 0; i < num; i++) {
         ColorTransform::TriStim ret(ToLinear(transfer_, colors[i].x()),
                                     ToLinear(transfer_, colors[i].y()),
@@ -473,8 +521,6 @@ class ColorTransformToLinear : public ColorTransformStep {
 
  private:
   ColorSpace::TransferID transfer_;
-  SkColorSpaceTransferFn fn_;
-  bool fn_valid_ = false;
 };
 
 // BT2020 Constant Luminance is different than most other
@@ -621,9 +667,12 @@ void ColorTransformInternal::AppendColorSpaceToColorSpaceTransform(
     return;
 
   SkColorSpaceTransferFn to_linear_fn;
-  bool to_linear_fn_valid = from.GetTransferFunction(&to_linear_fn);
-  steps_.push_back(base::MakeUnique<ColorTransformToLinear>(
-      from.transfer_, to_linear_fn, to_linear_fn_valid));
+  if (from.GetTransferFunction(&to_linear_fn)) {
+    steps_.push_back(
+        base::MakeUnique<ColorTransformSkTransferFn>(to_linear_fn));
+  } else {
+    steps_.push_back(base::MakeUnique<ColorTransformToLinear>(from.transfer_));
+  }
 
   if (from.matrix_ == ColorSpace::MatrixID::BT2020_CL) {
     // BT2020 CL is a special case.
@@ -640,9 +689,12 @@ void ColorTransformInternal::AppendColorSpaceToColorSpaceTransform(
   }
 
   SkColorSpaceTransferFn from_linear_fn;
-  bool from_linear_fn_valid = to.GetInverseTransferFunction(&from_linear_fn);
-  steps_.push_back(base::MakeUnique<ColorTransformFromLinear>(
-      to.transfer_, from_linear_fn, from_linear_fn_valid));
+  if (to.GetInverseTransferFunction(&from_linear_fn)) {
+    steps_.push_back(
+        base::MakeUnique<ColorTransformSkTransferFn>(from_linear_fn));
+  } else {
+    steps_.push_back(base::MakeUnique<ColorTransformFromLinear>(to.transfer_));
+  }
 
   steps_.push_back(
       base::MakeUnique<ColorTransformMatrix>(GetTransferMatrix(to)));