Adding option in SkPicture to record device-space bounds of draw commands.

Adding build option in SkPicture to record device-space bounds of draw commands.

[Justin Novosad, 2013-03-06 22:20:11]

This is a first increment in getting SkPicture to use pre-computed bounds.  It
is not yet ready to be turned on in tests.

[robertphillips, 2013-03-06 23:02:02]

initial comments

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

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPicture.cpp#new...
src/core/SkPicture.cpp:322: }
NULL !=

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:447: !remove_save_layer1(&fWriter,
fRestoreOffsetStack.top(), &fPaints,
SkToBool?

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:471: if
(!this->getClipDeviceBounds(&devClipBounds)) {
Comment here about this optimization and why it is valid with rescaling?

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:752: initialOffset =
this->addDrawWithBounds(DRAW_POINTS, &size, &paint, &bbox);
Could fLastDrawRejected be passed in as a parameter instead? It seems like that
might be clearer.

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:758: }
this->recordBounds()? Also in following changes too?

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:812: if (recordBounds()) {
What do you think about modifying addDraw to always take a bounds parameter so
we don't have the addDraw/addDrawWithBounds bifurcation? Boundless ops could
pass in NULL. The "if (recordBounds())" logic could then be handled inside
addDraw.

We would also have addDraw always return if the op was rejected.

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:982: SkScalar y, const SkPaint& paint) {
// We disallow DRAW_TEXT_TOP_BOTTOM when saving bounds b.c. ...

[Justin Novosad, 2013-03-07 14:41:42]

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:752: initialOffset =
this->addDrawWithBounds(DRAW_POINTS, &size, &paint, &bbox);
On 2013/03/06 23:02:02, robertphillips wrote:
> Could fLastDrawRejected be passed in as a parameter instead? It seems like
that
> might be clearer.

It could, but I need to have as a member anyways because BBoxHierarchy code will
have to be able to query this after the fact (in an upcoming CL).  Another
solution I thought of (heavy handed approach) would be to add a bool return val
to all canvas draw calls, and make them return false when we know nothing was
drawn.

https://codereview.chromium.org/12545009/diff/1007/src/core/SkPictureRecord.c...
src/core/SkPictureRecord.cpp:812: if (recordBounds()) {
On 2013/03/06 23:02:02, robertphillips wrote:
> What do you think about modifying addDraw to always take a bounds parameter so
> we don't have the addDraw/addDrawWithBounds bifurcation? Boundless ops could
> pass in NULL. The "if (recordBounds())" logic could then be handled inside
> addDraw.
> 
> We would also have addDraw always return if the op was rejected.

Well, I also want to avoid computing the bounds that are passed to
addDrawWithBounds because it is not always a trivial computation.  Half the call
sites would have to end up looking something like this:
SkRect* bounds = NULL;
SkRect boundsStorage;
if (this->recordBounds()) {
   //Compute bounds in boundsStorage
   (...)
   bounds = &boundsStorage;
}
addDraw(..., bounds);

Maybe that's not so bad... not sure which I dislike more.

[Justin Novosad, 2013-03-07 14:44:06]

Patch Set 3: addresses the simple review comments.
Working on the rest...

[Justin Novosad, 2013-03-07 16:43:02]

Patch!
* Got Rid of fLastDrawRejected.  Can put it back later if we absolutely need it
(we'll see)
* There is now just one addDraw method, with optional bounds and paint
arguments.

[robertphillips, 2013-03-07 18:37:40]

I think it's looking good - Mike, any comments?

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPicturePlaybac...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:699: if (!initialClipBoundsSet) {
Would it make sense to have a "bool get_initial_clip_bounds(SkCanvas*,
SkIRect*)" helper to share code between here and above (and hide the
'clipBounds' temp)?

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:702: clipBounds.roundOut(&initialClipBounds);
initialClipBoundsSet = true;?

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:742: }
add else here?

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:746: // local clip bounds at start of playback.
SkASSERT(initialClipBoundsSet);?

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:64: 
Do you want to also make this one static inline?

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:500: }
mv clippedDevBounds down to here?

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:978: uint32_t initialOffset;
Could you expand on this with a "because ..."

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:979: // Note: Bounds encoding not supported.
Move uint32_t down to line 980?

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1021: bool fast = !paint.isVerticalText() &&
paint.canComputeFastBounds() &&
spaces

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1119: bool fastBounds = !paint.isVerticalText() &&
paint.canComputeFastBounds() &&
spaces

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1208: bool fast = !paint.isVerticalText() &&
paint.canComputeFastBounds() &&
spaces

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1221: size_t numChars = paint.countText(text,
byteLength);
We should probably bail somewhere (or add an assert) if numChar == 0

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1223: bbox.fLeft  = xpos[0];
shouldn't this be 0 rather than numChars-1?

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.h:118: /*
This comment needs enhancing

[robertphillips, 2013-03-07 19:51:17]

In a separate CL we should also add the new bounds to the debugger's details
pane.

[Justin Novosad, 2013-03-07 20:25:47]

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:65: bool SkPictureRecord::canRecordBounds(DrawType
op) {
It needs to be accessible from SkPictureBitmap.

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1221: size_t numChars = paint.countText(text,
byteLength);
On 2013/03/07 18:37:40, robertphillips wrote:
> We should probably bail somewhere (or add an assert) if numChar == 0
Actually, I just realized this is redundant with 'points', computed above.

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1223: bbox.fLeft  = xpos[0];
On 2013/03/07 18:37:40, robertphillips wrote:
> shouldn't this be 0 rather than numChars-1?
For the algorithm to work, it actually does not matter.  Initializing like this
is more likely to be optimal for perf (assuming we usually write from left to
right) because we will never update bbox in the loop below in the well
conditioned cases.

[Justin Novosad, 2013-03-07 20:26:06]

Patch.

[reed1, 2013-03-07 20:36:44]

There is a fair amount of plumbing here to make it a runtime option. Is that for
testing purposes only, or do you think this should be optional?

What is the perf-hit
1. before and after this CL with the feature OFF
2. with the feature ON and OFF

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPicturePlaybac...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:761: // iterator until at or after skipTo
do {
   adjusted = it.draw()
} while (adjusted < skipTo);

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:64: 
Maybe its time to create a static uint8_t array that holds a bitfield of
attributes for each op-code, so we don't have to executed any switch-statements
when trying to classify an draw-op.

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1062: // Pad horizontal bounds on each side by half
of max vertical extents (this is sort of
80-cols in this file

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1065: // incorrect on most platforms (too small in
Linux, never even set in Windows).
yikes! please file a bug!

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1234: SkPaint::FontMetrics metrics;
Some amount of this code looks awefully similar to drawPosText and drawText. Can
we do refactoring to share some?

[robertphillips, 2013-03-07 20:57:37]

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1223: bbox.fLeft  = xpos[0];
Since we have no guarantee of monotonicity, what if xpos[0] is the rightmost?

[Justin Novosad, 2013-03-07 21:08:51]

On 2013/03/07 20:36:44, reed1 wrote:
> There is a fair amount of plumbing here to make it a runtime option. Is that
for
> testing purposes only, or do you think this should be optional?

It has to be optional at least until I can get it to work with TileGrid+RTree. 
We are still a few CLs away from that.  Even after that, it should probably
remain optional because it adds record-time overhead (computeing bounds).  Once
it is fully operational, I think it should probably be folded into the
OptimizeForClippedPlayback recording flag.
> 
> What is the perf-hit
> 1. before and after this CL with the feature OFF
> 2. with the feature ON and OFF
Excellent question. Let me make a spreadsheet...

[Justin Novosad, 2013-03-07 21:48:31]

Good catch. I guess we just have to start that loop at zero.

[Justin Novosad, 2013-03-08 14:49:13]

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/16001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1065: // incorrect on most platforms (too small in
Linux, never even set in Windows).
On 2013/03/07 20:36:44, reed1 wrote:
> yikes! please file a bug!

This was copy-pasted from SkBBoxRecord (r5423 by rileya).  Not sure what the
history behind this comment is. I feel this should be made more conservative due
to its fuzzy nature. I think I should remove the divide by two.

[Justin Novosad, 2013-03-08 15:13:41]

The perf win from this change is still not measurable because I still have to
work on a companion change that disables quickReject testing on the playback
canvas when this option is used.  Experiments I performed last month with a
cruder implementation showed a potential to improve  skp tiled playback perf by
10-20%.  For now I will make sure this CL does not regress perf for the the case
where kRecordBounds is not used. OK?

[reed1, 2013-03-08 15:45:07]

What if we just create a temp build-flag for this, rather than all of the
plumbing (and possible public exposure) while you're
testing/timing/experimenting, esp. since you think this functionality will get
rolled into an existing flag anyway?

Modulo the above, yes, landing this as a stage while other elements are worked
on seems fine.

[Justin Novosad, 2013-03-08 18:08:26]

Patch.

Made the option a build-time option. 

Verified that there is no measurable perf impact (bench_pictures record &
playback).

Made changes relative to all above review comments.

[Justin Novosad, 2013-03-08 18:12:47]

FYI: a lot of the plumbing related to turning recordBounds on/off at run time is
still there, but it gets optimized-out in release builds (when the compile
option is resolved).  I think it is useful to keep that plumbing around until we
decide whether we want record bounds to be turned on all the time.  I still
think we may end up tying it to kOptimizeForClippedPlayback.

[reed1, 2013-03-08 18:24:42]

ah, I see what you did. I was hoping for the opposite of adding yet more
plumbing, but using the build-flag just on the impl-side to reduce the size of
the CL, and keep the feature invisible from the outside for now.

[Justin Novosad, 2013-03-08 20:02:12]

Patch.

[reed1, 2013-03-08 20:23:23]

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPicturePlaybac...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:130: fHasRecordedBounds = true;
do we want false here?

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:596: fHasRecordedBounds = info.fFlags &
SkPictInfo::kHasRecordedBounds_Flag;
SkToBool(...)

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:660: static inline bool
get_intial_clip_bounds(const SkCanvas& canvas, SkIRect* bounds, bool* isSet) {
Maybe this is the best way, but this treats both isSet and bounds as
input/output params, which is unexpected in general. Of note, we call this
sometimes with an uninitialized bounds, and just happen to know that we are
passing the right value in isSet so that bounds won't be read-from. Is there a
cleaner way to organize this? Perhaps diff variants of this that don't overload
the params so much?

getClipBounds returns its answer in local coordinates, yet you are rounding it
to IRect (which we typically use for device-bounds).

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:71: static const uint8_t
gDrawTypeInfo[LAST_DRAWTYPE_ENUM + 1] = {
SWEET!!!!!!!

Wonder if the var name should more closely match the enum name...
gDrawTypeFlags?

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:520: if (0 != (*size & ~MASK_24) || *size ==
MASK_24) {
Yikes, this looks tricky. Do we need already have some macro/helper that
computes this stuff? Rob?

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:854: const SkRect* bounds =
path.isInverseFillType() ? NULL : &path.getBounds();
should we have SK_RECORD_BOUNDS_IN_PICTURE check here, and the other places that
are now passing a bounds to addDraw?

[robertphillips, 2013-03-08 20:40:04]

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPicturePlaybac...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:129: #else
false? or leave off #else since it is initialized to false?

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:659: 
//comment? esp. w.r.t. return value

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:698: if (NULL != fStateTree && NULL !=
fBoundingHierarchy) {
Ignore this if you like but I would test on !get_initial_clip_bounds and then
return making the stuff currently inside the if un-indented.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:64: 
comment - esp w.r.t. modifies state

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:119: 
modifies_state

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:131: 
op and 24bit size

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:153: 
overlength line

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:154: SkASSERT(kHasPaint_DrawTypeFlag &
gDrawTypeInfo[op]);   // really shouldn't be calling this method
What's up with this comment

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:943: uint32_t size = 5 * kUInt32Size;
Bacause -> Because

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:946: // canvas's initial tranform matrix at
playback time, and recorded bounds
initial

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1097: // already recording full bounds.
How about just setting these to false for RECORD_BOUND mode? It won't be as fast
but there will be less #if cruft. As is it is tough to tell what is going on.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1195: 
the "op + ..." comment should move up here too

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1213: bbox.fLeft  = xpos[0];
fix this?

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.h:125: * size needs its own storage location (i.e., it
overflows 24 bits).
Ignore if you want, I would have the two return value sentences after the bounds
sentences.

[Justin Novosad, 2013-03-08 22:35:42]

Patch!

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPicturePlaybac...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:130: fHasRecordedBounds = true;
On 2013/03/08 20:23:23, reed1 wrote:
> do we want false here?

Done.

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:596: fHasRecordedBounds = info.fFlags &
SkPictInfo::kHasRecordedBounds_Flag;
On 2013/03/08 20:23:23, reed1 wrote:
> SkToBool(...)

Done.

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:660: static inline bool
get_intial_clip_bounds(const SkCanvas& canvas, SkIRect* bounds, bool* isSet) {
On 2013/03/08 20:23:23, reed1 wrote:
> Maybe this is the best way, but this treats both isSet and bounds as
> input/output params, which is unexpected in general. Of note, we call this
> sometimes with an uninitialized bounds, and just happen to know that we are
> passing the right value in isSet so that bounds won't be read-from. Is there a
> cleaner way to organize this? Perhaps diff variants of this that don't
overload
> the params so much?
> 
> getClipBounds returns its answer in local coordinates, yet you are rounding it
> to IRect (which we typically use for device-bounds).

Done.

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:71: static const uint8_t
gDrawTypeInfo[LAST_DRAWTYPE_ENUM + 1] = {
On 2013/03/08 20:23:23, reed1 wrote:
> SWEET!!!!!!!
> 
> Wonder if the var name should more closely match the enum name...
> gDrawTypeFlags?

Done.

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:520: if (0 != (*size & ~MASK_24) || *size ==
MASK_24) {
On 2013/03/08 20:23:23, reed1 wrote:
> Yikes, this looks tricky. Do we need already have some macro/helper that
> computes this stuff? Rob?
I did not write this. It was copy/pasted from the old addDraw implementation
(used to be in the header)

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:854: const SkRect* bounds =
path.isInverseFillType() ? NULL : &path.getBounds();
On 2013/03/08 20:23:23, reed1 wrote:
> should we have SK_RECORD_BOUNDS_IN_PICTURE check here, and the other places
that
> are now passing a bounds to addDraw?

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPicturePlaybac...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:129: #else
On 2013/03/08 20:40:04, robertphillips wrote:
> false? or leave off #else since it is initialized to false?

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:659: 
On 2013/03/08 20:40:04, robertphillips wrote:
> //comment? esp. w.r.t. return value

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPicturePlaybac...
src/core/SkPicturePlayback.cpp:698: if (NULL != fStateTree && NULL !=
fBoundingHierarchy) {
On 2013/03/08 20:40:04, robertphillips wrote:
> Ignore this if you like but I would test on !get_initial_clip_bounds and then
> return making the stuff currently inside the if un-indented.

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:64: 
On 2013/03/08 20:40:04, robertphillips wrote:
> comment - esp w.r.t. modifies state

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:119: 
On 2013/03/08 20:40:04, robertphillips wrote:
> modifies_state

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:131: 
On 2013/03/08 20:40:04, robertphillips wrote:
> op and 24bit size

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:153: 
On 2013/03/08 20:40:04, robertphillips wrote:
> overlength line

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:154: SkASSERT(kHasPaint_DrawTypeFlag &
gDrawTypeInfo[op]);   // really shouldn't be calling this method
On 2013/03/08 20:40:04, robertphillips wrote:
> What's up with this comment

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:943: uint32_t size = 5 * kUInt32Size;
On 2013/03/08 20:40:04, robertphillips wrote:
> Bacause -> Because

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:946: // canvas's initial tranform matrix at
playback time, and recorded bounds
On 2013/03/08 20:40:04, robertphillips wrote:
> initial

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1097: // already recording full bounds.
On 2013/03/08 20:40:04, robertphillips wrote:
> How about just setting these to false for RECORD_BOUND mode? It won't be as
fast
> but there will be less #if cruft. As is it is tough to tell what is going on.

I like it this way. The day I revisit to get rid of the ifdefs and delete the
legacy code, I won't have unused vars to worry about.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1195: 
On 2013/03/08 20:40:04, robertphillips wrote:
> the "op + ..." comment should move up here too

Done.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:1213: bbox.fLeft  = xpos[0];
On 2013/03/08 20:40:04, robertphillips wrote:
> fix this?
It was fixed. The loop below now starts at 0 instead of 1.

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.chromium.org/12545009/diff/60001/src/core/SkPictureRecord....
src/core/SkPictureRecord.h:125: * size needs its own storage location (i.e., it
overflows 24 bits).
On 2013/03/08 20:40:04, robertphillips wrote:
> Ignore if you want, I would have the two return value sentences after the
bounds
> sentences.

Done.

[robertphillips, 2013-03-11 17:31:39]

LGTM

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/48001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:520: if (0 != (*size & ~MASK_24) || *size ==
MASK_24) {
We don't currently have one like WILL_OVERFLOW_24BITS. I will add one in a
separate CL.

https://codereview.chromium.org/12545009/diff/65001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/65001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:516: 
left justify the # commands?

[Justin Novosad, 2013-03-11 17:52:10]

https://codereview.chromium.org/12545009/diff/65001/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/65001/src/core/SkPictureRecord....
src/core/SkPictureRecord.cpp:516: 
On 2013/03/11 17:31:39, robertphillips wrote:
> left justify the # commands?

Done.

[reed1, 2013-03-11 18:46:46]

Do you have tests to validate correctness/performance of this change, with
different initial matrices and clips? I ask because on the surface it appears
that when the initial clip is large (w.r.t. picture-size) that this will slow us
down, and that when there is a scale-down (or other) initial matrix, we might
reject objects that should be drawn.

On a different track, if the idea is to also disable canvas' quickReject, then
we seem to be in a questionable state where we traded away the ability to cull
objects as the result of local clip changes within the picture, since we would
*only* compare against the initial canvas-clip.

Perhaps we might discuss these question over VC some time.

[Justin Novosad, 2013-03-11 19:01:13]

On 2013/03/11 18:46:46, reed1 wrote:
> Do you have tests to validate correctness/performance of this change, with
> different initial matrices and clips?
I ran render_pictures with tile playback (without tile grid), and it seemed to
work fine.
> I ask because on the surface it appears
> that when the initial clip is large (w.r.t. picture-size) that this will slow
us
> down,
Compared to the current implementation that use quickReject, the only thing I
can think of that might slow us down is the full bounds computations on the
drawText* commands.  In a follow-up CL in intend to make recorded bounds
optional in order to optimize some special cases.

> and that when there is a scale-down (or other) initial matrix, we might
> reject objects that should be drawn.

That does not happen. By applying the initial matrix to the recorded bounds, we
bring the bounds into the playback canvas's device space, and the clip test is
done in that coordinate space against the playback canvas's device clip bounds.
> 
> On a different track, if the idea is to also disable canvas' quickReject, then
> we seem to be in a questionable state where we traded away the ability to cull
> objects as the result of local clip changes within the picture, since we would
> *only* compare against the initial canvas-clip.

Ahah! But the bounds that are stored in the stream are already intersected with
the local clip bounds (within the picture), and clipped-out draws are
pre-rejected at record time.

> 
> Perhaps we might discuss these question over VC some time.

[reed1, 2013-03-11 19:06:25]

On 2013/03/11 19:01:13, junov wrote:
> On 2013/03/11 18:46:46, reed1 wrote:
> > Do you have tests to validate correctness/performance of this change, with
> > different initial matrices and clips?
> I ran render_pictures with tile playback (without tile grid), and it seemed to
> work fine.
> > I ask because on the surface it appears
> > that when the initial clip is large (w.r.t. picture-size) that this will
slow
> us
> > down,
> Compared to the current implementation that use quickReject, the only thing I
> can think of that might slow us down is the full bounds computations on the
> drawText* commands.  In a follow-up CL in intend to make recorded bounds
> optional in order to optimize some special cases.
> 
> > and that when there is a scale-down (or other) initial matrix, we might
> > reject objects that should be drawn.
> 
> That does not happen. By applying the initial matrix to the recorded bounds,
we
> bring the bounds into the playback canvas's device space, and the clip test is
> done in that coordinate space against the playback canvas's device clip
bounds.

Where is that happening? I must have missed it.

> > 
> > On a different track, if the idea is to also disable canvas' quickReject,
then
> > we seem to be in a questionable state where we traded away the ability to
cull
> > objects as the result of local clip changes within the picture, since we
would
> > *only* compare against the initial canvas-clip.
> 
> Ahah! But the bounds that are stored in the stream are already intersected
with
> the local clip bounds (within the picture), and clipped-out draws are
> pre-rejected at record time.

Ah, that's sweet.

> 
> > 
> > Perhaps we might discuss these question over VC some time.

[Justin Novosad, 2013-03-11 19:23:16]

On 2013/03/11 19:06:25, reed1 wrote:
> 
> Where is that happening? I must have missed it.

Wohoh, looks like I thought it but did not code it!  Which means I did not do my
testing right.  I'm gonna go back and fail&fix...
Good catch.

[Justin Novosad, 2013-03-11 20:10:20]

After all, there is no bug. It does all work fine. I just forgot the details
over the weekend :-) The trick is that get_intial_clip_bounds gets the playback
canvas's clip bounds in local space, which takes into account initialMatrix.  So
when we do the intersection test, we are comparing recording canvas device
bounds to initialClipBounds of the playback canvas in it initial local
coordinate space, so everything matches.

I uploaded a new patch that adds a unit test that is useful for
validating/debugging this logic.

[reed1, 2013-03-12 16:05:04]

https://codereview.chromium.org/12545009/diff/29004/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/12545009/diff/29004/tests/PictureTest.cpp#new...
tests/PictureTest.cpp:423: static void
test_subregion_playback(skiatest::Reporter* reporter) {
Can you simplify or document the complexity in this test?
- why is the picture 6x6, the rect 4,4,1,1, the target 3x3, and the translate
-3,-3
- do you need the clear() in the picture? is that better than initializing your
target?


How does this test the new (or old) code? Can you comment on that in some way?

https://codereview.chromium.org/12545009/diff/29004/tests/PictureTest.cpp#new...
tests/PictureTest.cpp:435: SkDevice device(store);
minor, but can't you just pass store to the canvas' constructor, and not need to
include SkDevice.h? (as we're trying to make that less public)

[reed1, 2013-03-12 16:06:39]

One metric for "correctness" might be to (potentially ahead of this CL) augment
SkCanvas to optionally record/count its quickRejects. We could then in this CL
assert that all quickReject calls always fail (i.e. let the draw actually
happen).

[reed1, 2013-03-12 16:07:13]

... then we could run this CL through not only translates, but scales (and
other) as well with confidence.

[Justin Novosad, 2013-05-13 20:12:35]

I'm resurrecting this CL!

New Patch.  Addressed all previous comments and updated to ToT skia.
Includes a "correctness" check: will assert if playback triggers quickRejects.
In Release builds, quickReject gets skipped entirely since it is redundant.

[reed1, 2013-05-14 13:31:11]

May need a more detail description for the CL, detailing the changes/impact on
canvas, recording, playback, bboxhierarchy.

The use of int-rects instead of scalars feels funny. Perhaps we can discuss that
sometime as well.

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

https://codereview.chromium.org/12545009/diff/124002/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:1405: #ifdef SK_DEBUG
Hmmm, not sure we always want to assert this. Seems like it would be
valid/interesting to disable quick-reject even if we then draw something that
will eventually get clipped out.

[Tom Hudson, 2013-05-14 17:00:49]

https://codereview.chromium.org/12545009/diff/124002/src/core/SkBBoxRecord.cpp
File src/core/SkBBoxRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/124002/src/core/SkBBoxRecord.cp...
src/core/SkBBoxRecord.cpp:71: INHERITED::drawPoints(mode, count, pts, paint);
I like the way this file starts losing special cases, but are we not going to
have to reinstantiate them somewhere?

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPicturePlayba...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:605: // Can't handle skps with recorded bounds in
a build that does support them.
Typo: does *not* support them?

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:690: fCanvas = NULL;
Why not pass a canvas to the constructor, and avoid set()?

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:693: if (canvas->fQuickRejectEnabled == true) {
// protect against nested Disables

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:735: return;
You now have an early return where it looks like you used to fall through?

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:753: // because it is redundant with early bounds
check.
Nit: Second line of this comment is more useful than the first.

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPicturePlayba...
src/core/SkPicturePlayback.cpp:754: dqr.set(&canvas);
Ugh, given that you need this to be conditionally activated I suppose I see why
you created set() instead of passing the canvas to the constructor.

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPictureRecord...
File src/core/SkPictureRecord.cpp (right):

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPictureRecord...
src/core/SkPictureRecord.cpp:632: SkIRect clippedDevBounds;  // Bounds of the
clipped draw in device space
Nit: clippedDevBounds and devClipBounds are a bit too similar for my taste; do
we have better names?

(even as small a change as devDrawBounds and devClipBounds?)

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPictureRecord...
src/core/SkPictureRecord.cpp:634: SkASSERT(!modifies_state(drawType));  //
Ensure devClipBounds is initialized
Not obvious how the comment follows from the assert. Is it correct?
Nor is it obvious how the logic chain above guarantees the assert.

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPictureRecord...
src/core/SkPictureRecord.cpp:1000: if (kInvalidOffset == initialOffset) {
Why not asserts here as above?

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPictureRecord...
src/core/SkPictureRecord.cpp:1490: // if we had a guarantee that these will be
monotonically increasing, this could be sped up
Would this break on RTL? Do we have a guarantee that these are monotonic, either
increasing or decreasing? (IIRC Chrome does changes RTL<->LTR as distinct runs
of text from our point of view.)

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPictureRecord.h
File src/core/SkPictureRecord.h (right):

https://codereview.chromium.org/12545009/diff/124002/src/core/SkPictureRecord...
src/core/SkPictureRecord.h:132: * returns true;
Nit: trailing semicolon - did you mean to do more here?
Nit: this ordering of clauses to me reads as though bounds are recorded in
something *other than* device space if !canRecordBounds(drawType).

[tomhudson, 2014-02-26 10:36:01]

This is still open; from email I think it's been abandoned?