Skip to content

Commit

Permalink
Remove cullRect() from PaintOpBuffer.
Browse files Browse the repository at this point in the history 
Pass it directly to RecordPaintCanvas and ToSkPicture, and other skia
methods which is where it is used.

This allows us to more easily get rid of cc::DisplayItem and its
subclasses, replacing them with a PaintOpBuffer in DisplayItemList
directly instead. The difficulty I faced with that was that if
DisplayItemList has a single PaintOpBuffer, then it has a single
cull rect. However when painting, each "batch" of PaintOps can
have a different cull rect (corresponding to the PaintOps that
would have been in a single DisplayItem before). So, instead the
cull rect should be a property of recording at the
RecordPaintCanvas level, which is a temporary object. As such,
creators of cc::RecordPaintCanvas (mostly thru cc::PaintRecorder)
need to manage the cull rect themselves to pass to things that
want to use it with the cc::PaintOpBuffer (aka cc::PaintRecord at
this time).

Original code review was done on gerrit:
https://chromium-review.googlesource.com/c/503472

R=chrishtr@chromium.org, enne@chromium.org, pdr@chromium.org
BUG=671433, 646010, 724367
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/2889653002 .
Cr-Original-Commit-Position: refs/heads/master@{#472917}
Committed: https://chromium.googlesource.com/chromium/src/+/c5f1b6126a7657234b9abc0c4359cbab45850b69
Review-Url: https://codereview.chromium.org/2889653002
Cr-Commit-Position: refs/heads/master@{#473975}
  • Loading branch information
@danakj
danakj authored and Commit bot committed May 23, 2017
1 parent c288f5b commit 29ef120
Show file tree
Hide file tree
Showing 68 changed files with 474 additions and 295 deletions.
6 changes: 4 additions & 2 deletions cc/blink/web_display_item_list_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include "third_party/skia/include/core/SkMatrix44.h"
#include "ui/gfx/geometry/rect_conversions.h"
#include "ui/gfx/geometry/safe_integer_conversions.h"
#include "ui/gfx/skia_util.h"
#include "ui/gfx/transform.h"

namespace cc_blink {
Expand All @@ -41,9 +42,10 @@ WebDisplayItemListImpl::WebDisplayItemListImpl(

void WebDisplayItemListImpl::AppendDrawingItem(
const blink::WebRect& visual_rect,
sk_sp<const cc::PaintRecord> record) {
sk_sp<const cc::PaintRecord> record,
const blink::WebRect& record_bounds) {
display_item_list_->CreateAndAppendDrawingItem<cc::DrawingDisplayItem>(
visual_rect, std::move(record));
visual_rect, std::move(record), gfx::RectToSkRect(record_bounds));
}

void WebDisplayItemListImpl::AppendClipItem(
Expand Down
3 changes: 2 additions & 1 deletion cc/blink/web_display_item_list_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ class WebDisplayItemListImpl : public blink::WebDisplayItemList {

// blink::WebDisplayItemList implementation.
void AppendDrawingItem(const blink::WebRect& visual_rect,
sk_sp<const cc::PaintRecord> record) override;
sk_sp<const cc::PaintRecord> record,
const blink::WebRect& record_bounds) override;
void AppendClipItem(
const blink::WebRect& clip_rect,
const blink::WebVector<SkRRect>& rounded_clip_rects) override;
Expand Down
3 changes: 2 additions & 1 deletion cc/layers/picture_image_layer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ scoped_refptr<DisplayItemList> PictureImageLayer::PaintContentsToDisplayList(
canvas->drawImage(image_, 0, 0);

display_list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
PaintableRegion(), recorder.finishRecordingAsPicture());
PaintableRegion(), recorder.finishRecordingAsPicture(),
gfx::RectToSkRect(PaintableRegion()));

display_list->Finalize();
return display_list;
Expand Down
4 changes: 2 additions & 2 deletions cc/paint/discardable_image_map_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ TEST_F(DiscardableImageMapTest, ClipsImageRects) {
display_list->CreateAndAppendPairedBeginItem<ClipDisplayItem>(
gfx::Rect(250, 250), std::vector<SkRRect>(), false);
display_list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
gfx::Rect(500, 500), record);
gfx::Rect(500, 500), record, SkRect::MakeWH(500, 500));
display_list->CreateAndAppendPairedEndItem<EndClipDisplayItem>();
display_list->Finalize();
display_list->GenerateDiscardableImagesMetadata();
Expand All @@ -688,7 +688,7 @@ TEST_F(DiscardableImageMapTest, GathersDiscardableImagesFromNestedOps) {
list_record->push<DrawImageOp>(discardable_image2, 100.f, 100.f, nullptr);
scoped_refptr<DisplayItemList> display_list = new DisplayItemList;
display_list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
gfx::Rect(100, 100, 100, 100), list_record);
gfx::Rect(100, 100, 100, 100), list_record, SkRect::MakeWH(100, 100));
display_list->Finalize();

PaintOpBuffer buffer;
Expand Down
14 changes: 7 additions & 7 deletions cc/paint/display_item_list.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ NOINLINE DISABLE_CFI_PERF void RasterItem(const DisplayItem& base_item,
break;
case DisplayItem::DRAWING: {
const auto& item = static_cast<const DrawingDisplayItem&>(base_item);
if (canvas->quickReject(item.picture->cullRect()))
if (canvas->quickReject(item.bounds))
break;

// TODO(enne): Maybe the PaintRecord itself could know whether this
Expand Down Expand Up @@ -427,15 +427,15 @@ DisplayItemList::CreateTracedValue(bool include_items) const {
state->EndArray();

state->BeginArray("cullRect");
state->AppendInteger(item.picture->cullRect().x());
state->AppendInteger(item.picture->cullRect().y());
state->AppendInteger(item.picture->cullRect().width());
state->AppendInteger(item.picture->cullRect().height());
state->AppendInteger(item.bounds.x());
state->AppendInteger(item.bounds.y());
state->AppendInteger(item.bounds.width());
state->AppendInteger(item.bounds.height());
state->EndArray();

std::string b64_picture;
PictureDebugUtil::SerializeAsBase64(ToSkPicture(item.picture).get(),
&b64_picture);
PictureDebugUtil::SerializeAsBase64(
ToSkPicture(item.picture, item.bounds).get(), &b64_picture);
state->SetString("skp64", b64_picture);
state->EndDictionary();
break;
Expand Down
89 changes: 57 additions & 32 deletions cc/paint/display_item_list_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,13 @@ void AppendFirstSerializationTestPicture(scoped_refptr<DisplayItemList> list,
PaintFlags red_paint;
red_paint.setColor(SK_ColorRED);

PaintCanvas* canvas = recorder.beginRecording(SkRect::MakeXYWH(
offset.x(), offset.y(), layer_size.width(), layer_size.height()));
SkRect bounds = SkRect::MakeXYWH(offset.x(), offset.y(), layer_size.width(),
layer_size.height());
PaintCanvas* canvas = recorder.beginRecording(bounds);
canvas->translate(offset.x(), offset.y());
canvas->drawRect(SkRect::MakeWH(4, 4), red_paint);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, recorder.finishRecordingAsPicture());
kVisualRect, recorder.finishRecordingAsPicture(), bounds);
}

} // namespace
Expand All @@ -130,7 +131,8 @@ TEST(DisplayItemListTest, SingleDrawingItem) {
canvas->drawRect(SkRect::MakeLTRB(0.f, 0.f, 60.f, 60.f), red_paint);
canvas->drawRect(SkRect::MakeLTRB(50.f, 50.f, 75.f, 75.f), blue_flags);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, recorder.finishRecordingAsPicture());
kVisualRect, recorder.finishRecordingAsPicture(),
gfx::RectFToSkRect(recording_rect));
list->Finalize();
DrawDisplayList(pixels, layer_rect, list);

