Ganesh: images upload to GPU performance fix (skip copying encoded data)

third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.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
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 99 matching lines @@
 ImageFrameGenerator::~ImageFrameGenerator()
 {
     ImageDecodingStore::instance().removeCacheIndexedByGenerator(this);
 }
 
 void ImageFrameGenerator::setData(PassRefPtr<SharedBuffer> data, bool allDataReceived)
 {
     m_data.setData(data.get(), allDataReceived);
 }
 
-void ImageFrameGenerator::copyData(RefPtr<SharedBuffer>* data, bool* allDataReceived)
+static void sharedBufSkDataReleaseProc(const void* addr, void* ctx)
 {
+    SharedBuffer* buffer = static_cast<SharedBuffer*>(ctx);
+    ASSERT(buffer && buffer->data() == addr);
+    // Deref SharedBuffer shared with m_data.
+    buffer->deref();

[Stephen White, 2015/12/02 16:15:14]

Are we 100% sure this is thread-safe? Does this callback always happen on the
same thread as the refSkData() below?

[chrishtr, 2015/12/02 16:44:28]

It's not clear to me why removing the RefPtr for the callback in this patchset
is correct. The pointer points at an object which is ref-counted by
ThreadSafeDataTransport. If that object dies before this callback, the pointer
will be deleted at that time and the access here will be invalid, right?

[Stephen White, 2015/12/02 18:25:47]

There's an explicit call to ref() in refSkData() below, which will keep 
the object alive until this deref().

[chrishtr, 2015/12/02 18:26:44]

Ah I see, thanks.

+}
+
+SkData* ImageFrameGenerator::refSkData()
+{
+    // Every SkData instance and SharedBuffer need to keep buffer->data() refcounted.
+    // Both SkData and m_data.m_readBuffer are having separate ref counting implementations.
+    //
+    // SkData's SkData::NewWithProc is designed with similar use case in mind,
+    // unmapping wrapped shmem data once all SkData instances are disposed.
+    // in this case, sharedBufSkDataReleaseProc would just need to deref (since m_data.m_readBuffer
+    // might still hold a reference) and that is happening in sharedBufSkDataReleaseProc.
+    //
+    // If m_data gets disposed, SkData would still need to hold the ref to SharedBuffer,
+    // and that is why SharedBuffer (accompanying returned SkData) is passed to client of this
+    // call - by contract they need to call SkData unref that would end up in sharedBufSkDataReleaseProc
+    // releasing the ref to SharedBuffer.
+    //
+    // A note about allDataReceived:
+    // Client side use case is valid only for full image (encoded) data downloaded,
+    // but, incidentally, this is in line with current Chromium implementation, where
+    // DeferredImageDecoder::setData is called only once with allDataReceived and after
+    // that m_data.m_readBuffer.data() is not changed. For the sake of not leaving loose ends,
+    // ThreadSafeDataTransport::data is checking if there is new data added after AllDataReceived
+    // was set to true.
     SharedBuffer* buffer = 0;
-    m_data.data(&buffer, allDataReceived);
-    if (buffer)
-        *data = buffer->copy();
+    bool allDataReceived = false;
+    m_data.data(&buffer, &allDataReceived);
+    if (!allDataReceived)
+        return nullptr;
+
+    buffer->ref();

[aleksandar.stojiljkovic, 2015/12/02 16:58:24]

I'll cover this with comment that the buffer->ref() is protecting it even m_data
get's disposed. Good point.

+    return SkData::NewWithProc(buffer->data(), buffer->size(), sharedBufSkDataReleaseProc, buffer);
 }
 
 bool ImageFrameGenerator::decodeAndScale(const SkImageInfo& info, size_t index, void* pixels, size_t rowBytes)
 {
     // This method is called to populate a discardable memory owned by Skia.
 
     // Prevents concurrent decode or scale operations on the same image data.
     MutexLocker lock(m_decodeMutex);
 
     // This implementation does not support scaling so check the requested size.
@@ skipping 237 matching lines @@
 
     // Setting a dummy ImagePlanes object signals to the decoder that we want to do YUV decoding.
     decoder->setData(data, allDataReceived);
     OwnPtr<ImagePlanes> dummyImagePlanes = adoptPtr(new ImagePlanes);
     decoder->setImagePlanes(dummyImagePlanes.release());
 
     return updateYUVComponentSizes(decoder.get(), componentSizes, ImageDecoder::SizeForMemoryAllocation);
 }
 
 } // namespace blink