Fold alpha to the inner savelayer in savelayer-savelayer-restore patterns

Fold alpha to the inner savelayer in savelayer-savelayer-restore patterns

Fold alpha to the inner savelayer in savelayer-savelayer-restore
patterns such as this:

  SaveLayer (non-opaque)
    Save
      ClipRect
      SaveLayer
      Restore
    Restore
  Restore

Current blink generates these for example for SVG content such as this:

<path style="opacity:0.5 filter:url(#blur_filter)"/>

The outer save layer is due to the opacity and the inner one is due to
blur filter being implemented with picture image filter.

Reduces layers in desk_carsvg.skp testcase from 115 to 78.

BUG=skia:3119
Committed: https://skia.googlesource.com/skia/+/dc0f408a961b6ce4e7631e06031bbfe1d8d7bcec

[Kimmo Kinnunen, 2015-01-13 09:08:53]

Robert, you mentioned:

"""
 2) SaveLayer1|SaveLayer2|Restore|Restore where SL2 has a
SkColorFilterImageFilter

 The SaveLayer1|SaveLayer2|Restore|Restore patterns do indeed contain an
SkPictureImageFilter. The exact filter DAG (on SaveLayer2) appears to be:

ColorFilterImageFilter1 (TableColorFilter) <- BlurImageFilter <-
ColorFilterImageFilter2 (TableColorFilter) <- PictureImageFilter

where the TableColorFilters have different tables. So ... the
SaveLayer1|SaveLayer2|Restore|Restore patterns would need to be changed upstream
in the Blink code.

"""

I looked at blink, and at the moment, the idea of reducing redundant layers
doesn't fit to the code (eg. there's no concern about redundant layers). I'd
imagine the current philosophy (if there is a concern) is of the sentiment "if
it is possible to reduce the layers, the correct place is in Skia". 

Do you see immediately the many problems with the approach in the patch?

There's the obvious one wrt. spending time in optimization pass, going through
the record many times. If the approach could work from other  perspectives, I
could try to work out with mtklein he'd be ok if all the patterns can be
evaluated in a single pass. (If that's possible, that is)

[mtklein, 2015-01-13 21:12:19]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-01-13 21:12:20]

We're gonna want tests before we land this.

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

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:170: SaveLayer (non-opaque)
This pattern seems very specific.  This makes me wonder:
  1) can we fix this in Blink?
  2) if we can't,  can we break this into smaller, more generally independent
passes?

E.g. 
  * Save - ClipRect - SaveLayer - Restore - Restore ~~~~> SaveLayer (with cull
rect clipped by ClipRect clip) - Restore
  * SaveLayer (alpha only) - SaveLayer - Restore - Restore ~~~~>  SaveLayer
(blended alpha) - Restore

How is the image filter set up again?  Is it a PictureImageFilter piped into a
BlurImageFIlter?  If so, I'm not sure I see a better way to do that than draw
via a SaveLayer (SaveLayer (with blur) - DrawPicture - Restore is functionally
identical to SaveLayer(with Picture->Blur) - Restore afaik).

[Kimmo Kinnunen, 2015-01-14 13:24:58]

kkinnunen@nvidia.com changed reviewers:
+ senorblanco@google.com

[Kimmo Kinnunen, 2015-01-14 13:44:03]

On 2015/01/13 21:12:20, mtklein wrote:
> We're gonna want tests before we land this.
> 
> https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp
> File src/core/SkRecordOpts.cpp (right):
> 
>
https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
> src/core/SkRecordOpts.cpp:170: SaveLayer (non-opaque)
> This pattern seems very specific.  This makes me wonder:
>   1) can we fix this in Blink?

So there's two savelayers.

Regarding the first save layer: this is due to the opacity. This is the one that
this commit tries to remove. In the comment#1, I mentioned far as I understand
the blink code, blink doesn't try to reduce this (eg. it is sort of by design to
not complicate that code).

So the other option removing the layer is to remove the cliprect, move that rect
to the filter save layer and let the existing savelayer-draw-restore pattern
handle this. However, this would be then savelayer-savelayer-restore-restore
-pattern, which needs a new pattern matcher. However, this pattern is arguably
more generic.