Expand Down Expand Up @@ -170,7 +172,8 @@ TEST(DisplayItemListTest, ClipItem) {
canvas->translate(first_offset.x(), first_offset.y());
canvas->drawRect(SkRect::MakeWH(60, 60), red_paint);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, recorder.finishRecordingAsPicture());
kVisualRect, recorder.finishRecordingAsPicture(),
gfx::RectFToSkRect(first_recording_rect));

gfx::Rect clip_rect(60, 60, 10, 10);
list->CreateAndAppendPairedBeginItem<ClipDisplayItem>(
Expand All @@ -183,7 +186,8 @@ TEST(DisplayItemListTest, ClipItem) {
canvas->translate(second_offset.x(), second_offset.y());
canvas->drawRect(SkRect::MakeLTRB(50.f, 50.f, 75.f, 75.f), blue_flags);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, recorder.finishRecordingAsPicture());
kVisualRect, recorder.finishRecordingAsPicture(),
gfx::RectFToSkRect(second_recording_rect));

list->CreateAndAppendPairedEndItem<EndClipDisplayItem>();
list->Finalize();
Expand Down Expand Up @@ -227,7 +231,8 @@ TEST(DisplayItemListTest, TransformItem) {
canvas->translate(first_offset.x(), first_offset.y());
canvas->drawRect(SkRect::MakeWH(60, 60), red_paint);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, recorder.finishRecordingAsPicture());
kVisualRect, recorder.finishRecordingAsPicture(),
gfx::RectFToSkRect(first_recording_rect));

