Do not consolidate data in BMPImageDecoder

Source/platform/image-decoders/bmp/BMPImageReader.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 BMPImageReader_h
 #define BMPImageReader_h
 
-#include <stdint.h>
+#include "platform/image-decoders/FastSharedBufferReader.h"
 #include "platform/image-decoders/ImageDecoder.h"
 #include "wtf/CPU.h"
+#include <stdint.h>

[Peter Kasting, 2015/08/31 19:53:42]

Why was this header moved down?  I think it should stay at the top of the list.

[scroggo_chromium, 2015/09/01 22:50:30]

The style guide disagrees
(https://www.chromium.org/blink/coding-style#TOC-include-Statements):
Includes of system headers must come after includes of other headers. This
happens to also be checked and ensured by alphabetical order, as < comes after
".

[Peter Kasting, 2015/09/02 22:08:54]

It is SO DUMB that Blink has its own style guide that flatly contradicts the
Chromium and Google ones.  Ugh.

 
 namespace blink {
 
 // This class decodes a BMP image.  It is used in the BMP and ICO decoders,
 // which wrap it in the appropriate code to read file headers, etc.
 class PLATFORM_EXPORT BMPImageReader {
     WTF_MAKE_FAST_ALLOCATED(BMPImageReader);
 public:
-    // Read a value from |data[offset]|, converting from little to native
+    // Read a value from |buffer|, converting from little to native
     // endianness.

[Peter Kasting, 2015/08/31 19:53:42]

Do we actually compile on any big-endian targets?  Maybe MIPS?

I feel like much of the rest of Chromium is little-endian-only, and if so maybe
we can just simplify these and avoid some small static object allocations in
various places.

[scroggo_chromium, 2015/09/01 22:50:30]

According to [1], you are correct!

From that same thread, it sounds like there at least was a planned big-endian
backend for V8, and Skia half-heartedly supports big-endian ([2]).

I thought maybe it should be a different CL, but I can make a small change here.
If we're wrong, or someone decides to add support, it should be easy to undo.
Are you talking about |result|? I would think this would be an automatic
allocation? Either way, removing the support for BIG_ENDIAN does eliminate some
allocations. It also removes some memcpys, which is the spirit of this CL, so it
seems like a fine change. (To be fair, this operation could be done without any
copying in the LITTLE_ENDIAN case, if we still wanted to support both.)

[1]
https://groups.google.com/a/google.com/forum/#!msg/chrome-team/vzRPeX7w6WQ/AS...
[2]
https://groups.google.com/a/google.com/forum/#!msg/skia-dev/EdzJnynHnzY/AoQ8U...

-    static inline uint16_t readUint16(SharedBuffer* data, int offset)
+    static inline uint16_t readUint16(const char* buffer)
     {
         uint16_t result;
-        memcpy(&result, &data->data()[offset], 2);
+        memcpy(&result, buffer, 2);
     #if CPU(BIG_ENDIAN)
         result = ((result & 0xff) << 8) | ((result & 0xff00) >> 8);
     #endif
         return result;
     }
 
-    static inline uint32_t readUint32(SharedBuffer* data, int offset)
+    static inline uint32_t readUint32(const char* buffer)
     {
         uint32_t result;
-        memcpy(&result, &data->data()[offset], 4);
+        memcpy(&result, buffer, 4);
     #if CPU(BIG_ENDIAN)
         result = ((result & 0xff) << 24) | ((result & 0xff00) << 8) | ((result & 0xff0000) >> 8) | ((result & 0xff000000) >> 24);
     #endif
         return result;
     }
 
     // |parent| is the decoder that owns us.
     // |startOffset| points to the start of the BMP within the file.
     // |buffer| points at an empty ImageFrame that we'll initialize and
     // fill with decoded data.
@@ skipping 46 matching lines @@
         uint16_t biBitCount;
         CompressionType biCompression;
         uint32_t biClrUsed;
     };
     struct RGBTriple {
         uint8_t rgbBlue;
         uint8_t rgbGreen;
         uint8_t rgbRed;
     };
 
-    inline uint16_t readUint16(int offset) const
+    inline uint16_t readUint16(FastSharedBufferReader* reader, int offset) const
     {
-        return readUint16(m_data.get(), m_decodedOffset + offset);
+        char buffer[2];
+        const char* data = reader->getConsecutiveData(m_decodedOffset + offset, 2, buffer);
+        return readUint16(data);
     }
 
-    inline uint32_t readUint32(int offset) const
+    inline uint32_t readUint32(FastSharedBufferReader* reader, int offset) const
     {
-        return readUint32(m_data.get(), m_decodedOffset + offset);
+        char buffer[4];
+        const char* data = reader->getConsecutiveData(m_decodedOffset + offset, 4, buffer);
+        return readUint32(data);
     }
 
     // Determines the size of the BMP info header.  Returns true if the size
     // is valid.
     bool readInfoHeaderSize();
 
     // Processes the BMP info header.  Returns true if the info header could
     // be decoded.
     bool processInfoHeader();
 
@@ skipping 44 matching lines @@
     inline bool pastEndOfImage(int numRows)
     {
         return m_isTopDown ? ((m_coord.y() + numRows) >= m_parent->size().height()) : ((m_coord.y() - numRows) < 0);
     }
 
     // Returns the pixel data for the current X coordinate in a uint32_t.
     // Assumes m_decodedOffset has been set to the beginning of the current
     // row.
     // NOTE: Only as many bytes of the return value as are needed to hold
     // the pixel data will actually be set.
-    inline uint32_t readCurrentPixel(int bytesPerPixel) const
+    inline uint32_t readCurrentPixel(FastSharedBufferReader* fastReader, int bytesPerPixel) const

[scroggo_chromium, 2015/08/31 19:07:00]

This seems like an odd API to me - initially I created the
FastSharedBufferReader inside readCurrentPixel, but that seemed unfortunate to
me, since we would be unnecessarily creating a new one for each pixel on a line.

We could take it a step further, and keep a member variable of
FastSharedBufferReader. We would need to update it each time setData is called,
but that might be fine. Thoughts?

[Peter Kasting, 2015/08/31 19:53:42]

I think it would make sense to have a member FastSharedBufferReader either on
this class, or else on the owning decoder (BMP or ICO) which this class would
have access to somehow.  (The advantage of this latter idea is that the owning
decoder is what gets the setData update notifications first, so we could maybe
remove setData or even the whole concept of m_data from the reader; and once the
owning decoder has the FastSharedBufferReader, it can use it in the place you've
changed the BMPImageDecoder to create one, as well as in the ICO decoder where
you added your TODO.)

[scroggo_chromium, 2015/09/01 22:50:30]

I've made the FastSharedBufferReader to the BMPImageReader. I did not
investigate moving it to the ImageDecoder. I tend to see the SharedBuffer as
belonging to the reader, rather than the ImageDecoder, which mostly does not use
it. Either way, I think that belongs in its own CL.

FWIW, I found the version with a single FastSharedBufferReader to be a little
bit slower than the version that puts it on the stack for decoding in 1357 byte
chunks. (It is still faster than the original version; see sheet 3 of the
spreadsheet in the description.) I do not know why it would be slower. It may be
noise, or maybe I've failed to consider something.

     {
+        // We need at most 4 bytes, starting at m_decodedOffset + offset.
+        char buffer[4];
         const int offset = m_coord.x() * bytesPerPixel;
+        const char* encodedPixel = fastReader->getConsecutiveData(m_decodedOffset + offset, bytesPerPixel, buffer);
         switch (bytesPerPixel) {
         case 2:
-            return readUint16(offset);
+            return readUint16(encodedPixel);
 
         case 3: {
             // It doesn't matter that we never set the most significant byte
             // of the return value here in little-endian mode, the caller
             // won't read it.
             uint32_t pixel;
-            memcpy(&pixel, &m_data->data()[m_decodedOffset + offset], 3);
+            memcpy(&pixel, encodedPixel, 3);
     #if CPU(BIG_ENDIAN)
             pixel = ((pixel & 0xff00) << 8) | ((pixel & 0xff0000) >> 8) | ((pixel & 0xff000000) >> 24);
     #endif
             return pixel;
         }
 
         case 4:
-            return readUint32(offset);
+            return readUint32(encodedPixel);
 
         default:
             ASSERT_NOT_REACHED();
             return 0;
         }
     }
 
     // Returns the value of the desired component (0, 1, 2, 3 == R, G, B, A)
     // in the given pixel data.
     inline unsigned getComponent(uint32_t pixel, int component) const
     {
         uint8_t value = (pixel & m_bitMasks[component]) >> m_bitShiftsRight[component];
         return m_lookupTableAddresses[component] ? m_lookupTableAddresses[component][value] : value;
     }
 
     inline unsigned getAlpha(uint32_t pixel) const
     {
         // For images without alpha, return alpha of 0xff.
         return m_bitMasks[3] ? getComponent(pixel, 3) : 0xff;
     }
 
+    // Retrieve one byte from |m_data| at |offset|
+    uint8_t getByte(size_t offset)
+    {
+        const char* segment;
+#if ENABLE(ASSERT)
+        unsigned bytes =

[Peter Kasting, 2015/08/31 19:53:42]

If you really want this assert, I'd do this:

#if ENABLE(ASSERT)
        unsigned bytes = m_data->getSomeData(segment, offset);
        ASSERT(bytes >= 1);
#else
        m_data->getSomeData(segment, offset);
#endif

I think this is a bit more readable and hard to accidentally break in the
future.

Otherwise, I would just remove the assert.  I don't know if it buys much.

[scroggo_chromium, 2015/09/01 22:50:30]

Removed.

+#endif
+        m_data->getSomeData(segment, offset);

[scroggo_chromium, 2015/08/31 19:07:00]

I think there might be a tradeoff between speed and simplicity. This is
typically called multiple times in a row. We could stick with the same segment
until it is exhausted (rather than calling getSomeData over and over again), but
that would result in more complicated code. (And maybe that code would be no
faster than the checks getSomeData already does.)

The FastSharedBufferReader caches the last segment accessed - maybe that would
make this faster. We would need to pass the FastSharedBufferReader to this
function (which seems strange, since we don't take advantage of what I see as
its primary purpose - consolidating data across segment boundaries), unless we
made a FastSharedBufferReader a member of the class.

[Peter Kasting, 2015/08/31 19:53:42]

It does kind of bug me that we have a method like this, since I feel like even
with caching, this is slower, and caller code already does something like what
we need by checking the length of m_data before reading.

That said, I think changing that calling code to try to do the right thing here
would actually be fairly tough to do without introducing unnecessary data
compaction.  So I don't know that we can practically do much better than what
you've written.  Therefore, I would probably just pursue the "member
FastSharedBufferReader" idea as discussed above.

+        ASSERT(bytes >= 1);
+        return segment[0];
+    }
+
+
     // Sets the current pixel to the color given by |colorIndex|.  This also
     // increments the relevant local variables to move the current pixel
     // right by one.
     inline void setI(size_t colorIndex)
     {
         setRGBA(m_colorTable[colorIndex].rgbRed, m_colorTable[colorIndex].rgbGreen, m_colorTable[colorIndex].rgbBlue, 0xff);
     }
 
     // Like setI(), but with the individual component values specified.
     inline void setRGBA(unsigned red,
@@ skipping 102 matching lines @@
     // ICOs store a 1bpp "mask" immediately after the main bitmap image data
     // (and, confusingly, add its height to the biHeight value in the info
     // header, thus doubling it). If |m_isInICO| is true, this variable tracks
     // whether we've begun decoding this mask yet.
     bool m_decodingAndMask;
 };
 
 } // namespace blink
 
 #endif