Add SkPictureRecord subclass that computes bounding boxes.

Add SkPictureRecord subclass that computes bounding boxes.
Committed: https://code.google.com/p/skia/source/detail?r=5423

[rileya, 2012-09-06 14:16:45]

http://codereview.appspot.com/6506082/diff/1/src/core/SkBBoxRecord.cpp
File src/core/SkBBoxRecord.cpp (right):

http://codereview.appspot.com/6506082/diff/1/src/core/SkBBoxRecord.cpp#newcod...
src/core/SkBBoxRecord.cpp:220: // well, that's no good...
I'm not totally sure how best to handle this case, setting the bounds to fill
the current clip might work as a super-cautious measure; that'd probably hurt
the R-Tree, but if it's reasonably rare that bounds can't be computed (I've yet
to see a case of this in a captured webpage), that may be the way to go.

http://codereview.appspot.com/6506082/diff/1/src/core/SkBBoxRecord.cpp#newcod...
src/core/SkBBoxRecord.cpp:230: // It could be that we got clipped out entirely,
if so we can just do nothing
Would it be safe to completely reject a draw from the picture based on this?
(maybe have this return a bool, and if false don't call up to the inherited
PictureRecord draw calls).

[bsalomon, 2012-09-06 14:27:51]

https://codereview.appspot.com/6506082/diff/1/src/core/SkBBoxRecord.cpp
File src/core/SkBBoxRecord.cpp (right):

https://codereview.appspot.com/6506082/diff/1/src/core/SkBBoxRecord.cpp#newco...
src/core/SkBBoxRecord.cpp:220: // well, that's no good...
On 2012/09/06 14:16:45, rileya wrote:
> I'm not totally sure how best to handle this case, setting the bounds to fill
> the current clip might work as a super-cautious measure; that'd probably hurt
> the R-Tree, but if it's reasonably rare that bounds can't be computed (I've
yet
> to see a case of this in a captured webpage), that may be the way to go.

Is it better/worse for the tree or picture to store a separate list of
unboundable ops that must be played back regardless of the tile bounds?

https://codereview.appspot.com/6506082/diff/1/src/core/SkBBoxRecord.cpp#newco...
src/core/SkBBoxRecord.cpp:230: // It could be that we got clipped out entirely,
if so we can just do nothing
On 2012/09/06 14:16:45, rileya wrote:
> Would it be safe to completely reject a draw from the picture based on this?
> (maybe have this return a bool, and if false don't call up to the inherited
> PictureRecord draw calls).

Makes sense to me.

[rileya, 2012-09-06 14:47:36]

On 2012/09/06 14:27:51, bsalomon wrote:
> https://codereview.appspot.com/6506082/diff/1/src/core/SkBBoxRecord.cpp
> File src/core/SkBBoxRecord.cpp (right):
> 
>
https://codereview.appspot.com/6506082/diff/1/src/core/SkBBoxRecord.cpp#newco...
> src/core/SkBBoxRecord.cpp:220: // well, that's no good...
> On 2012/09/06 14:16:45, rileya wrote:
> > I'm not totally sure how best to handle this case, setting the bounds to
fill
> > the current clip might work as a super-cautious measure; that'd probably
hurt
> > the R-Tree, but if it's reasonably rare that bounds can't be computed (I've
> yet
> > to see a case of this in a captured webpage), that may be the way to go.
> 
> Is it better/worse for the tree or picture to store a separate list of
> unboundable ops that must be played back regardless of the tile bounds?

That was what I was considering as an alternative, it may also be useful if we
could set a size threshold of some sort and put really huge draws (like the
initial drawRect that webkit clears the screen with) in that list too, rather
than potentially hurting the tree structure.

Although, if the unboundable draws are still guaranteed to obey the clip (are
there effects that do not guarantee this?), then it may not do too much harm to
just expand them out to the extents of the current clip.

[reed1, 2012-09-06 15:22:01]

regarding earlier thread: all draw...() calls respect the clip, so using the
clip bounds as a conservative bounds should be correct.

https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.cpp
File src/core/SkBBoxRecord.cpp (right):

https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.cpp#ne...
src/core/SkBBoxRecord.cpp:241: if (this->getClipBounds(&clip)) {
intersect() returns false if the result *would* be empty, but does not actually
set the rectangle to empty. You may need to check here and return false.

https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.h
File src/core/SkBBoxRecord.h (right):

https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.h#newc...
src/core/SkBBoxRecord.h:29: virtual void handleBounds(const SkRect& aabb) = 0;
handleBBox()?

[bsalomon, 2012-09-06 15:25:43]

On 2012/09/06 15:22:01, reed1 wrote:
> regarding earlier thread: all draw...() calls respect the clip, so using the
> clip bounds as a conservative bounds should be correct.
> 

So maybe we should forward them on. The tree builder can decided whether really
large rects relative to the recording canvas size are bad for tree construction
(and should be tracked separately) or not.

> https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.cpp
> File src/core/SkBBoxRecord.cpp (right):
> 
>
https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.cpp#ne...
> src/core/SkBBoxRecord.cpp:241: if (this->getClipBounds(&clip)) {
> intersect() returns false if the result *would* be empty, but does not
actually
> set the rectangle to empty. You may need to check here and return false.
> 
> https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.h
> File src/core/SkBBoxRecord.h (right):
> 
>
https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.h#newc...
> src/core/SkBBoxRecord.h:29: virtual void handleBounds(const SkRect& aabb) = 0;
> handleBBox()?

[rileya, 2012-09-06 15:54:00]

On 2012/09/06 15:25:43, bsalomon wrote:
> On 2012/09/06 15:22:01, reed1 wrote:
> > regarding earlier thread: all draw...() calls respect the clip, so using the
> > clip bounds as a conservative bounds should be correct.
> > 
> 
> So maybe we should forward them on. The tree builder can decided whether
really
> large rects relative to the recording canvas size are bad for tree
construction
> (and should be tracked separately) or not.

I think that sounds good. I'll probably need to add an optional parameter to the
RTree that gives it the area that the boxes will all exist in (in this case, the
record canvas dimensions), so it knows the total area/aspect ratio/etc up front
(in the bulk load case I suppose it could figure that out itself by intersecting
all the boxes it gets together, but that'd slow down tree creation).

https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.cpp
File src/core/SkBBoxRecord.cpp (right):

https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.cpp#ne...
src/core/SkBBoxRecord.cpp:241: if (this->getClipBounds(&clip)) {
On 2012/09/06 15:22:02, reed1 wrote:
> intersect() returns false if the result *would* be empty, but does not
actually
> set the rectangle to empty. You may need to check here and return false.

Ah, wasn't aware of that, fixed and cleaned up this logic a bit.

https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.h
File src/core/SkBBoxRecord.h (right):

https://codereview.appspot.com/6506082/diff/6001/src/core/SkBBoxRecord.h#newc...
src/core/SkBBoxRecord.h:29: virtual void handleBounds(const SkRect& aabb) = 0;
On 2012/09/06 15:22:02, reed1 wrote:
> handleBBox()?

Right, that's probably more consistent, changed.

[rileya, 2012-09-06 20:43:52]

Anyone have any more thoughts on this? I've got the next CL queued up and I'm
sorta blocked on this.

[bsalomon, 2012-09-06 20:45:26]

On 2012/09/06 20:43:52, rileya wrote:
> Anyone have any more thoughts on this? I've got the next CL queued up and I'm
> sorta blocked on this.

LGTM