Gpu Image Decode Controller

cc/tiles/gpu_image_decode_controller.h

Index: cc/tiles/gpu_image_decode_controller.h
diff --git a/cc/tiles/gpu_image_decode_controller.h b/cc/tiles/gpu_image_decode_controller.h
index 390ef8ed972cbdc45ccc59cc5efe79c194edbc9f..6243a74d51df213876566a5314557b9261f8d9e3 100644
--- a/cc/tiles/gpu_image_decode_controller.h
+++ b/cc/tiles/gpu_image_decode_controller.h
@@ -5,18 +5,54 @@
 #ifndef CC_TILES_GPU_IMAGE_DECODE_CONTROLLER_H_
 #define CC_TILES_GPU_IMAGE_DECODE_CONTROLLER_H_
 
+#include <list>
 #include <unordered_map>
 #include <unordered_set>
+#include <vector>
 
+#include "base/containers/mru_cache.h"
+#include "base/memory/discardable_memory.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/synchronization/lock.h"
+#include "base/trace_event/memory_dump_provider.h"
 #include "cc/base/cc_export.h"
 #include "cc/tiles/image_decode_controller.h"
+#include "skia/ext/refptr.h"
+
+class SkImageTextureData;
 
 namespace cc {
 
+class ContextProvider;
+
+// GpuImageDecodeController handles the decode and upload of images that will
+// be used by Skia's GPU raster path. It also maintains a cache of these
+// decoded/ uploaded images for later re-use.
+//
+// Generally, when an image is required for raster, GpuImageDecodeController
+// creates two tasks, one to decode the image, and one to upload the image to
+// the GPU. These tasks are completed before the raster task which depends on
+// the image. We need to seperate decode and upload tasks, as decode can occur
+// simultaneously on multiple threads, while upload requires the GL context
+// lockmust happen on our non-concurrent raster thread.
+//
+// Decoded and Uploaded image data share a single cache entry. Depending on how
+// far we've progressed, this cache entry may contain CPU-side decoded data,
+// GPU-side uploaded data, or both. Because CPU-side decoded data is stored in
+// discardable memory, and is only locked for short periods of time (until the
+// 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.
 class CC_EXPORT GpuImageDecodeController : public ImageDecodeController {
  public:
-  GpuImageDecodeController();
+  explicit GpuImageDecodeController(ContextProvider* context);
   ~GpuImageDecodeController() override;
 
   // ImageDecodeController overrides.
@@ -29,16 +65,88 @@ class CC_EXPORT GpuImageDecodeController : public ImageDecodeController {
                              const DecodedDrawImage& decoded_image) override;
   void ReduceCacheUsage() override;
 
+  // Called by Decode / Upload tasks.
   void DecodeImage(const DrawImage& image);
+  void UploadImage(const DrawImage& image);
+  void RemovePendingDecodeTaskForImage(const DrawImage& image);
+  void RemovePendingUploadTaskForImage(const DrawImage& image);
+  scoped_refptr<ImageDecodeTask> GetImageDecodeTaskAndRefLocked(

[vmpstr, 2016/03/25 22:37:30]

Can you put a separate comment here. I'm not sure how to parse the function
name. Specifically, what is Locked referring to? If it means that the mutex is
acquired before this function is called, then this function should really be
private. 

I think the public functions in the class should assume that the caller doesn't
know anything about any internal locks.

[ericrk, 2016/03/28 22:10:52]

This was called by the upload task, which was constructed with the lock held....
restructured to avoid this.

+      const DrawImage& image,
+      uint64_t prepare_tiles_id);
+  void UnrefImageDecode(const DrawImage& draw_image);
 
-  void RemovePendingTaskForImage(const DrawImage& image);
+  // For testing only.
+  void SetCachedItemLimit(size_t limit) { cached_items_limit_ = limit; }

[vmpstr, 2016/03/25 22:37:30]

These have to be named with ForTesting at the end if they are for testing only.

[ericrk, 2016/03/28 22:10:53]

Done.

+  void SetCachedBytesLimit(size_t limit) { cached_bytes_limit_ = limit; }
+  size_t GetBytesUsedForTesting() const { return bytes_used_; }
 
  private:
+  class ScopedRefImageLocked;
+
+  enum DecodedDataMode { DECODED_DATA_MODE_GPU, DECODED_DATA_MODE_CPU };

[vmpstr, 2016/03/25 22:37:30]

Can you do an enum class and call the values GPU and CPU.. that way you'd access
them as DecodedDataMode::GPU, which reads nicer than DECODED_DATA_MODE_GPU for
me. 

[ericrk, 2016/03/28 22:10:52]

yup - forgot that we could use these.

+
+  struct ImageData {
+    ImageData(DecodedDataMode mode, size_t size);
+    ~ImageData();
+    const DecodedDataMode mode;

[vmpstr, 2016/03/25 22:37:30]

nit: blank line before this one please

[ericrk, 2016/03/28 22:10:53]

Done.

+    const size_t size;
+
+    // May be null if image not yet decoded.

[vmpstr, 2016/03/25 22:37:30]

The cpu side and the gpu side seem logically to be different objects. Is it
cleaner to have separate smaller objects or is this better?

[ericrk, 2016/03/28 22:10:53]

Sure, I think I like that better as well.

+    scoped_ptr<base::DiscardableMemory> decoded_data;
+    uint32_t decoded_data_ref_count;
+    bool decoded_data_is_locked;
+
+    // May be null if image not yet uploaded / prepared.
+    skia::RefPtr<SkImage> uploaded_image;
+    // True if the image is pending upload, in which case it counts against our
+    // memory budget even though the uploaded_image may not yet exist.
+    bool image_pending;
+    uint32_t image_ref_count;
+
+    bool is_at_raster;
+  };
+
+  void RefImageDecodeLocked(const DrawImage& draw_image);
+  void UnrefImageDecodeLocked(const DrawImage& draw_image);
+  void RefImageLocked(const DrawImage& draw_image);
+  void UnrefImageLocked(const DrawImage& draw_image);
+  bool EnsureCapacityLocked(size_t new_size);

[vmpstr, 2016/03/25 22:37:30]

This function needs a comment. Not sure what it does...

[ericrk, 2016/03/28 22:10:53]

Done.

+
+  bool CanFitSize(size_t size) const;
+  bool CanFitCount(size_t num_elements) const;
+
+  void DecodeImageIfNecessaryLocked(const DrawImage& draw_image,
+                                    ImageData* image_data);
+  void UploadImageIfNecessaryLocked(const DrawImage& draw_image,
+                                    ImageData* image_data);
+
+  scoped_ptr<GpuImageDecodeController::ImageData> NewImageData(
+      const DrawImage& image);
+
+  void RemovePendingUploadTaskForImageLocked(const DrawImage& draw_image);
+  void RemovePendingDecodeTaskForImageLocked(const DrawImage& draw_image);
+
+  void DeletePendingImagesLocked();
+
   base::Lock lock_;
 
-  std::unordered_set<uint32_t> prerolled_images_;
   std::unordered_map<uint32_t, scoped_refptr<ImageDecodeTask>>
-      pending_image_tasks_;
+      pending_image_upload_tasks_;
+  std::unordered_map<uint32_t, scoped_refptr<ImageDecodeTask>>
+      pending_image_decode_tasks_;
+
+  base::MRUCache<uint32_t, scoped_ptr<ImageData>> image_data_;
+
+  size_t cached_items_limit_;
+  size_t cached_bytes_limit_;
+  size_t bytes_used_;
+  ContextProvider* context_;
+  GrContextThreadSafeProxy* context_threadsafe_proxy_;
+
+  // We can't release GPU backed SkImages without holding the Skia context lock,
+  // so we add them to this list and defer deletion until the next time the lock
+  // is held.
+  std::vector<skia::RefPtr<SkImage>> images_pending_deletion_;
 };
 
 }  // namespace cc