AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Don't adjust the bounds after a restore with the restore's paired saveLayer's paint.

Browse Source 
It makes no sense for the paint from a saveLayer to effect anything outside its saveLayer/restore block.  But as currently written, we'll adjust the clip bounds just after a restore by that paint.

Turns out the test I wrote for the last CL (which caused this bug) actually had the bug baked into its expectations.  I've updated them and added some notes to explain.

BUG=418417

Review URL: https://codereview.chromium.org/623563002
This commit is contained in:
mtklein 2014-10-01 12:48:58 -07:00 committed by Commit bot
parent 8324a6a27e
commit 8e393bf70e
2 changed files with 38 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

39
src/core/SkRecordDraw.cpp
View File

@ -7,7 +7,6 @@
#include "SkRecordDraw.h"
#include "SkPatchUtils.h"
#include "SkTLogic.h"
void SkRecordDraw(const SkRecord& record,
SkCanvas* canvas,
@ -181,26 +180,40 @@ private:
const SkPaint* paint; // Unowned. If set, adjusts the bounds of all ops in this block.
};
template <typename T> void updateCTM(const T&) { /* most ops don't change the CTM */ }
// Only Restore and SetMatrix change the CTM.
template <typename T> void updateCTM(const T&) {}
void updateCTM(const Restore& op) { fCTM = &op.matrix; }
void updateCTM(const SetMatrix& op) { fCTM = &op.matrix; }
// Most ops don't change the clip. Those that do generally have a field named devBounds.
SK_CREATE_MEMBER_DETECTOR(devBounds);
// Most ops don't change the clip.
template <typename T> void updateClipBounds(const T&) {}
template <typename T>
SK_WHEN(!HasMember_devBounds<T>, void) updateClipBounds(const T& op) {}
// Clip{Path,RRect,Rect,Region} obviously change the clip. They all know their bounds already.
void updateClipBounds(const ClipPath& op) { this->updateClipBoundsForClipOp(op.devBounds); }
void updateClipBounds(const ClipRRect& op) { this->updateClipBoundsForClipOp(op.devBounds); }
void updateClipBounds(const ClipRect& op) { this->updateClipBoundsForClipOp(op.devBounds); }
void updateClipBounds(const ClipRegion& op) { this->updateClipBoundsForClipOp(op.devBounds); }
// Each of the devBounds fields holds the state of the device bounds after the op.
// (So Restore's devBounds are those bounds saved by its paired Save or SaveLayer.)
template <typename T>
SK_WHEN(HasMember_devBounds<T>, void) updateClipBounds(const T& op) {
Bounds clip = SkRect::Make(op.devBounds);
// The bounds of clip ops need to be adjusted for the paints of saveLayers they're inside.
void updateClipBoundsForClipOp(const SkIRect& devBounds) {
Bounds clip = SkRect::Make(devBounds);
// We don't call adjustAndMap() because as its last step it would intersect the adjusted
// clip bounds with the previous clip, exactly what we can't do when the clip grows.
fCurrentClipBounds = this->adjustForSaveLayerPaints(&clip) ? clip : Bounds::MakeLargest();
}
// Restore holds the devBounds for the clip after the {save,saveLayer}/restore block completes.
void updateClipBounds(const Restore& op) {
// This is just like the clip ops above, but we need to skip the effects (if any) of our
// paired saveLayer (if it is one); it has not yet been popped off the save stack. Our
// devBounds reflect the state of the world after the saveLayer/restore block is done,
// so they are not affected by the saveLayer's paint.
const int kSavesToIgnore = 1;
Bounds clip = SkRect::Make(op.devBounds);
fCurrentClipBounds =
this->adjustForSaveLayerPaints(&clip, kSavesToIgnore) ? clip : Bounds::MakeLargest();
}
// We also take advantage of SaveLayer bounds when present to further cut the clip down.
void updateClipBounds(const SaveLayer& op) {
if (op.bounds) {
@ -478,8 +491,8 @@ private:
return true;
}
bool adjustForSaveLayerPaints(SkRect* rect) const {
for (int i = fSaveStack.count() - 1; i >= 0; i--) {
bool adjustForSaveLayerPaints(SkRect* rect, int savesToIgnore = 0) const {
for (int i = fSaveStack.count() - 1 - savesToIgnore; i >= 0; i--) {
if (!AdjustForPaint(fSaveStack[i].paint, rect)) {
return false;
}

17
tests/RecordDrawTest.cpp
View File

@ -227,6 +227,10 @@ DEF_TEST(RecordDraw_PartialClear, r) {
}
// A regression test for crbug.com/415468 and skbug.com/2957.
//
// This also now serves as a regression test for crbug.com/418417. We used to adjust the
// bounds for the saveLayer, clip, and restore to be greater than the bounds of the picture.
// (We were applying the saveLayer paint to the bounds after restore, which makes no sense.)
DEF_TEST(RecordDraw_SaveLayerAffectsClipBounds, r) {
SkRecord record;
SkRecorder recorder(&record, 50, 50);
@ -241,13 +245,16 @@ DEF_TEST(RecordDraw_SaveLayerAffectsClipBounds, r) {
recorder.drawRect(SkRect::MakeWH(20, 40), SkPaint());
recorder.restore();
// Under the original bug, all the right edge values would be 20 less than asserted here
// because we intersected them with a clip that had not been adjusted for the drop shadow.
// Under the original bug, the right edge value of the drawRect would be 20 less than asserted
// here because we intersected it with a clip that had not been adjusted for the drop shadow.
//
// 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(record, &bbh);
REPORTER_ASSERT(r, bbh.entries.count() == 4);
REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[0].bounds, SkRect::MakeLTRB(0, 0, 70, 50)));
REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[1].bounds, SkRect::MakeLTRB(0, 0, 70, 50)));
REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[0].bounds, SkRect::MakeLTRB(0, 0, 50, 50)));
REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[1].bounds, SkRect::MakeLTRB(0, 0, 50, 50)));
REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[2].bounds, SkRect::MakeLTRB(0, 0, 40, 40)));
REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[3].bounds, SkRect::MakeLTRB(0, 0, 70, 50)));
REPORTER_ASSERT(r, sloppy_rect_eq(bbh.entries[3].bounds, SkRect::MakeLTRB(0, 0, 50, 50)));
}