Skip to content

Commit

Permalink
Revert of Back PaintRecord with PaintOpBuffer instead of SkPicture (p…
Browse files Browse the repository at this point in the history 
…atchset #49 id:950001 of https://codereview.chromium.org/2768143002/ )

Reason for revert:
Breaks peach_pit-tot-chrome-pfq-informational Build #5967: https://uberchromegw.corp.google.com/i/chromeos.chrome/builders/peach_pit-tot-chrome-pfq-informational/builds/5967

Original issue's description:
> 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@{#467335}
> Committed: https://chromium.googlesource.com/chromium/src/+/f5a5ed867ba06ae56f8fc299a9eb194471277eec

TBR=chrishtr@chromium.org,danakj@chromium.org,mtklein@chromium.org,thakis@chromium.org,vitalybuka@chromium.org,vmpstr@chromium.org,enne@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Review-Url: https://codereview.chromium.org/2839343002
Cr-Commit-Position: refs/heads/master@{#467497}
  • Loading branch information
warx authored and Commit bot committed Apr 26, 2017
1 parent 145b24f commit afc38d5
Show file tree
Hide file tree
Showing 73 changed files with 343 additions and 2,639 deletions.
4 changes: 0 additions & 4 deletions cc/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -652,8 +652,6 @@ 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",
Expand Down Expand Up @@ -771,7 +769,6 @@ 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",
Expand Down Expand Up @@ -942,7 +939,6 @@ cc_test("cc_perftests") {
"//cc/ipc",
"//cc/ipc:interfaces",
"//cc/paint",
"//cc/paint",
"//cc/surfaces",
"//cc/surfaces:surface_id",
"//gpu",
Expand Down
6 changes: 0 additions & 6 deletions cc/paint/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,13 @@ cc_component("paint") {
"paint_canvas.cc",
"paint_canvas.h",
"paint_export.h",
"paint_flags.cc",
"paint_flags.h",
"paint_image.cc",
"paint_image.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",
Expand Down
60 changes: 5 additions & 55 deletions cc/paint/display_item_list.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,9 @@ NOINLINE DISABLE_CFI_PERF void RasterItem(const DisplayItem& base_item,
if (canvas->quickReject(item.picture->cullRect()))
break;

// 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();
// SkPicture always does a wrapping save/restore on the canvas, so it is
// not necessary here.
item.picture->playback(canvas, callback);
canvas->restore();
break;
}
case DisplayItem::FLOAT_CLIP: {
Expand Down Expand Up @@ -180,33 +176,6 @@ 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;
Expand All @@ -215,33 +184,14 @@ void DisplayItemList::Raster(SkCanvas* canvas,

std::vector<size_t> indices;
rtree_.Search(canvas_playback_rect, &indices);
for (size_t i = 0; i < indices.size(); ++i) {
for (size_t index : indices) {
RasterItem(items_[index], 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;

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);
}
}

Expand Down
122 changes: 1 addition & 121 deletions cc/paint/display_item_list_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,16 @@
#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"
Expand Down Expand Up @@ -81,19 +80,6 @@ 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);
Expand Down Expand Up @@ -718,110 +704,4 @@ 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
15 changes: 2 additions & 13 deletions cc/paint/paint_canvas.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,19 @@
#include "build/build_config.h"
#include "cc/paint/paint_export.h"
#include "cc/paint/paint_image.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:
PaintCanvas() {}
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
Expand All @@ -42,7 +36,7 @@ class CC_PAINT_EXPORT PaintCanvas {

virtual int save() = 0;
virtual int saveLayer(const SkRect* bounds, const PaintFlags* flags) = 0;
virtual int saveLayerAlpha(const SkRect* bounds, uint8_t alpha) = 0;
virtual int saveLayerAlpha(const SkRect* bounds, U8CPU alpha) = 0;

virtual void restore() = 0;
virtual int getSaveCount() const = 0;
Expand Down Expand Up @@ -93,8 +87,6 @@ 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,
Expand Down Expand Up @@ -188,9 +180,6 @@ class CC_PAINT_EXPORT PaintCanvas {
protected:
friend class PaintSurface;
friend class PaintRecorder;

private:
DISALLOW_COPY_AND_ASSIGN(PaintCanvas);
};

class CC_PAINT_EXPORT PaintCanvasAutoRestore {
Expand Down
42 changes: 0 additions & 42 deletions cc/paint/paint_flags.cc
View file

This file was deleted.

Loading

0 comments on commit afc38d5

Please sign in to comment.