can't abort looper loop, as saveCount won't be balanced

This is triggered by a recent change to clear the looper from the paint we return.
That change made the call to nothingToDraw() return true, which in turn meant
we didn't get the balancing call to restore in the looper's next() call.

Follow-up to https://skia-review.googlesource.com/c/skia/+/121062

Bug: skia:
Change-Id: I3ba7d487e4193103fb1d223d34c9c6eb486eca09
Reviewed-on: https://skia-review.googlesource.com/121220
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Florin Malita <fmalita@chromium.org>
This commit is contained in:
Mike Reed 2018-04-12 17:26:40 -04:00 committed by Skia Commit-Bot
parent 92cbf3fc01
commit 59af19f620
2 changed files with 27 additions and 8 deletions

View File

@ -487,7 +487,7 @@ bool AutoDrawLooper::doNext(SkDrawFilter::Type drawType) {
if (fFilter) { if (fFilter) {
if (!fFilter->filter(paint, drawType)) { if (!fFilter->filter(paint, drawType)) {
fDone = true; fDone = true;
return false; return false; // can we really do this, if we haven't finished fLooperContext?
} }
if (nullptr == fLooperContext) { if (nullptr == fLooperContext) {
// no looper means we only draw once // no looper means we only draw once
@ -500,12 +500,6 @@ bool AutoDrawLooper::doNext(SkDrawFilter::Type drawType) {
if (!fLooperContext && !fFilter) { if (!fLooperContext && !fFilter) {
fDone = true; fDone = true;
} }
// call this after any possible paint modifiers
if (fPaint->nothingToDraw()) {
fPaint = nullptr;
return false;
}
return true; return true;
} }
@ -2478,7 +2472,6 @@ void SkCanvas::onDrawTextRSXform(const void* text, size_t byteLength, const SkRS
void SkCanvas::onDrawTextBlob(const SkTextBlob* blob, SkScalar x, SkScalar y, void SkCanvas::onDrawTextBlob(const SkTextBlob* blob, SkScalar x, SkScalar y,
const SkPaint& paint) { const SkPaint& paint) {
SkRect storage; SkRect storage;
const SkRect* bounds = nullptr; const SkRect* bounds = nullptr;
if (paint.canComputeFastBounds()) { if (paint.canComputeFastBounds()) {

View File

@ -174,3 +174,29 @@ DEF_TEST(QuickReject_MatrixState, reporter) {
// quickReject() will assert if the matrix is out of sync. // quickReject() will assert if the matrix is out of sync.
canvas.quickReject(SkRect::MakeWH(100.0f, 100.0f)); canvas.quickReject(SkRect::MakeWH(100.0f, 100.0f));
} }
#include "SkLayerDrawLooper.h"
#include "SkSurface.h"
DEF_TEST(looper_nothingtodraw, reporter) {
auto surf = SkSurface::MakeRasterN32Premul(20, 20);
SkPaint paint;
paint.setColor(0);
REPORTER_ASSERT(reporter, paint.nothingToDraw());
SkLayerDrawLooper::Builder builder;
builder.addLayer();
paint.setDrawLooper(builder.detach());
// the presence of the looper fools this predicate, so we think it might draw
REPORTER_ASSERT(reporter, !paint.nothingToDraw());
// Before fixing, this would assert in ~AutoDrawLooper() in SkCanvas.cpp as it checked for
// a balance in the save/restore count after handling the looper. Before the fix, this
// code would call nothingToDraw() and since it now clears the looper, that predicate will
// return true, aborting the sequence prematurely, and not finishing the iterator on the looper
// which handles the final "restore". This was a bug -- we *must* call the looper's iterator
// until it returns done to keep the canvas balanced. The fix was to remove this early-exit
// in the autodrawlooper. Now this call won't assert.
// See https://skia-review.googlesource.com/c/skia/+/121220
surf->getCanvas()->drawRect({1, 1, 10, 10}, paint);
}