Enabling ico decoding with use of png and bmp decoders

src/codec/SkCodec_libbmp.cpp

Index: src/codec/SkCodec_libbmp.cpp
diff --git a/src/codec/SkCodec_libbmp.cpp b/src/codec/SkCodec_libbmp.cpp
index 62cda95733c97bc2c90bbe71e538800257b61d77..339d9919818189d9c9205d6bf6ceefedb03d00de 100644
--- a/src/codec/SkCodec_libbmp.cpp
+++ b/src/codec/SkCodec_libbmp.cpp
@@ -88,11 +88,31 @@ bool SkBmpCodec::IsBmp(SkStream* stream) {
 /*
  *
  * Assumes IsBmp was called and returned true
- * Creates a bitmap decoder
+ * Creates a bmp decoder
  * Reads enough of the stream to determine the image format
  *
  */
 SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) {
+    return SkBmpCodec::NewFromStream(stream, false);
+}
+
+/*
+ *
+ * Creates a bmp decoder for a bmp embedded in ico
+ * Reads enough of the stream to determine the image format
+ *
+ */
+SkCodec* SkBmpCodec::NewFromIco(SkStream* stream) {
+    return SkBmpCodec::NewFromStream(stream, true);
+}
+
+/*
+ *
+ * Creates a bmp decoder
+ * Reads enough of the stream to determine the image format
+ *
+ */
+SkCodec* SkBmpCodec::NewFromStream(SkStream* stream, const bool isIco) {
     // Header size constants
     static const uint32_t kBmpHeaderBytes = 14;
     static const uint32_t kBmpHeaderBytesPlusFour = kBmpHeaderBytes + 4;
@@ -106,39 +126,69 @@ SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) {
     static const uint32_t kBmpInfoV5Bytes = 124;
     static const uint32_t kBmpMaskBytes = 12;
 
-    // Read the first header and the size of the second header
-    SkAutoTDeleteArray<uint8_t> hBuffer(
-            SkNEW_ARRAY(uint8_t, kBmpHeaderBytesPlusFour));
-    if (stream->read(hBuffer.get(), kBmpHeaderBytesPlusFour) !=
-            kBmpHeaderBytesPlusFour) {
-        SkDebugf("Error: unable to read first bitmap header.\n");
-        return NULL;
-    }
-
     // The total bytes in the bmp file
-    // We only need to use this value for RLE decoding, so we will only check
-    // that it is valid in the RLE case.
-    const uint32_t totalBytes = get_int(hBuffer.get(), 2);
-
+    // We only need to use this value for RLE decoding, so we will only
+    // check that it is valid in the RLE case.
+    uint32_t totalBytes;
     // The offset from the start of the file where the pixel data begins
-    const uint32_t offset = get_int(hBuffer.get(), 10);
-    if (offset < kBmpHeaderBytes + kBmpOS2V1Bytes) {
-        SkDebugf("Error: invalid starting location for pixel data\n");
-        return NULL;
-    }
-
+    uint32_t offset;
     // The size of the second (info) header in bytes
     // The size is the first field of the second header, so we have already
     // read the first four infoBytes.

[scroggo, 2015/03/18 21:39:18]

No longer true.

[msarett, 2015/03/20 18:35:56]

Done.

-    const uint32_t infoBytes = get_int(hBuffer.get(), 14);
-    if (infoBytes < kBmpOS2V1Bytes) {
-        SkDebugf("Error: invalid second header size.\n");
-        return NULL;
+    uint32_t infoBytes;
+    // Bmps embedded in Icos skip the first Bmp header
+    if (!isIco) {
+        // Read the first header and the size of the second header
+        SkAutoTDeleteArray<uint8_t> hBuffer(
+                SkNEW_ARRAY(uint8_t, kBmpHeaderBytesPlusFour));
+        if (stream->read(hBuffer.get(), kBmpHeaderBytesPlusFour) !=
+                kBmpHeaderBytesPlusFour) {
+            SkDebugf("Error: unable to read first bitmap header.\n");
+            return NULL;
+        }
+
+        totalBytes = get_int(hBuffer.get(), 2);
+        offset = get_int(hBuffer.get(), 10);
+        if (offset < kBmpHeaderBytes + kBmpOS2V1Bytes) {
+            SkDebugf("Error: invalid starting location for pixel data\n");
+            return NULL;
+        }
+
+        // The size of the second (info) header in bytes
+        // The size is the first field of the second header, so we have already
+        // read the first four infoBytes.
+        infoBytes = get_int(hBuffer.get(), 14);
+        if (infoBytes < kBmpOS2V1Bytes) {
+            SkDebugf("Error: invalid second header size.\n");
+            return NULL;
+        }
+    } else {
+        // This value is only used by RLE compression.  Bmp in Ico files do not
+        // use RLE.  If the compression field is incorrectly signaled as RLE,
+        // we will catch this and signal an error below.
+        totalBytes = 0;
+
+        // Bmps in Ico cannot specify an offset.  We will always assume that

[scroggo, 2015/03/18 21:39:18]

Really? SkImageDecoder_libico reads an offset field. Was that a mistake?

[msarett, 2015/03/20 18:35:56]

Ico files have multiple directory entries.  Each stores an icoOffset which
indicates where the data for that directory entry begins within the Ico file. 
If it is a bmp-in-ico, this icoOffset will indicate the start of the second bmp
header (since bmp-in-ico omit the first header).  Since bmp-in-ico omit the
first header, there is no bmpOffset specified.  The bmp pixel data begins
immediately after the 2nd header or color table if it exists.

This is clearly confusing in the code.  I hope that adding comments to
SkCodec_libico.cpp will help?

+        // pixel data begins immediately after the color table.  This value
+        // will be corrected below.
+        offset = 0;
+
+        // Read the size of the second header
+        SkAutoTDeleteArray<uint8_t> hBuffer(
+                SkNEW_ARRAY(uint8_t, 4));
+        if (stream->read(hBuffer.get(), 4) != 4) {
+            SkDebugf("Error: unable to read size of second bitmap header.\n");
+            return NULL;
+        }
+        infoBytes = get_int(hBuffer.get(), 0);
+        if (infoBytes < kBmpOS2V1Bytes) {
+            SkDebugf("Error: invalid second header size.\n");
+            return NULL;
+        }
     }
-    const uint32_t infoBytesRemaining = infoBytes - 4;
-    hBuffer.free();
 
     // Read the second header
+    const uint32_t infoBytesRemaining = infoBytes - 4;
     SkAutoTDeleteArray<uint8_t> iBuffer(
             SkNEW_ARRAY(uint8_t, infoBytesRemaining));
     if (stream->read(iBuffer.get(), infoBytesRemaining) != infoBytesRemaining) {
@@ -243,6 +293,11 @@ SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) {
         height = -height;
         rowOrder = kTopDown_RowOrder;
     }
+    // The height field for bmp in ico is double the actual height because they
+    // contain an XOR mask followed by an AND mask
+    if (isIco) {
+        height /= 2;
+    }
     static const int kBmpMaxDim = 1 << 16;
     if (width < 0 || width >= kBmpMaxDim || height >= kBmpMaxDim) {
         // TODO: Decide if we want to support really large bmps.
@@ -344,16 +399,19 @@ SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) {
 
     // Most versions of bmps should be rendered as opaque.  Either they do
     // not have an alpha channel, or they expect the alpha channel to be
-    // ignored.  V4+ bmp files introduce an alpha mask and allow the creator
+    // ignored.  V3+ bmp files introduce an alpha mask and allow the creator
     // of the image to use the alpha channels.  However, many of these images
-    // leave the alpha channel blank and expect to be rendered as opaque.  For
-    // this reason, we set the alpha type to kUnknown for V4+ bmps and figure
-    // out the alpha type during the decode.
+    // leave the alpha channel blank and expect to be rendered as opaque.  This
+    // is the case for almost all V3 images, so we render these as opaque.  For
+    // V4+, we will use the alpha channel, and fix the image later if it turns
+    // out to be fully transparent.
+    // As an exception, V3 bmp-in-ico may use an alpha mask.
     SkAlphaType alphaType = kOpaque_SkAlphaType;
-    if (kInfoV4_BitmapHeaderType == headerType ||
+    if ((kInfoV3_BitmapHeaderType == headerType && isIco) ||
+            kInfoV4_BitmapHeaderType == headerType ||
             kInfoV5_BitmapHeaderType == headerType) {
         // Header types are matched based on size.  If the header is
-        // V4+, we are guaranteed to be able to read at least this size.
+        // V3+, we are guaranteed to be able to read at least this size.
         SkASSERT(infoBytesRemaining > 52);
         inputMasks.alpha = get_int(iBuffer.get(), 48);
         if (inputMasks.alpha != 0) {
@@ -362,6 +420,11 @@ SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) {
     }
     iBuffer.free();
 
+    // Additionally, 32 bit bmp-in-icos use the alpha channel
+    if (isIco && 32 == bitsPerPixel) {
+        alphaType = kUnpremul_SkAlphaType;
+    }
+
     // Check for valid bits per pixel input
     switch (bitsPerPixel) {
         // In addition to more standard pixel compression formats, bmp supports
@@ -406,7 +469,7 @@ SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) {
 
     // Calculate the number of bytes read so far
     const uint32_t bytesRead = kBmpHeaderBytes + infoBytes + maskBytes;
-    if (offset < bytesRead) {
+    if (!isIco && offset < bytesRead) {
         SkDebugf("Error: pixel data offset less than header size.\n");
         return NULL;
     }
@@ -420,7 +483,7 @@ SkCodec* SkBmpCodec::NewFromStream(SkStream* stream) {
     return SkNEW_ARGS(SkBmpCodec, (imageInfo, stream, bitsPerPixel,
                                    inputFormat, masks.detach(), numColors,
                                    bytesPerColor, offset - bytesRead,
-                                   rowOrder, RLEBytes));
+                                   rowOrder, RLEBytes, isIco));
 }
 
 /*
@@ -433,7 +496,7 @@ SkBmpCodec::SkBmpCodec(const SkImageInfo& info, SkStream* stream,
                        uint16_t bitsPerPixel, BitmapInputFormat inputFormat,
                        SkMasks* masks, uint32_t numColors,
                        uint32_t bytesPerColor, uint32_t offset,
-                       RowOrder rowOrder, size_t RLEBytes)
+                       RowOrder rowOrder, size_t RLEBytes, bool isIco)
     : INHERITED(info, stream)
     , fBitsPerPixel(bitsPerPixel)
     , fInputFormat(inputFormat)
@@ -444,6 +507,7 @@ SkBmpCodec::SkBmpCodec(const SkImageInfo& info, SkStream* stream,
     , fOffset(offset)
     , fRowOrder(rowOrder)
     , fRLEBytes(RLEBytes)
+    , fIsIco(isIco)
 {}
 
 /*
@@ -553,20 +617,25 @@ SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
         }
     }
 
-    // Check that we have not read past the pixel array offset
-    if(fOffset < colorBytes) {
-        // This may occur on OS 2.1 and other old versions where the color
-        // table defaults to max size, and the bmp tries to use a smaller color
-        // table.  This is invalid, and our decision is to indicate an error,
-        // rather than try to guess the intended size of the color table.
-        SkDebugf("Error: pixel data offset less than color table size.\n");
-        return false;
-    }
+    // Bmp-in-Ico files do not use an offset to indicate where the pixel data
+    // begins.  Pixel data always begins immediately after the color table.
+    if (!fIsIco) {

[scroggo, 2015/03/18 21:39:18]

I wonder if it would be possible/better to split into a few classes, possibly
subclassing from SkBmpCodec.

Looking at the list of member variables we have a few that are only valid for
different types:
fRLEBytes - only used for RLE
fMasks - possibly its own set (or used by RLE and others?)
fOffset - not used by ico
fIsIco - not needed if we used different classes

We'd still need to make sure we share code when we can, and maybe it wouldn't
split up as cleanly as I imagine.

[msarett, 2015/03/20 18:35:56]

It wouldn't be a clean split, but it's possible that some version of this would
be superior to the current design.  Still thinking about this.

+        // Check that we have not read past the pixel array offset
+        if(fOffset < colorBytes) {
+            // This may occur on OS 2.1 and other old versions where the color
+            // table defaults to max size, and the bmp tries to use a smaller
+            // color table.  This is invalid, and our decision is to indicate
+            // an error, rather than try to guess the intended size of the
+            // color table.
+            SkDebugf("Error: pixel data offset less than color table size.\n");
+            return false;
+        }
 
-    // After reading the color table, skip to the start of the pixel array
-    if (stream()->skip(fOffset - colorBytes) != fOffset - colorBytes) {
-        SkDebugf("Error: unable to skip to image data.\n");
-        return false;
+        // After reading the color table, skip to the start of the pixel array
+        if (stream()->skip(fOffset - colorBytes) != fOffset - colorBytes) {
+            SkDebugf("Error: unable to skip to image data.\n");
+            return false;
+        }
     }
 
     // Set the color table and return true on success
@@ -936,7 +1005,7 @@ SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
             config = SkSwizzler::kBGR;
             break;
         case 32:
-            if (kOpaque_SkAlphaType == dstInfo.alphaType()) {
+            if (kOpaque_SkAlphaType == dstInfo.alphaType() && !fIsIco) {
                 config = SkSwizzler::kBGRX;
             } else {
                 config = SkSwizzler::kBGRA;
@@ -987,7 +1056,6 @@ SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
     // SkSwizzler does not support.  Firstly, all bmp images that contain
     // alpha are masked by the alpha mask.  Secondly, many fully transparent
     // bmp images are intended to be opaque.  Here, we make those corrections.
-    // Modifying alpha is safe because colors are stored unpremultiplied.
     /*
     SkPMColor* dstRow = (SkPMColor*) dst;
     if (SkSwizzler::kBGRA == config) {
@@ -1004,6 +1072,66 @@ SkCodec::Result SkBmpCodec::decode(const SkImageInfo& dstInfo,
     }
     */
 
+    // Finally, apply the AND mask for bmp-in-ico images
+    if (fIsIco) {
+        // The AND mask is always 1 bit per pixel
+        const size_t rowBytes = SkAlign4(compute_row_bytes(width, 1));
+
+        // Find the proper start row and delta
+        int delta;
+        if (kTopDown_RowOrder == fRowOrder) {
+            dst = (SkPMColor*) dst;
+            delta = dstRowBytes;
+        } else {
+            dst = (SkPMColor*) SkTAddOffset<void>(dst,
+                    (height-1) * dstRowBytes);
+            delta = -dstRowBytes;
+        }
+
+        SkPMColor* dstRow32 = (SkPMColor*) dst;
+        uint16_t* dstRow16 = (uint16_t*) dst;
+        for (int y = 0; y < height; y++) {
+            // The srcBuffer will at least be large enough
+            if (stream()->read(srcBuffer.get(), rowBytes) != rowBytes) {
+                SkDebugf("Warning: incomplete AND mask for bmp-in-ico.\n");
+                return kIncompleteInput;
+            }
+
+            // If the AND bit is set, the pixel is transparent, and if the bit
+            // is zero, the pixel is unchanged.
+            switch (dstInfo.colorType()) {
+                case kN32_SkColorType: {
+                    for (int x = 0; x < width; x++) {
+                        uint32_t shift = 7 - (x % 8);
+                        uint32_t alphaBit =
+                                (srcBuffer.get()[x / 8] >> shift) & 0x1;

[scroggo, 2015/03/18 21:39:18]

nit: Avoiding divides in a loop is generally a good thing. Can we switch to x >>
3?

Ooh, even better: You are also doing a mod up above, and we have a function to
do both faster in SkMath.h: SkTDivMod

[msarett, 2015/03/20 18:35:56]

That's clever!  Done.

+                        dstRow32[x] &= alphaBit - 1;
+                    }
+                    dstRow32 = SkTAddOffset<SkPMColor>(dstRow32, delta);
+                    break;
+                }
+                case kRGB_565_SkColorType: {

[msarett, 2015/03/18 20:04:23]

Forgot to mention part 2:
This case can be removed and this code can be simplified quite a bit when I
remove the swizzles to 565.  As discussed, we do not need to enable decoding to
565 when the input has 24 or 32 bpp.  All inputs that use this function are 24
or 32 bpp.

+                    for (int x = 0; x < width; x++) {
+                        uint16_t shift = 7 - (x % 8);
+                        uint16_t alphaBit =
+                                (srcBuffer.get()[x / 8] >> shift) & 0x1;
+                        // Since there is no transparency in 565, we set the
+                        // pixel to white when the AND bit is set.
+                        dstRow16[x] |= ~(alphaBit - 1);
+                    }
+                    dstRow16 = SkTAddOffset<uint16_t>(dstRow16, delta);
+                    break;
+                }
+                default:
+                    // This case should not be reached.  We should catch an
+                    // invalid color type when we check that the conversion is
+                    // possible.
+                    SkASSERT(false);
+                    break;
+            }
+        }
+    }
+
     // Finished decoding the entire image
     return kSuccess;
 }