Fix for saveLayer() with filters vs. the BBox Hierarchy.

Fix for saveLayer() with filters vs. the BBox Hierarchy.

When building acceleration structures for SkPicture, we must transform
the primitive's bounds not only by its own SkPaint, but by the paints of
any saveLayer()s currently active above it.

We do this by pushing the SkPaint onto a stack on
saveLayer(), and popping them on restore(). We also push
a NULL paint in save(), so that the pushes and pops are
balanced.

BUG=skia:2734
R=reed@google.com,mtklein@google.com

Committed: https://skia.googlesource.com/skia/+/6ca0b6a46cbe9bef3e2b9b9db813ec864efd62de

Committed: https://skia.googlesource.com/skia/+/837f5321a409228a27fc710eb71c87866b820cfb

[Stephen White, 2014-07-10 22:27:12]

Mike^2: PTAL. This is a first stab at a fix for this issue.

Please check the lifetime semantics: is it safe to hang on 
to a bare pointer to the SkPaint that's passed to us in 
saveLayer(), or might it expire before the primitives are
drawn?

Also, we may be able to just get rid of SaveEntry, and just use an SkPaint*
instead. I thought I might need more stuff inside SaveEntry, but perhaps I don't
(a NULL paint indicates a save(), or a saveLayer() with a NULL paint -- in
either case we need an entry, so it balances with the pop()s in restore()).

[mtklein, 2014-07-10 23:00:59]

On 2014/07/10 22:27:12, Stephen White wrote:
> Mike^2: PTAL. This is a first stab at a fix for this issue.

I think this is essentially the correct fix for the problem as I understand it.

> Please check the lifetime semantics: is it safe to hang on 
> to a bare pointer to the SkPaint that's passed to us in 
> saveLayer(), or might it expire before the primitives are
> drawn?

I'm afraid we do need to be careful about lifetimes.  As we currently create
these BBHs today, we fill them while recording from the raw arguments passed in
by the client.  The client's free to destroy that paint right after making the
saveLayer call.  Sad to say I think you need to copy the paint in saveLayer (
and delete it in restore).  saveLayers are rare enough that I don't think it's a
big deal to make this extra copy (somewhere a step or two up the INHERITED chain
it will be copied again into the SkPictureData).

I'm going to be reimplementing this logic soon for SkRecord-backed pictures,
where lifetime won't be an issue.  In that world, we'll have already done all
the copies by the time we get to building a bounding box hierarchy, so unowned
pointers will work fine.
 
> Also, we may be able to just get rid of SaveEntry, and just use an SkPaint*
> instead. I thought I might need more stuff inside SaveEntry, but perhaps I
don't
> (a NULL paint indicates a save(), or a saveLayer() with a NULL paint -- in
> either case we need an entry, so it balances with the pop()s in restore()).

Either way works for me.  I'd personally go with just a const SkPaint*, but the
struct isn't hurting readability here.

[Stephen White, 2014-07-11 15:00:57]

On 2014/07/10 23:00:59, mtklein wrote:
> On 2014/07/10 22:27:12, Stephen White wrote:
> > Mike^2: PTAL. This is a first stab at a fix for this issue.
> 
> I think this is essentially the correct fix for the problem as I understand
it.
> 
> > Please check the lifetime semantics: is it safe to hang on 
> > to a bare pointer to the SkPaint that's passed to us in 
> > saveLayer(), or might it expire before the primitives are
> > drawn?
> 
> I'm afraid we do need to be careful about lifetimes.  As we currently create
> these BBHs today, we fill them while recording from the raw arguments passed
in
> by the client.  The client's free to destroy that paint right after making the
> saveLayer call.  Sad to say I think you need to copy the paint in saveLayer (
> and delete it in restore).  saveLayers are rare enough that I don't think it's
a
> big deal to make this extra copy (somewhere a step or two up the INHERITED
chain
> it will be copied again into the SkPictureData).

Done. For now, I've made it copy only if there's a filter in the SkPaint, to
avoid 
impacting record times in other cases.

