cc: Add scaling for checkered images.

cc/tiles/checker_image_tracker.cc

Index: cc/tiles/checker_image_tracker.cc
diff --git a/cc/tiles/checker_image_tracker.cc b/cc/tiles/checker_image_tracker.cc
index 022269d87a95fa52d4661581e4c527c4799f32d4..bdcf00f378a53705ed637294a8b64e35f3720536 100644
--- a/cc/tiles/checker_image_tracker.cc
+++ b/cc/tiles/checker_image_tracker.cc
@@ -90,9 +90,9 @@ void CheckerImageTracker::ClearTracker(bool can_clear_decode_policy_tracking) {
       auto it = image_async_decode_state_.find(image_id);
 
       DCHECK(it != image_async_decode_state_.end());
-      DCHECK_EQ(it->second, DecodePolicy::SYNC_DECODED_ONCE);
+      DCHECK_EQ(it->second.policy, DecodePolicy::SYNC_DECODED_ONCE);
 
-      it->second = DecodePolicy::ASYNC;
+      it->second.policy = DecodePolicy::ASYNC;
     }
   }
   images_pending_invalidation_.clear();
@@ -119,22 +119,22 @@ void CheckerImageTracker::DidFinishImageDecode(
     return;
   }
 
-  it->second = DecodePolicy::SYNC_DECODED_ONCE;
+  it->second.policy = DecodePolicy::SYNC_DECODED_ONCE;
   images_pending_invalidation_.insert(image_id);
   ScheduleNextImageDecode();
   client_->NeedsInvalidationForCheckerImagedTiles();
 }
 
-bool CheckerImageTracker::ShouldCheckerImage(const PaintImage& image,
+bool CheckerImageTracker::ShouldCheckerImage(const DrawImage& draw_image,
                                              WhichTree tree) {
+  const PaintImage& image = draw_image.paint_image();
+  PaintImage::Id image_id = image.stable_id();
   TRACE_EVENT1("cc", "CheckerImageTracker::ShouldCheckerImage", "image_id",
-               image.stable_id());
+               image_id);
 
   if (!enable_checker_imaging_)
     return false;
 
-  PaintImage::Id image_id = image.stable_id();
-
   // If the image was invalidated on the current sync tree and the tile is
   // for the active tree, continue checkering it on the active tree to ensure
   // the image update is atomic for the frame.
@@ -150,24 +150,65 @@ bool CheckerImageTracker::ShouldCheckerImage(const PaintImage& image,
     return true;
   }
 
-  auto insert_result =
-      image_async_decode_state_.insert(std::pair<PaintImage::Id, DecodePolicy>(
-          image_id, DecodePolicy::SYNC_PERMANENT));
+  auto insert_result = image_async_decode_state_.insert(
+      std::pair<PaintImage::Id, DecodeState>(image_id, DecodeState()));
   auto it = insert_result.first;
   if (insert_result.second) {
-    bool can_checker_image =
-        image.animation_type() == PaintImage::AnimationType::STATIC &&
+    bool complete =
         image.completion_state() == PaintImage::CompletionState::DONE;
-    if (can_checker_image) {
+    bool static_image =
+        image.animation_type() == PaintImage::AnimationType::STATIC;
+    // Only checker images that are static and completely loaded.
+    if (complete && static_image) {
       size_t size = SafeSizeOfImage(image.sk_image().get());
-      it->second = (size >= kMinImageSizeToCheckerBytes &&
-                    size <= image_controller_->image_cache_max_limit_bytes())
-                       ? DecodePolicy::ASYNC
-                       : DecodePolicy::SYNC_PERMANENT;
+      bool too_small = size < kMinImageSizeToCheckerBytes;
+      bool too_large = size > image_controller_->image_cache_max_limit_bytes();
+      it->second.policy = (too_small || too_large)

[ericrk, 2017/06/06 22:39:51]

The policy already defaults to SYNC_PERMANENT, and you rely on this in the other
branch. Maybe cleaner to just say:

if (!too_small && !too_large)
  it->second.policy = DecodePolicy::ASYNC

Or might be cleaner to just pull these out and have a single if statement? WDYT?

[Khushal, 2017/06/07 00:43:31]

How does it look now? I broke the size part seperately because I wanted to
capture what causes the veto in the trace. Looks cleaner too now.

+                              ? DecodePolicy::SYNC_PERMANENT
+                              : DecodePolicy::ASYNC;
+      TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
+                   "CheckerImageTracker::CheckerImagingDecision", "too_small",
+                   too_small, "too_large", too_large);
+    } else {
+      TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
+                   "CheckerImageTracker::CanNotChecker", "complete", complete,
+                   "static", static_image);

[ericrk, 2017/06/06 22:39:51]

nit: The naming of these trace values seems a bit odd - doesn't too_small /
too_large also mean we can't checker? Why is that tracked separately from
CanNotChecker?

[Khushal, 2017/06/07 00:43:31]

You're right. Its because I went to look for TRACE_EVENT3. :P Now I see there is
a better way.

     }
   }
 
