Raster display item lists via a visual rect RTree.

cc/playback/display_item_list.cc

Index: cc/playback/display_item_list.cc
diff --git a/cc/playback/display_item_list.cc b/cc/playback/display_item_list.cc
index 3ad738ab7dbd0b554e659ac0ff21f2ed9583040a..f183355f23b0b96af39ca81e12d3ddbe1a094268 100644
--- a/cc/playback/display_item_list.cc
+++ b/cc/playback/display_item_list.cc
@@ -9,6 +9,7 @@
 #include <string>
 
 #include "base/numerics/safe_conversions.h"
+#include "base/process/process.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/trace_event_argument.h"
@@ -26,6 +27,7 @@
 #include "third_party/skia/include/core/SkPictureRecorder.h"
 #include "third_party/skia/include/utils/SkPictureUtils.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/skia_util.h"
 
 namespace cc {
@@ -49,25 +51,21 @@ const int kDefaultNumDisplayItemsToReserve = 100;
 }  // namespace
 
 scoped_refptr<DisplayItemList> DisplayItemList::Create(

[vmpstr, 2016/06/22 20:14:07]

Just as a very optional nit, I've been slowly removing ::Create functions that
just forward things to the ctor, so your call but it may be something you might
as well throw in here. 

[wkorman, 2016/06/24 19:57:14]

It looks like we'd need to replace a bunch of callsites with
make_scoped_refptr(new DisplayItemList()), it's vaguely nice to have this method
instead as shorthand and for future flexibility?

[vmpstr, 2016/06/24 20:15:33]

Makes sense, and yeah in this case we also have CreateFromProto, so Create in
general makes sense.

-    const gfx::Rect& layer_rect,
     const DisplayItemListSettings& settings) {
-  return make_scoped_refptr(new DisplayItemList(
-      layer_rect, settings,
-      !settings.use_cached_picture || DisplayItemsTracingEnabled()));
+  return make_scoped_refptr(new DisplayItemList(settings));
 }
 
 scoped_refptr<DisplayItemList> DisplayItemList::CreateFromProto(
     const proto::DisplayItemList& proto,
     ImageSerializationProcessor* image_serialization_processor) {
-  gfx::Rect layer_rect = ProtoToRect(proto.layer_rect());
   scoped_refptr<DisplayItemList> list =
-      DisplayItemList::Create(ProtoToRect(proto.layer_rect()),
-                              DisplayItemListSettings(proto.settings()));
+      DisplayItemList::Create(DisplayItemListSettings(proto.settings()));
 
   for (int i = 0; i < proto.items_size(); i++) {
     const proto::DisplayItem& item_proto = proto.items(i);
+    const gfx::Rect visual_rect = ProtoToRect(proto.visual_rects(i));
     DisplayItemProtoFactory::AllocateAndConstruct(
-        layer_rect, list.get(), item_proto, image_serialization_processor);
+        visual_rect, list.get(), item_proto, image_serialization_processor);
   }
 
   list->Finalize();
@@ -75,27 +73,13 @@ scoped_refptr<DisplayItemList> DisplayItemList::CreateFromProto(
   return list;
 }
 
-DisplayItemList::DisplayItemList(gfx::Rect layer_rect,
-                                 const DisplayItemListSettings& settings,
-                                 bool retain_individual_display_items)
+DisplayItemList::DisplayItemList(const DisplayItemListSettings& settings)
     : items_(LargestDisplayItemSize(),
              LargestDisplayItemSize() * kDefaultNumDisplayItemsToReserve),
+      retain_visual_rects_(false),

[vmpstr, 2016/06/22 20:14:07]

nit: you can = false in the header (same for the rest of PODs)

[wkorman, 2016/06/24 19:57:14]

Done.

       settings_(settings),
-      retain_individual_display_items_(retain_individual_display_items),
-      layer_rect_(layer_rect),
-      is_suitable_for_gpu_rasterization_(true),
-      approximate_op_count_(0),
-      picture_memory_usage_(0) {
-  if (settings_.use_cached_picture) {
-    SkRTreeFactory factory;
-    recorder_.reset(new SkPictureRecorder());
-
-    SkCanvas* canvas = recorder_->beginRecording(
-        layer_rect_.width(), layer_rect_.height(), &factory);
-    canvas->translate(-layer_rect_.x(), -layer_rect_.y());
-    canvas->clipRect(gfx::RectToSkRect(layer_rect_));
-  }
-}
+      all_items_are_suitable_for_gpu_rasterization_(true),
+      approximate_op_count_(0) {}
 
 DisplayItemList::~DisplayItemList() {
 }
