Back PaintRecord with PaintOpBuffer instead of SkPicture

cc/paint/display_item_list_unittest.cc

Index: cc/paint/display_item_list_unittest.cc
diff --git a/cc/paint/display_item_list_unittest.cc b/cc/paint/display_item_list_unittest.cc
index f1b9e759f9b1c3aa65992a7ad22017d25176df28..46899bcdcd94aae4c814d995f055fb54849aed12 100644
--- a/cc/paint/display_item_list_unittest.cc
+++ b/cc/paint/display_item_list_unittest.cc
@@ -17,12 +17,13 @@
 #include "cc/paint/compositing_display_item.h"
 #include "cc/paint/drawing_display_item.h"
 #include "cc/paint/filter_display_item.h"
-
 #include "cc/paint/float_clip_display_item.h"
 #include "cc/paint/paint_canvas.h"
 #include "cc/paint/paint_flags.h"
 #include "cc/paint/paint_record.h"
 #include "cc/paint/paint_recorder.h"
+#include "cc/paint/skia_paint_canvas.h"
+#include "cc/paint/test_skcanvas.h"
 #include "cc/paint/transform_display_item.h"
 #include "cc/test/geometry_test_utils.h"
 #include "cc/test/pixel_test_utils.h"
@@ -80,6 +81,19 @@ sk_sp<const PaintRecord> CreateRectPicture(const gfx::Rect& bounds) {
   return recorder.finishRecordingAsPicture();
 }
 