gfx::Transform transform;
transform.Rotate(45.0);
Expand All @@ -240,7 +245,8 @@ TEST(DisplayItemListTest, TransformItem) {
canvas->translate(second_offset.x(), second_offset.y());
canvas->drawRect(SkRect::MakeLTRB(50.f, 50.f, 75.f, 75.f), blue_flags);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, recorder.finishRecordingAsPicture());
kVisualRect, recorder.finishRecordingAsPicture(),
gfx::RectFToSkRect(second_recording_rect));

list->CreateAndAppendPairedEndItem<EndTransformDisplayItem>();
list->Finalize();
Expand Down Expand Up @@ -303,14 +309,16 @@ TEST(DisplayItemListTest, FilterItem) {
PaintFlags red_paint;
red_paint.setColor(SK_ColorRED);

PaintCanvas* canvas = recorder.beginRecording(
SkRect::MakeXYWH(0, 0, layer_rect.width(), layer_rect.height()));
SkRect bounds =
SkRect::MakeXYWH(0, 0, layer_rect.width(), layer_rect.height());
PaintCanvas* canvas = recorder.beginRecording(bounds);
canvas->drawRect(
SkRect::MakeLTRB(filter_bounds.x(), filter_bounds.y(),
filter_bounds.right(), filter_bounds.bottom()),
red_paint);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
ToNearestRect(filter_bounds), recorder.finishRecordingAsPicture());
ToNearestRect(filter_bounds), recorder.finishRecordingAsPicture(),
bounds);
}

list->CreateAndAppendPairedEndItem<EndFilterDisplayItem>();
Expand Down Expand Up @@ -348,7 +356,8 @@ TEST(DisplayItemListTest, ApproximateMemoryUsage) {
ASSERT_GE(record_size, kNumCommandsInTestSkPicture * sizeof(SkRect));

auto list = make_scoped_refptr(new DisplayItemList);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(kVisualRect, record);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, record, gfx::RectToSkRect(layer_rect));
list->Finalize();
memory_usage = list->ApproximateMemoryUsage();
EXPECT_GE(memory_usage, record_size);
Expand Down Expand Up @@ -408,7 +417,8 @@ TEST(DisplayItemListTest, SizeOne) {
auto list = make_scoped_refptr(new DisplayItemList);
gfx::Rect drawing_bounds(5, 6, 1, 1);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_bounds, CreateRectPicture(drawing_bounds));
drawing_bounds, CreateRectPicture(drawing_bounds),
gfx::RectToSkRect(drawing_bounds));
EXPECT_EQ(1u, list->size());
}

Expand All @@ -428,7 +438,8 @@ TEST(DisplayItemListTest, AppendVisualRectSimple) {

gfx::Rect drawing_bounds(5, 6, 7, 8);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_bounds, CreateRectPicture(drawing_bounds));
drawing_bounds, CreateRectPicture(drawing_bounds),
gfx::RectToSkRect(drawing_bounds));

EXPECT_EQ(1u, list->size());
EXPECT_RECT_EQ(drawing_bounds, list->VisualRectForTesting(0));
Expand Down Expand Up @@ -480,7 +491,8 @@ TEST(DisplayItemListTest, AppendVisualRectBlockContainingDrawing) {

gfx::Rect drawing_bounds(5, 6, 1, 1);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_bounds, CreateRectPicture(drawing_bounds));
drawing_bounds, CreateRectPicture(drawing_bounds),
gfx::RectToSkRect(drawing_bounds));

list->CreateAndAppendPairedEndItem<EndClipDisplayItem>();

