Eliminate copies of encoded image data

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

Index: third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp b/third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
index 3ea9aa0300bc6dd4d3d38fe71a80f148f303e972..eb3b3734138cb82e765fcb04863acb70529126eb 100644
--- a/third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
@@ -29,6 +29,7 @@
  */
 
 #include "platform/image-decoders/FastSharedBufferReader.h"
+#include "platform/image-decoders/SegmentReader.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -44,6 +45,34 @@ void prepareReferenceData(char* buffer, size_t size)
         buffer[i] = static_cast<char>(i);
 }
 
+PassRefPtr<SegmentReader> copyToROBufferSegmentReader(PassRefPtr<SegmentReader> input)
+{
+    SkRWBuffer rwBuffer;
+    const char* segment = 0;
+    size_t position = 0;
+    while (size_t length = input->getSomeData(segment, position)) {
+        rwBuffer.append(segment, length);
+        position += length;
+    }
+    return SegmentReader::createFromSkROBuffer(adoptRef(rwBuffer.newRBufferSnapshot()));
+}
+
+PassRefPtr<SegmentReader> copyToDataSegmentReader(PassRefPtr<SegmentReader> input)
+{
+    return SegmentReader::createFromSkData(input->getAsSkData());
+}
+
+struct SegmentReaders {
+    RefPtr<SegmentReader> segmentReaders[3];
+};
+
+void createReaders(SegmentReaders* s, PassRefPtr<SharedBuffer> input)

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

Nit: Why not use a constructor for SegmentReaders (that takes |input|) instead?

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

Agreed, that is much better.

Done.

+{
+    s->segmentReaders[0] = SegmentReader::createFromSharedBuffer(input);
+    s->segmentReaders[1] = copyToROBufferSegmentReader(s->segmentReaders[0]);
+    s->segmentReaders[2] = copyToDataSegmentReader(s->segmentReaders[0]);
+}
+
 } // namespace
 
 TEST(FastSharedBufferReaderTest, nonSequentialReads)
@@ -53,15 +82,18 @@ TEST(FastSharedBufferReaderTest, nonSequentialReads)
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, sizeof(referenceData));
 
-    FastSharedBufferReader reader(data);
-
-    // Read size is prime such there will be a segment-spanning
-    // read eventually.
-    char tempBuffer[17];
-    for (size_t dataPosition = 0; dataPosition + sizeof(tempBuffer) < sizeof(referenceData); dataPosition += sizeof(tempBuffer)) {
-        const char* block = reader.getConsecutiveData(
-            dataPosition, sizeof(tempBuffer), tempBuffer);
-        ASSERT_FALSE(memcmp(block, referenceData + dataPosition, sizeof(tempBuffer)));
+    SegmentReaders readerStruct;
+    createReaders(&readerStruct, data);
+    for (auto segmentReader : readerStruct.segmentReaders) {
+        FastSharedBufferReader reader(segmentReader);
+        // Read size is prime such there will be a segment-spanning
+        // read eventually.
+        char tempBuffer[17];
+        for (size_t dataPosition = 0; dataPosition + sizeof(tempBuffer) < sizeof(referenceData); dataPosition += sizeof(tempBuffer)) {
+            const char* block = reader.getConsecutiveData(
+                dataPosition, sizeof(tempBuffer), tempBuffer);
+            ASSERT_FALSE(memcmp(block, referenceData + dataPosition, sizeof(tempBuffer)));
+        }
     }
 }
 
@@ -72,15 +104,18 @@ TEST(FastSharedBufferReaderTest, readBackwards)
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, sizeof(referenceData));
 
-    FastSharedBufferReader reader(data);
-
-    // Read size is prime such there will be a segment-spanning
-    // read eventually.
-    char tempBuffer[17];
-    for (size_t dataOffset = sizeof(tempBuffer); dataOffset < sizeof(referenceData); dataOffset += sizeof(tempBuffer)) {
-        const char* block = reader.getConsecutiveData(
-            sizeof(referenceData) - dataOffset, sizeof(tempBuffer), tempBuffer);
-        ASSERT_FALSE(memcmp(block, referenceData + sizeof(referenceData) - dataOffset, sizeof(tempBuffer)));
+    SegmentReaders readerStruct;
+    createReaders(&readerStruct, data);
+    for (auto segmentReader : readerStruct.segmentReaders) {
+        FastSharedBufferReader reader(segmentReader);
+        // Read size is prime such there will be a segment-spanning
+        // read eventually.
+        char tempBuffer[17];
+        for (size_t dataOffset = sizeof(tempBuffer); dataOffset < sizeof(referenceData); dataOffset += sizeof(tempBuffer)) {
+            const char* block = reader.getConsecutiveData(
+                sizeof(referenceData) - dataOffset, sizeof(tempBuffer), tempBuffer);
+            ASSERT_FALSE(memcmp(block, referenceData + sizeof(referenceData) - dataOffset, sizeof(tempBuffer)));
+        }
     }
 }
 
@@ -91,9 +126,13 @@ TEST(FastSharedBufferReaderTest, byteByByte)
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, sizeof(referenceData));
 
-    FastSharedBufferReader reader(data);
-    for (size_t i = 0; i < sizeof(referenceData); ++i) {
-        ASSERT_EQ(referenceData[i], reader.getOneByte(i));
+    SegmentReaders readerStruct;
+    createReaders(&readerStruct, data);
+    for (auto segmentReader : readerStruct.segmentReaders) {
+        FastSharedBufferReader reader(segmentReader);
+        for (size_t i = 0; i < sizeof(referenceData); ++i) {
+            ASSERT_EQ(referenceData[i], reader.getOneByte(i));
+        }
     }
 }
 
@@ -107,11 +146,35 @@ TEST(FastSharedBufferReaderTest, readAllOverlappingLastSegmentBoundary)
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, dataSize);
 
-    char buffer[dataSize];
-    FastSharedBufferReader reader(data);
-    reader.getConsecutiveData(0, dataSize, buffer);
+    SegmentReaders readerStruct;
+    createReaders(&readerStruct, data);
+    for (auto segmentReader : readerStruct.segmentReaders) {
+        FastSharedBufferReader reader(segmentReader);
+        char buffer[dataSize];
+        reader.getConsecutiveData(0, dataSize, buffer);
+        ASSERT_FALSE(memcmp(buffer, referenceData, dataSize));
+    }
+}
 
-    ASSERT_FALSE(memcmp(buffer, referenceData, dataSize));
+// Verify that reading past the end of the buffer does not break future reads.
+TEST(SegmentReaderTest, readPastEndThenRead)
+{
+    const unsigned dataSize = SharedBuffer::kSegmentSize;

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

Should this test test a buffer with more than one segment in it?

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

My goal in this test was to test reading past the end, which is independent of
the number of segments. But I'm fine with using more than one segment.

+    char referenceData[dataSize];
+    prepareReferenceData(referenceData, dataSize);
+    RefPtr<SharedBuffer> data = SharedBuffer::create();
+    data->append(referenceData, dataSize);
+
+    SegmentReaders readerStruct;
+    createReaders(&readerStruct, data);
+    for (auto segmentReader : readerStruct.segmentReaders) {
+        const char* contents;
+        size_t length = segmentReader->getSomeData(contents, dataSize);
+        EXPECT_EQ(length, 0u);

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

Nit: (expected, actual) (2 places)

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

Done.

+
+        length = segmentReader->getSomeData(contents, static_cast<size_t>(0));

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

Nit: Shouldn't need to static_cast here.

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

Done.

+        EXPECT_EQ(length, dataSize);
+    }
 }
 
 } // namespace blink