@@ -105,131 +89,92 @@ void DisplayItemList::ToProtobuf(
     ImageSerializationProcessor* image_serialization_processor) {
   // The flattened SkPicture approach is going away, and the proto
   // doesn't currently support serializing that flattened picture.
-  DCHECK(retain_individual_display_items_);
-
-  RectToProto(layer_rect_, proto->mutable_layer_rect());
   settings_.ToProtobuf(proto->mutable_settings());
 
   DCHECK_EQ(0, proto->items_size());
-  for (const auto& item : items_)
+  DCHECK_EQ(0, proto->visual_rects_size());
+  DCHECK(items_.size() == visual_rects_.size())
+      << "items.size() " << items_.size() << " visual_rects.size() "
+      << visual_rects_.size();
+  int i = 0;
+  for (const auto& item : items_) {

[vmpstr, 2016/06/22 20:14:07]

I wonder if we should add operator[] to ContiguousContainer and do a regular i =
0; i < items_.size() loop

[wkorman, 2016/06/24 19:57:14]

Added TODO.

     item.ToProtobuf(proto->add_items(), image_serialization_processor);
+    RectToProto(visual_rects_[i++], proto->add_visual_rects());
+  }
 }
 
 void DisplayItemList::Raster(SkCanvas* canvas,
                              SkPicture::AbortCallback* callback,
-                             const gfx::Rect& canvas_target_playback_rect,
+                             const gfx::Rect& canvas_playback_rect,
                              float contents_scale) const {
-  if (!settings_.use_cached_picture) {
-    canvas->save();
-    canvas->scale(contents_scale, contents_scale);
-    for (const auto& item : items_)
-      item.Raster(canvas, canvas_target_playback_rect, callback);
-    canvas->restore();
-  } else {
-    DCHECK(picture_);
-
-    canvas->save();
-    canvas->scale(contents_scale, contents_scale);
-    canvas->translate(layer_rect_.x(), layer_rect_.y());
-    if (callback) {
-      // If we have a callback, we need to call |draw()|, |drawPicture()|
-      // doesn't take a callback.  This is used by |AnalysisCanvas| to early
-      // out.
-      picture_->playback(canvas, callback);
-    } else {
-      // Prefer to call |drawPicture()| on the canvas since it could place the
-      // entire picture on the canvas instead of parsing the skia operations.
-      canvas->drawPicture(picture_.get());
-    }
-    canvas->restore();
+  canvas->save();
+  canvas->scale(contents_scale, contents_scale);
+
+  gfx::Rect query_rect = GetQueryRect(canvas_playback_rect, contents_scale);
+  std::vector<size_t> indices;
+  rtree_.Search(query_rect, &indices);
+  for (size_t index : indices) {
+    items_[index].Raster(canvas, callback);
+    // We use a callback during solid color analysis on the compositor thread to
+    // break out early. Since we're handling a sequence of pictures via rtree
+    // query results ourselves, we have to respect the callback and early out.
+    if (callback && callback->abort())
+      break;
   }
+  canvas->restore();
 }
 
-void DisplayItemList::ProcessAppendedItem(const DisplayItem* item) {
-  if (settings_.use_cached_picture) {
-    DCHECK(recorder_);
-    item->Raster(recorder_->getRecordingCanvas(), gfx::Rect(), nullptr);
-  }
-  if (!retain_individual_display_items_) {
-    items_.Clear();
-  }
-}
-
-void DisplayItemList::RasterIntoCanvas(const DisplayItem& item) {
-  DCHECK(recorder_);
-  DCHECK(!retain_individual_display_items_);
-
-  item.Raster(recorder_->getRecordingCanvas(), gfx::Rect(), nullptr);
-}
-
-bool DisplayItemList::RetainsIndividualDisplayItems() const {
-  return retain_individual_display_items_;
+gfx::Rect DisplayItemList::GetQueryRect(const gfx::Rect& rect,
+                                        float scale) const {
+  // If the playback rect is empty, assume that we need to play back everything.
+  return rect.IsEmpty() ? rtree_.GetBounds()
+                        : ScaleToEnclosingRect(rect, 1.f / scale);
 }
 
 void DisplayItemList::Finalize() {
   TRACE_EVENT0("cc", "DisplayItemList::Finalize");
   // TODO(dtrainor): Need to deal with serializing visual_rects_.
   // http://crbug.com/568757.
-  DCHECK(!retain_individual_display_items_ ||
-         items_.size() == visual_rects_.size())
+  DCHECK(items_.size() == visual_rects_.size())
       << "items.size() " << items_.size() << " visual_rects.size() "
       << visual_rects_.size();
+  rtree_.Build(visual_rects_);
 
-  // TODO(vmpstr): Build and make use of an RTree from the visual
-  // rects. For now we just clear them out since we won't ever need
-  // them to stick around post-Finalize. http://crbug.com/527245
-  // This clears both the vector and the vector's capacity, since visual_rects_
-  // won't be used anymore.
-  std::vector<gfx::Rect>().swap(visual_rects_);
-
-  if (settings_.use_cached_picture) {
-    // Convert to an SkPicture for faster rasterization.
-    DCHECK(settings_.use_cached_picture);
-    DCHECK(!picture_);
-    picture_ = recorder_->finishRecordingAsPicture();
-    DCHECK(picture_);
-    picture_memory_usage_ =
-        SkPictureUtils::ApproximateBytesUsed(picture_.get());
-    recorder_.reset();
-  }
+  if (!retain_visual_rects_)
+    // This clears both the vector and the vector's capacity, since
+    // visual_rects_
+    // won't be used anymore.
+    std::vector<gfx::Rect>().swap(visual_rects_);
 }
 
 bool DisplayItemList::IsSuitableForGpuRasterization() const {
-  return is_suitable_for_gpu_rasterization_;
+  // This is more permissive than Picture's implementation, since none of the

[vmpstr, 2016/06/22 20:14:07]

nit: Add a TODO(someone) to make it clear that this needs to be fixed

[wkorman, 2016/06/24 19:57:14]

Done.

+  // items might individually trigger a veto even though they collectively have
+  // enough "bad" operations that a corresponding Picture would get vetoed. See
+  // crbug.com/513016.
+  return all_items_are_suitable_for_gpu_rasterization_;
 }
 
 int DisplayItemList::ApproximateOpCount() const {
-  if (retain_individual_display_items_)
-    return approximate_op_count_;
-  return picture_ ? picture_->approximateOpCount() : 0;
+  return approximate_op_count_;
 }
 
 size_t DisplayItemList::ApproximateMemoryUsage() const {
-  // We double-count in this case. Produce zero to avoid being misleading.
-  if (settings_.use_cached_picture && retain_individual_display_items_)
-    return 0;
-
-  DCHECK(!settings_.use_cached_picture || picture_);
-
   size_t memory_usage = sizeof(*this);
 
   size_t external_memory_usage = 0;
-  if (retain_individual_display_items_) {
-    // Warning: this double-counts SkPicture data if use_cached_picture is
-    // also true.
-    for (const auto& item : items_) {
-      external_memory_usage += item.ExternalMemoryUsage();
-    }
+  // Warning: this double-counts SkPicture data if use_cached_picture is
+  // also true.
+  for (const auto& item : items_) {
+    external_memory_usage += item.ExternalMemoryUsage();
   }
 
   // Memory outside this class due to |items_|.
   memory_usage += items_.GetCapacityInBytes() + external_memory_usage;
 
-  // Memory outside this class due to |picture|.
-  memory_usage += picture_memory_usage_;
-
   // TODO(jbroman): Does anything else owned by this class substantially
   // contribute to memory usage?
+  // TODO(vmpstr): Probably DiscardableImageMap is worth counting here.
 
   return memory_usage;
 }
@@ -256,22 +201,19 @@ DisplayItemList::AsValue(bool include_items) const {
     }
     state->EndArray();  // "items".
   }
-  state->SetValue("layer_rect", MathUtil::AsValue(layer_rect_));
   state->EndDictionary();  // "params".
 
-  if (!layer_rect_.IsEmpty()) {
-    SkPictureRecorder recorder;
-    SkCanvas* canvas =
-        recorder.beginRecording(layer_rect_.width(), layer_rect_.height());
-    canvas->translate(-layer_rect_.x(), -layer_rect_.y());
-    canvas->clipRect(gfx::RectToSkRect(layer_rect_));
-    Raster(canvas, NULL, gfx::Rect(), 1.f);
-    sk_sp<SkPicture> picture = recorder.finishRecordingAsPicture();
-
-    std::string b64_picture;
-    PictureDebugUtil::SerializeAsBase64(picture.get(), &b64_picture);
-    state->SetString("skp64", b64_picture);
-  }
+  SkPictureRecorder recorder;
+  gfx::Rect bounds = rtree_.GetBounds();
+  SkCanvas* canvas = recorder.beginRecording(bounds.width(), bounds.height());
+  canvas->translate(-bounds.x(), -bounds.y());
+  canvas->clipRect(gfx::RectToSkRect(bounds));
+  Raster(canvas, nullptr, gfx::Rect(), 1.f);
+  sk_sp<SkPicture> picture = recorder.finishRecordingAsPicture();
+
+  std::string b64_picture;
+  PictureDebugUtil::SerializeAsBase64(picture.get(), &b64_picture);
+  state->SetString("skp64", b64_picture);
 
   return std::move(state);
 }
@@ -289,17 +231,11 @@ void DisplayItemList::EmitTraceSnapshot() const {
 void DisplayItemList::GenerateDiscardableImagesMetadata() {
   // This should be only called once, and only after CreateAndCacheSkPicture.
   DCHECK(image_map_.empty());
-  DCHECK(!settings_.use_cached_picture || picture_);
-  if (settings_.use_cached_picture && !picture_->willPlayBackBitmaps())
-    return;
 
-  // The cached picture is translated by -layer_rect_.origin during record,
-  // so we need to offset that back in order to get right positioning for
-  // images.
+  gfx::Rect bounds = rtree_.GetBounds();
   DiscardableImageMap::ScopedMetadataGenerator generator(
-      &image_map_, gfx::Size(layer_rect_.right(), layer_rect_.bottom()));
-  Raster(generator.canvas(), nullptr,
-         gfx::Rect(layer_rect_.right(), layer_rect_.bottom()), 1.f);
+      &image_map_, gfx::Size(bounds.right(), bounds.bottom()));
+  Raster(generator.canvas(), nullptr, gfx::Rect(), 1.f);
 }
 
 void DisplayItemList::GetDiscardableImagesInRect(