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"
+#include "SkTSort.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;
+    }
+
+    // This structure is used to represent the vital information about entries
+    // in the directory header.  We will obtain this information for each
+    // directory entry.
+    struct Entry {
+        uint32_t offset;
+        uint32_t size;
+    };
+    SkAutoTDeleteArray<Entry> directoryEntries(SkNEW_ARRAY(Entry, numImages));
+
+    // Iterate over directory entries
+    for (uint32_t i = 0; i < numImages; i++) {
+        // The directory entry contains information such as width, height,
+        // bits per pixel, and number of colors in the color palette.  We will
+        // ignore these fields since they are repeated in the header of the
+        // embedded image.  In the event of an inconsistency, we would always
+        // defer to the value in the embedded header anyway.
+
+        // Specifies the size of the embedded image, including the header
+        uint32_t size = get_int(entryBuffer.get(), 8 + i*kIcoDirEntryBytes);
+
+        // Specifies the offset of the embedded image from the start of file.
+        // It does not indicate the start of the pixel data, but rather the
+        // start of the embedded image header.
+        uint32_t offset = get_int(entryBuffer.get(), 12 + i*kIcoDirEntryBytes);
+
+        // Save the vital fields
+        directoryEntries.get()[i].offset = offset;
+        directoryEntries.get()[i].size = size;
+    }
+
+    // It is "customary" that the embedded images will be stored in order of
+    // increasing offset.  However, the specification does not indicate that
+    // they must be stored in this order, so we will not trust that this is the
+    // case.  Here we sort the embedded images by increasing offset.
+    struct EntryLessThan {
+        bool operator() (Entry a, Entry b) const {
+            return a.offset < b.offset;
+        }
+    };
+    EntryLessThan lessThan;
+    SkTQSort(directoryEntries.get(), directoryEntries.get() + numImages - 1,
+            lessThan);
+
+    // Now will construct a candidate codec for each of the embedded images
+    uint32_t bytesRead = kIcoDirectoryBytes + numImages*kIcoDirEntryBytes;
+    SkAutoTDeleteArray<SkCodec*> codecs(SkNEW_ARRAY(SkCodec*, numImages));
+    uint32_t index = 0;
+    for (uint32_t i = 0; i < numImages; i++) {
+        uint32_t offset = directoryEntries.get()[i].offset;
+        uint32_t size = directoryEntries.get()[i].size;
+        
+        // Ensure that the offset is valid
+        if (offset < bytesRead) {
+            SkDebugf("Warning: invalid ico offset.\n");
+            continue;
+        }
+
+        // If we cannot skip, assume we have reached the end of the stream and
+        // stop trying to make codecs
+        if (stream->skip(offset - bytesRead) != offset - bytesRead) {
+            SkDebugf("Warning: could not skip to ico offset.\n");
+            break;
+        }
+        bytesRead = offset;
+
+        // Create a new stream for the embedded codec
+

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

Will remove this line.

+        SkData* data = SkData::NewFromStream(stream, size);

[scroggo, 2015/03/20 19:36:01]

SkMemoryStream's constructor will call data->ref(), and data starts with a
refcount of 1, so you need to unref data (preferably by storing it in an
SkAutoTUnref).

[msarett, 2015/03/23 12:20:57]

Done.

+        SkAutoTDelete<SkMemoryStream>
+                embeddedStream(SkNEW_ARGS(SkMemoryStream, (data)));
+        if (NULL == embeddedStream.get()) {

[scroggo, 2015/03/20 19:36:00]

You called the constructor for SkMemoryStream, so it will not be NULL. data may
be NULL, on the other hand, so you should check for that.

[msarett, 2015/03/23 12:20:57]

Done.

+            SkDebugf("Warning: could not create embedded stream.\n");
+            break;

[scroggo, 2015/03/20 19:36:00]

The only way for NewFromStream to fail is if the stream had an error or was
truncated. I think in this case we need to treat the rest of the images as
missing.

[msarett, 2015/03/23 12:20:58]

Agreed.  That is why we break out of the loop here.  We may still have a chance
to decode any images that we identified before the stream was truncated.

[scroggo, 2015/03/23 13:41:24]

Oh, of course. nvm

+        }
+        bytesRead += size;
+
+        // Use the new stream to create the embedded decoder
+        // FIXME: Implement peek() for SkMemoryStream and use it to determine

[scroggo, 2015/03/20 19:36:00]

Actually, in this case, since you've created an SkMemoryStream, we don't need to
worry about peeking. SkMemoryStream can be rewound (cheaply!), and it will
rewind to its own beginning.

I think you should go ahead and behave similarly to SkCodec::NewFromStream:

call IsPng, rewind, return SkPngCodec::NewFromStrea if IsPng
call IsBMP, rewind, return SkBmpCodec::NewFromIco

[msarett, 2015/03/23 12:20:57]

Gotcha, will do.  We won't call isBmp because the first bmp header (including
the signature) is omitted.  Instead, if it isn't a png, we assume it's a bmp.

