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

[vmpstr, 2017/03/28 18:27:14]

I think it makes sense for the paint record to figure this out; it might be as
simple as a bool that says "we need a wrapping save/restore on playback" and set
it to true on any transform/clip type of things.

The code as-is is good as well, but we might be doing extra save/restores when
we don't need to (not sure how expensive that is, but work is work)

[enne (OOO), 2017/03/28 19:28:05]

I think this just needs some investigation.  I think the ideal situation would
be that PaintRecord doesn't do this at all and callers are responsible in all
cases for handling it.  I think we could track if we needed a save or restore
separately too (and then remove when all callers are doing it).

Other than just wanting to land this, the other reason I'm hesitant here is that
it seems really unfortunate to have to do add these in in a world where
cc::DisplayItemList collapses into PaintRecord.  Like, every single draw op
needs one of these around it?  I think it just kind of no longer makes sense
unless the caller handles it.

+      // 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();
 }
 
+static bool MergeAndDrawIfPossible(const CompositingDisplayItem& save_item,

[vmpstr, 2017/03/28 18:27:14]

nit: Can you add a comment mentioning what this does (and what the return value
means)?

[enne (OOO), 2017/03/28 19:28:05]

Done.

+                                   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
+  // 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/03/28 18:27:14]

I'm a bit <_< at "approximate" here... Is this guaranteed to not underestimate?

[enne (OOO), 2017/03/28 19:28:05]

This is the SkPicture API.  This patch changes PaintRecord to not be an
SkPicture, but still has to implement playback, cullRect, approximateOpCount,
and approximateBytesUsed.

I'll leave a TODO.

+    return false;
+
+  const PaintOp* op = record->GetFirstOp();
+  if (!op->IsDrawOp())
+    return false;
+
+  op->RasterWithAlpha(canvas, save_item.alpha);

[vmpstr, 2017/03/28 18:27:14]

Hmm is there a way to guard/dcheck against future changes? Like if we have some
other thing that isn't just alpha/color filter that pops up, is that going to
break things?

[enne (OOO), 2017/03/28 19:28:05]

I think there's really no way to guard.  There's no way to guard right now for
the SkRecordOpts, and I think anything that changes SkPaint just needs to be
aware of those optimizations.

+  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 += 2;

[vmpstr, 2017/03/28 18:27:14]

I think this has to be ++i because of the ++i in the for loop? Or is my math bad

[enne (OOO), 2017/03/28 19:28:05]

OOPS

+        // TODO(enne): does this ever happen? Is this worth catching?

[vmpstr, 2017/03/28 18:27:14]

I think this is possible in this situation:

- Compositing item
- Rect draw at 0x0 100,100
- Rect draw at 100,100 100x100
- End compositing item

Then the compositing item's visual rect would have to be 0x0 200x200 and if the
tile we're drawing is at 0,101 50x50 (which doesn't intersect any of the rects),
we'd still get the compositing/end compositing indices.

FWIW, the case we're dropping here is when i == indices.size() - 2; and we just
have the compositing/end compositing. I think that's easy enough to fix tho,
with like

if (type == COMPOSITING) {
 if (i < indices.size() - 1 && type[i+1] == END_COMPOSITING) {
   i++;
   continue;
 }
 ...
}
...

or... something <_<

[enne (OOO), 2017/03/28 19:28:05]

Yeah, I'm not saying it's not impossible.  It just certainly didn't come up in a
bunch of the layout tests that I ran, while the other case definitely did. 
Mostly just asking if this is worth bothering to check for or if it's so
uncommon that we should just let it happen.

+        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 += 3;

[vmpstr, 2017/03/28 18:27:14]

+= 2?

[enne (OOO), 2017/03/28 19:28:05]

***<_<***

+          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())
       break;
   }
 }
 
 void DisplayItemList::GrowCurrentBeginItemVisualRect(
@@ skipping 291 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