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 4800a3141ac19725d605923cccbbcb94373fba80..1b21a439abb79ba12913a12f52862c4dd44d8f71 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"
@@ -52,7 +51,7 @@ static bool compatibleInfo(const SkImageInfo& src, const SkImageInfo& dst)
 
 // Creates a SkPixelRef such that the memory for pixels is given by an external body.
 // This is used to write directly to the memory given by Skia during decoding.
-class ImageFrameGenerator::ExternalMemoryAllocator final : public SkBitmap::Allocator {
+class ExternalMemoryAllocator final : public SkBitmap::Allocator {
     USING_FAST_MALLOC(ExternalMemoryAllocator);
     WTF_MAKE_NONCOPYABLE(ExternalMemoryAllocator);
 public:
@@ -101,103 +100,39 @@ 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));
+    RefPtr<ExternalMemoryAllocator> externalAllocator = adoptRef(new ExternalMemoryAllocator(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);
+    // TODO (scroggo): Convert tryToResumeDecode() and decode() to take a PassRefPtr<SkBitmap::Allocator>
+    // instead of a bare pointer.

[Peter Kasting, 2016/03/25 06:24:36]

Nit: Why bother word-wrapping the comment when you're going to wrap so much
later than 80 columns?

[scroggo, 2016/03/25 15:49:52]

What would you prefer? Blink seems to prefer *not* wrapping at all, but I find
it hard to look at. I think 80 columns is a reasonable standard, although it
doesn't really map to monitors anymore (and doesn't match surrounding code). I
chose to wrap here based on a subconscious aesthetic that said "this looks
ugly", probably because it goes beyond the line of code below.

I wrapped to 80 columns.

[Peter Kasting, 2016/03/25 21:15:56]

FWIW, Blink style is soon going to be transitioning to be more like Chromium
style, and at that point I think they're adopting the 80-column rule, so
wrapping to 80 now seems good to me.

[scroggo_chromium, 2016/03/25 21:31:59]

Yay!

+    SkBitmap bitmap = tryToResumeDecode(data, allDataReceived, index, scaledSize, externalAllocator.get());
     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();
+    externalAllocator.clear();
 
     // Check to see if the decoder has written directly to the pixel memory
     // provided. If not, make a copy.
@@ -209,11 +144,10 @@ bool ImageFrameGenerator::decodeAndScale(size_t index, const SkImageInfo& info,
     return true;
 }
 
-bool ImageFrameGenerator::decodeToYUV(size_t index, const SkISize componentSizes[3], void* planes[3], const size_t rowBytes[3])
+bool ImageFrameGenerator::decodeToYUV(SegmentReader* data, size_t index, const SkISize componentSizes[3], void* planes[3], const size_t rowBytes[3])
 {
-    // Prevent concurrent decode or scale operations on the same image data.
-    MutexLocker lock(m_decodeMutex);
-
+    // TODO (scroggo): The only interesting thing this uses from the ImageFrameGenerator is m_decodeFailed.
+    // Move this into DecodingImageGenerator, which is the only class that calls it.
     if (m_decodeFailed)
         return false;
 
@@ -224,18 +158,11 @@ bool ImageFrameGenerator::decodeToYUV(size_t index, const SkISize componentSizes
         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());
@@ -252,16 +179,19 @@ bool ImageFrameGenerator::decodeToYUV(size_t index, const SkISize componentSizes
     return false;
 }
 
-SkBitmap ImageFrameGenerator::tryToResumeDecode(size_t index, const SkISize& scaledSize)
+SkBitmap ImageFrameGenerator::tryToResumeDecode(SegmentReader* data, bool allDataReceived, size_t index, const SkISize& scaledSize, SkBitmap::Allocator* allocator)
 {
     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.
+    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, allocator);
 
     if (!decoder)
         return SkBitmap();
@@ -321,14 +251,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, SkBitmap::Allocator* allocator)
 {

[Peter Kasting, 2016/03/25 06:24:36]

Nit: Maybe ASSERT(m_decodeMutex.locked())?

[scroggo, 2016/03/25 15:49:52]

Done.

     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;
@@ -347,8 +273,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(allocator);
+        (*decoder)->setMemoryAllocator(allocator);
     }
 
     (*decoder)->setData(data, allDataReceived);
@@ -361,7 +287,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(PassRefPtr<SegmentReader>(nullptr), false); // Unref SegmentReader from ImageDecoder.
     (*decoder)->clearCacheExceptFrame(index);
     (*decoder)->setMemoryAllocator(0);
 
@@ -391,24 +317,16 @@ bool ImageFrameGenerator::hasAlpha(size_t index)
     return true;
 }
 
-bool ImageFrameGenerator::getYUVComponentSizes(SkYUVSizeInfo* sizeInfo)
+bool ImageFrameGenerator::getYUVComponentSizes(SegmentReader* data, SkYUVSizeInfo* sizeInfo)
 {
     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());