cc: senorblanco@
Stephen, regarding following clip rectangle in blink code:

RenderSVGResourceFilter.cpp:141
   
context->clipRect(lastEffect->determineAbsolutePaintRect(lastEffect->maxEffectRect()));

Do you think it'd make sense for me to start trying to change blink so that that
clip rectangle gets set to the bounds rectangle of the save layer call that
contains the picture image filter? Currently the rectangle goes in the clip
before said save layer call. 

What I'm afraid is that I'd spend quite a bit of time with the coordinate
systems, as I'm not that familiar anymore with those in blink.

(For context: In the commit msg theres a Skia draw pattern generated by certain
svg content. The pattern creates two saveLayers calls, and one could be
optimized. The saveLayers are quite expensive for MSAA backends)



> How is the image filter set up again?  Is it a PictureImageFilter piped into a
> BlurImageFIlter?

Yeah, it's of the following form:
(SkColorFilterImageFilter: 
 (input: (SkBlurImageFilter: 
          (sigma: (27.773584, 27.773584) input 
                  (SkColorFilterImageFilter: 
                   (input: 
                    (SkPictureImageFilter: 
                     (crop: 
                      (84.704727,273.557098,811.767334,583.254822) picture: 
                      (0.000000,0.000000,728.000000,310.000000)))
                    color filter: SkTable_ColorFilter( ... )))))
  color filter: SkTable_ColorFilter( ... )))

> If so, I'm not sure I see a better way to do that than draw
> via a SaveLayer (SaveLayer (with blur) - DrawPicture - Restore is functionally
> identical to SaveLayer(with Picture->Blur) - Restore afaik).

That's what I thought, and so I started removing the layer doing the opacity..

[Stephen White, 2015-01-14 15:46:37]

On 2015/01/14 13:44:03, Kimmo Kinnunen wrote:
> On 2015/01/13 21:12:20, mtklein wrote:
> > We're gonna want tests before we land this.
> > 
> > https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp
> > File src/core/SkRecordOpts.cpp (right):
> > 
> >
>
https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
> > src/core/SkRecordOpts.cpp:170: SaveLayer (non-opaque)
> > This pattern seems very specific.  This makes me wonder:
> >   1) can we fix this in Blink?
> 
> So there's two savelayers.
> 
> Regarding the first save layer: this is due to the opacity. This is the one
that
> this commit tries to remove. In the comment#1, I mentioned far as I understand
> the blink code, blink doesn't try to reduce this (eg. it is sort of by design
to
> not complicate that code).

Do you know where the code is in Blink for that saveLayer()? I implemented the
one for
filters, but I'm not sure where the opacity one is. If we could forward the
alpha
down, we might be able to eliminate it (but only in the case that the filter is
going
to do it for us).

> So the other option removing the layer is to remove the cliprect, move that
rect
> to the filter save layer and let the existing savelayer-draw-restore pattern
> handle this. However, this would be then savelayer-savelayer-restore-restore
> -pattern, which needs a new pattern matcher. However, this pattern is arguably
> more generic.

> cc: senorblanco@
> Stephen, regarding following clip rectangle in blink code:
> 
> RenderSVGResourceFilter.cpp:141
>    
>
context->clipRect(lastEffect->determineAbsolutePaintRect(lastEffect->maxEffectRect()));
> 
> Do you think it'd make sense for me to start trying to change blink so that
that
> clip rectangle gets set to the bounds rectangle of the save layer call that
> contains the picture image filter? Currently the rectangle goes in the clip
> before said save layer call.

The clip rect is a bit tricky. IIRC, it currently has to be handled outside the
saveLayer, in order to handle the case where there's a filterRes attribute on
the
filter, or skew/rotate in the CTM (where we modify the CTM after the clip, but
before
the saveLayer). We want that to affect the bounds of the saveLayer, but *not* to

affect the clip, which is applied with the original, unmodified CTM during
restore().

This would all get a lot less tricky if we relocated this code from Blink to
Skia, 
aka https://code.google.com/p/skia/issues/detail?id=3288, but that would require
some infrastructure work in Skia.

