Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#464555}

cc/BUILD.gn

@@ -647,6 +647,8 @@ cc_static_library("test_support") {
     "test/test_occlusion_tracker.h",
     "test/test_shared_bitmap_manager.cc",
     "test/test_shared_bitmap_manager.h",
+    "test/test_skcanvas.cc",
+    "test/test_skcanvas.h",
     "test/test_task_graph_runner.cc",
     "test/test_task_graph_runner.h",
     "test/test_texture.cc",
@@ -764,6 +766,7 @@ cc_test("cc_unittests") {
     "output/texture_mailbox_deleter_unittest.cc",
     "paint/discardable_image_map_unittest.cc",
     "paint/display_item_list_unittest.cc",
+    "paint/paint_op_buffer_unittest.cc",
     "quads/draw_polygon_unittest.cc",
     "quads/draw_quad_unittest.cc",
     "quads/nine_patch_generator_unittest.cc",
@@ -934,6 +937,7 @@ cc_test("cc_perftests") {
     "//cc/ipc",
     "//cc/ipc:interfaces",
     "//cc/paint",
+    "//cc/paint",
     "//cc/surfaces",
     "//cc/surfaces:surface_id",
     "//gpu",

cc/paint/BUILD.gn

@@ -32,11 +32,17 @@ cc_component("paint") {
     "paint_canvas.cc",
     "paint_canvas.h",
     "paint_export.h",
+    "paint_flags.cc",
     "paint_flags.h",
+    "paint_op_buffer.cc",
+    "paint_op_buffer.h",
+    "paint_record.cc",
     "paint_record.h",
     "paint_recorder.cc",
     "paint_recorder.h",
     "paint_shader.h",
+    "record_paint_canvas.cc",
+    "record_paint_canvas.h",
     "skia_paint_canvas.cc",
     "skia_paint_canvas.h",
     "transform_display_item.cc",

cc/paint/display_item_list.cc

@@ -100,9 +100,13 @@ NOINLINE DISABLE_CFI_PERF void RasterItem(const DisplayItem& 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: {
@@ -176,6 +180,33 @@ void DisplayItemList::Raster(SkCanvas* canvas,
   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
+  // 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)
+    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;
@@ -184,14 +215,33 @@ void DisplayItemList::Raster(SkCanvas* canvas,
 
   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) {
     // 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;
+
+    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]];
+      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);
   }
 }
 

cc/paint/display_item_list_unittest.cc

@@ -17,16 +17,17 @@
 #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/transform_display_item.h"
 #include "cc/test/geometry_test_utils.h"
 #include "cc/test/pixel_test_utils.h"
 #include "cc/test/skia_common.h"
+#include "cc/test/test_skcanvas.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.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,110 @@ 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.save_count_);
+  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.
+// Here we're testing that Skia does an optimization that skips
+// save/restore with nothing in between.  If skia stops doing this
+// then we should reimplement this optimization in display list raster.
+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.save_count_);
+  EXPECT_EQ(0, canvas.restore_count_);
+}
+
+// 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.save_count_);
+  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.save_count_);
+  EXPECT_EQ(1, canvas.restore_count_);
+  EXPECT_EQ(gfx::RectToSkRect(kVisualRect), canvas.draw_rect_);
+  EXPECT_LE(40, canvas.paint_.getAlpha());
+}
+
 }  // namespace cc

cc/paint/paint_canvas.h

@@ -10,19 +10,24 @@
 #include "base/memory/ref_counted.h"
 #include "build/build_config.h"
 #include "cc/paint/paint_export.h"
-#include "cc/paint/paint_record.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 
 namespace cc {
 
 class DisplayItemList;
 class PaintFlags;
+class PaintOpBuffer;
+
+using PaintRecord = PaintOpBuffer;
 
 class CC_PAINT_EXPORT PaintCanvas {
  public:
   virtual ~PaintCanvas() {}
 
   virtual SkMetaData& getMetaData() = 0;
+
+  // TODO(enne): this only appears to mostly be used to determine if this is
+  // recording or not, so could be simplified or removed.
   virtual SkImageInfo imageInfo() const = 0;
 
   // TODO(enne): It would be nice to get rid of flush() entirely, as it
@@ -41,7 +46,7 @@ class CC_PAINT_EXPORT PaintCanvas {
                            int y) = 0;
   virtual int save() = 0;
   virtual int saveLayer(const SkRect* bounds, const PaintFlags* flags) = 0;
-  virtual int saveLayerAlpha(const SkRect* bounds, U8CPU alpha) = 0;
+  virtual int saveLayerAlpha(const SkRect* bounds, uint8_t alpha) = 0;
 
   virtual void restore() = 0;
   virtual int getSaveCount() const = 0;
@@ -92,6 +97,8 @@ class CC_PAINT_EXPORT PaintCanvas {
   virtual bool getDeviceClipBounds(SkIRect* bounds) const = 0;
   virtual void drawColor(SkColor color, SkBlendMode mode) = 0;
   void drawColor(SkColor color) { drawColor(color, SkBlendMode::kSrcOver); }
+
+  // TODO(enne): This is a synonym for drawColor with kSrc.  Remove it.
   virtual void clear(SkColor color) = 0;
 
   virtual void drawLine(SkScalar x0,

cc/paint/paint_flags.cc

@@ -0,0 +1,42 @@
+// Copyright 2017 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/paint_flags.h"
+
+namespace cc {
+
+bool PaintFlags::IsSimpleOpacity() const {
+  uint32_t color = getColor();
+  if (SK_ColorTRANSPARENT != SkColorSetA(color, SK_AlphaTRANSPARENT))
+    return false;
+  if (!isSrcOver())
+    return false;
+  if (getLooper())
+    return false;
+  if (getPathEffect())
+    return false;
+  if (getShader())
+    return false;
+  if (getMaskFilter())
+    return false;
+  if (getColorFilter())
+    return false;
+  if (getImageFilter())
+    return false;
+  return true;
+}
+
+bool PaintFlags::SupportsFoldingAlpha() const {
+  if (!isSrcOver())
+    return false;
+  if (getColorFilter())
+    return false;
+  if (getImageFilter())
+    return false;
+  if (getLooper())
+    return false;
+  return true;
+}
+
+}  // namespace cc

cc/paint/paint_flags.h

@@ -7,7 +7,6 @@
 
 #include "base/compiler_specific.h"
 #include "cc/paint/paint_export.h"
-#include "cc/paint/paint_shader.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkColorFilter.h"
 #include "third_party/skia/include/core/SkDrawLooper.h"
@@ -19,6 +18,8 @@
 
 namespace cc {
 
+using PaintShader = SkShader;
+
 class CC_PAINT_EXPORT PaintFlags {
  public:
   enum Style {
@@ -198,6 +199,14 @@ class CC_PAINT_EXPORT PaintFlags {
     return paint_.computeFastBounds(orig, storage);
   }
 
+  bool operator==(const PaintFlags& flags) { return flags.paint_ == paint_; }
+  bool operator!=(const PaintFlags& flags) { return flags.paint_ != paint_; }
+
+  // Returns true if this just represents an opacity blend when
+  // used as saveLayer flags.
+  bool IsSimpleOpacity() const;
+  bool SupportsFoldingAlpha() const;
+
  private:
   friend const SkPaint& ToSkPaint(const PaintFlags& flags);
   friend const SkPaint* ToSkPaint(const PaintFlags* flags);