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

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

 /*
  * 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 23 matching lines @@
 #include "wtf/Allocator.h"
 #include "wtf/Noncopyable.h"
 #include "wtf/PassOwnPtr.h"
 #include "wtf/PassRefPtr.h"
 #include "wtf/RefCounted.h"
 #include "wtf/RefPtr.h"
 #include "wtf/ThreadingPrimitives.h"
 #include "wtf/ThreadSafeRefCounted.h"
 #include "wtf/Vector.h"
 
+class SkData;
+
 namespace blink {
 
 class ImageDecoder;
 class SharedBuffer;
 
 class PLATFORM_EXPORT ImageDecoderFactory {
     USING_FAST_MALLOC(ImageDecoderFactory);
     WTF_MAKE_NONCOPYABLE(ImageDecoderFactory);
 public:
     ImageDecoderFactory() {}
@@ skipping 17 matching lines @@
     // into |pixels| with a stride of |rowBytes|.
     //
     // Returns true if decoding was successful.
     bool decodeAndScale(const SkImageInfo&, size_t index, void* pixels, size_t rowBytes);
 
     // Decodes YUV components directly into the provided memory planes.
     bool decodeToYUV(SkISize componentSizes[3], void* planes[3], size_t rowBytes[3]);
 
     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; }
 
     bool isMultiFrame() const { return m_isMultiFrame; }
 
     // FIXME: Return alpha state for each frame.
     bool hasAlpha(size_t);
 
     bool getYUVComponentSizes(SkISize componentSizes[3]);
 
 private:
     class ExternalMemoryAllocator;
     friend class ImageFrameGeneratorTest;
     friend class DeferredImageDecoderTest;
     // For testing. |factory| will overwrite the default ImageDecoder creation logic if |factory->create()| returns non-zero.
     void setImageDecoderFactory(PassOwnPtr<ImageDecoderFactory> factory) { m_imageDecoderFactory = factory; }
 
     void setHasAlpha(size_t index, bool hasAlpha);
 
     // These methods are called while m_decodeMutex is locked.
     SkBitmap tryToResumeDecode(const SkISize& scaledSize, size_t index);
 
     // Use the given decoder to decode. If a decoder is not given then try to create one.
     // Returns true if decoding was complete.
     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;
     int m_decodeCount;
     Vector<bool> m_frameComplete;
     size_t m_frameCount;
     OwnPtr<ExternalMemoryAllocator> m_externalAllocator;
 
     OwnPtr<ImageDecoderFactory> m_imageDecoderFactory;
 
     // Prevents multiple decode operations on the same data.
     Mutex m_decodeMutex;
 
     // Protect concurrent access to m_hasAlpha.
     Mutex m_alphaMutex;
 
 #if COMPILER(MSVC)
     friend struct ::WTF::OwnedPtrDeleter<ExternalMemoryAllocator>;
 #endif
 };
 
 } // namespace blink
 
 #endif