> What I'm afraid is that I'd spend quite a bit of time with the coordinate
> systems, as I'm not that familiar anymore with those in blink.
> 
> (For context: In the commit msg theres a Skia draw pattern generated by
certain
> svg content. The pattern creates two saveLayers calls, and one could be
> optimized. The saveLayers are quite expensive for MSAA backends)
> 
> 
> 
> > How is the image filter set up again?  Is it a PictureImageFilter piped into
a
> > BlurImageFIlter?
> 
> Yeah, it's of the following form:
> (SkColorFilterImageFilter: 
>  (input: (SkBlurImageFilter: 
>           (sigma: (27.773584, 27.773584) input 
>                   (SkColorFilterImageFilter: 
>                    (input: 
>                     (SkPictureImageFilter: 
>                      (crop: 
>                       (84.704727,273.557098,811.767334,583.254822) picture: 
>                       (0.000000,0.000000,728.000000,310.000000)))
>                     color filter: SkTable_ColorFilter( ... )))))
>   color filter: SkTable_ColorFilter( ... )))
> 
> > If so, I'm not sure I see a better way to do that than draw
> > via a SaveLayer (SaveLayer (with blur) - DrawPicture - Restore is
functionally
> > identical to SaveLayer(with Picture->Blur) - Restore afaik).
> 
> That's what I thought, and so I started removing the layer doing the opacity..

[Kimmo Kinnunen, 2015-01-15 09:07:15]

On 2015/01/14 15:46:37, Stephen White wrote:
> On 2015/01/14 13:44:03, Kimmo Kinnunen wrote:
> > On 2015/01/13 21:12:20, mtklein wrote:
> > > We're gonna want tests before we land this.
> > > 
> > > https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp
> > > File src/core/SkRecordOpts.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
> > > src/core/SkRecordOpts.cpp:170: SaveLayer (non-opaque)
> > > This pattern seems very specific.  This makes me wonder:
> > >   1) can we fix this in Blink?
> > 
> > So there's two savelayers.
> > 
> > Regarding the first save layer: this is due to the opacity. This is the one
> that
> > this commit tries to remove. In the comment#1, I mentioned far as I
understand
> > the blink code, blink doesn't try to reduce this (eg. it is sort of by
design
> to
> > not complicate that code).
> 
> Do you know where the code is in Blink for that saveLayer()? I implemented the
> one for
> filters, but I'm not sure where the opacity one is. If we could forward the
> alpha
> down, we might be able to eliminate it (but only in the case that the filter
is
> going
> to do it for us).

This is the bt for the Skia save layer induced by opacity for SVG elements:

0x00005555599fd350 in blink::GraphicsContext::beginLayer(float,
blink::CompositeOperator, blink::FloatRect const*, blink::ColorFilter,
SkImageFilter*) ()
#1  0x0000555559a2f027 in
blink::BeginTransparencyDisplayItem::replay(blink::GraphicsContext*) ()
#2  0x0000555557848da1 in
blink::TransparencyRecorder::TransparencyRecorder(blink::GraphicsContext*,
blink::DisplayItemClientInternalVoid*, blink::CompositeOperator,
blink::WebBlendMode const&, float, blink::CompositeOperator) ()
#3  0x0000555557aab03b in
blink::SVGRenderingContext::prepareToRenderSVGContent(blink::RenderObject*,
blink::PaintInfo&) ()
#4  0x0000555559b07b64 in blink::SVGShapePainter::paint(blink::PaintInfo const&)
()
#5  0x0000555557b04c5e in blink::RenderSVGShape::paint(blink::PaintInfo const&,
blink::LayoutPoint const&) ()
#6  0x0000555559b0550e in blink::SVGContainerPainter::paint(blink::PaintInfo
const&) ()
#7  0x0000555557aff5de in blink::RenderSVGContainer::paint(blink::PaintInfo
const&, blink::LayoutPoint const&) ()

[Kimmo Kinnunen, 2015-01-21 14:00:03]

