Make DarkModeFilter image operations thread-safe.

This patch makes image operations thread-safe in dark mode filter.
The data members used for image operations are made read-only.

Bug: 1094005
Change-Id: Ib4d31d824e04d54b2d681c45012869f4a4d1a738
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2465886
Commit-Queue: Prashant Nevase <prashant.n@samsung.com>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817319}

third_party/blink/renderer/platform/graphics/dark_mode_filter.cc

@@ -63,50 +63,44 @@ class DarkModeInvertedColorCache {
   WTF::LruCache<WTF::IntegralWithAllKeys<SkColor>, SkColor> cache_;
 };
 
-DarkModeFilter::DarkModeFilter()
-    : text_classifier_(nullptr),
-      background_classifier_(nullptr),
-      image_classifier_(nullptr),
-      color_filter_(nullptr),
-      image_filter_(nullptr),
-      inverted_color_cache_(new DarkModeInvertedColorCache()) {
-  DarkModeSettings default_settings;
-  UpdateSettings(default_settings);
-}
+DarkModeFilter::DarkModeFilter(const DarkModeSettings& settings)
+    : immutable_(settings),
+      inverted_color_cache_(new DarkModeInvertedColorCache()) {}
 
 DarkModeFilter::~DarkModeFilter() {}
 
-void DarkModeFilter::UpdateSettings(const DarkModeSettings& new_settings) {
-  inverted_color_cache_->Clear();
-
-  settings_ = new_settings;
-  color_filter_ = DarkModeColorFilter::FromSettings(settings_);
-  if (!color_filter_) {
-    image_filter_ = nullptr;
+DarkModeFilter::ImmutableData::ImmutableData(const DarkModeSettings& settings)
+    : settings(settings),
+      text_classifier(nullptr),
+      background_classifier(nullptr),
+      image_classifier(nullptr),
+      color_filter(nullptr),
+      image_filter(nullptr) {
+  color_filter = DarkModeColorFilter::FromSettings(settings);
+  if (!color_filter)
     return;
-  }
 
-  if (settings_.image_grayscale_percent > 0.0f)
-    image_filter_ = MakeGrayscaleFilter(settings_.image_grayscale_percent);
+  if (settings.image_grayscale_percent > 0.0f)
+    image_filter = MakeGrayscaleFilter(settings.image_grayscale_percent);
   else
-    image_filter_ = color_filter_->ToSkColorFilter();
+    image_filter = color_filter->ToSkColorFilter();
 
-  text_classifier_ =
-      DarkModeColorClassifier::MakeTextColorClassifier(settings_);
-  background_classifier_ =
-      DarkModeColorClassifier::MakeBackgroundColorClassifier(settings_);
-  image_classifier_ = std::make_unique<DarkModeImageClassifier>();
+  text_classifier = DarkModeColorClassifier::MakeTextColorClassifier(settings);
+  background_classifier =
+      DarkModeColorClassifier::MakeBackgroundColorClassifier(settings);
+  image_classifier = std::make_unique<DarkModeImageClassifier>();
 }
 
 SkColor DarkModeFilter::InvertColorIfNeeded(SkColor color, ElementRole role) {
-  if (!color_filter_)
+  if (!immutable_.color_filter)
     return color;
 
   if (role_override_.has_value())
     role = role_override_.value();
 
   if (ShouldApplyToColor(color, role)) {
-    return inverted_color_cache_->GetInvertedColor(color_filter_.get(), color);
+    return inverted_color_cache_->GetInvertedColor(
+        immutable_.color_filter.get(), color);
   }
 
   return color;
@@ -115,10 +109,10 @@ SkColor DarkModeFilter::InvertColorIfNeeded(SkColor color, ElementRole role) {
 DarkModeResult DarkModeFilter::AnalyzeShouldApplyToImage(
     const SkIRect& src,
     const SkIRect& dst) const {
-  if (settings().image_policy == DarkModeImagePolicy::kFilterNone)
+  if (immutable_.settings.image_policy == DarkModeImagePolicy::kFilterNone)
     return DarkModeResult::kDoNotApplyFilter;
 
-  if (settings().image_policy == DarkModeImagePolicy::kFilterAll)
+  if (immutable_.settings.image_policy == DarkModeImagePolicy::kFilterAll)
     return DarkModeResult::kApplyFilter;
 
   // Images being drawn from very smaller |src| rect, i.e. one of the dimensions
@@ -140,25 +134,25 @@ sk_sp<SkColorFilter> DarkModeFilter::ApplyToImage(const SkPixmap& pixmap,
                                                   const SkIRect& src,
                                                   const SkIRect& dst) const {
   DCHECK(AnalyzeShouldApplyToImage(src, dst) == DarkModeResult::kNotClassified);
-  DCHECK(settings().image_policy == DarkModeImagePolicy::kFilterSmart);
-  DCHECK(image_filter_);
+  DCHECK(immutable_.settings.image_policy == DarkModeImagePolicy::kFilterSmart);
+  DCHECK(immutable_.image_filter);
 
-  return (image_classifier_->Classify(pixmap, src) ==
+  return (immutable_.image_classifier->Classify(pixmap, src) ==
           DarkModeResult::kApplyFilter)
-             ? image_filter_
+             ? immutable_.image_filter
              : nullptr;
 }
 
 sk_sp<SkColorFilter> DarkModeFilter::GetImageFilter() const {
-  DCHECK(settings().image_policy == DarkModeImagePolicy::kFilterAll);
-  DCHECK(image_filter_);
-  return image_filter_;
+  DCHECK(immutable_.settings.image_policy == DarkModeImagePolicy::kFilterAll);
+  DCHECK(immutable_.image_filter);
+  return immutable_.image_filter;
 }
 
 base::Optional<cc::PaintFlags> DarkModeFilter::ApplyToFlagsIfNeeded(
     const cc::PaintFlags& flags,
     ElementRole role) {
-  if (!color_filter_)
+  if (!immutable_.color_filter)
     return base::nullopt;
 
   if (role_override_.has_value())
@@ -166,10 +160,10 @@ base::Optional<cc::PaintFlags> DarkModeFilter::ApplyToFlagsIfNeeded(
 
   cc::PaintFlags dark_mode_flags = flags;
   if (flags.HasShader()) {
-    dark_mode_flags.setColorFilter(color_filter_->ToSkColorFilter());
+    dark_mode_flags.setColorFilter(immutable_.color_filter->ToSkColorFilter());
   } else if (ShouldApplyToColor(flags.getColor(), role)) {
     dark_mode_flags.setColor(inverted_color_cache_->GetInvertedColor(
-        color_filter_.get(), flags.getColor()));
+        immutable_.color_filter.get(), flags.getColor()));
   }
 
   return base::make_optional<cc::PaintFlags>(std::move(dark_mode_flags));
@@ -178,19 +172,19 @@ base::Optional<cc::PaintFlags> DarkModeFilter::ApplyToFlagsIfNeeded(
 bool DarkModeFilter::ShouldApplyToColor(SkColor color, ElementRole role) {
   switch (role) {
     case ElementRole::kText:
-      DCHECK(text_classifier_);
-      return text_classifier_->ShouldInvertColor(color) ==
+      DCHECK(immutable_.text_classifier);
+      return immutable_.text_classifier->ShouldInvertColor(color) ==
              DarkModeResult::kApplyFilter;
     case ElementRole::kListSymbol:
-      // TODO(prashant.n): Rename text_classifier_ to foreground_classifier_,
+      // TODO(prashant.n): Rename text_classifier to foreground_classifier,
       // so that same classifier can be used for all roles which are supposed
       // to be at foreground.
-      DCHECK(text_classifier_);
-      return text_classifier_->ShouldInvertColor(color) ==
+      DCHECK(immutable_.text_classifier);
+      return immutable_.text_classifier->ShouldInvertColor(color) ==
              DarkModeResult::kApplyFilter;
     case ElementRole::kBackground:
-      DCHECK(background_classifier_);
-      return background_classifier_->ShouldInvertColor(color) ==
+      DCHECK(immutable_.background_classifier);
+      return immutable_.background_classifier->ShouldInvertColor(color) ==
              DarkModeResult::kApplyFilter;
     case ElementRole::kSVG:
       // 1) Inline SVG images are considered as individual shapes and do not

third_party/blink/renderer/platform/graphics/dark_mode_filter.h

@@ -27,12 +27,9 @@ class PLATFORM_EXPORT DarkModeFilter {
  public:
   // Dark mode is disabled by default. Enable it by calling UpdateSettings()
   // with a mode other than DarkMode::kOff.
-  DarkModeFilter();
+  explicit DarkModeFilter(const DarkModeSettings& settings);
   ~DarkModeFilter();
 
-  const DarkModeSettings& settings() const { return settings_; }
-  void UpdateSettings(const DarkModeSettings& new_settings);
-
   // TODO(gilmanmh): Add a role for shadows. In general, we don't want to
   // invert shadows, but we may need to do some other kind of processing for
   // them.
@@ -43,12 +40,15 @@ class PLATFORM_EXPORT DarkModeFilter {
       const cc::PaintFlags& flags,
       ElementRole element_role);
 
+  size_t GetInvertedColorCacheSizeForTesting();
+
   // Decides whether to apply dark mode or not based on |src| and |dst|.
   // DarkModeResult::kDoNotApplyFilter - Dark mode filter should not be applied.
   // DarkModeResult::kApplyFilter - Dark mode filter should be applied and to
   // get the color filter GetImageFilter() should be called.
   // DarkModeResult::kNotClassified - Dark mode filter should be applied and to
-  // get the color filter ApplyToImage() should be called.
+  // get the color filter ApplyToImage() should be called. This API is
+  // thread-safe.
   DarkModeResult AnalyzeShouldApplyToImage(const SkIRect& src,
                                            const SkIRect& dst) const;
 
@@ -57,7 +57,8 @@ class PLATFORM_EXPORT DarkModeFilter {
   // empty or |src| is larger than pixmap bounds. Before calling this function
   // AnalyzeShouldApplyToImage() must be called for early out or deciding
   // appropriate function call. This function should be called only if image
-  // policy is set to DarkModeImagePolicy::kFilterSmart.
+  // policy is set to DarkModeImagePolicy::kFilterSmart. This API is
+  // thread-safe.
   sk_sp<SkColorFilter> ApplyToImage(const SkPixmap& pixmap,
                                     const SkIRect& src,
                                     const SkIRect& dst) const;
@@ -65,25 +66,31 @@ class PLATFORM_EXPORT DarkModeFilter {
   // Returns dark mode color filter for images. Before calling this function
   // AnalyzeShouldApplyToImage() must be called for early out or deciding
   // appropriate function call. This function should be called only if image
-  // policy is set to DarkModeImagePolicy::kFilterAll.
+  // policy is set to DarkModeImagePolicy::kFilterAll. This API is thread-safe.
   sk_sp<SkColorFilter> GetImageFilter() const;
-
-  size_t GetInvertedColorCacheSizeForTesting();
-
  private:
   friend class ScopedDarkModeElementRoleOverride;
 
-  DarkModeSettings settings_;
+  struct ImmutableData {
+    explicit ImmutableData(const DarkModeSettings& settings);
+
+    DarkModeSettings settings;
+    std::unique_ptr<DarkModeColorClassifier> text_classifier;
+    std::unique_ptr<DarkModeColorClassifier> background_classifier;
+    std::unique_ptr<DarkModeImageClassifier> image_classifier;
+    std::unique_ptr<DarkModeColorFilter> color_filter;
+    sk_sp<SkColorFilter> image_filter;
+  };
 
   bool ShouldApplyToColor(SkColor color, ElementRole role);
 
-  std::unique_ptr<DarkModeColorClassifier> text_classifier_;
-  std::unique_ptr<DarkModeColorClassifier> background_classifier_;
-  std::unique_ptr<DarkModeImageClassifier> image_classifier_;
+  // This is read-only data and is thread-safe.
+  const ImmutableData immutable_;
 
-  std::unique_ptr<DarkModeColorFilter> color_filter_;
-  sk_sp<SkColorFilter> image_filter_;
+  // Following two members used for color classifications are not thread-safe.
+  // TODO(prashant.n): Remove element override concept.
   base::Optional<ElementRole> role_override_;
+  // TODO(prashant.n): Move cache out of dark mode filter.
   std::unique_ptr<DarkModeInvertedColorCache> inverted_color_cache_;
 };
 

third_party/blink/renderer/platform/graphics/dark_mode_filter_test.cc

@@ -14,11 +14,9 @@ namespace blink {
 namespace {
 
 TEST(DarkModeFilterTest, ApplyDarkModeToColorsAndFlags) {
-  DarkModeFilter filter;
-
   DarkModeSettings settings;
   settings.mode = DarkModeInversionAlgorithm::kSimpleInvertForTesting;
-  filter.UpdateSettings(settings);
+  DarkModeFilter filter(settings);
 
   EXPECT_EQ(SK_ColorBLACK,
             filter.InvertColorIfNeeded(
@@ -43,11 +41,9 @@ TEST(DarkModeFilterTest, ApplyDarkModeToColorsAndFlags) {
 }
 
 TEST(DarkModeFilterTest, InvertedColorCacheSize) {
-  DarkModeFilter filter;
   DarkModeSettings settings;
-
   settings.mode = DarkModeInversionAlgorithm::kSimpleInvertForTesting;
-  filter.UpdateSettings(settings);
+  DarkModeFilter filter(settings);
   EXPECT_EQ(0u, filter.GetInvertedColorCacheSizeForTesting());
   EXPECT_EQ(SK_ColorBLACK,
             filter.InvertColorIfNeeded(
@@ -58,18 +54,12 @@ TEST(DarkModeFilterTest, InvertedColorCacheSize) {
             filter.InvertColorIfNeeded(
                 SK_ColorWHITE, DarkModeFilter::ElementRole::kBackground));
   EXPECT_EQ(1u, filter.GetInvertedColorCacheSizeForTesting());
-
-  // On changing DarkModeSettings, cache should be reset.
-  settings.mode = DarkModeInversionAlgorithm::kInvertLightness;
-  filter.UpdateSettings(settings);
-  EXPECT_EQ(0u, filter.GetInvertedColorCacheSizeForTesting());
 }
 
 TEST(DarkModeFilterTest, InvertedColorCacheZeroMaxKeys) {
-  DarkModeFilter filter;
   DarkModeSettings settings;
   settings.mode = DarkModeInversionAlgorithm::kSimpleInvertForTesting;
-  filter.UpdateSettings(settings);
+  DarkModeFilter filter(settings);
 
   EXPECT_EQ(0u, filter.GetInvertedColorCacheSizeForTesting());
   EXPECT_EQ(SK_ColorBLACK,
@@ -92,11 +82,10 @@ TEST(DarkModeFilterTest, InvertedColorCacheZeroMaxKeys) {
 }
 
 TEST(DarkModeFilterTest, AnalyzeShouldApplyToImage) {
-  DarkModeFilter filter;
   DarkModeSettings settings;
   settings.mode = DarkModeInversionAlgorithm::kSimpleInvertForTesting;
   settings.image_policy = DarkModeImagePolicy::kFilterSmart;
-  filter.UpdateSettings(settings);
+  DarkModeFilter filter(settings);
 
   // |dst| is smaller than threshold size.
   EXPECT_EQ(filter.AnalyzeShouldApplyToImage(SkIRect::MakeWH(100, 100),

third_party/blink/renderer/platform/graphics/graphics_context.cc

@@ -154,8 +154,8 @@ GraphicsContext::~GraphicsContext() {
 
 DarkModeFilter* GraphicsContext::GetDarkModeFilter() {
   if (!dark_mode_filter_) {
-    dark_mode_filter_ = std::make_unique<DarkModeFilter>();
-    dark_mode_filter_->UpdateSettings(GetCurrentDarkModeSettings());
+    dark_mode_filter_ =
+        std::make_unique<DarkModeFilter>(GetCurrentDarkModeSettings());
   }
 
   return dark_mode_filter_.get();
@@ -163,7 +163,7 @@ DarkModeFilter* GraphicsContext::GetDarkModeFilter() {
 
 void GraphicsContext::UpdateDarkModeSettingsForTest(
     const DarkModeSettings& settings) {
-  GetDarkModeFilter()->UpdateSettings(settings);
+  dark_mode_filter_ = std::make_unique<DarkModeFilter>(settings);
 }
 
 void GraphicsContext::Save() {

third_party/blink/renderer/platform/graphics/raster_dark_mode_filter_impl.cc

@@ -11,8 +11,7 @@ namespace blink {
 
 RasterDarkModeFilterImpl::RasterDarkModeFilterImpl(
     const DarkModeSettings& settings) {
-  dark_mode_filter_ = std::make_unique<DarkModeFilter>();
-  dark_mode_filter_->UpdateSettings(settings);
+  dark_mode_filter_ = std::make_unique<DarkModeFilter>(settings);
 }
 
 cc::RasterDarkModeFilter::Result