Bug 1315554 - Part 7. Remove unnecessary buffering of BMP header in ICO decoder. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Sat, 22 Jul 2017 07:50:32 -0400
changeset 419117 80c948a98dca646e1f838a0c26318066fd927cc8
parent 419116 6cf887326d8894d24eff7301b4136c34437295c3
child 419118 1b2f04e53ea25844c7ef90e0ff297abe671f3aaa
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1315554
milestone56.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 1315554 - Part 7. Remove unnecessary buffering of BMP header in ICO decoder. r=tnikkel
image/decoders/nsICODecoder.cpp
image/decoders/nsICODecoder.h
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -244,114 +244,117 @@ nsICODecoder::ReadDirEntry(const char* a
   }
 
   return Transition::To(ICOState::DIR_ENTRY, ICODIRENTRYSIZE);
 }
 
 LexerTransition<ICOState>
 nsICODecoder::SniffResource(const char* aData)
 {
-  // Prepare a new iterator for the contained decoder to advance as it wills.
-  // Cloning at the point ensures it will begin at the resource offset.
-  mContainedIterator.emplace(mLexer.Clone(*mIterator, mDirEntry.mBytesInRes));
+  // We have BITMAPINFOSIZE bytes buffered at this point. We know an embedded
+  // BMP will have at least that many bytes by definition. We can also infer
+  // that any valid embedded PNG will contain that many bytes as well because:
+  //    BITMAPINFOSIZE
+  //      <
+  //    signature (8 bytes) +
+  //    IHDR (12 bytes header + 13 bytes data)
+  //    IDAT (12 bytes header)
 
   // We use the first PNGSIGNATURESIZE bytes to determine whether this resource
   // is a PNG or a BMP.
   bool isPNG = !memcmp(aData, nsPNGDecoder::pngSignatureBytes,
                        PNGSIGNATURESIZE);
   if (isPNG) {
-    if (mDirEntry.mBytesInRes <= PNGSIGNATURESIZE) {
+    if (mDirEntry.mBytesInRes <= BITMAPINFOSIZE) {
       return Transition::TerminateFailure();
     }
 
+    // Prepare a new iterator for the contained decoder to advance as it wills.
+    // Cloning at the point ensures it will begin at the resource offset.
+    SourceBufferIterator containedIterator
+      = mLexer.Clone(*mIterator, mDirEntry.mBytesInRes);
+
     // Create a PNG decoder which will do the rest of the work for us.
     mContainedDecoder =
       DecoderFactory::CreateDecoderForICOResource(DecoderType::PNG,
-                                                  Move(*mContainedIterator),
+                                                  Move(containedIterator),
                                                   WrapNotNull(this),
                                                   Some(GetRealSize()));
 
     // Read in the rest of the PNG unbuffered.
-    size_t toRead = mDirEntry.mBytesInRes - PNGSIGNATURESIZE;
+    size_t toRead = mDirEntry.mBytesInRes - BITMAPINFOSIZE;
     return Transition::ToUnbuffered(ICOState::FINISHED_RESOURCE,
                                     ICOState::READ_RESOURCE,
                                     toRead);
   } else {
     // Make sure we have a sane size for the bitmap information header.
     int32_t bihSize = LittleEndian::readUint32(aData);
     if (bihSize != static_cast<int32_t>(BITMAPINFOSIZE)) {
       return Transition::TerminateFailure();
     }
 
-    // Buffer the first part of the bitmap information header.
-    memcpy(mBIHraw, aData, PNGSIGNATURESIZE);
-
     // Read in the rest of the bitmap information header.
-    return Transition::To(ICOState::READ_BIH,
-                          BITMAPINFOSIZE - PNGSIGNATURESIZE);
+    return ReadBIH(aData);
   }
 }
 
 LexerTransition<ICOState>
 nsICODecoder::ReadResource()
 {
   if (!FlushContainedDecoder()) {
     return Transition::TerminateFailure();
   }
 
   return Transition::ContinueUnbuffered(ICOState::READ_RESOURCE);
 }
 
 LexerTransition<ICOState>
 nsICODecoder::ReadBIH(const char* aData)
 {
-  // Buffer the rest of the bitmap information header.
-  memcpy(mBIHraw + PNGSIGNATURESIZE, aData, BITMAPINFOSIZE - PNGSIGNATURESIZE);
-
   // Extract the BPP from the BIH header; it should be trusted over the one
   // we have from the ICO header which is usually set to 0.
-  mBPP = LittleEndian::readUint16(mBIHraw + 14);
+  mBPP = LittleEndian::readUint16(aData + 14);
+
+  // Check to make sure we have valid color settings.
+  uint16_t numColors = GetNumColors();
+  if (numColors == uint16_t(-1)) {
+    return Transition::TerminateFailure();
+  }
+
+  // The color table is present only if BPP is <= 8.
+  MOZ_ASSERT_IF(mBPP > 8, numColors == 0);
 
   // The ICO format when containing a BMP does not include the 14 byte
   // bitmap file header. So we create the BMP decoder via the constructor that
   // tells it to skip this, and pass in the required data (dataOffset) that
   // would have been present in the header.
-  uint32_t dataOffset = bmp::FILE_HEADER_LENGTH + BITMAPINFOSIZE;
-  if (mBPP <= 8) {
-    // The color table is present only if BPP is <= 8.
-    uint16_t numColors = GetNumColors();
-    if (numColors == (uint16_t)-1) {
-      return Transition::TerminateFailure();
-    }
-    dataOffset += 4 * numColors;
-  }
+  uint32_t dataOffset = bmp::FILE_HEADER_LENGTH + BITMAPINFOSIZE + 4 * numColors;
+
+  // Prepare a new iterator for the contained decoder to advance as it wills.
+  // Cloning at the point ensures it will begin at the resource offset.
+  SourceBufferIterator containedIterator
+    = mLexer.Clone(*mIterator, mDirEntry.mBytesInRes);
 
   // 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.
   mContainedDecoder =
     DecoderFactory::CreateDecoderForICOResource(DecoderType::BMP,
-                                                Move(*mContainedIterator),
+                                                Move(containedIterator),
                                                 WrapNotNull(this),
                                                 Some(GetRealSize()),
                                                 Some(dataOffset));
 
   RefPtr<nsBMPDecoder> bmpDecoder =
     static_cast<nsBMPDecoder*>(mContainedDecoder.get());
 
   // Ensure the decoder has parsed at least the BMP's bitmap info header.
   if (!FlushContainedDecoder()) {
     return Transition::TerminateFailure();
   }
 
-  // Check to make sure we have valid color settings.
-  uint16_t numColors = GetNumColors();
-  if (numColors == uint16_t(-1)) {
-    return Transition::TerminateFailure();
-  }
-
   // Do we have an AND mask on this BMP? If so, we need to read it after we read
   // the BMP data itself.
   uint32_t bmpDataLength = bmpDecoder->GetCompressedImageSize() + 4 * numColors;
   bool hasANDMask = (BITMAPINFOSIZE + bmpDataLength) < mDirEntry.mBytesInRes;
   ICOState afterBMPState = hasANDMask ? ICOState::PREPARE_FOR_MASK
                                       : ICOState::FINISHED_RESOURCE;
 
   // Read in the rest of the BMP unbuffered.
@@ -557,23 +560,21 @@ nsICODecoder::DoDecode(SourceBufferItera
     switch (aState) {
       case ICOState::HEADER:
         return ReadHeader(aData);
       case ICOState::DIR_ENTRY:
         return ReadDirEntry(aData);
       case ICOState::SKIP_TO_RESOURCE:
         return Transition::ContinueUnbuffered(ICOState::SKIP_TO_RESOURCE);
       case ICOState::FOUND_RESOURCE:
-        return Transition::To(ICOState::SNIFF_RESOURCE, PNGSIGNATURESIZE);
+        return Transition::To(ICOState::SNIFF_RESOURCE, BITMAPINFOSIZE);
       case ICOState::SNIFF_RESOURCE:
         return SniffResource(aData);
       case ICOState::READ_RESOURCE:
         return ReadResource();
-      case ICOState::READ_BIH:
-        return ReadBIH(aData);
       case ICOState::PREPARE_FOR_MASK:
         return PrepareForMask();
       case ICOState::READ_MASK_ROW:
         return ReadMaskRow(aData);
       case ICOState::FINISH_MASK:
         return FinishMask();
       case ICOState::SKIP_MASK:
         return Transition::ContinueUnbuffered(ICOState::SKIP_MASK);
--- a/image/decoders/nsICODecoder.h
+++ b/image/decoders/nsICODecoder.h
@@ -23,17 +23,16 @@ class RasterImage;
 enum class ICOState
 {
   HEADER,
   DIR_ENTRY,
   SKIP_TO_RESOURCE,
   FOUND_RESOURCE,
   SNIFF_RESOURCE,
   READ_RESOURCE,
-  READ_BIH,
   PREPARE_FOR_MASK,
   READ_MASK_ROW,
   FINISH_MASK,
   SKIP_MASK,
   FINISHED_RESOURCE
 };
 
 class nsICODecoder : public Decoder
@@ -96,19 +95,17 @@ private:
   LexerTransition<ICOState> ReadBIH(const char* aData);
   LexerTransition<ICOState> PrepareForMask();
   LexerTransition<ICOState> ReadMaskRow(const char* aData);
   LexerTransition<ICOState> FinishMask();
   LexerTransition<ICOState> FinishResource();
 
   StreamingLexer<ICOState, 32> mLexer; // The lexer.
   RefPtr<Decoder> mContainedDecoder; // Either a BMP or PNG decoder.
-  Maybe<SourceBufferIterator> mContainedIterator; // Iterator for the subdecoder.
   UniquePtr<uint8_t[]> mMaskBuffer;    // A temporary buffer for the alpha mask.
-  char mBIHraw[bmp::InfoHeaderLength::WIN_ICO]; // The bitmap information header.
   IconDirEntry mDirEntry;              // The dir entry for the selected resource.
   gfx::IntSize mBiggestResourceSize;   // Used to select the intrinsic size.
   gfx::IntSize mBiggestResourceHotSpot; // Used to select the intrinsic size.
   uint16_t mBiggestResourceColorDepth; // Used to select the intrinsic size.
   int32_t mBestResourceDelta;          // Used to select the best resource.
   uint16_t mBestResourceColorDepth;    // Used to select the best resource.
   uint16_t mNumIcons; // Stores the number of icons in the ICO file.
   uint16_t mCurrIcon; // Stores the current dir entry index we are processing.