Eliminate copies of encoded image data

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

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 12 matching lines @@
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "platform/graphics/DecodingImageGenerator.h"
 
 #include "platform/PlatformInstrumentation.h"
 #include "platform/SharedBuffer.h"
 #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)
 {
 }
 
 DecodingImageGenerator::~DecodingImageGenerator()
 {
 }
 
 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)
 {
     TRACE_EVENT1("blink", "DecodingImageGenerator::getPixels", "frame index", static_cast<int>(m_frameIndex));
 
     // Implementation doesn't support scaling yet so make sure we're not given a different size.
     if (info.width() != getInfo().width() || info.height() != getInfo().height())
         return false;
 
     if (info.colorType() != getInfo().colorType()) {
         // blink::ImageFrame may have changed the owning SkBitmap to kOpaque_SkAlphaType after fully decoding the image frame,
         // so if we see a request for opaque, that is ok even if our initial alpha type was not opaque.
         return false;
     }
 
     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;
 }
 
 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];
 
     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;
 }
 
 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