Eliminate copies of encoded image data

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

Index: third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp b/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
index 64d6223d6af84d4c2aaed13b95cd5ae9f5164a10..1c68086bf7700f6c7f1da84fcbfc3bb5388cf4af 100644
--- a/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/DecodingImageGenerator.cpp
@@ -30,13 +30,17 @@
 #include "platform/TraceEvent.h"
 #include "platform/graphics/ImageFrameGenerator.h"
 #include "platform/image-decoders/ImageDecoder.h"
+#include "platform/image-decoders/SkDataSegmentReader.h"
 #include "third_party/skia/include/core/SkData.h"
+#include "third_party/skia/src/core/SkRWBuffer.h"
 
 namespace blink {
 
-DecodingImageGenerator::DecodingImageGenerator(PassRefPtr<ImageFrameGenerator> frameGenerator, const SkImageInfo& info, size_t index)
+DecodingImageGenerator::DecodingImageGenerator(PassRefPtr<ImageFrameGenerator> frameGenerator, const SkImageInfo& info, PassRefPtr<SegmentReader> data, bool allDataReceived, size_t index)
     : SkImageGenerator(info)
     , m_frameGenerator(frameGenerator)
+    , m_data(data)
+    , m_allDataReceived(allDataReceived)
     , m_frameIndex(index)
     , m_generationId(0)
     , m_canYUVDecode(false)
@@ -51,7 +55,10 @@ SkData* DecodingImageGenerator::onRefEncodedData()
 {
     TRACE_EVENT0("blink", "DecodingImageGenerator::refEncodedData");
 
-    return m_frameGenerator->refEncodedData();
+    if (!m_allDataReceived)
+        return nullptr;
+
+    return m_data->getAsSkData().leakRef();

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

When combined with crrev.com/1567623002 (which first checks the first segment to
see if it can support it), this will mean we never copy the data here unless the
client knows it can support the contiguous buffer.

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

Can we pass this out as a smart object and thus avoid the leakRef() call?

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

This is part of the SkImageGenerator interface (called from Skia), and ATM only
exists in the raw ptr flavor.

As we're working on converting Skia to smart ptr based interfaces, I expect
we'll update it.

 }
 
 bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info, void* pixels, size_t rowBytes, SkPMColor table[], int* tableCount)
@@ -69,7 +76,7 @@ bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info, void* pixels,
     }
 
     PlatformInstrumentation::willDecodeLazyPixelRef(m_generationId);
-    bool decoded = m_frameGenerator->decodeAndScale(m_frameIndex, getInfo(), pixels, rowBytes);
+    bool decoded = m_frameGenerator->decodeAndScale(m_data.get(), m_allDataReceived, m_frameIndex, getInfo(), pixels, rowBytes);
     PlatformInstrumentation::didDecodeLazyPixelRef();
 
     return decoded;
