Back PaintRecord with PaintOpBuffer instead of SkPicture

cc/paint/display_item_list.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/paint/display_item_list.h"
 
 #include <stddef.h>
 
 #include <string>
 
@@ skipping 82 matching lines @@
       break;
     }
     case DisplayItem::END_COMPOSITING:
       canvas->restore();
       break;
     case DisplayItem::DRAWING: {
       const auto& item = static_cast<const DrawingDisplayItem&>(base_item);
       if (canvas->quickReject(item.picture->cullRect()))
         break;
 
-      // SkPicture always does a wrapping save/restore on the canvas, so it is
-      // not necessary here.
+      // TODO(enne): Maybe the PaintRecord itself could know whether this
+      // was needed?  It's not clear whether these save/restore semantics
+      // that SkPicture handles during playback are things that should be
+      // kept around.
+      canvas->save();
       item.picture->playback(canvas, callback);
+      canvas->restore();
       break;
     }
     case DisplayItem::FLOAT_CLIP: {
       const auto& item = static_cast<const FloatClipDisplayItem&>(base_item);
       canvas->save();
       canvas->clipRect(gfx::RectFToSkRect(item.clip_rect));
       break;
     }
     case DisplayItem::END_FLOAT_CLIP:
       canvas->restore();
@@ skipping 53 matching lines @@
     // instead because it ignores canvas CTM.
     SkRegion device_clip;
     device_clip.setRect(gfx::RectToSkIRect(canvas_target_playback_rect));
     canvas->clipRegion(device_clip);
   }
   canvas->scale(contents_scale, contents_scale);
   Raster(canvas, callback);
   canvas->restore();
 }
 
+// Atttempts to merge a CompositingDisplayItem and DrawingDisplayItem
+// into a single "draw with alpha".  This function returns true if
+// it was successful.  If false, then the caller is responsible for
+// drawing these items.  This is a DisplayItemList version of the
+// SkRecord optimization SkRecordNoopSaveLayerDrawRestores.
+static bool MergeAndDrawIfPossible(const CompositingDisplayItem& save_item,
+                                   const DrawingDisplayItem& draw_item,
+                                   SkCanvas* canvas) {
+  if (save_item.color_filter)
+    return false;
+  if (save_item.xfermode != SkBlendMode::kSrcOver)
+    return false;
+  // TODO(enne): I believe that lcd_text_requires_opaque_layer is not

[vmpstr, 2017/04/06 19:16:12]

Is there a way to test this assumption via unittests/pixel tests? Or are we
punting?

[enne (OOO), 2017/04/06 23:17:05]

I am punting!

+  // relevant here and that lcd text is preserved post merge, but I haven't
+  // tested that.
+  const PaintRecord* record = draw_item.picture.get();
+  if (record->approximateOpCount() != 1)

[vmpstr, 2017/04/06 19:16:12]

Can you please leave a comment here that approximate here is good enough,
because it's not really approximate? Every time I look at this I'm thinking too
much about whether this can break...

[enne (OOO), 2017/04/06 23:17:05]

Sorry about this.  I was just trying to not change the interface of
SkPicture/PaintRecord here as I made this change.  I will follow up with this
right after.

+    return false;
+
+  const PaintOp* op = record->GetFirstOp();
+  if (!op->IsDrawOp())
+    return false;
+
+  op->RasterWithAlpha(canvas, save_item.alpha);
+  return true;
+}
+
 void DisplayItemList::Raster(SkCanvas* canvas,
                              SkPicture::AbortCallback* callback) const {
   gfx::Rect canvas_playback_rect;
   if (!GetCanvasClipBounds(canvas, &canvas_playback_rect))
     return;
 
   std::vector<size_t> indices;
   rtree_.Search(canvas_playback_rect, &indices);
-  for (size_t index : indices) {
-    RasterItem(items_[index], canvas, callback);
+  for (size_t i = 0; i < indices.size(); ++i) {
+    const DisplayItem& item = items_[indices[i]];
+    // Optimize empty begin/end compositing and merge begin/draw/end compositing
+    // where possible.
+    // TODO(enne): remove empty clips here too?
+    // TODO(enne): does this happen recursively? Or is this good enough?
+    if (i < indices.size() - 2 && item.type == DisplayItem::COMPOSITING) {
+      const DisplayItem& second = items_[indices[i + 1]];
+      if (second.type == DisplayItem::END_COMPOSITING) {
+        i++;
+        // TODO(enne): does this ever happen? Is this worth catching?
+        continue;
+      }
+      const DisplayItem& third = items_[indices[i + 2]];
+      if (second.type == DisplayItem::DRAWING &&
+          third.type == DisplayItem::END_COMPOSITING) {
+        if (MergeAndDrawIfPossible(
+                static_cast<const CompositingDisplayItem&>(item),
+                static_cast<const DrawingDisplayItem&>(second), canvas)) {
+          i += 2;
+          continue;
+        }
+      }
+    }
+
+    RasterItem(item, 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())

[vmpstr, 2017/04/06 19:16:12]

nit: can you move this check to the top of the loop? You could technically end
up with composite/draw/endcomposite several times in a row, so you wouldn't be
checking the abort

       break;
   }
 }
 
 void DisplayItemList::GrowCurrentBeginItemVisualRect(
     const gfx::Rect& visual_rect) {
   if (!begin_item_indices_.empty())
     visual_rects_[begin_item_indices_.back()].Union(visual_rect);
 }
 
@@ skipping 286 matching lines @@
     float contents_scale,
     std::vector<DrawImage>* images) {
   image_map_.GetDiscardableImagesInRect(rect, contents_scale, images);
 }
 
 gfx::Rect DisplayItemList::GetRectForImage(ImageId image_id) const {
   return image_map_.GetRectForImage(image_id);
 }
 
 }  // namespace cc