Do not consolidate data in BMPImageDecoder

Source/platform/image-decoders/bmp/BMPImageReader.h

Index: Source/platform/image-decoders/bmp/BMPImageReader.h
diff --git a/Source/platform/image-decoders/bmp/BMPImageReader.h b/Source/platform/image-decoders/bmp/BMPImageReader.h
index 83d70caf4a5bc5cbc5559ae760cc74958289e1f1..ef309e62b83341d15fa6f45517cf4b7738e1f74e 100644
--- a/Source/platform/image-decoders/bmp/BMPImageReader.h
+++ b/Source/platform/image-decoders/bmp/BMPImageReader.h
@@ -31,9 +31,10 @@
 #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 {
 
@@ -42,22 +43,22 @@ namespace blink {
 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
@@ -124,14 +125,18 @@ private:
         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
@@ -196,19 +201,22 @@ private:
     // 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
@@ -216,7 +224,7 @@ private:
         }
 
         case 4:
-            return readUint32(offset);
+            return readUint32(encodedPixel);
 
         default:
             ASSERT_NOT_REACHED();
@@ -238,6 +246,19 @@ private:
         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.