Bug 1214072 (part 2) - Implement transparency properly for BMP images. r=seth.
authorNicholas Nethercote <nnethercote@mozilla.com>
Tue, 13 Oct 2015 21:20:10 -0700
changeset 269699 0bdbbc7673c5c6f8a6223424e3ac386d2d1675bb
parent 269698 e73b37f923c1c90bd0d2d9037d604537a9cdd495
child 269700 f1f6a7ae585eb6ed1eb6caebdf4e491fbfa87fd4
push id29588
push userkwierso@gmail.com
push dateTue, 27 Oct 2015 19:18:01 +0000
treeherdermozilla-central@5430b2dba98b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersseth
bugs1214072
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1214072 (part 2) - Implement transparency properly for BMP images. r=seth. Currently we don't implement transparency at all in BMP images except for an odd-duck case of BMPs within ICOs. This patch does the following. - It implements transparency properly for 16bpp and 32bpp images via bitfield masking. (For 32bpp images this also requires handling colors via bitfield masking.) The patch maintains the existing BMP-within-ICO transparency handling. - It also reworks BitFields::Value::Set(). * It now works correctly if the run of 1s goes all the way to bit 31 (the old code didn't set mBitWidth). * If the mask is 0, will give an mRightShift of 0 (old code gave 32, and right-shifting by 32 is dodgy). * It's now easier to read. - It renames transparent.bmp as transparent-if-within-ico.bmp. Ironically enough this file currently uses BITFIELDS compression and is WinBMPv5 format, which means it contains well-specified alpha data. In order to use it to test the hacky BMP-within-ICO transparency scheme the patch changes it to be WinBMPv3 format with RGB compression (i.e. no compression). I left the now-excess bytes (including the bitfields) in the info header in place because that's allowed -- thanks to the start of the pixel data being specified by the |dataoffset| field -- they'll just be ignored. - It tweaks the naming of the relevant gtests and some of their finer details to work with the new way of doing things. This fixes all four remaining failures in bmpsuite.
image/decoders/nsBMPDecoder.cpp
image/decoders/nsBMPDecoder.h
image/decoders/nsICODecoder.cpp
image/test/gtest/Common.cpp
image/test/gtest/TestMetadata.cpp
image/test/gtest/moz.build
image/test/gtest/transparent-if-within-ico.bmp
image/test/gtest/transparent.bmp
image/test/reftest/bmp/bmpsuite/g/reftest.list
image/test/reftest/bmp/bmpsuite/q/reftest.list
--- a/image/decoders/nsBMPDecoder.cpp
+++ b/image/decoders/nsBMPDecoder.cpp
@@ -55,16 +55,22 @@
 //   mask data from WinBMPv3-NT, plus alpha mask data, and also color-space and
 //   gamma correction fields.
 //
 // WinBMPv5.
 // - It extended the info header to 124 bytes, adding color profile data.
 // - It also added an optional color profile table after the pixel data (and
 //   another optional gap).
 //
+// WinBMPv3-ICO. This is a variant of WinBMPv3.
+// - It's the BMP format used for BMP images within ICO files.
+// - The only difference with WinBMPv3 is that if an image is 32bpp and has no
+//   compression, then instead of treating the pixel data as 0RGB it is treated
+//   as ARGB, but only if one or more of the A values are non-zero.
+//
 // OS/2 VERSIONS OF THE BMP FORMAT
 // -------------------------------
 // OS2-BMPv1.
 // - Almost identical to WinBMPv2; the differences are basically ignorable.
 //
 // OS2-BMPv2.
 // - Similar to WinBMPv3.
 // - The info header is 64 bytes but can be reduced to as little as 16; any
@@ -163,43 +169,36 @@ GetBMPLog()
     sBMPLog = PR_NewLogModule("BMPDecoder");
   }
   return sBMPLog;
 }
 
 nsBMPDecoder::nsBMPDecoder(RasterImage* aImage)
   : Decoder(aImage)
   , mLexer(Transition::To(State::FILE_HEADER, FileHeader::LENGTH))
+  , mIsWithinICO(false)
   , mMayHaveTransparency(false)
+  , mDoesHaveTransparency(false)
   , mNumColors(0)
   , mColors(nullptr)
   , mBytesPerColor(0)
   , mPreGapLength(0)
   , mCurrentRow(0)
   , mCurrentPos(0)
   , mAbsoluteModeNumPixels(0)
-  , mUseAlphaData(false)
-  , mHaveAlphaData(false)
 {
   memset(&mBFH, 0, sizeof(mBFH));
   memset(&mBIH, 0, sizeof(mBIH));
 }
 
 nsBMPDecoder::~nsBMPDecoder()
 {
   delete[] mColors;
 }
 
-// Sets whether or not the BMP will use alpha data.
-void
-nsBMPDecoder::SetUseAlphaData(bool useAlphaData)
-{
-  mUseAlphaData = useAlphaData;
-}
-
 // Obtains the bits per pixel from the internal BIH header.
 int32_t
 nsBMPDecoder::GetBitsPerPixel() const
 {
   return mBIH.bpp;
 }
 
 // Obtains the width from the internal BIH header.
@@ -246,17 +245,18 @@ nsBMPDecoder::FinishInternal()
 
   // Send notifications if appropriate.
   if (!IsMetadataDecode() && HasSize()) {
 
     // Invalidate.
     nsIntRect r(0, 0, mBIH.width, GetHeight());
     PostInvalidation(r);
 
-    if (mUseAlphaData && mHaveAlphaData) {
+    if (mDoesHaveTransparency) {
+      MOZ_ASSERT(mMayHaveTransparency);
       PostFrameStop(Opacity::SOME_TRANSPARENCY);
     } else {
       PostFrameStop(Opacity::OPAQUE);
     }
     PostDecodeDone();
   }
 }
 
