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 10 matching lines @@
 #include "SkPictureData.h"
 #include "SkPictureRecorder.h"
 #include "SkRandom.h"
 #include "SkRecordDraw.h"
 #include "SkRecorder.h"
 #include "SkSVGCanvas.h"
 #include "SkScaledCodec.h"
 #include "SkStream.h"
 #include "SkTLogic.h"
 #include "SkXMLWriter.h"
+#include "SkScaledCodec.h"
+#include "SkSwizzler.h"
 
 DEFINE_bool(multiPage, false, "For document-type backends, render the source"
             " into multiple pages");
 
 static bool lazy_decode_bitmap(const void* src, size_t size, SkBitmap* dst) {
     SkAutoTUnref<SkData> encoded(SkData::NewWithCopy(src, size));
     return encoded && SkDEPRECATED_InstallDiscardablePixelRef(encoded, dst);
 }
 
 namespace DM {
@@ skipping 292 matching lines @@
             }
             canvas->drawBitmap(bitmap, 0, 0);
             break;
         }
         case kScanline_Mode: {
             if (SkCodec::kSuccess != codec->startScanlineDecode(decodeInfo, NULL, colorPtr,
                                                                 colorCountPtr)) {
                 return Error::Nonfatal("Could not start scanline decoder");
             }
 
-            SkCodec::Result result = SkCodec::kUnimplemented;
+            void* dst = bitmap.getAddr(0, 0);
+            size_t rowBytes = bitmap.rowBytes();
+            uint32_t height = decodeInfo.height();
             switch (codec->getScanlineOrder()) {
                 case SkCodec::kTopDown_SkScanlineOrder:
                 case SkCodec::kBottomUp_SkScanlineOrder:
                 case SkCodec::kNone_SkScanlineOrder:
-                    result = codec->getScanlines(bitmap.getAddr(0, 0),
-                            decodeInfo.height(), bitmap.rowBytes());
+                    // We do not need to check the return value.  On an incomplete
+                    // image, memory will be filled with a default value.
+                    codec->getScanlines(dst, height, rowBytes);
                     break;
                 case SkCodec::kOutOfOrder_SkScanlineOrder: {
                     for (int y = 0; y < decodeInfo.height(); y++) {
-                        int dstY = codec->nextScanline();
+                        int dstY = codec->outputScanline(y);
                         void* dstPtr = bitmap.getAddr(0, dstY);
-                        result = codec->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);
-                        }
+                        // We complete the loop, even if this call begins to fail
+                        // due to incomplete image.  This ensures any uninitialized
+                        // memory will be filled with the proper value.
+                        codec->getScanlines(dstPtr, 1, bitmap.rowBytes());

[msarett, 2015/10/01 18:14:14]

Thanks for your thorough review of how the scanline tests (Scanline,
ScanlineSubset, and Stripe) handle incomplete images.  In the last patch set, I
didn't really feel strongly about whether they should succeed or fail (or even
that they be consistent) - but I think it is worth writing the code so that they
will fill uninitialized memory and succeed.

I have considered two ways to accomplish this:
(1) Call SkSwizzler::Fill() from here.  This is straightforward in most cases
and sometimes complex (ex: filling kOutOfOrder images).  It is also generally a
duplication of code in SkCodec.cpp.
(2) Ignore result values for getScanlines() and skipScanlines().  If images are
incomplete, these calls may fail over and over again, but all the memory will
get filled.

I have chosen to go with (2).  I like that it makes the test code simpler to
write.  I don't feel strongly, so let me know if you prefer (1) or another
option altogether.

[scroggo, 2015/10/01 20:48:58]

I mentioned this as a response to an older comment, so sorry for the
duplication: It seems a little weird to me that you can call getScanlines, get 0
for some kind of failure, and then keep calling it. On the other hand, I can see
the convenience. I like the comments here, and there should probably also be
comments on SkCodec::getScanlines.

Thinking about this a little more, it seems a little weird that getScanlines
returns 0 when it actually wrote to countLines lines.

Also, is the number of lines just returned so the client can decide whether they
want to use the incomplete image? If they decided not to, it seems like we
wasted time filling the rest of the image.

[msarett, 2015/10/01 22:34:51]

Thanks for your thoughts.  I'll respond to this as a comment on the patch set.

                     }
                     break;
                 }
             }
 
