Enabling ico decoding with use of png and bmp decoders

src/codec/SkCodec_libico.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 "SkCodec_libico.h"
+#include "SkCodec_libpng.h"
+#include "SkCodecPriv.h"
+#include "SkColorPriv.h"
+#include "SkData.h"
+#include "SkStream.h"
+
+/*
+ *
+ * Checks the start of the stream to see if the image is an Ico or Cur
+ *
+ */
+bool SkIcoCodec::IsIco(SkStream* stream) {
+    const char icoSig[] = { '\x00', '\x00', '\x01', '\x00' };
+    const char curSig[] = { '\x00', '\x00', '\x02', '\x00' };
+    char buffer[sizeof(icoSig)];
+    return stream->read(buffer, sizeof(icoSig)) == sizeof(icoSig) &&
+            (!memcmp(buffer, icoSig, sizeof(icoSig)) ||
+            !memcmp(buffer, curSig, sizeof(curSig)));
+}
+
+/*
+ *
+ * Assumes IsIco was called and returned true
+ * Creates an Ico decoder
+ * Reads enough of the stream to determine the image format
+ *
+ */
+SkCodec* SkIcoCodec::NewFromStream(SkStream* stream) {
+    // Header size constants
+    static const uint32_t kIcoDirectoryBytes = 6;
+    static const uint32_t kIcoDirEntryBytes = 16;
+
+    // Read the directory header
+    SkAutoTDeleteArray<uint8_t> dirBuffer(
+            SkNEW_ARRAY(uint8_t, kIcoDirectoryBytes));
+    if (stream->read(dirBuffer.get(), kIcoDirectoryBytes) !=
+            kIcoDirectoryBytes) {
+        SkDebugf("Error: unable to read ico directory header.\n");
+        return NULL;
+    }
+
+    // Process the directory header
+    const uint16_t numImages = get_short(dirBuffer.get(), 4);
+    if (0 == numImages) {
+        SkDebugf("Error: No images embedded in ico.\n");
+        return NULL;
+    }
+
+    // Ensure that we can read all of indicated directory entries
+    SkAutoTDeleteArray<uint8_t> entryBuffer(
+            SkNEW_ARRAY(uint8_t, numImages*kIcoDirEntryBytes));
+    if (stream->read(entryBuffer.get(), numImages*kIcoDirEntryBytes) !=
+            numImages*kIcoDirEntryBytes) {
+        SkDebugf("Error: unable to read ico directory entries.\n");
+        return NULL;
+    }
+
+    // Ico files may contain multiple embedded images (Bmp or Png).  We will
+    // decode the "best" of these embedded images.  The largest image in size

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

In getPixels, the client will specify a desired SkImageInfo. I think the "best"
image is the one that most closely matches the desired info (possibly choosing a
step bigger if there isn't a size that matches exactly). We should also
implement onGetScaledDimensions, so the client can find an image which matches
their desired scale.

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

Done.

+    // is considered the "best".  If two images have equal size, the image with
+    // more bits per pixel is the "best".  Note that bitsPerPixel is often left

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

If the client wanted 565, maybe an 8 bit image is not the best?

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

Acknowledged.

+    // blank, and we try to infer it from the color palette.  This inferred
+    // value is not what we use for the decode, it is just to guess the "best"
+    // image.
+    uint32_t bytesRead = kIcoDirectoryBytes + numImages*kIcoDirEntryBytes;
+    int bestWidth = 0;
+    int bestHeight = 0;
+    int bestSize = 0;
+    uint16_t bestBitsPerPixel = 0;
+    uint32_t offset = 0;
+    for (uint32_t i = 0; i < numImages; i++) {
+        // Read the width and height
+        // Width and height are both stored in a single byte, 0 is used as 256
+        int width = get_byte(entryBuffer.get(), 0 + i*kIcoDirEntryBytes);
+        if (width == 0) {
+            width = 256;
+        }
+        int height = get_byte(entryBuffer.get(), 1 + i*kIcoDirEntryBytes);
+        if (height == 0) {
+            height = 256;
+        }
+        int size = width * height;
+
+        // Read or infer the bit depth
+        uint32_t bitsPerPixel = get_short(entryBuffer.get(),
+                6 + i*kIcoDirEntryBytes);
+        if (0 == bitsPerPixel) {
+            // Number of colors in the color table
+            uint32_t numColors = get_byte(entryBuffer.get(),
+                    2 + i*kIcoDirEntryBytes);
+            // This is not documented in the spec but used by many real images
+            if (0 == numColors) {
+                numColors = 256;
+            }
+            // Guess the number of bits per pixel
+            for (uint32_t i = numColors - 1; i != 0; i >>= 1) {
+                bitsPerPixel++;
+            }
+        }
+
+        // Determine if the new entry is "best"
+        bool best = (bestSize == size) ?
+                (bitsPerPixel > bestBitsPerPixel) : (size > bestSize);
+
+        if (best) {
+            bestWidth = width;
+            bestHeight = height;
+            bestSize = size;
+            bestBitsPerPixel = bitsPerPixel;
+
+            // Offset of the image data from the start of the stream
+            offset = get_int(entryBuffer.get(),
+                    12 + i*kIcoDirEntryBytes);
+            if (offset < bytesRead) {
+                SkDebugf("Error: invalid image offset.\n");
+                return NULL;
+            }
+        }
+    }
+
+    // Create a codec for the "best" embedded image
+    if (stream->skip(offset - bytesRead) != offset - bytesRead) {
+            SkDebugf("Error: could not skip to image data.\n");
+            return NULL;
+    }
+    // We will pass the current stream to the embedded decoder
+    const bool isPng = SkPngCodec::IsPng(stream);
+    if (!stream->move(-PNG_BYTES_TO_CHECK)) {

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

Unfortunately, we cannot depend on this call on Android. Also, this depends on
internal knowledge of how IsPng is implemented, which is dangerous if PNG
decides to do something different.

If SkPngCodec gave us some more info/methods, we could know how much it read,
and tell it to skip that much (libpng actually has a method to tell it how many
you have read from the start). That doesn't help us in the case where it was BMP
though.

We've talked about adding a peek(size_t bytes) to SkStream which would allow
something like this. (FWIW, SkMemoryStream already has a method named peek,
which does something different, but I don't see any callers, at least
internally...)

I'm not sure what the best solution is for now :(

If nothing else, we can put a giant FIXME here, but I'd like to at least get a
band-aid in before submitting.

I was thinking you could wrap the stream in an SkFrontBufferedStream (which we
use elsewhere on Android). Unfortunately, it expects to own the passed in
stream, as does the SkCodec. (I'm to blame here - SkStream used to be
refcounted, in which case this wouldn't be a problem...)

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

I have added a FIXME.

+        SkDebugf("Error: could not rewind embedded ico stream.\n");
+        return NULL;
+    }
+    SkCodec* codec;
+    if (isPng) {
+        codec = SkPngCodec::NewFromStream(stream);
+    } else {
+        // Assume the embedded image is a Bmp.  Call a special constructor
+        // because Bmp in Ico images are stored without the first header.
+        codec = SkBmpCodec::NewFromIco(stream);
+    }
+    // Check for a valid result
+    if (NULL == codec) {
+        SkDebugf("Error: could not create embedded codec.\n");
+        return NULL;
+    }
+    SkAutoTDelete<SkCodec> embeddedCodec(codec);
+    if (embeddedCodec->getOriginalInfo().width() != bestWidth ||
+            embeddedCodec->getOriginalInfo().height() != bestHeight) {
+        SkDebugf("Warning: embedded dimensions do not match.\n");
+        // We will defer to the embedded dimensions
+        bestWidth = embeddedCodec->getOriginalInfo().width();
+        bestHeight = embeddedCodec->getOriginalInfo().height();
+    }
+
+    const SkImageInfo& imageInfo = SkImageInfo::Make(bestWidth, bestHeight,

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

This should not be a reference.

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

Done.

+            embeddedCodec->getOriginalInfo().colorType(),
+            embeddedCodec->getOriginalInfo().alphaType());
+    // Note that stream is owned by the embedded codec, the ico does not need
+    // direct access to the stream.
+    return SkNEW_ARGS(SkIcoCodec, (imageInfo, NULL, embeddedCodec.detach()));
+}
+
+/*
+ *
+ * Creates an instance of the decoder
+ * Called only by NewFromStream
+ *
+ */
+SkIcoCodec::SkIcoCodec(const SkImageInfo& info, SkStream* stream,
+                       SkCodec* embeddedCodec)
+    : INHERITED(info, stream)

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

If stream is always NULL, you can remove it from the parameter list for this
constructor, and just pass NULL here.

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

Done.

+    , fEmbeddedCodec(embeddedCodec)
+{}
+
+/*
+ *
+ * Initiates the Ico decode
+ *
+ */
+SkCodec::Result SkIcoCodec::onGetPixels(const SkImageInfo& dstInfo,
+                                        void* dst, size_t dstRowBytes,
+                                        const Options& opts, SkPMColor* ct,
+                                        int* ptr) {
+    if (!this->rewindIfNeeded()) {

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

How does this work? rewindIfNeeded will attempt to dereference fStream, which is
NULL, IIUC.

Also, it seems like PNG or BMP will next call rewindIfNeeded, which would always
rewind (assuming the first one *had* worked).

Would it be possible to make NewFromStream just return an SkBmpCodec or an
SkPngCodec, skipping the wrapper altogether? (Does giving the caller an option
of which image to pick make that impossible?)

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

Giving the caller an option of which image to pick does in fact prevent us from
skipping the wrapper.

+        return kCouldNotRewind;
+    }
+    if (dstInfo.dimensions() != this->getOriginalInfo().dimensions()) {
+        SkDebugf("Error: scaling not supported.\n");
+        return kInvalidScale;
+    }
+    // We will not check if the conversion is possible because the embedded bmp
+    // or png codec will make this check for us.
+
+    // Decode the embedded image
+    return fEmbeddedCodec->getPixels(dstInfo, dst, dstRowBytes);

[msarett, 2015/03/18 19:59:25]

Forgot to mention:
I'm not sure about the implications of calling getPixels vs onGetPixels here
(onGetPixels is protected so we can't call it).  getPixels does not allow us to
pass the options object, which I feel may be an issue.

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

There are two versions of getPixels. You should call the first one, which takes
all the parameters.

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

Done.

+}