SkRecordDraw: incorporate clip into BBH

SkRecordDraw: incorporate clip into BBH

NOTREECHECKS=true

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/a723b576aed31a6eb2bdda6388e6bd779d04c6b0

[Stephen White, 2014-08-14 17:23:07]

https://codereview.chromium.org/474983002/diff/40001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/474983002/diff/40001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:254: rect = paint->computeFastBounds(rect, &rect);
If you have a paint that can't computeFastBounds(), you'll have to 
assume the bounds are the entire clip rect(at least, that's how the
old code in SkBBoxRecord worked).

[Stephen White, 2014-08-14 17:40:04]

https://codereview.chromium.org/474983002/diff/40001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/474983002/diff/40001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:20: #if 1   // TODO: Why is this the right way to make
the query?  I'd think it'd be the else branch.
I'm not super-familiar with the BBH code, but it looks like
SkPicturePlayback::getActiveOps() does the same thing.
I'm guessing that SkBBH::search() takes a rect in object-space coords, 
not device-space coords, despite the fact that takes an SkIRect.

Maybe Justin knows.

[mtklein, 2014-08-14 17:54:03]

https://codereview.chromium.org/474983002/diff/40001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/474983002/diff/40001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:20: #if 1   // TODO: Why is this the right way to make
the query?  I'd think it'd be the else branch.
On 2014/08/14 17:40:04, Stephen White wrote:
> I'm not super-familiar with the BBH code, but it looks like
> SkPicturePlayback::getActiveOps() does the same thing.
> I'm guessing that SkBBH::search() takes a rect in object-space coords, 
> not device-space coords, despite the fact that takes an SkIRect.
> 
> Maybe Justin knows.

Yeah, stole this from there.

As far as I can tell, all the BBHs ought to work entirely in device coordinates,
and yet... many GMs draw wrong if we query the device coordinates directly.  A
little bit of debugging made it look like we maybe get back a different bounds
that I'd hope from getClipDeviceBounds().  The top-left tile looks right, and
everything's the right size, but the other tiles all start from (0,0).  No
translation offset.

https://codereview.chromium.org/474983002/diff/40001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:254: rect = paint->computeFastBounds(rect, &rect);
On 2014/08/14 17:23:07, Stephen White wrote:
> If you have a paint that can't computeFastBounds(), you'll have to 
> assume the bounds are the entire clip rect(at least, that's how the
> old code in SkBBoxRecord worked).

Thanks, good catch.  All the bounds are the clip right now, so it never came up.
:)

We don't do this check today for the SaveLayer paints.  Seem right to do fail to
the clip bounds if those paints can't compute bounds too?

[robertphillips, 2014-08-15 17:48:08]

lgtm modulo docs requests & a question

https://codereview.chromium.org/474983002/diff/80001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/474983002/diff/80001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:127: fCTM.setIdentity();
Could this be the bounds of the picture instead ?

https://codereview.chromium.org/474983002/diff/80001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:172: template <typename T> void updateClipBounds(const
T&) { /* most ops don't change the clip */ }
// Note: restore's updateClipBounds "pops" the clipBounds of its matching
save/saveLayer

?

https://codereview.chromium.org/474983002/diff/80001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:283: SkMatrix fCTM;
fCurClipBounds ?
// The bounds of the clip state currently in effect ?

[mtklein, 2014-08-15 18:34:01]

https://codereview.chromium.org/474983002/diff/80001/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/474983002/diff/80001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:127: fCTM.setIdentity();
On 2014/08/15 17:48:07, robertphillips wrote:
> Could this be the bounds of the picture instead ?

Yes.  Would have done this had the bounds been laying around conveniently.  In
practice, I don't think it'll make any difference.  Seems unusual to ask to draw
the picture outside its bounds.

https://codereview.chromium.org/474983002/diff/80001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:172: template <typename T> void updateClipBounds(const
T&) { /* most ops don't change the clip */ }
On 2014/08/15 17:48:07, robertphillips wrote:
> // Note: restore's updateClipBounds "pops" the clipBounds of its matching
> save/saveLayer
> 
> ?

Done.

https://codereview.chromium.org/474983002/diff/80001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:283: SkMatrix fCTM;
On 2014/08/15 17:48:07, robertphillips wrote:
> fCurClipBounds ?
> // The bounds of the clip state currently in effect ?

Done.

[mtklein, 2014-08-15 18:34:06]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-08-15 18:34:23]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/474983002/100001

[commit-bot: I haz the power, 2014-08-15 18:42:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)

[mtklein, 2014-08-15 18:42:24]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-08-15 18:42:35]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/474983002/120001

[commit-bot: I haz the power, 2014-08-15 18:49:54]

Committed patchset #7 (120001) as a723b576aed31a6eb2bdda6388e6bd779d04c6b0