> I'm going to be reimplementing this logic soon for SkRecord-backed pictures,
> where lifetime won't be an issue.  In that world, we'll have already done all
> the copies by the time we get to building a bounding box hierarchy, so unowned
> pointers will work fine.
>  
> > Also, we may be able to just get rid of SaveEntry, and just use an SkPaint*
> > instead. I thought I might need more stuff inside SaveEntry, but perhaps I
> don't
> > (a NULL paint indicates a save(), or a saveLayer() with a NULL paint -- in
> > either case we need an entry, so it balances with the pop()s in restore()).
> 
> Either way works for me.  I'd personally go with just a const SkPaint*, but
the
> struct isn't hurting readability here.

Done.

[mtklein, 2014-07-11 15:37:14]

lgtm.  Just some places I think deserve comments.

When this lands I think we can remove kNoBBH_Flag from resizeimagefilter and
imageresizetiled (and kNoBBH_Flag itself and the code to support it).  You can
roll that in here or I'm happy to do it as a follow up.

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

https://codereview.chromium.org/380373003/diff/20001/src/core/SkBBoxRecord.cp...
src/core/SkBBoxRecord.cpp:305: fSaveStack.push(paint && paint->getImageFilter()
? new SkPaint(*paint) : NULL);
// The only effects that can affect the bounds of draw commands inside this
layer are SkImageFilters.

?

https://codereview.chromium.org/380373003/diff/20001/src/core/SkBBoxRecord.h
File src/core/SkBBoxRecord.h (right):

https://codereview.chromium.org/380373003/diff/20001/src/core/SkBBoxRecord.h#...
src/core/SkBBoxRecord.h:74: SkTDStack<const SkPaint*> fSaveStack;
// Paints from (possibly nested) saveLayers that need to be applied to bounding
boxes of all draw commands inside them.  We own these pointers.

?

[Stephen White, 2014-07-11 15:48:34]

On 2014/07/11 15:37:14, mtklein wrote:
> lgtm.  Just some places I think deserve comments.

Done.

> When this lands I think we can remove kNoBBH_Flag from resizeimagefilter and
> imageresizetiled (and kNoBBH_Flag itself and the code to support it).  You can
> roll that in here or I'm happy to do it as a follow up.

Let's do that as a followup.

> https://codereview.chromium.org/380373003/diff/20001/src/core/SkBBoxRecord.cpp
> File src/core/SkBBoxRecord.cpp (right):
> 
>
https://codereview.chromium.org/380373003/diff/20001/src/core/SkBBoxRecord.cp...
> src/core/SkBBoxRecord.cpp:305: fSaveStack.push(paint &&
paint->getImageFilter()
> ? new SkPaint(*paint) : NULL);
> // The only effects that can affect the bounds of draw commands inside this
> layer are SkImageFilters.
> 
> ?
> 
> https://codereview.chromium.org/380373003/diff/20001/src/core/SkBBoxRecord.h
> File src/core/SkBBoxRecord.h (right):
> 
>
https://codereview.chromium.org/380373003/diff/20001/src/core/SkBBoxRecord.h#...
> src/core/SkBBoxRecord.h:74: SkTDStack<const SkPaint*> fSaveStack;
> // Paints from (possibly nested) saveLayers that need to be applied to
bounding
> boxes of all draw commands inside them.  We own these pointers.
> 
> ?

[Stephen White, 2014-07-11 16:06:23]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-07-11 16:06:29]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/senorblanco@chromium.org/380373003/60001

[commit-bot: I haz the power, 2014-07-11 16:06:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-11 16:06:31]

Failed to apply patch for src/core/SkBBoxRecord.cpp:
While running git apply --index -p1;
  error: patch failed: src/core/SkBBoxRecord.cpp:302
  error: src/core/SkBBoxRecord.cpp: patch does not apply

Patch:       src/core/SkBBoxRecord.cpp
Index: src/core/SkBBoxRecord.cpp
diff --git a/src/core/SkBBoxRecord.cpp b/src/core/SkBBoxRecord.cpp
index
e6f5f431db02cd95c7fc4de38e9e3e2bfd7c614e..3670f9145e8bcf7581dea8d4c7f6ff98ccab3ecc
100644
--- a/src/core/SkBBoxRecord.cpp
+++ b/src/core/SkBBoxRecord.cpp
@@ -302,6 +302,8 @@ void SkBBoxRecord::willSave() {
 SkCanvas::SaveLayerStrategy SkBBoxRecord::willSaveLayer(const SkRect* bounds,
                                                         const SkPaint* paint,
                                                         SaveFlags flags) {
+    // Image filters can affect the effective bounds of primitives drawn inside
saveLayer().
+    // Copy the paint so we can compute the modified bounds in
transformBounds().
     fSaveStack.push(paint && paint->getImageFilter() ? new SkPaint(*paint) :
NULL);
     return this->INHERITED::willSaveLayer(bounds, paint, flags);
 }

[Stephen White, 2014-07-11 16:13:19]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-07-11 16:13:29]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/senorblanco@chromium.org/380373003/80001