@@ -77,7 +84,7 @@ bool DecodingImageGenerator::onGetPixels(const SkImageInfo& info, void* pixels,
 
 bool DecodingImageGenerator::onGetYUV8Planes(SkISize sizes[3], void* planes[3], size_t rowBytes[3], SkYUVColorSpace* colorSpace)
 {
-    if (!m_canYUVDecode)
+    if (!m_canYUVDecode || !m_allDataReceived)

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

Why is it important to not YUV decode when we don't have all the data?  I assume
JPEG is the only format we have that uses YUV; does it store e.g. the full
image's Y data, then the full U, then the full V?  (Even then I would imagine we
could partially decode once the V started to appear?)

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

I'm not very familiar with the YUV code, but there has been a FIXME [1] since it
was introduced [2]. It may be that we should support partial decoding, but that
would be outside the scope of this change.

The commit message does not provide any details, but I can hazard a guess why we
wouldn't support partial YUV - we support partial images by decoding into
ImageFrames. We would need to keep track of partial planes to pass for each
call. In addition, we'd have to have a way to tell the GPU to ignore the rows we
are not using yet.

So we just need to do more work to make it function.

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

[Peter Kasting, 2016/03/24 22:05:43]

Ah.  Maybe it makes sense to have a comment here saying something like "For now
YUV doesn't support progressive decoding, see comment in ...", and then perhaps
beef up the comment in the other place with a little more rationale on why this
isn't supported yet.

[scroggo_chromium, 2016/03/25 01:05:10]

Done.

         return false;
 
     bool requestingYUVSizes = !planes || !planes[0];
@@ -85,13 +92,13 @@ bool DecodingImageGenerator::onGetYUV8Planes(SkISize sizes[3], void* planes[3],
     TRACE_EVENT1("blink", "DecodingImageGenerator::getYUV8Planes", requestingYUVSizes ? "sizes" : "frame index", static_cast<int>(m_frameIndex));
 
     if (requestingYUVSizes)
-        return m_frameGenerator->getYUVComponentSizes(sizes);
+        return m_frameGenerator->getYUVComponentSizes(m_data.get(), sizes);
 
     if (colorSpace)
         *colorSpace = kJPEG_SkYUVColorSpace;
 
     PlatformInstrumentation::willDecodeLazyPixelRef(m_generationId);
-    bool decoded = m_frameGenerator->decodeToYUV(m_frameIndex, sizes, planes, rowBytes);
+    bool decoded = m_frameGenerator->decodeToYUV(m_data.get(), m_frameIndex, sizes, planes, rowBytes);
     PlatformInstrumentation::didDecodeLazyPixelRef();
 
     return decoded;
@@ -99,26 +106,28 @@ bool DecodingImageGenerator::onGetYUV8Planes(SkISize sizes[3], void* planes[3],
 
 SkImageGenerator* DecodingImageGenerator::create(SkData* data)

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

Not new to this CL, but I think this factory is transferring SkData ownership,
so it should ideally use a smart ptr.  Maybe we can look into that separately (I
think it'll also involve updating some Skia interfaces).

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

That's correct. This method gets passed to SetImageGeneratorFromEncodedFactory,
which currently takes a raw pointer.

 {
-    RefPtr<SharedBuffer> buffer = SharedBuffer::create(data->bytes(), data->size());
+    if (!data)
+        return 0;

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

Is there a particular reason this explicit check needed to be added?

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

I have not added any calls that might pass null, so it is not currently needed.
Removed.

 
+    RefPtr<SegmentReader> segmentReader = adoptRef(new SkDataSegmentReader(data));
     // We just need the size of the image, so we have to temporarily create an ImageDecoder. Since
     // we only need the size, it doesn't really matter about premul or not, or gamma settings.
-    OwnPtr<ImageDecoder> decoder = ImageDecoder::create(*buffer.get(), ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileApplied);
+    OwnPtr<ImageDecoder> decoder = ImageDecoder::create(*segmentReader.get(), ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileApplied);
     if (!decoder)
         return 0;
 
-    decoder->setData(buffer.get(), true);
+    decoder->setData(segmentReader.get(), true);
     if (!decoder->isSizeAvailable())
         return 0;
 
     const IntSize size = decoder->size();
     const SkImageInfo info = SkImageInfo::MakeN32Premul(size.width(), size.height());
 
-    RefPtr<ImageFrameGenerator> frame = ImageFrameGenerator::create(SkISize::Make(size.width(), size.height()), buffer, true, false);
+    RefPtr<ImageFrameGenerator> frame = ImageFrameGenerator::create(SkISize::Make(size.width(), size.height()), false);
     if (!frame)
         return 0;
 
-    return new DecodingImageGenerator(frame, info, 0);
+    return new DecodingImageGenerator(frame, info, segmentReader, true, 0);

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

segmentReader.release()

(to minimize refcount churn, since we're done with the local)

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

Done.

 }
 
 } // namespace blink