cc: Add checker-imaging support to TileManager.

cc/tiles/checker_image_tracker.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.

[vmpstr, 2017/02/03 23:42:32]

2017

[Khushal, 2017/02/07 00:25:32]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "cc/tiles/checker_image_tracker.h"
+
+#include "base/bind.h"
+#include "cc/playback/raster_source.h"
+#include "cc/tiles/picture_layer_tiling.h"
+#include "cc/tiles/prioritized_tile.h"
+#include "cc/tiles/tile.h"
+#include "cc/trees/layer_tree_impl.h"
+
+namespace cc {
+namespace {
+// The minimum size of an image that we should consider checkering.
+size_t kMinImageSizeToCheckerBytes = 512 * 1024;
+}  // namespace
+
+CheckerImageTracker::CheckerImageTracker(ImageController* image_controller,
+                                         CheckerImageTrackerClient* client,
+                                         bool enable_checker_imaging)
+    : image_controller_(image_controller),
+      client_(client),
+      enable_checker_imaging_(enable_checker_imaging),
+      weak_factory_(this) {}
+
+CheckerImageTracker::~CheckerImageTracker() {
+  // Unlock all images pending decode requests.
+  for (auto it : image_id_to_decode_request_id_) {

[vmpstr, 2017/02/03 23:42:32]

nit: no braces

[Khushal, 2017/02/07 00:25:32]

Done.

+    image_controller_->UnlockImageDecode(it.second);
+  }
+}
+
+void CheckerImageTracker::FilterImagesForCheckeringForTile(
+    const PrioritizedTile& prioritized_tile,

[vmpstr, 2017/02/03 23:42:32]

I'd prefer if we don't expose prioritized tile here, instead just pass a tile
and a raster source.

Or better yet, make std::vector<DrawImage>* images as a an in/out parameter.
That is, you don't need to pass any tile here. You just need draw images and
tree, I think? Just pass that. It would make it easier to test as well.

[Khushal, 2017/02/07 00:25:32]

Awesome suggestion, eliminating all of these makes it much easier to test. Done.

+    std::vector<DrawImage>* images,
+    std::unordered_set<ImageId>* checkered_images) {
+  DCHECK(images->empty());
+  DCHECK(checkered_images->empty());
+
+  Tile* tile = prioritized_tile.tile();
+  prioritized_tile.raster_source()->GetDiscardableImagesInRect(
+      tile->enclosing_layer_rect(), tile->contents_scale(), images);
+  WhichTree tree = tile->tiling()->tree();
+
+  for (std::vector<DrawImage>::iterator it = images->begin();
+       it != images->end();) {
+    const sk_sp<const SkImage> image = it->image();
+    if (AnalyzeImageForCheckeringAndScheduleDecodeIfNecessary(image, tree)) {
+      checkered_images->insert(image->uniqueID());
+      it = images->erase(it);
+    } else {
+      ++it;
+    }
+  }
+}
+
+void CheckerImageTracker::AddInvalidationForCheckerImages(

[vmpstr, 2017/02/03 23:42:32]

I think this should be something like

Region TakeInvalidationForSyncTree() {
  images_invalidated_on_current_sync_tree_ = images_pending_invalidation_;
  images_pending_invalidation_ = Region();
  return images_invalidated_on_current_sync_tree_;
}

I don't think we should be exposing LayerTreeImpl here, instead the caller
should take the region and pass it to the sync tree.
 

[Khushal, 2017/02/07 00:25:32]

Great suggestion. Can't return the region since only the raster source on the
PictureLayerImpls knows the region for these images. But I returned the set that
the caller can pass to the tree.

+    LayerTreeImpl* sync_tree) {
+  DCHECK(images_invalidated_on_current_sync_tree_.empty())
+      << "Can not invalidate again on the sync tree";
+
+  sync_tree->InvalidateRegionForImages(images_pending_invalidation_);
+  images_invalidated_on_current_sync_tree_.swap(images_pending_invalidation_);

[vmpstr, 2017/02/03 23:42:32]

Should images_pending_invalidation_ be cleared here?

[Khushal, 2017/02/07 00:25:32]

Hmmm, there is a DCHECK above to assert that
|images_invalidated_on_current_sync_tree_| that we swapped it with is empty.
Still clear it?

+}
+
+void CheckerImageTracker::DidActivateSyncTree() {
+  for (auto image_id : images_invalidated_on_current_sync_tree_) {
+    auto it = image_id_to_decode_request_id_.find(image_id);
+    image_controller_->UnlockImageDecode(it->second);
+    image_id_to_decode_request_id_.erase(it);
+  }
+
+  images_invalidated_on_current_sync_tree_.clear();
+}
+
+void CheckerImageTracker::DidFinishImageDecode(
+    ImageId image_id,
+    ImageController::ImageDecodeRequestId request_id) {
+  TRACE_EVENT_ASYNC_END0("cc", "CheckerImageTracker::DeferImageDecode",
+                         image_id);
+
+  auto it = pending_image_decodes_.find(image_id);
+  DCHECK(it != pending_image_decodes_.end());
+  pending_image_decodes_.erase(it);

[vmpstr, 2017/02/03 23:42:32]

We typically write this type of pattern as:

DCHECK(pending_image_decodes_.find(image_id) != pending_image_decodes_.end());
pending_image_decodes_.erase(image_id);

(that way release build is a single erase call instead of find/erase)

[Khushal, 2017/02/07 00:25:32]

Done.

+
+  images_decoded_once_.insert(image_id);
+  images_pending_invalidation_.insert(image_id);
+  client_->NeedsInvalidationForCheckerImagedTiles();
+}
+
+bool CheckerImageTracker::AnalyzeImageForCheckeringAndScheduleDecodeIfNecessary(

[vmpstr, 2017/02/03 23:42:32]

I think this name makes it clear that this should really be two+ functions

if (ShouldCheckerImage(...)) {
  if (CheckerImageNeedsDecode(...))
    ScheduleDecode(...);
  ...
} else {
  ...
}

[Khushal, 2017/02/07 00:25:32]

Done.

+    const sk_sp<const SkImage> image,
+    WhichTree tree) {
+  TRACE_EVENT1("cc", "CheckerImageTracker::AnalyzeImageForCheckering",
+               "image_id", image->uniqueID());
+  if (!enable_checker_imaging_) {

[vmpstr, 2017/02/03 23:42:32]

nit (here and elsewhere), remove braces from single line condition, single line
statement ifs

[Khushal, 2017/02/07 00:25:32]

Done.

+    return false;
+  }
+
+  // If the image doesn't need to be decoded.
+  if (!image->isLazyGenerated()) {

[vmpstr, 2017/02/03 23:42:32]

If this is coming from discardable image map, then you can dcheck that it is
lazy generated. I believe the discardable image map filters images it gathers.

[Khushal, 2017/02/07 00:25:32]

Done.

+    return false;
+  }
+
+  // If a decode request is pending for this image, continue checkering it.
+  if (pending_image_decodes_.find(image->uniqueID()) !=
+      pending_image_decodes_.end()) {
+    return true;
+  }
+
+  // If the image is pending invalidation, continue checkering it. All tiles
+  // for these images will be invalidated on the next pending tree.
+  if (images_pending_invalidation_.find(image->uniqueID()) !=
+      images_pending_invalidation_.end()) {
+    return true;
+  }
+
+  // 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.
+  if (images_invalidated_on_current_sync_tree_.count(image->uniqueID()) != 0 &&
+      tree == WhichTree::ACTIVE_TREE) {

[vmpstr, 2017/02/03 23:42:32]

Check the tree first, since it's much quicker.

[Khushal, 2017/02/07 00:25:32]

Done.

+    return true;
+  }
+
+  // If the image has been decoded once before, don't checker it again.
+  if (images_decoded_once_.find(image->uniqueID()) !=
+      images_decoded_once_.end()) {
+    return false;
+  }
+
+  base::CheckedNumeric<size_t> checked_size = 4;
+  checked_size *= image->width();
+  checked_size *= image->height();
+  size_t image_size =
+      checked_size.ValueOrDefault(std::numeric_limits<size_t>::max());
+  bool defer_image_decode = image_size >= kMinImageSizeToCheckerBytes;
+
+  if (defer_image_decode) {

[vmpstr, 2017/02/03 23:42:32]

if (!defer_image_decode)
  return false;

...

[Khushal, 2017/02/07 00:25:32]

Simpler now?

+    ImageId image_id = image->uniqueID();
+    TRACE_EVENT_ASYNC_BEGIN0("cc", "CheckerImageTracker::DeferImageDecode",
+                             image_id);
+    DCHECK_EQ(image_id_to_decode_request_id_.count(image_id), 0U);
+
+    image_id_to_decode_request_id_[image_id] =
+        image_controller_->QueueImageDecode(
+            image, base::Bind(&CheckerImageTracker::DidFinishImageDecode,
+                              weak_factory_.GetWeakPtr(), image_id));
+    pending_image_decodes_.insert(image_id);
+  }
+
+  return defer_image_decode;
+}
+
+}  // namespace cc