Fill incomplete images in SkCodec parent class

include/codec/SkCodec.h

Index: include/codec/SkCodec.h
diff --git a/include/codec/SkCodec.h b/include/codec/SkCodec.h
index c0cce853a3aa07e36668bb497883bed2da4b1515..0eaedecde0b32c4aaf81af31936244e8763a72fa 100644
--- a/include/codec/SkCodec.h
+++ b/include/codec/SkCodec.h
@@ -234,6 +234,22 @@ public:
     }
 
     /**
+     * On a kIncompleteInput, we will fill any uninitialized scanlines.  We allow the subclass

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

I think "we" is a little vague here. I think you mean "getPixels"? Does the
scanline decoder also fill uninitialized scanlines?

On that note, I *think* that if we decode a single scanline, we'll fill, but if
we decode zero, it seems like the image is completely broken - should we fill in
that case? (Maybe we have a different answer for scanline decoding versus full
decoding?)

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

The current implementation will always fill for getPixels() and getScanlines(). 
I would argue that being consistent on this is a good security practice. 
"Sometimes" leaving memory uninitialized seems dangerous to me.

+     * to indicate what value to fill with.
+     *
+     * @param colorType Destination color type.
+     * @param alphaType Destination alpha type.
+     * @return          The value with which to fill uninitialized pixels.
+     *
+     * Note that we can interpret the return value as an SkPMColor, a 16-bit 565 color,
+     * an 8-bit gray color, or an 8-bit index into a color table, depending on the color
+     * type specified in dstInfo.
+     */
+    uint32_t getFillValue(SkColorType colorType, SkAlphaType alphaType) const {

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

Does this method need to be public?

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

Yes, SkScaledCodec needs to call fCodec->getFillValue().

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

FWIW, crrev.com/1372973002 makes SkScaledCodec a friend, which I think makes
sense. It was precisely for this kind of thing - SkScaledCodec wants to call a
method on SkCodec that I do not think should be accessible to other callers.
(Why would a normal client of SkCodec need to know the fill value?)

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

That seems like a better solution.  I am adding SkScaledCodec as a friend and
making this function protected.

+        return this->onGetFillValue(colorType, alphaType);
+    }
+
+    /**
      * The remaining functions revolve around decoding scanlines.
      */
 
@@ -280,8 +296,9 @@ 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

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

Does this fill if some were decoded? What does it mean if it's less than
countLines? (Please add to comments.)

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

Yes it does.  Adding the comment.

      */
-    Result getScanlines(void* dst, int countLines, size_t rowBytes);
+    uint32_t getScanlines(void* dst, int countLines, size_t rowBytes);
 
     /**
      *  Skip count scanlines.
@@ -292,8 +309,13 @@ 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 true if the scanlines were successfully skipped
+     *          false on failure, possible reasons for failure include:
+     *              An incomplete input image stream
+     *              Invalid parameters passed to skipScanlines()

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

What are invalid parameters?

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

Adding more detail to the comment.

      */
-    Result skipScanlines(int countLines);
+    bool skipScanlines(int countLines);
 
     /**
      *  The order in which rows are output from the scanline decoder is not the
@@ -370,9 +392,13 @@ public:
      *
      *  Results are undefined when not in scanline decoding mode.
      */
-    int nextScanline() const {
-        return this->onNextScanline();
-    }
+    int nextScanline() const { return this->nextScanline(fCurrScanline); }
+    /**

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

nit: newline between these methods

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

Done.

+     * Alternate version that allows the client to specify the srcY value.
+     * The corresponding dstY will be returned, regardless of the value of
+     * fCurrScanline.
+     */
+    int nextScanline(int srcY) const;

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

I don't think this name is right. My first thought was something like

  int outputScanline(int inputScanline)

but I'm not sure that's very clear either. You're not returning the "next"
scanline, you're returning the logical output location of the (inputVar)'th row
stored in the encoded data.

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

Hmmm yeah, it made more sense for these to have the same name when the name was
getY().  I like your suggestion.

 
 protected:
     SkCodec(const SkImageInfo&, SkStream*);
