Towards deleting YUV to RGB computation redundancy

cc/output/gl_renderer.cc

 // Copyright 2010 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 "cc/output/gl_renderer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 2117 matching lines @@
   };
 
   // Same as above, but without the head and footroom.
   float yuv_adjust_full[3] = {
       0.0f, -128.f, -128.f,
   };
 
   float* yuv_to_rgb = NULL;
   float* yuv_adjust = NULL;
 
+  gfx::ColorSpace gfx_color_space;
   switch (color_space) {
     case YUVVideoDrawQuad::REC_601:
       yuv_to_rgb = yuv_to_rgb_rec601;
       yuv_adjust = yuv_adjust_constrained;
+      gfx_color_space = gfx::ColorSpace::CreateREC601();
       break;
     case YUVVideoDrawQuad::REC_709:
       yuv_to_rgb = yuv_to_rgb_rec709;
       yuv_adjust = yuv_adjust_constrained;
+      gfx_color_space = gfx::ColorSpace::CreateREC709();
       break;
     case YUVVideoDrawQuad::JPEG:
       yuv_to_rgb = yuv_to_rgb_jpeg;
       yuv_adjust = yuv_adjust_full;
+      gfx_color_space = gfx::ColorSpace::CreateJpeg();
       break;
   }
 
   // Formula according to BT.601-7 section 2.5.3.
   DCHECK_LE(YUVVideoDrawQuad::kMinBitsPerChannel, bits_per_channel);
   DCHECK_LE(bits_per_channel, YUVVideoDrawQuad::kMaxBitsPerChannel);
   float adjustment_multiplier =
       (1 << (bits_per_channel - 8)) * 1.0f / ((1 << bits_per_channel) - 1);
 
   for (int i = 0; i < 9; ++i)
     yuv_to_rgb_multiplied[i] = yuv_to_rgb[i] * resource_multiplier;
 
   for (int i = 0; i < 3; ++i) {
     yuv_adjust_with_offset[i] =
         yuv_adjust[i] * adjustment_multiplier / resource_multiplier -
         resource_offset;
   }
+
+  // TODO(ccameron): Delete the above code, and just the below code instead.
+
+  // Start with the resource adjust.
+  SkMatrix44 full_transform;

[hubbe, 2017/02/02 04:51:37]

I still think GetAffineApproximation() is a better & simpler way to do this.
(https://codereview.chromium.org/2559273002/)

[ccameron, 2017/02/02 17:33:52]

This patch is solving a different problem than that would solve.

The goal of this patch is to

- ensure that the old YUV->RGB code matches the new one (considering that it
found 2 bugs, mission accomplished)
- delete redundant code (or, prepare to delete redundant code)

[hubbe, 2017/02/02 18:35:25]

Matching is perhaps not the right goal since the code that tags the video frames
as JPEG/REC_601/REC_709 is not very accurate.
However, it is helpful to know if they match or not. (And *why* if they do not.)

+  {
+    full_transform.setScale(resource_multiplier, resource_multiplier,
+                            resource_multiplier);
+    full_transform.postTranslate(resource_offset, resource_offset,
+                                 resource_offset);
+  }

[hubbe, 2017/02/02 04:51:36]

What is this {} for?
(Same for the ones below.)

[ccameron, 2017/02/02 17:33:52]

In the instances below, it's to show to the reader that the temporary variables
are not used outside of the block. 

In this instance, it's was for consistency ... which doesn't really help, so I
got rid of it.

[hubbe, 2017/02/02 18:35:25]

For me the add cognitive load when I read the code as I keep looking to see if
there is a condition or loop before it.

+
+  // Then apply the range adjust.
+  {
+    SkMatrix44 range_adjust;
+    gfx_color_space.GetRangeAdjustMatrix(&range_adjust);
+    full_transform.postConcat(range_adjust);
+  }
+
+  // Then apply the YUV to RGB full_transform.
+  {
+    SkMatrix44 rgb_to_yuv;
+    SkMatrix44 yuv_to_rgb;
+    gfx_color_space.GetTransferMatrix(&rgb_to_yuv);

[hubbe, 2017/02/02 04:51:37]

In one case, this is is not a linear matrix...

[ccameron, 2017/02/02 17:33:52]

How to handle this in the general case is a good question. For the 3 cases that
we're replacing here, there exists a RGB-to-YUV matrix.

For the BT2020_CL case, it's a RGB-to-RYB matrix (and for others its
RGB-to-YCgCo or RGB-to-YDxDy), which are similar in character, so it didn't
quite seem right to break them off.

[hubbe, 2017/02/02 18:35:25]

The problem with the BT2020_CL case is that it depends on additional stuff
happening later. Also that "additional stuff" cannot be expressed as a matrix.

+    rgb_to_yuv.invert(&yuv_to_rgb);
+    full_transform.postConcat(yuv_to_rgb);
+  }
+
+  // For the upcoming DCHECKs, convert from the form
+  //   rgb = A*yuv+b
+  // to the form
+  //   rgb = A*(yuv+b)
+  float adjust[4] = {0, 0, 0, 0};
+  {
+    SkMatrix44 full_transform_inverse;
+    full_transform.invert(&full_transform_inverse);
+    float adjust_preimage[4] = {full_transform.get(0, 3),
+                                full_transform.get(1, 3),
+                                full_transform.get(2, 3), 0};
+    full_transform_inverse.mapScalars(adjust_preimage, adjust);
+  }
+
+  // TODO(ccameron): The gfx::ColorSpace-based approach produces some pixel
+  // differences. For the initial checkin, DCHECK that the parameters are
+  // very close. The subsequent checkins will delete the old path.
+  const float kEpsilon = 1.f / 255.f;
+  for (int i = 0; i < 3; ++i) {
+    for (int j = 0; j < 3; ++j) {
+      float diff = yuv_to_rgb_multiplied[3 * j + i] - full_transform.get(i, j);
+      DCHECK_LT(std::abs(diff), kEpsilon);
+    }
+    float diff = yuv_adjust_with_offset[i] - adjust[i];
+    DCHECK_LT(std::abs(diff), kEpsilon);
+  }
 }
 
 void GLRenderer::DrawYUVVideoQuad(const DrawingFrame* frame,
                                   const YUVVideoDrawQuad* quad,
                                   const gfx::QuadF* clip_region) {
   SetBlendEnabled(quad->ShouldDrawWithBlending());
 
   TexCoordPrecision tex_coord_precision = TexCoordPrecisionRequired(
       gl_, &highp_threshold_cache_, highp_threshold_min_,
       quad->shared_quad_state->visible_quad_layer_rect.bottom_right());
@@ skipping 1420 matching lines @@
     return;
 
   // Report GPU overdraw as a percentage of |max_result|.
   TRACE_COUNTER1(
       TRACE_DISABLED_BY_DEFAULT("cc.debug.overdraw"), "GPU Overdraw",
       (std::accumulate(overdraw->begin(), overdraw->end(), 0) * 100) /
           max_result);
 }
 
 }  // namespace cc