Make SkAndroidCodec support ico

src/codec/SkCodec_libico.cpp

Index: src/codec/SkCodec_libico.cpp
diff --git a/src/codec/SkCodec_libico.cpp b/src/codec/SkCodec_libico.cpp
index 8c5a1b34120976e5d02c7fab1dad9c9185667771..4950ef9c49cc8344fdf42e2b12a44c0dab11337e 100644
--- a/src/codec/SkCodec_libico.cpp
+++ b/src/codec/SkCodec_libico.cpp
@@ -15,6 +15,43 @@
 #include "SkTDArray.h"
 #include "SkTSort.h"
 
+static bool ico_conversion_possible(const SkImageInfo& dstInfo) {
+    // We only support kN32_SkColorType.
+    // This makes sense for BMP-in-ICO.  The presence of an AND
+    // mask (which changes colors and adds transparency) means that
+    // we cannot use k565 or kIndex8.
+    // FIXME: For PNG-in-ICO, we could technically support whichever

[scroggo, 2015/12/02 16:27:03]

What is preventing us from supporting this now? I think this might need to first
figure out the size, and then based on the embedded codec to use, decide whether
this is supported. (Which might make it tricky for me to land my virtual
function version.)

[msarett, 2015/12/03 19:19:16]

Yes I think that's exactly what we need to do.  Along with minor changes to
NewFromStream() (to actually sometimes recommend opaque).

Can we keep this for a later change?

[scroggo, 2015/12/03 19:39:24]

Sounds fine to me (maybe include a bug?)

[msarett, 2015/12/03 19:54:15]

skbug.com/4620

+    //        color types that the png supports.
+    if (kN32_SkColorType != dstInfo.colorType()) {
+        return false;
+    }
+
+    // We only support transparent alpha types.  This is necessary for
+    // BMP-in-ICOs since there will be an AND mask.
+    // FIXME: For opaque PNG-in-ICOs, we should be able to support kOpaque.
+    return kPremul_SkAlphaType == dstInfo.alphaType() ||
+            kUnpremul_SkAlphaType == dstInfo.alphaType();
+}
+
+static SkImageInfo fix_embedded_alpha(const SkImageInfo& dstInfo, SkAlphaType embeddedAlpha) {
+    // FIXME (msarett): ICO is considered non-opaque, even if the embedded BMP
+    // incorrectly claims it has no alpha.
+    switch (embeddedAlpha) {
+        case kPremul_SkAlphaType:
+        case kUnpremul_SkAlphaType:
+            // Use the requested alpha type if the embedded codec supports alpha.
+            embeddedAlpha = dstInfo.alphaType();
+            break;
+        case kOpaque_SkAlphaType:
+            // If the embedded codec claims it is opaque, decode as if it is opaque.
+            break;
+        default:
+            SkASSERT(false);
+            break;
+    }
+    return dstInfo.makeAlphaType(embeddedAlpha);
+}
+
 /*
  * Checks the start of the stream to see if the image is an Ico or Cur
  */
@@ -197,6 +234,7 @@ SkIcoCodec::SkIcoCodec(const SkImageInfo& info,
                        SkTArray<SkAutoTDelete<SkCodec>, true>* codecs)
     : INHERITED(info, nullptr)
     , fEmbeddedCodecs(codecs)