On 2015/01/13 21:12:20, mtklein wrote:
> This pattern seems very specific.  This makes me wonder:
>   2) if we can't,  can we break this into smaller, more generally independent
> passes?
> 
> E.g. 
>   * Save - ClipRect - SaveLayer - Restore - Restore ~~~~> SaveLayer (with cull
> rect clipped by ClipRect clip) - Restore
>   * SaveLayer (alpha only) - SaveLayer - Restore - Restore ~~~~>  SaveLayer
> (blended alpha) - Restore

So in order to do this, one would need to be able to guarantee that SaveLayer
bounds is strictly respected in every case.

SkCanvas.h API says this:
"... drawing may be clipped to it, though that clipping is not guaranteed to
happen. If exact clipping is desired, use clipRect()."

SkCanvas.cpp also has code such as functions related to
"gIgnoreSaveLayerBounds", which explicitly ignores the save layer cull rect.

Now if I start working on this suggestion, it would imply that I remove the
above "save layer bounds is optional" code, and ensure that every backend every
time respects the bounds.

Did you mean that if I want to work on making Skia not do the layers in this
case, I need to do all that beforehand?

[reed1, 2015-01-21 14:03:45]

On 2015/01/21 14:00:03, Kimmo Kinnunen wrote:
> On 2015/01/13 21:12:20, mtklein wrote:
> > This pattern seems very specific.  This makes me wonder:
> >   2) if we can't,  can we break this into smaller, more generally
independent
> > passes?
> > 
> > E.g. 
> >   * Save - ClipRect - SaveLayer - Restore - Restore ~~~~> SaveLayer (with
cull
> > rect clipped by ClipRect clip) - Restore
> >   * SaveLayer (alpha only) - SaveLayer - Restore - Restore ~~~~>  SaveLayer
> > (blended alpha) - Restore
> 
> So in order to do this, one would need to be able to guarantee that SaveLayer
> bounds is strictly respected in every case.
> 
> SkCanvas.h API says this:
> "... drawing may be clipped to it, though that clipping is not guaranteed to
> happen. If exact clipping is desired, use clipRect()."
> 
> SkCanvas.cpp also has code such as functions related to
> "gIgnoreSaveLayerBounds", which explicitly ignores the save layer cull rect.
> 
> Now if I start working on this suggestion, it would imply that I remove the
> above "save layer bounds is optional" code, and ensure that every backend
every
> time respects the bounds.
> 
> Did you mean that if I want to work on making Skia not do the layers in this
> case, I need to do all that beforehand?

SaveLayer bounds are NOT always respected. If we are relying on that, that is a
bug. (e.g. what happens if the world is rotated? should we enforce a rotated
clip?) Bounds optional parameter is purely a hint so that we *can* decide to
reduce the size of the offscreen if we choose to.

[reed1, 2015-01-21 14:03:58]

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

[mtklein, 2015-01-21 16:20:30]

