Upstream Android modifications to the image encoders/decoders.

include/images/SkImageDecoder.h

Index: include/images/SkImageDecoder.h
diff --git a/include/images/SkImageDecoder.h b/include/images/SkImageDecoder.h
index 6d1bb7241d5dd3aeb44e584d02840de23e7eb678..97898e6248a7640257d09fdf58ca70d6fdabd375 100644
--- a/include/images/SkImageDecoder.h
+++ b/include/images/SkImageDecoder.h
@@ -13,6 +13,7 @@
 #include "SkBitmap.h"
 #include "SkBitmapFactory.h"
 #include "SkImage.h"
+#include "SkRect.h"
 #include "SkRefCnt.h"
 
 class SkStream;
@@ -25,6 +26,7 @@ class SkImageDecoder {
 public:
     virtual ~SkImageDecoder();
 
+    // Should be consistent with kFormatName
     enum Format {
         kUnknown_Format,
         kBMP_Format,
@@ -33,14 +35,29 @@ public:
         kJPEG_Format,
         kPNG_Format,
         kWBMP_Format,
+        kWEBP_Format,
 
-        kLastKnownFormat = kWBMP_Format
+        kLastKnownFormat = kWEBP_Format
     };
 
+    /** Contains the image format name.
+     *  This should be consistent with Format.
+     *
+     *  The format name gives a more meaningful error message than enum.
+     */

[robertphillips, 2013/03/13 13:41:29]

So you don't think replacing the '8' with "kLastKnownFormat+1" would be
worthwhile?

[djsollen, 2013/03/13 16:02:48]

Done.

+    static const char *kFormatName[8];

[scroggo, 2013/03/13 16:04:14]

Why does this have a 'k' at the front? Isn't that reserved for enums? Also, does
it need to be public?

[djsollen, 2013/03/13 17:35:09]

Done.

+
     /** Return the compressed data's format (see Format enum)
     */
     virtual Format getFormat() const;
 
+    /** Return the compressed data's format name.
+    */
+    const char* getFormatName() const {
+        SkASSERT(SK_ARRAY_COUNT(kFormatName) == kLastKnownFormat);

[robertphillips, 2013/03/13 13:41:29]

this->getFormat()

[djsollen, 2013/03/13 16:02:48]

Done.

+        return kFormatName[getFormat()];
+    }
+
     /** Returns true if the decoder should try to dither the resulting image.
         The default setting is true.
     */
@@ -51,6 +68,20 @@ public:
     */
     void setDitherImage(bool dither) { fDitherImage = dither; }
 
+    /** Returns true if the decoder should try to decode the
+        resulting image to a higher quality even at the expense of
+        the decoding speed.
+    */
+    bool getPreferQualityOverSpeed() const { return fPreferQualityOverSpeed; }
+
+    /** Set to true if the the decoder should try to decode the
+        resulting image to a higher quality even at the expense of
+        the decoding speed.
+    */
+    void setPreferQualityOverSpeed(bool qualityOverSpeed) {
+        fPreferQualityOverSpeed = qualityOverSpeed;
+    }
+
     /** \class Peeker
 
         Base class for optional callbacks to retrieve meta/chunk data out of
@@ -175,11 +206,30 @@ public:
 
         note: document use of Allocator, Peeker and Chooser
     */
-    bool decode(SkStream*, SkBitmap* bitmap, SkBitmap::Config pref, Mode);
-    bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode) {
-        return this->decode(stream, bitmap, SkBitmap::kNo_Config, mode);
+    bool decode(SkStream*, SkBitmap* bitmap, SkBitmap::Config pref, Mode, bool reuseBitmap = false);

[scroggo, 2013/03/13 16:04:14]

Some thoughts on reuseBitmap:
It appears to be only meaningful if Mode == kDecodePixels, making for an ugly
API.
I dislike that passing true to reuseBitmap can turn into a mysterious failure.
At the very least,  we should document the behavior. Also, it is not clear to me
why some decoders do not allow reuse of a bitmap.
It almost seems like it could be replaced with a custom allocator which just
reuses the existing pixels. I guess the drawback to that approach is that the
subclass cannot deny reuse. I suppose some caller attempts to reuse when it's
not allowed?

[djsollen, 2013/03/13 17:35:09]

The lack of support is not a technical problem, just that the android authors
didn't spend the time to do all codecs.

FWIW, I don't think this is a great API either, but it is what is currently in
use.

+    bool decode(SkStream* stream, SkBitmap* bitmap, Mode mode, bool reuseBitmap = false) {
+        return this->decode(stream, bitmap, SkBitmap::kNo_Config, mode, reuseBitmap);
     }
 
+    /**
+     * Given a stream, build an index for doing tile-based decode.
+     * The built index will be saved in the decoder, and the image size will
+     * be returned in width and height.
+     *
+     * Return true for success or false on failure.
+     */
+    virtual bool buildTileIndex(SkStream*, int *width, int *height);
+
+    /**
+     * Decode a rectangle region in the image specified by rect.
+     * The method can only be called after buildTileIndex().
+     *
+     * Return true for success.
+     * Return false if the index is never built or failing in decoding.
+     */
+    virtual bool decodeRegion(SkBitmap* bitmap, const SkIRect& rect,

[scroggo, 2013/03/13 16:04:14]

decodeRect?

[djsollen, 2013/03/13 17:35:09]

I don't want to change method signatures in this CL.

+                              SkBitmap::Config pref);
+
     /** Given a stream, this will try to find an appropriate decoder object.
         If none is found, the method returns NULL.
     */
@@ -296,6 +346,38 @@ protected:
     // must be overridden in subclasses. This guy is called by decode(...)
     virtual bool onDecode(SkStream*, SkBitmap* bitmap, Mode) = 0;
 

[robertphillips, 2013/03/13 13:41:29]

Isn't the pattern to have a non-virtual decodeRegion/buildTileIndex and then
virtual on* methods? Do we need virtuals in all four locations?

[djsollen, 2013/03/13 17:35:09]

Done.

+    // If the decoder wants to support tiled based decoding,
+    // this method must be overridden. This guy is called by buildTileIndex(...)
+    virtual bool onBuildTileIndex(SkStream*, int *width, int *height) {
+        return false;
+    }
+
+    // If the decoder wants to support tiled based decoding,
+    // this method must be overridden. This guy is called by decodeRegion(...)
+    virtual bool onDecodeRegion(SkBitmap* bitmap, const SkIRect& rect) {
+        return false;
+    }
+
+    /*
+     * Crop a rectangle from the src Bitmap to the dest Bitmap. src and dst are
+     * both sampled by sampleSize from an original Bitmap.
+     *
+     * @param dst the destination bitmap.
+     * @param src the source bitmap that is sampled by sampleSize from the
+     *            original bitmap.
+     * @param sampleSize the sample size that src is sampled from the original bitmap.

[robertphillips, 2013/03/13 13:41:29]

make order in the comment match the order in the function call? bitimap ->
bitmap

[djsollen, 2013/03/13 17:35:09]

Done.

+     * @param (srcX, srcY) the upper-left point of the src bitimap in terms of
+     *                     the coordinate in the original bitmap.
+     * @param (width, height) the width and height of the unsampled dst.
+     * @param (dstX, dstY) the upper-left point of the dest bitmap in terms of
+     *                     the coordinate in the original bitmap.
+     */
+    void cropBitmap(SkBitmap *dst, SkBitmap *src, int sampleSize,
+                    int dstX, int dstY, int width, int height,
+                    int srcX, int srcY);
+
+
+
     /** Can be queried from within onDecode, to see if the user (possibly in
         a different thread) has requested the decode to cancel. If this returns
         true, your onDecode() should stop and return false.
@@ -346,6 +428,7 @@ private:
     bool                    fDitherImage;
     bool                    fUsePrefTable;
     mutable bool            fShouldCancelDecode;
+    bool                    fPreferQualityOverSpeed;
 
     // illegal
     SkImageDecoder(const SkImageDecoder&);
@@ -396,5 +479,6 @@ DECLARE_DECODER_CREATOR(ICOImageDecoder);
 DECLARE_DECODER_CREATOR(JPEGImageDecoder);
 DECLARE_DECODER_CREATOR(PNGImageDecoder);
 DECLARE_DECODER_CREATOR(WBMPImageDecoder);
+DECLARE_DECODER_CREATOR(WEBPImageDecoder);
 
 #endif