Fill incomplete images in SkCodec parent class

src/codec/SkCodec.cpp

Index: src/codec/SkCodec.cpp
diff --git a/src/codec/SkCodec.cpp b/src/codec/SkCodec.cpp
index cabcadcccda5c872b69834daa18bbe1701be82fd..dff9a3454c81411f47a936925526b28c18e7f789 100644
--- a/src/codec/SkCodec.cpp
+++ b/src/codec/SkCodec.cpp
@@ -153,11 +153,27 @@ SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t
     if (nullptr == options) {
         options = &optsStorage;
     }
-    const Result result = this->onGetPixels(info, pixels, rowBytes, *options, ctable, ctableCount);
+
+    // On an incomplete decode, the subclass will specify the number of scanlines that it decoded
+    // successfully.
+    int rowsDecoded = 0;
+    const Result result = this->onGetPixels(info, pixels, rowBytes, *options, ctable, ctableCount,
+            &rowsDecoded);
 
     if ((kIncompleteInput == result || kSuccess == result) && ctableCount) {
         SkASSERT(*ctableCount >= 0 && *ctableCount <= 256);
     }
+
+    // A return value of kIncompleteInput indicates a truncated image stream.
+    // In this case, we will fill any uninitialized memory with a default value.
+    // Some subclasses will take care of filling any uninitialized memory on
+    // their own.  They indicate that all of the memory has been filled by
+    // setting rowsDecoded equal to the height.
+    if (kIncompleteInput == result && rowsDecoded != info.height()) {
+        this->fillIncompleteImage(info, pixels, rowBytes, options->fZeroInitialized,
+                info.height(), rowsDecoded);
+    }
+
     return result;
 }
 
@@ -207,36 +223,84 @@ SkCodec::Result SkCodec::startScanlineDecode(const SkImageInfo& dstInfo) {
     return this->startScanlineDecode(dstInfo, nullptr, nullptr, nullptr);
 }
 
-SkCodec::Result SkCodec::getScanlines(void* dst, int countLines, size_t rowBytes) {
+uint32_t SkCodec::getScanlines(void* dst, int countLines, size_t rowBytes) {

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

If we wanted to, we could make getScanlines return a Result (so it could return
errors for invalid params and decoding not started), and onGetScanlines return a
number of lines decoded. Maybe the caller does not care exactly how many lines
were decoded? (On the other hand, maybe it is nice for the client to know how
many lines were decoded, so they can decide whether they want to display the
partial image. getPixels does not provide that information, so maybe this one
shouldn't either?)

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

It's fair to point out that this API change is no longer necessary now that
SkCodec and SkScanlineDecoder are merged.  And now this API provides more
information than getPixels(), which might be weird.

So I do think it is worth considering this API change separately from the rest
of this CL.

I would argue that we should make this change because it provides more useful
information for an intelligent client.  The enum provides more useful
information for a dumb client, which I would argue is less important.

At this point, we are still the only client.  Looking at how we use this API, it
doesn't seem to make a ton of difference either way.

     if (fCurrScanline < 0) {
-        return kScanlineDecodingNotStarted;

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

Do we still need this enum value?

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

Looks like we don't.

+        return 0;
     }
 
     SkASSERT(!fDstInfo.isEmpty());
     if ((rowBytes < fDstInfo.minRowBytes() && countLines > 1 ) || countLines <= 0
          || fCurrScanline + countLines > fDstInfo.height()) {
-        return kInvalidParameters;
+        return 0;
     }
 
-    const Result result = this->onGetScanlines(dst, countLines, rowBytes);
-    fCurrScanline += countLines;
-    return result;
+    const uint32_t linesDecoded = this->onGetScanlines(dst, countLines, rowBytes);
+    if (linesDecoded < countLines) {
+        this->fillIncompleteImage(this->dstInfo(), dst, rowBytes, this->options().fZeroInitialized,
+                countLines, linesDecoded);
+    }
+    fCurrScanline += linesDecoded;
+    return linesDecoded;
 }
 
-SkCodec::Result SkCodec::skipScanlines(int countLines) {
+bool SkCodec::skipScanlines(int countLines) {
     if (fCurrScanline < 0) {
-        return kScanlineDecodingNotStarted;
+        return false;
     }
 
     SkASSERT(!fDstInfo.isEmpty());
     if (fCurrScanline + countLines > fDstInfo.height()) {
         // Arguably, we could just skip the scanlines which are remaining,
-        // and return kSuccess. We choose to return invalid so the client
+        // and return true. We choose to return false so the client
         // can catch their bug.
-        return SkCodec::kInvalidParameters;
+        return false;
     }
 
-    const Result result = this->onSkipScanlines(countLines);
+    if (!this->onSkipScanlines(countLines)) {
+        return false;
+    }
     fCurrScanline += countLines;
-    return result;
+    return true;
+}
+
+int SkCodec::nextScanline(int srcY) const {
+    SkASSERT(0 <= srcY && srcY < this->getInfo().height());
+    switch (this->getScanlineOrder()) {
+        case kTopDown_SkScanlineOrder:
+        case kNone_SkScanlineOrder:
+            return srcY;
+        case kBottomUp_SkScanlineOrder:
+            return this->dstInfo().height() - srcY - 1;
+        case kOutOfOrder_SkScanlineOrder:
+            return get_output_row_interlaced(srcY, this->getInfo().height());

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

It's odd to me that kBottomUp takes the dstInfo into account, but kOutOfOrder
does not.

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

We can use getInfo() in both places.  This is probably best to avoid confusion
(and bugs).  If we ever wanted to call nextScanline() from onGetPixels for a
kBottomUp image, this would have failed because dstInfo() would have been empty.

+    }
+}
+
+void SkCodec::fillIncompleteImage(const SkImageInfo& info, void* dst, size_t rowBytes,
+        ZeroInitialized zeroInit, int linesRequested, int linesDecoded) {
+    void* fillDst;
+    SkImageInfo fillInfo;
+    const uint32_t fillValue = this->getFillValue(info.colorType(), info.alphaType());
+    const int linesRemaining = linesRequested - linesDecoded;
+    switch (this->getScanlineOrder()) {
+        case kTopDown_SkScanlineOrder:
+        case kNone_SkScanlineOrder:
+            fillDst = SkTAddOffset<void>(dst, linesDecoded * rowBytes);
+            fillInfo = info.makeWH(info.width(), linesRemaining);
+            SkSwizzler::Fill(fillDst, fillInfo, rowBytes, fillValue, zeroInit);
+            break;
+        case kBottomUp_SkScanlineOrder:
+            fillDst = dst;
+            fillInfo = info.makeWH(info.width(), linesRemaining);
+            SkSwizzler::Fill(fillDst, fillInfo, rowBytes, fillValue, zeroInit);
+            break;
+        case kOutOfOrder_SkScanlineOrder:
+            SkASSERT(1 == linesRequested || this->getInfo().height() == linesRequested);

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

Do you need the second half? It seems like if it's true the first half will be,
too?

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

I don't understand.  linesRequested can be 1 or the full height.  I think we
need both.

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

Oh, I think I was confused. This is just to discourage a dumb client from asking
for more than one line but less than all of them for interlaced GIF?

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

Yes!

+            fillInfo = info.makeWH(info.width(), 1);
+            for (int srcY = linesDecoded; srcY < linesRequested; srcY++) {
+                fillDst = SkTAddOffset<void>(dst, this->nextScanline(srcY) * rowBytes);
+                SkSwizzler::Fill(fillDst, fillInfo, rowBytes, fillValue, zeroInit);
+            }
+            break;
+    }
 }