Expand All @@ -501,7 +513,8 @@ TEST(DisplayItemListTest, AppendVisualRectBlockContainingEscapedDrawing) {

gfx::Rect drawing_bounds(1, 2, 3, 4);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_bounds, CreateRectPicture(drawing_bounds));
drawing_bounds, CreateRectPicture(drawing_bounds),
gfx::RectToSkRect(drawing_bounds));

list->CreateAndAppendPairedEndItem<EndClipDisplayItem>();

Expand All @@ -520,15 +533,17 @@ TEST(DisplayItemListTest,

gfx::Rect drawing_a_bounds(1, 2, 3, 4);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_a_bounds, CreateRectPicture(drawing_a_bounds));
drawing_a_bounds, CreateRectPicture(drawing_a_bounds),
gfx::RectToSkRect(drawing_a_bounds));

gfx::Rect clip_bounds(5, 6, 7, 8);
list->CreateAndAppendPairedBeginItem<ClipDisplayItem>(
clip_bounds, std::vector<SkRRect>(), true);

gfx::Rect drawing_b_bounds(13, 14, 1, 1);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_b_bounds, CreateRectPicture(drawing_b_bounds));
drawing_b_bounds, CreateRectPicture(drawing_b_bounds),
gfx::RectToSkRect(drawing_b_bounds));

list->CreateAndAppendPairedEndItem<EndClipDisplayItem>();

Expand All @@ -550,13 +565,15 @@ TEST(DisplayItemListTest, AppendVisualRectTwoBlocksTwoDrawings) {

gfx::Rect drawing_a_bounds(5, 6, 1, 1);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_a_bounds, CreateRectPicture(drawing_a_bounds));
drawing_a_bounds, CreateRectPicture(drawing_a_bounds),
gfx::RectToSkRect(drawing_a_bounds));

list->CreateAndAppendPairedBeginItem<TransformDisplayItem>(gfx::Transform());

gfx::Rect drawing_b_bounds(7, 8, 1, 1);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_b_bounds, CreateRectPicture(drawing_b_bounds));
drawing_b_bounds, CreateRectPicture(drawing_b_bounds),
gfx::RectToSkRect(drawing_b_bounds));

list->CreateAndAppendPairedEndItem<EndTransformDisplayItem>();
list->CreateAndAppendPairedEndItem<EndClipDisplayItem>();
Expand Down Expand Up @@ -585,13 +602,15 @@ TEST(DisplayItemListTest,

gfx::Rect drawing_a_bounds(5, 6, 1, 1);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_a_bounds, CreateRectPicture(drawing_a_bounds));
drawing_a_bounds, CreateRectPicture(drawing_a_bounds),
gfx::RectToSkRect(drawing_a_bounds));

list->CreateAndAppendPairedBeginItem<TransformDisplayItem>(gfx::Transform());

gfx::Rect drawing_b_bounds(1, 2, 3, 4);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_b_bounds, CreateRectPicture(drawing_b_bounds));
drawing_b_bounds, CreateRectPicture(drawing_b_bounds),
gfx::RectToSkRect(drawing_b_bounds));

list->CreateAndAppendPairedEndItem<EndTransformDisplayItem>();
list->CreateAndAppendPairedEndItem<EndClipDisplayItem>();
Expand Down Expand Up @@ -620,13 +639,15 @@ TEST(DisplayItemListTest,

gfx::Rect drawing_a_bounds(1, 2, 3, 4);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_a_bounds, CreateRectPicture(drawing_a_bounds));
drawing_a_bounds, CreateRectPicture(drawing_a_bounds),
gfx::RectToSkRect(drawing_a_bounds));

list->CreateAndAppendPairedBeginItem<TransformDisplayItem>(gfx::Transform());

gfx::Rect drawing_b_bounds(7, 8, 1, 1);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_b_bounds, CreateRectPicture(drawing_b_bounds));
drawing_b_bounds, CreateRectPicture(drawing_b_bounds),
gfx::RectToSkRect(drawing_b_bounds));

list->CreateAndAppendPairedEndItem<EndTransformDisplayItem>();
list->CreateAndAppendPairedEndItem<EndClipDisplayItem>();
Expand Down Expand Up @@ -655,13 +676,15 @@ TEST(DisplayItemListTest,

gfx::Rect drawing_a_bounds(13, 14, 1, 1);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_a_bounds, CreateRectPicture(drawing_a_bounds));
drawing_a_bounds, CreateRectPicture(drawing_a_bounds),
gfx::RectToSkRect(drawing_a_bounds));

