Split SkBmpCodec into three separate classes

src/codec/SkBmpCodec.h

Index: src/codec/SkBmpCodec.h
diff --git a/src/codec/SkBmpCodec.h b/src/codec/SkBmpCodec.h
index 9909945c51047865cdb72c45a77cfea9125bc395..c211ca5300f9e8c10b129d9eb0b3d40dbb6467d1 100644
--- a/src/codec/SkBmpCodec.h
+++ b/src/codec/SkBmpCodec.h
@@ -4,6 +4,8 @@
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
+#ifndef SkBmpCodec_DEFINED
+#define SkBmpCodec_DEFINED
 
 #include "SkCodec.h"
 #include "SkColorTable.h"
@@ -13,19 +15,15 @@
 #include "SkSwizzler.h"
 #include "SkTypes.h"

[msarett, 2015/08/03 22:52:36]

Changes:

Deleted functions that relate to specific SkBmp<Name>Codecs.

Moved helper functions to protected to be shared by SkBmp<Name>Codecs.

Create new protected helper functions.

 
-// TODO: rename SkCodec_libbmp files to SkBmpCodec
 /*
- *
- * This class implements the decoding for bmp images
- *
+ * This class enables code sharing between its bmp codec subclasses.  It is the

[scroggo, 2015/08/04 16:16:46]

nit: This second sentence seems unnecessarily wordy. How about:

The subclasses actually do the work.

[msarett, 2015/08/04 23:14:44]

Yep agreed

[msarett, 2015/08/04 23:14:44]

Done.

+ * subclasses which perform the actual decoding.
  */
 class SkBmpCodec : public SkCodec {
 public:
 
     /*
-     *
      * Describes if rows of the input start at the top or bottom of the image
-     *
      */
     enum RowOrder {
         kTopDown_RowOrder,
@@ -33,161 +31,84 @@ public:
     };
 
     /*
-     *
      * Checks the start of the stream to see if the image is a bmp
-     *
      */
     static bool IsBmp(SkStream*);
 
     /*
-     *
      * Assumes IsBmp was called and returned true
      * Creates a bmp decoder
      * Reads enough of the stream to determine the image format
-     *
      */
     static SkCodec* NewFromStream(SkStream*);
 
     /*
-     *
      * Creates a bmp decoder for a bmp embedded in ico
      * Reads enough of the stream to determine the image format
-     *
      */
     static SkCodec* NewFromIco(SkStream*);
 
 protected:
 
-    /*
-     *
-     * Initiates the bmp decode
-     *
-     */
-    Result onGetPixels(const SkImageInfo& dstInfo, void* dst,
-                       size_t dstRowBytes, const Options&, SkPMColor*,
-                       int*) override;
+    SkBmpCodec(const SkImageInfo& info, SkStream* stream, uint16_t bitsPerPixel,
+            RowOrder rowOrder);
 
     SkEncodedFormat onGetEncodedFormat() const override { return kBMP_SkEncodedFormat; }
 
-private:
-
-    /*
-     *
-     * Used to define the input format of the bmp
-     *
-     */
-    enum BitmapInputFormat {
-        kStandard_BitmapInputFormat,
-        kRLE_BitmapInputFormat,
-        kBitMask_BitmapInputFormat,
-        kUnknown_BitmapInputFormat
-    };
-
-    /*
-     *
-     * Creates the color table
-     * Sets colorCount to the new color count if it is non-NULL
-     */
-     bool createColorTable(SkAlphaType alphaType, int* colorCount);
-
-    /*
-     *
-     * Creates a bmp decoder
-     * Reads enough of the stream to determine the image format
-     *
-     */
-    static SkCodec* NewFromStream(SkStream*, bool isIco);
-
     /*
-     *
      * Read enough of the stream to initialize the SkBmpCodec. Returns a bool
      * representing success or failure. If it returned true, and codecOut was
      * not NULL, it will be set to a new SkBmpCodec.
      * Does *not* take ownership of the passed in SkStream.
-     *
      */
     static bool ReadHeader(SkStream*, bool isIco, SkCodec** codecOut);
 
     /*
-     *
-     * Performs the bitmap decoding for bit masks input format
-     *
+     * Fix the decoded output when a bmp is encoded as fully transparent.
+     * It was most likely not intended to be opaque.
      */
-    Result decodeMask(const SkImageInfo& dstInfo, void* dst,
-                      size_t dstRowBytes, const Options& opts);
+    static SkCodec::Result FixTransparentDecode(void* dst, size_t dstRowBytes,

[msarett, 2015/08/03 22:52:36]

FixTransparentDecode() is new.  It is an improvement over the old method of
fixing transparent decodes in several ways.

1) It is used by both SkBmpMaskCodec and SkBmpStandardCodec.  This is a behavior
change in that we used to not fix transparent decodes in Standard mode.  We do
not have any test images for Standard mode that require this fix.  In fact, we
don't even have any images in Standard mode that use alpha.  The chromium bmp
decoder claims that these images exist and that we need to correct them.  Having
this function makes it easy, so why not go ahead and correct them (and get rid
of a giant FIXME).

2) We now decode bmps in Mask mode line by line instead of allocating a whole
image sized block of memory.  We can do this because this version of the fix
restarts the entire decode rather reusing an image buffer sized block of memory.
 This will be significant when we implement scanline decoders.

This also makes fixing transparent decodes worse one way:

1) We must allow callers to ask for kOpaque when we recommend kUnpremul.  This
is because, in FixTransparentDecode, we repeat the decode with kOpaque passed in
as the alpha type.

[scroggo, 2015/08/04 16:16:46]

