Bmp Image Decoding

src/codec/SkCodec_libbmp.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.
+ */
+
+#include "SkCodec.h"
+#include "SkImageInfo.h"
+#include "SkStream.h"
+#include "SkTypes.h"
+
+#ifdef SK_SUPPORT_LEGACY_IMAGE_GENERATOR_RETURN
+    #define onGetPixels onGetPixelsEnum
+#endif
+
+/*
+ * 
+ * This class implements the decoding for bitmap images

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

nit: "bitmap" is unfortunately overloaded. I'd prefer saying "BMP" or "bmp", so
as not to confuse with "SkBitmap". Maybe others won't find this confusing
though.

+ *
+ */
+class SkBmpCodec : public SkCodec {
+public:
+
+    /*
+     *
+     * Checks the start of the stream to see if the image is a bitmap
+     *
+     */
+    static bool IsBmp(SkStream*);
+    
+    /*
+     * 
+     * Assumes IsBmp was called and returned true
+     * Creates a bitmap decoder
+     * Reads enough of the stream to determine the image format
+     *
+     */
+    static SkCodec* NewFromStream(SkStream*);
+    
+protected:
+
+    /*
+     *
+     * Initiates the bitmap decode
+     *
+     */
+    virtual Result onGetPixels(const SkImageInfo&, void*, size_t, SkPMColor*,
+            int*) SK_OVERRIDE;
+private:
+
+    /*
+     *
+     * Used to define the inout format of the bitmap
+     *
+     */
+    enum BitmapInputFormat {
+        kStandard_BitmapInputFormat,
+        k4BitRLE_BitmapInputFormat,
+        k8BitRLE_BitmapInputFormat,
+        kBitMask_BitmapInputFormat,
+        kUnknown_BitmapInputFormat
+    };
+
+    /*
+     *
+     * Performs the bitmap decoding for standard and bit masks input format
+     *
+     */
+    Result decode(const SkImageInfo&, void*, uint32_t);
+    
+    /*
+     *
+     * Creates an instance of the decoder
+     * Called only by NewFromStream
+     *
+     */
+    SkBmpCodec(const SkImageInfo&, SkStream*, const uint16_t,

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

Can you add parameter names?

Also, please document which parameters the SkBmpCodec takes ownership of.

+               const BitmapInputFormat, uint32_t*, SkPMColor*, const bool);

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

Typically we do not mark primitive parameters as const.

+
+    /*
+     *
+     * Clean up memory used by the decoder
+     *
+     */
+    ~SkBmpCodec();
+    
+    // Constants
+    static const int kBmpHeaderBytes = 14;

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

These are all details of the implementation. I think they belong in the cpp
file.

+    static const int kBmpOS2V1Bytes =  12;
+    static const int kBmpOS2V2Bytes =  64;
+    static const int kBmpInfoV1Bytes = 40;
+    static const int kBmpInfoV2Bytes = 52;
+    static const int kBmpInfoV3Bytes = 56;
+    static const int kBmpInfoV4Bytes = 108;
+    static const int kBmpInfoV5Bytes = 124;
+    static const int kBmpMaskBytes =   12;
+    static const int kBmpMaxDim =      1 << 16;
+
+    const uint16_t                 fBitsPerPixel;
+    const BitmapInputFormat        fInputFormat;
+    const SkAutoTDelete<uint32_t>  fBitMasks;
+    const SkAutoTDelete<SkPMColor> fColorTable;

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

I believe this is an array. SkAutoTDelete calls "delete", not "delete[]".

I'd lean towards using an SkColorTable (with SkAutoTUnref), but you could also
use SkAutoTDeleteArray if you'd rather stick with the array.

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

Oops I meant to make these SkAutoTDeleteArray.

+    const bool fInverted;
+    
+    typedef SkCodec INHERITED;
+};