Hold pre-decoded images until use or 250ms rather than 2 frames

We currently unlock pre-decoded images after 2 frames of any type. This doesn't always give callers a chance to use their images (if main thread is a bit backed up). Instead we should wait for the caller to actually use the image. Additionally, we drop images after 250ms to prevent excessive growth if a user fails to use images in a timely manner. See the linked bug for example traces which show the benefits. R=khushalsagar Bug: 869491 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Icd94709aad5c076b1ba295495eaab24dc893ebdb Reviewed-on: https://chromium-review.googlesource.com/1159203 Commit-Queue: Eric Karl <[email protected]> Reviewed-by: Khushal <[email protected]> Cr-Commit-Position: refs/heads/master@{#583171}
feiyue1206 · Aug 15, 2018 · 520ce5d · 520ce5d
1 parent 8d5f940
commit 520ce5d
Show file tree

Hide file tree

Showing 6 changed files with 343 additions and 43 deletions.
diff --git a/cc/tiles/decoded_image_tracker.cc b/cc/tiles/decoded_image_tracker.cc
@@ -7,17 +7,33 @@
 
 namespace cc {
 namespace {
-const int kNumFramesToLock = 2;
+// Timeout images after 250ms, whether or not they've been used. This prevents
+// unbounded cache usage.
+const int64_t kTimeoutDurationMs = 250;
 }  // namespace
 
-DecodedImageTracker::DecodedImageTracker(ImageController* controller)
-    : image_controller_(controller) {
+DecodedImageTracker::ImageLock::ImageLock(
+    DecodedImageTracker* tracker,
+    ImageController::ImageDecodeRequestId request_id,
+    base::TimeTicks lock_time)
+    : tracker_(tracker), request_id_(request_id), lock_time_(lock_time) {}
+
+DecodedImageTracker::ImageLock::~ImageLock() {
+  tracker_->image_controller_->UnlockImageDecode(request_id_);
+}
+
+DecodedImageTracker::DecodedImageTracker(
+    ImageController* controller,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : image_controller_(controller),
+      task_runner_(std::move(task_runner)),
+      now_fn_(base::Bind(&base::TimeTicks::Now)),
+      weak_ptr_factory_(this) {
   DCHECK(image_controller_);
 }
 
 DecodedImageTracker::~DecodedImageTracker() {
-  for (auto& pair : locked_images_)
-    image_controller_->UnlockImageDecode(pair.first);
+  UnlockAllImages();
 }
 
 void DecodedImageTracker::QueueImageDecode(
@@ -35,38 +51,74 @@ void DecodedImageTracker::QueueImageDecode(
   DrawImage draw_image(image, image_bounds, kNone_SkFilterQuality,
                        SkMatrix::I(), frame_index, target_color_space);
   image_controller_->QueueImageDecode(
-      draw_image, base::Bind(&DecodedImageTracker::ImageDecodeFinished,
-                             base::Unretained(this), callback));
+      draw_image,
+      base::Bind(&DecodedImageTracker::ImageDecodeFinished,
+                 base::Unretained(this), callback, image.stable_id()));
 }
 
-void DecodedImageTracker::NotifyFrameFinished() {
-  // Go through the images and if the frame ref count goes to 0, unlock the
-  // image in the controller.
-  for (auto it = locked_images_.begin(); it != locked_images_.end();) {
-    auto id = it->first;
-    int& ref_count = it->second;
-    if (--ref_count != 0) {
-      ++it;
-      continue;
-    }
-    image_controller_->UnlockImageDecode(id);
-    it = locked_images_.erase(it);
-  }
+void DecodedImageTracker::UnlockAllImages() {
+  locked_images_.clear();
+}
+
+void DecodedImageTracker::OnImagesUsedInDraw(
+    const std::vector<DrawImage>& draw_images) {
+  for (const DrawImage& draw_image : draw_images)
+    locked_images_.erase(draw_image.paint_image().stable_id());
 }
 
 void DecodedImageTracker::ImageDecodeFinished(
     const base::Callback<void(bool)>& callback,
-    ImageController::ImageDecodeRequestId id,
+    PaintImage::Id image_id,
+    ImageController::ImageDecodeRequestId request_id,
     ImageController::ImageDecodeResult result) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "DecodedImageTracker::ImageDecodeFinished");
 
-  if (result == ImageController::ImageDecodeResult::SUCCESS)
-    locked_images_.push_back(std::make_pair(id, kNumFramesToLock));
+  if (result == ImageController::ImageDecodeResult::SUCCESS) {
+    // If this image already exists, just replace it with the new (latest)
+    // decode.
+    locked_images_.erase(image_id);
+    locked_images_.emplace(
+        image_id, std::make_unique<ImageLock>(this, request_id, now_fn_.Run()));
+    EnqueueTimeout();
+  }
   bool decode_succeeded =
       result == ImageController::ImageDecodeResult::SUCCESS ||
       result == ImageController::ImageDecodeResult::DECODE_NOT_REQUIRED;
   callback.Run(decode_succeeded);
 }
 
+void DecodedImageTracker::OnTimeoutImages() {
+  timeout_pending_ = false;
+  if (locked_images_.size() == 0)
+    return;
+
+  auto now = now_fn_.Run();
+  auto timeout = base::TimeDelta::FromMilliseconds(kTimeoutDurationMs);
+  for (auto it = locked_images_.begin(); it != locked_images_.end();) {
+    auto& image = it->second;
+    if (now - image->lock_time() < timeout) {
+      ++it;
+      continue;
+    }
+    it = locked_images_.erase(it);
+  }
+
+  EnqueueTimeout();
+}
+
+void DecodedImageTracker::EnqueueTimeout() {
+  if (timeout_pending_)
+    return;
+  if (locked_images_.size() == 0)
+    return;
+
+  timeout_pending_ = true;
+  task_runner_->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&DecodedImageTracker::OnTimeoutImages,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::TimeDelta::FromMilliseconds(kTimeoutDurationMs));
+}
+
 }  // namespace cc
