Support more color types for ICOs

BUG=skia:4620
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1694513002

Review URL: https://codereview.chromium.org/1694513002

4 files changed, 24 insertions, 71 deletions

diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp
index 4828f7f7e2..cbba28c172 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp
@@ -476,6 +476,14 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
     }
 
     if (codecOut) {
+        // BMPs-in-ICOs contain an alpha mask after the image which means we
+        // cannot guarantee that an image is opaque, even if the bmp thinks
+        // it is.
+        bool isOpaque = kOpaque_SkAlphaType == alphaType;
+        if (inIco) {
+            alphaType = kUnpremul_SkAlphaType;
+        }
+
         // Set the image info
         const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
                 colorType, alphaType);
@@ -486,7 +494,7 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
                 // We require streams to have a memory base for Bmp-in-Ico decodes.
                 SkASSERT(!inIco || nullptr != stream->getMemoryBase());
                 *codecOut = new SkBmpStandardCodec(imageInfo, stream, bitsPerPixel, numColors,
-                        bytesPerColor, offset - bytesRead, rowOrder, inIco);
+                        bytesPerColor, offset - bytesRead, rowOrder, isOpaque, inIco);
                 return true;
             case kBitMask_BmpInputFormat:
                 // Bmp-in-Ico must be standard mode
diff --git a/src/codec/SkBmpStandardCodec.cpp b/src/codec/SkBmpStandardCodec.cpp
index 203e7da654..66a8c9a12a 100644
--- a/src/codec/SkBmpStandardCodec.cpp
+++ b/src/codec/SkBmpStandardCodec.cpp
@@ -17,7 +17,8 @@
 SkBmpStandardCodec::SkBmpStandardCodec(const SkImageInfo& info, SkStream* stream,
                                        uint16_t bitsPerPixel, uint32_t numColors,
                                        uint32_t bytesPerColor, uint32_t offset,
-                                       SkCodec::SkScanlineOrder rowOrder, bool inIco)
+                                       SkCodec::SkScanlineOrder rowOrder,
+                                       bool isOpaque, bool inIco)
     : INHERITED(info, stream, bitsPerPixel, rowOrder)
     , fColorTable(nullptr)
     , fNumColors(numColors)
