Fixing SkPicture command pattern optimizations to make them work correctly with bounding box hierar…

Fixing SkPicture command pattern optimizations to make them work correctly with bounding box hierarchies

BUG=https://code.google.com/p/chromium/issues/detail?id=180645
TEST=render_pictures -r <skp_dir> --validate --bbh <grid|rtree> --mode tile 256 256

Committed: http://code.google.com/p/skia/source/detail?r=8171

[Justin Novosad, 2013-03-14 18:02:49]

PTAL.
This patch eliminates all significant pixel diffs we were reported by
render_pictures, when using --bbh.

With this patch, the worst cases yield pixel diffs of 1.

https://codereview.chromium.org/12817011/diff/9001/src/core/SkPicturePlayback...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/12817011/diff/9001/src/core/SkPicturePlayback...
src/core/SkPicturePlayback.cpp:718: size_t skipTo = 0;
The changes below may look familiar: they were copied from this CL:
https://codereview.chromium.org/12545009/

[Justin Novosad, 2013-03-14 18:03:23]

PTAL.

[robertphillips, 2013-03-14 18:10:17]

Were those alpha pixels diffs or color pixels diffs?

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

https://codereview.chromium.org/12817011/diff/9001/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:527: case kCollapseSaveLayer_OptType:
NULL !=

https://codereview.chromium.org/12817011/diff/9001/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:534: // unreferenced branches dangling.
NULL !=

https://codereview.chromium.org/12817011/diff/9001/src/core/SkTileGrid.cpp
File src/core/SkTileGrid.cpp (right):

https://codereview.chromium.org/12817011/diff/9001/src/core/SkTileGrid.cpp#ne...
src/core/SkTileGrid.cpp:126: SkASSERT(fClient);
pre-increment?

[Justin Novosad, 2013-03-14 18:16:19]

