Color: Separate ICCProfile and ColorSpace structures

ui/gfx/icc_profile.cc

+// Copyright 2016 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 "ui/gfx/icc_profile.h"
+
+#include <list>
+
+#include "base/lazy_instance.h"
+#include "base/synchronization/lock.h"
+
+namespace gfx {
+
+namespace {
+const size_t kMinProfileLength = 128;
+const size_t kMaxProfileLength = 4 * 1024 * 1024;
+
+// Allow keeping around a maximum of 8 cached ICC profiles. Beware that
+// we do lots of linear-time operations here, so do not increase this
+// without optimizing it.
+const size_t kMaxCachedICCProfiles = 8;
+static base::LazyInstance<std::list<ICCProfile>> g_mru_cache;

[dcheng, 2016/07/22 05:00:27]

FWIW, we do have some MRU cache abstractions in //base/containers/mru_cache.h.
Does this intentionally avoid using them?

[ccameron, 2016/07/22 21:02:41]

Oh, that's nice! Switched to using them.

+static base::LazyInstance<base::Lock> g_mru_cache_lock;
+
+uint64_t g_next_id = 1;
+
+}  // namespace
+
+ICCProfile::ICCProfile() = default;
+ICCProfile::ICCProfile(ICCProfile&& other) = default;
+ICCProfile::ICCProfile(const ICCProfile& other) = default;
+ICCProfile& ICCProfile::operator=(const ICCProfile& other) = default;
+ICCProfile::~ICCProfile() = default;
+
+bool ICCProfile::operator==(const ICCProfile& other) const {
+  return valid_ == other.valid_ && data_ == other.data_;
+}
+
+// static
+ICCProfile ICCProfile::FromData(const std::vector<char>& icc_profile_data) {
+  ICCProfile icc_profile;
+  if (IsValidProfileLength(icc_profile_data.size())) {
+    icc_profile.valid_ = true;
+    icc_profile.data_ = icc_profile_data;
+  }
+  // Move this ICC profile to the front of the cache, and use its cached ID if
+  // it's already in the cache.
+  if (icc_profile.valid_) {
+    base::AutoLock lock(g_mru_cache_lock.Get());
+    for (auto iter = g_mru_cache.Get().begin(); iter != g_mru_cache.Get().end();
+         ++iter) {
+      if (*iter == icc_profile) {
+        icc_profile.id_ = iter->id_;
+        g_mru_cache.Get().splice(g_mru_cache.Get().end(), g_mru_cache.Get(),
+                                 iter);
+        return icc_profile;
+      }
+    }
+    icc_profile.id_ = g_next_id++;
+    g_mru_cache.Get().push_back(icc_profile);
+    while (g_mru_cache.Get().size() > kMaxCachedICCProfiles)
+      g_mru_cache.Get().pop_front();
+  }
+  return icc_profile;
+}
+
+#if !defined(OS_WIN) && !defined(OS_MACOSX)
+// static
+ICCProfile ICCProfile::FromBestMonitor() {
+  return ICCProfile();
+}
+#endif
+
+// static
+ICCProfile ICCProfile::FromColorSpace(const gfx::ColorSpace& color_space) {
+  // Retrieve ICC profiles from the cache.
+  if (color_space.icc_profile_id_) {
+    base::AutoLock lock(g_mru_cache_lock.Get());
+    for (auto iter = g_mru_cache.Get().begin(); iter != g_mru_cache.Get().end();
+         ++iter) {
+      if (iter->id_ == color_space.icc_profile_id_) {
+        g_mru_cache.Get().splice(g_mru_cache.Get().end(), g_mru_cache.Get(),
+                                 iter);
+        return *iter;
+      }
+    }
+  }
+  // TODO(ccameron): Support constructing ICC profiles from arbitrary ColorSpace
+  // objects.

[dcheng, 2016/07/22 05:00:27]

There are plans to start sending this from less-privileged to more-privileged
processes right? I'm just curious what the eventual plan is, since a renderer
can just make up an arbitrary icc profile ID.

[ccameron, 2016/07/22 21:02:41]

Full plan:

We'll only be sending gfx::ICCProfile more-privileged->less-privileged. Parsing
these is complicated and I wouldn't vouch for a parser of this being secure.

We will be sending gfx::ColorSpace from less-privileged->more-privileged. We'll
be expanding gfx::ColorSpace to include a number of enums and a matrix. The
browser process will use this to adjust the color of content coming from that
renderer (and only that renderer). Making up an arbitrary ID for the
gfx::ColorSpace of a cc::TransferableResource will result in that resource
having different/wrong color. The globally cached ICC profiles are not
privileged information.

WRT generating ICC profiles, we will not be generating new ICC profile data from
gfx::ColorSpaces coming from the renderer. When we add ICC profile generation
code, it will have limited functionality, and be robust to any inputs (so that
even if we were to generate an ICC profile from a renderer's color space, that
would still be secure).

+  return ICCProfile();
+}
+
+const std::vector<char>& ICCProfile::GetData() const {
+  return data_;
+}
+
+ColorSpace ICCProfile::GetColorSpace() const {
+  ColorSpace color_space;
+  color_space.valid_ = true;
+  color_space.icc_profile_id_ = id_;
+
+  // Move this ICC profile to the most recently used end of the cache.
+  base::AutoLock lock(g_mru_cache_lock.Get());
+  for (auto iter = g_mru_cache.Get().begin(); iter != g_mru_cache.Get().end();
+       ++iter) {
+    if (iter->id_ == id_) {
+      g_mru_cache.Get().splice(g_mru_cache.Get().end(), g_mru_cache.Get(),
+                               iter);
+      return color_space;
+    }
+  }
+  g_mru_cache.Get().push_back(*this);
+  while (g_mru_cache.Get().size() > kMaxCachedICCProfiles)
+    g_mru_cache.Get().pop_front();
+  return color_space;
+}
+
+// static
+bool ICCProfile::IsValidProfileLength(size_t length) {
+  return length >= kMinProfileLength && length <= kMaxProfileLength;
+}
+
+}  // namespace gfx