Fill incomplete images in SkCodec parent class

dm/DMSrcSink.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 "DMSrcSink.h"
 #include "SamplePipeControllers.h"
 #include "SkCodec.h"
@@ skipping 334 matching lines @@
             canvas->drawBitmap(bitmap, 0, 0);
             break;
         }
         case kScanline_Mode: {
             SkAutoTDelete<SkScanlineDecoder> scanlineDecoder(
                     start_scanline_decoder(encoded.get(), decodeInfo, colorPtr, colorCountPtr));
             if (nullptr == scanlineDecoder) {
                 return Error::Nonfatal("Could not start scanline decoder");
             }
 
-            SkCodec::Result result = SkCodec::kUnimplemented;
+            uint32_t lines = 0;

[scroggo, 2015/09/22 18:02:48]

Maybe this should be linesDecoded?

[msarett, 2015/09/23 13:22:39]

Done.

             switch (scanlineDecoder->getScanlineOrder()) {
                 case SkScanlineDecoder::kTopDown_SkScanlineOrder:
                 case SkScanlineDecoder::kBottomUp_SkScanlineOrder:
                 case SkScanlineDecoder::kNone_SkScanlineOrder:
-                    result = scanlineDecoder->getScanlines(bitmap.getAddr(0, 0),
+                    lines = scanlineDecoder->getScanlines(bitmap.getAddr(0, 0),
                             decodeInfo.height(), bitmap.rowBytes());
                     break;
                 case SkScanlineDecoder::kOutOfOrder_SkScanlineOrder: {
                     for (int y = 0; y < decodeInfo.height(); y++) {
                         int dstY = scanlineDecoder->getY();
                         void* dstPtr = bitmap.getAddr(0, dstY);
-                        result = scanlineDecoder->getScanlines(dstPtr, 1, bitmap.rowBytes());
-                        if (SkCodec::kSuccess != result && SkCodec::kIncompleteInput != result) {
-                            return SkStringPrintf("%s failed with error message %d",
-                                                  fPath.c_str(), (int) result);
+                        uint32_t line = scanlineDecoder->getScanlines(dstPtr, 1, bitmap.rowBytes());
+                        if (1 != line) {
+                            break;
                         }
+                        lines += line;
                     }
                     break;
                 }
             }
-
-            switch (result) {
-                case SkCodec::kSuccess:
-                case SkCodec::kIncompleteInput:
-                    break;
-                default:
-                    return SkStringPrintf("%s failed with error message %d",
-                                          fPath.c_str(), (int) result);
+            if ((int) lines < decodeInfo.height()) {
+                // We have not implemented a function in SkScanlineDecoder to fill remaining
+                // scanlines on an incomplete decode.  As long as we are the only client that
+                // uses SkScanlineDecoder, I think this is ok.  If this changes, maybe we want
+                // to consider providing an API for this?
+                return Error::Nonfatal("Image is incomplete, could not get scanlines");
             }
             canvas->drawBitmap(bitmap, 0, 0);
             break;
         }
         case kScanline_Subset_Mode: {
             //this mode decodes the image in divisor*divisor subsets, using a scanline decoder
             const int divisor = 2;
             const int w = decodeInfo.width();
             const int h = decodeInfo.height();
             if (divisor > w || divisor > h) {
@@ skipping 42 matching lines @@
                     if (nullptr == decoder || SkScanlineDecoder::kTopDown_SkScanlineOrder !=
                             decoder->getScanlineOrder()) {
                         if (x == 0 && y == 0) {
                             //first try, image may not be compatible
                             return Error::Nonfatal("Could not start top-down scanline decoder");
                         } else {
                             return "Error scanline decoder is nullptr";
                         }
                     }
                     //skip to first line of subset
-                    const SkCodec::Result skipResult = decoder->skipScanlines(y);
-                    switch (skipResult) {
-                        case SkCodec::kSuccess:
-                        case SkCodec::kIncompleteInput:
-                            break;
-                        default:
-                            return SkStringPrintf("%s failed after attempting to skip %d scanlines"
-                                    "with error message %d", fPath.c_str(), y, (int) skipResult);
+                    int lines = decoder->skipScanlines(y);
+                    if (lines < y) {
+                        return Error::Nonfatal("Image is incomplete, could not get scanlines");
                     }
                     //create and set size of subsetBm
                     SkBitmap subsetBm;
                     SkIRect bounds = SkIRect::MakeWH(subsetWidth, subsetHeight);
                     bounds.setXYWH(0, 0, currentSubsetWidth, currentSubsetHeight);
                     SkAssertResult(largestSubsetBm.extractSubset(&subsetBm, bounds));
                     SkAutoLockPixels autlockSubsetBm(subsetBm, true);
-                    const SkCodec::Result subsetResult =
-                            decoder->getScanlines(buffer, currentSubsetHeight, rowBytes);
-                    switch (subsetResult) {
-                        case SkCodec::kSuccess:
-                        case SkCodec::kIncompleteInput:
-                            break;
-                        default:
-                            return SkStringPrintf("%s failed with error message %d",
-                                    fPath.c_str(), (int) subsetResult);
+                    lines = decoder->getScanlines(buffer, currentSubsetHeight, rowBytes);
+                    if (lines < currentSubsetHeight) {
+                        return Error::Nonfatal("Image is incomplete, could not get scanlines");

[scroggo, 2015/09/22 18:02:48]

This is going to change the behavior, right? Previously we did the same thing on
kSuccess and kIncompleteInput, but now we won't ever draw it and be able to look
at it in gold.

(Maybe this is okay, and correct, since we won't have filled in the rest of the
image, so we'll get uninitialized memory?)

[msarett, 2015/09/23 13:22:39]

Yes I should have been more explicit about behavior changes.

Before this CL, getScanlines() would fill the buffer on kIncompleteInput.  Now
getScanlines() just returns the number of lines successfully decoded and leaves
the remaining lines uninitialized.

Not sure if this is "correct", but this is what I intended.  Clients of
SkScanlineDecoder (SkScaledCodec is currently the only one) can and should fill
the uninitialized lines.  But a side effect is that we can't test the
SkScanlineDecoder on incomplete images from DM, which I think is ok.

[scroggo, 2015/09/25 15:55:05]

Sure we can. We just need to make DM fill the buffer, now that it knows how many
scanlines were decoded.

[msarett, 2015/10/01 12:44:51]

kScanlineSubset_Mode now fills on incomplete images, instead of failing.  For
consistency, kScanline_Mode has also been modified to fill incomplete images.

                     }
                     const size_t bpp = decodeInfo.bytesPerPixel();
                     /*
                      * we copy all the lines at once becuase when calling getScanlines for
                      * interlaced pngs the entire image must be read regardless of the number
                      * of lines requested.  Reading an interlaced png in a loop, line-by-line, would
                      * decode the entire image height times, which is very slow
                      * it is aknowledged that copying each line as you read it in a loop
                      * may be faster for other types of images.  Since this is a correctness test
                      * that's okay.
@@ skipping 21 matching lines @@
             // Decode odd stripes
             SkAutoTDelete<SkScanlineDecoder> decoder(
                     start_scanline_decoder(encoded.get(), decodeInfo, colorPtr, colorCountPtr));
             if (nullptr == decoder ||
                     SkScanlineDecoder::kTopDown_SkScanlineOrder != decoder->getScanlineOrder()) {
                 // This mode was designed to test the new skip scanlines API in libjpeg-turbo.
                 // Jpegs have kTopDown_SkScanlineOrder, and at this time, it is not interesting
                 // to run this test for image types that do not have this scanline ordering.
                 return Error::Nonfatal("Could not start top-down scanline decoder");
             }
+            int lines;

[scroggo, 2015/09/22 18:02:48]

I would lean towards removing this variable and instead making each of the below
lines look like:

if (decoder->skip/getScanlines() < linesToRead) {
  return Error::Nonfatal
}

The reason is because its value is never reused - you assign to it each time you
read from it.

[msarett, 2015/09/23 13:22:39]

Done.

             for (int i = 0; i < numStripes; i += 2) {
                 // Skip a stripe
                 const int linesToSkip = SkTMin(stripeHeight, height - i * stripeHeight);
-                SkCodec::Result result = decoder->skipScanlines(linesToSkip);
-                switch (result) {
-                    case SkCodec::kSuccess:
-                    case SkCodec::kIncompleteInput:
-                        break;
-                    default:
-                        return SkStringPrintf("Cannot skip scanlines for %s.", fPath.c_str());
+                lines = decoder->skipScanlines(linesToSkip);
+                if (lines < linesToSkip) {
+                    return Error::Nonfatal("Image is incomplete, could not get scanlines");
                 }
 
                 // Read a stripe
                 const int startY = (i + 1) * stripeHeight;
                 const int linesToRead = SkTMin(stripeHeight, height - startY);
                 if (linesToRead > 0) {
-                    result = decoder->getScanlines(bitmap.getAddr(0, startY),
-                            linesToRead, bitmap.rowBytes());
-                    switch (result) {
-                        case SkCodec::kSuccess:
-                        case SkCodec::kIncompleteInput:
-                            break;
-                        default:
-                            return SkStringPrintf("Cannot get scanlines for %s.", fPath.c_str());
+                    lines = decoder->getScanlines(bitmap.getAddr(0, startY), linesToRead,
+                            bitmap.rowBytes());
+                    if (lines < linesToRead) {
+                        return Error::Nonfatal("Image is incomplete, could not get scanlines");
                     }
                 }
             }
 
             // Decode even stripes
             const SkCodec::Result startResult = decoder->start(decodeInfo, nullptr, colorPtr,
                                                                colorCountPtr);
             if (SkCodec::kSuccess != startResult) {
                 return "Failed to restart scanline decoder with same parameters.";
             }
             for (int i = 0; i < numStripes; i += 2) {
                 // Read a stripe
                 const int startY = i * stripeHeight;
                 const int linesToRead = SkTMin(stripeHeight, height - startY);
-                SkCodec::Result result = decoder->getScanlines(bitmap.getAddr(0, startY),
-                        linesToRead, bitmap.rowBytes());
-                switch (result) {
-                    case SkCodec::kSuccess:
-                    case SkCodec::kIncompleteInput:
-                        break;
-                    default:
-                        return SkStringPrintf("Cannot get scanlines for %s.", fPath.c_str());
+                lines = decoder->getScanlines(bitmap.getAddr(0, startY), linesToRead,
+                        bitmap.rowBytes());
+                if (lines < linesToRead) {
+                    return Error::Nonfatal("Image is incomplete, could not get scanlines");
                 }
 
                 // Skip a stripe
                 const int linesToSkip = SkTMin(stripeHeight, height - (i + 1) * stripeHeight);
                 if (linesToSkip > 0) {
-                    result = decoder->skipScanlines(linesToSkip);
-                    switch (result) {
-                        case SkCodec::kSuccess:
-                        case SkCodec::kIncompleteInput:
-                            break;
-                        default:
-                            return SkStringPrintf("Cannot skip scanlines for %s.", fPath.c_str());
+                    lines = decoder->skipScanlines(linesToSkip);
+                    if (lines < linesToSkip) {
+                        return Error::Nonfatal("Image is incomplete, could not get scanlines");
                     }
                 }
             }
             canvas->drawBitmap(bitmap, 0, 0);
             break;
         }
         case kSubset_Mode: {
             // Arbitrarily choose a divisor.
             int divisor = 2;
             // Total width/height of the image.
@@ skipping 683 matching lines @@
             skr.visit<void>(i, drawsAsSingletonPictures);
         }
         SkAutoTUnref<SkPicture> macroPic(macroRec.endRecordingAsPicture());
 
         canvas->drawPicture(macroPic);
         return "";
     });
 }
 
 }  // namespace DM