Never consolidate data in ICO decoder

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

 /*
  * 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 13 matching lines @@
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #ifndef ICOImageDecoder_h
 #define ICOImageDecoder_h
 
+#include "platform/image-decoders/FastSharedBufferReader.h"
 #include "platform/image-decoders/bmp/BMPImageReader.h"
 
 namespace blink {
 
 class PNGImageDecoder;
 
 // This class decodes the ICO and CUR image formats.
 class PLATFORM_EXPORT ICOImageDecoder : public ImageDecoder {
 public:
     ICOImageDecoder(AlphaOption, GammaAndColorProfileOption, size_t maxDecodedBytes);
@@ skipping 32 matching lines @@
 
     // Returns true if |a| is a preferable icon entry to |b|.
     // Larger sizes, or greater bitdepths at the same size, are preferable.
     static bool compareEntries(const IconDirectoryEntry& a, const IconDirectoryEntry& b);
 
     // ImageDecoder:
     virtual void decodeSize() { decode(0, true); }
     size_t decodeFrameCount() override;
     void decode(size_t index) override { decode(index, false); }
 
+    // TODO (scroggo): These functions are identical to functions in BMPImageReader. Share code?

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

Yeah, we should share the code.

This seems like another argument for making the buffer reader object part of
ImageDecoder.  Then we could make these functions protected members of
ImageDecoder as well.  We could also avoid having to customize setData() to
update the buffer reader in each decoder.

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

On the other hand, not all of the classes need a FastSharedBufferReader (PNG and
JPEG do not need one; I have not looked at WEBP yet.)

[Peter Kasting, 2015/09/08 21:01:05]

Well, with PNG and JPEG, we rely on an external decoding library.  Doesn't that
library require consolidating the data passed to it?  So it's not so much "don't
need" as "as currently implemented these just unconditionally coalesce".

[scroggo_chromium, 2015/09/09 14:32:23]

Actually, no. JPEG does use an external library, but with my recent changes to
JPEGImageDecoder, libjpeg directly uses the data we give to it. So it will not
do any coalescing.

I haven't dug into PNG very much, but presumably it copies the data when we
supply it. It still doesn't need a FastSharedBufferReader. Maybe it would be
fine for it to have an unused one (it is only four fields, plus the RefPtr; it
also would mean some unnecessary calls to FastSharedBufferReader::setData, but
maybe this is all negligible).

+    inline uint8_t readUint8(size_t offset) const
+    {
+        return m_fastReader.getOneByte(m_decodedOffset + offset);
+    }
+
     inline uint16_t readUint16(int offset) const
     {
-        // TODO (scroggo): This consolidates the data, meaning unnecessary copies.
-        return BMPImageReader::readUint16(&m_data->data()[m_decodedOffset + offset]);
+        char buffer[2];
+        const char* data = m_fastReader.getConsecutiveData(m_decodedOffset + offset, 2, buffer);
+        return BMPImageReader::readUint16(data);
     }
 
     inline uint32_t readUint32(int offset) const
     {
-        // TODO (scroggo): This consolidates the data, meaning unnecessary copies.
-        return BMPImageReader::readUint32(&m_data->data()[m_decodedOffset + offset]);
+        char buffer[4];
+        const char* data = m_fastReader.getConsecutiveData(m_decodedOffset + offset, 4, buffer);
+        return BMPImageReader::readUint32(data);
     }
 
     // If the desired PNGImageDecoder exists, gives it the appropriate data.
     void setDataForPNGDecoderAtIndex(size_t);
 
     // Decodes the entry at |index|.  If |onlySize| is true, stops decoding
     // after calculating the image size.  If decoding fails but there is no
     // more data coming, sets the "decode failure" flag.
     void decode(size_t index, bool onlySize);
 
@@ skipping 19 matching lines @@
     bool hotSpotAtIndex(size_t index, IntPoint& hotSpot) const;
 
     // Reads and returns a directory entry from the current offset into
     // |data|.
     IconDirectoryEntry readDirectoryEntry();
 
     // Determines whether the desired entry is a BMP or PNG.  Returns true
     // if the type could be determined.
     ImageType imageTypeAtIndex(size_t);
 
+    FastSharedBufferReader m_fastReader;
+
     // An index into |m_data| representing how much we've already decoded.
     // Note that this only tracks data _this_ class decodes; once the
     // BMPImageReader takes over this will not be updated further.
     size_t m_decodedOffset;
 
     // Which type of file (ICO/CUR) this is.
     FileType m_fileType;
 
     // The headers for the ICO.
     typedef Vector<IconDirectoryEntry> IconDirectoryEntries;
     IconDirectoryEntries m_dirEntries;
 
     // The image decoders for the various frames.
     typedef Vector<OwnPtr<BMPImageReader>> BMPReaders;
     BMPReaders m_bmpReaders;
     typedef Vector<OwnPtr<PNGImageDecoder>> PNGDecoders;
     PNGDecoders m_pngDecoders;
 
     // Valid only while a BMPImageReader is decoding, this holds the size
     // for the particular entry being decoded.
     IntSize m_frameSize;
 };
 
 } // namespace blink
 
 #endif