Eliminate copies of encoded image data

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

Index: third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
index 09f931bb74b691ffd32a4543fee80a1a121818ab..8a774c003ca276855d301436228f92018a7f9a15 100644
--- a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
@@ -26,7 +26,6 @@
 #include "platform/graphics/ImageFrameGenerator.h"
 
 #include "SkData.h"
-#include "platform/SharedBuffer.h"
 #include "platform/TraceEvent.h"
 #include "platform/graphics/ImageDecodingStore.h"
 #include "platform/image-decoders/ImageDecoder.h"
@@ -98,104 +97,36 @@ static bool updateYUVComponentSizes(ImageDecoder* decoder, SkISize componentSize
     return true;
 }
 
-ImageFrameGenerator::ImageFrameGenerator(const SkISize& fullSize, PassRefPtr<SharedBuffer> data, bool allDataReceived, bool isMultiFrame)
+ImageFrameGenerator::ImageFrameGenerator(const SkISize& fullSize, bool isMultiFrame)
     : m_fullSize(fullSize)
-    , m_data(adoptRef(new ThreadSafeDataTransport()))
     , m_isMultiFrame(isMultiFrame)
     , m_decodeFailed(false)
     , m_frameCount(0)
-    , m_encodedData(nullptr)
 {
-    setData(data.get(), allDataReceived);
 }
 
 ImageFrameGenerator::~ImageFrameGenerator()
 {
-    if (m_encodedData)
-        m_encodedData->unref();
     ImageDecodingStore::instance().removeCacheIndexedByGenerator(this);
 }
 
