Merge SkCodec with SkScanlineDecoder

include/codec/SkCodec.h

Index: include/codec/SkCodec.h
diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index bdfa2326fba5acc4c89bdc346e2b972a0ddb25d4..d4b0a2dfe4a01ca482ae911007e795a57489261d 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -45,7 +45,7 @@ public:
     /**
      *  Return the ImageInfo associated with this codec.
      */
-    const SkImageInfo& getInfo() const { return fInfo; }
+    const SkImageInfo& getInfo() const { return fSrcInfo; }
 
     /**
      *  Return a size that approximately supports the desired scale factor.
@@ -131,9 +131,14 @@ public:
          */
         kCouldNotRewind,
         /**
-         *  This method is not implemented by this generator.
+         *  This method is not implemented by this codec.
+         *  FIXME: Perhaps this should be kUnsupported?
          */
         kUnimplemented,
+        /**
+         *  start() was not called before calling getScanlines.
+         */
+        kScanlineDecodingNotStarted,
     };
 
     /**
@@ -202,6 +207,9 @@ public:
      *  If info is not kIndex8_SkColorType, then the last two parameters may be NULL. If ctableCount
      *  is not null, it will be set to 0.
      *
+     *  If a scanline decode is in progress, scanline mode will end, requiring the client to call
+     *  start() in order to return to decoding scanlines.
+     *
      *  @return Result kSuccess, or another value explaining the type of failure.
      */
     Result getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes, const Options*,
@@ -225,6 +233,173 @@ public:
         return this->onReallyHasAlpha();
     }
 