+sk_sp<const PaintRecord> CreateRectPictureWithAlpha(const gfx::Rect& bounds,
+                                                    uint8_t alpha) {
+  PaintRecorder recorder;
+  PaintCanvas* canvas =
+      recorder.beginRecording(bounds.width(), bounds.height());
+  PaintFlags flags;
+  flags.setAlpha(alpha);
+  canvas->drawRect(
+      SkRect::MakeXYWH(bounds.x(), bounds.y(), bounds.width(), bounds.height()),
+      flags);
+  return recorder.finishRecordingAsPicture();
+}
+
 void AppendFirstSerializationTestPicture(scoped_refptr<DisplayItemList> list,
                                          const gfx::Size& layer_size) {
   gfx::PointF offset(2.f, 3.f);
@@ -704,4 +718,103 @@ TEST(DisplayItemListTest, AppendVisualRectBlockContainingFilterNoDrawings) {
   EXPECT_RECT_EQ(filter_bounds, list->VisualRectForTesting(3));
 }
 
+// Verify that raster time optimizations for compositing item / draw single op /
+// end compositing item can be collapsed together into a single draw op
+// with the opacity from the compositing item folded in.
+TEST(DisplayItemListTest, SaveDrawRestore) {
+  auto list = make_scoped_refptr(new DisplayItemList);
+
+  list->CreateAndAppendPairedBeginItem<CompositingDisplayItem>(
+      80, SkBlendMode::kSrcOver, nullptr, nullptr, false);
+  list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
+      kVisualRect, CreateRectPictureWithAlpha(kVisualRect, 40));
+  list->CreateAndAppendPairedEndItem<EndCompositingDisplayItem>();
+  list->Finalize();
+
+  SaveCountingCanvas canvas;
+  list->Raster(&canvas, nullptr);
+
+  EXPECT_EQ(0, canvas.restore_count_);
+  EXPECT_EQ(gfx::RectToSkRect(kVisualRect), canvas.draw_rect_);
+
+  float expected_alpha = 80 * 40 / 255.f;
+  EXPECT_LE(std::abs(expected_alpha - canvas.paint_.getAlpha()), 1.f);
+}
+
+// Verify that compositing item / end compositing item is a noop.
+TEST(DisplayItemListTest, SaveRestoreNoops) {
+  auto list = make_scoped_refptr(new DisplayItemList);
+
+  list->CreateAndAppendPairedBeginItem<CompositingDisplayItem>(
+      80, SkBlendMode::kSrcOver, nullptr, nullptr, false);
+  list->CreateAndAppendPairedEndItem<EndCompositingDisplayItem>();
+  list->CreateAndAppendPairedBeginItem<CompositingDisplayItem>(
+      255, SkBlendMode::kSrcOver, nullptr, nullptr, false);
+  list->CreateAndAppendPairedEndItem<EndCompositingDisplayItem>();
+  list->CreateAndAppendPairedBeginItem<CompositingDisplayItem>(
+      255, SkBlendMode::kSrc, nullptr, nullptr, false);
+  list->CreateAndAppendPairedEndItem<EndCompositingDisplayItem>();
+  list->Finalize();
+
+  SaveCountingCanvas canvas;
+  list->Raster(&canvas, nullptr);
+
+  EXPECT_EQ(0, canvas.restore_count_);

[vmpstr, 2017/04/12 21:47:12]

This is a bit interesting for me. Specifically, for the case of exactly two
items (Compositing/EndCompositing), the if on line 230 in
https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
fails, because i == 0 which is not < 2 - 2. So, we should be sending a save
layer and a restore?

Am I missing something or is skia optimizing this situation under the covers?

I see the optimization in a single op buffer and that would handle this no
problem, but afaict this test is supposed to test the DisplayItemList::Raster
code path.

[mtklein_C, 2017/04/12 22:32:00]

For what it's worth, SkCanvas does internally delay save()s until they're
needed.  E.g.

 save()
   drawRect()
   translate()
   drawRect()
 restore()

does become

 drawRect()
 save()
   translate()
   drawRect()
 restore()

and of course save()-restore() annihilates without doing anything.

saveLayer() is a different story, and I don't think there's any sort of
trickiness going on in there.

[enne (OOO), 2017/04/12 22:41:30]

OOPS.  The unit test appears to pass without this, so am removing this, and
leaving a comment on the unit test.

[enne (OOO), 2017/04/12 22:41:30]

Skia does appear to optimize out noop saveLayer restores, so far as I can tell.

+}
+
+// The same as SaveDrawRestore, but with save flags that prevent the
+// optimization.
+TEST(DisplayItemListTest, SaveDrawRestoreFail_BadSaveFlags) {
+  auto list = make_scoped_refptr(new DisplayItemList);
+
+  // Use a blend mode that's not compatible with the SaveDrawRestore
+  // optimization.
+  list->CreateAndAppendPairedBeginItem<CompositingDisplayItem>(
+      80, SkBlendMode::kSrc, nullptr, nullptr, false);
+  list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
+      kVisualRect, CreateRectPictureWithAlpha(kVisualRect, 40));
+  list->CreateAndAppendPairedEndItem<EndCompositingDisplayItem>();
+  list->Finalize();
+
+  SaveCountingCanvas canvas;
+  list->Raster(&canvas, nullptr);
+
+  EXPECT_EQ(1, canvas.restore_count_);
+  EXPECT_EQ(gfx::RectToSkRect(kVisualRect), canvas.draw_rect_);
+  EXPECT_LE(40, canvas.paint_.getAlpha());
+}
+
+// The same as SaveDrawRestore, but with too many ops in the PaintRecord.
+TEST(DisplayItemListTest, SaveDrawRestoreFail_TooManyOps) {
+  sk_sp<const PaintRecord> record;
+  {
+    PaintRecorder recorder;
+    PaintCanvas* canvas =
+        recorder.beginRecording(kVisualRect.width(), kVisualRect.height());
+    PaintFlags flags;
+    flags.setAlpha(40);
+    canvas->drawRect(gfx::RectToSkRect(kVisualRect), flags);
+    // Add an extra op here.
+    canvas->drawRect(gfx::RectToSkRect(kVisualRect), flags);
+    record = recorder.finishRecordingAsPicture();
+  }
+  EXPECT_GT(record->approximateOpCount(), 1);
+
+  auto list = make_scoped_refptr(new DisplayItemList);
+
+  list->CreateAndAppendPairedBeginItem<CompositingDisplayItem>(
+      80, SkBlendMode::kSrcOver, nullptr, nullptr, false);
+  list->CreateAndAppendDrawingItem<DrawingDisplayItem>(kVisualRect,
+                                                       std::move(record));
+  list->CreateAndAppendPairedEndItem<EndCompositingDisplayItem>();
+  list->Finalize();
+
+  SaveCountingCanvas canvas;
+  list->Raster(&canvas, nullptr);
+
+  EXPECT_EQ(1, canvas.restore_count_);
+  EXPECT_EQ(gfx::RectToSkRect(kVisualRect), canvas.draw_rect_);
+  EXPECT_LE(40, canvas.paint_.getAlpha());
+}
+
 }  // namespace cc