Color: Don't duplicate ICC profile data

ui/gfx/color_space.cc

Index: ui/gfx/color_space.cc
diff --git a/ui/gfx/color_space.cc b/ui/gfx/color_space.cc
index 7650b1e3a9d274e45bbb538fb08041a19607d863..b7d27b32df4a89e600f724eade21d09d93c2d2ad 100644
--- a/ui/gfx/color_space.cc
+++ b/ui/gfx/color_space.cc
@@ -2,15 +2,90 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "build/build_config.h"
 #include "ui/gfx/color_space.h"
 
+#include <map>
+
+#include "base/synchronization/lock.h"
+
 namespace gfx {
 
 namespace {
 static const size_t kMinProfileLength = 128;
 static const size_t kMaxProfileLength = 4 * 1024 * 1024;
-}
+}  // namespace
+
+// The structure used to look up GlobalData structures.
+struct ColorSpace::Key {
+  Key(ColorSpace::Type type, const std::vector<char>& icc_profile)
+      : type(type), icc_profile(icc_profile) {}
+
+  bool operator<(const Key& other) const {
+    if (type < other.type)
+      return true;
+    if (type > other.type)
+      return false;
+    if (type != Type::ICC_PROFILE)
+      return false;
+
+    if (icc_profile.size() < other.icc_profile.size())
+      return true;
+    if (icc_profile.size() > other.icc_profile.size())
+      return false;
+    for (size_t i = 0; i < icc_profile.size(); ++i) {
+      if (icc_profile[i] < other.icc_profile[i])
+        return true;
+      if (icc_profile[i] > other.icc_profile[i])
+        return false;
+    }
+    return false;
+  }
+
+  ColorSpace::Type type;
+  const std::vector<char> icc_profile;
+};
+
+// Because this structure is shared across gfx::ColorSpace objects on
+// different threads, it needs to be thread-safe.
+class ColorSpace::GlobalData
+    : public base::RefCountedThreadSafe<ColorSpace::GlobalData> {
+ public:
+  static void Get(const Key& key, scoped_refptr<GlobalData>* value) {
+    base::AutoLock lock(map_lock_);
+    auto found = map_.find(key);

[hubbe, 2016/07/11 22:51:59]

use insert here

[ccameron, 2016/07/12 02:49:01]

Done.

+    if (found != map_.end()) {
+      *value = make_scoped_refptr(found->second);
+      return;
+    }
+    GlobalData* global_data = new GlobalData(key);
+    map_.insert(std::make_pair(key, global_data));
+    *value = make_scoped_refptr(global_data);
+  }
+
+  const std::vector<char>& GetICCProfile() const { return key_.icc_profile; }
+
+ private:
+  friend class base::RefCountedThreadSafe<GlobalData>;
+
+  GlobalData(const Key& key) : key_(key) {
+    // TODO: Compute the ICC profile for typed color spaces here.
+  }
+  ~GlobalData() {
+    base::AutoLock lock(map_lock_);
+    auto found = map_.find(key_);
+    DCHECK(found != map_.end());
+    DCHECK(found->second == this);
+    map_.erase(found);
+  }
+
+  Key key_;

[hubbe, 2016/07/11 22:51:59]

I think this is really confusing, because the key is also the data.

[ccameron, 2016/07/12 02:49:00]

Good point. We need to be able to erase |this| from |map_| when its last
reference goes away, but that can be done by keeping around an iterator into
|map_|, instead.

+
+  static std::map<Key, GlobalData*> map_;

[hubbe, 2016/07/11 22:51:59]

I think this should be hash_set<scoped_refptr<GlobalData>, CustomHash,
CustomCompare> ?

[ccameron, 2016/07/12 02:49:00]

The hash_set would then keep a reference to the GlobalData, which would keep it
around forever -- my goal with this complication is to keep the GlobalData
around for as long as there exists instances of the color space (and then get
rid of it).

+  static base::Lock map_lock_;
+};
+
+std::map<ColorSpace::Key, ColorSpace::GlobalData*> ColorSpace::GlobalData::map_;
+base::Lock ColorSpace::GlobalData::map_lock_;
 
 ColorSpace::ColorSpace() = default;
 ColorSpace::ColorSpace(ColorSpace&& other) = default;
@@ -19,13 +94,26 @@ ColorSpace& ColorSpace::operator=(const ColorSpace& other) = default;
 ColorSpace::~ColorSpace() = default;
 
 bool ColorSpace::operator==(const ColorSpace& other) const {
-  return icc_profile_ == other.icc_profile_;
+  if (type_ == Type::ICC_PROFILE && other.type_ == Type::ICC_PROFILE)
+    return global_data_ == other.global_data_;
+  return type_ == other.type_;
 }
 
+bool ColorSpace::operator<(const ColorSpace& other) const {
+  // Note that this does a pointer-based comparision.
+  if (type_ == Type::ICC_PROFILE && other.type_ == Type::ICC_PROFILE)
+    return global_data_.get() < other.global_data_.get();
+  return type_ < other.type_;
+}
+
+// static
 ColorSpace ColorSpace::FromICCProfile(const std::vector<char>& icc_profile) {
   ColorSpace color_space;
-  if (IsValidProfileLength(icc_profile.size()))
-    color_space.icc_profile_ = icc_profile;
+  if (IsValidProfileLength(icc_profile.size())) {
+    color_space.type_ = Type::ICC_PROFILE;
+    Key key(Type::ICC_PROFILE, icc_profile);
+    GlobalData::Get(key, &color_space.global_data_);
+  }
   return color_space;
 }
 
@@ -36,6 +124,14 @@ ColorSpace ColorSpace::FromBestMonitor() {
 }
 #endif
 
+const std::vector<char>& ColorSpace::GetICCProfile() const {
+  if (!global_data_) {
+    Key key(type_, std::vector<char>());
+    GlobalData::Get(key, &global_data_);
+  }
+  return global_data_->GetICCProfile();
+}
+
 // static
 bool ColorSpace::IsValidProfileLength(size_t length) {
   return length >= kMinProfileLength && length <= kMaxProfileLength;