+    /**
+     * The remaining functions revolve around decoding scanlines.
+     */
+
+    /**
+     *  Initialize on the first scanline, with the specified options.

[msarett, 2015/09/28 14:48:50]

"Initialize on the first scanline" is a little unclear to me.

Maybe "Prepare for a scanline decode" or "Initialize scanline decoder with the
specified image info and options"?

[scroggo, 2015/09/28 16:01:52]

Sgtm. I think it made a little more sense back when it was on SkScanlineDecoder,
which was only used for scanline decodes.

+     *
+     *  This must be called in order to call getScanlines or skipScanlines.
+     *
+     *  This may require rewinding the stream.
+     *
+     *  Not all SkCodecs support this.
+     *
+     *  @param dstInfo Info of the destination. If the dimensions do not match
+     *      those of getInfo, this implies a scale.
+     *  @param options Contains decoding options, including if memory is zero
+     *      initialized.
+     *  @param ctable A pointer to a color table.  When dstInfo.colorType() is
+     *      kIndex8, this should be non-NULL and have enough storage for 256
+     *      colors.  The color table will be populated after decoding the palette.
+     *  @param ctableCount A pointer to the size of the color table.  When
+     *      dstInfo.colorType() is kIndex8, this should be non-NULL.  It will
+     *      be modified to the true size of the color table (<= 256) after
+     *      decoding the palette.
+     *  @return Enum representing success or reason for failure.
+     */
+    Result start(const SkImageInfo& dstInfo, const SkCodec::Options* options,

[scroggo, 2015/09/25 16:07:49]

I left the name "start" to make the changeover simpler. If this name is too
ambiguous, I previously used the name "prepareForScanlineDecode", which is a
little long. Thoughts?

[msarett, 2015/09/28 14:48:50]

I like a shorter name but that's because I know what this function does.  I
guess it's not really a clear name unless it includes some reference to
scanlines?

A couple ideas:
prepareScanlines() : A shorter name, and this is kind of uniform with
getScanlines() and skipScanlines(), but I guess you could argue that we're not
really preparing the scanlines themselves.
startScanlineDecode() : Not as clear as prepareForSD, but it's a little shorter.

prepareScanlines() gets my vote, but I'm indifferent on this.

[scroggo, 2015/09/28 16:01:52]

These sound fine with me. I'll wait for API review to do a search and replace
(if any).

+                 SkPMColor ctable[], int* ctableCount);
+
+    /**
+     *  Simplified version of start() that asserts that info is NOT
+     *  kIndex8_SkColorType and uses the default Options.
+     */
+    Result start(const SkImageInfo& dstInfo);
+
+    /**
+     *  Write the next countLines scanlines into dst.
+     *
+     *  Not valid to call before calling start().
+     *
+     *  @param dst Must be non-null, and large enough to hold countLines
+     *      scanlines of size rowBytes.
+     *  @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.
+     */
+    Result getScanlines(void* dst, int countLines, size_t rowBytes) {
+        if (fCurrScanline < 0) {

[scroggo, 2015/09/25 16:07:49]

Alternatively, I could have checked fDstInfo, which I would need to reset where
I reset fCurrScanline.

[msarett, 2015/09/28 14:48:50]

I like what you've chosen to do.

+            return kScanlineDecodingNotStarted;

[scroggo, 2015/09/25 16:07:49]

I know (msarett@) you have a CL out for review which makes this method return a
number of scanlines decoded. Once that lands, maybe this should return 0. It's
kind of too bad we don't provide more information in that case, but maybe it's
okay?

[msarett, 2015/09/28 14:48:50]

Yeah I think that's ok.  Maybe we can provide more information by also adding an
SkASSERT() before returning kScanlineDecodingNotStarted?

Considering this on its own (without thinking about future CLs), I think this
makes sense as is.

[scroggo, 2015/09/28 16:01:52]

I think returning kScanlineDecodingNoStarted is probably enough, but I could be
convinced to add it.

+        }
+
+        SkASSERT(!fDstInfo.isEmpty());
+        if ((rowBytes < fDstInfo.minRowBytes() && countLines > 1 ) || countLines <= 0
+                || fCurrScanline + countLines > fDstInfo.height()) {
+            return kInvalidParameters;
+        }
+        const Result result = this->onGetScanlines(dst, countLines, rowBytes);
+        fCurrScanline += countLines;
+        return result;
+    }
+
+    /**
+     *  Skip count scanlines.
+     *
+     *  Not valid to call before calling start().
+     *
+     *  The default version just calls onGetScanlines and discards the dst.
+     *  NOTE: If skipped lines are the only lines with alpha, this default
+     *  will make reallyHasAlpha return true, when it could have returned
+     *  false.
+     */
+    Result skipScanlines(int countLines) {
+        if (fCurrScanline < 0) {
+            return kScanlineDecodingNotStarted;
+        }
+        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
+            // can catch their bug.
+            return SkCodec::kInvalidParameters;
+        }
+        const Result result = this->onSkipScanlines(countLines);
+        fCurrScanline += countLines;
+        return result;
+    }
+
+    /**
+     *  The order in which rows are output from the scanline decoder is not the
+     *  same for all variations of all image types.  This explains the possible
+     *  output row orderings.
+     */
+    enum SkScanlineOrder {
+        /*
+         * By far the most common, this indicates that the image can be decoded
+         * reliably using the scanline decoder, and that rows will be output in
+         * the logical order.
+         */
+        kTopDown_SkScanlineOrder,
+
+        /*
+         * This indicates that the scanline decoder reliably outputs rows, but
+         * they will be returned in reverse order.  If the scanline format is
+         * kBottomUp, the getY() API can be used to determine the actual
+         * y-coordinate of the next output row, but the client is not forced
+         * to take advantage of this, given that it's not too tough to keep
+         * track independently.
+         *
+         * For full image decodes, it is safe to get all of the scanlines at
+         * once, since the decoder will handle inverting the rows as it
+         * decodes.
+         *
+         * For subset decodes and sampling, it is simplest to get and skip
+         * scanlines one at a time, using the getY() API.  It is possible to
+         * ask for larger chunks at a time, but this should be used with
+         * caution.  As with full image decodes, the decoder will handle
+         * inverting the requested rows, but rows will still be delivered
+         * starting from the bottom of the image.
+         *
+         * Upside down bmps are an example.
+         */
+        kBottomUp_SkScanlineOrder,
+
+        /*
+         * This indicates that the scanline decoder reliably outputs rows, but
+         * they will not be in logical order.  If the scanline format is
+         * kOutOfOrder, the getY() API should be used to determine the actual
+         * y-coordinate of the next output row.
+         *
+         * For this scanline ordering, it is advisable to get and skip
+         * scanlines one at a time.
+         *
+         * Interlaced gifs are an example.
+         */
+        kOutOfOrder_SkScanlineOrder,
+
+        /*
+         * Indicates that the entire image must be decoded in order to output
+         * any amount of scanlines.  In this case, it is a REALLY BAD IDEA to
+         * request scanlines 1-by-1 or in small chunks.  The client should
+         * determine which scanlines are needed and ask for all of them in
+         * a single call to getScanlines().
+         *
+         * Interlaced pngs are an example.
+         */
+        kNone_SkScanlineOrder,
+    };
+
+    /**
+     *  An enum representing the order in which scanlines will be returned by
+     *  the scanline decoder.
+     */
+    SkScanlineOrder getScanlineOrder() const { return this->onGetScanlineOrder(); }
+
+    /**
+     *  Returns the y-coordinate of the next 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.
+     *
+     *  Results are undefined when not in scanline decoding mode.
+     */
+    int getY() const {
+        SkASSERT(kNone_SkScanlineOrder != this->getScanlineOrder());

[msarett, 2015/09/28 14:48:50]

Given that the interlaced png scanline decoder now makes use of onGetY(), it
seems odd that we have gone to the trouble of forbidding clients from calling
getY() when decoding interlaced pngs.

It is inefficient to call getScanlines() more than once for interlaced pngs,
which was my original reasoning for discouraging the use of getY().  Stated
another way, calling getY() should be unnecessary if we only plan to
getScanlines() once.

However, I think your use of onGetY() in the SkPngInterlacedSD makes a lot of
sense.  I think maybe we should remove this SkASSERT (and the comment above),
and trust that we are intelligent enough clients of our own code to not misuse
getScanlines() with interlaced pngs.

[scroggo, 2015/09/28 16:01:52]

I've removed the assert. FWIW, interlaced png calls onGetY directly since getY
doesn't add anything for a subclass, rather than to get around the assert.

+        return this->onGetY();
+    }
+
 protected:
     SkCodec(const SkImageInfo&, SkStream*);
 
