Merge SkCodec with SkScanlineDecoder

tests/CodexTest.cpp

Index: tests/CodexTest.cpp
diff --git a/tests/CodexTest.cpp b/tests/CodexTest.cpp
index 2ea6eda57ec86a899a83f5778b31ddea2b0fada6..c19cbc91cf08f64d443f72a5c1e9ec961afa8ec6 100644
--- a/tests/CodexTest.cpp
+++ b/tests/CodexTest.cpp
@@ -11,7 +11,6 @@
 #include "SkMD5.h"
 #include "SkRandom.h"
 #include "SkScaledCodec.h"
-#include "SkScanlineDecoder.h"
 #include "Test.h"
 
 static SkStreamAsset* resource(const char path[]) {
@@ -147,25 +146,44 @@ static void check(skiatest::Reporter* r,
 
     // Scanline decoding follows.
 
-    stream.reset(resource(path));
-    SkAutoTDelete<SkScanlineDecoder> scanlineDecoder(
-            SkScanlineDecoder::NewFromStream(stream.detach()));
+    // Need to call start() first.
+    REPORTER_ASSERT(r, codec->getScanlines(bm.getAddr(0, 0), 1, 0)
+            == SkCodec::kScanlineDecodingNotStarted);

[scroggo, 2015/09/25 16:07:50]

Again, these can check for 0/false once we change the return values, as in
Matt's other CL.

+    REPORTER_ASSERT(r, codec->skipScanlines(1)
+            == SkCodec::kScanlineDecodingNotStarted);
+
+    const SkCodec::Result startResult = codec->start(info);
     if (supportsScanlineDecoding) {
         bm.eraseColor(SK_ColorYELLOW);
-        REPORTER_ASSERT(r, scanlineDecoder);
 
-        REPORTER_ASSERT(r, scanlineDecoder->start(info) == SkCodec::kSuccess);
+        REPORTER_ASSERT(r, startResult == SkCodec::kSuccess);
 
         for (int y = 0; y < info.height(); y++) {
-            result = scanlineDecoder->getScanlines(bm.getAddr(0, y), 1, 0);
+            result = codec->getScanlines(bm.getAddr(0, y), 1, 0);
             REPORTER_ASSERT(r, result == SkCodec::kSuccess);
         }
         // verify that scanline decoding gives the same result.
-        if (SkScanlineDecoder::kTopDown_SkScanlineOrder == scanlineDecoder->getScanlineOrder()) {
+        if (SkCodec::kTopDown_SkScanlineOrder == codec->getScanlineOrder()) {
             compare_to_good_digest(r, digest, bm);
         }
+
+        // Cannot continue to decode scanlines beyond the end

[msarett, 2015/09/28 14:48:50]

+1 for new tests

+        REPORTER_ASSERT(r, codec->getScanlines(bm.getAddr(0, 0), 1, 0)
+                == SkCodec::kInvalidParameters);
+
+        // Interrupting a scanline decode with a full decode starts from
+        // scratch
+        REPORTER_ASSERT(r, codec->start(info) == SkCodec::kSuccess);
+        REPORTER_ASSERT(r, codec->getScanlines(bm.getAddr(0, 0), 1, 0)
+                == SkCodec::kSuccess);
+        REPORTER_ASSERT(r, codec->getPixels(bm.info(), bm.getPixels(), bm.rowBytes())
+                == SkCodec::kSuccess);
+        REPORTER_ASSERT(r, codec->getScanlines(bm.getAddr(0, 0), 1, 0)
+                == SkCodec::kScanlineDecodingNotStarted);
+        REPORTER_ASSERT(r, codec->skipScanlines(1)
+                == SkCodec::kScanlineDecodingNotStarted);
     } else {
-        REPORTER_ASSERT(r, !scanlineDecoder);
+        REPORTER_ASSERT(r, startResult == SkCodec::kUnimplemented);
     }
 
     // The rest of this function tests decoding subsets, and will decode an arbitrary number of
@@ -247,10 +265,89 @@ DEF_TEST(Codec, r) {
     check(r, "mandrill_512.png", SkISize::Make(512, 512), true, false);
     check(r, "mandrill_64.png", SkISize::Make(64, 64), true, false);
     check(r, "plane.png", SkISize::Make(250, 126), true, false);
+    check(r, "plane_interlaced.png", SkISize::Make(250, 126), true, false);
     check(r, "randPixels.png", SkISize::Make(8, 8), true, false);
     check(r, "yellow_rose.png", SkISize::Make(400, 301), true, false);
 }
 
+// Test interlaced PNG in stripes, similar to DM's kStripe_Mode

[scroggo, 2015/09/25 16:07:50]

Maybe this test is redundant with kStripe_Mode, but I like it because it allows
me to test locally without searching for an interlaced image I want to test.

It's also not really interlace specific; I wanted to make sure I didn't break
interlacing (since I now use the base class's fCurrScanline instead of a
separate variable), but it could easily be run on other types (and it could use
37, as kStripe_Mode does, if we want to replace it - not sure whether it's more
useful to have here versus run on more images in DM).

I could also move this code into check, which might simplify things a little
bit.

[msarett, 2015/09/28 14:48:50]

I like this as a separate test - I think it's good to verify this functionality
of scanline decoding for interlaced pngs (and for all top-down image types if we
wanted to generalize this test).  I'm in favor of keeping it and am not really
bothered that it's similar to the DM test.

Did we ever discuss forbidding the client from calling getScanlines() more than
once on interlaced pngs (since it is so inefficient)?  If we wanted to do that,
it would break this test.

It's probably worth noting that the kStripe_Mode test in DM is skipped unless
the image is kTopDown (interlaced pngs are kNone).  There's actually no reason
why the DM test won't work with kNone (it'll just be extremely inefficient).

AFAICT, this test, just like the test in DM, will fail on kBottomUp and
kOutOfOrder images.

IMO, this test is not enough to replace kStripe_Mode in DM.  I think it's
important to keep that for regression testing on future updates of
libjpeg-turbo.  Testing on interesting values of numStripes would help, but it's
also nice to test on all of the jpegs we have on the bots.

[scroggo, 2015/09/28 16:01:53]

Thanks for the feedback. FWIW, I don't think anything about this test is
actually interlaced specific - it should work for top-down as is (unless I've
forgotten about something...).
I don't know if we did discuss. I don't think we should forbid it though. It's
possible that a client wants to get multiple subsets without rewinding (e.g.
top-left and then bottom-left). That would be a valid use case for calling it
more than once.
Yeah, when Emmalee initially added some interlaced images she changed DM to
decode all the lines desired (rather than one at a time), since it was so slow.
We probably don't want to run all interlaced images through stripe mode, just in
the interest of speed.
Agreed. Added a check for that.
sgtm

+DEF_TEST(Codec_stripes, r) {
+    const char * path = "plane_interlaced.png";
+    SkAutoTDelete<SkStream> stream(resource(path));
+    if (!stream) {
+        SkDebugf("Missing resource '%s'\n", path);
+    }
+
+    SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream.detach()));
+    REPORTER_ASSERT(r, codec);
+
+    // Baseline for what the image should look like, using N32.
+    const SkImageInfo info = codec->getInfo().makeColorType(kN32_SkColorType);
+
+    SkBitmap bm;
+    bm.allocPixels(info);
+    SkAutoLockPixels autoLockPixels(bm);
+    SkCodec::Result result = codec->getPixels(info, bm.getPixels(), bm.rowBytes());
+    REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+    SkMD5::Digest digest;
+    md5(bm, &digest);
+
+    // Now decode in stripes
+    const int height = info.height();
+    const int numStripes = 4;
+    int stripeHeight;
+    int remainingLines;
+    SkTDivMod(height, numStripes, &stripeHeight, &remainingLines);
+
+    bm.eraseColor(SK_ColorYELLOW);
+
+    result = codec->start(info);
+    REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+    // Odd stripes
+    for (int i = 1; i < numStripes; i += 2) {
+        // Skip the even stripes
+        result = codec->skipScanlines(stripeHeight);
+        REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+        result = codec->getScanlines(bm.getAddr(0, i * stripeHeight), stripeHeight,
+                                     bm.rowBytes());
+        REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+    }
+
+    // Even stripes
+    result = codec->start(info);
+    REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+    for (int i = 0; i < numStripes; i += 2) {
+        result = codec->getScanlines(bm.getAddr(0, i * stripeHeight), stripeHeight,
+                                     bm.rowBytes());
+        REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+        // Skip the odd stripes
+        if (i + 1 < numStripes) {
+            result = codec->skipScanlines(stripeHeight);
+            REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+        }
+    }
+
+    // Remainder at the end
+    if (remainingLines > 0) {
+        result = codec->start(info);
+        REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+        result = codec->skipScanlines(height - remainingLines);
+        REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+
+        result = codec->getScanlines(bm.getAddr(0, height - remainingLines),
+                                     remainingLines, bm.rowBytes());
+        REPORTER_ASSERT(r, result == SkCodec::kSuccess);
+    }
+
+    compare_to_good_digest(r, digest, bm);
+}
+
 static void test_invalid_stream(skiatest::Reporter* r, const void* stream, size_t len) {
     SkCodec* codec = SkCodec::NewFromStream(new SkMemoryStream(stream, len, false));
     // We should not have gotten a codec. Bots should catch us if we leaked anything.
@@ -369,8 +466,7 @@ static void test_invalid_parameters(skiatest::Reporter* r, const char path[]) {
         SkDebugf("Missing resource '%s'\n", path);
         return;
     }
-    SkAutoTDelete<SkScanlineDecoder> decoder(SkScanlineDecoder::NewFromStream(
-        stream.detach()));
+    SkAutoTDelete<SkCodec> decoder(SkCodec::NewFromStream(stream.detach()));
     
     // This should return kSuccess because kIndex8 is supported.
     SkPMColor colorStorage[256];