[commit-bot: I haz the power, 2014-07-11 16:56:09]

Change committed as 6ca0b6a46cbe9bef3e2b9b9db813ec864efd62de

[robertphillips, 2014-07-13 18:57:29]

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

https://codereview.chromium.org/380373003/diff/80001/src/core/SkBBoxRecord.cp...
src/core/SkBBoxRecord.cpp:334: 
I don't think SkTDStack::index does what you want! If the number of elements in
your stack exceeds the amount cached in each SkTDStack::Rec object an assert
will fire (and sorrow will ensue). Just running:

render_pictures -r desk_pokemonwiki.skp --mode tile 256 256 --bbh grid 256 256

should reproduce the problem

[Stephen White, 2014-07-14 15:35:13]

mtklein: PTAL. New patch up.

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

https://codereview.chromium.org/380373003/diff/80001/src/core/SkBBoxRecord.cp...
src/core/SkBBoxRecord.cpp:334: 
On 2014/07/13 18:57:29, robertphillips wrote:
> I don't think SkTDStack::index does what you want! If the number of elements
in
> your stack exceeds the amount cached in each SkTDStack::Rec object an assert
> will fire (and sorrow will ensue). Just running:
> 
> render_pictures -r desk_pokemonwiki.skp --mode tile 256 256 --bbh grid 256 256
> 
> should reproduce the problem

Yeah; looks like SkTDStack is hopelessly broken. Switched to using SkTDArray for
now.

[reed1, 2014-07-14 16:07:02]

On 2014/07/14 15:35:13, Stephen White wrote:
> mtklein: PTAL. New patch up.
> 
> https://codereview.chromium.org/380373003/diff/80001/src/core/SkBBoxRecord.cpp
> File src/core/SkBBoxRecord.cpp (right):
> 
>
https://codereview.chromium.org/380373003/diff/80001/src/core/SkBBoxRecord.cp...
> src/core/SkBBoxRecord.cpp:334: 
> On 2014/07/13 18:57:29, robertphillips wrote:
> > I don't think SkTDStack::index does what you want! If the number of elements
> in
> > your stack exceeds the amount cached in each SkTDStack::Rec object an assert
> > will fire (and sorrow will ensue). Just running:
> > 
> > render_pictures -r desk_pokemonwiki.skp --mode tile 256 256 --bbh grid 256
256
> > 
> > should reproduce the problem
> 
> Yeah; looks like SkTDStack is hopelessly broken. Switched to using SkTDArray
for
> now.

Should we fix it or remove it?

[mtklein, 2014-07-14 16:15:23]

On 2014/07/14 16:07:02, reed1 wrote:
> On 2014/07/14 15:35:13, Stephen White wrote:
> > mtklein: PTAL. New patch up.
> > 
> >
https://codereview.chromium.org/380373003/diff/80001/src/core/SkBBoxRecord.cpp
> > File src/core/SkBBoxRecord.cpp (right):
> > 
> >
>
https://codereview.chromium.org/380373003/diff/80001/src/core/SkBBoxRecord.cp...
> > src/core/SkBBoxRecord.cpp:334: 
> > On 2014/07/13 18:57:29, robertphillips wrote:
> > > I don't think SkTDStack::index does what you want! If the number of
elements
> > in
> > > your stack exceeds the amount cached in each SkTDStack::Rec object an
assert
> > > will fire (and sorrow will ensue). Just running:
> > > 
> > > render_pictures -r desk_pokemonwiki.skp --mode tile 256 256 --bbh grid 256
> 256
> > > 
> > > should reproduce the problem
> > 
> > Yeah; looks like SkTDStack is hopelessly broken. Switched to using SkTDArray
> for
> > now.
> 
> Should we fix it or remove it?

