cc: Add color space to image decode caches

cc/tiles/gpu_image_decode_cache.cc

Index: cc/tiles/gpu_image_decode_cache.cc
diff --git a/cc/tiles/gpu_image_decode_cache.cc b/cc/tiles/gpu_image_decode_cache.cc
index 70216a376f1a91890b4520d57f45c9cc1697d844..4b49ad3045efd66fef2e4423a3cf52145d2b67e2 100644
--- a/cc/tiles/gpu_image_decode_cache.cc
+++ b/cc/tiles/gpu_image_decode_cache.cc
@@ -109,30 +109,38 @@ gfx::Size CalculateSizeForMipLevel(const DrawImage& draw_image, int mip_level) {
   return MipMapUtil::GetSizeForLevel(base_size, mip_level);
 }
 
-// Generates a uint64_t which uniquely identifies a DrawImage for the purposes
-// of the |in_use_cache_|. The key is generated as follows:
-//   ╔══════════════════════╤═══════════╤═══════════╗
-//   ║       image_id       │ mip_level │  quality  ║

[vmpstr, 2017/04/04 01:16:43]

Oh no, the fancy comment is gone!

[ericrk, 2017/04/04 02:38:04]

;_;

-//   ╚════════32═bits═══════╧══16═bits══╧══16═bits══╝
-uint64_t GenerateInUseCacheKey(const DrawImage& draw_image) {
+}  // namespace
+
+// Extract the information to uniquely identify a DrawImage for the purposes of
+// the |in_use_cache_|.
+GpuImageDecodeCache::InUseCacheKey::InUseCacheKey(const DrawImage& draw_image)
+    : image_id(draw_image.image()->uniqueID()),
+      mip_level(CalculateUploadScaleMipLevel(draw_image)),
+      filter_quality(CalculateUploadScaleFilterQuality(draw_image)),
+      target_color_space(draw_image.target_color_space()) {
   static_assert(

[ericrk, 2017/04/04 02:38:04]

this static_assert isn't needed now that we aren't squishing SkFilterQuality
into a uint16_t.

actually, the rest of the function is sort of not needed any more (comments and
DCHECKs mattered when we were packing things into a uin64_t)... collapse to {}?

[ccameron, 2017/04/04 06:41:10]

Done.

       kLast_SkFilterQuality <= std::numeric_limits<uint16_t>::max(),
       "InUseCacheKey depends on SkFilterQuality fitting in a uint16_t.");
-
-  SkFilterQuality filter_quality =
-      CalculateUploadScaleFilterQuality(draw_image);
   DCHECK_LE(filter_quality, kLast_SkFilterQuality);
 
   // An image has at most log_2(max(width, height)) mip levels, so given our
   // usage of 32-bit sizes for images, key.mip_level is at most 31.
-  int32_t mip_level = CalculateUploadScaleMipLevel(draw_image);
   DCHECK_LT(mip_level, 32);
+}
 
-  return (static_cast<uint64_t>(draw_image.image()->uniqueID()) << 32) |
-         (mip_level << 16) | filter_quality;
+bool GpuImageDecodeCache::InUseCacheKey::operator==(
+    const InUseCacheKey& other) const {
+  return image_id == other.image_id && mip_level == other.mip_level &&
+         filter_quality == other.filter_quality &&
+         target_color_space == other.target_color_space;
 }
 
-}  // namespace
+size_t GpuImageDecodeCache::InUseCacheKeyHash::operator()(
+    const InUseCacheKey& cache_key) const {
+  return static_cast<size_t>(cache_key.image_id) ^ (cache_key.mip_level << 16) ^

[vmpstr, 2017/04/04 01:16:43]

There has base hash functions that I think you should use here instead (see
base::HashInts). I'm not 100% sure what's better, but it seems a bit awkward to
have your own hashing. 

[ccameron, 2017/04/04 06:41:10]

Sounds reasonable -- added a nested HashInts(w, HashInts(x, HashInts(y, z)))

+         (cache_key.filter_quality << 24) ^
+         cache_key.target_color_space.GetHash();
+}
 
 GpuImageDecodeCache::InUseCacheEntry::InUseCacheEntry(
     scoped_refptr<ImageData> image_data)
@@ -326,8 +334,12 @@ void GpuImageDecodeCache::UploadedImageData::ReportUsageStats() const {
 GpuImageDecodeCache::ImageData::ImageData(
     DecodedDataMode mode,
     size_t size,
+    const gfx::ColorSpace& target_color_space,
     const SkImage::DeferredTextureImageUsageParams& upload_params)
-    : mode(mode), size(size), upload_params(upload_params) {}
+    : mode(mode),
+      size(size),
+      target_color_space(target_color_space),
+      upload_params(upload_params) {}
 
 GpuImageDecodeCache::ImageData::~ImageData() {
   // We should never delete ImageData while it is in use or before it has been
@@ -766,7 +778,7 @@ void GpuImageDecodeCache::RefImageDecode(const DrawImage& draw_image) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "GpuImageDecodeCache::RefImageDecode");
   lock_.AssertAcquired();
-  auto found = in_use_cache_.find(GenerateInUseCacheKey(draw_image));
+  auto found = in_use_cache_.find(InUseCacheKey(draw_image));
   DCHECK(found != in_use_cache_.end());
   ++found->second.ref_count;
   ++found->second.image_data->decode.ref_count;
@@ -777,7 +789,7 @@ void GpuImageDecodeCache::UnrefImageDecode(const DrawImage& draw_image) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "GpuImageDecodeCache::UnrefImageDecode");
   lock_.AssertAcquired();
