Reland software image cache rework patches. (again)

This essentially reverts the following revert:
a599a458.

This reworks the software cache to simplify it and add ability to reuse
larger decoded images instead of always decoding the original image.

R=khushalsagar@chromium.org, ericrk@chromium.org

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I60d0a6aeb46c136cb8230aa7e1d106ef0386462b
Reviewed-on: https://chromium-review.googlesource.com/783636
Commit-Queue: vmpstr <vmpstr@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523822}

cc/BUILD.gn

@@ -654,6 +654,7 @@ cc_test("cc_unittests") {
     "tiles/picture_layer_tiling_set_unittest.cc",
     "tiles/picture_layer_tiling_unittest.cc",
     "tiles/software_image_decode_cache_unittest.cc",
+    "tiles/software_image_decode_cache_unittest_combinations.cc",
     "tiles/tile_manager_unittest.cc",
     "tiles/tile_priority_unittest.cc",
     "trees/damage_tracker_unittest.cc",
@@ -712,6 +713,13 @@ cc_test("cc_unittests") {
     ]
   }
 
+  if (is_win) {
+    # TODO(vmpstr): Some SoftwareImageDecodeCacheTests use virtual inheritance,
+    # which MSVC doesn't like. Suppress "Foo inherits Bar via dominance"
+    # warnings for now.
+    cflags = [ "/wd4250" ]
+  }
+
   # TODO(khushalsagar): Remove once crbug.com/683263 is fixed.
   configs = [ "//build/config/compiler:no_size_t_to_int_warning" ]
 

cc/paint/paint_image.cc

@@ -143,13 +143,6 @@ SkISize PaintImage::GetSupportedDecodeSize(
   return SkISize::Make(width(), height());
 }
 