+    , fCodec(nullptr)
 {}
 
 /*
@@ -226,15 +264,21 @@ SkISize SkIcoCodec::onGetScaledDimensions(float desiredScale) const {
     return fEmbeddedCodecs->operator[](minIndex)->getInfo().dimensions();
 }
 
-bool SkIcoCodec::onDimensionsSupported(const SkISize& dim) {
+int SkIcoCodec::chooseCodec(const SkISize& requestedSize, int startIndex) {
+    SkASSERT(startIndex >= 0);
+
     // FIXME: Cache the index from onGetScaledDimensions?
-    for (int32_t i = 0; i < fEmbeddedCodecs->count(); i++) {
-        if (fEmbeddedCodecs->operator[](i)->getInfo().dimensions() == dim) {
-            return true;
+    for (int i = startIndex; i < fEmbeddedCodecs->count(); i++) {
+        if (fEmbeddedCodecs->operator[](i)->getInfo().dimensions() == requestedSize) {
+            return i;
         }
     }
 
-    return false;
+    return -1;
+}
+
+bool SkIcoCodec::onDimensionsSupported(const SkISize& dim) {
+    return this->chooseCodec(dim, 0) >= 0;
 }
 
 /*
@@ -249,54 +293,88 @@ SkCodec::Result SkIcoCodec::onGetPixels(const SkImageInfo& dstInfo,
         return kUnimplemented;
     }
 
-    if (!valid_alpha(dstInfo.alphaType(), this->getInfo().alphaType())) {
+    if (!ico_conversion_possible(dstInfo)) {
         return kInvalidConversion;
     }
 
-    // We return invalid scale if there is no candidate image with matching
-    // dimensions.
-    Result result = kInvalidScale;
-    for (int32_t i = 0; i < fEmbeddedCodecs->count(); i++) {
-        SkCodec* embeddedCodec = fEmbeddedCodecs->operator[](i);
-        // If the dimensions match, try to decode
-        if (dstInfo.dimensions() == embeddedCodec->getInfo().dimensions()) {
-
-            // Perform the decode
-            // FIXME (msarett): ICO is considered non-opaque, even if the embedded BMP
-            // incorrectly claims it has no alpha.
-            SkAlphaType embeddedAlpha = embeddedCodec->getInfo().alphaType();
-            switch (embeddedAlpha) {
-                case kPremul_SkAlphaType:
-                case kUnpremul_SkAlphaType:
-                    // Use the requested alpha type if the embedded codec supports alpha.
-                    embeddedAlpha = dstInfo.alphaType();
-                    break;
-                case kOpaque_SkAlphaType:
-                    // If the embedded codec claims it is opaque, decode as if it is opaque.
-                    break;
-                default:
-                    SkASSERT(false);
-                    break;
-            }
-            SkImageInfo info = dstInfo.makeAlphaType(embeddedAlpha);
-            result = embeddedCodec->getPixels(info, dst, dstRowBytes, &opts, colorTable,
-                    colorCount);
-            // The embedded codec will handle filling incomplete images, so we will indicate
-            // that all of the rows are initialized.
-            *rowsDecoded = info.height();
-
-            // On a fatal error, keep trying to find an image to decode
-            if (kInvalidConversion == result || kInvalidInput == result ||
-                    kInvalidScale == result) {
-                SkCodecPrintf("Warning: Attempt to decode candidate ico failed.\n");
-                continue;
-            }
-
-            // On success or partial success, return the result
+    int index = 0;
+    SkCodec::Result result = kInvalidScale;
+    while (true) {
+        index = this->chooseCodec(dstInfo.dimensions(), index);
+        if (index < 0) {
+            break;
+        }
+
+        SkCodec* embeddedCodec = fEmbeddedCodecs->operator[](index);
+        SkImageInfo decodeInfo = fix_embedded_alpha(dstInfo, embeddedCodec->getInfo().alphaType());
+        SkASSERT(decodeInfo.colorType() == kN32_SkColorType);
+        result = embeddedCodec->getPixels(decodeInfo, dst, dstRowBytes, &opts, colorTable,
+                colorCount);
+
+        switch (result) {
+            case kSuccess:
+            case kIncompleteInput:
+                // The embedded codec will handle filling incomplete images, so we will indicate
+                // that all of the rows are initialized.
+                *rowsDecoded = decodeInfo.height();
+                return result;
+            default:
+                // Continue trying to find a valid embedded codec on a failed decode.
+                break;
+        }
+
+        index++;
+    }
+
+    SkCodecPrintf("Error: No matching candidate image in ico.\n");
+    return result;
+}
+
+SkCodec::Result SkIcoCodec::onStartScanlineDecode(const SkImageInfo& dstInfo,
+        const SkCodec::Options& options, SkPMColor colorTable[], int* colorCount) {
+    if (!ico_conversion_possible(dstInfo)) {
+        return kInvalidConversion;
+    }
+
+    int index = 0;
+    SkCodec::Result result = kInvalidScale;
+    while (true) {
+        index = this->chooseCodec(dstInfo.dimensions(), index);
+        if (index < 0) {
+            break;
+        }
+
+        SkCodec* embeddedCodec = fEmbeddedCodecs->operator[](index);
+        SkImageInfo decodeInfo = fix_embedded_alpha(dstInfo, embeddedCodec->getInfo().alphaType());
+        result = embeddedCodec->startScanlineDecode(decodeInfo, &options, colorTable, colorCount);
+        if (kSuccess == result) {
+            fCodec = embeddedCodec;
             return result;
         }
+
+        index++;
     }
 
     SkCodecPrintf("Error: No matching candidate image in ico.\n");
     return result;
 }
+
+int SkIcoCodec::onGetScanlines(void* dst, int count, size_t rowBytes) {
+    SkASSERT(fCodec);
+    return fCodec->getScanlines(dst, count, rowBytes);
+}
+
+bool SkIcoCodec::onSkipScanlines(int count) {
+    SkASSERT(fCodec);
+    return fCodec->skipScanlines(count);
+}
+
+SkCodec::SkScanlineOrder SkIcoCodec::onGetScanlineOrder() const {
+    // FIXME: This function will possibly return the wrong value if it is called

[scroggo, 2015/12/02 16:27:03]

I think it's more interesting than that. This function returns different values
depending on which embedded codec is being used. Does anyone need to call
getScanlineOrder outside of scanline decoding? Maybe we should explicitly state
that getScanlineOrder is undefined before startScanlineDecode is called?

[msarett, 2015/12/03 19:19:16]

Yes you're right.

The good news is that no one needs to call this outside of scanline decoding.

We can add comments to SkCodec to explicitly state that this is undefined before
startScanlineDecode is called.

I think this unfortunate because this is the only case where it is undefined. 
And we actually went to the trouble of adding more stuff to ReadHeader() in
SkGifCodec in order to avoid this problem (though there were other reasons too).
 However, I think this is the best option.

Alternatively, if fCurrScanlineCodec is null, we could call chooseCodec() on
this->getInfo()->dimensions() here and return the scanline order for whatever
the "default codec" is.  But that could still lead to confusion if the caller
expects the scanline order to stay the same after calling startScanlineDecode().

[scroggo, 2015/12/03 19:39:24]

This may be the only case, but as you state above, it's not needed when it would
be undefined.
Agreed. There is no reason to make an arbitrary guess in this situation.

+    //        before startScanlineDecode().
+    return fCodec ? fCodec->getScanlineOrder() : INHERITED::onGetScanlineOrder();
+}
+
+SkSampler* SkIcoCodec::getSampler(bool createIfNecessary) {
+    return fCodec ? fCodec->getSampler(createIfNecessary) : nullptr;
+}