@@ -275,9 +450,60 @@ protected:
         return fStream.get();
     }
 
+    /**
+     *  The remaining functions revolve around decoding scanlines.
+     */
+
+    /**
+     *  Most images types will be kTopDown and will not need to override this function.
+     */
+    virtual SkScanlineOrder onGetScanlineOrder() const { return kTopDown_SkScanlineOrder; }
+
+    /**
+     *  Most images will be kTopDown and will not need to override this function.
+     */
+    virtual int onGetY() const { return fCurrScanline; }
+
+    /**
+     *  Update the next scanline. Used by interlaced png.
+     */
+    void updateY(int newY) { fCurrScanline = newY; }

[scroggo, 2015/09/25 16:07:49]

I named this updateY to mirror getY, but I might prefer something that
references "next" or "current" "scanline".

[msarett, 2015/09/28 14:48:50]

I'm in favor of renaming these.

I think my original name for getY() was getCurrScanline() but I felt that was
confusing since it seemed like it should actually decode a scanline.  Maybe that
confusion is avoided with currScanline() or nextScanline()?

[scroggo, 2015/09/28 16:01:52]

I'll add a FIXME to discuss for API review. I would probably lean towards
something like nextScanline. 

+
+    const SkImageInfo& dstInfo() const { return fDstInfo; }
+
+    const SkCodec::Options& options() const { return fOptions; }
+
 private:
-    const SkImageInfo       fInfo;
+    const SkImageInfo       fSrcInfo;
     SkAutoTDelete<SkStream> fStream;
     bool                    fNeedsRewind;
+    // These fields are only meaningful during scanline decodes.
+    SkImageInfo             fDstInfo;
+    SkCodec::Options        fOptions;
+    int                     fCurrScanline;
+
+    // Methods for scanline decoding.
+    virtual SkCodec::Result onStart(const SkImageInfo& dstInfo,
+                                    const SkCodec::Options& options,
+                                    SkPMColor ctable[], int* ctableCount) {
+        return kUnimplemented;
+    }
+
+    // Naive default version just calls onGetScanlines on temp memory.
+    virtual SkCodec::Result 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,
+        // onGetScanlines bypasses that check.
+        // Calling the virtual method also means we do not double count
+        // countLines.
+        return this->onGetScanlines(storage.get(), countLines, 0);
+    }
+
+    virtual SkCodec::Result onGetScanlines(void* dst, int countLines,
+                                                    size_t rowBytes) {
+        return kUnimplemented;
+    }
+
 };
 #endif // SkCodec_DEFINED