Bmp Image Decoding

src/codec/SkSwizzler.h

Index: src/codec/SkSwizzler.h
diff --git a/src/codec/SkSwizzler.h b/src/codec/SkSwizzler.h
index 1adb14db0e6d409a295964257bc32a0049ad3e20..a793b45a886b46a0d05f7d90f4c0c9241af236fc 100644
--- a/src/codec/SkSwizzler.h
+++ b/src/codec/SkSwizzler.h
@@ -18,28 +18,60 @@ public:
      *  Enum describing the config of the source data.
      */
     enum SrcConfig {
-        kGray,  // 1 byte per pixel
-        kIndex, // 1 byte per pixel
-        kRGB,   // 3 bytes per pixel
-        kRGBX,  // 4 byes per pixel (ignore 4th)
-        kRGBA,  // 4 bytes per pixel
-        kRGB_565 // 2 bytes per pixel
+        kGray,
+        kIndex1,

[scroggo, 2015/02/25 17:22:43]

Alternatively, we could make the caller specify how big a pixel is, and just
have kIndex.

+        kIndex2,
+        kIndex4,
+        kIndex8,
+        kRGB,
+        kBGR,
+        kRGBX,
+        kBGRX,
+        kRGBA,
+        kBGRA,
+        kRGB_565,
+        kMask16,
+        kMask24,
+        kMask32
     };
 
     static int BytesPerPixel(SrcConfig sc) {
         switch (sc) {
+            case kIndex1:
+            case kIndex2:
+            case kIndex4:
+                return 0; // Flag used when pixels are less than a byte

[scroggo, 2015/02/25 17:22:43]

I don't like having both BitsPerPixel and BytesPerPixel. Can we convert all
callers to use BitsPerPixel? (Or maybe there's another solution?)

             case kGray:
-            case kIndex:
+            case kIndex8:
                 return 1;
             case kRGB:
+            case kBGR:
+            case kMask24:
                 return 3;
             case kRGBX:
+            case kBGRX:
             case kRGBA:
+            case kBGRA:
+            case kMask32:
                 return 4;
             case kRGB_565:
+            case kMask16:
                 return 2;
         }
     }
+    
+    static int BitsPerPixel(SrcConfig sc) {
+        switch (sc) {
+            case kIndex1:
+                return 1;
+            case kIndex2:
+                return 2;
+            case kIndex4:
+                return 4;
+            default:
+                return BytesPerPixel(sc) * 8;
+        }
+    }
 
     /**
      *  Create a new SkSwizzler.
@@ -50,11 +82,17 @@ public:
      *  @param dstRowBytes rowBytes for dst.
      *  @param skipZeroes Whether to skip writing zeroes. Useful if dst is
      *              zero-initialized. The implementation may or may not respect this.
+     *  @param bitMasks Custom bit masks used to interpret pixels

[scroggo, 2015/02/25 17:22:43]

What does custom mean?

[scroggo, 2015/02/25 17:22:43]

What does this mean? Does the swizzler always do the same thing with it? If so,
it should be described here

+     *  @param fixAlpha Whether to monitor for fully transparent images and set
+     *              them as opaque
      *  @return A new SkSwizzler or NULL on failure.
      */
     static SkSwizzler* CreateSwizzler(SrcConfig sc, const SkPMColor* ctable,
                                       const SkImageInfo& info, void* dst,
-                                      size_t dstRowBytes, bool skipZeroes);
+                                      size_t dstRowBytes, bool skipZeroes,
+                                      const uint32_t* bitMasks = NULL,
+                                      const bool fixAlpha = false,
+                                      const bool inverted = false);

[scroggo, 2015/02/25 17:22:43]

Can you add a comment explaining "inverted"?

I shouldn't have made skipZeroes a boolean. We tend to encourage using enums so
the call site is clearer. With three booleans, now, it's hard to tell what's
what.

     /**
      *  Swizzle the next line. Call height times, once for each row of source.
      *  @param src The next row of the source data.
@@ -73,19 +111,29 @@ private:
      */
     typedef bool (*RowProc)(void* SK_RESTRICT dstRow,
                             const uint8_t* SK_RESTRICT src,
-                            int width, int bpp, int y,
-                            const SkPMColor ctable[]);
+                            int width, int bpp, int bitsPerPixel, int y,
+                            const SkPMColor ctable[], const uint32_t* masks,

[scroggo, 2015/02/25 17:22:43]

please add comments for all of these.

+                            const bool fixAlpha, bool* fSeenNonZeroAlphaPtr,

[scroggo, 2015/02/25 17:22:43]

fCamelCase is reserved for member fields. Parameter names should be camelCase
(e.g. seenNonZeroAlphaPtr).

[msarett, 2015/02/26 23:58:18]

I was trying to draw attention to the fact that I was using ptrs to fields,
which I think is pretty hacky.  Any thoughts on this?

+                            bool* fZeroPrevRowsPtr);
 
     const RowProc       fRowProc;
     const SkPMColor*    fColorTable;    // Unowned pointer
+    const uint32_t*     fBitMasks;      // Unowned pointer
     const int           fSrcPixelSize;
+    const int           fSrcBitsPerPixel;
     const SkImageInfo   fDstInfo;
     void*               fDstRow;
     const size_t        fDstRowBytes;
     int                 fCurrY;
+    const bool          fFixAlpha;
+    const bool          fInverted;
+    bool                fSeenNonZeroAlpha;
+    bool                fZeroPrevRows;
 
-    SkSwizzler(RowProc proc, const SkPMColor* ctable, int srcBpp,
-               const SkImageInfo& info, void* dst, size_t rowBytes);
+    SkSwizzler(RowProc proc, const SkPMColor* ctable, int srcBpp, int srcBitsPP,
+               const SkImageInfo& info, void* dst, size_t rowBytes,
+               const uint32_t* bitMasks, const bool fixAlpha,
+               const bool inverted);
 
 };
 #endif // SkSwizzler_DEFINED