SkRecord refactor: fill bounds array instead of BBH directly
This should be a strict refactor, just pulling out the bounds array.
(It's the rescued nice parts of a dead-end CL targeting skia:4492.)
BUG=skia:
Review URL: https://codereview.chromium.org/1424553002
diff --git a/src/core/SkPictureRecorder.cpp b/src/core/SkPictureRecorder.cpp
index 6397ca0..62fa0e9 100644
--- a/src/core/SkPictureRecorder.cpp
+++ b/src/core/SkPictureRecorder.cpp
@@ -72,11 +72,16 @@
drawableList ? drawableList->newDrawableSnapshot() : nullptr;
if (fBBH.get()) {
+ SkAutoTMalloc<SkRect> bounds(fRecord->count());
if (saveLayerData) {
- SkRecordComputeLayers(fCullRect, *fRecord, pictList, fBBH.get(), saveLayerData);
+ SkRecordComputeLayers(fCullRect, *fRecord, bounds, pictList, saveLayerData);
} else {
- SkRecordFillBounds(fCullRect, *fRecord, fBBH.get());
+ SkRecordFillBounds(fCullRect, *fRecord, bounds);
}
+ fBBH->insert(bounds, fRecord->count());
+
+ // Now that we've calculated content bounds, we can update fCullRect, often trimming it.
+ // TODO: get updated fCullRect from bounds instead of forcing the BBH to return it?
SkRect bbhBound = fBBH->getRootBound();
SkASSERT((bbhBound.isEmpty() || fCullRect.contains(bbhBound))
|| (bbhBound.isEmpty() && fCullRect.isEmpty()));
@@ -153,14 +158,12 @@
}
SkAutoTUnref<SkLayerInfo> saveLayerData;
-
if (fBBH && fDoSaveLayerInfo) {
+ // TODO: can we avoid work by not allocating / filling these bounds?
+ SkAutoTMalloc<SkRect> scratchBounds(fRecord->count());
saveLayerData.reset(new SkLayerInfo);
- SkBBoxHierarchy* bbh = nullptr; // we've already computed fBBH (received in constructor)
- // TODO: update saveLayer info computation to reuse the already computed
- // bounds in 'fBBH'
- SkRecordComputeLayers(fBounds, *fRecord, pictList, bbh, saveLayerData);
+ SkRecordComputeLayers(fBounds, *fRecord, scratchBounds, pictList, saveLayerData);
}
size_t subPictureBytes = 0;
@@ -183,7 +186,9 @@
SkRecordOptimize(fRecord);
if (fBBH.get()) {
- SkRecordFillBounds(fCullRect, *fRecord, fBBH.get());
+ SkAutoTMalloc<SkRect> bounds(fRecord->count());
+ SkRecordFillBounds(fCullRect, *fRecord, bounds);
+ fBBH->insert(bounds, fRecord->count());
}
SkDrawable* drawable =
diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp
index 12920da..ab0cb71 100644
--- a/src/core/SkRecordDraw.cpp
+++ b/src/core/SkRecordDraw.cpp
@@ -150,20 +150,15 @@
// in for all the control ops we stashed away.
class FillBounds : SkNoncopyable {
public:
- FillBounds(const SkRect& cullRect, const SkRecord& record)
+ FillBounds(const SkRect& cullRect, const SkRecord& record, SkRect bounds[])
: fNumRecords(record.count())
, fCullRect(cullRect)
- , fBounds(record.count())
- {
- // Calculate bounds for all ops. This won't go quite in order, so we'll need
- // to store the bounds separately then feed them in to the BBH later in order.
+ , fBounds(bounds) {
fCTM = &SkMatrix::I();
fCurrentClipBounds = fCullRect;
}
- void setCurrentOp(int currentOp) { fCurrentOp = currentOp; }
-
- void cleanUp(SkBBoxHierarchy* bbh) {
+ void cleanUp() {
// If we have any lingering unpaired Saves, simulate restores to make
// sure all ops in those Save blocks have their bounds calculated.
while (!fSaveStack.isEmpty()) {
@@ -174,13 +169,11 @@
while (!fControlIndices.isEmpty()) {
this->popControl(fCullRect);
}
-
- // Finally feed all stored bounds into the BBH. They'll be returned in this order.
- if (bbh) {
- bbh->insert(fBounds.get(), fNumRecords);
- }
}
+ void setCurrentOp(int currentOp) { fCurrentOp = currentOp; }
+
+
template <typename T> void operator()(const T& op) {
this->updateCTM(op);
this->updateClipBounds(op);
@@ -580,7 +573,7 @@
const SkRect fCullRect;
// Conservative identity-space bounds for each op in the SkRecord.
- SkAutoTMalloc<Bounds> fBounds;
+ Bounds* fBounds;
// We walk fCurrentOp through the SkRecord, as we go using updateCTM()
// and updateClipBounds() to maintain the exact CTM (fCTM) and conservative
@@ -597,27 +590,27 @@
// SkRecord visitor to gather saveLayer/restore information.
class CollectLayers : SkNoncopyable {
public:
- CollectLayers(const SkRect& cullRect, const SkRecord& record,
+ CollectLayers(const SkRect& cullRect, const SkRecord& record, SkRect bounds[],
const SkBigPicture::SnapshotArray* pictList, SkLayerInfo* accelData)
: fSaveLayersInStack(0)
, fAccelData(accelData)
, fPictList(pictList)
- , fFillBounds(cullRect, record)
+ , fFillBounds(cullRect, record, bounds)
{}
- void setCurrentOp(int currentOp) { fFillBounds.setCurrentOp(currentOp); }
-
- void cleanUp(SkBBoxHierarchy* bbh) {
+ void cleanUp() {
// fFillBounds must perform its cleanUp first so that all the bounding
// boxes associated with unbalanced restores are updated (prior to
// fetching their bound in popSaveLayerInfo).
- fFillBounds.cleanUp(bbh);
-
+ fFillBounds.cleanUp();
while (!fSaveLayerStack.isEmpty()) {
this->popSaveLayerInfo();
}
}
+ void setCurrentOp(int currentOp) { fFillBounds.setCurrentOp(currentOp); }
+
+
template <typename T> void operator()(const T& op) {
fFillBounds(op);
this->trackSaveLayers(op);
@@ -792,27 +785,22 @@
} // namespace SkRecords
-void SkRecordFillBounds(const SkRect& cullRect, const SkRecord& record, SkBBoxHierarchy* bbh) {
- SkRecords::FillBounds visitor(cullRect, record);
-
+void SkRecordFillBounds(const SkRect& cullRect, const SkRecord& record, SkRect bounds[]) {
+ SkRecords::FillBounds visitor(cullRect, record, bounds);
for (int curOp = 0; curOp < record.count(); curOp++) {
visitor.setCurrentOp(curOp);
record.visit<void>(curOp, visitor);
}
-
- visitor.cleanUp(bbh);
+ visitor.cleanUp();
}
-void SkRecordComputeLayers(const SkRect& cullRect, const SkRecord& record,
- const SkBigPicture::SnapshotArray* pictList, SkBBoxHierarchy* bbh,
- SkLayerInfo* data) {
- SkRecords::CollectLayers visitor(cullRect, record, pictList, data);
-
+void SkRecordComputeLayers(const SkRect& cullRect, const SkRecord& record, SkRect bounds[],
+ const SkBigPicture::SnapshotArray* pictList, SkLayerInfo* data) {
+ SkRecords::CollectLayers visitor(cullRect, record, bounds, pictList, data);
for (int curOp = 0; curOp < record.count(); curOp++) {
visitor.setCurrentOp(curOp);
record.visit<void>(curOp, visitor);
}
-
- visitor.cleanUp(bbh);
+ visitor.cleanUp();
}
diff --git a/src/core/SkRecordDraw.h b/src/core/SkRecordDraw.h
index c2db37a..fdf9882 100644
--- a/src/core/SkRecordDraw.h
+++ b/src/core/SkRecordDraw.h
@@ -17,12 +17,14 @@
class SkDrawable;
class SkLayerInfo;
-// Fill a BBH to be used by SkRecordDraw to accelerate playback.
-void SkRecordFillBounds(const SkRect& cullRect, const SkRecord&, SkBBoxHierarchy*);
+// Calculate conservative identity space bounds for each op in the record.
+void SkRecordFillBounds(const SkRect& cullRect, const SkRecord&, SkRect bounds[]);
-void SkRecordComputeLayers(const SkRect& cullRect, const SkRecord& record,
- const SkBigPicture::SnapshotArray*,
- SkBBoxHierarchy* bbh, SkLayerInfo* data);
+// SkRecordFillBounds(), and gathers information about saveLayers and stores it for later
+// use (e.g., layer hoisting). The gathered information is sufficient to determine
+// where each saveLayer will land and which ops in the picture it represents.
+void SkRecordComputeLayers(const SkRect& cullRect, const SkRecord&, SkRect bounds[],
+ const SkBigPicture::SnapshotArray*, SkLayerInfo* data);
// Draw an SkRecord into an SkCanvas. A convenience wrapper around SkRecords::Draw.
void SkRecordDraw(const SkRecord&, SkCanvas*, SkPicture const* const drawablePicts[],
diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp
index d8ca48d..13be115 100644
--- a/tests/RecordDrawTest.cpp
+++ b/tests/RecordDrawTest.cpp
@@ -122,26 +122,6 @@
REPORTER_ASSERT(r, setMatrix->matrix == expected);
}
-struct TestBBH : public SkBBoxHierarchy {
- void insert(const SkRect boundsArray[], int N) override {
- fEntries.setCount(N);
- for (int i = 0; i < N; i++) {
- Entry e = { i, boundsArray[i] };
- fEntries[i] = e;
- }
- }
-
- void search(const SkRect& query, SkTDArray<int>* results) const override {}
- size_t bytesUsed() const override { return 0; }
- SkRect getRootBound() const override { return SkRect::MakeEmpty(); }
-
- struct Entry {
- int opIndex;
- SkRect bounds;
- };
- SkTDArray<Entry> fEntries;
-};
-
// Like a==b, with a little slop recognizing that float equality can be weird.
static bool sloppy_rect_eq(SkRect a, SkRect b) {
SkRect inset(a), outset(a);
@@ -150,9 +130,7 @@
return outset.contains(b) && !inset.contains(b);
}
-// This test is not meant to make total sense yet. It's testing the status quo
-// of SkRecordFillBounds(), which itself doesn't make total sense yet.
-DEF_TEST(RecordDraw_BBH, r) {
+DEF_TEST(RecordDraw_BasicBounds, r) {
SkRecord record;
SkRecorder recorder(&record, W, H);
recorder.save();
@@ -161,14 +139,11 @@
recorder.drawRect(SkRect::MakeWH(320, 240), SkPaint());
recorder.restore();
- TestBBH bbh;
- SkRecordFillBounds(SkRect::MakeWH(SkIntToScalar(W), SkIntToScalar(H)), record, &bbh);
+ SkAutoTMalloc<SkRect> bounds(record.count());
+ SkRecordFillBounds(SkRect::MakeWH(SkIntToScalar(W), SkIntToScalar(H)), record, bounds);
- REPORTER_ASSERT(r, bbh.fEntries.count() == 5);
- for (int i = 0; i < bbh.fEntries.count(); i++) {
- REPORTER_ASSERT(r, bbh.fEntries[i].opIndex == i);
-
- REPORTER_ASSERT(r, sloppy_rect_eq(SkRect::MakeWH(400, 480), bbh.fEntries[i].bounds));
+ for (int i = 0; i < record.count(); i++) {
+ REPORTER_ASSERT(r, sloppy_rect_eq(SkRect::MakeWH(400, 480), bounds[i]));
}
}
@@ -187,15 +162,14 @@
const SkPoint pos[] = { {40, 50}, {60, 70} };
recorder.drawPosText(text, bytes, pos, SkPaint());
- TestBBH bbh;
- SkRecordFillBounds(SkRect::MakeWH(SkIntToScalar(W), SkIntToScalar(H)), record, &bbh);
- REPORTER_ASSERT(r, bbh.fEntries.count() == 2);
+ SkAutoTMalloc<SkRect> bounds(record.count());
+ SkRecordFillBounds(SkRect::MakeWH(SkIntToScalar(W), SkIntToScalar(H)), record, bounds);
// We can make these next assertions confidently because SkRecordFillBounds
// builds its bounds by overestimating font metrics in a platform-independent way.
// If that changes, these tests will need to be more flexible.
- REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[0].bounds, SkRect::MakeLTRB(0, 0, 140, 60)));
- REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(0, 20, 180, 100)));
+ REPORTER_ASSERT(r, sloppy_rect_eq(bounds[0], SkRect::MakeLTRB(0, 0, 140, 60)));
+ REPORTER_ASSERT(r, sloppy_rect_eq(bounds[1], SkRect::MakeLTRB(0, 20, 180, 100)));
}
// Base test to ensure start/stop range is respected
@@ -248,13 +222,12 @@
//
// The second bug showed up as adjusting the picture bounds (0,0,50,50) by the drop shadow too.
// The saveLayer, clipRect, and restore bounds were incorrectly (0,0,70,50).
- TestBBH bbh;
- SkRecordFillBounds(SkRect::MakeWH(50, 50), record, &bbh);
- REPORTER_ASSERT(r, bbh.fEntries.count() == 4);
- REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[0].bounds, SkRect::MakeLTRB(0, 0, 50, 50)));
- REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(0, 0, 50, 50)));
- REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[2].bounds, SkRect::MakeLTRB(0, 0, 40, 40)));
- REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[3].bounds, SkRect::MakeLTRB(0, 0, 50, 50)));
+ SkAutoTMalloc<SkRect> bounds(record.count());
+ SkRecordFillBounds(SkRect::MakeWH(50, 50), record, bounds);
+ REPORTER_ASSERT(r, sloppy_rect_eq(bounds[0], SkRect::MakeLTRB(0, 0, 50, 50)));
+ REPORTER_ASSERT(r, sloppy_rect_eq(bounds[1], SkRect::MakeLTRB(0, 0, 50, 50)));
+ REPORTER_ASSERT(r, sloppy_rect_eq(bounds[2], SkRect::MakeLTRB(0, 0, 40, 40)));
+ REPORTER_ASSERT(r, sloppy_rect_eq(bounds[3], SkRect::MakeLTRB(0, 0, 50, 50)));
}
// When a saveLayer provides an explicit bound and has a complex paint (e.g., one that
@@ -267,18 +240,17 @@
SkPaint p;
p.setXfermodeMode(SkXfermode::kSrc_Mode);
- SkRect bounds = SkRect::MakeLTRB(10, 10, 40, 40);
- recorder.saveLayer(&bounds, &p);
+ SkRect layerBounds = SkRect::MakeLTRB(10, 10, 40, 40);
+ recorder.saveLayer(&layerBounds, &p);
recorder.drawRect(SkRect::MakeLTRB(20, 20, 30, 30), SkPaint());
recorder.restore();
- TestBBH bbh;
- SkRecordFillBounds(SkRect::MakeWH(50, 50), record, &bbh);
- REPORTER_ASSERT(r, bbh.fEntries.count() == 3);
+ SkAutoTMalloc<SkRect> bounds(record.count());
+ SkRecordFillBounds(SkRect::MakeWH(50, 50), record, bounds);
if (!SkCanvas::Internal_Private_GetIgnoreSaveLayerBounds()) {
- REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[0].bounds, SkRect::MakeLTRB(10, 10, 40, 40)));
- REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[1].bounds, SkRect::MakeLTRB(20, 20, 30, 30)));
- REPORTER_ASSERT(r, sloppy_rect_eq(bbh.fEntries[2].bounds, SkRect::MakeLTRB(10, 10, 40, 40)));
+ REPORTER_ASSERT(r, sloppy_rect_eq(bounds[0], SkRect::MakeLTRB(10, 10, 40, 40)));
+ REPORTER_ASSERT(r, sloppy_rect_eq(bounds[1], SkRect::MakeLTRB(20, 20, 30, 30)));
+ REPORTER_ASSERT(r, sloppy_rect_eq(bounds[2], SkRect::MakeLTRB(10, 10, 40, 40)));
}
}