Prevent picture recording from over optimizing the culling of clips.

Prevent picture recording from over optimizing the culling of clips.

BUG=skia:1496
R=mtklein@google.com, reed@google.com, robertphillips@google.com

Committed: https://code.google.com/p/skia/source/detail?r=10689

[djsollen, 2013-08-12 20:51:06]

This is still failing on tiled and rtree playback and I don't know why.

[robertphillips, 2013-08-13 11:36:15]

I only get "error: old chunk mismatch" errors when I try to review this.

[robertphillips, 2013-08-13 12:12:31]

lgtm + nits & some (mainly comment) suggestions

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp
File gm/canvasstate.cpp (right):

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcode12
gm/canvasstate.cpp:12: #include "SkRect.h"
put this in the skiagm namespace?

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcode13
gm/canvasstate.cpp:13: 
// This GM exercises the flags to save(). save() and restore()'s action can //
limited to only a portion of the state through the use of save's flags 
// field.

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcode46
gm/canvasstate.cpp:46: fInsetRect = SkRect::MakeXYWH(1, 1, WIDTH-2, HEIGHT-2);
I'm surprised fFullRect isn't 0,0 -> WIDTH,HEIGHT

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcode50
gm/canvasstate.cpp:50: protected:
SK_OVERRIDE?

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcode54
gm/canvasstate.cpp:54: 
SK_OVERRIDE?

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcode58
gm/canvasstate.cpp:58: 
SK_OVERRIDE?

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcode64
gm/canvasstate.cpp:64: // test that we don't drop the clip since we are not
setting the clip bit
Would it make sense to encapsulate all these in:

drawStuff(saveFlags, doClip, doScale);

and call the function 6 times?

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcod...
gm/canvasstate.cpp:141: 
DEF_GM?

https://codereview.chromium.org/22875008/diff/16001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/22875008/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:481: SkASSERT(SAVE == op);
Add a kSaveSize near the kSaveLayerNoBoundsSize global and test that it is
correct in save()?

https://codereview.chromium.org/22875008/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:485: SkCanvas::SaveFlags saveFlag =
(SkCanvas::SaveFlags) *writer->peek32(offset+4);
Is the evil combo we see everywhere a MatrixClip save? If not this could cause a
regression.

https://codereview.chromium.org/22875008/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:486: if (SkCanvas::kMatrixClip_SaveFlag !=
(SkCanvas::kMatrixClip_SaveFlag & saveFlag)) {
// The following code is only correct for kMatrixClip style save's.
// TODO: set checkMatrix & checkClip booleans here and then check for the
offending operations in the following loop.

[robertphillips, 2013-08-13 12:36:28]

Also, could you enhance the save comment in SkCanvas to include something like:

@param flags - these flags govern what portions of the Matrix/Clip/drawFilter
state the save (and matching restore) effect. If only kMatrix is specified, only
the matrix state will be pushed and popped. If only kClip is specified, only the
clip state will be pushed and popped.

[robertphillips, 2013-08-13 12:41:08]

Note: the drawFilter is always pushed and popped.

[djsollen, 2013-08-13 13:24:45]

I think this addresses all your comments.

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp
File gm/canvasstate.cpp (right):

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcode12
gm/canvasstate.cpp:12: #include "SkRect.h"
On 2013/08/13 12:12:32, robertphillips wrote:
> put this in the skiagm namespace?

Done.

https://codereview.chromium.org/22875008/diff/16001/gm/canvasstate.cpp#newcode13
gm/canvasstate.cpp:13: 
On 2013/08/13 12:12:32, robertphillips wrote:
> // This GM exercises the flags to save(). save() and restore()'s action can //
> limited to only a portion of the state through the use of save's flags 
> // field.

Done.

https://codereview.chromium.org/22875008/diff/16001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/22875008/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:485: SkCanvas::SaveFlags saveFlag =
(SkCanvas::SaveFlags) *writer->peek32(offset+4);
On 2013/08/13 12:12:32, robertphillips wrote:
> Is the evil combo we see everywhere a MatrixClip save? If not this could cause
a
> regression.

Chrome doesn't do selective save and restores like this so yes we will still
catch all the evil cases.

[robertphillips, 2013-08-13 13:45:55]

nits + a question

https://codereview.chromium.org/22875008/diff/22001/gm/canvasstate.cpp
File gm/canvasstate.cpp (right):

https://codereview.chromium.org/22875008/diff/22001/gm/canvasstate.cpp#newcode62
gm/canvasstate.cpp:62: virtual SkISize onISize() SK_OVERRIDE {
Should the width & height here be computed from WIDTH & HEIGHT?

https://codereview.chromium.org/22875008/diff/22001/gm/canvasstate.cpp#newcode68
gm/canvasstate.cpp:68: // test that we don't drop the clip since we are not
setting the clip bit
this-> on all these?

https://codereview.chromium.org/22875008/diff/22001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/22875008/diff/22001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:474: }
What is this?

[djsollen, 2013-08-13 13:53:05]

I think the latest patch should be good for review.

[mtklein, 2013-08-13 13:57:53]

lgtm with some nits of my own

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:153: SkASSERT(kSaveSize == size);
This seems a little roundabout to me, given that they're both literally "2 *
kUInt32Size".  Why not
  uint32_t size = kSaveSize;
?

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:488: SkCanvas::SaveFlags saveFlag =
(SkCanvas::SaveFlags) *writer->peek32(offset+4);
This is a nit, but given that it's a bitwise enum, calling it flags instead of
flag can be a useful distinction.

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:488: SkCanvas::SaveFlags saveFlag =
(SkCanvas::SaveFlags) *writer->peek32(offset+4);
Again, a nit, but this guy can be const.

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:489: if (SkCanvas::kMatrixClip_SaveFlag !=
(SkCanvas::kMatrixClip_SaveFlag & saveFlag)) {
Maybe I'm brainfarting, but can't we write this as

if (SkCanvas::kMatrixClip_SaveFlag != saveFlag) ?

[reed1, 2013-08-13 14:28:00]

lgtm

[djsollen, 2013-08-13 14:29:16]

Committed patchset #7 manually as r10689.

[robertphillips, 2013-08-13 14:33:03]

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:153: SkASSERT(kSaveSize == size);
Mainly for solidarity with 
kSaveLayerNoBoundsSize and kSaveLayerWithBoundsSize

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:489: if (SkCanvas::kMatrixClip_SaveFlag !=
(SkCanvas::kMatrixClip_SaveFlag & saveFlag)) {
Unfortunately there are several other save flags that need to be filtered out.

[mtklein, 2013-08-13 14:40:47]

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/22875008/diff/31001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:489: if (SkCanvas::kMatrixClip_SaveFlag !=
(SkCanvas::kMatrixClip_SaveFlag & saveFlag)) {
On 2013/08/13 14:33:03, robertphillips wrote:
> Unfortunately there are several other save flags that need to be filtered out.

Either way, aren't we filtering out anything which is not kMatrixClip?