Add SkEncodedInfo to report properties of encoded image data

src/codec/SkBmpCodec.cpp

Index: src/codec/SkBmpCodec.cpp
diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp
index ad6f0ddc4d5312b6e28e19ccb8b0655325c63bf9..564852b5dd0d7f1456dd8ab6bc2ad532e1ad0110 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp
@@ -415,32 +415,43 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
 
     switch (inputFormat) {
         case kStandard_BmpInputFormat: {
-            // BMPs-in-ICOs often contain an alpha mask after the image, which
-            // means we cannot guarantee that an image is opaque, even if the
-            // embedded bmp is opaque.
-            // We use |isOpaque| to indicate if the BMP itself is opaque, but
-            // still need to recommend kUnpremul when it is contained in an ICO.
-            SkColorType colorType = kN32_SkColorType;
-            SkAlphaType alphaType = inIco ? kUnpremul_SkAlphaType : kOpaque_SkAlphaType;
+            // BMPs are generally opaque, however BMPs-in-ICOs may contain
+            // a transparency mask after the image.  Therefore, we mark the
+            // alpha as kBinary if the BMP is contained in an ICO.
+            // We use |isOpaque| to indicate if the BMP itself is opaque.
+            SkEncodedInfo::Alpha alpha = inIco ? SkEncodedInfo::kBinary_Alpha :
+                    SkEncodedInfo::kOpaque_Alpha;
             bool isOpaque = true;
+
+            uint8_t bitsPerComponent = SkTMin((uint16_t) 8, bitsPerPixel);

[scroggo, 2016/03/23 14:48:50]

It seems odd that you set bitsPerComponent to min(bitsPerPixel, 8). Might be
worth a comment? (e.g. <= 8 are palette; >8 are 8 bitsPerComponent)

[msarett, 2016/03/24 16:20:43]

Agreed.  I think maybe it's clearer to handle it in the switch statement.

+            SkEncodedInfo::Color color;
             switch (bitsPerPixel) {
                 // Palette formats
                 case 1:
                 case 2:
                 case 4:
                 case 8:
-                    // We cannot recommend a palette color type for ICOs because they
-                    // may contain a transparency mask.
-                    if (!inIco) {
-                        colorType = kIndex_8_SkColorType;
+                    // In the case of ICO, kBGRA is actually the closest match,
+                    // since we will need to apply a transparency mask.
+                    if (inIco) {
+                        color = SkEncodedInfo::kBGRA_Color;
+                        bitsPerComponent = 8;
+                    } else {
+                        color = SkEncodedInfo::kPalette_Color;
                     }
                     break;
                 case 24:
+                    color = SkEncodedInfo::kBGR_Color;
+                    break;
                 case 32:
                     // 32-bit BMP-in-ICOs actually use the alpha channel in place of a
                     // transparency mask.
                     if (inIco) {
                         isOpaque = false;
+                        alpha = SkEncodedInfo::kUnpremul_Alpha;
+                        color = SkEncodedInfo::kBGRA_Color;
+                    } else {
+                        color = SkEncodedInfo::kBGRX_Color;
                     }
                     break;
                 default:
@@ -453,9 +464,9 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
                 SkASSERT(!inIco || nullptr != stream->getMemoryBase());
 
                 // Set the image info and create a codec.
-                const SkImageInfo imageInfo = SkImageInfo::Make(width, height, colorType,
-                        alphaType);
-                *codecOut = new SkBmpStandardCodec(imageInfo, stream, bitsPerPixel, numColors,
+                const SkEncodedInfo info = SkEncodedInfo::Make(width, height, color, alpha,
+                        bitsPerComponent);
+                *codecOut = new SkBmpStandardCodec(info, stream, bitsPerPixel, numColors,
                         bytesPerColor, offset - bytesRead, rowOrder, isOpaque, inIco);
 
             }
@@ -495,12 +506,21 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
                     return false;
                 }
 
-                // Set the image info
-                SkAlphaType alphaType = masks->getAlphaMask() ? kUnpremul_SkAlphaType :
-                        kOpaque_SkAlphaType;
-                const SkImageInfo imageInfo = SkImageInfo::Make(width, height, kN32_SkColorType,
-                        alphaType);
-                *codecOut = new SkBmpMaskCodec(imageInfo, stream, bitsPerPixel, masks.release(),
+                // Masked bmps are not a great fit for SkEncodedInfo, since they have

[scroggo, 2016/03/23 14:48:50]

Should SkEncodedInfo adapt to support masked bmps? I thought that bmp had some
cases that weren't covered, but I couldn't remember them. I guess that whatever
info would be specific to mask bmp would not make a client decide to treat it
differently than if we just choose types that are already there?

[msarett, 2016/03/24 16:20:44]

I thought about 3 options:
(1) Create all of the modes we might see in masked bmps...

kGRB, kGBR, kRBG, kBRG (also other variations including A) with values of
bitsPerComponent for each component (since they don't have to be the same).

I think this is pretty clearly not worth it...it's a lot of extra complexity in
our struct without knowing if there are any clients who would use this info or
care.

Additionally, we would have to interpret the masks to figure all of this out.

(2) Add a kMask_Color to SkEncodedInfo

I almost did this...  I ended up not doing it because it's not a great fit with
the others.  Ex: we can't tell the number of components from the name, we still
have a problem with how to set bits per component.

Still, this might be worth it.  The client could recognize that this is a
"weird" bmp.  It could be useful to report the max/min bits per component.

(3) Include mask bmps with kBGR/kBGRA and handle them differently without
telling the client.

This seems easiest right now.  Especially because we don't really know what the
client wants or if they want anything.  I don't mind having bmp being a weird
special case.  We can always change this decision later.

[scroggo, 2016/03/24 20:51:03]

I'm also fine with Mask BMP being a weird special case.

+                // arbitrary component orderings and bits per component.  Here we choose
+                // somewhat reasonable values - it's ok that we don't match exactly
+                // because SkBmpMaskCodec has its own mask swizzler anyway.
+                SkEncodedInfo::Color color;
+                SkEncodedInfo::Alpha alpha;
+                if (masks->getAlphaMask()) {
+                    color = SkEncodedInfo::kBGRA_Color;
+                    alpha = SkEncodedInfo::kUnpremul_Alpha;
+                } else {
+                    color = SkEncodedInfo::kBGR_Color;
+                    alpha = SkEncodedInfo::kOpaque_Alpha;
+                }
+                const SkEncodedInfo info = SkEncodedInfo::Make(width, height, color, alpha, 8);
+                *codecOut = new SkBmpMaskCodec(info, stream, bitsPerPixel, masks.release(),
                         rowOrder);
             }
             return true;
@@ -526,10 +546,11 @@ bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
             if (codecOut) {
                 // RLE inputs may skip pixels, leaving them as transparent.  This
                 // is uncommon, but we cannot be certain that an RLE bmp will be
-                // opaque.
-                const SkImageInfo imageInfo = SkImageInfo::Make(width, height, kN32_SkColorType,
-                        kUnpremul_SkAlphaType);
-                *codecOut = new SkBmpRLECodec(imageInfo, stream, bitsPerPixel, numColors,
+                // opaque or that we will be able to represent it with a palette.
+                // For that reason, we always indicate that we are kBGRA.
+                const SkEncodedInfo info = SkEncodedInfo::Make(width, height,
+                        SkEncodedInfo::kBGRA_Color, SkEncodedInfo::kBinary_Alpha, 8);
+                *codecOut = new SkBmpRLECodec(info, stream, bitsPerPixel, numColors,
                         bytesPerColor, offset - bytesRead, rowOrder, RLEBytes);
             }
             return true;
@@ -557,7 +578,7 @@ SkCodec* SkBmpCodec::NewFromStream(SkStream* stream, bool inIco) {
     return nullptr;
 }
 
-SkBmpCodec::SkBmpCodec(const SkImageInfo& info, SkStream* stream,
+SkBmpCodec::SkBmpCodec(const SkEncodedInfo& info, SkStream* stream,
         uint16_t bitsPerPixel, SkCodec::SkScanlineOrder rowOrder)
     : INHERITED(info, stream)
     , fBitsPerPixel(bitsPerPixel)