I'd vote to remove it.  I think SkTDArray can replace it everywhere.

[robertphillips, 2014-07-14 16:25:37]

I second the motion (to remove it).

[mtklein, 2014-07-14 16:43:13]

no dealbreakers

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

https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
src/core/SkBBoxRecord.cpp:11: SkBBoxRecord::~SkBBoxRecord() {
Consider just calling fSaveStack.deleteAll() ?

https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
src/core/SkBBoxRecord.cpp:306: *fSaveStack.append() = paint &&
paint->getImageFilter() ? new SkPaint(*paint) : NULL;
Completely fine, but consider

fSaveStack.push(paint && paint->getImageFilter() ? new SkPaint(*paint) : NULL);

https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
src/core/SkBBoxRecord.cpp:311: int last = fSaveStack.count() - 1;
delete fSaveStack.top();
fSaveStack.pop();
?

https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
src/core/SkBBoxRecord.cpp:336: const SkPaint* paint = fSaveStack.getAt(i);
fSaveStack[i] ?

[Stephen White, 2014-07-14 16:55:01]

New patch up; PTAL.

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

https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
src/core/SkBBoxRecord.cpp:11: SkBBoxRecord::~SkBBoxRecord() {
On 2014/07/14 16:43:12, mtklein wrote:
> Consider just calling fSaveStack.deleteAll() ?

Done.

https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
src/core/SkBBoxRecord.cpp:306: *fSaveStack.append() = paint &&
paint->getImageFilter() ? new SkPaint(*paint) : NULL;
On 2014/07/14 16:43:12, mtklein wrote:
> Completely fine, but consider
> 
> fSaveStack.push(paint && paint->getImageFilter() ? new SkPaint(*paint) :
NULL);

Ha! Didn't notice those, thanks!

Done.

https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
src/core/SkBBoxRecord.cpp:311: int last = fSaveStack.count() - 1;
On 2014/07/14 16:43:12, mtklein wrote:
> delete fSaveStack.top();
> fSaveStack.pop();
> ?

Done.

https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
src/core/SkBBoxRecord.cpp:336: const SkPaint* paint = fSaveStack.getAt(i);
On 2014/07/14 16:43:12, mtklein wrote:
> fSaveStack[i] ?

Think I'll stick with the explicit version here (operator overloads can be
misleading).

[mtklein, 2014-07-14 16:57:56]

On 2014/07/14 16:55:01, Stephen White wrote:
> New patch up; PTAL.
> 
>
https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.cpp
> File src/core/SkBBoxRecord.cpp (right):
> 
>
https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
> src/core/SkBBoxRecord.cpp:11: SkBBoxRecord::~SkBBoxRecord() {
> On 2014/07/14 16:43:12, mtklein wrote:
> > Consider just calling fSaveStack.deleteAll() ?
> 
> Done.
> 
>
https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
> src/core/SkBBoxRecord.cpp:306: *fSaveStack.append() = paint &&
> paint->getImageFilter() ? new SkPaint(*paint) : NULL;
> On 2014/07/14 16:43:12, mtklein wrote:
> > Completely fine, but consider
> > 
> > fSaveStack.push(paint && paint->getImageFilter() ? new SkPaint(*paint) :
> NULL);
> 
> Ha! Didn't notice those, thanks!
> 
> Done.
> 
>
https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
> src/core/SkBBoxRecord.cpp:311: int last = fSaveStack.count() - 1;
> On 2014/07/14 16:43:12, mtklein wrote:
> > delete fSaveStack.top();
> > fSaveStack.pop();
> > ?
> 
> Done.
> 
>
https://codereview.chromium.org/380373003/diff/100001/src/core/SkBBoxRecord.c...
> src/core/SkBBoxRecord.cpp:336: const SkPaint* paint = fSaveStack.getAt(i);
> On 2014/07/14 16:43:12, mtklein wrote:
> > fSaveStack[i] ?
> 
> Think I'll stick with the explicit version here (operator overloads can be
> misleading).

lgtm

[Stephen White, 2014-07-14 17:03:57]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-07-14 17:04:14]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/senorblanco@chromium.org/380373003/14...

[commit-bot: I haz the power, 2014-07-14 17:19:59]

Change committed as 837f5321a409228a27fc710eb71c87866b820cfb