Fix saveLayer() with a pixel-moving filter vs SkBBoxHierarchyRecord / SkRecordDraw

Fix saveLayer() with a pixel-moving filter vs SkBBoxHierarchyRecord / SkRecordDraw

In SkBBoxHierarchyRecord:
  Since the bounds we pass to saveLayer are in the pre-filtering
  coordinate space, they aren't correct for determining the actual
  device pixels touched by the saveLayer in this case.

  The easiest fix for now is to pass the clip bounds, since the final
  draw done in restore() will never draw outside the clip.

In SkRecordDraw:
  We do adjust the bounds passed to saveLayer, so we just need to make
  sure that when we're using a paint that may affect transparent black,
  we ignore the calculated bounds of draw ops and use the clip intersected
  with those adjusted bounds.

See originally crrev.com/497773002

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/d910f544439fffa6c2bcc5181b79b2811a4c130a

[mtklein, 2014-08-22 15:40:03]

Here's what I understand to be the implementation for SkRecordDraw.  SkBBHR.cpp
and test are unchanged.

[Stephen White, 2014-08-22 15:45:10]

Great!

LGTM (with the bug description a bit more verbose)

[mtklein, 2014-08-22 15:45:58]

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

https://codereview.chromium.org/496963003/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:182: if (op.bounds &&
!PaintMayAffectTransparentBlack(op.paint)) {
Just reading again:

"Since the bounds we pass to saveLayer are in the pre-filtering
coordinate space, they aren't correct for determining the actual
device pixels touched by the saveLayer in this case."

Does the fact that I'm running bounds through adjustAndMap() (adjust for paint
affects, map back to indentity space) mean I can logically drop this part of the
change?

[mtklein, 2014-08-22 15:49:25]

On 2014/08/22 15:45:58, mtklein wrote:
> https://codereview.chromium.org/496963003/diff/1/src/core/SkRecordDraw.cpp
> File src/core/SkRecordDraw.cpp (right):
> 
>
https://codereview.chromium.org/496963003/diff/1/src/core/SkRecordDraw.cpp#ne...
> src/core/SkRecordDraw.cpp:182: if (op.bounds &&
> !PaintMayAffectTransparentBlack(op.paint)) {
> Just reading again:
> 
> "Since the bounds we pass to saveLayer are in the pre-filtering
> coordinate space, they aren't correct for determining the actual
> device pixels touched by the saveLayer in this case."
> 
> Does the fact that I'm running bounds through adjustAndMap() (adjust for paint
> affects, map back to indentity space) mean I can logically drop this part of
the
> change?

Please see PS2.  All tests, including the new one, pass on PS1 and PS2.

[Stephen White, 2014-08-22 15:51:26]

On 2014/08/22 15:49:25, mtklein wrote:
> On 2014/08/22 15:45:58, mtklein wrote:
> > https://codereview.chromium.org/496963003/diff/1/src/core/SkRecordDraw.cpp
> > File src/core/SkRecordDraw.cpp (right):
> > 
> >
>
https://codereview.chromium.org/496963003/diff/1/src/core/SkRecordDraw.cpp#ne...
> > src/core/SkRecordDraw.cpp:182: if (op.bounds &&
> > !PaintMayAffectTransparentBlack(op.paint)) {
> > Just reading again:
> > 
> > "Since the bounds we pass to saveLayer are in the pre-filtering
> > coordinate space, they aren't correct for determining the actual
> > device pixels touched by the saveLayer in this case."
> > 
> > Does the fact that I'm running bounds through adjustAndMap() (adjust for
paint
> > affects, map back to indentity space) mean I can logically drop this part of
> the
> > change?
> 
> Please see PS2.  All tests, including the new one, pass on PS1 and PS2.

Yes, I think if you're munging the bounds in the restore, that should be enough
to fix it.
We couldn't do that in the old path (since we had forgotten the paint by then or
something, I forget),
so we stuck in a dummy node between the saveLayer & restore to trigger insertion
into the bbox.

[mtklein, 2014-08-22 15:54:26]

> Yes, I think if you're munging the bounds in the restore, that should be
enough
> to fix it.
> We couldn't do that in the old path (since we had forgotten the paint by then
or
> something, I forget),
> so we stuck in a dummy node between the saveLayer & restore to trigger
insertion
> into the bbox.

Great!  I've updated the description to match.  Here we go...

[mtklein, 2014-08-22 15:55:29]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-08-22 15:56:08]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/496963003/20001

[commit-bot: I haz the power, 2014-08-22 16:06:40]

Committed patchset #2 (20001) as d910f544439fffa6c2bcc5181b79b2811a4c130a