Never consolidate data in ICO decoder

Source/platform/image-decoders/ico/ICOImageDecoder.cpp

 /*
  * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 28 matching lines @@
 namespace blink {
 
 // Number of bits in .ICO/.CUR used to store the directory and its entries,
 // respectively (doesn't match sizeof values for member structs since we omit
 // some fields).
 static const size_t sizeOfDirectory = 6;
 static const size_t sizeOfDirEntry = 16;
 
 ICOImageDecoder::ICOImageDecoder(AlphaOption alphaOption, GammaAndColorProfileOption colorOptions, size_t maxDecodedBytes)
     : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes)
+    , m_fastReader(nullptr)
     , m_decodedOffset(0)
 {
 }
 
 ICOImageDecoder::~ICOImageDecoder()
 {
 }
 
 void ICOImageDecoder::onSetData(SharedBuffer* data)
 {
+    m_fastReader.setData(data);
+
     for (BMPReaders::iterator i(m_bmpReaders.begin()); i != m_bmpReaders.end(); ++i) {
         if (*i)
             (*i)->setData(data);
     }
     for (size_t i = 0; i < m_pngDecoders.size(); ++i)
         setDataForPNGDecoderAtIndex(i);
 }
 
 IntSize ICOImageDecoder::size() const
 {
@@ skipping 50 matching lines @@
 {
     decodeSize();
     return m_dirEntries.size();
 }
 
 void ICOImageDecoder::setDataForPNGDecoderAtIndex(size_t index)
 {
     if (!m_pngDecoders[index])
         return;
 
-    const IconDirectoryEntry& dirEntry = m_dirEntries[index];
-    // Copy out PNG data to a separate vector and send to the PNG decoder.
-    // FIXME: Save this copy by making the PNG decoder able to take an
-    // optional offset.
-    RefPtr<SharedBuffer> pngData(SharedBuffer::create(&m_data->data()[dirEntry.m_imageOffset], m_data->size() - dirEntry.m_imageOffset));
-    m_pngDecoders[index]->setData(pngData.get(), isAllDataReceived());
+    m_pngDecoders[index]->setData(m_data.get(), isAllDataReceived());
 }
 
 void ICOImageDecoder::decode(size_t index, bool onlySize)
 {
     if (failed())
         return;
 
     // If we couldn't decode the image but we've received all the data, decoding
     // has failed.
     if ((!decodeDirectory() || (!onlySize && !decodeAtIndex(index))) && isAllDataReceived())
@@ skipping 34 matching lines @@
             m_bmpReaders[index]->setData(m_data.get());
             m_bmpReaders[index]->setBuffer(&m_frameBufferCache[index]);
         }
         m_frameSize = dirEntry.m_size;
         bool result = m_bmpReaders[index]->decodeBMP(false);
         m_frameSize = IntSize();
         return result;
     }
 
     if (!m_pngDecoders[index]) {
-        m_pngDecoders[index] = adoptPtr(
-            new PNGImageDecoder(m_premultiplyAlpha ? AlphaPremultiplied : AlphaNotPremultiplied,
-                m_ignoreGammaAndColorProfile ? GammaAndColorProfileIgnored : GammaAndColorProfileApplied, m_maxDecodedBytes));
+        AlphaOption alphaOption = m_premultiplyAlpha ? AlphaPremultiplied : AlphaNotPremultiplied;
+        GammaAndColorProfileOption colorOptions = m_ignoreGammaAndColorProfile ? GammaAndColorProfileIgnored : GammaAndColorProfileApplied;

[Peter Kasting, 2015/09/08 20:03:13]

Nit: I'd name this "option" instead of "options" to match the singularity of the
type name

(Although I might omit these and just reformat like this:)

        m_pngDecoders[index] = adoptPtr(new PNGImageDecoder(
            m_premultiplyAlpha ? AlphaPremultiplied : AlphaNotPremultiplied,
            m_ignoreGammaAndColorProfile ? GammaAndColorProfileIgnored :
GammaAndColorProfileApplied,
            m_maxDecodedBytes,
            dirEntry.m_imageOffset));

[scroggo_chromium, 2015/09/08 20:52:51]

This name was suggested to me by noel@ a refactoring CL, and it matches the
other uses of this type.

+
+        // dirEntry.m_imageOffset is known to be correct, since this function is never called unless decodeDirectory succeeds.

[Peter Kasting, 2015/09/08 20:03:13]

Nit: This comment is probably unnecessary, we assume similar ordering
requirements many places.

I would probably remove the blank lines above and below here too.

[scroggo_chromium, 2015/09/08 20:52:51]

Done.

+        m_pngDecoders[index] = adoptPtr(new PNGImageDecoder(alphaOption, colorOptions, m_maxDecodedBytes, dirEntry.m_imageOffset));
+
         setDataForPNGDecoderAtIndex(index);
     }
+
     // Fail if the size the PNGImageDecoder calculated does not match the size
     // in the directory.
     if (m_pngDecoders[index]->isSizeAvailable() && (m_pngDecoders[index]->size() != dirEntry.m_size))
         return setFailed();
     m_frameBufferCache[index] = *m_pngDecoders[index]->frameBufferAtIndex(0);
     m_frameBufferCache[index].setPremultiplyAlpha(m_premultiplyAlpha);
     return !m_pngDecoders[index]->failed() || setFailed();
 }
 
 bool ICOImageDecoder::processDirectory()
@@ skipping 42 matching lines @@
     // The image size is the size of the largest entry.
     const IconDirectoryEntry& dirEntry = m_dirEntries.first();
     // Technically, this next call shouldn't be able to fail, since the width
     // and height here are each <= 256, and |m_frameSize| is empty.
     return setSize(dirEntry.m_size.width(), dirEntry.m_size.height());
 }
 
 ICOImageDecoder::IconDirectoryEntry ICOImageDecoder::readDirectoryEntry()
 {
     // Read icon data.
-    // The casts to uint8_t in the next few lines are because that's the on-disk
-    // type of the width and height values.  Storing them in ints (instead of
-    // matching uint8_ts) is so we can record dimensions of size 256 (which is
-    // what a zero byte really means).
-    int width = static_cast<uint8_t>(m_data->data()[m_decodedOffset]);
+    // The following calls to readUint8 return an uint8_t, which is appropriate

[Peter Kasting, 2015/09/08 20:03:13]

Nit: "...readUint8() return a uint8_t..." (add parent, an -> a)

[scroggo_chromium, 2015/09/08 20:52:51]

Done.

+    // because that's the on-disk type of the width and height values.  Storing
+    // them in ints (instead of matching uint8_ts) is so we can record dimensions
+    // of size 256 (which is what a zero byte really means).
+    int width = readUint8(0);
     if (!width)
         width = 256;
-    int height = static_cast<uint8_t>(m_data->data()[m_decodedOffset + 1]);
+    int height = readUint8(1);
     if (!height)
         height = 256;
     IconDirectoryEntry entry;
     entry.m_size = IntSize(width, height);
     if (m_fileType == CURSOR) {
         entry.m_bitCount = 0;
         entry.m_hotSpot = IntPoint(readUint16(4), readUint16(6));
     } else {
         entry.m_bitCount = readUint16(6);
         entry.m_hotSpot = IntPoint();
     }
     entry.m_imageOffset = readUint32(12);
 
     // Some icons don't have a bit depth, only a color count.  Convert the
     // color count to the minimum necessary bit depth.  It doesn't matter if
     // this isn't quite what the bitmap info header says later, as we only use
     // this value to determine which icon entry is best.
     if (!entry.m_bitCount) {
-        int colorCount = static_cast<uint8_t>(m_data->data()[m_decodedOffset + 2]);
+        int colorCount = readUint8(2);
         if (!colorCount)
             colorCount = 256;  // Vague in the spec, needed by real-world icons.
         for (--colorCount; colorCount; colorCount >>= 1)
             ++entry.m_bitCount;
     }
 
     m_decodedOffset += sizeOfDirEntry;
     return entry;
 }
 
 ICOImageDecoder::ImageType ICOImageDecoder::imageTypeAtIndex(size_t index)
 {
     // Check if this entry is a BMP or a PNG; we need 4 bytes to check the magic
     // number.
     ASSERT_WITH_SECURITY_IMPLICATION(index < m_dirEntries.size());
     const uint32_t imageOffset = m_dirEntries[index].m_imageOffset;
     if ((imageOffset > m_data->size()) || ((m_data->size() - imageOffset) < 4))
         return Unknown;
-    return strncmp(&m_data->data()[imageOffset], "\x89PNG", 4) ? BMP : PNG;
+    char buffer[4];
+    const char* data = m_fastReader.getConsecutiveData(imageOffset, 4, buffer);
+    return strncmp(data, "\x89PNG", 4) ? BMP : PNG;
 }
 
 }