Eliminate copies of encoded image data

third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp

Index: third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp b/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
index 1411c494248a4a21170f245dc5e8aaa5899035ae..34f75aac43c2dbd8015b5cc8ebfc84af79bb260c 100644
--- a/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
@@ -26,10 +26,14 @@
 #include "platform/graphics/DeferredImageDecoder.h"
 
 #include "platform/RuntimeEnabledFeatures.h"
+#include "platform/SharedBuffer.h"
 #include "platform/graphics/DecodingImageGenerator.h"
 #include "platform/graphics/FrameData.h"
 #include "platform/graphics/ImageDecodingStore.h"
 #include "platform/graphics/ImageFrameGenerator.h"
+#include "platform/image-decoders/ROBufferSegmentReader.h"
+#include "platform/image-decoders/SharedBufferSegmentReader.h"
+#include "skia/ext/refptr.h"
 #include "third_party/skia/include/core/SkImage.h"
 #include "wtf/PassOwnPtr.h"
 
@@ -39,7 +43,10 @@ bool DeferredImageDecoder::s_enabled = true;
 
 PassOwnPtr<DeferredImageDecoder> DeferredImageDecoder::create(const SharedBuffer& data, ImageDecoder::AlphaOption alphaOption, ImageDecoder::GammaAndColorProfileOption colorOptions)
 {
-    OwnPtr<ImageDecoder> actualDecoder = ImageDecoder::create(data, alphaOption, colorOptions);
+    // FIXME: This const_cast is necessary because the SharedBufferSegmentReader expects a non-const, but
+    // it does not modify it.

[f(malita), 2016/03/23 16:41:57]

Since SharedBufferSegmentReader only calls const SharedBuffer methods, can it be
updated to take/store a const SharedBuffer (and then get rid of this cast)?

[scroggo_chromium, 2016/03/24 13:59:44]

I don't think so. ref() and deref() are not marked const, but it's kind of silly
that this even needs a SegmentReader, when it could easily use a SharedBuffer
directly. Updated so this is unnecessary.

+    RefPtr<SegmentReader> segmentReader = adoptRef(new SharedBufferSegmentReader(const_cast<SharedBuffer*>(&data)));
+    OwnPtr<ImageDecoder> actualDecoder = ImageDecoder::create(*segmentReader.get(), alphaOption, colorOptions);
 
     if (!actualDecoder)
         return nullptr;
@@ -54,7 +61,6 @@ PassOwnPtr<DeferredImageDecoder> DeferredImageDecoder::createForTesting(PassOwnP
 
 DeferredImageDecoder::DeferredImageDecoder(PassOwnPtr<ImageDecoder> actualDecoder)
     : m_allDataReceived(false)
-    , m_lastDataSize(0)
     , m_actualDecoder(std::move(actualDecoder))
     , m_repetitionCount(cAnimationNone)
     , m_hasColorProfile(false)
@@ -111,15 +117,22 @@ PassRefPtr<SkImage> DeferredImageDecoder::createFrameAtIndex(size_t index)
 void DeferredImageDecoder::setData(SharedBuffer& data, bool allDataReceived)
 {
     if (m_actualDecoder) {
-        m_data = RefPtr<SharedBuffer>(data);
-        m_lastDataSize = data.size();
+        RefPtr<SegmentReader> segmentReader = adoptRef(new SharedBufferSegmentReader(RefPtr<SharedBuffer>(data)));

[Peter Kasting, 2016/03/23 02:42:58]

Nit: Move down one line

[scroggo_chromium, 2016/03/24 13:59:44]

Done.

         m_allDataReceived = allDataReceived;
-        m_actualDecoder->setData(&data, allDataReceived);
+        m_actualDecoder->setData(segmentReader.get(), allDataReceived);

[Peter Kasting, 2016/03/23 02:42:58]

This pattern (create a local SharedBufferSegmentReader, pass to setData())
happens so many places in this CL that I think it may be worth adding a wrapper
function to ImageDecoder that takes a SharedBuffer directly.

[scroggo_chromium, 2016/03/24 13:59:44]

Agreed. Done.

         prepareLazyDecodedFrames();
     }
 
-    if (m_frameGenerator)
-        m_frameGenerator->setData(&data, allDataReceived);
+    if (m_frameGenerator) {
+        if (!m_rwBuffer) {

[Peter Kasting, 2016/03/23 02:42:58]

Nit: No {} (anywhere we have a one-line body)

[scroggo_chromium, 2016/03/24 13:59:44]

Done.

+            m_rwBuffer = adoptPtr(new SkRWBuffer(data.size()));
+        }
+
+        const char* segment = 0;
+        while (size_t length = data.getSomeData(segment, m_rwBuffer->size())) {

[scroggo_chromium, 2016/03/22 20:18:41]

This copy is analogous to the code in ThreadSafeDataTransport's setData (which
has been removed).

+            m_rwBuffer->append(segment, length);
+        }
+    }
 }
 
 bool DeferredImageDecoder::isSizeAvailable()
@@ -227,7 +240,8 @@ void DeferredImageDecoder::activateLazyDecoding()
     m_hasColorProfile = m_actualDecoder->hasColorProfile();
 
     const bool isSingleFrame = m_actualDecoder->repetitionCount() == cAnimationNone || (m_allDataReceived && m_actualDecoder->frameCount() == 1u);
-    m_frameGenerator = ImageFrameGenerator::create(SkISize::Make(m_actualDecoder->decodedSize().width(), m_actualDecoder->decodedSize().height()), m_data, m_allDataReceived, !isSingleFrame);
+    m_frameGenerator = ImageFrameGenerator::create(SkISize::Make(m_actualDecoder->decodedSize().width(), m_actualDecoder->decodedSize().height()),

[Peter Kasting, 2016/03/23 02:42:58]

Nit: Either put everything on one line, or linebreak in such a way that the args
start at the same horizontal position each line.

[scroggo_chromium, 2016/03/24 13:59:44]

I created a temporary variable, as is done with isSingleFrame, which I think
makes this more readable. (Blink's style guide does not mention anything about
lining up parameters, and I think I tried that, but got a presubmit error. I
think it suggested lining them all up 4 spaces from the first line.)

+        !isSingleFrame);
 }
 
 void DeferredImageDecoder::prepareLazyDecodedFrames()
