Fill incomplete images in SkCodec parent class

include/codec/SkScanlineDecoder.h

Index: include/codec/SkScanlineDecoder.h
diff --git a/include/codec/SkScanlineDecoder.h b/include/codec/SkScanlineDecoder.h
index 61184d644a15d9043e2fef92f3aa1e54a01e21e8..29e003aa90720172e6cd5b6c904ca42e1ea1b78c 100644
--- a/include/codec/SkScanlineDecoder.h
+++ b/include/codec/SkScanlineDecoder.h
@@ -100,16 +100,17 @@ public:
      *  @param countLines Number of lines to write.
      *  @param rowBytes Number of bytes per row. Must be large enough to hold
      *      a scanline based on the SkImageInfo used to create this object.
+     *  @return the number of lines successfully decoded
      */
-    SkCodec::Result getScanlines(void* dst, int countLines, size_t rowBytes) {
+    uint32_t getScanlines(void* dst, int countLines, size_t rowBytes) {
         SkASSERT(!fDstInfo.isEmpty());
         if ((rowBytes < fDstInfo.minRowBytes() && countLines > 1 ) || countLines <= 0
                 || fCurrScanline + countLines > fDstInfo.height()) {
             return SkCodec::kInvalidParameters;
         }
-        const SkCodec::Result result = this->onGetScanlines(dst, countLines, rowBytes);
-        fCurrScanline += countLines;
-        return result;
+        uint32_t decodedLines = this->onGetScanlines(dst, countLines, rowBytes);
+        fCurrScanline += decodedLines;
+        return decodedLines;
     }
 
     /**
@@ -121,8 +122,10 @@ public:
      *  NOTE: If skipped lines are the only lines with alpha, this default
      *  will make reallyHasAlpha return true, when it could have returned
      *  false.
+     *
+     *  @return the number of lines successfully skipped

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

Is this useful to know? It seems like skipping partway is no better than
skipping none.

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

Um no this isn't useful.  I'm making this return a bool.

I considered going further and making onSkipScanlines also return a bool, but
this prevents us from updating fCurrScanline.  Any thoughts?

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

I think they should return the same value. If you want onSkipScanlines to return
a bool, you'll just add countLines (like we were doing before).

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

Will change, I thought returning different values was a bit strange as well.

On the other hand, we may skip "some" of the count lines before failing.  In
this case, fCurrScanline will not be updated properly.  This is not a big deal
because we are treating failures to skip scanlines as errors.  But if we were,
for some reason, to continue using the scanline decoder after a failed skip,
this would be a bug.

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

In general, I think it's better to report failures early. In the case of
skipping some of the lines, but not all, it seems like the next call to either
get or skip should definitely fail, so we might as well report that now.

I thought perhaps we should set fCurrScanline to -1, so the next call will
immediately return false - that would make us fail sooner next time, but the
next call ought to fail anyway, so it might be unnecessary (and perhaps
confusing if someone was debugging and trying to figure out why start had not
been called).

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

Gotcha.  For now, I'm happy with it as is.

      */
-    SkCodec::Result skipScanlines(int countLines) {
+    uint32_t skipScanlines(int countLines) {
         SkASSERT(!fDstInfo.isEmpty());
         if (fCurrScanline + countLines > fDstInfo.height()) {
             // Arguably, we could just skip the scanlines which are remaining,
@@ -130,9 +133,9 @@ public:
             // can catch their bug.
             return SkCodec::kInvalidParameters;
         }
-        const SkCodec::Result result = this->onSkipScanlines(countLines);
-        fCurrScanline += countLines;
-        return result;
+        uint32_t skippedLines = this->onSkipScanlines(countLines);
+        fCurrScanline += skippedLines;
+        return skippedLines;
     }
 
     /**
@@ -220,19 +223,39 @@ public:
     SkScanlineOrder getScanlineOrder() const { return this->onGetScanlineOrder(); }
 
     /**
-     *  Returns the y-coordinate of the next row to be returned by the scanline
+     *  Returns the y-coordinate of the next dst row to be returned by the scanline
      *  decoder.  This will be overridden in the case of
-     *  kOutOfOrder_SkScanlineOrder and should be unnecessary in the case of
-     *  kNone_SkScanlineOrder.
+     *  kOutOfOrder_SkScanlineOrder or kBottomUp_SkScanlineOrder, and should be
+     *  unnecessary in the case of kNone_SkScanlineOrder.
      */
     int getY() const {
         SkASSERT(kNone_SkScanlineOrder != this->getScanlineOrder());
-        return this->onGetY();
+        return this->onGetY(fCurrScanline);

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

Could this function be non-virtual, and just depend on this->getScanlineOrder?

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

Yes I think so!  Will do.

The one thing that may be considered a drawback is that we will need to share
some code that is really only relevant to interlaced gifs.

+    }
+
+    /**
+     *  Returns the y-coordinate of the decoded row that corresponds to the
+     *  input encoded y-coordinate.  This will be overridden in the case of
+     *  kOutOfOrder_SkScanlineOrder or kBottomUp_SkScanlineOrder, and should be
+     *  unnecessary in the case of kNone_SkScanlineOrder.
+     */
+    int getY(int encodedY) const {

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

I find the name encodedY confusing. Maybe srcY? I'm curious why this is needed,
but I suppose I'll find out by the time I finish reviewing this...

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

In order to Fill() for kOutOfOrder images, we need to be able to continue to
iterate over rows, even when fCurrScanline stops advancing.

+        SkASSERT(kNone_SkScanlineOrder != this->getScanlineOrder());
+        return this->onGetY(encodedY);
+    }
+
+    uint32_t getFillValue(const SkImageInfo& dstInfo) const {
+        return fCodec->getFillValue(dstInfo);

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

This stuff will become cleaner once I merge codec and scanline decoder...

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

:)

+    }
+
+    void* getFillDst(void* dstStart, size_t rowBytes, uint32_t decodedScanlines) const {
+        return fCodec->getFillDst(dstStart, rowBytes, decodedScanlines);
     }
 
 protected:
-    SkScanlineDecoder(const SkImageInfo& srcInfo)
-        : fSrcInfo(srcInfo)
+    SkScanlineDecoder(SkCodec* codec, const SkImageInfo& srcInfo)
+        : fCodec(codec)
+        , fSrcInfo(srcInfo)
         , fDstInfo()
         , fOptions()
         , fCurrScanline(0) {}
@@ -254,24 +277,25 @@ protected:
     /**
      *  Most images will be kTopDown and will not need to override this function.
      */
-    virtual int onGetY() const { return fCurrScanline; }
+    virtual int onGetY(int encodedY) const { return encodedY; }
 
     const SkImageInfo& dstInfo() const { return fDstInfo; }
 
     const SkCodec::Options& options() const { return fOptions; }
 
 private:
-    const SkImageInfo   fSrcInfo;
-    SkImageInfo         fDstInfo;
-    SkCodec::Options    fOptions;
-    int                 fCurrScanline;
+    SkAutoTDelete<SkCodec> fCodec;
+    const SkImageInfo      fSrcInfo;
+    SkImageInfo            fDstInfo;
+    SkCodec::Options       fOptions;
+    int                    fCurrScanline;
 
     virtual SkCodec::Result onStart(const SkImageInfo& dstInfo,
                                     const SkCodec::Options& options,
                                     SkPMColor ctable[], int* ctableCount) = 0;
 
     // Naive default version just calls onGetScanlines on temp memory.
-    virtual SkCodec::Result onSkipScanlines(int countLines) {
+    virtual uint32_t onSkipScanlines(int countLines) {
         SkAutoMalloc storage(fDstInfo.minRowBytes());
         // Note that we pass 0 to rowBytes so we continue to use the same memory.
         // Also note that while getScanlines checks that rowBytes is big enough,
@@ -281,8 +305,7 @@ private:
         return this->onGetScanlines(storage.get(), countLines, 0);
     }
 
-    virtual SkCodec::Result onGetScanlines(void* dst, int countLines,
-                                                    size_t rowBytes) = 0;
+    virtual uint32_t onGetScanlines(void* dst, int countLines, size_t rowBytes) = 0;
 
 };
 #endif // SkScanlineDecoder_DEFINED