Fill incomplete images in SkCodec parent class

include/codec/SkCodec.h

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef SkCodec_DEFINED
 #define SkCodec_DEFINED
 
@@ skipping 216 matching lines @@
      *  less than 100%. This function can be called *after* decoding to
      *  determine if such an image truly had alpha. Calling it before decoding
      *  is undefined.
      *  FIXME: see skbug.com/3582.
      */
     bool reallyHasAlpha() const {
         return this->onReallyHasAlpha();
     }
 
     /**
+     * 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.
      */
 
     /**
      *  Prepare for a scanline decode with the specified options.
      *
      *  After this call, this class will be ready to decode the first scanline.
      *
      *  This must be called in order to call getScanlines or skipScanlines.
      *
@@ skipping 26 matching lines @@
     /**
      *  Write the next countLines scanlines into dst.
      *
      *  Not valid to call before calling startScanlineDecode().
      *
      *  @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.
+     *  @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.
      *
      *  Not valid to call before calling startScanlineDecode().
      *
      *  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.
+     *
+     *  @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
      *  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
@@ skipping 56 matching lines @@
     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 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*);
 
     virtual SkISize onGetScaledDimensions(float /* desiredScale */) const {
         // By default, scaling is not supported.
         return this->getInfo().dimensions();
     }
 
     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.
         return false;
     }
 
     virtual bool onReallyHasAlpha() const { return false; }
 
     /**
      *  If the stream was previously read, attempt to rewind.
@@ skipping 10 matching lines @@
     /**
      *  Called by rewindIfNeeded, if the stream needed to be rewound.
      *
      *  Subclasses should do any set up needed after a rewind.
      */
     virtual bool onRewind() {
         return true;
     }
 
     /**
+     * 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() {
         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 onNextScanline() const { return fCurrScanline; }
-
-    /**
      *  Update the next scanline. Used by interlaced png.
      */
     void updateNextScanline(int newY) { fCurrScanline = newY; }
 
     const SkImageInfo& dstInfo() const { return fDstInfo; }
 
     const SkCodec::Options& options() const { return fOptions; }
 
 private:
     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 onStartScanlineDecode(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) {
+    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