[Text Contrast] Plumb values into Skia

This is the final change to enable contrast and gamma values from the
registry to get plumbed into Skia. This is not currently enabled
for web_tests - instead command line values are used to verify
that the text contrast has changed. A follow-up will enable registry
values for web_tests.

Values for contrast and gamma are stored on FontRenderParams, and
plumbed into Skia via LegacyDisplayGlobals. Tests were added for
rendering via a new VirtualTestSuite, and since this goes through
a different path than the browser, additional tests were added for
plumbing FontRenderParams values.

Two new command line flags are added here, for setting contrast
and gamma respectively. These are used in the virtual test suite
but also apply to the browser.

Bug: 1160653
Change-Id: I892a17dae636f2779c13dc3bba1bb81dc97dbff9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5283107
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Commit-Queue: Kurt Catti-Schmidt <kschmi@microsoft.com>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1272465}

content/browser/BUILD.gn

@@ -300,6 +300,7 @@ source_set("browser") {
     "//content/browser/indexed_db",
     "//ipc",
     "//media/mojo/mojom:remoting",
+    "//skia:skia_switches",
     "//third_party/blink/public/mojom:embedded_frame_sink_mojo_bindings",
     "//third_party/leveldatabase",
     "//ui/base/cursor",

content/browser/renderer_host/render_process_host_impl.cc

@@ -194,6 +194,7 @@
 #include "services/network/public/mojom/network_service.mojom.h"
 #include "services/resource_coordinator/public/mojom/memory_instrumentation/memory_instrumentation.mojom.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
+#include "skia/ext/switches.h"
 #include "storage/browser/database/database_tracker.h"
 #include "storage/browser/quota/quota_manager.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_registry.h"
@@ -3369,16 +3370,16 @@ void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
   // Propagate the following switches to the renderer command line (along
   // with any associated values) if present in the browser command line.
   static const char* const kSwitchNames[] = {
-    switches::kDisableInProcessStackTraces,
-    sandbox::policy::switches::kDisableSeccompFilterSandbox,
-    sandbox::policy::switches::kNoSandbox,
+      switches::kDisableInProcessStackTraces,
+      sandbox::policy::switches::kDisableSeccompFilterSandbox,
+      sandbox::policy::switches::kNoSandbox,
 #if BUILDFLAG(IS_LINUX) && !BUILDFLAG(IS_CHROMEOS_ASH) && \
     !BUILDFLAG(IS_CHROMEOS_LACROS)
-    switches::kDisableDevShmUsage,
+      switches::kDisableDevShmUsage,
 #endif
 #if BUILDFLAG(IS_MAC)
-    // Allow this to be set when invoking the browser and relayed along.
-    sandbox::policy::switches::kEnableSandboxLogging,
+      // Allow this to be set when invoking the browser and relayed along.
+      sandbox::policy::switches::kEnableSandboxLogging,
 #endif
     switches::kAllowCommandLinePlugins,
     switches::kAllowLoopbackInPeerConnection,
@@ -3546,6 +3547,8 @@ void RenderProcessHostImpl::PropagateBrowserCommandLineToRenderer(
 #endif
 #if BUILDFLAG(IS_WIN)
       switches::kDisableHighResTimer,
+      switches::kTextContrast,
+      switches::kTextGamma,
       switches::kTrySupportedChannelLayouts,
       switches::kRaiseTimerFrequency,
 #endif

content/renderer/render_view_browsertest.cc

@@ -25,6 +25,7 @@
 #include "base/task/single_thread_task_runner.h"
 #include "base/test/bind.h"
 #include "base/test/gmock_callback_support.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "build/build_config.h"
@@ -32,6 +33,7 @@
 #include "cc/trees/layer_tree_host.h"
 #include "content/common/renderer.mojom.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/renderer_preferences_util.h"
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/browser/web_ui_controller_factory.h"
 #include "content/public/common/bindings_policy.h"
@@ -66,6 +68,7 @@
 #include "net/dns/public/resolve_error_info.h"
 #include "net/http/http_util.h"
 #include "services/network/public/cpp/resource_request_body.h"
+#include "skia/ext/legacy_display_globals.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/common/dom_storage/session_storage_namespace_id.h"
@@ -108,6 +111,7 @@
 #include "third_party/blink/public/web/web_window_features.h"
 #include "ui/accessibility/ax_mode.h"
 #include "ui/base/ime/mojom/text_input_state.mojom.h"
+#include "ui/base/ui_base_features.h"
 #include "ui/events/base_event_utils.h"
 #include "ui/events/event.h"
 #include "ui/events/keycodes/dom/dom_code.h"
@@ -3320,4 +3324,51 @@ TEST_F(RenderViewImplTest, CollapseSelectionNotChangeFocus) {
   EXPECT_EQ(1, is_body_again);
 }
 
+#if BUILDFLAG(IS_WIN)
+class RenderViewImplContrastGammaSettingsTest : public RenderViewImplTest {
+ protected:
+  void SetUp() override {
+    RenderViewImplTest::SetUp();
+    feature_list_.InitAndEnableFeature(
+        features::kUseGammaContrastRegistrySettings);
+  }
+  base::test::ScopedFeatureList feature_list_;
+};
+
+TEST_F(RenderViewImplContrastGammaSettingsTest,
+       ContrastGammaSetRendererPreferences) {
+  LoadHTML(R"HTML(
+      <input id='test' type='text'></input>
+    )HTML");
+
+  // Use non-default values for contrast and gamma.
+  constexpr float test_contrast = 0.95;
+  static_assert(test_contrast != SK_GAMMA_CONTRAST);
+  static_assert(test_contrast >= SkSurfaceProps::kMinContrastInclusive);
+  static_assert(test_contrast <= SkSurfaceProps::kMaxContrastInclusive);
+
+  constexpr float test_gamma = 3.99;
+  static_assert(test_gamma != SK_GAMMA_EXPONENT);
+  static_assert(test_gamma >= SkSurfaceProps::kMinGammaInclusive);
+  static_assert(test_gamma < SkSurfaceProps::kMaxGammaExclusive);
+
+  blink::RendererPreferences renderer_preferences =
+      web_view_->GetRendererPreferences();
+  EXPECT_NE(renderer_preferences.text_contrast, test_contrast);
+  EXPECT_NE(renderer_preferences.text_gamma, test_gamma);
+
+  // Set the non-default values on `RendererPreferences`.
+  renderer_preferences.text_contrast = test_contrast;
+  renderer_preferences.text_gamma = test_gamma;
+  web_view_->SetRendererPreferences(renderer_preferences);
+
+  // `GetSkSurfaceProps` should have the updated contrast and
+  // gamma properties from above.
+  SkSurfaceProps surface_props =
+      skia::LegacyDisplayGlobals::GetSkSurfaceProps();
+  EXPECT_EQ(surface_props.textContrast(), test_contrast);
+  EXPECT_EQ(surface_props.textGamma(), test_gamma);
+}
+#endif  // BUILDFLAG(IS_WIN)
+
 }  // namespace content

skia/BUILD.gn

@@ -484,11 +484,13 @@ component("skia") {
   deps += [
     ":skcms",
     ":skia_opts",
+    ":skia_switches",
     "//base",
     "//base/third_party/dynamic_annotations",
     "//third_party/libpng",
     "//third_party/libwebp",
     "//third_party/libwebp:libwebp_webp",
+    "//ui/base:features",
   ]
   public_deps = [
     ":buildflags",
@@ -606,6 +608,18 @@ component("skia") {
   }
 }
 
+component("skia_switches") {
+  sources = [
+    "ext/switches.cc",
+    "ext/switches.h",
+    "ext/switches_export.h",
+  ]
+
+  defines = [ "SKIA_SWITCHES_IMPLEMENTATION" ]
+
+  deps = [ "//base" ]
+}
+
 # Template for things that are logically part of :skia, but need to be split out
 # so custom compile flags can be applied.
 #

skia/ext/legacy_display_globals.cc

@@ -4,8 +4,13 @@
 
 #include "skia/ext/legacy_display_globals.h"
 
+#include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/no_destructor.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/synchronization/lock.h"
+#include "skia/ext/switches.h"
+#include "ui/base/ui_base_features.h"
 
 namespace skia {
 
@@ -21,6 +26,23 @@ base::Lock& GetLock() {
   static base::NoDestructor<base::Lock> lock;
   return *lock;
 }
+
+void GetContrastAndGammaValuesFromCommandLine(float& out_contrast,
+                                              float& out_gamma) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  std::string contrast_string =
+      command_line->GetSwitchValueASCII(switches::kTextContrast);
+  double contrast_double;
+  if (base::StringToDouble(contrast_string, &contrast_double)) {
+    out_contrast = base::checked_cast<float>(contrast_double);
+  }
+  std::string gamma_string =
+      command_line->GetSwitchValueASCII(switches::kTextGamma);
+  double gamma_double;
+  if (base::StringToDouble(gamma_string, &gamma_double)) {
+    out_gamma = base::checked_cast<float>(gamma_double);
+  }
+}
 }
 
 // static
@@ -29,8 +51,10 @@ void LegacyDisplayGlobals::SetCachedParams(SkPixelGeometry pixel_geometry,
                                            float text_gamma) {
   base::AutoLock lock(GetLock());
   g_pixel_geometry = pixel_geometry;
+#if BUILDFLAG(IS_WIN)
   g_text_contrast = text_contrast;
   g_text_gamma = text_gamma;
+#endif  // BUILDFLAG(IS_WIN)
 }
 
 // static
@@ -41,7 +65,12 @@ SkSurfaceProps LegacyDisplayGlobals::GetSkSurfaceProps() {
 // static
 SkSurfaceProps LegacyDisplayGlobals::GetSkSurfaceProps(uint32_t flags) {
   base::AutoLock lock(GetLock());
-  return SkSurfaceProps{flags, g_pixel_geometry};
+  float contrast = g_text_contrast;
+  float gamma = g_text_gamma;
+
+  // Prioritize values from the command line for testing.
+  GetContrastAndGammaValuesFromCommandLine(contrast, gamma);
+  return SkSurfaceProps{flags, g_pixel_geometry, contrast, gamma};
 }
 
 SkSurfaceProps LegacyDisplayGlobals::ComputeSurfaceProps(
@@ -50,8 +79,15 @@ SkSurfaceProps LegacyDisplayGlobals::ComputeSurfaceProps(
   if (can_use_lcd_text) {
     return LegacyDisplayGlobals::GetSkSurfaceProps(flags);
   }
+  base::AutoLock lock(GetLock());
+  float contrast = g_text_contrast;
+  float gamma = g_text_gamma;
+
+  // Prioritize values from the command line for testing.
+  GetContrastAndGammaValuesFromCommandLine(contrast, gamma);
+
   // Use unknown pixel geometry to disable LCD text.
-  return SkSurfaceProps{flags, kUnknown_SkPixelGeometry};
+  return SkSurfaceProps{flags, kUnknown_SkPixelGeometry, contrast, gamma};
 }
 
 }  // namespace skia

skia/ext/switches.cc

@@ -0,0 +1,13 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "skia/ext/switches.h"
+#include "base/component_export.h"
+
+namespace switches {
+
+const char kTextContrast[] = "text-contrast";
+const char kTextGamma[] = "text-gamma";
+
+}  // namespace switches

skia/ext/switches.h

@@ -0,0 +1,17 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SKIA_EXT_SWITCHES_H_
+#define SKIA_EXT_SWITCHES_H_
+
+#include "skia/ext/switches_export.h"
+
+namespace switches {
+
+SKIA_SWITCHES_EXPORT extern const char kTextContrast[];
+SKIA_SWITCHES_EXPORT extern const char kTextGamma[];
+
+}  // namespace switches
+
+#endif  // SKIA_EXT_SWITCHES_H_

skia/ext/switches_export.h

@@ -0,0 +1,29 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef SKIA_EXT_SWITCHES_EXPORT_H_
+#define SKIA_EXT_SWITCHES_EXPORT_H_
+
+#if defined(COMPONENT_BUILD)
+#if defined(WIN32)
+
+#if defined(SKIA_SWITCHES_IMPLEMENTATION)
+#define SKIA_SWITCHES_EXPORT __declspec(dllexport)
+#else
+#define SKIA_SWITCHES_EXPORT __declspec(dllimport)
+#endif  // defined(SKIA_SWITCHES_IMPLEMENTATION)
+
+#else  // defined(WIN32)
+#if defined(SKIA_SWITCHES_IMPLEMENTATION)
+#define SKIA_SWITCHES_EXPORT __attribute__((visibility("default")))
+#else
+#define SKIA_SWITCHES_EXPORT
+#endif
+#endif
+
+#else  // defined(COMPONENT_BUILD)
+#define SKIA_SWITCHES_EXPORT
+#endif
+
+#endif  // SKIA_EXT_SWITCHES_EXPORT_H_

third_party/blink/common/renderer_preferences/renderer_preferences_mojom_traits.cc

@@ -28,6 +28,11 @@ bool StructTraits<blink::mojom::RendererPreferencesDataView,
     return false;
   out->use_subpixel_positioning = data.use_subpixel_positioning();
 
+#if BUILDFLAG(IS_WIN)
+  out->text_contrast = data.text_contrast();
+  out->text_gamma = data.text_gamma();
+#endif  // BUILDFLAG(IS_WIN)
+
   out->focus_ring_color = data.focus_ring_color();
   out->active_selection_bg_color = data.active_selection_bg_color();
   out->active_selection_fg_color = data.active_selection_fg_color();

third_party/blink/web_tests/VirtualTestSuites

@@ -1048,6 +1048,7 @@
       "virtual/text-antialias/basic/014.html"
     ],
     "args": ["--enable-font-antialiasing", "--enable-features=UseGammaContrastRegistrySettings",
+             "--text-contrast=0.97", "--text-gamma=1.97",
              "--disable-threaded-compositing", "--disable-threaded-animation"],
     "expires": "August 1, 2024"
   },

ui/gfx/font_render_params_win.cc

@@ -12,6 +12,7 @@
 #include "base/functional/callback_helpers.h"
 #include "base/memory/singleton.h"
 #include "base/win/registry.h"
+#include "skia/ext/legacy_display_globals.h"
 #include "ui/base/ui_base_features.h"
 #include "ui/gfx/font_util_win.h"
 #include "ui/gfx/win/singleton_hwnd_observer.h"
@@ -95,7 +96,10 @@ class CachedFontRenderParams {
       params_->text_gamma = SK_GAMMA_EXPONENT;
     }
 
-    // TODO(kschmi): set contrast and gamma values via `LegacyDisplayGlobals`.
+    skia::LegacyDisplayGlobals::SetCachedParams(
+        FontRenderParams::SubpixelRenderingToSkiaPixelGeometry(
+            params_->subpixel_rendering),
+        params_->text_contrast, params_->text_gamma);
 
     singleton_hwnd_observer_ =
         std::make_unique<SingletonHwndObserver>(base::BindRepeating(

ui/gfx/font_render_params_win_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/check_op.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_reg_util_win.h"
+#include "skia/ext/legacy_display_globals.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/ui_base_features.h"
 #include "ui/gfx/font_util_win.h"
@@ -62,8 +63,13 @@ TEST_F(FontRenderParamsTest, DefaultRegistryState) {
     DWORD gamma;
     ASSERT_EQ(key.ReadValueDW(L"GammaLevel", &gamma), ERROR_SUCCESS);
 
-    EXPECT_FLOAT_EQ(params.text_contrast * kContrastMultiplier, contrast);
-    EXPECT_FLOAT_EQ(params.text_gamma * kGammaMultiplier, gamma);
+    // Registry values are unbounded, however our code will clamp values to be
+    // within Skia's expected range, so we must clamp expected values to match.
+    // Handle the exclusive value by subtracting epsilon.
+    EXPECT_FLOAT_EQ(params.text_contrast * kContrastMultiplier,
+                    FontUtilWin::ClampContrast(contrast));
+    EXPECT_FLOAT_EQ(params.text_gamma * kGammaMultiplier,
+                    FontUtilWin::ClampGamma(contrast));
   } else {
     // If the registry keys aren't set, `IDWriteRenderingParams` defaults
     // to the following hard-coded values:
@@ -72,6 +78,12 @@ TEST_F(FontRenderParamsTest, DefaultRegistryState) {
     EXPECT_FLOAT_EQ(params.text_contrast, default_contrast);
     EXPECT_FLOAT_EQ(params.text_gamma, default_gamma);
   }
+
+  // Values from `LegacyDisplayGlobals` should match `FontRenderParams`.
+  SkSurfaceProps surface_props =
+      skia::LegacyDisplayGlobals::GetSkSurfaceProps();
+  EXPECT_EQ(surface_props.textContrast(), params.text_contrast);
+  EXPECT_EQ(surface_props.textGamma(), params.text_gamma);
 }
 
 TEST_F(FontRenderParamsTest, OverrideRegistryValues) {

ui/gfx/font_util_win.cc

@@ -8,6 +8,7 @@
 #include <wrl/client.h>
 
 #include "base/files/file_path.h"
+#include "third_party/skia/include/core/SkSurfaceProps.h"
 #include "third_party/skia/include/core/SkTypes.h"
 #include "ui/gfx/win/direct_write.h"
 
@@ -29,8 +30,9 @@ TextParameters GetTextParameters() {
       // We only support the primary device currently.
       Microsoft::WRL::ComPtr<IDWriteRenderingParams> textParams;
       if (SUCCEEDED(factory->CreateRenderingParams(&textParams))) {
-        text_parameters.contrast = textParams->GetEnhancedContrast();
-        text_parameters.gamma = textParams->GetGamma();
+        text_parameters.contrast =
+            FontUtilWin::ClampContrast(textParams->GetEnhancedContrast());
+        text_parameters.gamma = FontUtilWin::ClampGamma(textParams->GetGamma());
       } else {
         text_parameters.contrast = SK_GAMMA_CONTRAST;
         text_parameters.gamma = SK_GAMMA_EXPONENT;
@@ -58,6 +60,20 @@ base::win::RegKey FontUtilWin::GetTextSettingsRegistryKey(REGSAM access) {
   return base::win::RegKey{};
 }
 
+// static
+float FontUtilWin::ClampContrast(float value) {
+  return std::clamp(value, SkSurfaceProps::kMinContrastInclusive,
+                    SkSurfaceProps::kMaxContrastInclusive);
+}
+
+// static
+float FontUtilWin::ClampGamma(float value) {
+  // Handle the exclusive max by subtracting espilon.
+  return std::clamp(value, SkSurfaceProps::kMinGammaInclusive,
+                    SkSurfaceProps::kMaxGammaExclusive -
+                        std::numeric_limits<float>::epsilon());
+}
+
 // static
 float FontUtilWin::GetContrastFromRegistry() {
   return GetTextParameters().contrast;

ui/gfx/font_util_win.h

@@ -12,6 +12,8 @@ namespace gfx {
 class FontUtilWin {
  public:
   static base::win::RegKey GetTextSettingsRegistryKey(REGSAM access = KEY_READ);
+  static float ClampContrast(float value);
+  static float ClampGamma(float value);
   static float GetContrastFromRegistry();
   static float GetGammaFromRegistry();
 };

ui/gfx/switches.cc

@@ -38,7 +38,7 @@ const char kHeadless[] = "headless";
 const char kX11Display[] = "display";
 // Disables MIT-SHM extension. In use only with Ozone/X11.
 const char kNoXshm[] = "no-xshm";
-#endif
+#endif  // BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS)
 
 }  // namespace switches