list->CreateAndAppendPairedBeginItem<TransformDisplayItem>(gfx::Transform());

gfx::Rect drawing_b_bounds(1, 2, 3, 4);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
drawing_b_bounds, CreateRectPicture(drawing_b_bounds));
drawing_b_bounds, CreateRectPicture(drawing_b_bounds),
gfx::RectToSkRect(drawing_b_bounds));

list->CreateAndAppendPairedEndItem<EndTransformDisplayItem>();
list->CreateAndAppendPairedEndItem<EndClipDisplayItem>();
Expand Down Expand Up @@ -727,7 +750,8 @@ TEST(DisplayItemListTest, SaveDrawRestore) {
list->CreateAndAppendPairedBeginItem<CompositingDisplayItem>(
80, SkBlendMode::kSrcOver, nullptr, nullptr, false);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, CreateRectPictureWithAlpha(kVisualRect, 40));
kVisualRect, CreateRectPictureWithAlpha(kVisualRect, 40),
gfx::RectToSkRect(kVisualRect));
list->CreateAndAppendPairedEndItem<EndCompositingDisplayItem>();
list->Finalize();

Expand Down Expand Up @@ -777,7 +801,8 @@ TEST(DisplayItemListTest, SaveDrawRestoreFail_BadSaveFlags) {
list->CreateAndAppendPairedBeginItem<CompositingDisplayItem>(
80, SkBlendMode::kSrc, nullptr, nullptr, false);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, CreateRectPictureWithAlpha(kVisualRect, 40));
kVisualRect, CreateRectPictureWithAlpha(kVisualRect, 40),
gfx::RectToSkRect(kVisualRect));
list->CreateAndAppendPairedEndItem<EndCompositingDisplayItem>();
list->Finalize();

Expand All @@ -793,10 +818,10 @@ TEST(DisplayItemListTest, SaveDrawRestoreFail_BadSaveFlags) {
// The same as SaveDrawRestore, but with too many ops in the PaintRecord.
TEST(DisplayItemListTest, SaveDrawRestoreFail_TooManyOps) {
sk_sp<const PaintRecord> record;
SkRect bounds = SkRect::MakeWH(kVisualRect.width(), kVisualRect.height());
{
PaintRecorder recorder;
PaintCanvas* canvas =
recorder.beginRecording(kVisualRect.width(), kVisualRect.height());
PaintCanvas* canvas = recorder.beginRecording(bounds);
PaintFlags flags;
flags.setAlpha(40);
canvas->drawRect(gfx::RectToSkRect(kVisualRect), flags);
Expand All @@ -810,8 +835,8 @@ TEST(DisplayItemListTest, SaveDrawRestoreFail_TooManyOps) {

list->CreateAndAppendPairedBeginItem<CompositingDisplayItem>(
80, SkBlendMode::kSrcOver, nullptr, nullptr, false);
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(kVisualRect,
std::move(record));
list->CreateAndAppendDrawingItem<DrawingDisplayItem>(
kVisualRect, std::move(record), bounds);
list->CreateAndAppendPairedEndItem<EndCompositingDisplayItem>();
list->Finalize();

Expand Down
10 changes: 6 additions & 4 deletions cc/paint/drawing_display_item.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@

namespace cc {

DrawingDisplayItem::DrawingDisplayItem() : DisplayItem(DRAWING) {}
DrawingDisplayItem::DrawingDisplayItem()
: DisplayItem(DRAWING), bounds(SkRect::MakeEmpty()) {}

DrawingDisplayItem::DrawingDisplayItem(sk_sp<const PaintRecord> record)
: DisplayItem(DRAWING), picture(std::move(record)) {}
DrawingDisplayItem::DrawingDisplayItem(sk_sp<const PaintRecord> record,
const SkRect& bounds)
: DisplayItem(DRAWING), picture(std::move(record)), bounds(bounds) {}

DrawingDisplayItem::DrawingDisplayItem(const DrawingDisplayItem& item)
: DisplayItem(DRAWING), picture(item.picture) {}
: DisplayItem(DRAWING), picture(item.picture), bounds(item.bounds) {}

DrawingDisplayItem::~DrawingDisplayItem() = default;

Expand Down
Loading

0 comments on commit 29ef120

Please sign in to comment.