Add display-resolution caching to GPU IDC

cc/tiles/gpu_image_decode_controller.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_CONTROLLER_H_
 #define CC_TILES_GPU_IMAGE_DECODE_CONTROLLER_H_
 
 #include <memory>
 #include <unordered_map>
 #include <vector>
@@ skipping 31 matching lines @@
 // upload completes), this memory is not counted against our sized cache
 // limits. Uploaded GPU memory, being non-discardable, always counts against
 // our limits.
 //
 // In cases where the number of images needed exceeds our cache limits, we
 // operate in an "at-raster" mode. In this mode, there are no decode/upload
 // tasks, and images are decoded/uploaded as needed, immediately before being
 // used in raster. Cache entries for at-raster tasks are marked as such, which
 // prevents future tasks from taking a dependency on them and extending their
 // lifetime longer than is necessary.
+//
+// In order to save memory, images which are going to be scaled may be uploaded
+// at lower than original resolution. In these cases, we may later need to
+// re-upload the image at a higher resolution, to accomodate a new use case. In
+// dealing with multiple scales of the same image, we take the following
+// approach:
+//   1) If an image is already uploaded at a larger resolution than is needed,
+//   use that upload directly.
+//   2) If an image is already uploaded at a lower resolution than is needed,
+//   orphan the existing upload (removing it from the main cache) and start
+//   uploading the larger version.
+//      2a) Orphaned images will be deleted as soon as all existing references
+//      to them are removed.
+//
+// In order to accomodate orphaned tasks, we have a two-part caching system. The
+// primary cache, |image_data_|, stores one ImageData per image id. These
+// ImageDatas are not necessarily associated with a given DrawImage, and are
+// saved even when their ref-count reaches zero to allow for future re-use.

[vmpstr, 2016/06/14 21:35:27]

It would be good to mention that it's kept there assuming that the budget isn't
breached (ie this image_data_ is governed by the specified memory limit)

[ericrk, 2016/06/16 23:03:33]

Done.

+//
+// The second cache, |image_data_for_draw_image_|, stores one image data per
+// DrawImage - this may be the same ImageData that is in the primary cache.
+// These cache entries are more transient than the primary cache and are deleted
+// as soon as all refs to the given DrawImage are released.
+//
+// We consider an ImageData "orphaned" if it is in the secondary cache, but not
+// the primary. This typically happens when we need a higher-quality/larger
+// version of an image than what is in the primary cache, so we replace the
+// primary cache entry. Orphaned ImageDatas still count against our budgets,
+// but are cleaned up as soon as their ref count reaches zero.
 class CC_EXPORT GpuImageDecodeController
     : public ImageDecodeController,
       public base::trace_event::MemoryDumpProvider {
  public:
   explicit GpuImageDecodeController(ContextProvider* context,
                                     ResourceFormat decode_format,
                                     size_t max_gpu_image_bytes);
   ~GpuImageDecodeController() override;
 
   // ImageDecodeController overrides.
@@ skipping 24 matching lines @@
   void OnImageUploadTaskCompleted(const DrawImage& image);
 
   // For testing only.
   void SetCachedItemLimitForTesting(size_t limit) {
     cached_items_limit_ = limit;
   }
   void SetCachedBytesLimitForTesting(size_t limit) {
     cached_bytes_limit_ = limit;
   }
   size_t GetBytesUsedForTesting() const { return bytes_used_; }
+  size_t GetDrawImageSizeForTesting(const DrawImage& image);
   void SetImageDecodingFailedForTesting(const DrawImage& image);
   bool DiscardableIsLockedForTesting(const DrawImage& image);
 
  private:
   enum class DecodedDataMode { GPU, CPU };
 
   // Stores the CPU-side decoded bits of an image and supporting fields.
   struct DecodedImageData {
     DecodedImageData();
     ~DecodedImageData();
@@ skipping 51 matching lines @@
       int ref_reached_zero_count = 0;
     };
 
     void ReportUsageStats() const;
 
     // May be null if image not yet uploaded / prepared.
     sk_sp<SkImage> image_;
     UsageStats usage_stats_;
   };
 
