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

src/core/SkRecordOpts.cpp

 /*
  * Copyright 2014 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkRecordOpts.h"
 
 #include "SkRecordPattern.h"
 #include "SkRecords.h"
 #include "SkTDArray.h"
 
 using namespace SkRecords;
+static void SkRecordNoopSaveLayerSaveLayerRestores(SkRecord* record);

[mtklein, 2015/01/21 16:20:29]

This is a sign we need unit tests. :)  You'll want this to be visible in the
header so you can call it there.

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

Done.

 
 void SkRecordOptimize(SkRecord* record) {
     // This might be useful  as a first pass in the future if we want to weed
     // out junk for other optimization passes.  Right now, nothing needs it,
     // and the bounding box hierarchy will do the work of skipping no-op
     // Save-NoDraw-Restore sequences better than we can here.
     //SkRecordNoopSaveRestores(record);
 
     SkRecordNoopSaveLayerDrawRestores(record);
+    SkRecordNoopSaveLayerSaveLayerRestores(record);

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

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?

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

No, they are not related. They can not generate new patterns where the output of
the other would be accepted by the another.
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. 
No. These don't really interact.

 }
 
 // Most of the optimizations in this file are pattern-based.  These are all defined as structs with:
 //   - a Pattern typedef
 //   - a bool onMatch(SkRceord*, Pattern*, unsigned begin, unsigned end) method,
 //     which returns true if it made changes and false if not.
 
 // Run a pattern-based optimization once across the SkRecord, returning true if it made any changes.
 // It looks for spans which match Pass::Pattern, and when found calls onMatch() with the pattern,
 // record, and [begin,end) span of the commands that matched.
@@ skipping 15 matching lines @@
                      Star<Or<Is<NoOp>, IsDraw> >,
                      Is<Restore> >
         Pattern;
 
     bool onMatch(SkRecord* record, Pattern* pattern, unsigned begin, unsigned end) {
         record->replace<NoOp>(begin);  // Save
         record->replace<NoOp>(end-1);  // Restore
         return true;
     }
 };
+
+static bool has_any_effect(const SkPaint& paint) {
+    return paint.getPathEffect()  ||
+            paint.getShader()      ||
+            paint.getXfermode()    ||
+            paint.getMaskFilter()  ||
+            paint.getColorFilter() ||
+            paint.getRasterizer()  ||
+            paint.getLooper()      ||
+            paint.getImageFilter();
+}
+
+static bool is_only_alpha(SkColor color) {
+    return SK_ColorTRANSPARENT == SkColorSetA(color, SK_AlphaTRANSPARENT);
+}
+
+static SkColor blend_opacity_layer_color(SkColor baseColor, SkColor opacityLayerColor) {
+    return SkColorSetA(baseColor, SkFloatToIntRound(SkColorGetA(baseColor) *
+                                                    SkColorGetA(opacityLayerColor) / 255.0f));
+}
+
 // Turns logical no-op Save-[non-drawing command]*-Restore patterns into actual no-ops.
 struct SaveNoDrawsRestoreNooper {
     // Star matches greedily, so we also have to exclude Save and Restore.
     // Nested SaveLayers need to be excluded, or we'll match their Restore!
     typedef Pattern3<Is<Save>,
                      Star<Not<Or4<Is<Save>,
                                   Is<SaveLayer>,
                                   Is<Restore>,
                                   IsDraw> > >,
                      Is<Restore> >
@@ skipping 35 matching lines @@
 
         SkPaint* drawPaint = pattern->second<SkPaint>();
         if (drawPaint == NULL) {
             // We can just give the draw the SaveLayer's paint.
             // TODO(mtklein): figure out how to do this clearly
             return false;
         }
 
         const uint32_t layerColor = layerPaint->getColor();
         const uint32_t  drawColor =  drawPaint->getColor();
-        if (!IsOnlyAlpha(layerColor) || HasAnyEffect(*layerPaint) || CantFoldAlpha(*drawPaint)) {
+        if (!is_only_alpha(layerColor) || has_any_effect(*layerPaint) || CantFoldAlpha(*drawPaint)) {
             // Too fancy for us.
             return false;
         }
 
-        drawPaint->setColor(SkColorSetA(drawColor,
-                                        SkFloatToIntRound(SkColorGetA(drawColor) *
-                                                          SkColorGetA(layerColor) / 255.0f)));
+        drawPaint->setColor(blend_opacity_layer_color(drawColor, layerColor));
         return KillSaveLayerAndRestore(record, begin);
     }
 
-    static bool KillSaveLayerAndRestore(SkRecord* record, unsigned saveLayerIndex) {
-        record->replace<NoOp>(saveLayerIndex);    // SaveLayer
-        record->replace<NoOp>(saveLayerIndex+2);  // Restore
-        return true;
-    }
-
-    static bool HasAnyEffect(const SkPaint& paint) {
-        return paint.getPathEffect()  ||
-               paint.getShader()      ||
-               paint.getXfermode()    ||
-               paint.getMaskFilter()  ||
-               paint.getColorFilter() ||
-               paint.getRasterizer()  ||
-               paint.getLooper()      ||
-               paint.getImageFilter();
-    }
-
     // The alpha folding can proceed if the single draw's paint has a shader,
     // path effect, mask filter and/or rasterizer.
     // TODO: most likely the looper and only some xfer modes are the hard
     // constraints
     static bool CantFoldAlpha(const SkPaint& paint) {
         return paint.getXfermode()    ||
                paint.getColorFilter() ||
                paint.getLooper()      ||
                paint.getImageFilter();
     }
 
-    static bool IsOnlyAlpha(SkColor color) {
-        return SK_ColorTRANSPARENT == SkColorSetA(color, SK_AlphaTRANSPARENT);
+    static bool KillSaveLayerAndRestore(SkRecord* record, unsigned saveLayerIndex) {
+        record->replace<NoOp>(saveLayerIndex);    // SaveLayer
+        record->replace<NoOp>(saveLayerIndex+2);  // Restore
+        return true;
     }
 };
 void SkRecordNoopSaveLayerDrawRestores(SkRecord* record) {
     SaveLayerDrawRestoreNooper pass;
     apply(&pass, record);
 }
 
+
+/* For SVG generated:
+  SaveLayer (non-opaque)

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

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/22 13:55:01]

Acknowledged.

+    Save
+      Clip Rect
+      Save Layer
+      Restore
+    Restore
+  Restore
+*/
+struct SaveLayerSaveLayerRestoreNooper {

[mtklein, 2015/01/21 16:20:29]

Might be I've set a bad naming precedent here.  Maybe something like
SvgLayerMerger?

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

Done.

+    typedef Pattern7<Is<SaveLayer>, Is<Save>, Is<ClipRect>, Is<SaveLayer>,
+                     Is<Restore>, Is<Restore>, Is<Restore> > Pattern;
+
+    bool onMatch(SkRecord* record, Pattern* pattern, unsigned begin, unsigned end) {
+        SaveLayer* opacityLayer = pattern->first<SaveLayer>();
+        if (opacityLayer->bounds != NULL) {
+            // SaveLayer with bounds is too tricky for us.

[mtklein, 2015/01/21 16:20:29]

I removed this check in the other pattern.  I think it's safe to remove it here
too?

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

Done.

+            return false;
+        }
+
+        SkPaint* opacityPaint = opacityLayer->paint;
+        if (NULL == opacityPaint) {
+            // There wasn't really any point to this SaveLayer at all.
+            return KillSaveLayerAndRestore(record, begin);
+        }
+
+        SkPaint* secondLayerPaint = pattern->fourth<SaveLayer>()->paint;
+        if (secondLayerPaint == NULL) {
+            // We can just give the draw the SaveLayer's paint.

[mtklein, 2015/01/21 16:20:29]

// We can just give the inner SaveLayer the outer SaveLayer's paint?

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

Done.

+            // TODO(mtklein): figure out how to do this clearly
+            return false;
+        }
+
+        const uint32_t opacityColor = opacityPaint->getColor();
+        const uint32_t secondLayerColor =  secondLayerPaint->getColor();
+        if (!is_only_alpha(opacityColor)  || has_any_effect(*opacityPaint)) {

[mtklein, 2015/01/21 16:20:29]

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]

Done.

+            // Too fancy for us.
+            return false;
+        }
+
+        secondLayerPaint->setColor(blend_opacity_layer_color(secondLayerColor, opacityColor));
+        return KillSaveLayerAndRestore(record, begin);
+    }
+
+    static bool KillSaveLayerAndRestore(SkRecord* record, unsigned saveLayerIndex) {
+        record->replace<NoOp>(saveLayerIndex);     // SaveLayer
+        record->replace<NoOp>(saveLayerIndex + 6); // Restore
+        return true;
+    }
+};
+
+static void SkRecordNoopSaveLayerSaveLayerRestores(SkRecord* record) {
+    SaveLayerSaveLayerRestoreNooper pass;
+    apply(&pass, record);
+}