+        //        if we have a png or bmp.
+        const char pngSig[] = { '\x89', 'P', 'N', 'G' };
+        char* buffer[sizeof(pngSig)];
+        if (embeddedStream->read(buffer, sizeof(pngSig))
+                != sizeof(pngSig)) {
+            break;
+        }
+        if (!embeddedStream->move(-sizeof(pngSig))) {
+            SkDebugf("Warning: could not rewind embedded ico stream.\n");
+            continue;
+        }
+
+        // Check if the embedded codec is bmp or png and create the codec
+        SkCodec* codec;
+        if (memcmp(buffer, pngSig, sizeof(pngSig))) {
+            codec = SkBmpCodec::NewFromIco(embeddedStream.detach());
+        } else {
+            codec = SkPngCodec::NewFromStream(embeddedStream.detach());
+        }
+
+        // Save a valid codec
+        if (NULL != codec) {
+            codecs.get()[index++] = codec;

[scroggo, 2015/03/20 19:36:01]

I think you might be better served here by using an SkTDArray<SkCodec*> or
SkTArray<SkCodec*>. These arrays are similar to vector, allowing you to append
new items and keeping track of the count.

[msarett, 2015/03/23 12:20:57]

Yeah of course.  I was trying to use SkTArray but I couldn't figure out to take
away owership.  Glad to know there is SkTDArray.

+        }
+    }
+    uint32_t numValidImages = index;
+
+    // Use the largest codec as a "suggestion" for dimensions
+    int maxWidth = 0;
+    int maxHeight = 0;
+    SkColorType maxColorType = kUnknown_SkColorType;
+    SkAlphaType maxAlphaType = kUnknown_SkAlphaType;
+    for (uint32_t i = 0; i < numValidImages; i++) {
+        int width = codecs.get()[i]->getOriginalInfo().width();

[scroggo, 2015/03/20 19:36:01]

If you're up to date, you can call getInfo() instead of getOriginalInfo().

[msarett, 2015/03/23 12:20:57]

Done.

+        int height = codecs.get()[i]->getOriginalInfo().height();
+        if (width * height > maxWidth * maxHeight) {
+            maxWidth = width;
+            maxHeight = height;
+            maxColorType = codecs.get()[i]->getOriginalInfo().colorType();

[scroggo, 2015/03/20 19:36:00]

I think it might be worth it to set up a local variable for the info so we can
simplify these.

Actually, why not just store the index of the largest one? (You need to store
the maxWidth/maxHeight, but the other ones seem unnecessary.)

[msarett, 2015/03/23 12:20:57]

Yes this is better.

+            maxAlphaType = codecs.get()[i]->getOriginalInfo().alphaType();
+        }
+    }
+
+    // Recognize if there are no valid codecs
+    if (0 == maxWidth) {

[scroggo, 2015/03/20 19:36:00]

I would argue that if a codec reported its width or height as 0 (I think all the
subimages would have to, in this case), NewFromStream should have returned NULL
anyway (the embedded ones, keeping consistent with this one).

[msarett, 2015/03/23 12:20:57]

Agreed.  What I am actually checking is if there are no valid images to loop
over.  This check has been rewritten in a way that actually makes sense.

+        SkDebugf("Error: could not find any valid embedded ico codecs.\n");
+        return NULL;
+    }
+
+    const SkImageInfo info = SkImageInfo::Make(maxWidth, maxHeight,
+            maxColorType, maxAlphaType);
+
+    // Note that stream is owned by the embedded codec, the ico does not need
+    // direct access to the stream.
+    return SkNEW_ARGS(SkIcoCodec, (info, codecs.detach(), numValidImages));
+}
+
+/*
+ * Creates an instance of the decoder
+ * Called only by NewFromStream
+ */
+SkIcoCodec::SkIcoCodec(const SkImageInfo& info, SkCodec** embeddedCodecs,
+                       uint32_t numImages)
+    : INHERITED(info, NULL)
+    , fEmbeddedCodecs(embeddedCodecs)
+    , fNumImages(numImages)
+{}
+
+/*
+ * Initiates the Ico decode
+ */
+SkCodec::Result SkIcoCodec::onGetPixels(const SkImageInfo& dstInfo,
+                                        void* dst, size_t dstRowBytes,
+                                        const Options& opts, SkPMColor* ct,
+                                        int* ptr) {
+    // FIXME: How do we handle rewindIfNeeded?
+    // TODO: Should we also try to match color and alpha type?

[scroggo, 2015/03/20 19:36:01]

Good question. If we have multiple images with the same size but different color
types, we could check the result of getPixels, and if it's a color conversion
failure, try others.

[msarett, 2015/03/23 12:20:57]

Done.

+    // Note that we do not handle conversion possible because the embedded
+    // codec will make these checks.
+    for (uint32_t i = 0; i < fNumImages; i++) {
+        if (dstInfo.dimensions() ==
+                fEmbeddedCodecs.get()[i]->getOriginalInfo().dimensions()) {

[scroggo, 2015/03/20 19:36:00]

Is it possible that two embedded images have the same dimensions?

[msarett, 2015/03/23 12:20:57]

Yes and it's not that rare.  The new version will try again if an image with
matching dimensions fails to decode.

+            return fEmbeddedCodecs.get()[i]->getPixels(dstInfo, dst,
+                    dstRowBytes, &opts, ct, ptr);
+        }
+    }
+
+    SkDebugf("Error: No ico candidate image with matching dimensions.\n");

[scroggo, 2015/03/20 19:36:01]

Please implement onGetScaledDimensions, so a caller can find the dimensions to
use.

[msarett, 2015/03/23 12:20:57]

Done.

+    return kInvalidScale;
+}