diff --git a/cc/tiles/decoded_image_tracker.h b/cc/tiles/decoded_image_tracker.h
@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/time/time.h"
 #include "cc/cc_export.h"
 #include "cc/tiles/image_controller.h"
 
@@ -24,7 +25,9 @@ namespace cc {
 // are silently ignored.
 class CC_EXPORT DecodedImageTracker {
  public:
-  explicit DecodedImageTracker(ImageController* controller);
+  explicit DecodedImageTracker(
+      ImageController* controller,
+      scoped_refptr<base::SequencedTaskRunner> task_runner);
   ~DecodedImageTracker();
 
   // Request that the given image be decoded. This issues a callback upon
@@ -33,18 +36,57 @@ class CC_EXPORT DecodedImageTracker {
   void QueueImageDecode(const PaintImage& image,
                         const gfx::ColorSpace& target_color_space,
                         const base::Callback<void(bool)>& callback);
-  void NotifyFrameFinished();
+
+  // Unlock all locked images - used to respond to memory pressure or
+  // application background.
+  void UnlockAllImages();
+
+  // Notifies the tracker that images have been used, allowing it to
+  // unlock them.
+  void OnImagesUsedInDraw(const std::vector<DrawImage>& draw_images);
+
+  using NowFn = base::Callback<base::TimeTicks()>;
+  void SetNowFunctionForTesting(NowFn now_fn) { now_fn_ = now_fn; }
+
+  // Test only functions:
+  size_t NumLockedImagesForTesting() const { return locked_images_.size(); }
 
  private:
   friend class DecodedImageTrackerTest;
 
   void ImageDecodeFinished(const base::Callback<void(bool)>& callback,
-                           ImageController::ImageDecodeRequestId id,
+                           PaintImage::Id image_id,
+                           ImageController::ImageDecodeRequestId request_id,
                            ImageController::ImageDecodeResult result);
+  void OnTimeoutImages();
+  void EnqueueTimeout();
 
   ImageController* image_controller_;
-  std::vector<std::pair<ImageController::ImageDecodeRequestId, int>>
-      locked_images_;
+
+  // Helper class tracking a locked image decode. Automatically releases the
+  // lock using the provided DecodedImageTracker* on destruction.
+  class ImageLock {
+   public:
+    ImageLock(DecodedImageTracker* tracker,
+              ImageController::ImageDecodeRequestId request_id,
+              base::TimeTicks lock_time);
+    ~ImageLock();
+    base::TimeTicks lock_time() const { return lock_time_; }
+
+   private:
+    DecodedImageTracker* tracker_;
+    ImageController::ImageDecodeRequestId request_id_;
+    base::TimeTicks lock_time_;
+    DISALLOW_COPY_AND_ASSIGN(ImageLock);
+  };
+  base::flat_map<PaintImage::Id, std::unique_ptr<ImageLock>> locked_images_;
+  bool timeout_pending_ = false;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+
+  // Defaults to base::TimeTicks::Now(), but overrideable for testing.
+  NowFn now_fn_;
+
+  base::WeakPtrFactory<DecodedImageTracker> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DecodedImageTracker);
 };

diff --git a/cc/tiles/decoded_image_tracker_unittest.cc b/cc/tiles/decoded_image_tracker_unittest.cc
@@ -5,6 +5,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "cc/paint/paint_image_builder.h"
 #include "cc/test/skia_common.h"
 #include "cc/tiles/decoded_image_tracker.h"
@@ -63,15 +64,22 @@ class TestImageController : public ImageController {
 
 class DecodedImageTrackerTest : public testing::Test {
  public:
-  DecodedImageTrackerTest() : decoded_image_tracker_(&image_controller_) {}
+  DecodedImageTrackerTest()
+      : task_runner_(new base::TestMockTimeTaskRunner()),
+        decoded_image_tracker_(&image_controller_, task_runner_) {
+    decoded_image_tracker_.SetNowFunctionForTesting(
+        base::Bind(&base::TestMockTimeTaskRunner::NowTicks, task_runner_));
+  }
 
   TestImageController* image_controller() { return &image_controller_; }
   DecodedImageTracker* decoded_image_tracker() {
     return &decoded_image_tracker_;
   }
+  base::TestMockTimeTaskRunner* task_runner() { return task_runner_.get(); }
 
  private:
   TestImageController image_controller_;
+  scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
   DecodedImageTracker decoded_image_tracker_;
 };
 
@@ -85,7 +93,7 @@ TEST_F(DecodedImageTrackerTest, QueueImageLocksImages) {
   EXPECT_EQ(1u, image_controller()->num_locked_images());
 }
 
-TEST_F(DecodedImageTrackerTest, NotifyFrameFinishedUnlocksImages) {
+TEST_F(DecodedImageTrackerTest, Colorspace) {
   bool locked = false;
   gfx::ColorSpace decoded_color_space(
       gfx::ColorSpace::PrimaryID::XYZ_D50,
@@ -96,11 +104,6 @@ TEST_F(DecodedImageTrackerTest, NotifyFrameFinishedUnlocksImages) {
       paint_image, decoded_color_space,
       base::Bind([](bool* locked, bool success) { *locked = true; },
                  base::Unretained(&locked)));
-  EXPECT_TRUE(locked);
-  EXPECT_EQ(1u, image_controller()->num_locked_images());
-
-  decoded_image_tracker()->NotifyFrameFinished();
-  EXPECT_EQ(1u, image_controller()->num_locked_images());
 
   // Check that the decoded color space images are locked, but if the color
   // space differs then that image is not locked. Note that we use the high
@@ -114,19 +117,97 @@ TEST_F(DecodedImageTrackerTest, NotifyFrameFinishedUnlocksImages) {
                             kHigh_SkFilterQuality, SkMatrix::I(),
                             PaintImage::kDefaultFrameIndex, srgb_color_space);
   EXPECT_FALSE(image_controller()->IsDrawImageLocked(srgb_draw_image));
+}
+
+TEST_F(DecodedImageTrackerTest, ImagesTimeOut) {
+  // Add an image, this will start a 250ms timeout to release it.
+  bool locked = false;
+  decoded_image_tracker()->QueueImageDecode(
+      CreateDiscardablePaintImage(gfx::Size(1, 1)), gfx::ColorSpace(),
+      base::Bind([](bool* locked, bool success) { *locked = true; },
+                 base::Unretained(&locked)));
+  EXPECT_TRUE(locked);
+  EXPECT_EQ(1u, image_controller()->num_locked_images());
+
+  // Advance by 150ms, the image should still be locked.
+  task_runner()->FastForwardBy(base::TimeDelta::FromMilliseconds(150));
+  EXPECT_EQ(1u, image_controller()->num_locked_images());
+
+  // Add an image, this will not start a new timeout, as one is pending.
+  decoded_image_tracker()->QueueImageDecode(
+      CreateDiscardablePaintImage(gfx::Size(1, 1)), gfx::ColorSpace(),
+      base::Bind([](bool* locked, bool success) { *locked = true; },
+                 base::Unretained(&locked)));
+  EXPECT_TRUE(locked);
+  EXPECT_EQ(2u, image_controller()->num_locked_images());
+
+  // Advance by 100ms, we our first image should be released.
+  // Trigger a single commit, the first image should be unlocked.
+  task_runner()->FastForwardBy(base::TimeDelta::FromMilliseconds(100));
+  EXPECT_EQ(1u, image_controller()->num_locked_images());
+
+  // Advance by another 250ms, our second image should release.
+  task_runner()->FastForwardBy(base::TimeDelta::FromMilliseconds(250));
+  EXPECT_EQ(0u, image_controller()->num_locked_images());
+}
 
-  locked = false;
+TEST_F(DecodedImageTrackerTest, ImageUsedInDraw) {
+  // Insert two images:
+  bool locked = false;
+  auto paint_image_1 = CreateDiscardablePaintImage(gfx::Size(1, 1));
   decoded_image_tracker()->QueueImageDecode(
-      CreateDiscardablePaintImage(gfx::Size(1, 1)), decoded_color_space,
+      paint_image_1, gfx::ColorSpace(),
+      base::Bind([](bool* locked, bool success) { *locked = true; },
+                 base::Unretained(&locked)));
+  EXPECT_TRUE(locked);
+  EXPECT_EQ(1u, image_controller()->num_locked_images());
+
+  auto paint_image_2 = CreateDiscardablePaintImage(gfx::Size(1, 1));
+  decoded_image_tracker()->QueueImageDecode(
+      paint_image_2, gfx::ColorSpace(),
       base::Bind([](bool* locked, bool success) { *locked = true; },
                  base::Unretained(&locked)));
   EXPECT_TRUE(locked);
   EXPECT_EQ(2u, image_controller()->num_locked_images());
 
-  decoded_image_tracker()->NotifyFrameFinished();
+  // Create dummy draw images for each:
+  DrawImage draw_image_1(paint_image_1, SkIRect::MakeWH(1, 1),
+                         kHigh_SkFilterQuality, SkMatrix::I(), 0,
+                         gfx::ColorSpace());
+  DrawImage draw_image_2(paint_image_2, SkIRect::MakeWH(1, 1),
+                         kHigh_SkFilterQuality, SkMatrix::I(), 0,
+                         gfx::ColorSpace());
+
+  // Both should be in the cache:
+  EXPECT_TRUE(image_controller()->IsDrawImageLocked(draw_image_1));
+  EXPECT_TRUE(image_controller()->IsDrawImageLocked(draw_image_2));
+
+  // Pretend we've drawn with image 2.
+  decoded_image_tracker()->OnImagesUsedInDraw({draw_image_2});
+
+  // Only image 1 should now be in the cache.
+  EXPECT_TRUE(image_controller()->IsDrawImageLocked(draw_image_1));
+  EXPECT_FALSE(image_controller()->IsDrawImageLocked(draw_image_2));
+}
+
+TEST_F(DecodedImageTrackerTest, UnlockAllImages) {
+  // Insert two images:
+  bool locked = false;
+  decoded_image_tracker()->QueueImageDecode(
+      CreateDiscardablePaintImage(gfx::Size(1, 1)), gfx::ColorSpace(),
+      base::Bind([](bool* locked, bool success) { *locked = true; },
+                 base::Unretained(&locked)));
+  EXPECT_TRUE(locked);
   EXPECT_EQ(1u, image_controller()->num_locked_images());
+  decoded_image_tracker()->QueueImageDecode(
+      CreateDiscardablePaintImage(gfx::Size(1, 1)), gfx::ColorSpace(),
+      base::Bind([](bool* locked, bool success) { *locked = true; },
+                 base::Unretained(&locked)));
+  EXPECT_TRUE(locked);
+  EXPECT_EQ(2u, image_controller()->num_locked_images());
 
-  decoded_image_tracker()->NotifyFrameFinished();
+  // Unlock all images.
+  decoded_image_tracker()->UnlockAllImages();
   EXPECT_EQ(0u, image_controller()->num_locked_images());
 }
 

diff --git a/cc/tiles/tile_manager.cc b/cc/tiles/tile_manager.cc
@@ -362,7 +362,7 @@ TileManager::TileManager(
       did_oom_on_last_assign_(false),
       image_controller_(origin_task_runner,
                         std::move(image_worker_task_runner)),
-      decoded_image_tracker_(&image_controller_),
+      decoded_image_tracker_(&image_controller_, origin_task_runner),
       checker_image_tracker_(&image_controller_,
                              this,
                              tile_manager_settings_.enable_checker_imaging,
@@ -1032,8 +1032,10 @@ void TileManager::ScheduleTasks(PrioritizedWorkToSchedule work_to_schedule) {
       prepare_tiles_count_, TilePriority::SOON,
       ImageDecodeCache::TaskType::kInRaster);
   std::vector<scoped_refptr<TileTask>> new_locked_image_tasks =
-      image_controller_.SetPredecodeImages(std::move(new_locked_images),
-                                           tracing_info);
+      image_controller_.SetPredecodeImages(new_locked_images, tracing_info);
+  // Notify |decoded_image_tracker_| after |image_controller_| to ensure we've
+  // taken new refs on the images before releasing the predecode API refs.
+  decoded_image_tracker_.OnImagesUsedInDraw(new_locked_images);
   work_to_schedule.extra_prepaint_images.clear();
 
   for (auto& task : new_locked_image_tasks) {
@@ -1155,6 +1157,9 @@ scoped_refptr<TileTask> TileManager::CreateRasterTask(
   bool has_at_raster_images = false;
   image_controller_.GetTasksForImagesAndRef(
       &sync_decoded_images, &decode_tasks, &has_at_raster_images, tracing_info);
+  // Notify |decoded_image_tracker_| after |image_controller_| to ensure we've
+  // taken new refs on the images before releasing the predecode API refs.
+  decoded_image_tracker_.OnImagesUsedInDraw(sync_decoded_images);
 
   const bool has_checker_images = !checkered_images.empty();
   tile->set_raster_task_scheduled_with_checker_images(has_checker_images);