Ganesh: images upload to GPU performance fix (skip copying encoded 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 0d29bce0f18e6463f7fcbfd94cabe702c6455231..7a8f61189b0931b59f78b688439a680fc5b361cd 100644
--- a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
+++ b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.cpp
@@ -27,6 +27,7 @@
 
 #include "platform/graphics/ImageFrameGenerator.h"
 
+#include "SkData.h"
 #include "platform/SharedBuffer.h"
 #include "platform/TraceEvent.h"
 #include "platform/graphics/ImageDecodingStore.h"
@@ -101,6 +102,7 @@ static bool updateYUVComponentSizes(ImageDecoder* decoder, SkISize componentSize
 
 ImageFrameGenerator::ImageFrameGenerator(const SkISize& fullSize, PassRefPtr<SharedBuffer> data, bool allDataReceived, bool isMultiFrame)
     : m_fullSize(fullSize)
+    , m_data(new ThreadSafeDataTransport())

[Noel Gordon, 2015/12/04 03:28:03]

Shouldn't this be using adoptRef?

[aleksandar.stojiljkovic, 2015/12/04 11:11:43]

Done.

     , m_isMultiFrame(isMultiFrame)
     , m_decodeFailedAndEmpty(false)
     , m_decodeCount(0)
@@ -116,15 +118,50 @@ ImageFrameGenerator::~ImageFrameGenerator()
 
 void ImageFrameGenerator::setData(PassRefPtr<SharedBuffer> data, bool allDataReceived)
 {
-    m_data.setData(data.get(), allDataReceived);
+    m_data->setData(data.get(), allDataReceived);
 }
 
-void ImageFrameGenerator::copyData(RefPtr<SharedBuffer>* data, bool* allDataReceived)
+static void sharedBufSkDataReleaseProc(const void* addr, void* ctx)

[Noel Gordon, 2015/12/04 03:28:03]

What value does "Buf" add here?  sharedSkDataReleaseProc or even
sharedSkDataReleaseCallback since we do not abbrv in Blink code per style. 


Where is addr used in the function?  If it s not used, change the

"(const void* addr," -> (const void*,

ctx- > context.

[aleksandar.stojiljkovic, 2015/12/04 11:11:43]

Using address now in assert bellow. abbrv resolved.

 {
+    ThreadSafeDataTransport* someonesMData = static_cast<ThreadSafeDataTransport*>(ctx);

[Noel Gordon, 2015/12/04 03:28:03]

someonesMData, is actually someone's dataTransport.  Call it dataTransport.

[aleksandar.stojiljkovic, 2015/12/04 11:11:43]

Done.

+    ASSERT(someonesMData);

[Noel Gordon, 2015/12/04 03:28:03]

Is there a stronger assertion you could make here?  We don't appear to be
deref()ing any old data here, we are appear to be deref-ing our own m_data here.

[aleksandar.stojiljkovic, 2015/12/04 11:11:43]

There is release_assert covering the situation in ThreadSafeDataTransport, added
assert here too. Thanks 

+    // Deref m_data now.
+    someonesMData->deref();
+}
+
+SkData* ImageFrameGenerator::refEncodedData() const
+{
+    // 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.
+    //
+    // Preventing m_data disposal if ImageFrameGenerator is thread safe - SkData could get disposed
+    // in any thread, and SkData would still need to hold the ref to SharedBuffer if ImageFrameGenerator
+    // is deleted. This is why full m_data (ThreadSafeRefCounted) is passed to client - to hold the
+    // reference to SharedBuffer (accompanying returned SkData). By contract in Skia's SkData,
+    // client needs to call SkData unref that would end up in sharedBufSkDataReleaseProc
+    // releasing the ref to ThreadSafeDataTransport (in thread safe way) and underlying SharedBuffer.
+    //
+    // A note about allDataReceived:
+    // Client side use case is valid only for full image (encoded) data downloaded,

[Noel Gordon, 2015/12/04 03:28:03]

What is "Client side use case"?  I have no clue what it means.

[aleksandar.stojiljkovic, 2015/12/04 11:11:43]

Done. It is irrelevant sentence since this code is returing SkData only when all
data is received.

[Noel Gordon, 2015/12/04 12:40:30]

Thanks, and feel free to remove any other "irrelevant sentences" here too.

Then check the result for accuracy.

So for example, in patch #59 (the lastest) I noted this comment contains
"sharedBufSkDataReleaseProc", but you changed the name of that routine. 
Comments are a burden on our project, especially if they are hard to keep brief,
accurate, and precise.

I find the comment hard to follow, frankly.  I hope others understand what it is
trying to say.

[aleksandar.stojiljkovic, 2015/12/04 14:31:15]

Done.

+    // 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;
+
+    // While SkData is holding reference to underlying data, prevent disposing m_data and its content.
+    m_data->ref();
+    return SkData::NewWithProc(buffer->data(), buffer->size(), sharedBufSkDataReleaseProc, m_data.get());
 }
 
 bool ImageFrameGenerator::decodeAndScale(const SkImageInfo& info, size_t index, void* pixels, size_t rowBytes)
@@ -184,7 +221,7 @@ bool ImageFrameGenerator::decodeToYUV(SkISize componentSizes[3], void* planes[3]
 
     SharedBuffer* data = 0;
     bool allDataReceived = false;
-    m_data.data(&data, &allDataReceived);
+    m_data->data(&data, &allDataReceived);
 
     // FIXME: YUV decoding does not currently support progressive decoding.
     ASSERT(allDataReceived);
@@ -290,7 +327,7 @@ bool ImageFrameGenerator::decode(size_t index, ImageDecoder** decoder, SkBitmap*
     SharedBuffer* data = 0;
     bool allDataReceived = false;
     bool newDecoder = false;
-    m_data.data(&data, &allDataReceived);
+    m_data->data(&data, &allDataReceived);
 
     // Try to create an ImageDecoder if we are not given one.
     if (!*decoder) {
@@ -358,7 +395,7 @@ bool ImageFrameGenerator::getYUVComponentSizes(SkISize componentSizes[3])
 
     SharedBuffer* data = 0;
     bool allDataReceived = false;
-    m_data.data(&data, &allDataReceived);
+    m_data->data(&data, &allDataReceived);
 
     // FIXME: YUV decoding does not currently support progressive decoding.
     if (!allDataReceived)