Night Light: Don't apply gamma tables

We found out that the hardware split gamma mode
doesn't do interpolation of the values, which led
to some bad side effects such as ugly banding in
blur effects.

This CL removes all the default gamma tables, and
uses the newly exposed data to identify if the
hardware does color correction in linear gamma
space or not. This lets us select which type of
matrix to apply.

BUG=851156,749250
TEST=Expanded tests

Change-Id: I0342b6e4a0257e833c7d13690930dfd32ce89fe7
Reviewed-on: https://chromium-review.googlesource.com/1112734Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570279}

ash/display/display_color_manager.cc

@@ -13,6 +13,7 @@
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sequenced_task_runner.h"
+#include "base/stl_util.h"
 #include "base/task_runner_util.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/threading/thread_restrictions.h"
@@ -175,40 +176,6 @@ SkMatrix44 SkMatrix44FromColorMatrixVector(
   return matrix;
 }
 
-constexpr int kGammaMaxValue = (1 << 16) - 1;
-constexpr size_t kDefaultTableSize = 512;
-
-// The below functions are generating default Lookup Tables (LUTs) that use
-// the BT.709 transfer function for gamma/degamma.
-std::vector<display::GammaRampRGBEntry> GetDefaultGammaLut() {
-  std::vector<display::GammaRampRGBEntry> table;
-  table.reserve(kDefaultTableSize);
-
-  constexpr float kGammaValue = 1.0f / 2.2f;
-  for (size_t i = 0; i < kDefaultTableSize; ++i) {
-    const uint16_t v = static_cast<uint16_t>(
-        kGammaMaxValue *
-        std::pow(static_cast<float>(i) / kDefaultTableSize, kGammaValue));
-    table.emplace_back(display::GammaRampRGBEntry{v, v, v});
-  }
-
-  return table;
-}
-
-std::vector<display::GammaRampRGBEntry> GetDefaultDeGammaLut() {
-  std::vector<display::GammaRampRGBEntry> table;
-  table.reserve(kDefaultTableSize);
-
-  for (size_t i = 0; i < kDefaultTableSize; ++i) {
-    const uint16_t v = static_cast<uint16_t>(
-        kGammaMaxValue *
-        std::pow(static_cast<float>(i) / kDefaultTableSize, 2.2f));
-    table.emplace_back(display::GammaRampRGBEntry{v, v, v});
-  }
-
-  return table;
-}
-
 bool HasColorCorrectionMatrix(display::DisplayConfigurator* configurator,
                               int64_t display_id) {
   for (const auto* display_snapshot : configurator->cached_displays()) {
@@ -228,8 +195,6 @@ DisplayColorManager::DisplayColorManager(
     display::Screen* screen_to_observe)
     : configurator_(configurator),
       matrix_buffer_(9, 0.0f),  // 3x3 matrix.
-      default_gamma_lut_(GetDefaultGammaLut()),
-      default_degamma_lut_(GetDefaultDeGammaLut()),
       sequenced_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
           {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
            base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
@@ -254,29 +219,40 @@ bool DisplayColorManager::SetDisplayColorMatrix(
     if (display_snapshot->display_id() != display_id)
       continue;
 
-    if (!display_snapshot->has_color_correction_matrix()) {
-      // This display doesn't support setting a CRTC matrix.
-      return false;
-    }
-
-    // Always overwrite any existing matrix for this display.
-    displays_color_matrix_map_[display_id] = color_matrix;
-    const auto iter = calibration_map_.find(display_snapshot->product_code());
-    SkMatrix44 combined_matrix = color_matrix;
-    if (iter != calibration_map_.end()) {
-      DCHECK(iter->second);
-      combined_matrix.preConcat(
-          SkMatrix44FromColorMatrixVector(iter->second->correction_matrix));
-    }
-
-    ColorMatrixVectorFromSkMatrix44(combined_matrix, &matrix_buffer_);
-    return configurator_->SetColorMatrix(display_id, matrix_buffer_);
+    return SetDisplayColorMatrix(display_snapshot, color_matrix);
   }
 
   LOG(ERROR) << "Display ID: " << display_id << " cannot be found.";
   return false;
 }
 
+bool DisplayColorManager::SetDisplayColorMatrix(
+    const display::DisplaySnapshot* display_snapshot,
+    const SkMatrix44& color_matrix) {
+  DCHECK(display_snapshot);
+  DCHECK(
+      base::ContainsValue(configurator_->cached_displays(), display_snapshot));
+
+  if (!display_snapshot->has_color_correction_matrix()) {
+    // This display doesn't support setting a CRTC matrix.
+    return false;
+  }
+
+  // Always overwrite any existing matrix for this display.
+  const int64_t display_id = display_snapshot->display_id();
+  displays_color_matrix_map_[display_id] = color_matrix;
+  const auto iter = calibration_map_.find(display_snapshot->product_code());
+  SkMatrix44 combined_matrix = color_matrix;
+  if (iter != calibration_map_.end()) {
+    DCHECK(iter->second);
+    combined_matrix.preConcat(
+        SkMatrix44FromColorMatrixVector(iter->second->correction_matrix));
+  }
+
+  ColorMatrixVectorFromSkMatrix44(combined_matrix, &matrix_buffer_);
+  return configurator_->SetColorMatrix(display_id, matrix_buffer_);
+}
+
 void DisplayColorManager::OnDisplayModeChanged(
     const display::DisplayConfigurator::DisplayStateList& display_states) {
   size_t displays_with_ctm_support_count = 0;
@@ -332,22 +308,9 @@ void DisplayColorManager::ApplyDisplayColorCalibration(
       LOG(WARNING) << "Error applying the color matrix.";
   }
 
-  // When applying gamma correction, we either use the gamma/degamma tables from
-  // the calibration data if available, or we apply default ones. This makes
-  // sure that color transform matrices are always applied in the RGB linear
-  // space, which gives us consistent colors on all platforms.
-  // The associated gamma/degamma tables should be applied together; i.e. either
-  // both the default gamma/degamma tables, or both the tables from the ICC
-  // profile should be applied together.
-  const bool should_apply_default_tables =
-      calibration_data.degamma_lut.empty() &&
-      calibration_data.gamma_lut.empty();
-  if (!configurator_->SetGammaCorrection(
-          display_id,
-          should_apply_default_tables ? default_degamma_lut_
-                                      : calibration_data.degamma_lut,
-          should_apply_default_tables ? default_gamma_lut_
-                                      : calibration_data.gamma_lut)) {
+  if (!configurator_->SetGammaCorrection(display_id,
+                                         calibration_data.degamma_lut,
+                                         calibration_data.gamma_lut)) {
     LOG(WARNING) << "Error applying gamma correction data.";
   }
 }

ash/display/display_color_manager.h

@@ -69,6 +69,11 @@ class ASH_EXPORT DisplayColorManager
   bool SetDisplayColorMatrix(int64_t display_id,
                              const SkMatrix44& color_matrix);
 
+  // Similar to the above but can be used when a display snapshot is known to
+  // the caller.
+  bool SetDisplayColorMatrix(const display::DisplaySnapshot* display_snapshot,
+                             const SkMatrix44& color_matrix);
+
   // display::DisplayConfigurator::Observer
   void OnDisplayModeChanged(
       const display::DisplayConfigurator::DisplayStateList& outputs) override;
@@ -137,9 +142,6 @@ class ASH_EXPORT DisplayColorManager
   base::flat_map<int64_t, std::unique_ptr<ColorCalibrationData>>
       calibration_map_;
 
-  std::vector<display::GammaRampRGBEntry> default_gamma_lut_;
-  std::vector<display::GammaRampRGBEntry> default_degamma_lut_;
-
   SEQUENCE_CHECKER(sequence_checker_);
   scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
 

ash/display/display_color_manager_unittest.cc

@@ -26,9 +26,7 @@ namespace ash {
 namespace {
 
 constexpr gfx::Size kDisplaySize(1024, 768);
-const char kResetGammaAction[] =
-    "*set_gamma_correction(id=123,degamma[0]*degamma[511]*,"
-    "gamma[0]*gamma[511]*)";
+const char kResetGammaAction[] = "*set_gamma_correction(id=123)";
 const char kSetGammaAction[] =
     "*set_gamma_correction(id=123,gamma[0]*gamma[255]=???????????\?)";
 const char kSetFullCTMAction[] =

ash/system/night_light/night_light_controller.cc

@@ -27,8 +27,10 @@
 #include "ui/compositor/layer.h"
 #include "ui/compositor/scoped_animation_duration_scale_mode.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
+#include "ui/display/manager/display_configurator.h"
 #include "ui/display/manager/display_manager.h"
 #include "ui/display/types/display_constants.h"
+#include "ui/display/types/display_snapshot.h"
 #include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/animation/linear_animation.h"
 
@@ -145,6 +147,32 @@ void UpdateCompositorMatrix(aura::WindowTreeHost* host,
   }
 }
 
+// Attempts setting one of the given color matrices on the display hardware of
+// |display_id| depending on the hardware capability. The matrix
+// |linear_gamma_space_matrix| will be applied if the hardware applies the
+// CTM in the linear gamma space (i.e. after gamma decoding), whereas the
+// matrix |gamma_compressed_matrix| will be applied instead if the hardware
+// applies the CTM in the gamma compressed space (i.e. after degamma
+// encoding).
+// Returns true if the hardware supports this operation and one of the
+// matrices was successfully sent to the GPU.
+bool AttemptSettingHardwareCtm(int64_t display_id,
+                               const SkMatrix44& linear_gamma_space_matrix,
+                               const SkMatrix44& gamma_compressed_matrix) {
+  for (const auto* snapshot :
+       Shell::Get()->display_configurator()->cached_displays()) {
+    if (snapshot->display_id() == display_id &&
+        snapshot->has_color_correction_matrix()) {
+      return Shell::Get()->display_color_manager()->SetDisplayColorMatrix(
+          snapshot, snapshot->color_correction_in_linear_space()
+                        ? linear_gamma_space_matrix
+                        : gamma_compressed_matrix);
+    }
+  }
+
+  return false;
+}
+
 // Applies the given |temperature| to the display associated with the given
 // |host|. This is useful for when we have a host and not a display ID.
 void ApplyTemperatureToHost(aura::WindowTreeHost* host, float temperature) {
@@ -162,11 +190,11 @@ void ApplyTemperatureToHost(aura::WindowTreeHost* host, float temperature) {
 
   const SkMatrix44 linear_gamma_space_matrix =
       MatrixFromTemperature(temperature, true);
-  const bool crtc_result =
-      Shell::Get()->display_color_manager()->SetDisplayColorMatrix(
-          display_id, linear_gamma_space_matrix);
-  UpdateCompositorMatrix(host, MatrixFromTemperature(temperature, false),
-                         crtc_result);
+  const SkMatrix44 gamma_compressed_matrix =
+      MatrixFromTemperature(temperature, false);
+  const bool crtc_result = AttemptSettingHardwareCtm(
+      display_id, linear_gamma_space_matrix, gamma_compressed_matrix);
+  UpdateCompositorMatrix(host, gamma_compressed_matrix, crtc_result);
 }
 
 // Applies the given |temperature| value by converting it to the corresponding
@@ -178,14 +206,13 @@ void ApplyTemperatureToAllDisplays(float temperature) {
       MatrixFromTemperature(temperature, false);
 
   Shell* shell = Shell::Get();
-  DisplayColorManager* color_manager = shell->display_color_manager();
   WindowTreeHostManager* wth_manager = shell->window_tree_host_manager();
   for (int64_t display_id :
        shell->display_manager()->GetCurrentDisplayIdList()) {
     DCHECK_NE(display_id, display::kUnifiedDisplayId);
 
-    const bool crtc_result = color_manager->SetDisplayColorMatrix(
-        display_id, linear_gamma_space_matrix);
+    const bool crtc_result = AttemptSettingHardwareCtm(
+        display_id, linear_gamma_space_matrix, gamma_compressed_matrix);
 
     aura::Window* root_window =
         wth_manager->GetRootWindowForDisplayId(display_id);
@@ -315,7 +342,7 @@ float NightLightController::GreenColorScaleFromTemperature(
   // If we only tone down the blue scale, the screen will look very green so
   // we also need to tone down the green, but with a less value compared to
   // the blue scale to avoid making things look very red.
-  return 1.0f - (in_linear_space ? 0.7f : 0.4f) * temperature;
+  return 1.0f - (in_linear_space ? 0.7f : 0.5f) * temperature;
 }
 
 // static

ash/system/night_light/night_light_controller.h

@@ -102,12 +102,14 @@ class ASH_EXPORT NightLightController
   static float GreenColorScaleFromTemperature(float temperature,
                                               bool in_linear_space);
 
-  // When using the CRTC color correction, the matrix is applied in the linear
-  // color space (i.e. after gamma decoding). Our standard temperature we use
-  // here, which the user changes, follow a linear slope from 0.0f to 1.0f. This
-  // won't give the same linear rate of change in colors as the temperature
-  // changes in the linear color space. To account for this, we want the
-  // temperature to follow the same slope as that of the gamma factor.
+  // When using the CRTC color correction, depending on the hardware, the matrix
+  // may be applied in the linear gamma space (i.e. after gamma decoding), or in
+  // the non-linear gamma compressed space (i.e. after degamma encoding). Our
+  // standard temperature we use here, which the user changes, follow a linear
+  // slope from 0.0f to 1.0f. This won't give the same linear rate of change in
+  // colors as the temperature changes in the linear color space. To account for
+  // this, we want the temperature to follow the same slope as that of the gamma
+  // factor.
   // This function returns the non-linear temperature that corresponds to the
   // linear |temperature| value.
   static float GetNonLinearTemperature(float temperature);

ui/display/manager/fake_display_snapshot.cc

@@ -226,6 +226,11 @@ Builder& Builder::SetHasColorCorrectionMatrix(bool val) {
   return *this;
 }
 
+Builder& Builder::SetColorCorrectionInLinearSpace(bool val) {
+  color_correction_in_linear_space_ = val;
+  return *this;
+}
+
 Builder& Builder::SetName(const std::string& name) {
   name_ = name;
   return *this;

ui/display/manager/fake_display_snapshot.h

@@ -58,6 +58,7 @@ class DISPLAY_MANAGER_EXPORT FakeDisplaySnapshot : public DisplaySnapshot {
     Builder& SetIsAspectPerservingScaling(bool is_aspect_preserving_scaling);
     Builder& SetHasOverscan(bool has_overscan);
     Builder& SetHasColorCorrectionMatrix(bool val);
+    Builder& SetColorCorrectionInLinearSpace(bool val);
     Builder& SetName(const std::string& name);
     Builder& SetProductCode(int64_t product_code);
     Builder& SetMaximumCursorSize(const gfx::Size& maximum_cursor_size);