On 2013/03/14 18:10:17, robertphillips wrote:
> Were those alpha pixels diffs or color pixels diffs?
> 
I know there were color diffs for sure, not sure if there were alpha-only diffs
in some cases.  (The logs don't say) 

http://108.170.217.252:10117/builders/Skia_Shuttle_Ubuntu12_ATI5770_Float_Deb...

[Justin Novosad, 2013-03-14 18:20:31]

Patch.

[robertphillips, 2013-03-14 18:55:44]

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchy.h
File src/core/SkBBoxHierarchy.h (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchy.h...
src/core/SkBBoxHierarchy.h:15: 
comment

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchy.h...
src/core/SkBBoxHierarchy.h:19: 
Could this be something more than a void*?

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchy.h...
src/core/SkBBoxHierarchy.h:68: 
"{ fClient = client; }" - spaces

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchyRe...
File src/core/SkBBoxHierarchyRecord.cpp (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchyRe...
src/core/SkBBoxHierarchyRecord.cpp:106: 
comment? // Why does reverting any Draw state after the writeStream's size work?

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

https://codereview.chromium.org/12817011/diff/4002/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:524: 
comment? // This method is called after the passed in optimization has been
applied ...
Would it be better to just pass in what is supposed to be done rather than the
index of the optimization?

https://codereview.chromium.org/12817011/diff/4002/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:532: case kRewind_OptType:
This is a rather ominous comment.

https://codereview.chromium.org/12817011/diff/4002/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:567: initialOffset = fWriter.size();
this->

https://codereview.chromium.org/12817011/diff/4002/src/core/SkPictureStateTree.h
File src/core/SkPictureStateTree.h (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkPictureStateTre...
src/core/SkPictureStateTree.h:118: Node* fRoot;
Do we want a comment like: // ideally the state tree would be doubly linked so
we could XXX but for right now we allow one level patches
I'm thinking about the case where we apply the peephole optimizations
iteratively and we can get consecutive collapses.

https://codereview.chromium.org/12817011/diff/4002/src/core/SkRTree.cpp
File src/core/SkRTree.cpp (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkRTree.cpp#newco...
src/core/SkRTree.cpp:436: 
What's going to happen in none deferred mode?

https://codereview.chromium.org/12817011/diff/4002/src/core/SkTileGrid.cpp
File src/core/SkTileGrid.cpp (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkTileGrid.cpp#ne...
src/core/SkTileGrid.cpp:126: SkASSERT(fClient);
ouch - every tile.

https://codereview.chromium.org/12817011/diff/4002/src/core/SkTileGrid.h
File src/core/SkTileGrid.h (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkTileGrid.h#newc...
src/core/SkTileGrid.h:57: 
comment - what is the contract for this operation?

[Justin Novosad, 2013-03-14 20:04:14]

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchy.h
File src/core/SkBBoxHierarchy.h (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchy.h...
src/core/SkBBoxHierarchy.h:15: 
On 2013/03/14 18:55:44, robertphillips wrote:
> comment

Done.

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchy.h...
src/core/SkBBoxHierarchy.h:19: 
On 2013/03/14 18:55:44, robertphillips wrote:
> Could this be something more than a void*?
void* is used to reference SkPictureStateTree::Draw objects in all the BBH
classes that are not related to SkPicture.  I think this should be refactored
some day to become SkBBoxHierarchy::Element*, but that would be another CL

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchy.h...
src/core/SkBBoxHierarchy.h:68: 
On 2013/03/14 18:55:44, robertphillips wrote:
> "{ fClient = client; }" - spaces

Done.

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchyRe...
File src/core/SkBBoxHierarchyRecord.cpp (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkBBoxHierarchyRe...
src/core/SkBBoxHierarchyRecord.cpp:106: 
On 2013/03/14 18:55:44, robertphillips wrote:
> comment? // Why does reverting any Draw state after the writeStream's size
work?

Done.

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

https://codereview.chromium.org/12817011/diff/4002/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:532: case kRewind_OptType:
On 2013/03/14 18:55:44, robertphillips wrote:
> This is a rather ominous comment.

Done.

https://codereview.chromium.org/12817011/diff/4002/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:567: initialOffset = fWriter.size();
On 2013/03/14 18:55:44, robertphillips wrote:
> this->

Done.

https://codereview.chromium.org/12817011/diff/4002/src/core/SkPictureStateTree.h
File src/core/SkPictureStateTree.h (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkPictureStateTre...
src/core/SkPictureStateTree.h:118: Node* fRoot;
On 2013/03/14 18:55:44, robertphillips wrote:
> Do we want a comment like: // ideally the state tree would be doubly linked so
> we could XXX but for right now we allow one level patches
> I'm thinking about the case where we apply the peephole optimizations
> iteratively and we can get consecutive collapses.

Done.

https://codereview.chromium.org/12817011/diff/4002/src/core/SkRTree.cpp
File src/core/SkRTree.cpp (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkRTree.cpp#newco...
src/core/SkRTree.cpp:436: 
On 2013/03/14 18:55:44, robertphillips wrote:
> What's going to happen in none deferred mode?
Then rewind optimizations will be illegal, and we will have to improve this
method.  That use case is not currently exercised by any of our test programs,
and RTree is not even used in production, so I see no point in investing effort
in that direction for now.

https://codereview.chromium.org/12817011/diff/4002/src/core/SkTileGrid.cpp
File src/core/SkTileGrid.cpp (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkTileGrid.cpp#ne...
src/core/SkTileGrid.cpp:126: SkASSERT(fClient);
On 2013/03/14 18:55:44, robertphillips wrote:
> ouch - every tile.

You just gave me a good idea for a follow-up patch.  This method could receive a
bounds argument in order to only target the tiles affected by the rewound
commands.  I just filed a bug for it.
https://code.google.com/p/skia/issues/detail?id=1165

https://codereview.chromium.org/12817011/diff/4002/src/core/SkTileGrid.h
File src/core/SkTileGrid.h (right):

https://codereview.chromium.org/12817011/diff/4002/src/core/SkTileGrid.h#newc...
src/core/SkTileGrid.h:57: 
On 2013/03/14 18:55:44, robertphillips wrote:
> comment - what is the contract for this operation?

The contract is documented in the base class header.

[robertphillips, 2013-03-14 23:20:32]

lgtm

[commit-bot: I haz the power, 2013-03-15 13:55:56]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/junov@chromium.org/12817011/13002

[commit-bot: I haz the power, 2013-03-15 15:06:13]

Change committed as 8171