-  return it->second == DecodePolicy::ASYNC;
+  // Update the decode state from the latest image we have seen. Note that it
+  // is not necessary to perform this in the early out cases above since in
+  // each of those cases the image has already been decoded.
+  UpdateDecodeState(draw_image, image_id, &it->second);
+
+  return it->second.policy == DecodePolicy::ASYNC;
+}
+
+void CheckerImageTracker::UpdateDecodeState(const DrawImage& draw_image,
+                                            PaintImage::Id paint_image_id,
+                                            DecodeState* decode_state) {
+  // If the policy is not async then either we decoded this image already or
+  // we decided not to ever checker it.
+  if (decode_state->policy != DecodePolicy::ASYNC)
+    return;
+
+  // If the decode is already in flight, then we will have to live with what we
+  // have now.
+  if (outstanding_image_decode_.has_value() &&
+      outstanding_image_decode_.value().stable_id() == paint_image_id) {
+    return;
+  }
+
+  // Choose the max scale, going up till the original decode size, and filter
+  // quality. This keeps the memory usage to the minimum possible while still
+  // increasing the possibility of getting a cache hit.
+  decode_state->scale.fWidth = std::min(

[ericrk, 2017/06/06 22:39:51]

Do we really not want to scale up at this point? This has no impact on GPU
(which will never scale over 1, even if requested), but seems like it could be
nice for SW, getting a potentially expensive scale out of the way? Or does the
memory usage outweigh the potential perf gains?

[Khushal, 2017/06/07 00:43:31]

I don't think the memory impact would be much. Now that I think about it, it
might be better since right now we keep the original decode locked in addition
to the scaled version. There is a possibility that if we see the same image with
different scales and high filter quality then the software cache wouldn't use
it, because it takes the exact scale value (could happen during pinch-zoom). But
I guess we can consider that case rare enough to not avoid avoid the performance
benefit of scaling.

+      1.0f, std::max(decode_state->scale.fWidth, draw_image.scale().fWidth));
+  decode_state->scale.fHeight = std::min(

[ericrk, 2017/06/06 22:39:51]

Is this ever called with a non-default DecodeState? do you need to consider the
decode_state scale value? 

[Khushal, 2017/06/07 00:43:31]

It will be called with a non-default state if we see 2 calls for the same
PaintImage. That's why its keeping the max of what we have seen so far.

+      1.0f, std::max(decode_state->scale.fHeight, draw_image.scale().fHeight));
+  decode_state->filter_quality =
+      std::max(decode_state->filter_quality, draw_image.filter_quality());
+  decode_state->color_space = draw_image.target_color_space();
 }
 
 void CheckerImageTracker::ScheduleNextImageDecode() {
@@ -178,6 +219,7 @@ void CheckerImageTracker::ScheduleNextImageDecode() {
   if (outstanding_image_decode_.has_value())
     return;
 
+  DrawImage draw_image;
   while (!image_decode_queue_.empty()) {
     auto candidate = std::move(image_decode_queue_.front());
     image_decode_queue_.erase(image_decode_queue_.begin());
@@ -189,9 +231,14 @@ void CheckerImageTracker::ScheduleNextImageDecode() {
     PaintImage::Id image_id = candidate.stable_id();
     auto it = image_async_decode_state_.find(image_id);
     DCHECK(it != image_async_decode_state_.end());
-    if (it->second != DecodePolicy::ASYNC)
+    if (it->second.policy != DecodePolicy::ASYNC)
       continue;
 
+    draw_image = DrawImage(candidate, candidate.sk_image()->bounds(),
+                           it->second.filter_quality,
+                           SkMatrix::MakeScale(it->second.scale.width(),
+                                               it->second.scale.height()),
+                           it->second.color_space);
     outstanding_image_decode_.emplace(candidate);
     break;
   }
@@ -209,9 +256,8 @@ void CheckerImageTracker::ScheduleNextImageDecode() {
                            image_id);
   ImageController::ImageDecodeRequestId request_id =
       image_controller_->QueueImageDecode(
-          outstanding_image_decode_.value().sk_image(),
-          base::Bind(&CheckerImageTracker::DidFinishImageDecode,
-                     weak_factory_.GetWeakPtr(), image_id));
+          draw_image, base::Bind(&CheckerImageTracker::DidFinishImageDecode,
+                                 weak_factory_.GetWeakPtr(), image_id));
 
   image_id_to_decode_.emplace(image_id, base::MakeUnique<ScopedDecodeHolder>(
                                             image_controller_, request_id));