We need to adjust the bounds of clip ops with SaveLayer paints too.

We need to adjust the bounds of clip ops with SaveLayer paints too.

Before this CL, SkRecord only adjusted the bounds of draw ops for SaveLayers' paints.
That worked fine, but as a final step we intersect the bounds of draw ops with the
bounds of the current clip, essentially undoing all that work.

I think the right fix here is to also adjust the bounds of the clip ops.

BUG=skia:2957, 415468
R=robertphillips@google.com

Committed: https://skia.googlesource.com/skia/+/271a030f5d0d3c59715fbeffb31c761279f3f8ca

[mtklein, 2014-09-23 15:17:25]

mtklein@google.com changed reviewers:
+ mtklein@google.com, robertphillips@google.com, senorblanco@chromium.org

[mtklein, 2014-09-23 15:17:29]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-09-23 15:17:59]

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

[commit-bot: I haz the power, 2014-09-23 15:17:59]

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-09-23 21:17 UTC

[Stephen White, 2014-09-23 16:03:51]

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp
File gm/imageblurtiled2.cpp (right):

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp#n...
gm/imageblurtiled2.cpp:22: virtual SkString onShortName() { return
SkString("imageblurtiled2");  }
Nit: this GM should probably be renamed dropshadowtiled (or
dropshadowimagefiltertiled, but maybe that's redundant), since it's not a blur.

Also, we should consider making this a unit test instead.

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

https://codereview.chromium.org/595953002/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:201: fCurrentClipBounds =
this->adjustForSaveLayerPaints(&clip) ? clip : Bounds::MakeLargest();
This seems reasonable, but I'm wondering why this wasn't necessary in the old
(non-SkRecord) code? Or was it there and I missed it?

[robertphillips, 2014-09-23 16:36:39]

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp
File gm/imageblurtiled2.cpp (right):

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp#n...
gm/imageblurtiled2.cpp:20: // This GM is a regression test for
http://skbug.com/2957 and http://crbug.com/415468.
// It draws a clipped rect in a layer which, in turn, has an exceedingly offset
drop shadow.
// This exercises the bounding box bloating effects of a saveLayer's paint in
the face of a clip.
?

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp#n...
gm/imageblurtiled2.cpp:21: struct ImageBlurTiled2GM : public skiagm::GM {
SK_OVERRIDE on these two ?

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp#n...
gm/imageblurtiled2.cpp:26: 
Just make this "const SkPicture* create_picture();" ?

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp#n...
gm/imageblurtiled2.cpp:40: 
SK_OVERRIDE ?

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp#n...
gm/imageblurtiled2.cpp:52: for (SkScalar y = bounds.top() ; y < bounds.bottom();
y += TILE) {
indent here?

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp#n...
gm/imageblurtiled2.cpp:60: }
    private:
        typedef GM INHERITED;
?

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp#n...
gm/imageblurtiled2.cpp:61: };
virtual uint32_t onGetFlags() const SK_OVERRIDE { return kSkipTiled_Flag; }

since it tiles internally?

[mtklein, 2014-09-23 17:43:13]

PTAL

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp
File gm/imageblurtiled2.cpp (right):

https://codereview.chromium.org/595953002/diff/20001/gm/imageblurtiled2.cpp#n...
gm/imageblurtiled2.cpp:22: virtual SkString onShortName() { return
SkString("imageblurtiled2");  }
> Also, we should consider making this a unit test instead.

Very good idea.  Done.

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

https://codereview.chromium.org/595953002/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:201: fCurrentClipBounds =
this->adjustForSaveLayerPaints(&clip) ? clip : Bounds::MakeLargest();
On 2014/09/23 16:03:51, Stephen White wrote:
> This seems reasonable, but I'm wondering why this wasn't necessary in the old
> (non-SkRecord) code? Or was it there and I missed it?

I'm trying to piece that together myself.  Between SkBBoxRecord,
SkBBoxHierarchyRecord, and SkPictureStateTree, it's _rather_ hard to follow. 
Will update you guys when I've got it sorted out.

[mtklein, 2014-09-23 18:02:09]

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

https://codereview.chromium.org/595953002/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:201: fCurrentClipBounds =
this->adjustForSaveLayerPaints(&clip) ? clip : Bounds::MakeLargest();
On 2014/09/23 17:43:13, mtklein wrote:
> On 2014/09/23 16:03:51, Stephen White wrote:
> > This seems reasonable, but I'm wondering why this wasn't necessary in the
old
> > (non-SkRecord) code? Or was it there and I missed it?
> 
> I'm trying to piece that together myself.  Between SkBBoxRecord,
> SkBBoxHierarchyRecord, and SkPictureStateTree, it's _rather_ hard to follow. 
> Will update you guys when I've got it sorted out.

As far as I can tell, the old implementation doesn't do the final intersection
of the bounds with the current clip.  This bug stems from doing that
incorrectly; if you don't do it you'll end up with fine (if perhaps large)
bounds.

[mtklein, 2014-09-23 18:02:18]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-09-23 18:02:54]

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