-  auto found = in_use_cache_.find(GenerateInUseCacheKey(draw_image));
+  auto found = in_use_cache_.find(InUseCacheKey(draw_image));
   DCHECK(found != in_use_cache_.end());
   DCHECK_GT(found->second.image_data->decode.ref_count, 0u);
   DCHECK_GT(found->second.ref_count, 0u);
@@ -793,7 +805,7 @@ void GpuImageDecodeCache::RefImage(const DrawImage& draw_image) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "GpuImageDecodeCache::RefImage");
   lock_.AssertAcquired();
-  InUseCacheKey key = GenerateInUseCacheKey(draw_image);
+  InUseCacheKey key = InUseCacheKey(draw_image);
   auto found = in_use_cache_.find(key);
 
   // If no secondary cache entry was found for the given |draw_image|, then
@@ -818,7 +830,7 @@ void GpuImageDecodeCache::RefImage(const DrawImage& draw_image) {
 
 void GpuImageDecodeCache::UnrefImageInternal(const DrawImage& draw_image) {
   lock_.AssertAcquired();
-  auto found = in_use_cache_.find(GenerateInUseCacheKey(draw_image));
+  auto found = in_use_cache_.find(InUseCacheKey(draw_image));
   DCHECK(found != in_use_cache_.end());
   DCHECK_GT(found->second.image_data->upload.ref_count, 0u);
   DCHECK_GT(found->second.ref_count, 0u);
@@ -1085,10 +1097,12 @@ void GpuImageDecodeCache::DecodeImageIfNecessary(const DrawImage& draw_image,
         // TODO(crbug.com/649167): Params should not have changed since initial
         // sizing. Somehow this still happens. We should investigate and re-add
         // DCHECKs here to enforce this.
-
         if (!draw_image.image()->getDeferredTextureImageData(
                 *context_threadsafe_proxy_.get(), &image_data->upload_params, 1,
-                backing_memory->data())) {
+                backing_memory->data(),
+                draw_image.target_color_space().ToSkColorSpace().get())) {
+          DLOG(ERROR) << "getDeferredTextureImageData failed despite params "
+                      << "having validated.";
           backing_memory->Unlock();
           backing_memory.reset();
         }
@@ -1178,7 +1192,8 @@ GpuImageDecodeCache::CreateImageData(const DrawImage& draw_image) {
       draw_image.matrix(), CalculateUploadScaleFilterQuality(draw_image),
       upload_scale_mip_level);
   size_t data_size = draw_image.image()->getDeferredTextureImageData(
-      *context_threadsafe_proxy_.get(), &params, 1, nullptr);
+      *context_threadsafe_proxy_.get(), &params, 1, nullptr,
+      draw_image.target_color_space().ToSkColorSpace().get());
 
   if (data_size == 0) {
     // Can't upload image, too large or other failure. Try to use SW fallback.
@@ -1190,7 +1205,8 @@ GpuImageDecodeCache::CreateImageData(const DrawImage& draw_image) {
     mode = DecodedDataMode::GPU;
   }
 
-  return make_scoped_refptr(new ImageData(mode, data_size, params));
+  return make_scoped_refptr(
+      new ImageData(mode, data_size, draw_image.target_color_space(), params));
 }
 
 void GpuImageDecodeCache::DeletePendingImages() {
@@ -1217,7 +1233,7 @@ GpuImageDecodeCache::ImageData* GpuImageDecodeCache::GetImageDataForDrawImage(
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "GpuImageDecodeCache::GetImageDataForDrawImage");
   lock_.AssertAcquired();
-  auto found_in_use = in_use_cache_.find(GenerateInUseCacheKey(draw_image));
+  auto found_in_use = in_use_cache_.find(InUseCacheKey(draw_image));
   if (found_in_use != in_use_cache_.end())
     return found_in_use->second.image_data.get();
 
@@ -1250,7 +1266,13 @@ bool GpuImageDecodeCache::IsCompatible(const ImageData* image_data,
                              image_data->upload_params.fPreScaleMipLevel;
   bool quality_is_compatible = CalculateUploadScaleFilterQuality(draw_image) <=
                                image_data->upload_params.fQuality;
-  return !is_scaled || (scale_is_compatible && quality_is_compatible);
+  bool color_is_compatible =
+      image_data->target_color_space == draw_image.target_color_space();
+  if (!color_is_compatible)
+    return false;
+  if (is_scaled && (!scale_is_compatible || !quality_is_compatible))
+    return false;
+  return true;
 }
 
 size_t GpuImageDecodeCache::GetDrawImageSizeForTesting(const DrawImage& image) {
@@ -1279,7 +1301,7 @@ bool GpuImageDecodeCache::DiscardableIsLockedForTesting(
 
 bool GpuImageDecodeCache::IsInInUseCacheForTesting(
     const DrawImage& image) const {
-  auto found = in_use_cache_.find(GenerateInUseCacheKey(image));
+  auto found = in_use_cache_.find(InUseCacheKey(image));
   return found != in_use_cache_.end();
 }