-void ImageFrameGenerator::setData(PassRefPtr<SharedBuffer> data, bool allDataReceived)
+bool ImageFrameGenerator::decodeAndScale(SegmentReader* data, bool allDataReceived, size_t index, const SkImageInfo& info, void* pixels, size_t rowBytes)
 {
-    m_data->setData(data.get(), allDataReceived);
-}
-
-static void sharedSkDataReleaseCallback(const void* address, void* context)
-{
-    // This gets called when m_encodedData reference count becomes 0 - and it could happen in
-    // ImageFrameGenerator destructor or later when m_encodedData gets dereferenced.
-    // In this method, we deref ThreadSafeDataTransport, as ThreadSafeDataTransport is the owner
-    // of data returned via refEncodedData.
-
-    ThreadSafeDataTransport* dataTransport = static_cast<ThreadSafeDataTransport*>(context);
-#if ENABLE(ASSERT)
-    ASSERT(dataTransport);
-    SharedBuffer* buffer = 0;
-    bool allDataReceived = false;
-    dataTransport->data(&buffer, &allDataReceived);
-    ASSERT(allDataReceived && buffer && buffer->data() == address);
-#endif
-    // Dereference m_data now.
-    dataTransport->deref();
-}
-
-SkData* ImageFrameGenerator::refEncodedData()
-{
-    // SkData is returned only when full image (encoded) data is received. This is important
-    // since DeferredImageDecoder::setData is called only once with allDataReceived set to true,
-    // and after that m_data->m_readBuffer.data() is not changed. See also RELEASE_ASSERT used in
-    // ThreadSafeDataTransport::data().
-    SharedBuffer* buffer = 0;
-    bool allDataReceived = false;
-    m_data->data(&buffer, &allDataReceived);
-    if (!allDataReceived)
-        return nullptr;
-
-    {
-        // Prevents concurrent access to m_encodedData creation.
-        MutexLocker lock(m_decodeMutex);
-        if (m_encodedData) {
-            m_encodedData->ref();
-            return m_encodedData;
-        }
-        // m_encodedData is created with initial reference count == 1. ImageFrameGenerator always holds one
-        // reference to m_encodedData, as it prevents write access in SkData::writable_data.
-        m_encodedData = SkData::NewWithProc(buffer->data(), buffer->size(), sharedSkDataReleaseCallback, m_data.get());
-        // While m_encodedData is referenced, prevent disposing m_data and its content.
-        // it is dereferenced in sharedSkDataReleaseCallback, called when m_encodedData gets dereferenced.
-        m_data->ref();
-    }
-    // Increase the reference, caller must decrease it. One reference is always kept by ImageFrameGenerator and released
-    // in destructor.
-    m_encodedData->ref();
-    return m_encodedData;
-}
-
-bool ImageFrameGenerator::decodeAndScale(size_t index, const SkImageInfo& info, void* pixels, size_t rowBytes)
-{
-    // Prevent concurrent decode or scale operations on the same image data.
-    MutexLocker lock(m_decodeMutex);
-
     if (m_decodeFailed)
         return false;
 
     TRACE_EVENT1("blink", "ImageFrameGenerator::decodeAndScale", "frame index", static_cast<int>(index));
 
-    m_externalAllocator = adoptPtr(new ExternalMemoryAllocator(info, pixels, rowBytes));
+    ExternalMemoryAllocator externalAllocator(info, pixels, rowBytes);
 
     // This implementation does not support scaling so check the requested size.
     SkISize scaledSize = SkISize::Make(info.width(), info.height());
     ASSERT(m_fullSize == scaledSize);
 
-    SkBitmap bitmap = tryToResumeDecode(index, scaledSize);
+    SkBitmap bitmap = tryToResumeDecode(data, allDataReceived, index, scaledSize, &externalAllocator);
     if (bitmap.isNull())
         return false;
 
-    // Don't keep the allocator because it contains a pointer to memory
-    // that we do not own.
-    m_externalAllocator.clear();
-
     // Check to see if the decoder has written directly to the pixel memory
     // provided. If not, make a copy.
     ASSERT(bitmap.width() == scaledSize.width());
@@ -206,11 +137,10 @@ bool ImageFrameGenerator::decodeAndScale(size_t index, const SkImageInfo& info,
     return true;
 }
 
-bool ImageFrameGenerator::decodeToYUV(size_t index, SkISize componentSizes[3], void* planes[3], size_t rowBytes[3])
+bool ImageFrameGenerator::decodeToYUV(SegmentReader* data, size_t index, SkISize componentSizes[3], void* planes[3], size_t rowBytes[3])
 {
-    // Prevent concurrent decode or scale operations on the same image data.
-    MutexLocker lock(m_decodeMutex);
-
+    // FIXME: The only interesting thing this uses from the ImageFrameGenerator is m_decodeFailed.
+    // Should we move this into DecodingImageGenerator, which is the only class that calls it?

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

I don't know.  I don't know much about YUV decoding.  I would likely make the
decision based on which route wound up with simpler code.

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

I think moving it into DecodingImageGenerator is simpler. I made this a TODO and
plan to follow up in another CL.

     if (m_decodeFailed)
         return false;
 
@@ -221,18 +151,11 @@ bool ImageFrameGenerator::decodeToYUV(size_t index, SkISize componentSizes[3], v
         return false;
     }
 
-    SharedBuffer* data = 0;
-    bool allDataReceived = false;
-    m_data->data(&data, &allDataReceived);
-
-    // FIXME: YUV decoding does not currently support progressive decoding.
-    ASSERT(allDataReceived);
-
     OwnPtr<ImageDecoder> decoder = ImageDecoder::create(*data, ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileApplied);
-    if (!decoder)
-        return false;
+    // getYUVComponentSizes was already called and was successful, so ImageDecoder::create must succeed.
+    ASSERT(decoder);
 
-    decoder->setData(data, allDataReceived);
+    decoder->setData(data, true);
 
     OwnPtr<ImagePlanes> imagePlanes = adoptPtr(new ImagePlanes(planes, rowBytes));
     decoder->setImagePlanes(imagePlanes.release());
@@ -250,16 +173,19 @@ bool ImageFrameGenerator::decodeToYUV(size_t index, SkISize componentSizes[3], v
     return false;
 }
 
-SkBitmap ImageFrameGenerator::tryToResumeDecode(size_t index, const SkISize& scaledSize)
+SkBitmap ImageFrameGenerator::tryToResumeDecode(SegmentReader* data, bool allDataReceived, size_t index, const SkISize& scaledSize, ExternalMemoryAllocator* externalAllocator)
 {
     TRACE_EVENT1("blink", "ImageFrameGenerator::tryToResumeDecode", "frame index", static_cast<int>(index));
 
     ImageDecoder* decoder = 0;
+
+    // Lock the mutex, so only one thread can use the decoder at once.

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

Do we have to lock for the whole rest of this function?  That's a lot of code.

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

I agree that this is a lot of code to keep the mutex locked. We used to lock for
longer, though. In the old code, we locked in decodeAndScale, which calls this
method and then potentially copies the entire bitmap, both while the mutex is
locked.

I actually considered whether we could avoid locking the mutex here. The
ImageDecoderStore, which provides an ImageDecoder in lockDecoder(), expects that
we'll only use that decoder in one thread at a time, and this ensures that. We
could rewrite ImageDecodingStore to detach the decoder and require us to put it
back when we're done, so another call will get a new ImageDecoder if we're still
using the first one. This would remove the need for this mutex.

But I'm not sure that's the right thing to do. Using the same ImageDecoder
(which happens now if we leave the mutex lock where it was before my CL, or
where I've move it to here) means that the second user of the same ImageDecoder
will not need to do any/as much work, since the ImageDecoder already decoded
some or all of the image (into its cache, which will be reused the second time).

There is an exception - if both calls are decoding directly into the output
memory we might as well use two different decoders, since that skips the
ImageDecoder's internal cache. We do not know that yet, though. That will only
happen if allDataReceived is true and lockDecoder() does not have a decoder. So
we *could* unlock the mutex after calling lockDecoder() if it does not give us a
decoder. (If we pursue this, we should isolate it to a follow up CL.) On the
other hand, that still means that we are wasting work, since we could've just
waited and used the same SkImage. vmpstr is making changes in the compositor so
that multiple tiles using the same SkImage will all wait until the image is
decoded, so this shouldn't be a problem.

+    MutexLocker lock(m_decodeMutex);
     const bool resumeDecoding = ImageDecodingStore::instance().lockDecoder(this, m_fullSize, &decoder);
     ASSERT(!resumeDecoding || decoder);
 
     SkBitmap fullSizeImage;
-    bool complete = decode(index, &decoder, &fullSizeImage);
+    bool complete = decode(data, allDataReceived, index, &decoder, &fullSizeImage, externalAllocator);
 
     if (!decoder)
         return SkBitmap();
@@ -324,14 +250,10 @@ void ImageFrameGenerator::setHasAlpha(size_t index, bool hasAlpha)
     m_hasAlpha[index] = hasAlpha;
 }
 
-bool ImageFrameGenerator::decode(size_t index, ImageDecoder** decoder, SkBitmap* bitmap)
+bool ImageFrameGenerator::decode(SegmentReader* data, bool allDataReceived, size_t index, ImageDecoder** decoder, SkBitmap* bitmap, ExternalMemoryAllocator* externalAllocator)
 {
     TRACE_EVENT2("blink", "ImageFrameGenerator::decode", "width", m_fullSize.width(), "height", m_fullSize.height());
 
-    SharedBuffer* data = 0;
-    bool allDataReceived = false;
-    m_data->data(&data, &allDataReceived);
-
     // Try to create an ImageDecoder if we are not given one.
     ASSERT(decoder);
     bool newDecoder = false;
@@ -350,8 +272,8 @@ bool ImageFrameGenerator::decode(size_t index, ImageDecoder** decoder, SkBitmap*
     if (!m_isMultiFrame && newDecoder && allDataReceived) {
         // If we're using an external memory allocator that means we're decoding
         // directly into the output memory and we can save one memcpy.
-        ASSERT(m_externalAllocator.get());
-        (*decoder)->setMemoryAllocator(m_externalAllocator.get());
+        ASSERT(externalAllocator);
+        (*decoder)->setMemoryAllocator(externalAllocator);
     }
 
     (*decoder)->setData(data, allDataReceived);
@@ -364,7 +286,7 @@ bool ImageFrameGenerator::decode(size_t index, ImageDecoder** decoder, SkBitmap*
     if (allDataReceived)
         m_frameCount = (*decoder)->frameCount();
 
-    (*decoder)->setData(0, false); // Unref SharedBuffer from ImageDecoder.
+    (*decoder)->setData(0, false); // Unref SegmentReader from ImageDecoder.
     (*decoder)->clearCacheExceptFrame(index);
     (*decoder)->setMemoryAllocator(0);
 
@@ -394,24 +316,16 @@ bool ImageFrameGenerator::hasAlpha(size_t index)
     return true;
 }
 
-bool ImageFrameGenerator::getYUVComponentSizes(SkISize componentSizes[3])
+bool ImageFrameGenerator::getYUVComponentSizes(SegmentReader* data, SkISize componentSizes[3])
 {
     TRACE_EVENT2("blink", "ImageFrameGenerator::getYUVComponentSizes", "width", m_fullSize.width(), "height", m_fullSize.height());
 
-    SharedBuffer* data = 0;
-    bool allDataReceived = false;
-    m_data->data(&data, &allDataReceived);
-
-    // FIXME: YUV decoding does not currently support progressive decoding.
-    if (!allDataReceived)
-        return false;
-
     OwnPtr<ImageDecoder> decoder = ImageDecoder::create(*data, ImageDecoder::AlphaPremultiplied, ImageDecoder::GammaAndColorProfileApplied);
     if (!decoder)
         return false;
 
     // Setting a dummy ImagePlanes object signals to the decoder that we want to do YUV decoding.
-    decoder->setData(data, allDataReceived);
+    decoder->setData(data, true);
     OwnPtr<ImagePlanes> dummyImagePlanes = adoptPtr(new ImagePlanes);
     decoder->setImagePlanes(dummyImagePlanes.release());