Eliminate copies of encoded image data

third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
index 663f0d5d1ede08172707f54097054bba8349cca9..c19de44073751212521cb9bfaa9c7af08011c5d6 100644
--- a/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
@@ -32,67 +32,51 @@
 
 namespace blink {
 
-static size_t copyFromSharedBuffer(char* buffer, size_t bufferLength, const SharedBuffer& sharedBuffer, size_t offset)
-{
-    size_t bytesExtracted = 0;
-    const char* moreData;
-    while (size_t moreDataLength = sharedBuffer.getSomeData(moreData, offset)) {
-        size_t bytesToCopy = std::min(bufferLength - bytesExtracted, moreDataLength);
-        memcpy(buffer + bytesExtracted, moreData, bytesToCopy);
-        bytesExtracted += bytesToCopy;
-        if (bytesExtracted == bufferLength)
-            break;
-        offset += bytesToCopy;
-    }
-    return bytesExtracted;
-}
-
-inline bool matchesJPEGSignature(char* contents)
+inline bool matchesJPEGSignature(const char* contents)
 {
     return !memcmp(contents, "\xFF\xD8\xFF", 3);
 }
 
-inline bool matchesPNGSignature(char* contents)
+inline bool matchesPNGSignature(const char* contents)
 {
     return !memcmp(contents, "\x89\x50\x4E\x47\x0D\x0A\x1A\x0A", 8);
 }
 
-inline bool matchesGIFSignature(char* contents)
+inline bool matchesGIFSignature(const char* contents)
 {
     return !memcmp(contents, "GIF87a", 6) || !memcmp(contents, "GIF89a", 6);
 }
 
-inline bool matchesWebPSignature(char* contents)
+inline bool matchesWebPSignature(const char* contents)
 {
     return !memcmp(contents, "RIFF", 4) && !memcmp(contents + 8, "WEBPVP", 6);
 }
 
-inline bool matchesICOSignature(char* contents)
+inline bool matchesICOSignature(const char* contents)
 {
     return !memcmp(contents, "\x00\x00\x01\x00", 4);
 }
 
-inline bool matchesCURSignature(char* contents)
+inline bool matchesCURSignature(const char* contents)
 {
     return !memcmp(contents, "\x00\x00\x02\x00", 4);
 }
 
-inline bool matchesBMPSignature(char* contents)
+inline bool matchesBMPSignature(const char* contents)
 {
     return !memcmp(contents, "BM", 2);
 }
 
-PassOwnPtr<ImageDecoder> ImageDecoder::create(const SharedBuffer& data, AlphaOption alphaOption, GammaAndColorProfileOption colorOptions)
+PassOwnPtr<ImageDecoder> ImageDecoder::create(const char* contents, size_t length, AlphaOption alphaOption, GammaAndColorProfileOption colorOptions)
 {
     const size_t longestSignatureLength = sizeof("RIFF????WEBPVP") - 1;
     ASSERT(longestSignatureLength == 14);
 
-    size_t maxDecodedBytes = Platform::current() ? Platform::current()->maxDecodedImageBytes() : noDecodedImageByteLimit;
-
-    char contents[longestSignatureLength];
-    if (copyFromSharedBuffer(contents, longestSignatureLength, data, 0) < longestSignatureLength)
+    if (length < longestSignatureLength)
         return nullptr;
 
+    size_t maxDecodedBytes = Platform::current() ? Platform::current()->maxDecodedImageBytes() : noDecodedImageByteLimit;
+
     if (matchesJPEGSignature(contents))
         return adoptPtr(new JPEGImageDecoder(alphaOption, colorOptions, maxDecodedBytes));
 
@@ -114,6 +98,20 @@ PassOwnPtr<ImageDecoder> ImageDecoder::create(const SharedBuffer& data, AlphaOpt
     return nullptr;
 }
 
+PassOwnPtr<ImageDecoder> ImageDecoder::create(const SharedBuffer& data, AlphaOption alphaOption, GammaAndColorProfileOption colorOptions)
+{
+    const char* contents;
+    const size_t length = data.getSomeData(contents, static_cast<size_t>(0));

[Peter Kasting, 2016/03/25 06:24:37]

Nit: Should not need static_cast

[scroggo, 2016/03/25 15:49:53]

I was surprised by this one, too, but position is a STRICTLY_TYPED_ARG so I have
to use this cast. I've actually seen this elsewhere as a solution for using a
literal [1]. It seems like something isn't working though; it looks to have a
default argument, but calling data.getSomeData(contents) does not compile.

[1] e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Peter Kasting, 2016/03/25 21:15:56]

I think the one-arg version might compile if you do
getSomeData<size_t>(contents), since the macros expand to a templatized function
like this.

Also, I think if you change the body of getSomeData() to use
ALLOW_NUMERIC_ARG_TYPES_PROMOTABLE_TO() where it uses STRICT_ARG_TYPE(), you
could pass 0U here (maybe just 0, but I'm not sure).  Consider doing this as a
separate (possible precursor?) change and possibly also changing the other uses
of that macro in that file similarly.

Although, since this seems to be the only class in the codebase which uses these
macros, I think it would be even better to just rip these out entirely.  I doubt
they buy significant safety and it's not compelling to have them for just one of
the many classes which can take buffer/byte sizes.  This would also probably
eliminate the weirdness where the default arg doesn't work.

[scroggo_chromium, 2016/03/25 21:31:59]

FWIW, this macro was introduced fairly recently, in crrev.com/1571233003. In
that commit message junov states that it would be a giant patch to use it
everywhere, but this started where there was a problem.

If it's alright with you, I'll specify the template for now and not expand the
scope of this CL.

+    return create(contents, length, alphaOption, colorOptions);
+}
+
+PassOwnPtr<ImageDecoder> ImageDecoder::create(const SegmentReader& data, AlphaOption alphaOption, GammaAndColorProfileOption colorOptions)
+{
+    const char* contents;
+    const size_t length = data.getSomeData(contents, 0);
+    return create(contents, length, alphaOption, colorOptions);
+}
+
 size_t ImageDecoder::frameCount()
 {
     const size_t oldSize = m_frameBufferCache.size();