[commit-bot: I haz the power, 2014-09-23 18:02:55]

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-09-24 00:02 UTC

[Stephen White, 2014-09-23 18:03:18]

On 2014/09/23 18:02:09, mtklein wrote:
> https://codereview.chromium.org/595953002/diff/20001/src/core/SkRecordDraw.cpp
> File src/core/SkRecordDraw.cpp (right):
> 
>
https://codereview.chromium.org/595953002/diff/20001/src/core/SkRecordDraw.cp...
> src/core/SkRecordDraw.cpp:201: fCurrentClipBounds =
> this->adjustForSaveLayerPaints(&clip) ? clip : Bounds::MakeLargest();
> On 2014/09/23 17:43:13, mtklein wrote:
> > On 2014/09/23 16:03:51, Stephen White wrote:
> > > This seems reasonable, but I'm wondering why this wasn't necessary in the
> old
> > > (non-SkRecord) code? Or was it there and I missed it?
> > 
> > I'm trying to piece that together myself.  Between SkBBoxRecord,
> > SkBBoxHierarchyRecord, and SkPictureStateTree, it's _rather_ hard to follow.

> > Will update you guys when I've got it sorted out.
> 
> As far as I can tell, the old implementation doesn't do the final intersection
> of the bounds with the current clip.  This bug stems from doing that
> incorrectly; if you don't do it you'll end up with fine (if perhaps large)
> bounds.

Makes sense. Thanks for digging into it!

This looks good to me, but I'll leave for Rob.

[robertphillips, 2014-09-23 18:56:35]

lgtm + some comment suggestions

https://codereview.chromium.org/595953002/diff/60001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/595953002/diff/60001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:243: 
lower -> less ?

https://codereview.chromium.org/595953002/diff/60001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:244: // Under the original bug, all the right edge
values would be 20 lower than asserted here
intersect -> intersected ?
that's -> that had ?

[mtklein, 2014-09-23 19:03:18]

The CQ bit was checked by mtklein@google.com

[mtklein, 2014-09-23 19:03:22]

On 2014/09/23 18:56:35, robertphillips wrote:
> lgtm + some comment suggestions
> 
> https://codereview.chromium.org/595953002/diff/60001/tests/RecordDrawTest.cpp
> File tests/RecordDrawTest.cpp (right):
> 
>
https://codereview.chromium.org/595953002/diff/60001/tests/RecordDrawTest.cpp...
> tests/RecordDrawTest.cpp:243: 
> lower -> less ?
> 
>
https://codereview.chromium.org/595953002/diff/60001/tests/RecordDrawTest.cpp...
> tests/RecordDrawTest.cpp:244: // Under the original bug, all the right edge
> values would be 20 lower than asserted here
> intersect -> intersected ?
> that's -> that had ?

done

[mtklein_C, 2014-09-23 19:28:43]

Committed patchset #5 (id:80001) manually as 271a030 (presubmit successful).