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 1a901a97ac4314b210dd6deeeebf14d4957760e9..4bd9810079a4a2758de6272d66e7ebc2ee3d5e89 100644
--- a/src/codec/SkCodec.cpp
+++ b/src/codec/SkCodec.cpp
@@ -167,11 +167,26 @@ SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t
         return kInvalidScale;
     }
 
-    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.colorType(), info.alphaType(), pixels, info.width(),

[scroggo, 2015/10/07 18:46:39]

Generally speaking, it is better (less error-prone) to have less parameters to a
function. Sometimes we have to have a lot, but here it seems like we could just
use an SkImageInfo to include info.colorType(), info.alphaType(), info.width(),
and info.height().

A downside is that in some places you don't already have an info to pass, so
you'd have to create a new one.

[msarett, 2015/10/07 20:53:23]

Yeah I've gone back and forth on this...but I think you're right.  Will change.

+                rowBytes, options->fZeroInitialized, info.height(), rowsDecoded);
+    }
+
     return result;
 }
 
@@ -233,36 +248,95 @@ 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) {
+int SkCodec::getScanlines(void* dst, int countLines, size_t rowBytes) {
     if (fCurrScanline < 0) {
-        return kScanlineDecodingNotStarted;
+        return 0;
     }
 
     SkASSERT(!fDstInfo.isEmpty());
-
     if (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().colorType(), this->dstInfo().alphaType(), dst,
+                this->dstInfo().width(), rowBytes, this->options().fZeroInitialized, countLines,
+                linesDecoded);
+    }
+    fCurrScanline += linesDecoded;

[scroggo, 2015/10/07 18:46:39]

Refresh my memory - updating fCurrScanline, even when the image has no more
rows, allows the client to keep calling getScanlines to fill the rest of their
memory, right?

[msarett, 2015/10/07 20:53:23]

I like that we update fCurrScanline here to the "right" value regardless of how
many scanlines we are able to get.

I think that we could choose to only update it on success and there would be no
behavior change.

Note that if we fail to decode any lines, fCurrScanline will stay the same.

+    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()) {
+    if (countLines < 0 || 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;

[scroggo, 2015/10/07 18:46:39]

I notice that incomplete from getScanlines updates fCurrScanline, while in
skipScanlines it does not. Is there a reason for this difference? Maybe this is
because onSkipScanlines does not tell how many it successfully skipped? Are
there any consequences of this? (e.g. if someone keeps calling get/skip after
they've run out of data, does this mean fCurrScanline will get updated
differently, so it may not work right for kOutOfOrder?)

[msarett, 2015/10/07 20:53:23]

I had argued that onSkipScanlines() should return an int, so we would be able to
update fCurrScanlines to the "correct" value here.  We decided that it was fine
to return a bool, since it is an error when skipScanlines() fails and we will
need not to use this field again anyway.
There is a subtle distinction here that may be a little bit confusing.

The client can't trust fCurrScanline to keep incrementing on incomplete decodes,
whether they are calling get() or skip().  Once it fails, fCurrScanlines stops
incrementing.  So then, the nextScanline() API stops working (maybe we should
keep incrementing fCurrScanline no matter what, so nextScanline() always works).

In order to fill remaining lines one-by-one the client needs to call
outputScanline(y).  By passing in the y value, it is not depending on
fCurrScanline being correct.

After writing all of that, I think the right thing to do is always update
fCurrScanline by countLines, so it always keeps track of the number of scanlines
that have been asked for.  That way nextScanline() and outputScanline(y) will
both always work properly.  Also, if the client asks for too many lines, we will
catch it.  This would have saved us in the SubsetBench bug.

+    }
     fCurrScanline += countLines;
-    return result;
+    return true;
+}
+
+int SkCodec::outputScanline(int inputScanline) const {
+    SkASSERT(0 <= inputScanline && inputScanline < this->getInfo().height());

[scroggo, 2015/10/07 18:46:39]

nit:Do we need this assert in both places?

[msarett, 2015/10/07 20:53:23]

No, you're right we don't.

+    return this->onOutputScanline(inputScanline);
+}
+
+int SkCodec::onOutputScanline(int inputScanline) const {
+    SkASSERT(0 <= inputScanline && inputScanline < this->getInfo().height());
+    switch (this->getScanlineOrder()) {
+        case kTopDown_SkScanlineOrder:
+        case kNone_SkScanlineOrder:
+            return inputScanline;
+        case kBottomUp_SkScanlineOrder:
+            return this->getInfo().height() - inputScanline - 1;
+        case kOutOfOrder_SkScanlineOrder:
+            // This case indicates an interlaced gif and is implemented by SkGifCodec.
+            SkASSERT(false);
+            return 0;
+    }
+}
+
+void SkCodec::fillIncompleteImage(SkColorType colorType, SkAlphaType alphaType, void* dst,
+        int width, size_t rowBytes, ZeroInitialized zeroInit, int linesRequested,
+        int linesDecoded) {
+
+    SkSampler* sampler = this->getSampler(false);

[scroggo, 2015/10/07 18:46:39]

Is this the only place where we need to get the sampler and read its scaled
width? This is a step in the right direction. What if, instead of reading this
value, we just told the sampler to do the filling? It seems like it already
knows its width, so we can just tell it to fill so many lines, and it knows how
to deal with that.

(If there's no sampler, which is just in the case of jpeg, I think, then we'll
have to fall back to calling fill directly, but in that case we know the output
width. Maybe this leads to some duped code?)

[msarett, 2015/10/07 20:53:23]

Yes we can do this, but the switch statement will be duplicated.

I do think this might be better...let me know what you think.

+    width = sampler ? sampler->getScaledWidth() : width;

[scroggo, 2015/10/07 18:46:39]

Alternatively, this could say:

if (sampler) {
  width = sampler->getScaledWidth();
}

Don't know if that makes a big difference. (I think it might be a little clearer
for me, but maybe not a big deal.)

[msarett, 2015/10/07 20:53:23]

Will make it clearer.

+
+    void* fillDst;
+    const uint32_t fillValue = this->getFillValue(colorType, alphaType);
+    const int linesRemaining = linesRequested - linesDecoded;
+
+    switch (this->getScanlineOrder()) {
+        case kTopDown_SkScanlineOrder:
+        case kNone_SkScanlineOrder:
+            fillDst = SkTAddOffset<void>(dst, linesDecoded * rowBytes);
+            SkSampler::Fill(fillDst, colorType, width, linesRemaining, rowBytes, fillValue,
+                    zeroInit);
+            break;
+        case kBottomUp_SkScanlineOrder:
+            fillDst = dst;
+            SkSampler::Fill(fillDst, colorType, width, linesRemaining, rowBytes, fillValue,
+                    zeroInit);
+            break;
+        case kOutOfOrder_SkScanlineOrder:
+            SkASSERT(1 == linesRequested || this->getInfo().height() == linesRequested);
+            for (int srcY = linesDecoded; srcY < linesRequested; srcY++) {
+                fillDst = SkTAddOffset<void>(dst, this->outputScanline(srcY) * rowBytes);
+                SkSampler::Fill(fillDst, colorType, width, 1, rowBytes, fillValue, zeroInit);
+            }
+            break;
+    }
 }