cc: Add color space to image decode caches

cc/tiles/gpu_image_decode_cache.h

Index: cc/tiles/gpu_image_decode_cache.h
diff --git a/cc/tiles/gpu_image_decode_cache.h b/cc/tiles/gpu_image_decode_cache.h
index c80c5a9b43ccca53b1b67e760c27fc7d29e213f0..60c36c909e61e8c496c3d782153f593ab140c6b4 100644
--- a/cc/tiles/gpu_image_decode_cache.h
+++ b/cc/tiles/gpu_image_decode_cache.h
@@ -232,10 +232,12 @@ class CC_EXPORT GpuImageDecodeCache
   struct ImageData : public base::RefCounted<ImageData> {
     ImageData(DecodedDataMode mode,
               size_t size,
+              const gfx::ColorSpace& target_color_space,
               const SkImage::DeferredTextureImageUsageParams& upload_params);
 
     const DecodedDataMode mode;
     const size_t size;
+    gfx::ColorSpace target_color_space;
     bool is_at_raster = false;
     SkImage::DeferredTextureImageUsageParams upload_params;
 
@@ -265,7 +267,17 @@ class CC_EXPORT GpuImageDecodeCache
 
   // Uniquely identifies (without collisions) a specific DrawImage for use in
   // the |in_use_cache_|.
-  using InUseCacheKey = uint64_t;
+  struct InUseCacheKey {

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

I don't think we ever access the internals of this class - can you make this a
class and make the data members private?

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

Done.

+    explicit InUseCacheKey(const DrawImage& draw_image);

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

Can you use a similar pattern as in the software case (static
InUseCacheKey::FromDrawImage(...))

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

Done.

+    bool operator==(const InUseCacheKey& other) const;
+    uint32_t image_id;
+    int mip_level;
+    SkFilterQuality filter_quality;
+    gfx::ColorSpace target_color_space;

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

This looks really big (close to 100 bytes) - I sort of wonder about copying this
whole thing to the cache key.

Do you think we should do something like store a scoped_refptr to this (so it's
still copyable) and maybe cache the hash for performance?

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

I'm surprised that this is a performance-critical path.

That said, gfx::ColorSpace needs to go on a diet. I have a patch to shrink it a
little, and I may change its floats to half-floats to shrink it some more (to
~40 bytes or so). And maybe even make it internally be a refptr of sorts.

That and that said, there's really no reason not to be using a
sk_sp<SkColorSpace> here. So, I could do that.

That and that and that said, WRT performance, I worry about the cost of
refcounting, especially if it needs to be thread-safe, and especially on ARM. My
(like, five-years-out-of-date) recollection is that doing a thread-safe refcount
increment on ARM was non-trivial (especially compared to just copying a ~100
bytes).

[ericrk, 2017/04/04 16:28:40]

I was actually more concerned about the memory cost - it feels like the
performance is probably not too big a deal in either case.

Also, given that the object holds an sk_sp<SkColorSpace>, isn't there a refcount
either way?

We can end up with a few hundred of these objects live at a time - 10 or 20k may
be an OK memory cost to pay, it just feels heavier than needed. vmpstr@, WDYT?

I don't feel super strongly - especially if we're going to slim this guy down.

[vmpstr, 2017/04/04 17:30:08]

I don't feel strongly about this. I think we do end up copying this objects,
although we've been careful about this in the past (in both gpu and software).
If we do move to sk_sp, does that mean we'll share the color space object itself
across different keys? If we always allocate a new one, the memory impact would
be the same...

I vote for making this an sk_sp to indicate that it's not a trivial thing to
copy, but am willing to be convinced otherwise. 

+  };
+  struct InUseCacheKeyHash {
+    size_t operator()(const InUseCacheKey&) const;
+  };
 
   // All private functions should only be called while holding |lock_|. Some
   // functions also require the |context_| lock. These are indicated by
@@ -337,7 +349,8 @@ class CC_EXPORT GpuImageDecodeCache
 
   // |in_use_cache_| represents the in-use (short-lived) cache. Entries are
   // cleaned up as soon as their ref count reaches zero.
-  using InUseCache = std::unordered_map<InUseCacheKey, InUseCacheEntry>;
+  using InUseCache =
+      std::unordered_map<InUseCacheKey, InUseCacheEntry, InUseCacheKeyHash>;
   InUseCache in_use_cache_;
 
   size_t max_working_set_bytes_;