-            switch (result) {
-                case SkCodec::kSuccess:
-                case SkCodec::kIncompleteInput:
-                    break;
-                default:
-                    return SkStringPrintf("%s failed with error message %d",
-                                          fPath.c_str(), (int) result);
-            }
             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) {
                 return Error::Nonfatal(SkStringPrintf("Cannot decode subset: divisor %d is too big"
@@ skipping 39 matching lines @@
                                                                         colorCountPtr)
                             // TODO (msarett): Support this mode for all scanline orderings.
                             || SkCodec::kTopDown_SkScanlineOrder != codec->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 = codec->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);
-                    }
+                    // Skip to the first line of subset.  We ignore the result value here.
+                    // If the skip value fails, this will indicate an incomplete image.
+                    // This means that the call to getScanlines() will also fail, but it
+                    // will fill the buffer with a default value, so we can still draw the
+                    // image.
+                    codec->skipScanlines(y);
+
                     //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 =
-                            codec->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);
-                    }
+                    codec->getScanlines(buffer, currentSubsetHeight, rowBytes);
+
                     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.
-                    */
                     char* bufferRow = buffer;
                     for (int subsetY = 0; subsetY < currentSubsetHeight; ++subsetY) {
                         memcpy(subsetBm.getAddr(0, subsetY), bufferRow + x*bpp,
                                 currentSubsetWidth*bpp);
                         bufferRow += rowBytes;
                     }
 
                     subsetBm.notifyPixelsChanged();
                     canvas->drawBitmap(subsetBm, SkIntToScalar(x), SkIntToScalar(y));
                 }
             }
             break;
         }
         case kStripe_Mode: {
             const int height = decodeInfo.height();
             // This value is chosen arbitrarily.  We exercise more cases by choosing a value that
             // does not align with image blocks.
             const int stripeHeight = 37;
             const int numStripes = (height + stripeHeight - 1) / stripeHeight;
 
             // Decode odd stripes
             if (SkCodec::kSuccess != codec->startScanlineDecode(decodeInfo, NULL, colorPtr,
                                                                 colorCountPtr)
                     || SkCodec::kTopDown_SkScanlineOrder != codec->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");
             }
+
             for (int i = 0; i < numStripes; i += 2) {
                 // Skip a stripe
                 const int linesToSkip = SkTMin(stripeHeight, height - i * stripeHeight);
-                SkCodec::Result result = codec->skipScanlines(linesToSkip);
-                switch (result) {
-                    case SkCodec::kSuccess:
-                    case SkCodec::kIncompleteInput:
-                        break;
-                    default:
-                        return SkStringPrintf("Cannot skip scanlines for %s.", fPath.c_str());
-                }
+                codec->skipScanlines(linesToSkip);
 
                 // Read a stripe
                 const int startY = (i + 1) * stripeHeight;
                 const int linesToRead = SkTMin(stripeHeight, height - startY);
                 if (linesToRead > 0) {
-                    result = codec->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());
-                    }
+                    codec->getScanlines(bitmap.getAddr(0, startY), linesToRead, bitmap.rowBytes());
                 }
             }
 
             // Decode even stripes
             const SkCodec::Result startResult = codec->startScanlineDecode(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 = codec->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());
-                }
+                codec->getScanlines(bitmap.getAddr(0, startY), linesToRead, bitmap.rowBytes());
 
                 // Skip a stripe
                 const int linesToSkip = SkTMin(stripeHeight, height - (i + 1) * stripeHeight);
                 if (linesToSkip > 0) {
-                    result = codec->skipScanlines(linesToSkip);
-                    switch (result) {
-                        case SkCodec::kSuccess:
-                        case SkCodec::kIncompleteInput:
-                            break;
-                        default:
-                            return SkStringPrintf("Cannot skip scanlines for %s.", fPath.c_str());
-                    }
+                    codec->skipScanlines(linesToSkip);
                 }
             }
             canvas->drawBitmap(bitmap, 0, 0);
             break;
         }
         case kSubset_Mode: {
             // Arbitrarily choose a divisor.
             int divisor = 2;
             // Total width/height of the image.
             const int W = codec->getInfo().width();
@@ skipping 689 matching lines @@
             skr.visit<void>(i, drawsAsSingletonPictures);
         }
         SkAutoTUnref<SkPicture> macroPic(macroRec.endRecordingAsPicture());
 
         canvas->drawPicture(macroPic);
         return "";
     });
 }
 
 }  // namespace DM