Counterproposal for skirting the BBH when the query fully contains the picture.

Counterproposal for skirting the BBH when the query fully contains the picture.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/3e8232b6ef6b04290f0c280f88b38f8cd2c3f3ec

[reed1, 2014-08-18 18:48:44]

new placement seems fine.

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

https://codereview.chromium.org/485703002/diff/1/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:202: canvas->getClipBounds(&clipBounds);
If this returns false, clipBounds is undefined.

https://codereview.chromium.org/485703002/diff/1/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:203: const bool useBBH =
!clipBounds.contains(SkRect::MakeWH(this->width(), this->height()));
This will generate int->float warnings

[mtklein, 2014-08-18 19:02:49]

also added a unit test

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

https://codereview.chromium.org/485703002/diff/1/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:202: canvas->getClipBounds(&clipBounds);
On 2014/08/18 18:48:44, reed1 wrote:
> If this returns false, clipBounds is undefined.

Done.

https://codereview.chromium.org/485703002/diff/1/src/core/SkPicture.cpp#newco...
src/core/SkPicture.cpp:203: const bool useBBH =
!clipBounds.contains(SkRect::MakeWH(this->width(), this->height()));
On 2014/08/18 18:48:44, reed1 wrote:
> This will generate int->float warnings

Let's see?

[mtklein, 2014-08-18 19:10:47]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-08-18 19:10:56]

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

[commit-bot: I haz the power, 2014-08-18 19:10:58]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from a valid reviewer till 2014-08-19 01:10 UTC

[reed1, 2014-08-18 19:42:39]

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

https://codereview.chromium.org/485703002/diff/40001/src/core/SkPicture.cpp#n...
src/core/SkPicture.cpp:201: SkRect clipBounds = { 0, 0, 0, 0 };
fyi : we do have MakeEmpty(), but I'm fine with the { 0... }

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

https://codereview.chromium.org/485703002/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.
Not sure I follow the comment.
Do we want to keep the #if 1 in the checked-in code?

[mtklein, 2014-08-18 19:45:24]

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

https://codereview.chromium.org/485703002/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/18 19:42:39, reed1 wrote:
> Not sure I follow the comment.
> Do we want to keep the #if 1 in the checked-in code?

Yeah, I'd say so, at least until I understand why we build the BBH in
device-coordinates but seemingly make queries in user-space.  As it is, it
happens that GMs draw right this way and wrong in the else case, but I'd like to
leave a better explanation here.

[reed1, 2014-08-18 19:48:54]

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

https://codereview.chromium.org/485703002/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/18 19:45:24, mtklein wrote:
> On 2014/08/18 19:42:39, reed1 wrote:
> > Not sure I follow the comment.
> > Do we want to keep the #if 1 in the checked-in code?
> 
> Yeah, I'd say so, at least until I understand why we build the <span
id="abbrex-spnAbbr_12230" class="abbrex-Abbr abbrex-Abbr_BBH abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">BBH</span> in
> device-coordinates but seemingly make queries in user-space.  As it is, it
> happens that <span id="abbrex-spnAbbr_12231" class="abbrex-Abbr abbrex-Abbr_GM
abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">GM</span>s draw right this way and
wrong in the else case, but I'd like to
> leave a better explanation here.

Ah. Does the comment mean : "why isn't getClipDeviceBounds sufficient" ?

Lets say something about how to reproduce the problem here in the code if we're
going to have #else.

e.g.
/*
 * Seems like getClipDeviceBounds should be sufficient, but if we call it, some
GMs fail to draw correctly.
 * Lets figure out why : is that a bug in the GM, a bug in BBH, a bug in
getClipDeviceBounds, or other.
 */

[reed1, 2014-08-18 19:49:12]

lgtm w/ better dox for #if/else

[mtklein, 2014-08-18 20:29:56]

On 2014/08/18 19:49:12, reed1 wrote:
> lgtm w/ better dox for #if/else

Done!  Thanks for the help.

[mtklein, 2014-08-18 20:30:12]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-08-18 20:31:05]

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

[commit-bot: I haz the power, 2014-08-18 20:39:16]

Committed patchset #4 (60001) as 3e8232b6ef6b04290f0c280f88b38f8cd2c3f3ec