Shrink saveLayer device bounds when it supplies an explicit bounds and has a complex paint

Shrink saveLayer device bounds when it supplies an explicit bounds and has a complex paint

This CL shrinks the bound computed for saveLayers that possess both an explicit
bound and a complex paint (e.g., one that affects transparent black). In this
case the bound of the layer should be the clipped explicit bound rather then
the clip prior/after the saveLayer/restore block.

In the following the first bound is the currently computed bound 
while the second is the new/desired one:

For a 100x100 picture

saveLayer (no bound, no paint) [ 0 0 100 100 ]  [ 50 50 100 100 ]
  setMatrix (translate 50, 50) [ 0 0 100 100 ] [ 50 50 100 100 ]
  saveLayer (bound of 0, 0, 50, 50 - complex paint) [ 0 0 100 100 ] [ 50 50 100 100 ]
  restore [ 0 0 100 100 ] [ 50 50 100 100 ]
restore [ 0 0 100 100 ] [ 50 50 100 100 ]

Committed: https://skia.googlesource.com/skia/+/4d52afef5cf90a2fed3bb69db71675c6450ab397

[robertphillips, 2014-10-31 17:55:18]

robertphillips@google.com changed reviewers:
+ mtklein@google.com, reed@google.com

[mtklein, 2014-10-31 18:21:01]

Can you add your example as a new unit test?

Is this a perf-motivated change or a correctness-motivated one?  Seems like
correctness from the code, but perf from the example?

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

https://codereview.chromium.org/696763002/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:124: static const SkRect kUnbounded = { -2e9f, -2e9f,
2e9f, 2e9f };
Should be we can do a big find kUnbounded / replace cullRect now?

https://codereview.chromium.org/696763002/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:152: fCurrentClipBounds = cullRect;
This seems an orthogonal change.  Might stick to kUnbounded for now and we
exploit the cull rect separately?  If we're going to use it, we might as well
use it everywhere we use kUnbounded.

I'm not sure we want to use the cull like this until we have an API that lets
the users pass it as a full rect instead of a width/height.  We're disabling the
(perhaps unintentional) feature of being able to draw in negative space if we
do.

[robertphillips, 2014-10-31 18:49:42]

> Can you add your example as a new unit test?

This came up because I'm setting up to layer GrPictureUtils::CollectLayers on
top of 
FillBounds (since ~95% of their code is shared). When setting up for that 
(Upgrade GPUOptimize - https://codereview.chromium.org/685263004/) the 
test_gpu_picture_optimization (in PictureTest.cpp) began failing. The 
"Upgrade GPUOptimize" patch has a hacky work around for the problem which 
this patch attempts to fix. So ... test_gpu_picture_optimization is the unit
test
(or will be for real once the layering is complete).

> Is this a perf-motivated change or a correctness-motivated one?

It is sort of both. Having the smaller bound yields less rendering in the
hoisting pass.

> This seems an orthogonal change.  Might stick to kUnbounded for now and we
> exploit the cull rect separately?  If we're going to use it, we might as well
> use it everywhere we use kUnbounded.

I had initially intended only to alter the processing in pushSaveBlock and
popSaveBlock. Doing so, however, resulted in unclipped saveLayers getting a
final device space bound of kUnbounded. While correct in some ways that bound
wasn't particularly useful in others. I guess the fundamental question is
should anything in a picture ever have a bounding box larger then the cull rect.

> I'm not sure we want to use the cull like this until we have an API that lets
> the users pass it as a full rect instead of a width/height.  We're disabling
the
> (perhaps unintentional) feature of being able to draw in negative space if we
> do.

It is problematic however to count on any drawing outside of the cull rect.
Even though negative space could be used now it seems like it could run into
trouble with
compositing, BBHs and subpictures.

[robertphillips, 2014-10-31 18:49:55]

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

https://codereview.chromium.org/696763002/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:124: static const SkRect kUnbounded = { -2e9f, -2e9f,
2e9f, 2e9f };
On 2014/10/31 18:21:01, mtklein wrote:
> Should be we can do a big find kUnbounded / replace cullRect now?

Done.

[mtklein, 2014-11-03 14:59:31]

lgtm, just some nits

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

https://codereview.chromium.org/696763002/diff/40001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:253: // then the current clip bounds.
than, stray tab

https://codereview.chromium.org/696763002/diff/40001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:538: SkRect fCullRect;
const this so we don't accidentally change it?

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

https://codereview.chromium.org/696763002/diff/40001/src/core/SkRecordDraw.h#...
src/core/SkRecordDraw.h:18: void SkRecordFillBounds(const SkRect&, const
SkRecord&, SkBBoxHierarchy*);
Might give cullRect a name here too.  The record and bbh seem obvious without
names, but it's not immediately clear what rectangle is the rectangle to pass
here.

https://codereview.chromium.org/696763002/diff/40001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/696763002/diff/40001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:279: REPORTER_ASSERT(r, bbh.fEntries.count() == 3);
Thanks!

[robertphillips, 2014-11-03 16:11:18]

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

https://codereview.chromium.org/696763002/diff/40001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:253: // then the current clip bounds.
On 2014/11/03 14:59:31, mtklein wrote:
> than, stray tab

Done.

https://codereview.chromium.org/696763002/diff/40001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:538: SkRect fCullRect;
On 2014/11/03 14:59:31, mtklein wrote:
> const this so we don't accidentally change it?

Done.

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

https://codereview.chromium.org/696763002/diff/40001/src/core/SkRecordDraw.h#...
src/core/SkRecordDraw.h:18: void SkRecordFillBounds(const SkRect&, const
SkRecord&, SkBBoxHierarchy*);
On 2014/11/03 14:59:31, mtklein wrote:
> Might give cullRect a name here too.  The record and bbh seem obvious without
> names, but it's not immediately clear what rectangle is the rectangle to pass
> here.

Done.

[robertphillips, 2014-11-03 16:11:23]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-11-03 16:12:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/696763002/60001

[commit-bot: I haz the power, 2014-11-03 16:19:49]

Committed patchset #4 (id:60001) as 4d52afef5cf90a2fed3bb69db71675c6450ab397