@@ -384,9 +410,16 @@ protected:
 
     virtual SkEncodedFormat onGetEncodedFormat() const = 0;
 
+    /**
+     * @param rowsDecoded When the encoded image stream is incomplete, this function
+     *                    will return kIncompleteInput and rowsDecoded will be set to
+     *                    the number of scanlines that were successfully decoded.
+     *                    This will allow getPixels() to fill the uninitialized memory.
+     */
     virtual Result onGetPixels(const SkImageInfo& info,
                                void* pixels, size_t rowBytes, const Options&,
-                               SkPMColor ctable[], int* ctableCount) = 0;
+                               SkPMColor ctable[], int* ctableCount,
+                               int* rowsDecoded) = 0;
 
     virtual bool onGetValidSubset(SkIRect* /* desiredSubset */) const {
         // By default, subsets are not supported.
@@ -417,6 +450,20 @@ protected:
     }
 
     /**
+     * Some subclasses will override this function, but this is a useful default for the color
+     * types that we support.  Note that for color types that do not use the full 32-bits,
+     * we will simply take the low bits of the fill value.
+     *
+     * kN32_SkColorType: Transparent or Black
+     * kRGB_565_SkColorType: Black
+     * kGray_8_SkColorType: Black
+     * kIndex_8_SkColorType: First color in color table
+     */
+    virtual uint32_t onGetFillValue(SkColorType colorType, SkAlphaType alphaType) const {
+        return kOpaque_SkAlphaType == alphaType ? SK_ColorBLACK : SK_ColorTRANSPARENT;
+    }
+
+    /**
      * Get method for the input stream
      */
     SkStream* stream() {
@@ -433,11 +480,6 @@ protected:
     virtual SkScanlineOrder onGetScanlineOrder() const { return kTopDown_SkScanlineOrder; }
 
     /**
-     *  Most images will be kTopDown and will not need to override this function.
-     */
-    virtual int onNextScanline() const { return fCurrScanline; }
-
-    /**
      *  Update the next scanline. Used by interlaced png.
      */
     void updateNextScanline(int newY) { fCurrScanline = newY; }
@@ -462,20 +504,33 @@ private:
     }
 
     // Naive default version just calls onGetScanlines on temp memory.
-    virtual SkCodec::Result onSkipScanlines(int countLines) {
+    virtual bool onSkipScanlines(int countLines) {
+        // FIXME (msarett): We should not reallocate this memory on every call.

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

Is the real fix to always override onSkipScanlines? I thought we discussed that
we could almost always do better than this default (we can at least skip
swizzling - only jpeg does not require a swizzle always, but it already has an
override). We could make this pure virtual and force the caller to be smarter. I
think many (maybe all) of the subclasses have their own buffers that they reuse,
so we can always avoid the reallocation.

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

Yes I agree that the solution is to always override this.  Changing the FIXME.

         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);
+        return (uint32_t) countLines == this->onGetScanlines(storage.get(), countLines, 0);
     }
 
-    virtual SkCodec::Result onGetScanlines(void* dst, int countLines,
-                                                    size_t rowBytes) {
-        return kUnimplemented;
-    }
+    virtual uint32_t onGetScanlines(void* dst, int countLines, size_t rowBytes) { return 0; }
+
+    /**
+     * On an incomplete decode, getPixels() and getScanlines() will call this function
+     * to fill any uinitialized memory.
+     *
+     * @param info           Destination image info
+     * @param dst            Destination pixel memory

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

I think this is different depending on the scanline order?

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

In the redesign, this is always a pointer to the start of destination memory. 
Adding to the comment.

+     * @param rowBytes       Stride length in destination pixel memory
+     * @param zeroInit       Indicates if memory is zero initialized
+     * @param linesRequested Number of lines that the client requested
+     * @param linesDecoded   Number of lines that were successfully decoded
+     */
+    void fillIncompleteImage(const SkImageInfo& info, void* dst, size_t rowBytes,
+            ZeroInitialized zeroInit, int linesRequested, int linesDecoded);
 
 };
 #endif // SkCodec_DEFINED