-  struct ImageData {
-    ImageData(DecodedDataMode mode, size_t size);
-    ~ImageData();
+  struct ImageData : public base::RefCounted<ImageData> {
+    ImageData(DecodedDataMode mode,
+              size_t size,
+              int pre_scale_mip_level,
+              SkFilterQuality pre_scale_filter_quality);
 
     const DecodedDataMode mode;
     const size_t size;
     bool is_at_raster = false;
+    int pre_scale_mip_level;
+    SkFilterQuality pre_scale_filter_quality;
+    bool is_orphaned = false;
 
     DecodedImageData decode;
     UploadedImageData upload;
+
+   private:
+    friend class base::RefCounted<ImageData>;
+    ~ImageData();
   };
 
-  using ImageDataMRUCache =
-      base::MRUCache<uint32_t, std::unique_ptr<ImageData>>;
+  // A ref-count and ImageData, used to associate the ImageData with a specific
+  // DrawImage in the secondary |image_data_for_draw_image_| cache.
+  struct ImageDataForDrawImageEntry {
+    explicit ImageDataForDrawImageEntry(
+        const scoped_refptr<ImageData>& image_data);

[vmpstr, 2016/06/14 21:35:27]

Pass by value, move into place?

[ericrk, 2016/06/16 23:03:33]

Done.

+    ImageDataForDrawImageEntry(const ImageDataForDrawImageEntry&);

[vmpstr, 2016/06/14 21:35:27]

nit: can you name the parameters? "other"?

Also, does the compiler complain if they are = default inline?

[ericrk, 2016/06/16 23:03:33]

sadness: 
error: [chromium-style] Complex constructor has an inlined body.

+    ImageDataForDrawImageEntry(ImageDataForDrawImageEntry&&);
+    ~ImageDataForDrawImageEntry();
+
+    uint32_t ref_count = 0;
+    scoped_refptr<ImageData> image_data;
+  };
+
+  // A key used to identify a DrawImage in the |image_data_for_draw_image_|
+  // cache, as well as the pending task maps.
+  struct DrawImageKey {
+    uint32_t image_id;

[vmpstr, 2016/06/14 21:35:27]

= 0 here and below

[ericrk, 2016/06/16 23:03:33]

Done.

+    int mip_level;
+    SkFilterQuality quality;
+
+    bool operator==(const DrawImageKey& other) const {
+      return other.image_id == image_id && other.mip_level == mip_level &&
+             other.quality == quality;
+    }
+  };
+
+  struct DrawImageKeyHash {
+    std::size_t operator()(const DrawImageKey& key) const {
+      return base::HashInts(key.image_id,
+                            base::HashInts(key.mip_level, key.quality));

[vmpstr, 2016/06/14 21:35:27]

I wonder if you can do something smarter than HashInts, since you know the
magnitude of possible values here. (HashInts might be slow for these, but I
don't think it matters that much).

[ericrk, 2016/06/16 23:03:33]

Sure, I'll make a better hash :P

+    }
+  };
 
   // 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);
 
   void RefImageDecode(const DrawImage& draw_image);
   void UnrefImageDecode(const DrawImage& draw_image);
   void RefImage(const DrawImage& draw_image);
   void UnrefImageInternal(const DrawImage& draw_image);
   void RefCountChanged(ImageData* image_data);
 
   // Ensures that the cache can hold an element of |required_size|, freeing
   // unreferenced cache entries if necessary to make room.
   bool EnsureCapacity(size_t required_size);
   bool CanFitSize(size_t size) const;
   bool ExceedsPreferredCount() const;
 
   void DecodeImageIfNecessary(const DrawImage& draw_image,
                               ImageData* image_data);
 
-  std::unique_ptr<GpuImageDecodeController::ImageData> CreateImageData(
+  scoped_refptr<GpuImageDecodeController::ImageData> CreateImageData(
       const DrawImage& image);
   SkImageInfo CreateImageInfoForDrawImage(const DrawImage& draw_image) const;
+  ImageData* GetImageDataForDrawImage(const DrawImage& image);
+
+  // Returns true if the given ImageData is as large or larger than the

[vmpstr, 2016/06/14 21:35:28]

This comment reads like like this could be an out of line function, but I guess
you want it be able to access private structs?

[ericrk, 2016/06/16 23:03:33]

Now that the key is a uint64_t, moved this out-of-line.

+  // DrawImage.
+  bool IsCompatibleWithDrawImage(const ImageData* image_data,
+                                 const DrawImage& draw_image) const;
+
+  // Generates a key which can be used to look up the provided |draw_image| in
+  // the |image_data_for_draw_image_| map.
+  DrawImageKey GenerateDrawImageKey(const DrawImage& draw_image) const;

[vmpstr, 2016/06/14 21:35:27]

This needs a better comment; specifically, can you mention the difference
between what each function does? Also, maybe one can be Generate if it creates a
new thing, and Find if looks something up?

[ericrk, 2016/06/16 23:03:33]

Removed the second fn and cleaned this up.

+
+  // Generates a key which can be used to look up the task for the provided
+  // |draw_image| in the task maps.
+  DrawImageKey GenerateTaskKeyForDrawImage(const DrawImage& draw_image);
 
   // The following two functions also require the |context_| lock to be held.
   void UploadImageIfNecessary(const DrawImage& draw_image,
                               ImageData* image_data);
   void DeletePendingImages();
 
   const ResourceFormat format_;
   ContextProvider* context_;
   sk_sp<GrContextThreadSafeProxy> context_threadsafe_proxy_;
 
   // All members below this point must only be accessed while holding |lock_|.
   base::Lock lock_;
 
-  std::unordered_map<uint32_t, scoped_refptr<TileTask>>
-      pending_image_upload_tasks_;
-  std::unordered_map<uint32_t, scoped_refptr<TileTask>>
-      pending_image_decode_tasks_;
+  using ImageDataMRUCache = base::MRUCache<uint32_t, scoped_refptr<ImageData>>;
+  // |image_data_| represents the primary (long-lived) cache.
+  ImageDataMRUCache image_data_;
 
-  ImageDataMRUCache image_data_;
+  template <typename T>
+  using DrawImageMap = std::unordered_map<DrawImageKey, T, DrawImageKeyHash>;
+  // |image_data_for_draw_image_| represents the secondary (short-lived) cache.
+  DrawImageMap<ImageDataForDrawImageEntry> image_data_for_draw_image_;

[vmpstr, 2016/06/14 21:35:27]

I'd find better names for these caches, maybe something like 

unique_id_cache_
draw_image_cache_

or maybe something else?

[ericrk, 2016/06/16 23:03:33]

Renamed to |persistent_cache_| and |in_use_cache_| to represent the data they
store. WDYT?

+
+  // Upload and decode tasks are keyed on the exact DrawImage they are decoding.
+  DrawImageMap<scoped_refptr<TileTask>> pending_image_upload_tasks_;
+  DrawImageMap<scoped_refptr<TileTask>> pending_image_decode_tasks_;
 
   size_t cached_items_limit_;
   size_t cached_bytes_limit_;
   size_t bytes_used_;
   const size_t max_gpu_image_bytes_;
 
   // 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_CONTROLLER_H_