Skip unnecessary quick reject testing during playback when using a BBoxHierarchy

Skip unnecessary quick reject testing during playback when using a BBoxHierarchy

TEST=bench_pictures

[junov1, 2013-02-11 21:21:23]

PTAL

Note: This patch should not be landed until after this one lands:
https://codereview.appspot.com/7311075/
See comments in that other CL about perf impact.

This patch eliminates quick rejection of draw calls based on bounds testing
during picture playback in cases where
a) quick rejection was already performed at record time, and
b) A bounding box hierarchy was used to further reject calls that are outside of
playback bounds.

[reed1, 2013-02-11 21:52:41]

I think this is an interesting change, as we have talked about making
quick-reject optional for a while. However, I'd like to evaluate and measure it
on its own, and not just in conjunction w/ the other proposed CL.

1. Not sure I agree that we should complicate quickReject(rect) by adding a
paint parameter. This makes that code now perform at least one extra test and a
rect-copy before landing where the old code was (i.e. isFinite). What motivated
this change? Could it make sense to just add a new entry-point that took a const
SkPaint&? That version could call into the current one, and would not need to
check for null.

2. What is the perf cost (if any) for checking this new bool? Can we try just
making that change, and then timing a bunch of calls to quickReject? Hopefully
its very small, but we need to know if we are adding a cost in the case where we
*do* perform the reject test.

3. Nit: I think I'd rather have the API and field not have a negative in its
name. Can we call it enableQuickReject or something like that? Not critical, but
I always have to think harder when I read predicates that are sort of negative.

[junov1, 2013-02-11 22:09:47]

On 2013/02/11 21:52:41, reed1 wrote:
> I think this is an interesting change, as we have talked about making
> quick-reject optional for a while. However, I'd like to evaluate and measure
it
> on its own, and not just in conjunction w/ the other proposed CL.

Not sure that is such a good idea. Without the other CL, quickReject is not 100%
redundant when playing back to a clipped destination canvas.  For non-tiled
playback however, it would be fine.
> 
> 1. Not sure I agree that we should complicate quickReject(rect) by adding a
> paint parameter. This makes that code now perform at least one extra test and
a
> rect-copy before landing where the old code was (i.e. isFinite). What
motivated
> this change? 
I wanted to skip the call to paint->computeFastBounds without adding complexity
to the call site.
>Could it make sense to just add a new entry-point that took a const
> SkPaint&? That version could call into the current one, and would not need to
> check for null.
Yeah that would work.
> 
> 2. What is the perf cost (if any) for checking this new bool? Can we try just
> making that change, and then timing a bunch of calls to quickReject? Hopefully
> its very small, but we need to know if we are adding a cost in the case where
we
> *do* perform the reject test.
Already did. With bench_pictures --mode record.  The recording canvas calls
quickReject in SkBBoxRecord::transformBounds:  No measurable impact.

> 
> 3. Nit: I think I'd rather have the API and field not have a negative in its
> name. Can we call it enableQuickReject or something like that? Not critical,
but
> I always have to think harder when I read predicates that are sort of
negative.
sgtm