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());
+                    codec->getScanlines(dst, height, rowBytes);

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

getScanlines() now fills when it cannot get all of the lines, so we do not need
to inspect the return value.

                     break;
                 case SkCodec::kOutOfOrder_SkScanlineOrder: {
                     for (int y = 0; y < decodeInfo.height(); y++) {
-                        int dstY = codec->nextScanline();
+                        int dstY = codec->nextScanline(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);
-                        }
+                        codec->getScanlines(dstPtr, 1, bitmap.rowBytes());

[scroggo, 2015/10/01 14:48:31]

Don't we want to break out of the loop if 0 is returned?

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

We could do that.  But then we need to iterate over the remaining lines and fill
line by line.  This duplicates the fill code we already have in SkCodec.cpp. 
See Patch Set 4 for this implementation.

Alternatively, here we keep calling getScanlines, and every time it fails, it
will fill the line.  I know we've discussed that we don't really like this
approach.  I think I prefer it here because it simplifies our test code.  But I
don't feel strongly and would be happy to change.

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

Oh yeah, now I remember talking about this, but I don't remember if we came to a
decision.

I guess I sort of imagine an error to mean that calling getScanlines again does
not work, but you're right, this does simplify things here in this testing code.
Maybe add a comment explaining this?

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

Acknowledged.

                     }
                     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 40 matching lines @@
                             // 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);
+                    if (!codec->skipScanlines(y)) {
+                        return Error::Nonfatal("Image is incomplete, could not get scanlines");

[scroggo, 2015/10/01 14:48:31]

Since we've returned an error, we will not be able to look at the output, right?
I think we still want to draw the output.

Same for the rest of our incomplete images.

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

Yeah the way the test is written now, we will only see the output if the image
is at least half complete.  We can do better than this.

                     }
+
                     //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);

[scroggo, 2015/10/01 14:48:31]

Note that you do something different if skipScanlines reveals the image to be
incomplete versus when getScanlines does. I think this is the correct behavior
(continue)

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

Acknowledged.

+
                     const size_t bpp = decodeInfo.bytesPerPixel();
                     /*
                      * we copy all the lines at once becuase when calling getScanlines for

[scroggo, 2015/10/01 14:48:31]

This isn't due to your CL, but it seems like this comment no longer goes with
the code it applies to?

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

Yeah I'm a bit confused by this comment.  Removing it.

                      * 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,
@@ skipping 16 matching lines @@
 
             // 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());
+                if (!codec->skipScanlines(linesToSkip)) {
+                    return Error::Nonfatal("Image is incomplete, could not get scanlines");

[scroggo, 2015/10/01 14:48:31]

Not only was the image incomplete, but it was so incomplete that we did not
decode anything.

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

This could be improved as well.

                 }
 
                 // 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());
+                    if ((int) codec->getScanlines(bitmap.getAddr(0, startY), linesToRead,
+                            bitmap.rowBytes()) < linesToRead) {
+                        return Error::Nonfatal("Image is incomplete, could not get scanlines");
                     }
                 }
             }
 
             // 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());
+                if ((int) codec->getScanlines(bitmap.getAddr(0, startY), linesToRead,
+                        bitmap.rowBytes()) < 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 = codec->skipScanlines(linesToSkip);
-                    switch (result) {
-                        case SkCodec::kSuccess:
-                        case SkCodec::kIncompleteInput:
-                            break;
-                        default:
-                            return SkStringPrintf("Cannot skip scanlines for %s.", fPath.c_str());
+                    if (!codec->skipScanlines(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 690 matching lines @@
             skr.visit<void>(i, drawsAsSingletonPictures);
         }
         SkAutoTUnref<SkPicture> macroPic(macroRec.endRecordingAsPicture());
 
         canvas->drawPicture(macroPic);
         return "";
     });
 }
 
 }  // namespace DM