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

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

Index: third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h
diff --git a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h
index 06664c71efcd8d4b562fa11ad75a1786e45a3593..4846a939b4a78bab70725b1f72c64496d49ef788 100644
--- a/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h
+++ b/third_party/WebKit/Source/platform/graphics/ImageFrameGenerator.h
@@ -41,6 +41,8 @@
 #include "wtf/ThreadSafeRefCounted.h"
 #include "wtf/Vector.h"
 
+class SkData;
+
 namespace blink {
 
 class ImageDecoder;
@@ -78,8 +80,9 @@ public:
 
     void setData(PassRefPtr<SharedBuffer>, bool allDataReceived);
 
-    // Creates a new SharedBuffer containing the data received so far.
-    void copyData(RefPtr<SharedBuffer>*, bool* allDataReceived);
+    // Return our encoded image data. Caller takes ownership and must unref the data
+    // according to the contract SkImageGenerator::refEncodedData.
+    SkData* refEncodedData() const;
 

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

Noticed one review question about this "Could this be const?".  I read that as a
request for

  const SkData* refEncodedData() const;

since we probably don't want callers changing our encoded data.

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

That is handled by
const void* SkData::data() const { return fPtr; } - so client cannot alter our
data.

Otherwise, it cannot be like this "const SkData* refEncodedData() const;"
because it is non const through the call stack. Signatures:

virtual SkData* SkImageGenerator::onRefEncodedData();
overriden here: SkData* DecodingImageGenerator::onRefEncodedData();

calling:

SkData* DecodingImageGenerator::onRefEncodedData();
SkData* ImageFrameGenerator::refEncodedData() const;

[scroggo_chromium, 2015/12/04 16:15:25]

SkData also has writable_data()
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i...).

which allows modifying its data. FWIW, its documentation recommends using with
caution, and it asserts that there is only one owner.
When SkImageGenerator::onRefEncodedData was added (back in May 2014 - see
https://codereview.chromium.org/300263005), SkData was always assumed to be
const (it had no non-const methods, which we can verify with
https://skia.googlesource.com/skia/+/00f8d6c75d22ce8f95f932c5b101354b196fa0df...).
writable_data() was added more recently (September 2014:
https://codereview.chromium.org/560653004). I think it makes sense to make these
all return a const SkData.

On a related note, one could argue that it's incorrect to label the method
const, since it modifies its ThreadSafeDataTransport's SharedBuffer (by
collapsing the data). That also seems like it might conflict with my efforts in
crbug.com/467772 to stop collapsing the data. I think that's okay though - here
we only collapse the data when the image is complete, so we only do it once
(rather than the worst case scenario of doing it several times during load).

But it will still be unnecessary, in the case where the image is (for example) a
GIF file that will not be decoded on the GPU. It's much better than the existing
case - where we create a new copy each time the method is called, even if the
image is not complete, but I think we could combine this with a check to
determine whether the format is one that can be GPU decoded to save even more
wasted work.

     SkISize getFullSize() const { return m_fullSize; }
 
@@ -107,7 +110,11 @@ private:
     bool decode(size_t index, ImageDecoder**, SkBitmap*);
 
     SkISize m_fullSize;
-    ThreadSafeDataTransport m_data;
+
+    // ThreadSafeDataTransport is referenced by ImageFrameGenerator and SkData
+    // from refEncodedData. In case ImageFrameGenerator get's deleted, SkData
+    // would hold the reference to it (and underlying data).
+    RefPtr<ThreadSafeDataTransport> m_data;
     bool m_isMultiFrame;
     bool m_decodeFailedAndEmpty;
     Vector<bool> m_hasAlpha;