-SkImageInfo PaintImage::CreateDecodeImageInfo(const SkISize& size,
-                                              SkColorType color_type) const {
-  DCHECK(GetSupportedDecodeSize(size) == size);
-  return SkImageInfo::Make(size.width(), size.height(), color_type,
-                           kPremul_SkAlphaType);
-}
-
 bool PaintImage::Decode(void* memory,
                         SkImageInfo* info,
                         sk_sp<SkColorSpace> color_space,

cc/paint/paint_image.h

@@ -116,11 +116,6 @@ class CC_PAINT_EXPORT PaintImage {
   // GetSupportedDecodeSize(size).
   SkISize GetSupportedDecodeSize(const SkISize& requested_size) const;
 
-  // Returns SkImageInfo that should be used to decode this image to the given
-  // size and color type. The size must be supported.
-  SkImageInfo CreateDecodeImageInfo(const SkISize& size,
-                                    SkColorType color_type) const;
-
   // Decode the image into the given memory for the given SkImageInfo.
   // - Size in |info| must be supported.
   // - The amount of memory allocated must be at least

cc/test/fake_paint_image_generator.cc

@@ -27,6 +27,8 @@ bool FakePaintImageGenerator::GetPixels(const SkImageInfo& info,
                                         size_t row_bytes,
                                         size_t frame_index,
                                         uint32_t lazy_pixel_ref) {
+  if (image_backing_memory_.empty())
+    return false;
   frames_decoded_.insert(frame_index);
   CHECK(image_pixmap_.readPixels(info, pixels, row_bytes, 0, 0));
   return true;

cc/tiles/software_image_decode_cache.h

@@ -142,11 +142,12 @@ class CC_EXPORT SoftwareImageDecodeCache
 
   // Decode the given image and store it in the cache. This is only called by an
   // image decode task from a worker thread.
-  void DecodeImage(const ImageKey& key,
-                   const DrawImage& image,
-                   DecodeTaskType task_type);
+  void DecodeImageInTask(const ImageKey& key,
+                         const PaintImage& paint_image,
+                         DecodeTaskType task_type);
 
-  void RemovePendingTask(const ImageKey& key, DecodeTaskType task_type);
+  void OnImageDecodeTaskCompleted(const ImageKey& key,
+                                  DecodeTaskType task_type);
 
   // MemoryDumpProvider overrides.
   bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
@@ -155,29 +156,30 @@ class CC_EXPORT SoftwareImageDecodeCache
   size_t GetNumCacheEntriesForTesting() const { return decoded_images_.size(); }
 
  private:
-  // DecodedImage is a convenience storage for discardable memory. It can also
+  // CacheEntry is a convenience storage for discardable memory. It can also
   // construct an image out of SkImageInfo and stored discardable memory.
-  class DecodedImage {
+  class CacheEntry {
    public:
-    DecodedImage(const SkImageInfo& info,
-                 std::unique_ptr<base::DiscardableMemory> memory,
-                 const SkSize& src_rect_offset);
-    ~DecodedImage();
-
-    const sk_sp<SkImage>& image() const {
-      DCHECK(locked_);
+    CacheEntry();
+    CacheEntry(const SkImageInfo& info,
+               std::unique_ptr<base::DiscardableMemory> memory,
+               const SkSize& src_rect_offset);
+    ~CacheEntry();
+
+    void MoveImageMemoryTo(CacheEntry* entry);
+
+    sk_sp<SkImage> image() const {
+      if (!memory)
+        return nullptr;
+      DCHECK(is_locked);
       return image_;
     }
-
     const SkSize& src_rect_offset() const { return src_rect_offset_; }
 
-    bool is_locked() const { return locked_; }
     bool Lock();
     void Unlock();
 
-    const base::DiscardableMemory* memory() const { return memory_.get(); }
-
-    // An ID which uniquely identifies this DecodedImage within the image decode
+    // An ID which uniquely identifies this CacheEntry within the image decode
     // cache. Used in memory tracing.
     uint64_t tracing_id() const { return tracing_id_; }
     // Mark this image as being used in either a draw or as a source for a
@@ -186,6 +188,21 @@ class CC_EXPORT SoftwareImageDecodeCache
     void mark_used() { usage_stats_.used = true; }
     void mark_out_of_raster() { usage_stats_.first_lock_out_of_raster = true; }
 
+    // Since this is an inner class, we expose these variables publicly for
+    // simplicity.
+    // TODO(vmpstr): A good simple clean-up would be to rethink this class
+    // and its interactions to instead expose a few functions which would also
+    // facilitate easier DCHECKs.
+    int ref_count = 0;
+    bool decode_failed = false;
+    bool is_locked = false;
+    bool is_budgeted = false;
+
+    scoped_refptr<TileTask> in_raster_task;
+    scoped_refptr<TileTask> out_of_raster_task;
+
+    std::unique_ptr<base::DiscardableMemory> memory;
+
    private:
     struct UsageStats {
       // We can only create a decoded image in a locked state, so the initial
@@ -197,9 +214,7 @@ class CC_EXPORT SoftwareImageDecodeCache
       bool first_lock_out_of_raster = false;
     };
 
-    bool locked_;
     SkImageInfo image_info_;
-    std::unique_ptr<base::DiscardableMemory> memory_;
     sk_sp<SkImage> image_;
     SkSize src_rect_offset_;
     uint64_t tracing_id_;
@@ -225,62 +240,22 @@ class CC_EXPORT SoftwareImageDecodeCache
   };
 
   using ImageMRUCache = base::
-      HashingMRUCache<ImageKey, std::unique_ptr<DecodedImage>, ImageKeyHash>;
-
-  // Looks for the key in the cache and returns true if it was found and was
-  // successfully locked (or if it was already locked). Note that if this
-  // function returns true, then a ref count is increased for the image.
-  bool LockDecodedImageIfPossibleAndRef(const ImageKey& key);
+      HashingMRUCache<ImageKey, std::unique_ptr<CacheEntry>, ImageKeyHash>;
 
   // Actually decode the image. Note that this function can (and should) be
   // called with no lock acquired, since it can do a lot of work. Note that it
   // can also return nullptr to indicate the decode failed.
-  std::unique_ptr<DecodedImage> DecodeImageInternal(
-      const ImageKey& key,
-      const DrawImage& draw_image);
-
-  // Get the decoded draw image for the given key and draw_image. Note that this
-  // function has to be called with no lock acquired, since it will acquire its
-  // own locks and might call DecodeImageInternal above. Also note that this
-  // function will use the provided key, even if
-  // ImageKey::FromDrawImage(draw_image) would return a different key.
-  // Note that when used internally, we still require that
-  // DrawWithImageFinished() is called afterwards.
-  DecodedDrawImage GetDecodedImageForDrawInternal(const ImageKey& key,
+  std::unique_ptr<CacheEntry> DecodeImageInternal(const ImageKey& key,
                                                   const DrawImage& draw_image);
 
-  // GetExactSizeImageDecode is called by DecodeImageInternal when the
-  // quality does not scale the image. Like DecodeImageInternal, it should be
-  // called with no lock acquired and it returns nullptr if the decoding failed.
-  std::unique_ptr<DecodedImage> GetExactSizeImageDecode(
+  // Get the decoded draw image for the given key and paint_image. Note that
+  // this function has to be called with no lock acquired, since it will acquire
+  // its own locks and might call DecodeImageInternal above. Note that
+  // when used internally, we still require that DrawWithImageFinished() is
+  // called afterwards.
+  DecodedDrawImage GetDecodedImageForDrawInternal(
       const ImageKey& key,
-      const PaintImage& image);
-
-  // GetSubrectImageDecode is similar to GetExactSizeImageDecode in that the
-  // image is decoded to exact scale. However, we extract a subrect (copy it
-  // out) and only return this subrect in order to cache a smaller amount of
-  // memory. Note that this uses GetExactSizeImageDecode to get the initial
-  // data, which ensures that we cache an unlocked version of the original image
-  // in case we need to extract multiple subrects (as would be the case in an
-  // atlas).
-  std::unique_ptr<DecodedImage> GetSubrectImageDecode(const ImageKey& key,
-                                                      const PaintImage& image);
-
-  // GetScaledImageDecode is called by DecodeImageInternal when the quality
-  // requires the image be scaled. Like DecodeImageInternal, it should be
-  // called with no lock acquired and it returns nullptr if the decoding or
-  // scaling failed.
-  std::unique_ptr<DecodedImage> GetScaledImageDecode(const ImageKey& key,
-                                                     const PaintImage& image);
-
-  void RefImage(const ImageKey& key);
-  void RefAtRasterImage(const ImageKey& key);
-  void UnrefAtRasterImage(const ImageKey& key);
-
-  // Helper function which dumps all images in a specific ImageMRUCache.
-  void DumpImageMemoryForCache(const ImageMRUCache& cache,
-                               const char* cache_name,
-                               base::trace_event::ProcessMemoryDump* pmd) const;
+      const PaintImage& paint_image);
 
   // Removes unlocked decoded images until the number of decoded images is
   // reduced within the given limit.
@@ -296,10 +271,22 @@ class CC_EXPORT SoftwareImageDecodeCache
                                            const TracingInfo& tracing_info,
                                            DecodeTaskType type);
 
-  void CacheDecodedImages(const ImageKey& key,
-                          std::unique_ptr<DecodedImage> decoded_image);
-  void CleanupDecodedImagesCache(const ImageKey& key,
-                                 ImageMRUCache::iterator it);
+  CacheEntry* AddCacheEntry(const ImageKey& key);
+
+  void DecodeImageIfNecessary(const ImageKey& key,
+                              const PaintImage& paint_image,
+                              CacheEntry* cache_entry);
+  void AddBudgetForImage(const ImageKey& key, CacheEntry* entry);
+  void RemoveBudgetForImage(const ImageKey& key, CacheEntry* entry);
+
+  std::unique_ptr<CacheEntry> DoDecodeImage(const ImageKey& key,
+                                            const PaintImage& image);
+  std::unique_ptr<CacheEntry> GenerateCacheEntryFromCandidate(
+      const ImageKey& key,
+      const DecodedDrawImage& candidate,
+      bool needs_extract_subset);
+
+  void UnrefImage(const ImageKey& key);
 
   std::unordered_map<ImageKey, scoped_refptr<TileTask>, ImageKeyHash>
       pending_in_raster_image_tasks_;
@@ -314,7 +301,6 @@ class CC_EXPORT SoftwareImageDecodeCache
 
   // Decoded images and ref counts (predecode path).
   ImageMRUCache decoded_images_;
-  std::unordered_map<ImageKey, int, ImageKeyHash> decoded_images_ref_counts_;
 
   // A map of PaintImage::FrameKey to the ImageKeys for cached decodes of this
   // PaintImage.
@@ -323,11 +309,6 @@ class CC_EXPORT SoftwareImageDecodeCache
                      PaintImage::FrameKeyHash>
       frame_key_to_image_keys_;
 
-  // Decoded image and ref counts (at-raster decode path).
-  ImageMRUCache at_raster_decoded_images_;
-  std::unordered_map<ImageKey, int, ImageKeyHash>
-      at_raster_decoded_images_ref_counts_;
-
   MemoryBudget locked_images_budget_;
 
   SkColorType color_type_;

cc/tiles/software_image_decode_cache_unittest.cc

@@ -654,6 +654,36 @@ TEST(SoftwareImageDecodeCacheTest, GetTaskForImageSameImage) {
   cache.UnrefImage(draw_image);
 }
 
+TEST(SoftwareImageDecodeCacheTest, GetTaskForImageProcessUnrefCancel) {
+  TestSoftwareImageDecodeCache cache;
+  PaintImage paint_image = CreatePaintImage(100, 100);
+  bool is_decomposable = true;
+  SkFilterQuality quality = kHigh_SkFilterQuality;
+
+  DrawImage draw_image(
+      paint_image, SkIRect::MakeWH(paint_image.width(), paint_image.height()),
+      quality, CreateMatrix(SkSize::Make(0.5f, 0.5f), is_decomposable),
+      PaintImage::kDefaultFrameIndex, DefaultColorSpace());
+  ImageDecodeCache::TaskResult result =
+      cache.GetTaskForImageAndRef(draw_image, ImageDecodeCache::TracingInfo());
+  EXPECT_TRUE(result.need_unref);
+  EXPECT_TRUE(result.task);
+
+  TestTileTaskRunner::ProcessTask(result.task.get());
+  cache.UnrefImage(draw_image);
+
+  result =
+      cache.GetTaskForImageAndRef(draw_image, ImageDecodeCache::TracingInfo());
+  EXPECT_TRUE(result.need_unref);
+  EXPECT_TRUE(result.task);
+
+  TestTileTaskRunner::CancelTask(result.task.get());
+  TestTileTaskRunner::CompleteTask(result.task.get());
+  // This is expected to pass instead of DCHECKing since we're reducing the ref
+  // for an image which isn't locked to begin with.
+  cache.UnrefImage(draw_image);
+}
+
 TEST(SoftwareImageDecodeCacheTest, GetTaskForImageSameImageDifferentQuality) {
   TestSoftwareImageDecodeCache cache;
   PaintImage paint_image = CreatePaintImage(100, 100);
@@ -1082,97 +1112,6 @@ TEST(SoftwareImageDecodeCacheTest,
   cache.DrawWithImageFinished(draw_image, another_decoded_draw_image);
 }
 
-TEST(SoftwareImageDecodeCacheTest,
-     GetDecodedImageForDrawAtRasterDecodeDoesNotPreventTasks) {
-  TestSoftwareImageDecodeCache cache;
-  bool is_decomposable = true;
-  SkFilterQuality quality = kHigh_SkFilterQuality;
-
-  PaintImage paint_image = CreatePaintImage(100, 100);
-  DrawImage draw_image(
-      paint_image, SkIRect::MakeWH(paint_image.width(), paint_image.height()),
-      quality, CreateMatrix(SkSize::Make(0.5f, 0.5f), is_decomposable),
-      PaintImage::kDefaultFrameIndex, DefaultColorSpace());
-
-  DecodedDrawImage decoded_draw_image =
-      cache.GetDecodedImageForDraw(draw_image);
-  EXPECT_TRUE(decoded_draw_image.image());
-  EXPECT_EQ(50, decoded_draw_image.image()->width());
-  EXPECT_EQ(50, decoded_draw_image.image()->height());
-  EXPECT_FLOAT_EQ(0.5f, decoded_draw_image.scale_adjustment().width());
-  EXPECT_FLOAT_EQ(0.5f, decoded_draw_image.scale_adjustment().height());
-  EXPECT_EQ(kLow_SkFilterQuality, decoded_draw_image.filter_quality());
-  EXPECT_FALSE(decoded_draw_image.is_scale_adjustment_identity());
-  EXPECT_TRUE(decoded_draw_image.is_at_raster_decode());
-
-  ImageDecodeCache::TaskResult result =
-      cache.GetTaskForImageAndRef(draw_image, ImageDecodeCache::TracingInfo());
-  EXPECT_TRUE(result.need_unref);
-  EXPECT_TRUE(result.task);
-
-  TestTileTaskRunner::ProcessTask(result.task.get());
-
-  DecodedDrawImage another_decoded_draw_image =
-      cache.GetDecodedImageForDraw(draw_image);
-  // This should get the new decoded/locked image, not the one we're using at
-  // raster.
-  // TODO(vmpstr): We can possibly optimize this so that the decode simply moves
-  // the image to the right spot.
-  EXPECT_NE(decoded_draw_image.image()->uniqueID(),
-            another_decoded_draw_image.image()->uniqueID());
-  EXPECT_FALSE(another_decoded_draw_image.is_at_raster_decode());
-
-  cache.DrawWithImageFinished(draw_image, decoded_draw_image);
-  cache.DrawWithImageFinished(draw_image, another_decoded_draw_image);
-  cache.UnrefImage(draw_image);
-}
-
-TEST(SoftwareImageDecodeCacheTest,
-     GetDecodedImageForDrawAtRasterDecodeIsUsedForLockedCache) {
-  TestSoftwareImageDecodeCache cache;
-  bool is_decomposable = true;
-  SkFilterQuality quality = kHigh_SkFilterQuality;
-
-  PaintImage paint_image = CreatePaintImage(100, 100);
-  DrawImage draw_image(
-      paint_image, SkIRect::MakeWH(paint_image.width(), paint_image.height()),
-      quality, CreateMatrix(SkSize::Make(0.5f, 0.5f), is_decomposable),
-      PaintImage::kDefaultFrameIndex, DefaultColorSpace());
-
-  DecodedDrawImage decoded_draw_image =
-      cache.GetDecodedImageForDraw(draw_image);
-  EXPECT_TRUE(decoded_draw_image.image());
-  EXPECT_EQ(50, decoded_draw_image.image()->width());
-  EXPECT_EQ(50, decoded_draw_image.image()->height());
-  EXPECT_FLOAT_EQ(0.5f, decoded_draw_image.scale_adjustment().width());
-  EXPECT_FLOAT_EQ(0.5f, decoded_draw_image.scale_adjustment().height());
-  EXPECT_EQ(kLow_SkFilterQuality, decoded_draw_image.filter_quality());
-  EXPECT_FALSE(decoded_draw_image.is_scale_adjustment_identity());
-  EXPECT_TRUE(decoded_draw_image.is_at_raster_decode());
-
-  ImageDecodeCache::TaskResult result =
-      cache.GetTaskForImageAndRef(draw_image, ImageDecodeCache::TracingInfo());
-  EXPECT_TRUE(result.need_unref);
-  EXPECT_TRUE(result.task);
-
-  // If we finish the draw here, then we will use it for the locked decode
-  // instead of decoding again.
-  cache.DrawWithImageFinished(draw_image, decoded_draw_image);
-
-  TestTileTaskRunner::ProcessTask(result.task.get());
-
-  DecodedDrawImage another_decoded_draw_image =
-      cache.GetDecodedImageForDraw(draw_image);
-  // This should get the decoded/locked image which we originally decoded at
-  // raster time, since it's now in the locked cache.
-  EXPECT_EQ(decoded_draw_image.image()->uniqueID(),
-            another_decoded_draw_image.image()->uniqueID());
-  EXPECT_FALSE(another_decoded_draw_image.is_at_raster_decode());
-
-  cache.DrawWithImageFinished(draw_image, another_decoded_draw_image);
-  cache.UnrefImage(draw_image);
-}
-
 TEST(SoftwareImageDecodeCacheTest, ZeroSizedImagesAreSkipped) {
   TestSoftwareImageDecodeCache cache;
   bool is_decomposable = true;
@@ -1638,20 +1577,18 @@ TEST(SoftwareImageDecodeCacheTest, RemoveUnusedImage) {
   std::vector<PaintImage::FrameKey> frame_keys;
 
   for (int i = 0; i < 10; ++i) {
+    SCOPED_TRACE(i);
     PaintImage paint_image = CreatePaintImage(100, 100);
     DrawImage draw_image(
         paint_image, SkIRect::MakeWH(paint_image.width(), paint_image.height()),
         quality, CreateMatrix(SkSize::Make(1.0f, 1.0f), is_decomposable),
         PaintImage::kDefaultFrameIndex, DefaultColorSpace());
     frame_keys.push_back(draw_image.frame_key());
-    DecodedDrawImage decoded_draw_image =
-        cache.GetDecodedImageForDraw(draw_image);
     ImageDecodeCache::TaskResult result = cache.GetTaskForImageAndRef(
         draw_image, ImageDecodeCache::TracingInfo());
     EXPECT_TRUE(result.need_unref);
     EXPECT_TRUE(result.task);
     TestTileTaskRunner::ProcessTask(result.task.get());
-    cache.DrawWithImageFinished(draw_image, decoded_draw_image);
     cache.UnrefImage(draw_image);
   }
 
@@ -1718,5 +1655,35 @@ TEST(SoftwareImageDecodeCacheTest, CacheDecodesExpectedFrames) {
   cache.DrawWithImageFinished(subset_draw_image, decoded_image);
 }
 
+TEST(SoftwareImageDecodeCacheTest, SizeSubrectingIsHandled) {
+  const int min_dimension = 4 * 1024 + 2;
+  TestSoftwareImageDecodeCache cache;
+  bool is_decomposable = true;
+  SkFilterQuality quality = kLow_SkFilterQuality;
+
+  auto paint_image =
+      CreateDiscardablePaintImage(gfx::Size(min_dimension, min_dimension),
+                                  DefaultColorSpace().ToSkColorSpace(), false);
+  DrawImage draw_image(paint_image, SkIRect::MakeXYWH(0, 0, 10, 10), quality,
+                       CreateMatrix(SkSize::Make(1.f, 1.f), is_decomposable),
+                       PaintImage::kDefaultFrameIndex, DefaultColorSpace());
+
+  ImageDecodeCache::TaskResult result =
+      cache.GetTaskForImageAndRef(draw_image, ImageDecodeCache::TracingInfo());
+  EXPECT_TRUE(result.task);
+  EXPECT_TRUE(result.need_unref);
+
+  TestTileTaskRunner::ProcessTask(result.task.get());
+
+  DecodedDrawImage decoded_draw_image =
+      cache.GetDecodedImageForDraw(draw_image);
+  // Since we didn't allocate any backing for the memory, we expect this to be
+  // false. This test is here to ensure that we at least got to the point where
+  // we tried to decode something instead of recursing infinitely.
+  EXPECT_FALSE(decoded_draw_image.image());
+  cache.DrawWithImageFinished(draw_image, decoded_draw_image);
+  cache.UnrefImage(draw_image);
+}
+
 }  // namespace
 }  // namespace cc