Ah, good point. We cannot merge ClipRect into a SaveLayer.  Let's go ahead with
the 7-slot pattern for now.

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

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:15: static void
SkRecordNoopSaveLayerSaveLayerRestores(SkRecord* record);
This is a sign we need unit tests. :)  You'll want this to be visible in the
header so you can call it there.

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:25: SkRecordNoopSaveLayerSaveLayerRestores(record);
Can you see any SKPs where these optimizations feed each other?   Is it worth
running these to fix-point rather than once?  If just once is enough, does it
make a difference which order we run these two in?

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:178: struct SaveLayerSaveLayerRestoreNooper {
Might be I've set a bad naming precedent here.  Maybe something like
SvgLayerMerger?

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:185: // SaveLayer with bounds is too tricky for us.
I removed this check in the other pattern.  I think it's safe to remove it here
too?

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:197: // We can just give the draw the SaveLayer's
paint.
// We can just give the inner SaveLayer the outer SaveLayer's paint?

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:204: if (!is_only_alpha(opacityColor)  ||
has_any_effect(*opacityPaint)) {
We're not doing a CantFoldAlpha check here, and I'm not sure if that's
intentional or a bug.  I feel like we'd be served well here by a single
higher-level static function to do all the checks and merging instead of a bunch
of small ones.  Something like this?

static bool fold_layer_paint(const SkPaint& layerPaint, SkPaint* paint);

?

I'd throw in as much common code from the two save layer optimizations as
possible.

[Kimmo Kinnunen, 2015-01-22 13:55:01]

How about now?

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

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:15: static void
SkRecordNoopSaveLayerSaveLayerRestores(SkRecord* record);
On 2015/01/21 16:20:29, mtklein wrote:
> This is a sign we need unit tests. :)  You'll want this to be visible in the
> header so you can call it there.

Done.

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:25: SkRecordNoopSaveLayerSaveLayerRestores(record);
On 2015/01/21 16:20:30, mtklein wrote:
> Can you see any SKPs where these optimizations feed each other? 

No, they are not related. They can not generate new patterns where the output of
the other would be accepted by the another.

>  Is it worth running these to fix-point rather than once? 

At the moment no. Theoretically there could be yet-another outer savelayer, but
then the pattern would need to contain Star<Is<Nop> >.

Same concern is with the existing SkRecordNoopSaveLayerDrawRestores. 

To simplify the problem, one could just say that this theoretical concern should
be addressed after the state machine can track correctly the match state over
substitutions without restarts. 

> If just once is enough, does it make a difference which order we run these two
in?

No. These don't really interact.

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:170: SaveLayer (non-opaque)
On 2015/01/13 21:12:20, mtklein wrote:
> This pattern seems very specific.  This makes me wonder:
>   1) can we fix this in Blink?
>   2) if we can't,  can we break this into smaller, more generally independent
> passes?
> 
> E.g. 
>   * Save - ClipRect - SaveLayer - Restore - Restore ~~~~> SaveLayer (with cull
> rect clipped by ClipRect clip) - Restore
>   * SaveLayer (alpha only) - SaveLayer - Restore - Restore ~~~~>  SaveLayer
> (blended alpha) - Restore
> 
> How is the image filter set up again?  Is it a PictureImageFilter piped into a
> BlurImageFIlter?  If so, I'm not sure I see a better way to do that than draw
> via a SaveLayer (SaveLayer (with blur) - DrawPicture - Restore is functionally
> identical to SaveLayer(with Picture->Blur) - Restore afaik).

