save-draw*-restore -> noop-draw*-noop

save-drawA-drawB-drawC-restore always means drawA-drawB-drawC,
no matter what flags we use for save().

This one triggers all over the silk SKPs and in several of the bot SKPs, typically as save-drawBitmap-restore.

BUG=skia:2378
R=robertphillips@google.com, mtklein@google.com

Author: mtklein@chromium.org

Review URL: https://codereview.chromium.org/267793006

git-svn-id: http://skia.googlecode.com/svn/trunk@14618 2bbb7eff-a529-9590-31e7-b0007b416f81

2 files changed, 41 insertions, 12 deletions

diff --git a/src/record/SkRecordOpts.cpp b/src/record/SkRecordOpts.cpp
index 5b648f4215..d855124917 100644
--- a/src/record/SkRecordOpts.cpp
+++ b/src/record/SkRecordOpts.cpp
@@ -42,8 +42,21 @@ static bool apply(Pass* pass, SkRecord* record) {
     return changed;
 }
 
+// Turns the logical NoOp Save and Restore in Save-Draw*-Restore patterns into actual NoOps.
+struct SaveOnlyDrawsRestoreNooper {
+    typedef Pattern3<Is<Save>,
+                     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;
+    }
+};
 // Turns logical no-op Save-[non-drawing command]*-Restore patterns into actual no-ops.
-struct SaveRestoreNooper {
+struct SaveNoDrawsRestoreNooper {
     // Star matches greedily, so we also have to exclude Save and Restore.
     typedef Pattern3<Is<Save>,
                      Star<Not<Or3<Is<Save>,
@@ -66,8 +79,11 @@ struct SaveRestoreNooper {
     }
 };
 void SkRecordNoopSaveRestores(SkRecord* record) {
-    SaveRestoreNooper pass;
-    while (apply(&pass, record));  // Run until it stops changing things.
+    SaveOnlyDrawsRestoreNooper onlyDraws;
+    SaveNoDrawsRestoreNooper noDraws;
+
+    // Run until they stop changing things.
+    while (apply(&onlyDraws, record) || apply(&noDraws, record));
 }
 
 // For some SaveLayer-[drawing command]-Restore patterns, merge the SaveLayer's alpha into the
diff --git a/tests/RecordOptsTest.cpp b/tests/RecordOptsTest.cpp
index 90347eda1f..33142c8a3d 100644
--- a/tests/RecordOptsTest.cpp
+++ b/tests/RecordOptsTest.cpp
@@ -108,6 +108,28 @@ DEF_TEST(RecordOpts_TextBounding, r) {
     REPORTER_ASSERT(r, bounded->maxY >= SK_Scalar1 + defaults.getTextSize());
 }
 
+DEF_TEST(RecordOpts_NoopDrawSaveRestore, r) {
+    SkRecord record;
+    SkRecorder recorder(SkRecorder::kWriteOnly_Mode, &record, W, H);
+
+    // The save and restore are pointless if there's only draw commands in the middle.
+    recorder.save();
+        recorder.drawRect(SkRect::MakeWH(200, 200), SkPaint());
+        recorder.drawRect(SkRect::MakeWH(300, 300), SkPaint());
+        recorder.drawRect(SkRect::MakeWH(100, 100), SkPaint());
+    recorder.restore();
+
+    record.replace<SkRecords::NoOp>(2);  // NoOps should be allowed.
+
+    SkRecordNoopSaveRestores(&record);
+
+    assert_type<SkRecords::NoOp>(r, record, 0);
+    assert_type<SkRecords::DrawRect>(r, record, 1);
+    assert_type<SkRecords::NoOp>(r, record, 2);
+    assert_type<SkRecords::DrawRect>(r, record, 3);
+    assert_type<SkRecords::NoOp>(r, record, 4);
+}
+
 DEF_TEST(RecordOpts_SingleNoopSaveRestore, r) {
     SkRecord record;
     SkRecorder recorder(SkRecorder::kWriteOnly_Mode, &record, W, H);
@@ -139,19 +161,10 @@ DEF_TEST(RecordOpts_NoopSaveRestores, r) {
         recorder.restore();
     recorder.restore();
 
-    // These will be kept (though some future optimization might noop the save and restore).
-    recorder.save();
-        recorder.drawRect(SkRect::MakeWH(200, 200), SkPaint());
-    recorder.restore();
-
     SkRecordNoopSaveRestores(&record);
-
     for (unsigned index = 0; index < 8; index++) {
         assert_type<SkRecords::NoOp>(r, record, index);
     }
-    assert_type<SkRecords::Save>(r, record, 8);
-    assert_type<SkRecords::DrawRect>(r, record, 9);
-    assert_type<SkRecords::Restore>(r, record, 10);
 }
 
 static void assert_savelayer_restore(skiatest::Reporter* r,