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

src/core/SkRecordOpts.cpp

Index: src/core/SkRecordOpts.cpp
diff --git a/src/core/SkRecordOpts.cpp b/src/core/SkRecordOpts.cpp
index 17595c01c55b5cef09a6509163346f2ebb0996a2..bdb15cbd15b0eb0efef622e0b11206af78e90d2d 100644
--- a/src/core/SkRecordOpts.cpp
+++ b/src/core/SkRecordOpts.cpp
@@ -12,6 +12,7 @@
 #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
@@ -21,6 +22,7 @@ void SkRecordOptimize(SkRecord* record) {
     //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:
@@ -56,6 +58,27 @@ struct SaveOnlyDrawsRestoreNooper {
         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.
@@ -111,34 +134,15 @@ struct SaveLayerDrawRestoreNooper {
 
         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
@@ -150,8 +154,10 @@ struct SaveLayerDrawRestoreNooper {
                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) {
@@ -159,3 +165,59 @@ void SkRecordNoopSaveLayerDrawRestores(SkRecord* record) {
     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);
+}