Acknowledged.

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:178: struct SaveLayerSaveLayerRestoreNooper {
On 2015/01/21 16:20:29, mtklein wrote:
> Might be I've set a bad naming precedent here.  Maybe something like
> SvgLayerMerger?

Done.

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:185: // SaveLayer with bounds is too tricky for us.
On 2015/01/21 16:20:29, mtklein wrote:
> I removed this check in the other pattern.  I think it's safe to remove it
here
> too?

Done.

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:197: // We can just give the draw the SaveLayer's
paint.
On 2015/01/21 16:20:29, mtklein wrote:
> // We can just give the inner SaveLayer the outer SaveLayer's paint?

Done.

https://codereview.chromium.org/835973005/diff/1/src/core/SkRecordOpts.cpp#ne...
src/core/SkRecordOpts.cpp:204: if (!is_only_alpha(opacityColor)  ||
has_any_effect(*opacityPaint)) {
On 2015/01/21 16:20:29, mtklein wrote:
> We're not doing a CantFoldAlpha check here, and I'm not sure if that's
> intentional or a bug.  I feel like we'd be served well here by a single
> higher-level static function to do all the checks and merging instead of a
bunch
> of small ones.  Something like this?
> 
> static bool fold_layer_paint(const SkPaint& layerPaint, SkPaint* paint);
> 
> ?
> 
> I'd throw in as much common code from the two save layer optimizations as
> possible.

Done.

[mtklein, 2015-01-22 14:40:43]

Thanks.  Just one last question.

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

https://codereview.chromium.org/835973005/diff/40001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:215: static bool
IsParentAlphaModulationTransitive(const SkPaint& paint) {
This seems like it should logically be part of
fold_opacity_layer_color_to_paint(), as should CantFoldAlpha in the other
optimization.  Reading your notes, it sounds like this is the right
implementation.  Which of the two is correct, or are the conditions for folding
really distinct?

[Kimmo Kinnunen, 2015-01-23 14:46:35]

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

https://codereview.chromium.org/835973005/diff/40001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:215: static bool
IsParentAlphaModulationTransitive(const SkPaint& paint) {
On 2015/01/22 14:40:43, mtklein wrote:
> This seems like it should logically be part of
> fold_opacity_layer_color_to_paint(), as should CantFoldAlpha in the other
> optimization.  Reading your notes, it sounds like this is the right
> implementation.  Which of the two is correct, or are the conditions for
folding
> really distinct?

Done.
I uploaded new version where the difference is articulated a bit differently.

Turns out I didn't quite understand correctly the filters, though. I hope the
understanding is better now, but the track record is not very good.

I added a gm to test the code, but we can remove it if it's redundant..

[mtklein, 2015-01-23 15:12:07]

lgtm, with a few suggestions for code tweaks and comments

Just to make sure I understand, is the GM correct with:
  - two columns
  - left and right columns always identical
  - first 3 rows are green, with a white box in the middle row
  - next 6 rows are green, with a grey box in the middle row
  - last 6 rows are grey
?

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp
File gm/recordopts.cpp (right):

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcode57
gm/recordopts.cpp:57: static void
draw_save_layer_draw_rect_restore_sequence(SkCanvas* canvas, const SkColor&
shapeColor,
SkColors are probably cheaper to pass by value.

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcode63
gm/recordopts.cpp:63: SkPaint drawPaint;
sometimes I find adding indentation at a call to save() or saveLayer() and back
up again at restore() can help readability.  Might try that?

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcode87
gm/recordopts.cpp:87: p.setColor(SK_ColorWHITE);
Might help readability to also SkASSERT(shapeColor != SK_ColorWHITE);

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcode96
gm/recordopts.cpp:96: // inner savelayer. We know that alpah folding happens to
inner savelayer, so add detector there.
spelling of "alpah"

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcod...
gm/recordopts.cpp:125: class RecordOptsGM : public skiagm::GM {
This is a good candidate for DEF_SIMPLE_GM:

DEF_SIMPLE_GM(recordopts, canvas, (kTestRectSize+1)*2, (kTestRectSize+1)*15) {
   <code from onDraw here>
}

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

https://codereview.chromium.org/835973005/diff/80001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:61: static bool
fold_opacity_layer_color_to_paint(const SkPaint& layerPaint,
// We assume layerPaint is always from a saveLayer.  If isSaveLayer is true, we
assume paint is too.

https://codereview.chromium.org/835973005/diff/80001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:77: // modulated with the paint color.
Might add to the end:

, so it's fine to proceed with the fold for saveLayer paints with image filters.

https://codereview.chromium.org/835973005/diff/80001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:87: // give the type out easily, so just do not
optimize that at the moment.
Just FYI, this is trivially easy to get, but possibly difficult to manage in
complexity.  Instead of matching IsDraw, you can match Is<DrawBitmap>,
Is<DrawSprite>, etc.  Best approach would be to write a new matcher
IsUniformDraw, then match on that and Not<IsUniformDraw> separately.

[mtklein, 2015-01-23 15:13:11]

>   - next 6 rows are green, with a grey box in the middle row
The grey box is from the color filter removing everything but our good green,
right?

[Kimmo Kinnunen, 2015-01-26 07:11:01]

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp
File gm/recordopts.cpp (right):

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcode57
gm/recordopts.cpp:57: static void
draw_save_layer_draw_rect_restore_sequence(SkCanvas* canvas, const SkColor&
shapeColor,
On 2015/01/23 15:12:06, mtklein wrote:
> SkColors are probably cheaper to pass by value.

Done.

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcode63
gm/recordopts.cpp:63: SkPaint drawPaint;
On 2015/01/23 15:12:07, mtklein wrote:
> sometimes I find adding indentation at a call to save() or saveLayer() and
back
> up again at restore() can help readability.  Might try that?

Done.

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcode87
gm/recordopts.cpp:87: p.setColor(SK_ColorWHITE);
On 2015/01/23 15:12:06, mtklein wrote:
> Might help readability to also SkASSERT(shapeColor != SK_ColorWHITE);

Done.

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcode96
gm/recordopts.cpp:96: // inner savelayer. We know that alpah folding happens to
inner savelayer, so add detector there.
On 2015/01/23 15:12:06, mtklein wrote:
> spelling of "alpah"

Done.

https://codereview.chromium.org/835973005/diff/80001/gm/recordopts.cpp#newcod...
gm/recordopts.cpp:125: class RecordOptsGM : public skiagm::GM {
On 2015/01/23 15:12:06, mtklein wrote:
> This is a good candidate for DEF_SIMPLE_GM:
> 
> DEF_SIMPLE_GM(recordopts, canvas, (kTestRectSize+1)*2, (kTestRectSize+1)*15) {
>    <code from onDraw here>
> }

Done.

[Kimmo Kinnunen, 2015-01-26 07:12:25]

On 2015/01/23 15:12:07, mtklein wrote:
> lgtm, with a few suggestions for code tweaks and comments
> 
> Just to make sure I understand, is the GM correct with:
>   - two columns
>   - left and right columns always identical
>   - first 3 rows are green, with a white box in the middle row
>   - next 6 rows are green, with a grey box in the middle row
>   - last 6 rows are grey
> ?

This is correct. Added this as a comment.

>   - next 6 rows are green, with a grey box in the middle row
The grey box is from the color filter removing everything but our good green,
right?

This is correct. Added this as a comment.

>
https://codereview.chromium.org/835973005/diff/80001/src/core/SkRecordOpts.cp...
> src/core/SkRecordOpts.cpp:87: // give the type out easily, so just do not
> optimize that at the moment.
> Just FYI, this is trivially easy to get, but possibly difficult to manage in
> complexity.  Instead of matching IsDraw, you can match Is<DrawBitmap>,
> Is<DrawSprite>, etc.  Best approach would be to write a new matcher
> IsUniformDraw, then match on that and Not<IsUniformDraw> separately.

I could try that in a separate commit..

[Kimmo Kinnunen, 2015-01-26 07:13:48]

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

https://codereview.chromium.org/835973005/diff/80001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:61: static bool
fold_opacity_layer_color_to_paint(const SkPaint& layerPaint,
On 2015/01/23 15:12:07, mtklein wrote:
> // We assume layerPaint is always from a saveLayer.  If isSaveLayer is true,
we
> assume paint is too.

Done.

https://codereview.chromium.org/835973005/diff/80001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:77: // modulated with the paint color.
On 2015/01/23 15:12:07, mtklein wrote:
> Might add to the end:
> 
> , so it's fine to proceed with the fold for saveLayer paints with image
filters.

Done.

https://codereview.chromium.org/835973005/diff/80001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:87: // give the type out easily, so just do not
optimize that at the moment.
On 2015/01/23 15:12:07, mtklein wrote:
> Just FYI, this is trivially easy to get, but possibly difficult to manage in
> complexity.  Instead of matching IsDraw, you can match Is<DrawBitmap>,
> Is<DrawSprite>, etc.  Best approach would be to write a new matcher
> IsUniformDraw, then match on that and Not<IsUniformDraw> separately.

Acknowledged.

[Kimmo Kinnunen, 2015-01-26 07:13:57]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-01-26 07:14:18]

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

[commit-bot: I haz the power, 2015-01-26 07:19:58]

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

[commit-bot: I haz the power, 2015-01-26 07:19:59]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[Kimmo Kinnunen, 2015-01-26 07:33:25]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-01-26 07:34:18]

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

[commit-bot: I haz the power, 2015-01-26 07:38:18]

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

[commit-bot: I haz the power, 2015-01-26 07:38:20]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on client.skia.compile
(http://build.chromium.org/p/client.skia.compile/builders/Build-Win-VS2013-x86...)

[Kimmo Kinnunen, 2015-01-26 08:06:35]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-01-26 08:07:29]

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

[commit-bot: I haz the power, 2015-01-26 08:14:30]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/dc0f408a961b6ce4e7631e06031bbfe1d8d7bcec