cc: Add color space to image decode caches

cc/tiles/gpu_image_decode_cache.h

 // 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.
 
 #ifndef CC_TILES_GPU_IMAGE_DECODE_CACHE_H_
 #define CC_TILES_GPU_IMAGE_DECODE_CACHE_H_
 
 #include <memory>
 #include <unordered_map>
 #include <vector>
@@ skipping 214 matching lines @@
     void ReportUsageStats() const;
 
     // May be null if image not yet uploaded / prepared.
     sk_sp<SkImage> image_;
     UsageStats usage_stats_;
   };
 
   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;
 
     // If true, this image is no longer in our |persistent_cache_| and will be
     // deleted as soon as its ref count reaches zero.
     bool is_orphaned = false;
 
     DecodedImageData decode;
     UploadedImageData upload;
 
    private:
     friend class base::RefCounted<ImageData>;
     ~ImageData();
   };
 
   // A ref-count and ImageData, used to associate the ImageData with a specific
   // DrawImage in the |in_use_cache_|.
   struct InUseCacheEntry {
     explicit InUseCacheEntry(scoped_refptr<ImageData> image_data);
     InUseCacheEntry(const InUseCacheEntry& other);
     InUseCacheEntry(InUseCacheEntry&& other);
     ~InUseCacheEntry();
 
     uint32_t ref_count = 0;
     scoped_refptr<ImageData> image_data;
   };
 
   // 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
   // additional comments.
 
   // Similar to GetTaskForImageAndRef, but gets the dependent decode task
   // rather than the upload task, if necessary.
   scoped_refptr<TileTask> GetImageDecodeTaskAndRef(
       const DrawImage& image,
       const TracingInfo& tracing_info,
@@ skipping 51 matching lines @@
   // All members below this point must only be accessed while holding |lock_|.
   base::Lock lock_;
 
   // |persistent_cache_| represents the long-lived cache, keeping a certain
   // budget of ImageDatas alive even when their ref count reaches zero.
   using PersistentCache = base::MRUCache<uint32_t, scoped_refptr<ImageData>>;
   PersistentCache persistent_cache_;
 
   // |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_;
   const size_t normal_max_cache_bytes_;
   size_t cached_bytes_limit_ = normal_max_cache_bytes_;
   size_t bytes_used_ = 0;
   base::MemoryState memory_state_ = base::MemoryState::NORMAL;
 
   // We can't release GPU backed SkImages without holding the context lock,
   // so we add them to this list and defer deletion until the next time the lock
   // is held.
   std::vector<sk_sp<SkImage>> images_pending_deletion_;
 };
 
 }  // namespace cc
 
 #endif  // CC_TILES_GPU_IMAGE_DECODE_CACHE_H_