Fix cull nesting assertion.

Fix cull nesting assertion.

Cull rects are in local coordinates and cannot be compared directly.

No wonder it was so hard enforcing this in Blink :o

This moves the validation logic into SkCanvas, using a device-space
cull stack (debug build only).

There are still some Blink bugs causing violations, so for now I'd like
to keep this as an error message only.

R=reed@google.com,robertphillips@google.com

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

[robertphillips, 2014-03-20 12:23:56]

https://codereview.chromium.org/200923008/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/200923008/diff/1/include/core/SkCanvas.h#newc...
include/core/SkCanvas.h:1397: #ifdef SK_DEBUG
// The cull stack cull rects are in device-space

https://codereview.chromium.org/200923008/diff/1/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/200923008/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1166: // Ensure that cull rects are properly nested in
device space.
I think this is a bad name since it conflates validation with the actual push
operation.

https://codereview.chromium.org/200923008/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1175: if (!devCull.intersect(devClip))
{} needed

https://codereview.chromium.org/200923008/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1186: #ifdef ASSERT_NESTED_CULLING
This assert seems odd inside this code block.

https://codereview.chromium.org/200923008/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1195: void SkCanvas::pushCull(const SkRect& cullRect) {
this->

https://codereview.chromium.org/200923008/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (left):

https://codereview.chromium.org/200923008/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1561: void SkPictureRecord::onPushCull(const
SkRect& cullRect) {
Is this no longer useful?

[f(malita), 2014-03-20 13:30:58]

https://codereview.chromium.org/200923008/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/200923008/diff/1/include/core/SkCanvas.h#newc...
include/core/SkCanvas.h:1397: #ifdef SK_DEBUG
On 2014/03/20 12:23:56, robertphillips wrote:
> // The cull stack cull rects are in device-space 

Done.

https://codereview.chromium.org/200923008/diff/1/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/200923008/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1166: // Ensure that cull rects are properly nested in
device space.
On 2014/03/20 12:23:56, robertphillips wrote:
> I think this is a bad name since it conflates validation with the actual push
> operation.

Renamed to validateAndTrack().

https://codereview.chromium.org/200923008/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1175: if (!devCull.intersect(devClip))
On 2014/03/20 12:23:56, robertphillips wrote:
> {} needed

Done.

https://codereview.chromium.org/200923008/diff/1/src/core/SkCanvas.cpp#newcod...
src/core/SkCanvas.cpp:1186: #ifdef ASSERT_NESTED_CULLING
On 2014/03/20 12:23:56, robertphillips wrote:
> This assert seems odd inside this code block.

Hmm, the intention is to provide an easy way to select error behavior. Maybe
DEBUGFAIL would not seem so strange? Done.

Or would you rather remove it completely?

https://codereview.chromium.org/200923008/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (left):

https://codereview.chromium.org/200923008/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1561: void SkPictureRecord::onPushCull(const
SkRect& cullRect) {
On 2014/03/20 12:23:56, robertphillips wrote:
> Is this no longer useful?

Potentially still useful, but not easy to pull off efficiently with rects being
in arbitrary coord systems: we'd have to add a stack and map everything into a
common space (the assert cull stack essentially).

I'm worried that it would hurt record performance considerably. Running some
Telemetry tests to see what the impact of removing this is.

[robertphillips, 2014-03-20 14:20:41]

https://codereview.chromium.org/200923008/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (left):

https://codereview.chromium.org/200923008/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1561: void SkPictureRecord::onPushCull(const
SkRect& cullRect) {
Aren't they all in device space now?

[f(malita), 2014-03-20 14:38:20]

https://codereview.chromium.org/200923008/diff/1/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (left):

https://codereview.chromium.org/200923008/diff/1/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:1561: void SkPictureRecord::onPushCull(const
SkRect& cullRect) {
On 2014/03/20 14:20:41, robertphillips wrote:
> Aren't they all in device space now?

They are in SkCanvas::fCullStack - but that is debug-time-only, with the sole
purpose of verifying monotonic nesting. For clients (and at this level:
prevCull, cullRect) they're in local coordinates.

[f(malita), 2014-03-20 15:23:51]

Relocated the tracking logic into pushCull proper so that it's balanced vs.
popCull.

[robertphillips, 2014-03-20 15:28:14]

lgtm fwiw

[reed1, 2014-03-20 20:03:35]

lgtm

[f(malita), 2014-03-20 20:17:26]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:17:28]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/fmalita@chromium.org/200923008/40001

[commit-bot: I haz the power, 2014-03-20 20:25:17]

Change committed as 13885