Enabling ico decoding with use of png and bmp decoders

src/codec/SkCodec_libbmp.cpp

 /*
  * 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_libbmp.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
@@ skipping 70 matching lines @@
     // TODO: ICO files may contain a BMP and need to use this decoder
     const char bmpSig[] = { 'B', 'M' };
     char buffer[sizeof(bmpSig)];
     return stream->read(buffer, sizeof(bmpSig)) == sizeof(bmpSig) &&
             !memcmp(buffer, bmpSig, sizeof(bmpSig));
 }
 
 /*
  *
  * 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;
     static const uint32_t kBmpOS2V1Bytes = 12;
     static const uint32_t kBmpOS2V2Bytes = 64;
     static const uint32_t kBmpInfoBaseBytes = 16;
     static const uint32_t kBmpInfoV1Bytes = 40;
     static const uint32_t kBmpInfoV2Bytes = 52;
     static const uint32_t kBmpInfoV3Bytes = 56;
     static const uint32_t kBmpInfoV4Bytes = 108;
     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) {
         SkDebugf("Error: unable to read second bitmap header.\n");
         return NULL;
     }
 
     // The number of bits used per pixel in the pixel data
     uint16_t bitsPerPixel;
 
@@ skipping 84 matching lines @@
         SkDebugf("Error: second bitmap header size is invalid.\n");
         return NULL;
     }
 
     // Check for valid dimensions from header
     RowOrder rowOrder = kBottomUp_RowOrder;
     if (height < 0) {
         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.
         SkDebugf("Error: invalid bitmap dimensions.\n");
         return NULL;
     }
 
     // Create mask struct
     SkMasks::InputMasks inputMasks;
     memset(&inputMasks, 0, sizeof(SkMasks::InputMasks));
@@ skipping 81 matching lines @@
             // TODO: Same as above.
             SkDebugf("Error: CMYK not supported for bitmap decoding.\n");
             return NULL;
         default:
             SkDebugf("Error: invalid format for bitmap decoding.\n");
             return NULL;
     }
 
     // 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) {
             alphaType = kUnpremul_SkAlphaType;
         }
     }
     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
         // the use of bit masks to determine pixel components.  The standard
         // format for representing 16-bit colors is 555 (XRRRRRGGGGGBBBBB),
         // which does not map well to any Skia color formats.  For this reason,
         // we will always enable mask mode with 16 bits per pixel.
         case 16:
             if (kBitMask_BitmapInputFormat != inputFormat) {
                 inputMasks.red = 0x7C00;
@@ skipping 24 matching lines @@
 
     // Check for a valid number of total bytes when in RLE mode
     if (totalBytes <= offset && kRLE_BitmapInputFormat == inputFormat) {
         SkDebugf("Error: RLE requires valid input size.\n");
         return NULL;
     }
     const size_t RLEBytes = totalBytes - offset;
 
     // 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;
     }
 
     // Return the codec
     // We will use ImageInfo to store width, height, and alpha type.  We will
     // set color type to kN32_SkColorType because that should be the default
     // output.
     const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
             kN32_SkColorType, alphaType);
     return SkNEW_ARGS(SkBmpCodec, (imageInfo, stream, bitsPerPixel,
                                    inputFormat, masks.detach(), numColors,
                                    bytesPerColor, offset - bytesRead,
-                                   rowOrder, RLEBytes));
+                                   rowOrder, RLEBytes, isIco));
 }
 
 /*
  *
  * Creates an instance of the decoder
  * Called only by NewFromStream
  *
  */
 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)
     , fMasks(masks)
     , fColorTable(NULL)
     , fNumColors(numColors)
     , fBytesPerColor(bytesPerColor)
     , fOffset(offset)
     , fRowOrder(rowOrder)
     , fRLEBytes(RLEBytes)
+    , fIsIco(isIco)
 {}
 
 /*
  *
  * Initiates the bitmap decode
  *
  */
 SkCodec::Result SkBmpCodec::onGetPixels(const SkImageInfo& dstInfo,
                                         void* dst, size_t dstRowBytes,
                                         const Options&,
@@ skipping 89 matching lines @@
         }
 
         // To avoid segmentation faults on bad pixel data, fill the end of the
         // color table with black.  This is the same the behavior as the
         // chromium decoder.
         for (; i < maxColors; i++) {
             colorTable[i] = SkPackARGB32NoCheck(0xFF, 0, 0, 0);
         }
     }
 
-    // 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
     fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorTable, maxColors)));
     return true;
 }
 
 /*
  *
  * Performs the bitmap decoding for bit masks input format
@@ skipping 349 matching lines @@
         case 4:
             config = SkSwizzler::kIndex4;
             break;
         case 8:
             config = SkSwizzler::kIndex;
             break;
         case 24:
             config = SkSwizzler::kBGR;
             break;
         case 32:
-            if (kOpaque_SkAlphaType == dstInfo.alphaType()) {
+            if (kOpaque_SkAlphaType == dstInfo.alphaType() && !fIsIco) {
                 config = SkSwizzler::kBGRX;
             } else {
                 config = SkSwizzler::kBGRA;
             }
             break;
         default:
             SkASSERT(false);
             return kInvalidInput;
     }
 
@@ skipping 30 matching lines @@
 
     // FIXME: This code exists to match the behavior in the chromium decoder
     // and to follow the bmp specification as it relates to alpha masks.  It is
     // commented out because we have yet to discover a test image that provides
     // an alpha mask and uses this decode mode.
 
     // Now we adjust the output image with some additional behavior that
     // 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) {
         for (int y = 0; y < height; y++) {
             for (int x = 0; x < width; x++) {
                 if (transparent) {
                     dstRow[x] |= 0xFF000000;
                 } else {
                     dstRow[x] &= alphaMask;
                 }
                 dstRow = SkTAddOffset<SkPMColor>(dstRow, dstRowBytes);
             }
         }
     }
     */
 
+    // 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;
 }