Merge SkCodec with SkScanlineDecoder

tests/CodexTest.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "Resources.h"
 #include "SkBitmap.h"
 #include "SkCodec.h"
 #include "SkMD5.h"
 #include "SkRandom.h"
 #include "SkScaledCodec.h"
-#include "SkScanlineDecoder.h"
 #include "Test.h"
 
 static SkStreamAsset* resource(const char path[]) {
     SkString fullPath = GetResourcePath(path);
     return SkStream::NewFromFile(fullPath.c_str());
 }
 
 static void md5(const SkBitmap& bm, SkMD5::Digest* digest) {
     SkAutoLockPixels autoLockPixels(bm);
     SkASSERT(bm.getPixels());
@@ skipping 115 matching lines @@
             } else {
                 otherAt = kPremul_SkAlphaType;
             }
             // The other non-opaque alpha type should always succeed, but not match.
             test_info(r, codec, info.makeAlphaType(otherAt), SkCodec::kSuccess, nullptr);
         }
     }
 
     // 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
     // random subsets.
     // Do not attempt to decode subsets of an image of only once pixel, since there is no
     // meaningful subset.
     if (size.width() * size.height() == 1) {
         return;
     }
 
@@ skipping 61 matching lines @@
     check(r, "baby_tux.png", SkISize::Make(240, 246), true, false);
     check(r, "color_wheel.png", SkISize::Make(128, 128), true, false);
     check(r, "half-transparent-white-pixel.png", SkISize::Make(1, 1), true, false);
     check(r, "mandrill_128.png", SkISize::Make(128, 128), true, false);
     check(r, "mandrill_16.png", SkISize::Make(16, 16), true, false);
     check(r, "mandrill_256.png", SkISize::Make(256, 256), true, false);
     check(r, "mandrill_32.png", SkISize::Make(32, 32), true, false);
     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.
     REPORTER_ASSERT(r, !codec);
 }
 
 // Ensure that SkCodec::NewFromStream handles freeing the passed in SkStream,
 // even on failure. Test some bad streams.
 DEF_TEST(Codec_leaks, r) {
     // No codec should claim this as their format, so this tests SkCodec::NewFromStream.
@@ skipping 98 matching lines @@
     // This image is an ico with an embedded mask-bmp.  This is illegal.
     test_invalid(r, "invalid_images/mask-bmp-ico.ico");
 }
 
 static void test_invalid_parameters(skiatest::Reporter* r, const char path[]) {
     SkAutoTDelete<SkStream> stream(resource(path));
     if (!stream) {
         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];
     int colorCount;
     SkCodec::Result result = decoder->start(
         decoder->getInfo().makeColorType(kIndex_8_SkColorType), nullptr, colorStorage, &colorCount);
     REPORTER_ASSERT(r, SkCodec::kSuccess == result);
     // The rest of the test is uninteresting if kIndex8 is not supported
     if (SkCodec::kSuccess != result) {
         return;
     }
 
     // This should return kInvalidParameters because, in kIndex_8 mode, we must pass in a valid
     // colorPtr and a valid colorCountPtr.
     result = decoder->start(
         decoder->getInfo().makeColorType(kIndex_8_SkColorType), nullptr, nullptr, nullptr);
     REPORTER_ASSERT(r, SkCodec::kInvalidParameters == result);
     result = decoder->start(
         decoder->getInfo().makeColorType(kIndex_8_SkColorType));
     REPORTER_ASSERT(r, SkCodec::kInvalidParameters == result);
 }
 
 DEF_TEST(Codec_Params, r) {
     test_invalid_parameters(r, "index8.png");
     test_invalid_parameters(r, "mandrill.wbmp");
 }