@@ -26,6 +27,7 @@ SkBmpStandardCodec::SkBmpStandardCodec(const SkImageInfo& info, SkStream* stream
     , fSwizzler(nullptr)
     , fSrcRowBytes(SkAlign4(compute_row_bytes(this->getInfo().width(), this->bitsPerPixel())))
     , fSrcBuffer(new uint8_t [fSrcRowBytes])
+    , fIsOpaque(isOpaque)
     , fInIco(inIco)
     , fAndMaskRowBytes(fInIco ? SkAlign4(compute_row_bytes(this->getInfo().width(), 1)) : 0)
 {}
@@ -94,8 +96,7 @@ SkCodec::Result SkBmpStandardCodec::onGetPixels(const SkImageInfo& dstInfo,
 
         // Choose the proper packing function
         SkPMColor (*packARGB) (uint32_t, uint32_t, uint32_t, uint32_t);
-        SkAlphaType encodedAlphaType = this->getInfo().alphaType();
-        if (kOpaque_SkAlphaType == encodedAlphaType || kUnpremul_SkAlphaType == dstAlphaType) {
+        if (fIsOpaque || kUnpremul_SkAlphaType == dstAlphaType) {
             packARGB = &SkPackARGB32NoCheck;
         } else {
             packARGB = &SkPremultiplyARGBInline;
@@ -108,7 +109,7 @@ SkCodec::Result SkBmpStandardCodec::onGetPixels(const SkImageInfo& dstInfo,
             uint8_t green = get_byte(cBuffer.get(), i*fBytesPerColor + 1);
             uint8_t red = get_byte(cBuffer.get(), i*fBytesPerColor + 2);
             uint8_t alpha;
-            if (kOpaque_SkAlphaType == encodedAlphaType) {
+            if (fIsOpaque) {
                 alpha = 0xFF;
             } else {
                 alpha = get_byte(cBuffer.get(), i*fBytesPerColor + 3);
@@ -172,7 +173,7 @@ bool SkBmpStandardCodec::initializeSwizzler(const SkImageInfo& dstInfo, const Op
             config = SkSwizzler::kBGR;
             break;
         case 32:
-            if (kOpaque_SkAlphaType == this->getInfo().alphaType()) {
+            if (fIsOpaque) {
                 config = SkSwizzler::kBGRX;
             } else {
                 config = SkSwizzler::kBGRA;
diff --git a/src/codec/SkBmpStandardCodec.h b/src/codec/SkBmpStandardCodec.h
index b5f56f0a2a..725b7bb68f 100644
--- a/src/codec/SkBmpStandardCodec.h
+++ b/src/codec/SkBmpStandardCodec.h
@@ -27,18 +27,20 @@ public:
      * @param srcInfo contains the source width and height
      * @param stream the stream of encoded image data
      * @param bitsPerPixel the number of bits used to store each pixel
-     * @param format the format of the bmp file
      * @param numColors the number of colors in the color table
      * @param bytesPerColor the number of bytes in the stream used to represent
                             each color in the color table
      * @param offset the offset of the image pixel data from the end of the
      *               headers
      * @param rowOrder indicates whether rows are ordered top-down or bottom-up
+     * @param isOpaque indicates if the bmp itself is opaque (before applying
+     *                 the icp mask, if there is one)
+     * @param inIco    indicates if the bmp is embedded in an ico file
      */
     SkBmpStandardCodec(const SkImageInfo& srcInfo, SkStream* stream,
             uint16_t bitsPerPixel, uint32_t numColors, uint32_t bytesPerColor,
-            uint32_t offset, SkCodec::SkScanlineOrder rowOrder,
-            bool isIco);
+            uint32_t offset, SkCodec::SkScanlineOrder rowOrder, bool isOpaque,
+            bool inIco);
 
 protected:
 
@@ -90,6 +92,7 @@ private:
     SkAutoTDelete<SkSwizzler>           fSwizzler;
     const size_t                        fSrcRowBytes;
     SkAutoTDeleteArray<uint8_t>         fSrcBuffer;
+    const bool                          fIsOpaque;
     const bool                          fInIco;
     const size_t                        fAndMaskRowBytes; // only used for fInIco decodes
 
diff --git a/src/codec/SkIcoCodec.cpp b/src/codec/SkIcoCodec.cpp
index 62269237ae..dc4222a43c 100644
--- a/src/codec/SkIcoCodec.cpp
+++ b/src/codec/SkIcoCodec.cpp
@@ -15,43 +15,6 @@
 #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
-    //        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
  */
@@ -207,17 +170,6 @@ SkCodec* SkIcoCodec::NewFromStream(SkStream* stream) {
     }
     SkImageInfo info = codecs->operator[](maxIndex)->getInfo();
 
-    // ICOs contain an alpha mask after the image which means we cannot
-    // guarantee that an image is opaque, even if the sub-codec thinks it
-    // is.
-    // FIXME (msarett): The BMP decoder depends on the alpha type in order
-    // to decode correctly, otherwise it could report kUnpremul and we would
-    // not have to correct it here. Is there a better way?
-    // FIXME (msarett): This is only true for BMP in ICO - could a PNG in ICO
-    // be opaque? Is it okay that we missed out on the opportunity to mark
-    // such an image as opaque?
-    info = info.makeAlphaType(kUnpremul_SkAlphaType);
-
     // Note that stream is owned by the embedded codec, the ico does not need
     // direct access to the stream.
     return new SkIcoCodec(info, codecs.detach());
@@ -290,10 +242,6 @@ SkCodec::Result SkIcoCodec::onGetPixels(const SkImageInfo& dstInfo,
         return kUnimplemented;
     }
 
-    if (!ico_conversion_possible(dstInfo)) {
-        return kInvalidConversion;
-    }
-
     int index = 0;
     SkCodec::Result result = kInvalidScale;
     while (true) {
@@ -303,9 +251,7 @@ SkCodec::Result SkIcoCodec::onGetPixels(const SkImageInfo& dstInfo,
         }
 
         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,
+        result = embeddedCodec->getPixels(dstInfo, dst, dstRowBytes, &opts, colorTable,
                 colorCount);
 
         switch (result) {
@@ -313,7 +259,7 @@ SkCodec::Result SkIcoCodec::onGetPixels(const SkImageInfo& dstInfo,
             case kIncompleteInput:
                 // The embedded codec will handle filling incomplete images, so we will indicate
                 // that all of the rows are initialized.
-                *rowsDecoded = decodeInfo.height();
+                *rowsDecoded = dstInfo.height();
                 return result;
             default:
                 // Continue trying to find a valid embedded codec on a failed decode.
@@ -329,10 +275,6 @@ SkCodec::Result SkIcoCodec::onGetPixels(const SkImageInfo& dstInfo,
 
 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) {
@@ -342,8 +284,7 @@ SkCodec::Result SkIcoCodec::onStartScanlineDecode(const SkImageInfo& dstInfo,
         }
 
         SkCodec* embeddedCodec = fEmbeddedCodecs->operator[](index);
-        SkImageInfo decodeInfo = fix_embedded_alpha(dstInfo, embeddedCodec->getInfo().alphaType());
-        result = embeddedCodec->startScanlineDecode(decodeInfo, &options, colorTable, colorCount);
+        result = embeddedCodec->startScanlineDecode(dstInfo, &options, colorTable, colorCount);
         if (kSuccess == result) {
             fCurrScanlineCodec = embeddedCodec;
             return result;