We should find those images. Have you checked out src-internal in Chromium? (See
https://wiki.corp.google.com/twiki/bin/view/Main/ChromeBuildInstructions) It
contains some test images you could look at.
I think the problem is larger than that. For a PNG image, for example, we may
recommend unpremul because the image uses a format that supports alpha, but we
may find out during decode that none of the pixels actually have a non-opaque
alpha. So we recommend unpremul, and we reject a request for opaque, but the
image actually *is* opaque. 

[msarett, 2015/08/04 23:14:44]

I'll chat with you tomorrow, but I checked out src-internal and I'm not finding
extra test images.  I think we went through not being able to find a test image
for FixTransparentDecode in standard mode, which is ultimately why we decided to
disable it with a FIXME.  I reenabled it because I thought this new design makes
it easy to fix and doesn't require an image sized buffer.

+            SkPMColor* colorPtr, int* colorCountPtr, SkStream* stream);

[scroggo, 2015/08/04 16:16:46]

Can you update the comment that this requires a call to stream::duplicate?

That said, I do not know that that is such a good idea. We need to be able to
use this class when duplicating the stream is not necessarily doable (unless, of
course, we copy the stream into an SkData, like I believe we do in ICO).

[msarett, 2015/08/04 23:14:44]

Yeah it's unfortunate.  I see the trade off as:
Rare images decode incorrectly when we cannot duplicate the stream
OR
We cannot scanline decode Mask bmps (and maybe not Standard bmps either if we
can find a test case where FixTransparentDecode is necessary).

 
     /*
-     *
-     * Set an RLE pixel using the color table
-     *
+     * Rewinds the image stream if necessary
      */
-    void setRLEPixel(void* dst, size_t dstRowBytes,
-                     const SkImageInfo& dstInfo, uint32_t x, uint32_t y,
-                     uint8_t index);
+    bool handleRewind(bool isIco);
+
     /*
-     *
-     * Set an RLE24 pixel from R, G, B values
-     *
+     * Get the destination row to start filling from
+     * Used to fill the remainder of the image on incomplete input for bmps
      */
-    void setRLE24Pixel(void* dst, size_t dstRowBytes,
-                       const SkImageInfo& dstInfo, uint32_t x, uint32_t y,
-                       uint8_t red, uint8_t green, uint8_t blue);
+    void* getDstStartRow(void* dst, size_t dstRowBytes, int32_t y) const;
 
     /*
-     *
-     * Performs the bitmap decoding for RLE input format
-     *
+     * Accessors used by subclasses
      */
-    Result decodeRLE(const SkImageInfo& dstInfo, void* dst,
-                     size_t dstRowBytes, const Options& opts);
+    const uint16_t bitsPerPixel() const { return fBitsPerPixel; }

[scroggo, 2015/08/04 16:16:46]

It seems a little weird to return a const basic data type. Did you mean to
return a const reference?

[msarett, 2015/08/04 23:14:44]

Nope let's just make it a basic data type.

+    const RowOrder rowOrder() const {return fRowOrder; }

[scroggo, 2015/08/04 16:16:46]

nit: space between "{" and "return"

[msarett, 2015/08/04 23:14:45]

Done.

+
+private:
 
     /*
-     *
-     * Performs the bitmap decoding for standard input format
-     *
+     * Used to define the input format of the bmp
      */
-    Result decode(const SkImageInfo& dstInfo, void* dst, size_t dstRowBytes, const Options& opts);
+    enum BmpInputFormat {

[scroggo, 2015/08/04 16:16:46]

It looks like this is no longer used by the header file. How do you feel about
moving it into the implementation with the other enums.

[msarett, 2015/08/04 23:14:44]

Done.

+        kStandard_BmpInputFormat,
+        kRLE_BmpInputFormat,
+        kBitMask_BmpInputFormat,
+        kUnknown_BmpInputFormat
+    };
 
     /*
-     *
-     * Creates an instance of the decoder
-     * Called only by NewFromStream
-     *
-     * @param srcInfo contains the source width and height
-     * @param stream the stream of image data
-     * @param bitsPerPixel the number of bits used to store each pixel
-     * @param format the format of the bmp file
-     * @param masks optional color masks for certain bmp formats, passes
-                    ownership to SkBmpCodec
-     * @param numColors the number of colors in the color table
-     * @param bytesPerColor the number of bytes in the stream used to represent
-                            each color in the color table
-     * @param offset the offset of the image pixel data from the end of the
-     *               headers
-     * @param rowOrder indicates whether rows are ordered top-down or bottom-up
-     * @param RLEBytes used only for RLE decodes, as we must decode all
-     *                  of the data at once rather than row by row
-     *                  it indicates the amount of data left in the stream
-     *                  after decoding the headers
-     *
+     * Creates a bmp decoder
+     * Reads enough of the stream to determine the image format
      */
-    SkBmpCodec(const SkImageInfo& srcInfo, SkStream* stream,
-               uint16_t bitsPerPixel, BitmapInputFormat format,
-               SkMasks* masks, uint32_t numColors, uint32_t bytesPerColor,
-               uint32_t offset, RowOrder rowOrder, size_t RLEBytes,
-               bool isIco);
-
-    // Fields
-    const uint16_t                      fBitsPerPixel;
-    const BitmapInputFormat             fInputFormat;
-    SkAutoTDelete<SkMasks>              fMasks;          // owned
-    SkAutoTUnref<SkColorTable>          fColorTable;     // owned
-    uint32_t                            fNumColors;
-    const uint32_t                      fBytesPerColor;
-    const uint32_t                      fOffset;
-    const RowOrder                      fRowOrder;
-    const size_t                        fRLEBytes;
-    const bool                          fIsIco;
+    static SkCodec* NewFromStream(SkStream*, bool isIco);
+
+    const uint16_t fBitsPerPixel;
+    const RowOrder fRowOrder;
 
     typedef SkCodec INHERITED;
 };
+
+#endif