Eliminate copies of encoded image data

third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp

Index: third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
index 2672862d85e13b5b65612075ffccf1ef424f6463..fae908a78e567ec303b4b967a767735cffe0861c 100644
--- a/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
@@ -144,6 +144,7 @@ void WEBPImageDecoder::clear()
 {
     WebPDemuxDelete(m_demux);
     m_demux = 0;
+    m_consolidatedData.clear();
     clearDecoder();
 }
 
@@ -155,7 +156,7 @@ void WEBPImageDecoder::clearDecoder()
     m_frameBackgroundHasAlpha = false;
 }
 
-void WEBPImageDecoder::onSetData(SharedBuffer*)
+void WEBPImageDecoder::onSetData(SegmentReader*)
 {
     m_haveAlreadyParsedThisData = false;
 }
@@ -195,10 +196,14 @@ bool WEBPImageDecoder::updateDemuxer()
         return false; // Await VP8X header so WebPDemuxPartial succeeds.
 
     WebPDemuxDelete(m_demux);
-    WebPData inputData = { reinterpret_cast<const uint8_t*>(m_data->data()), m_data->size() };
+    m_consolidatedData = m_data->getAsSkData();

[urvang, 2016/03/28 21:29:32]

You need to clear existing data in m_consolidatedData before assigning new data
to it? Otherwise, you'll leak data on consecutive updateDemuxer() calls, IIUC.
[Note how the line above deletes m_demux object too before creating a new
m_demux object, similarly].

[scroggo_chromium, 2016/04/04 13:59:47]

m_demux is a raw pointer, so we definitely need to delete it before reassigning
to it. m_consolidatedData is a RefPtr<SkData>. As I understand it, assigning a
PassRefPtr (which is returned by getAsSkData) to it will clear the old value.
(To be more precise, based on RefPtr's operator=, which takes a RefPtr
parameter, the PassRefPtr will be implicitly converted to a RefPtr, which will
be swapped with the existing value. The anonymous RefPtr's destructor will unref
the previous SkData.) Am I missing something?

+    WebPData inputData = { reinterpret_cast<const uint8_t*>(m_consolidatedData->data()), m_consolidatedData->size() };
     m_demux = WebPDemuxPartial(&inputData, &m_demuxState);
-    if (!m_demux || (isAllDataReceived() && m_demuxState != WEBP_DEMUX_DONE))
+    if (!m_demux || (isAllDataReceived() && m_demuxState != WEBP_DEMUX_DONE)) {
+        if (!m_demux)
+            m_consolidatedData.clear();

[urvang, 2016/03/28 21:29:32]

Why this special clear() call? Wouldn't this automatically happen in the
WebPIMageDecoder destructor anyway?

[scroggo_chromium, 2016/04/04 13:59:47]

This was to tie the lifetime of m_consolidatedData to the lifetime of m_demux.
Here we failed to create m_demux, so I cleared m_consolidatedData. Yes,
m_consolidatedData will get cleared in the destructor, although as far as I can
tell, we will not necessarily call the destructor soon, just because the decoder
has failed. For example, it looks like the ImageFrameGenerator leaves a failed
decoder in the ImageDecodingStore [1]. I think the intent here is so that the
next call to decode still knows that it failed before. But it does not need
m_consolidatedData anymore (or m_demux, for that matter, but it only gets
cleared in the destructor or in one of the places where we call setFailed). As I
said, I tied the lifetime to the demuxer, but it seems like neither is needed
after failure. I'd be interested to delete m_demux when it's no longer needed,
but I think that should be an isolated change.

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

         return setFailed();
+    }
 
     ASSERT(m_demuxState > WEBP_DEMUX_PARSING_HEADER);
     if (!WebPDemuxGetI(m_demux, WEBP_FF_FRAME_COUNT))