From a723b576aed31a6eb2bdda6388e6bd779d04c6b0 Mon Sep 17 00:00:00 2001 From: mtklein Date: Fri, 15 Aug 2014 11:49:49 -0700 Subject: [PATCH] SkRecordDraw: incorporate clip into BBH NOTREECHECKS=true BUG=skia: R=robertphillips@google.com, senorblanco@chromium.org, mtklein@google.com Author: mtklein@chromium.org Review URL: https://codereview.chromium.org/474983002 --- src/core/SkRecordDraw.cpp | 110 ++++++++++++++++++++++++++++++-------- src/core/SkRecorder.cpp | 10 ++-- src/core/SkRecorder.h | 6 +++ src/core/SkRecords.h | 10 ++-- tests/RecordDrawTest.cpp | 45 ++++++++++++++++ 5 files changed, 148 insertions(+), 33 deletions(-) diff --git a/src/core/SkRecordDraw.cpp b/src/core/SkRecordDraw.cpp index c9e029b8db..46241d72ac 100644 --- a/src/core/SkRecordDraw.cpp +++ b/src/core/SkRecordDraw.cpp @@ -16,10 +16,16 @@ void SkRecordDraw(const SkRecord& record, if (NULL != bbh) { // Draw only ops that affect pixels in the canvas's current clip. - SkIRect devBounds; - canvas->getClipDeviceBounds(&devBounds); + SkIRect query; +#if 1 // TODO: Why is this the right way to make the query? I'd think it'd be the else branch. + SkRect clipBounds; + canvas->getClipBounds(&clipBounds); + clipBounds.roundOut(&query); +#else + canvas->getClipDeviceBounds(&query); +#endif SkTDArray ops; - bbh->search(devBounds, &ops); + bbh->search(query, &ops); // FIXME: QuadTree doesn't send these back in the order we inserted them. :( // Also remove the sort in SkPictureData::getActiveOps()? @@ -117,7 +123,9 @@ public: FillBounds(const SkRecord& record, SkBBoxHierarchy* bbh) : 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. + const SkIRect largest = SkIRect::MakeLargest(); fCTM.setIdentity(); + fCurrentClipBounds = largest; for (fCurrentOp = 0; fCurrentOp < record.count(); fCurrentOp++) { record.visit(fCurrentOp, *this); } @@ -130,7 +138,7 @@ public: // Any control ops not part of any Save/Restore block draw everywhere. while (!fControlIndices.isEmpty()) { - this->popControl(SkIRect::MakeLargest()); + this->popControl(largest); } // Finally feed all stored bounds into the BBH. They'll be returned in this order. @@ -143,28 +151,44 @@ public: bbh->flushDeferredInserts(); } - template void operator()(const T& r) { - this->updateCTM(r); - this->trackBounds(r); + template void operator()(const T& op) { + this->updateCTM(op); + this->updateClipBounds(op); + this->trackBounds(op); } private: struct SaveBounds { - int controlOps; // Number of control ops in this Save block, including the Save. - SkIRect bounds; // Bounds of everything in the block. + int controlOps; // Number of control ops in this Save block, including the Save. + SkIRect bounds; // Bounds of everything in the block. + const SkPaint* paint; // Unowned. If set, adjusts the bounds of all ops in this block. }; template void updateCTM(const T&) { /* most ops don't change the CTM */ } - void updateCTM(const Restore& r) { fCTM = r.matrix; } - void updateCTM(const SetMatrix& r) { fCTM = r.matrix; } - void updateCTM(const Concat& r) { fCTM.preConcat(r.matrix); } + void updateCTM(const Restore& op) { fCTM = op.matrix; } + void updateCTM(const SetMatrix& op) { fCTM = op.matrix; } + void updateCTM(const Concat& op) { fCTM.preConcat(op.matrix); } + + template void updateClipBounds(const T&) { /* most ops don't change the clip */ } + // Each of these devBounds fields is the state of the device bounds after the op. + // So Restore's devBounds are those bounds saved by its paired Save or SaveLayer. + void updateClipBounds(const Restore& op) { fCurrentClipBounds = op.devBounds; } + void updateClipBounds(const ClipPath& op) { fCurrentClipBounds = op.devBounds; } + void updateClipBounds(const ClipRRect& op) { fCurrentClipBounds = op.devBounds; } + void updateClipBounds(const ClipRect& op) { fCurrentClipBounds = op.devBounds; } + void updateClipBounds(const ClipRegion& op) { fCurrentClipBounds = op.devBounds; } + void updateClipBounds(const SaveLayer& op) { + if (op.bounds) { + fCurrentClipBounds.intersect(this->adjustAndMap(*op.bounds, op.paint)); + } + } // The bounds of these ops must be calculated when we hit the Restore // from the bounds of the ops in the same Save block. - void trackBounds(const Save&) { this->pushSaveBlock(); } + void trackBounds(const Save&) { this->pushSaveBlock(NULL); } // TODO: bounds of SaveLayer may be more complicated? - void trackBounds(const SaveLayer&) { this->pushSaveBlock(); } - void trackBounds(const Restore&) { fBounds[fCurrentOp] = this->popSaveBlock(); } + void trackBounds(const SaveLayer& op) { this->pushSaveBlock(op.paint); } + void trackBounds(const Restore&) { fBounds[fCurrentOp] = this->popSaveBlock(); } void trackBounds(const Concat&) { this->pushControl(); } void trackBounds(const SetMatrix&) { this->pushControl(); } @@ -179,12 +203,9 @@ private: this->updateSaveBounds(fBounds[fCurrentOp]); } - // TODO: remove this trivially-safe default when done bounding all ops - template SkIRect bounds(const T&) { return SkIRect::MakeLargest(); } - - void pushSaveBlock() { + void pushSaveBlock(const SkPaint* paint) { // Starting a new Save block. Push a new entry to represent that. - SaveBounds sb = { 0, SkIRect::MakeEmpty() }; + SaveBounds sb = { 0, SkIRect::MakeEmpty(), paint }; fSaveStack.push(sb); this->pushControl(); } @@ -223,11 +244,54 @@ private: } } - SkIRect bounds(const NoOp&) { return SkIRect::MakeEmpty(); } // NoOps don't draw anywhere. + // TODO: Remove this default when done bounding all ops. + template SkIRect bounds(const T&) { return fCurrentClipBounds; } + SkIRect bounds(const Clear&) { return SkIRect::MakeLargest(); } // Ignores the clip + SkIRect bounds(const NoOp&) { return SkIRect::MakeEmpty(); } // NoOps don't draw anywhere. - SkAutoTMalloc fBounds; // One for each op in the record. - SkMatrix fCTM; + // Adjust rect for all paints that may affect its geometry, then map it to device space. + SkIRect adjustAndMap(SkRect rect, const SkPaint* paint) { + // Adjust rect for its own paint. + if (paint) { + if (paint->canComputeFastBounds()) { + rect = paint->computeFastBounds(rect, &rect); + } else { + // The paint could do anything. The only safe answer is the current clip. + return fCurrentClipBounds; + } + } + + // Adjust rect for all the paints from the SaveLayers we're inside. + // For SaveLayers, only image filters will affect the bounds. + for (int i = fSaveStack.count() - 1; i >= 0; i--) { + if (fSaveStack[i].paint && fSaveStack[i].paint->getImageFilter()) { + if (paint->canComputeFastBounds()) { + rect = fSaveStack[i].paint->computeFastBounds(rect, &rect); + } else { + // Same deal as above. + return fCurrentClipBounds; + } + } + } + + // Map the rect back to device space. + fCTM.mapRect(&rect); + SkIRect devRect; + rect.roundOut(&devRect); + return devRect; + } + + // Conservative device bounds for each op in the SkRecord. + SkAutoTMalloc fBounds; + + // We walk fCurrentOp through the SkRecord, as we go using updateCTM() + // and updateClipBounds() to maintain the exact CTM (fCTM) and conservative + // device bounds of the current clip (fCurrentClipBounds). unsigned fCurrentOp; + SkMatrix fCTM; + SkIRect fCurrentClipBounds; + + // Used to track the bounds of Save/Restore blocks and the control ops inside them. SkTDArray fSaveStack; SkTDArray fControlIndices; }; diff --git a/src/core/SkRecorder.cpp b/src/core/SkRecorder.cpp index 53522c2110..d49de20e68 100644 --- a/src/core/SkRecorder.cpp +++ b/src/core/SkRecorder.cpp @@ -230,7 +230,7 @@ SkCanvas::SaveLayerStrategy SkRecorder::willSaveLayer(const SkRect* bounds, } void SkRecorder::didRestore() { - APPEND(Restore, this->getTotalMatrix()); + APPEND(Restore, this->devBounds(), this->getTotalMatrix()); INHERITED(didRestore); } @@ -254,21 +254,21 @@ void SkRecorder::didSetMatrix(const SkMatrix& matrix) { } void SkRecorder::onClipRect(const SkRect& rect, SkRegion::Op op, ClipEdgeStyle edgeStyle) { - APPEND(ClipRect, rect, op, edgeStyle == kSoft_ClipEdgeStyle); INHERITED(onClipRect, rect, op, edgeStyle); + APPEND(ClipRect, this->devBounds(), rect, op, edgeStyle == kSoft_ClipEdgeStyle); } void SkRecorder::onClipRRect(const SkRRect& rrect, SkRegion::Op op, ClipEdgeStyle edgeStyle) { - APPEND(ClipRRect, rrect, op, edgeStyle == kSoft_ClipEdgeStyle); INHERITED(updateClipConservativelyUsingBounds, rrect.getBounds(), op, false); + APPEND(ClipRRect, this->devBounds(), rrect, op, edgeStyle == kSoft_ClipEdgeStyle); } void SkRecorder::onClipPath(const SkPath& path, SkRegion::Op op, ClipEdgeStyle edgeStyle) { - APPEND(ClipPath, delay_copy(path), op, edgeStyle == kSoft_ClipEdgeStyle); INHERITED(updateClipConservativelyUsingBounds, path.getBounds(), op, path.isInverseFillType()); + APPEND(ClipPath, this->devBounds(), delay_copy(path), op, edgeStyle == kSoft_ClipEdgeStyle); } void SkRecorder::onClipRegion(const SkRegion& deviceRgn, SkRegion::Op op) { - APPEND(ClipRegion, delay_copy(deviceRgn), op); INHERITED(onClipRegion, deviceRgn, op); + APPEND(ClipRegion, this->devBounds(), delay_copy(deviceRgn), op); } diff --git a/src/core/SkRecorder.h b/src/core/SkRecorder.h index 97eeb9f454..1373f8a77f 100644 --- a/src/core/SkRecorder.h +++ b/src/core/SkRecorder.h @@ -110,6 +110,12 @@ private: template T* copy(const T[], size_t count); + SkIRect devBounds() const { + SkIRect devBounds; + this->getClipDeviceBounds(&devBounds); + return devBounds; + } + SkRecord* fRecord; }; diff --git a/src/core/SkRecords.h b/src/core/SkRecords.h index 3d5b190893..bc768d9496 100644 --- a/src/core/SkRecords.h +++ b/src/core/SkRecords.h @@ -197,7 +197,7 @@ private: RECORD0(NoOp); -RECORD1(Restore, SkMatrix, matrix); +RECORD2(Restore, SkIRect, devBounds, SkMatrix, matrix); RECORD0(Save); RECORD3(SaveLayer, Optional, bounds, Optional, paint, SkCanvas::SaveFlags, flags); @@ -207,10 +207,10 @@ RECORD0(PopCull); RECORD1(Concat, SkMatrix, matrix); RECORD1(SetMatrix, SkMatrix, matrix); -RECORD3(ClipPath, SkPath, path, SkRegion::Op, op, bool, doAA); -RECORD3(ClipRRect, SkRRect, rrect, SkRegion::Op, op, bool, doAA); -RECORD3(ClipRect, SkRect, rect, SkRegion::Op, op, bool, doAA); -RECORD2(ClipRegion, SkRegion, region, SkRegion::Op, op); +RECORD4(ClipPath, SkIRect, devBounds, SkPath, path, SkRegion::Op, op, bool, doAA); +RECORD4(ClipRRect, SkIRect, devBounds, SkRRect, rrect, SkRegion::Op, op, bool, doAA); +RECORD4(ClipRect, SkIRect, devBounds, SkRect, rect, SkRegion::Op, op, bool, doAA); +RECORD3(ClipRegion, SkIRect, devBounds, SkRegion, region, SkRegion::Op, op); RECORD1(Clear, SkColor, color); // While not strictly required, if you have an SkPaint, it's fastest to put it first. diff --git a/tests/RecordDrawTest.cpp b/tests/RecordDrawTest.cpp index 2690013e16..e850dfee4c 100644 --- a/tests/RecordDrawTest.cpp +++ b/tests/RecordDrawTest.cpp @@ -95,3 +95,48 @@ DEF_TEST(RecordDraw_SetMatrixClobber, r) { expected.postConcat(translate); REPORTER_ASSERT(r, setMatrix->matrix == expected); } + +struct TestBBH : public SkBBoxHierarchy { + virtual void insert(void* data, const SkIRect& bounds, bool defer) SK_OVERRIDE { + Entry e = { (uintptr_t)data, bounds }; + entries.push(e); + } + virtual int getCount() const SK_OVERRIDE { return entries.count(); } + + virtual void flushDeferredInserts() SK_OVERRIDE {} + + virtual void search(const SkIRect& query, SkTDArray* results) const SK_OVERRIDE {} + virtual void clear() SK_OVERRIDE {} + virtual void rewindInserts() SK_OVERRIDE {} + virtual int getDepth() const SK_OVERRIDE { return -1; } + + struct Entry { + uintptr_t data; + SkIRect bounds; + }; + SkTDArray entries; +}; + +// 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) { + TestBBH bbh; + + SkRecord record; + + SkRecorder recorder(&record, W, H); + recorder.save(); + recorder.clipRect(SkRect::MakeWH(400, 500)); + recorder.scale(2, 2); + recorder.drawRect(SkRect::MakeWH(320, 240), SkPaint()); + recorder.restore(); + + SkRecordFillBounds(record, &bbh); + + REPORTER_ASSERT(r, bbh.entries.count() == 5); + for (int i = 0; i < bbh.entries.count(); i++) { + REPORTER_ASSERT(r, bbh.entries[i].data == (uintptr_t)i); + + REPORTER_ASSERT(r, bbh.entries[i].bounds == SkIRect::MakeWH(400, 500)); + } +}