Fix for empty saveLayer() with a filter which affects transparent black.

Fix for empty saveLayer() with a filter which affects transparent black.

If an saveLayer()/restore() is recorded, tilegrid/rtree will cull them
out and not draw anything. This is correct for most cases, but if the
paint in the saveLayer() is one that affects transparent black (e.g.,
it contains a color filter or image filter which affects transparent
black), this is incorrect: the filter should be applied.

Fixed by adding a no-op between the saveLayer() and restore(), and
adding a bbox node pointing at that node with the saveLayer()'s bounds.

This exposed a bug in SkPictureRecord.cpp's match(), where it would
assert if the NOOP was the last op seen. Fixed with an early-out before
calling peek_op_and_size().

BUG=skia:2254
Committed: https://code.google.com/p/skia/source/detail?r=14604

[Stephen White, 2014-05-06 22:31:02]

sugoi@: PTAL. This is a variant of your patch, using a no-op
node to fix the clipping issues.

[Justin Novosad, 2014-05-06 22:38:02]

On 2014/05/06 22:31:02, Stephen White wrote:
> sugoi@: PTAL. This is a variant of your patch, using a no-op
> node to fix the clipping issues.

Nice trickery.
lgtm

[Stephen White, 2014-05-06 22:52:59]

Committed patchset #1 manually as r14604 (presubmit successful).

[mtklein, 2014-05-07 15:14:51]

Crazy.

I don't anticipate any analogous problems in SkRecord.  We may skip over
SaveLayer/Restore blocks via culls, but bad culling is on the client's head. 
We're looking at optimizations now that peel off SaveLayers, but they get
flustered and bail out if the SaveLayer has any paint effect.

The assertions in this new test should work for any canvas, right?  In that
case, it might be worth upgrading or doubling this as a GM, so we're checked on
all our weird-SkCanvas fronts.

[Stephen White, 2014-05-07 15:36:39]

On 2014/05/07 15:14:51, mtklein wrote:
> Crazy.
> 
> I don't anticipate any analogous problems in SkRecord.  We may skip over
> SaveLayer/Restore blocks via culls, but bad culling is on the client's head. 
> We're looking at optimizations now that peel off SaveLayers, but they get
> flustered and bail out if the SaveLayer has any paint effect.
> 
> The assertions in this new test should work for any canvas, right?  In that
> case, it might be worth upgrading or doubling this as a GM, so we're checked
on
> all our weird-SkCanvas fronts.

Sounds good, although I'm kinda preferring unit tests these days (this test 
actually started life as a GM). They run faster, often are shorter in code, 
require fewer resources (i.e., no baselines), and can't be accidentally 
rebaselined if someone doesn't understand the results.

I've written a few that run in both CPU and GPU modes; maybe we could come up
with
a generalized way of writing a unit test and automatically running it with
different
SkCanvases?

[mtklein, 2014-05-07 15:39:54]

> I've written a few that run in both CPU and GPU modes; maybe we could come up
> with
> a generalized way of writing a unit test and automatically running it with
> different
> SkCanvases?

Isn't a unit test that automatically runs with different canvases a GM?  The
only common way we have to check all canvases is to flush, read the pixels and
inspect them.  I guess what's missing is the ability to pack the expected bitmap
as part of the code rather than as a separate file?

[Stephen White, 2014-05-07 15:49:28]

On 2014/05/07 15:39:54, mtklein wrote:
> > I've written a few that run in both CPU and GPU modes; maybe we could come
up
> > with
> > a generalized way of writing a unit test and automatically running it with
> > different
> > SkCanvases?
> 
> Isn't a unit test that automatically runs with different canvases a GM?  The
> only common way we have to check all canvases is to flush, read the pixels and
> inspect them.  I guess what's missing is the ability to pack the expected
bitmap
> as part of the code rather than as a separate file?

I'm mostly talking about regression tests such as this, where the unit test can 
often be arbitrarily small (e.g., 10x10 or even 1x1) as long as it passes with 
the fix, and fails without it. No need for an expected bitmap if you only need
to test a single pixel. If you made that a GM, it'd be easy to rebaseline by 
accident, or misinterpret the results visually.