@@ -264,7 +278,6 @@ void DeferredImageDecoder::prepareLazyDecodedFrames()
     if (m_allDataReceived) {
         m_repetitionCount = m_actualDecoder->repetitionCount();
         m_actualDecoder.clear();

[scroggo_chromium, 2016/03/22 20:18:42]

We hang on to the SkRWBuffer here, unlike the old code and its SharedBuffer,
since we may get more calls to createFrameAtIndex, which will take a snapshot
from the SkRWBuffer.

[Peter Kasting, 2016/03/23 02:42:58]

Might want to write a comment about that.

[scroggo_chromium, 2016/03/24 13:59:45]

Done.

-        m_data = nullptr;
     }
 }
 
@@ -279,7 +292,9 @@ PassRefPtr<SkImage> DeferredImageDecoder::createFrameImageAtIndex(size_t index,
     ASSERT(decodedSize.width() > 0);
     ASSERT(decodedSize.height() > 0);
 
-    DecodingImageGenerator* generator = new DecodingImageGenerator(m_frameGenerator, imageInfoFrom(decodedSize, knownToBeOpaque), index);
+    skia::RefPtr<SkROBuffer> roBuffer = skia::AdoptRef(m_rwBuffer->newRBufferSnapshot());

[scroggo_chromium, 2016/03/22 20:18:42]

Some of the Blink code that uses a Skia object wraps it in a skia::RefPtr, and
some uses a RefPtr. I'm not sure which is right (or if we should be using an
sk_sp, which did not exist when I started writing this code). Preferences?

[Peter Kasting, 2016/03/23 02:42:58]

Use skia::RefPtr for now, I think.

[f(malita), 2016/03/23 16:41:58]

skia::RefPtr is only used in Chromium code AFAIK.  In Blink we stick to native
RefPtr/PassRefptr/adoptRef which are compatible with SkRefCnt-derived objects.

My preference would be RefPtr<SkROBuffer> ... = adoptRef(...);

[scroggo_chromium, 2016/03/24 13:59:44]

Yes, it does appear skia::RefPtr is only used in Chromium. Done.

+    RefPtr<SegmentReader> segmentReader = adoptRef(new ROBufferSegmentReader(roBuffer));
+    DecodingImageGenerator* generator = new DecodingImageGenerator(m_frameGenerator, imageInfoFrom(decodedSize, knownToBeOpaque), segmentReader, m_allDataReceived, index);
     RefPtr<SkImage> image = adoptRef(SkImage::NewFromGenerator(generator)); // SkImage takes ownership of the generator.
     if (!image)
         return nullptr;