Merge SkCodec with SkScanlineDecoder

src/codec/SkCodec_libgif.cpp

Index: src/codec/SkCodec_libgif.cpp
diff --git a/src/codec/SkCodec_libgif.cpp b/src/codec/SkCodec_libgif.cpp
index d8889defafa310fe76f2ad2d18bc1d2b3a73480b..ad7fb5b578d56263a3963aa673ddd021053acda5 100644
--- a/src/codec/SkCodec_libgif.cpp
+++ b/src/codec/SkCodec_libgif.cpp
@@ -582,124 +582,94 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
     return kSuccess;
 }
 
-// TODO (msarett): skbug.com/3582
-//                 Should we implement reallyHasAlpha?  Or should we read extension blocks in the
-//                 header?  Or should we do both?
-
-class SkGifScanlineDecoder : public SkScanlineDecoder {
-public:
-    SkGifScanlineDecoder(const SkImageInfo& srcInfo, SkGifCodec* codec)
-        : INHERITED(srcInfo)
-        , fCodec(codec)
-    {}
-
-    SkEncodedFormat onGetEncodedFormat() const override {
-        return kGIF_SkEncodedFormat;
-    }
-
-    SkCodec::Result onStart(const SkImageInfo& dstInfo, const SkCodec::Options& opts,
-                            SkPMColor inputColorPtr[], int* inputColorCount) override {
-        SkCodec::Result result = fCodec->prepareToDecode(dstInfo, inputColorPtr, inputColorCount,
-                this->options());
-        if (SkCodec::kSuccess != result) {
-            return result;
-        }
+SkCodec::Result SkGifCodec::onStartScanlineDecode(const SkImageInfo& dstInfo,
+        const SkCodec::Options& opts, SkPMColor inputColorPtr[], int* inputColorCount) {
+    Result result = this->prepareToDecode(dstInfo, inputColorPtr, inputColorCount,
+                                          this->options());
+    if (kSuccess != result) {
+        return result;
+    }
 
-        // Check to see if scaling was requested.
-        if (dstInfo.dimensions() != this->getInfo().dimensions()) {
-            if (!SkScaledCodec::DimensionsSupportedForSampling(this->getInfo(), dstInfo)) {
-                return gif_error("Scaling not supported.\n", SkCodec::kInvalidScale);
-            }
+    // Check to see if scaling was requested.
+    if (dstInfo.dimensions() != this->getInfo().dimensions()) {
+        if (!SkScaledCodec::DimensionsSupportedForSampling(this->getInfo(), dstInfo)) {
+            return gif_error("Scaling not supported.\n", SkCodec::kInvalidScale);
         }
+    }
 
-        // Initialize the swizzler
-        if (fCodec->fFrameIsSubset) {
-            int sampleX;
-            SkScaledCodec::ComputeSampleSize(dstInfo, fCodec->getInfo(), &sampleX, NULL);
-            const SkImageInfo subsetDstInfo = dstInfo.makeWH(
-                    get_scaled_dimension(fCodec->fFrameDims.width(), sampleX),
-                    fCodec->fFrameDims.height());
-            if (SkCodec::kSuccess != fCodec->initializeSwizzler(subsetDstInfo,
-                    opts.fZeroInitialized)) {
-                return gif_error("Could not initialize swizzler.\n", SkCodec::kUnimplemented);
-            }
-        } else {
-            if (SkCodec::kSuccess != fCodec->initializeSwizzler(dstInfo, opts.fZeroInitialized)) {
-                return gif_error("Could not initialize swizzler.\n", SkCodec::kUnimplemented);
-            }
+    // Initialize the swizzler
+    if (fFrameIsSubset) {
+        int sampleX;
+        SkScaledCodec::ComputeSampleSize(dstInfo, this->getInfo(), &sampleX, NULL);
+        const SkImageInfo subsetDstInfo = dstInfo.makeWH(
+                    get_scaled_dimension(fFrameDims.width(), sampleX),
+                    fFrameDims.height());
+        if (kSuccess != this->initializeSwizzler(subsetDstInfo, opts.fZeroInitialized)) {
+            return gif_error("Could not initialize swizzler.\n", kUnimplemented);
+        }
+    } else {
+        if (kSuccess != this->initializeSwizzler(dstInfo, opts.fZeroInitialized)) {
+            return gif_error("Could not initialize swizzler.\n", kUnimplemented);
         }
-
-        return SkCodec::kSuccess;
     }
 
-    SkCodec::Result onGetScanlines(void* dst, int count, size_t rowBytes) override {
-        if (fCodec->fFrameIsSubset) {
-            // Fill the requested rows
-            const SkPMColor* colorPtr = get_color_ptr(fCodec->fColorTable.get());
-            SkSwizzler::Fill(dst, this->dstInfo(), rowBytes, count, fCodec->fFillIndex,
-                    colorPtr, this->options().fZeroInitialized);
-
-            // Do nothing for rows before the image frame
-            int rowsBeforeFrame = fCodec->fFrameDims.top() - INHERITED::getY();
-            if (rowsBeforeFrame > 0) {
-                count = SkTMin(0, count - rowsBeforeFrame);
-                dst = SkTAddOffset<void>(dst, rowBytes * rowsBeforeFrame);
-            }
-
-            // Do nothing for rows after the image frame
-            int rowsAfterFrame = INHERITED::getY() + count - fCodec->fFrameDims.bottom();
-            if (rowsAfterFrame > 0) {
-                count = SkTMin(0, count - rowsAfterFrame);
-            }
+    return kSuccess;
+}
 
-            // Adjust dst pointer for left offset
-            dst = SkTAddOffset<void>(dst, SkColorTypeBytesPerPixel(
-                    this->dstInfo().colorType()) * fCodec->fFrameDims.left());
+SkCodec::Result SkGifCodec::onGetScanlines(void* dst, int count, size_t rowBytes) {
+    if (fFrameIsSubset) {
+        // Fill the requested rows
+        const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
+        SkSwizzler::Fill(dst, this->dstInfo(), rowBytes, count, fFillIndex,
+                         colorPtr, this->options().fZeroInitialized);
+
+        // Do nothing for rows before the image frame
+        // FIXME: nextScanline is not virtual, so using "INHERITED" does not change

[scroggo, 2015/09/30 13:32:20]

Matt, I noticed this when I did the search and replace. I assume this was
intended to be this->INHERITED::onGetY(), but if our tests cover it (i.e. it's
working as intended), then it would have been calling this->getY(). Do you have
an interlaced gif where the frame is subset? (I assume that's the case that
matters, since if it is not interlaced, both implementations are the same.)

[msarett, 2015/09/30 13:53:30]

My intention was to call INHERITED::onNextScanline() - I think this code becomes
correct with that change.

I don't have any interlaced gifs where fFrameIsSubset.  I would guess that they
are extremely rare or don't exist at all.  I doubt I could find one, but I'm
sure I can make one.  I need to make one and add a test because I suspect we
have some broken code in this case.

My guess is that, in the interlaced frame is subset case,
SkGifCodec::onNextScanline() needs to say:
return fFrameDims.top() + get_output_row_interlaced(fCurrScanline -
fFrameDims.top())
or something like that.

Nice catch on this bug!

+        // behavior. Was the intent to call this->INHERITED::onNextScanline()? Same
+        // for the next call down below.
+        int rowsBeforeFrame = fFrameDims.top() - this->INHERITED::nextScanline();
+        if (rowsBeforeFrame > 0) {
+            count = SkTMin(0, count - rowsBeforeFrame);
+            dst = SkTAddOffset<void>(dst, rowBytes * rowsBeforeFrame);
         }
 
-        for (int i = 0; i < count; i++) {
-            if (SkCodec::kSuccess != fCodec->readRow()) {
-                const SkPMColor* colorPtr = get_color_ptr(fCodec->fColorTable.get());
-                SkSwizzler::Fill(dst, this->dstInfo(), rowBytes,
-                        count - i, fCodec->fFillIndex, colorPtr,
-                        this->options().fZeroInitialized);
-                return gif_error("Could not decode line\n", SkCodec::kIncompleteInput);
-            }
-            fCodec->fSwizzler->swizzle(dst, fCodec->fSrcBuffer.get());
-            dst = SkTAddOffset<void>(dst, rowBytes);
+        // Do nothing for rows after the image frame
+        int rowsAfterFrame = this->INHERITED::nextScanline() + count - fFrameDims.bottom();
+        if (rowsAfterFrame > 0) {
+            count = SkTMin(0, count - rowsAfterFrame);
         }
-        return SkCodec::kSuccess;
-    }
 
-    SkScanlineOrder onGetScanlineOrder() const override {
-        if (fCodec->fGif->Image.Interlace) {
-            return kOutOfOrder_SkScanlineOrder;
-        } else {
-            return kTopDown_SkScanlineOrder;
-        }
+            // Adjust dst pointer for left offset
+        int bpp = SkColorTypeBytesPerPixel(this->dstInfo().colorType()) * fFrameDims.left();
+        dst = SkTAddOffset<void>(dst, bpp);
     }
 
-    int onGetY() const override {
-        if (fCodec->fGif->Image.Interlace) {
-            return get_output_row_interlaced(INHERITED::onGetY(), this->dstInfo().height());
-        } else {
-            return INHERITED::onGetY();
+    for (int i = 0; i < count; i++) {
+        if (kSuccess != this->readRow()) {
+            const SkPMColor* colorPtr = get_color_ptr(fColorTable.get());
+            SkSwizzler::Fill(dst, this->dstInfo(), rowBytes, count - i, fFillIndex, colorPtr,
+                             this->options().fZeroInitialized);
+            return gif_error("Could not decode line\n", SkCodec::kIncompleteInput);
         }
+        fSwizzler->swizzle(dst, fSrcBuffer.get());
+        dst = SkTAddOffset<void>(dst, rowBytes);
     }
+    return SkCodec::kSuccess;
+}
 
-private:
-    SkAutoTDelete<SkGifCodec>   fCodec;
-
-    typedef SkScanlineDecoder INHERITED;
-};
-
-SkScanlineDecoder* SkGifCodec::NewSDFromStream(SkStream* stream) {
-    SkAutoTDelete<SkGifCodec> codec (static_cast<SkGifCodec*>(SkGifCodec::NewFromStream(stream)));
-    if (!codec) {
-        return NULL;
+SkCodec::SkScanlineOrder SkGifCodec::onGetScanlineOrder() const {
+    if (fGif->Image.Interlace) {
+        return kOutOfOrder_SkScanlineOrder;
+    } else {
+        return kTopDown_SkScanlineOrder;
     }
+}
 
-    const SkImageInfo& srcInfo = codec->getInfo();
-
-    return new SkGifScanlineDecoder(srcInfo, codec.detach());
+int SkGifCodec::onNextScanline() const {
+    if (fGif->Image.Interlace) {
+        return get_output_row_interlaced(this->INHERITED::onNextScanline(), this->dstInfo().height());
+    } else {
+        return this->INHERITED::onNextScanline();
+    }
 }
+