@@ -264,31 +264,49 @@ nsBMPDecoder::FinishInternal()
 // Actual Data Processing
 // ----------------------------------------
 
 void
 BitFields::Value::Set(uint32_t aMask)
 {
   mMask = aMask;
 
+  // Handle this exceptional case first. The chosen values don't matter
+  // (because a mask of zero will always give a value of zero) except that
+  // mBitWidth:
+  // - shouldn't be zero, because that would cause an infinite loop in Get();
+  // - shouldn't be 5 or 8, because that could cause a false positive match in
+  //   IsR5G5B5() or IsR8G8B8().
+  if (mMask == 0x0) {
+    mRightShift = 0;
+    mBitWidth = 1;
+    return;
+  }
+
   // Find the rightmost 1.
-  bool started = false;
-  mRightShift = mBitWidth = 0;
-  for (uint8_t pos = 0; pos <= 31; pos++) {
-    if (!started && (aMask & (1 << pos))) {
-      mRightShift = pos;
-      started = true;
-    } else if (started && !(aMask & (1 << pos))) {
-      mBitWidth = pos - mRightShift;
+  uint8_t i;
+  for (i = 0; i < 32; i++) {
+    if (mMask & (1 << i)) {
       break;
     }
   }
+  mRightShift = i;
+
+  // Now find the leftmost 1 in the same run of 1s. (If there are multiple runs
+  // of 1s -- which isn't valid -- we'll behave as if only the lowest run was
+  // present, which seems reasonable.)
+  for (i = i + 1; i < 32; i++) {
+    if (!(mMask & (1 << i))) {
+      break;
+    }
+  }
+  mBitWidth = i - mRightShift;
 }
 
-inline uint8_t
+MOZ_ALWAYS_INLINE uint8_t
 BitFields::Value::Get(uint32_t aValue) const
 {
   // Extract the unscaled value.
   uint32_t v = (aValue & mMask) >> mRightShift;
 
   // Idea: to upscale v precisely we need to duplicate its bits, possibly
   // repeatedly, possibly partially in the last case, from bit 7 down to bit 0
   // in v2. For example:
@@ -311,37 +329,73 @@ BitFields::Value::Get(uint32_t aValue) c
   for (i = 8 - mBitWidth; i > 0; i -= mBitWidth) {
     v2 |= v << uint32_t(i);
   }
   v2 |= v >> uint32_t(-i);
   return v2;
 }
 
 MOZ_ALWAYS_INLINE uint8_t
+BitFields::Value::GetAlpha(uint32_t aValue, bool& aHasAlphaOut) const
+{
+  if (mMask == 0x0) {
+    return 0xff;
+  }
+  aHasAlphaOut = true;
+  return Get(aValue);
+}
+
+MOZ_ALWAYS_INLINE uint8_t
 BitFields::Value::Get5(uint32_t aValue) const
 {
   MOZ_ASSERT(mBitWidth == 5);
   uint32_t v = (aValue & mMask) >> mRightShift;
   return (v << 3u) | (v >> 2u);
 }
 
+MOZ_ALWAYS_INLINE uint8_t
+BitFields::Value::Get8(uint32_t aValue) const
+{
+  MOZ_ASSERT(mBitWidth == 8);
+  uint32_t v = (aValue & mMask) >> mRightShift;
+  return v;
+}
+
 void
 BitFields::SetR5G5B5()
 {
   mRed.Set(0x7c00);
   mGreen.Set(0x03e0);
   mBlue.Set(0x001f);
 }
 
+void
+BitFields::SetR8G8B8()
+{
+  mRed.Set(0xff0000);
+  mGreen.Set(0xff00);
+  mBlue.Set(0x00ff);
+}
+
 bool
 BitFields::IsR5G5B5() const
 {
   return mRed.mBitWidth == 5 &&
          mGreen.mBitWidth == 5 &&
-         mBlue.mBitWidth == 5;
+         mBlue.mBitWidth == 5 &&
+         mAlpha.mMask == 0x0;
+}
+
+bool
+BitFields::IsR8G8B8() const
+{
+  return mRed.mBitWidth == 8 &&
+         mGreen.mBitWidth == 8 &&
+         mBlue.mBitWidth == 8 &&
+         mAlpha.mMask == 0x0;
 }
 
 uint32_t*
 nsBMPDecoder::RowBuffer()
 {
   if (mDownscaler) {
     return reinterpret_cast<uint32_t*>(mDownscaler->RowBuffer()) + mCurrentPos;
   }
@@ -454,16 +508,19 @@ nsBMPDecoder::ReadInfoHeaderSize(const c
                    mBIH.bihsize == InfoHeaderLength::WIN_V4 ||
                    mBIH.bihsize == InfoHeaderLength::WIN_V5 ||
                    (mBIH.bihsize >= InfoHeaderLength::OS2_V2_MIN &&
                     mBIH.bihsize <= InfoHeaderLength::OS2_V2_MAX);
   if (!bihsizeOk) {
     PostDataError();
     return Transition::Terminate(State::FAILURE);
   }
+  // ICO BMPs must have a WinVMPv3 header. nsICODecoder should have already
+  // terminated decoding if this isn't the case.
+  MOZ_ASSERT_IF(mIsWithinICO, mBIH.bihsize == InfoHeaderLength::WIN_V3);
 
   return Transition::To(State::INFO_HEADER_REST,
                         mBIH.bihsize - BIHSIZE_FIELD_LENGTH);
 }
 
 LexerTransition<nsBMPDecoder::State>
 nsBMPDecoder::ReadInfoHeaderRest(const char* aData, size_t aLength)
 {
@@ -537,63 +594,71 @@ nsBMPDecoder::ReadInfoHeaderRest(const c
   // Compute this even for a metadate decode because GetCompressedImageSize()
   // relies on it.
   mPixelRowSize = (mBIH.bpp * mBIH.width + 7) / 8;
   uint32_t surplus = mPixelRowSize % 4;
   if (surplus != 0) {
     mPixelRowSize += 4 - surplus;
   }
 
-  // We treat BMPs as transparent if they're 32bpp and alpha is enabled, but
-  // also if they use RLE encoding, because the 'delta' mode can skip pixels
-  // and cause implicit transparency.
-  mMayHaveTransparency = (mBIH.compression == Compression::RLE8) ||
-                         (mBIH.compression == Compression::RLE4) ||
-                         (mBIH.bpp == 32 && mUseAlphaData);
-  if (mMayHaveTransparency) {
-    PostHasTransparency();
-  }
-
   size_t bitFieldsLengthStillToRead = 0;
   if (mBIH.compression == Compression::BITFIELDS) {
     // Need to read bitfields.
     if (mBIH.bihsize >= InfoHeaderLength::WIN_V4) {
       // Bitfields are present in the info header, so we can read them
       // immediately.
-      mBitFields.ReadFromHeader(aData + 36);
+      mBitFields.ReadFromHeader(aData + 36, /* aReadAlpha = */ true);
     } else {
       // Bitfields are present after the info header, so we will read them in
       // ReadBitfields().
       bitFieldsLengthStillToRead = BitFields::LENGTH;
     }
   } else if (mBIH.bpp == 16) {
     // No bitfields specified; use the default 5-5-5 values.
     mBitFields.SetR5G5B5();
+  } else if (mBIH.bpp == 32) {
+    // No bitfields specified; use the default 8-8-8 values.
+    mBitFields.SetR8G8B8();
   }
 
   return Transition::To(State::BITFIELDS, bitFieldsLengthStillToRead);
 }
 
 void
-BitFields::ReadFromHeader(const char* aData)
+BitFields::ReadFromHeader(const char* aData, bool aReadAlpha)
 {
   mRed.Set  (LittleEndian::readUint32(aData + 0));
   mGreen.Set(LittleEndian::readUint32(aData + 4));
   mBlue.Set (LittleEndian::readUint32(aData + 8));
+  if (aReadAlpha) {
+    mAlpha.Set(LittleEndian::readUint32(aData + 12));
+  }
 }
 
 LexerTransition<nsBMPDecoder::State>
 nsBMPDecoder::ReadBitfields(const char* aData, size_t aLength)
 {
   mPreGapLength += aLength;
 
   // If aLength is zero there are no bitfields to read, or we already read them
   // in ReadInfoHeader().
   if (aLength != 0) {
-    mBitFields.ReadFromHeader(aData);
+    mBitFields.ReadFromHeader(aData, /* aReadAlpha = */ false);
+  }
+
+  // Note that RLE-encoded BMPs might be transparent because the 'delta' mode
+  // can skip pixels and cause implicit transparency.
+  mMayHaveTransparency =
+    (mBIH.compression == Compression::RGB && mIsWithinICO && mBIH.bpp == 32) ||
+    mBIH.compression == Compression::RLE8 ||
+    mBIH.compression == Compression::RLE4 ||
+    (mBIH.compression == Compression::BITFIELDS &&
+     mBitFields.mAlpha.IsPresent());
+  if (mMayHaveTransparency) {
+    PostHasTransparency();
   }
 
   // We've now read all the headers. If we're doing a metadata decode, we're
   // done.
   if (IsMetadataDecode()) {
     return Transition::Terminate(State::SUCCESS);
   }
 
@@ -721,48 +786,89 @@ nsBMPDecoder::ReadPixelRow(const char* a
           uint16_t val = LittleEndian::readUint16(src);
           SetPixel(dst, mBitFields.mRed.Get5(val),
                         mBitFields.mGreen.Get5(val),
                         mBitFields.mBlue.Get5(val));
           --lpos;
           src += 2;
         }
       } else {
+        bool anyHasAlpha = false;
         while (lpos > 0) {
           uint16_t val = LittleEndian::readUint16(src);
           SetPixel(dst, mBitFields.mRed.Get(val),
                         mBitFields.mGreen.Get(val),
-                        mBitFields.mBlue.Get(val));
+                        mBitFields.mBlue.Get(val),
+                        mBitFields.mAlpha.GetAlpha(val, anyHasAlpha));
           --lpos;
           src += 2;
         }
+        if (anyHasAlpha) {
+          MOZ_ASSERT(mMayHaveTransparency);
+          mDoesHaveTransparency = true;
+        }
       }
       break;
 
     case 24:
       while (lpos > 0) {
         SetPixel(dst, src[2], src[1], src[0]);
         --lpos;
         src += 3;
       }
       break;
 
     case 32:
-      while (lpos > 0) {
-        if (mUseAlphaData) {
-          if (MOZ_UNLIKELY(!mHaveAlphaData && src[3])) {
-            PostHasTransparency();
-            mHaveAlphaData = true;
+      if (mBIH.compression == Compression::RGB && mIsWithinICO &&
+          mBIH.bpp == 32) {
+        // This is a special case only used for 32bpp WinBMPv3-ICO files, which
+        // could be in either 0RGB or ARGB format.
+        while (lpos > 0) {
+          // If src[3] is zero, we can't tell at this point if the image is
+          // 0RGB or ARGB. So we just use 0 value as-is. If the image is 0RGB
+          // then mDoesHaveTransparency will be false at the end, we'll treat
+          // the image as opaque, and the 0 alpha values will be ignored. If
+          // the image is ARGB then mDoesHaveTransparency will be true at the
+          // end and we'll treat the image as non-opaque. (Note: a
+          // fully-transparent ARGB image is indistinguishable from a 0RGB
+          // image, and we will render such an image as a 0RGB image, i.e.
+          // opaquely. This is unlikely to be a problem in practice.)
+          if (src[3] != 0) {
+            MOZ_ASSERT(mMayHaveTransparency);
+            mDoesHaveTransparency = true;
           }
           SetPixel(dst, src[2], src[1], src[0], src[3]);
-        } else {
-          SetPixel(dst, src[2], src[1], src[0]);
+          src += 4;
+          --lpos;
+        }
+      } else if (mBitFields.IsR8G8B8()) {
+        // Specialize this common case.
+        while (lpos > 0) {
+          uint32_t val = LittleEndian::readUint32(src);
+          SetPixel(dst, mBitFields.mRed.Get8(val),
+                        mBitFields.mGreen.Get8(val),
+                        mBitFields.mBlue.Get8(val));
+          --lpos;
+          src += 4;
         }
-        --lpos;
-        src += 4;
+      } else {
+        bool anyHasAlpha = false;
+        while (lpos > 0) {
+          uint32_t val = LittleEndian::readUint32(src);
+          SetPixel(dst, mBitFields.mRed.Get(val),
+                        mBitFields.mGreen.Get(val),
+                        mBitFields.mBlue.Get(val),
+                        mBitFields.mAlpha.GetAlpha(val, anyHasAlpha));
+          --lpos;
+          src += 4;
+        }
+        if (anyHasAlpha) {
+          MOZ_ASSERT(mMayHaveTransparency);
+          mDoesHaveTransparency = true;
+        }
       }
       break;
 
     default:
       MOZ_CRASH("Unsupported color depth; earlier check didn't catch it?");
   }
 
   FinishRow();
@@ -835,23 +941,20 @@ nsBMPDecoder::ReadRLESegment(const char*
     length++;
   }
   return Transition::To(State::RLE_ABSOLUTE, length);
 }
 
 LexerTransition<nsBMPDecoder::State>
 nsBMPDecoder::ReadRLEDelta(const char* aData)
 {
-  // Delta encoding makes it possible to skip pixels making the image
+  // Delta encoding makes it possible to skip pixels making part of the image
   // transparent.
-  if (MOZ_UNLIKELY(!mHaveAlphaData)) {
-    PostHasTransparency();
-    mHaveAlphaData = true;
-  }
-  mUseAlphaData = mHaveAlphaData = true;
+  MOZ_ASSERT(mMayHaveTransparency);
+  mDoesHaveTransparency = true;
 
   if (mDownscaler) {
     // Clear the skipped pixels. (This clears to the end of the row,
     // which is perfect if there's a Y delta and harmless if not).
     mDownscaler->ClearRow(/* aStartingAtCol = */ mCurrentPos);
   }
 
   // Handle the XDelta.
--- a/image/decoders/nsBMPDecoder.h
+++ b/image/decoders/nsBMPDecoder.h
@@ -33,81 +33,105 @@ class BitFields {
     uint32_t mMask;       // The mask for the value.
     uint8_t mRightShift;  // The amount to right-shift after masking.
     uint8_t mBitWidth;    // The width (in bits) of the value.
 
     /// Sets the mask (and thus the right-shift and bit-width as well).
     void Set(uint32_t aMask);
 
   public:
+    Value()
+    {
+      mMask = 0;
+      mRightShift = 0;
+      mBitWidth = 0;
+    }
+
+    /// Returns true if this channel is used. Only used for alpha.
+    bool IsPresent() const { return mMask != 0x0; }
+
     /// Extracts the single color value from the multi-color value.
     uint8_t Get(uint32_t aVal) const;
 
-    /// Specialized version of Get() for the case where the bit-width is 5.
-    /// (It will assert if called and the bit-width is not 5.)
+    /// Like Get(), but specially for alpha.
+    uint8_t GetAlpha(uint32_t aVal, bool& aHasAlphaOut) const;
+
+    /// Specialized versions of Get() for when the bit-width is 5 or 8.
+    /// (They will assert if called and the bit-width is not 5 or 8.)
     uint8_t Get5(uint32_t aVal) const;
+    uint8_t Get8(uint32_t aVal) const;
   };
 
 public:
   /// The individual color channels.
   Value mRed;
   Value mGreen;
   Value mBlue;
+  Value mAlpha;
 
   /// Set bitfields to the standard 5-5-5 16bpp values.
   void SetR5G5B5();
 
+  /// Set bitfields to the standard 8-8-8 32bpp values.
+  void SetR8G8B8();
+
   /// Test if bitfields have the standard 5-5-5 16bpp values.
   bool IsR5G5B5() const;
 
-  /// Read the bitfields from a header.
-  void ReadFromHeader(const char* aData);
+  /// Test if bitfields have the standard 8-8-8 32bpp values.
+  bool IsR8G8B8() const;
+
+  /// Read the bitfields from a header. The reading of the alpha mask is
+  /// optional.
+  void ReadFromHeader(const char* aData, bool aReadAlpha);
 
   /// Length of the bitfields structure in the BMP file.
   static const size_t LENGTH = 12;
 };
 
 } // namespace bmp
 
 class RasterImage;
 
 /// Decoder for BMP-Files, as used by Windows and OS/2.
 
 class nsBMPDecoder : public Decoder
 {
 public:
   ~nsBMPDecoder();
 
-  /// Specifies whether or not the BMP file will contain alpha data. If set to
-  /// true and the BMP is 32BPP, the alpha data will be retrieved from the 4th
-  /// byte of image data per pixel.
-  void SetUseAlphaData(bool useAlphaData);
-
   /// Obtains the bits per pixel from the internal BIH header.
   int32_t GetBitsPerPixel() const;
 
   /// Obtains the width from the internal BIH header.
   int32_t GetWidth() const;
 
   /// Obtains the abs-value of the height from the internal BIH header.
   int32_t GetHeight() const;
 
   /// Obtains the internal output image buffer.
   uint32_t* GetImageData();
   size_t GetImageDataLength() const { return mImageDataLength; }
 
   /// Obtains the size of the compressed image resource.
   int32_t GetCompressedImageSize() const;
 
-  /// Obtains whether or not a BMP file had alpha data in its 4th byte for 32BPP
-  /// bitmaps. Use only after the bitmap has been processed.
-  bool HasAlphaData() const { return mHaveAlphaData; }
+  /// Mark this BMP as being within an ICO file.
+  void SetIsWithinICO() { mIsWithinICO = true; }
+
+  /// Did the BMP file have alpha data of any kind? (Only use this after the
+  /// bitmap has been fully decoded.)
+  bool HasTransparency() const { return mDoesHaveTransparency; }
 
-  /// Marks this BMP as having alpha data (due to e.g. an ICO alpha mask).
-  void SetHasAlphaData() { mHaveAlphaData = true; }
+  /// Force transparency from outside. (Used by the ICO decoder.)
+  void SetHasTransparency()
+  {
+    mMayHaveTransparency = true;
+    mDoesHaveTransparency = true;
+  }
 
   virtual void WriteInternal(const char* aBuffer,
                              uint32_t aCount) override;
   virtual void FinishInternal() override;
 
 private:
   friend class DecoderFactory;
   friend class nsICODecoder;
@@ -146,22 +170,29 @@ private:
   LexerTransition<State> ReadRLEDelta(const char* aData);
   LexerTransition<State> ReadRLEAbsolute(const char* aData, size_t aLength);
 
   StreamingLexer<State> mLexer;
 
   bmp::FileHeader mBFH;
   bmp::V5InfoHeader mBIH;
 
+  // If the BMP is within an ICO file our treatment of it differs slightly.
+  bool mIsWithinICO;
+
   bmp::BitFields mBitFields;
 
   // Might the image have transparency? Determined from the headers during
   // metadata decode. (Does not guarantee the image actually has transparency.)
   bool mMayHaveTransparency;
 
+  // Does the image have transparency? Determined during full decoding, so only
+  // use this after that has been completed.
+  bool mDoesHaveTransparency;
+
   uint32_t mNumColors;      // The number of used colors, i.e. the number of
                             // entries in mColors, if it's present.
   bmp::ColorTableEntry* mColors; // The color table, if it's present.
   uint32_t mBytesPerColor;  // 3 or 4, depending on the format
 
   // The number of bytes prior to the optional gap that have been read. This
   // is used to find the start of the pixel data.
   uint32_t mPreGapLength;
@@ -170,26 +201,14 @@ private:
 
   int32_t mCurrentRow;      // Index of the row of the image that's currently
                             // being decoded: [height,1].
   int32_t mCurrentPos;      // Index into the current line; only used when
                             // doing RLE decoding.
 
   // Only used in RLE_ABSOLUTE state: the number of pixels to read.
   uint32_t mAbsoluteModeNumPixels;
-
-  // Stores whether the image data may store alpha data, or if the alpha data
-  // is unspecified and filled with a padding byte of 0. When a 32BPP bitmap
-  // is stored in an ICO or CUR file, its 4th byte is used for alpha
-  // transparency.  When it is stored in a BMP, its 4th byte is reserved and
-  // is always 0. Reference:
-  //   http://en.wikipedia.org/wiki/ICO_(file_format)#cite_note-9
-  // Bitmaps where the alpha bytes are all 0 should be fully visible.
-  bool mUseAlphaData;
-
-  // Whether the 4th byte alpha data was found to be non zero and hence used.
-  bool mHaveAlphaData;
 };
 
 } // namespace image
 } // namespace mozilla
 
 #endif // mozilla_image_decoders_nsBMPDecoder_h
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -388,17 +388,17 @@ nsICODecoder::SniffResource(const char* 
     return Transition::ToUnbuffered(ICOState::FINISHED_RESOURCE,
                                     ICOState::READ_PNG,
                                     toRead);
   } else {
     // Create a BMP decoder which will do most of the work for us; the exception
     // is the AND mask, which isn't present in standalone BMPs.
     nsBMPDecoder* bmpDecoder = new nsBMPDecoder(mImage);
     mContainedDecoder = bmpDecoder;
-    bmpDecoder->SetUseAlphaData(true);
+    bmpDecoder->SetIsWithinICO();
     mContainedDecoder->SetMetadataDecode(IsMetadataDecode());
     mContainedDecoder->SetDecoderFlags(GetDecoderFlags());
     mContainedDecoder->SetSurfaceFlags(GetSurfaceFlags());
     if (mDownscaler) {
       mContainedDecoder->SetTargetSize(mDownscaler->TargetSize());
     }
     mContainedDecoder->Init();
 
@@ -520,19 +520,19 @@ nsICODecoder::PrepareForMask()
   MOZ_ASSERT(numColors != uint16_t(-1));
 
   // Determine the length of the AND mask.
   uint32_t bmpLengthWithHeader =
     BITMAPINFOSIZE + bmpDecoder->GetCompressedImageSize() + 4 * numColors;
   MOZ_ASSERT(bmpLengthWithHeader < mDirEntry.mBytesInRes);
   uint32_t maskLength = mDirEntry.mBytesInRes - bmpLengthWithHeader;
 
-  // If we have a 32-bpp BMP with alpha data, we ignore the AND mask. We can
+  // If the BMP provides its own transparency, we ignore the AND mask. We can
   // also obviously ignore it if the image has zero width or zero height.
-  if ((bmpDecoder->GetBitsPerPixel() == 32 && bmpDecoder->HasAlphaData()) ||
+  if (bmpDecoder->HasTransparency() ||
       GetRealWidth() == 0 || GetRealHeight() == 0) {
     return Transition::ToUnbuffered(ICOState::FINISHED_RESOURCE,
                                     ICOState::SKIP_MASK,
                                     maskLength);
   }
 
   // Compute the row size for the mask.
   mMaskRowSize = ((GetRealWidth() + 31) / 32) * 4; // + 31 to round up
@@ -653,17 +653,17 @@ nsICODecoder::FinishMask()
   }
 
   // If the mask contained any transparent pixels, record that fact.
   if (mHasMaskAlpha) {
     PostHasTransparency();
 
     RefPtr<nsBMPDecoder> bmpDecoder =
       static_cast<nsBMPDecoder*>(mContainedDecoder.get());
-    bmpDecoder->SetHasAlphaData();
+    bmpDecoder->SetHasTransparency();
   }
 
   return Transition::To(ICOState::FINISHED_RESOURCE, 0);
 }
 
 LexerTransition<ICOState>
 nsICODecoder::FinishResource()
 {
--- a/image/test/gtest/Common.cpp
+++ b/image/test/gtest/Common.cpp
@@ -189,23 +189,23 @@ ImageTestCase TransparentGIFTestCase()
 }
 
 ImageTestCase FirstFramePaddingGIFTestCase()
 {
   return ImageTestCase("transparent.gif", "image/gif", IntSize(16, 16),
                        TEST_CASE_IS_TRANSPARENT);
 }
 
-ImageTestCase TransparentBMPWhenBMPAlphaEnabledTestCase()
+ImageTestCase TransparentIfWithinICOBMPTestCase(TestCaseFlags aFlags)
 {
-  // Note that we only decode this test case as transparent when the BMP decoder
-  // is set to use alpha data. (That's not the default, which is why it's not marked
-  // TEST_CASE_IS_TRANSPARENT; tests that want to treat this testcase as
-  // transparent need to handle this case manually.)
-  return ImageTestCase("transparent.bmp", "image/bmp", IntSize(32, 32));
+  // This is a BMP that is only transparent when decoded as if it is within an
+  // ICO file. (Note: aFlags needs to be set to TEST_CASE_DEFAULT_FLAGS or
+  // TEST_CASE_IS_TRANSPARENT accordingly.)
+  return ImageTestCase("transparent-if-within-ico.bmp", "image/bmp",
+                       IntSize(32, 32), aFlags);
 }
 
 ImageTestCase RLE4BMPTestCase()
 {
   return ImageTestCase("rle4.bmp", "image/bmp", IntSize(320, 240),
                        TEST_CASE_IS_TRANSPARENT);
 }
 
--- a/image/test/gtest/TestMetadata.cpp
+++ b/image/test/gtest/TestMetadata.cpp
@@ -33,25 +33,25 @@ TEST(ImageMetadata, ImageModuleAvailable
   // We can run into problems if XPCOM modules get initialized in the wrong
   // order. It's important that this test run first, both as a sanity check and
   // to ensure we get the module initialization order we want.
   nsCOMPtr<imgITools> imgTools =
     do_CreateInstance("@mozilla.org/image/tools;1");
   EXPECT_TRUE(imgTools != nullptr);
 }
 
-enum class BMPAlpha
+enum class BMPWithinICO
 {
-  DISABLED,
-  ENABLED
+  NO,
+  YES
 };
 
 static void
 CheckMetadata(const ImageTestCase& aTestCase,
-              BMPAlpha aBMPAlpha = BMPAlpha::DISABLED)
+              BMPWithinICO aBMPWithinICO = BMPWithinICO::NO)
 {
   nsCOMPtr<nsIInputStream> inputStream = LoadFile(aTestCase.mPath);
   ASSERT_TRUE(inputStream != nullptr);
 
   // Figure out how much data we have.
   uint64_t length;
   nsresult rv = inputStream->Available(&length);
   ASSERT_TRUE(NS_SUCCEEDED(rv));
@@ -65,23 +65,23 @@ CheckMetadata(const ImageTestCase& aTest
 
   // Create a metadata decoder.
   DecoderType decoderType =
     DecoderFactory::GetDecoderType(aTestCase.mMimeType);
   RefPtr<Decoder> decoder =
     DecoderFactory::CreateAnonymousMetadataDecoder(decoderType, sourceBuffer);
   ASSERT_TRUE(decoder != nullptr);
 
-  if (aBMPAlpha == BMPAlpha::ENABLED) {
-    static_cast<nsBMPDecoder*>(decoder.get())->SetUseAlphaData(true);
+  if (aBMPWithinICO == BMPWithinICO::YES) {
+    static_cast<nsBMPDecoder*>(decoder.get())->SetIsWithinICO();
   }
 
   // Run the metadata decoder synchronously.
   decoder->Decode();
-  
+
   // Ensure that the metadata decoder didn't make progress it shouldn't have
   // (which would indicate that it decoded past the header of the image).
   Progress metadataProgress = decoder->TakeProgress();
   EXPECT_TRUE(0 == (metadataProgress & ~(FLAG_SIZE_AVAILABLE |
                                          FLAG_HAS_TRANSPARENCY |
                                          FLAG_IS_ANIMATED)));
 
   // If the test case is corrupt, assert what we can and return early.
@@ -95,37 +95,37 @@ CheckMetadata(const ImageTestCase& aTest
 
   // Check that we got the expected metadata.
   EXPECT_TRUE(metadataProgress & FLAG_SIZE_AVAILABLE);
 
   IntSize metadataSize = decoder->GetSize();
   EXPECT_EQ(aTestCase.mSize.width, metadataSize.width);
   EXPECT_EQ(aTestCase.mSize.height, metadataSize.height);
 
-  bool expectTransparency = aBMPAlpha == BMPAlpha::ENABLED
+  bool expectTransparency = aBMPWithinICO == BMPWithinICO::YES
                           ? true
                           : bool(aTestCase.mFlags & TEST_CASE_IS_TRANSPARENT);
   EXPECT_EQ(expectTransparency, bool(metadataProgress & FLAG_HAS_TRANSPARENCY));
 
   EXPECT_EQ(bool(aTestCase.mFlags & TEST_CASE_IS_ANIMATED),
             bool(metadataProgress & FLAG_IS_ANIMATED));
 
   // Create a full decoder, so we can compare the result.
   decoder =
     DecoderFactory::CreateAnonymousDecoder(decoderType, sourceBuffer,
                                            DefaultSurfaceFlags());
   ASSERT_TRUE(decoder != nullptr);
 
-  if (aBMPAlpha == BMPAlpha::ENABLED) {
-    static_cast<nsBMPDecoder*>(decoder.get())->SetUseAlphaData(true);
+  if (aBMPWithinICO == BMPWithinICO::YES) {
+    static_cast<nsBMPDecoder*>(decoder.get())->SetIsWithinICO();
   }
 
   // Run the full decoder synchronously.
   decoder->Decode();
-  
+
   EXPECT_TRUE(decoder->GetDecodeDone() && !decoder->HasError());
   Progress fullProgress = decoder->TakeProgress();
 
   // If the metadata decoder set a progress bit, the full decoder should also
   // have set the same bit.
   EXPECT_EQ(fullProgress, metadataProgress | fullProgress);
 
   // The full decoder and the metadata decoder should agree on the image's size.
@@ -159,24 +159,26 @@ TEST(ImageMetadata, AnimatedPNG)
   CheckMetadata(GreenFirstFrameAnimatedPNGTestCase());
 }
 
 TEST(ImageMetadata, FirstFramePaddingGIF)
 {
   CheckMetadata(FirstFramePaddingGIFTestCase());
 }
 
-TEST(ImageMetadata, TransparentBMPWithBMPAlphaOff)
+TEST(ImageMetadata, TransparentIfWithinICOBMPNotWithinICO)
 {
-  CheckMetadata(TransparentBMPWhenBMPAlphaEnabledTestCase(), BMPAlpha::ENABLED);
+  CheckMetadata(TransparentIfWithinICOBMPTestCase(TEST_CASE_DEFAULT_FLAGS),
+                BMPWithinICO::NO);
 }
 
-TEST(ImageMetadata, TransparentBMPWithBMPAlphaOn)
+TEST(ImageMetadata, TransparentIfWithinICOBMPWithinICO)
 {
-  CheckMetadata(TransparentBMPWhenBMPAlphaEnabledTestCase(), BMPAlpha::ENABLED);
+  CheckMetadata(TransparentIfWithinICOBMPTestCase(TEST_CASE_IS_TRANSPARENT),
+                BMPWithinICO::YES);
 }
 
 TEST(ImageMetadata, RLE4BMP) { CheckMetadata(RLE4BMPTestCase()); }
 TEST(ImageMetadata, RLE8BMP) { CheckMetadata(RLE8BMPTestCase()); }
 
 TEST(ImageMetadata, Corrupt) { CheckMetadata(CorruptTestCase()); }
 
 TEST(ImageMetadata, NoFrameDelayGIF)
--- a/image/test/gtest/moz.build
+++ b/image/test/gtest/moz.build
@@ -24,17 +24,17 @@ TEST_HARNESS_FILES.gtest += [
     'green.gif',
     'green.ico',
     'green.icon',
     'green.jpg',
     'green.png',
     'no-frame-delay.gif',
     'rle4.bmp',
     'rle8.bmp',
-    'transparent.bmp',
+    'transparent-if-within-ico.bmp',
     'transparent.gif',
     'transparent.png',
 ]
 
 LOCAL_INCLUDES += [
     '/image',
 ]
 
rename from image/test/gtest/transparent.bmp
rename to image/test/gtest/transparent-if-within-ico.bmp
index c3ee2289598752d21f1f922520f5430f689382f3..4dc04c181ba7d4ca9ac41fcb670f6f5fa3ca37dd
GIT binary patch
literal 4234
zc$~eLF;4<P5QQZg6Rk{)EwM7xQrplN3Ja~R`2)7JHY9|agkQi9Ahy_26FXW83o7gL
zzJq7Rb(h;akGn9C-P_xF^XAR$(RmnL9A(!awN`4^J8NX$>9xE6`%3i52A%t-m(lxI
z>+!95)qMSIwA<~=UaxnP+Ua;aKD#^rIBq?UZtV^y&W^L$EXtP4WwKhWlGycC+k8Gx
z`u%<~nM@L})ae_4_{QM9+wCUP>9jT+#E#!IubJ<y5Vkn#h#kN3DGsa&$D#PJzI^^=
z;DoCxaag|d{M$8E;6P)(7;QfG*>E^a7K=q`3~uboHT%Y5z~G{a*u^b|fBnYrP*n52
z*j0b@AWVFK!?(U{ooa7i?84#Jh4={Ji1m$LLbYZ0CZ~3cZ|TB^gSnV`$ctTuK4j7t
z{#~8eXY%}WzU_-$=MVm5dHu-Z0BfFqpPuDcF$#~)g^=cK4aJ_FmP0+qUOrv1K2(3~
z<8s5=ymtsU|I(F@XXbr+abvfdiKziMc6@~Tf$tu3E-#E@^sk(<C(ndp*BYB!^+O*g
zMS7nbyQ^m~!KKBebAfqRL$R@|1{Ayctnw@_Y>YZ<>09^~XEp~#&mX&wua~I?S+`!V
ze`4lY#<pL?_C2=;hw4f>;r?c``Lj+nu5sD(UzmOSD|Y=SaP<Z*@l?gT7{1hl<cm4g
z9{B}_bf?<?&%2M058;sR>{$sXjKN|1b=3wAaUjiGU827-nk$P#HH%5Nhl)K^E4JT|
UpSu4a3J1K$o-v2YNzvH90W$(5#Q*>R
--- a/image/test/reftest/bmp/bmpsuite/g/reftest.list
+++ b/image/test/reftest/bmp/bmpsuite/g/reftest.list
@@ -103,12 +103,10 @@ fuzzy(1,1516) == rgb16.bmp rgb16.png
 # BITFIELDS segment). There are 8 bits per color channel, and 8 unused bits.
 # The unused bits are set to 0."
 == rgb32.bmp rgb24.png
 
 # BMP: bihsize=40, 127 x 64, bpp=32, compression=3, colors=0
 # "A 32-bit image with a BITFIELDS segment. As usual, there are 8 bits per color
 # channel, and 8 unused bits. But the color channels are in an unusual order,
 # so the viewer must read the BITFIELDS, and not just guess."
-# [XXX: we get this completely wrong because we don't handle BITFIELDS at all
-# in 32bpp BMPs. Chromium gets this right.]
-fails == rgb32bf.bmp rgb24.png
+== rgb32bf.bmp rgb24.png
 
--- a/image/test/reftest/bmp/bmpsuite/q/reftest.list
+++ b/image/test/reftest/bmp/bmpsuite/q/reftest.list
@@ -66,19 +66,17 @@ fuzzy(1,899) == pal8os2v2-16.bmp pal8.pn
 # (possibly including one round of bit replication), instead of proper
 # scaling."
 == rgb16-231.bmp rgb16-231.png
 
 # BMP: bihsize=124, 127 x 64, bpp=16, compression=3, colors=0
 # "A 16-bit image with an alpha channel. There are 4 bits for each color
 # channel, and 4 bits for the alpha channel. It’s not clear if this is valid,
 # but I can’t find anything that suggests it isn’t."
-# [XXX: we don't even try to do transparency for 16bpp. Chromium gets the
-# transparency right.]
-fails == rgba16-4444.bmp rgba16-4444.png
+== rgba16-4444.bmp rgba16-4444.png
 
 # BMP: bihsize=40, 127 x 64, bpp=24, compression=0, colors=300
 # "A 24-bit image, with a palette containing 300 colors. The fact that the
 # palette has more than 256 colors may cause some viewers to complain, but the
 # documentation does not mention a size limit."
 # [We accept it. So does Chromium.]
 == rgb24largepal.bmp rgb24.png
 
@@ -109,28 +107,24 @@ fails == rgba16-4444.bmp rgba16-4444.png
 # the viewer uses heuristics to guess whether the undefined data represents
 # transparency."
 # [We don't apply transparency here. Chromium does the same.]
 == rgb32fakealpha.bmp rgb24.png
 
 # BMP: bihsize=40, 127 x 64, bpp=32, compression=3, colors=0
 # "A 32 bits/pixel image, with all 32 bits used: 11 each for red and green, and
 # 10 for blue. As far as I know, this is perfectly valid, but it is unusual."
-# [XXX: we get this completely wrong because we don't handle BITFIELDS at all
-# in 32bpp BMPs. Chromium gets this right.]
-fails == rgb32-111110.bmp rgb24.png
+fuzzy(1,1408) == rgb32-111110.bmp rgb24.png
 
 # BMP: bihsize=124, 127 x 64, bpp=32, compression=3, colors=0
 # "A BMP with an alpha channel. Transparency is barely documented, so it’s
 # possible that this file is not correctly formed. The color channels are in an
 # unusual order, to prevent viewers from passing this test by making a lucky
 # guess."
-# [XXX: we get this completely wrong because we don't handle BITFIELDS at all
-# in 32bpp BMPs, especially not with alpha. Chromium gets this right.]
-fails == rgba32.bmp rgba32.png
+== rgba32.bmp rgba32.png
 
 # BMP: bihsize=40, 127 x 64, bpp=32, compression=6, colors=0
 # "An image of type BI_ALPHABITFIELDS. Supposedly, this was used on Windows CE.
 # I don’t know whether it is constructed correctly."
 # [We reject it. So does Chromium.]
 == wrapper.html?rgba32abf.bmp about:blank