Teach Skia to use discardable memory

Teach Skia to use discardable memory

Using discardable memory in Skia requires the following changes:
1. Use of SkDiscardablePixelRef.
2. Implementation of SkImageGenerator to decode an image.

This change implements items above by reusing existing infrastructure
in ImageFrameGenerator and DeferredImageDecoder. ImageFrameGenerator
performs the actual decoding. DeferredImageDecoder is responsible for
generating SkBitmap backed by SkDiscardablePixelRef.

There are some limitations of this implementations:
1. Decoder does not write directly to the memory provided by Skia.
   memcpy() is required.
2. Decoding allocates heap memory as output buffer.
3. ImageDecodingStore is still in use and manages a pool of cache.

Since the deferred image decoding is still used by Android the two code
paths will coexist until the transition is complete. Upcoming changes
will loosen up the above limitations.

BUG=318490
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163743

[Alpha Left Google, 2013-12-05 19:50:35]

Review ping.

[Stephen White, 2013-12-05 21:52:13]

LGTM; my comments are just nits

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
File Source/core/platform/graphics/DecodingImageGenerator.cpp (right):

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
Source/core/platform/graphics/DecodingImageGenerator.cpp:57: SkData* skdata =
SkData::NewWithCopy(buffer->data(), buffer->size());
Nit: no need for the temporary; could just be

return SkData::NewWithCopy(buffer->data(), buffer->size());

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
File Source/core/platform/graphics/DecodingImageGenerator.h (right):

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
Source/core/platform/graphics/DecodingImageGenerator.h:44: // as and adapter to
ImageFrameGenerator which actually performs decoding.
Naming nit: if it's an adapter, maybe it should have Adapter in the name?

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
File Source/core/platform/graphics/DeferredImageDecoder.h (right):

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
Source/core/platform/graphics/DeferredImageDecoder.h:52: static void
setEnabledSkiaDiscardableMemory(bool);
Nit: kind of awkward. Could this be setSkiaDiscardableMemoryEnabled()?

[Alpha Left Google, 2013-12-08 00:30:11]

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
File Source/core/platform/graphics/DecodingImageGenerator.cpp (right):

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
Source/core/platform/graphics/DecodingImageGenerator.cpp:57: SkData* skdata =
SkData::NewWithCopy(buffer->data(), buffer->size());
On 2013/12/05 21:52:14, Stephen White wrote:
> Nit: no need for the temporary; could just be
> 
> return SkData::NewWithCopy(buffer->data(), buffer->size());

Done.

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
File Source/core/platform/graphics/DecodingImageGenerator.h (right):

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
Source/core/platform/graphics/DecodingImageGenerator.h:44: // as and adapter to
ImageFrameGenerator which actually performs decoding.
On 2013/12/05 21:52:14, Stephen White wrote:
> Naming nit: if it's an adapter, maybe it should have Adapter in the name?

I prefer to keep this name. It sounds awkward to call this
ImageGeneratorAdapter.

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
File Source/core/platform/graphics/DeferredImageDecoder.h (right):

https://codereview.chromium.org/105773003/diff/1/Source/core/platform/graphic...
Source/core/platform/graphics/DeferredImageDecoder.h:52: static void
setEnabledSkiaDiscardableMemory(bool);
On 2013/12/05 21:52:14, Stephen White wrote:
> Nit: kind of awkward. Could this be setSkiaDiscardableMemoryEnabled()?

Done.

[commit-bot: I haz the power, 2013-12-08 00:32:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/105773003/20001

[commit-bot: I haz the power, 2013-12-08 00:32:50]

Failed to apply patch for
Source/core/platform/graphics/DecodingImageGenerator.h:
While running svn copy
Source/core/platform/graphics/test/MockDiscardablePixelRef.h
Source/core/platform/graphics/DecodingImageGenerator.h --config-dir
/b/commit-queue/subversion_config --non-interactive --username
commit-bot@chromium.org;
  svn: Path 'Source/core/platform/graphics/test/MockDiscardablePixelRef.h' does
not exist

Patch:   NR 
Source/core/platform/graphics/test/MockDiscardablePixelRef.h->Source/core/platform/graphics/DecodingImageGenerator.h
Index: Source/core/platform/graphics/DecodingImageGenerator.h
diff --git a/Source/core/platform/graphics/test/MockDiscardablePixelRef.h
b/Source/core/platform/graphics/DecodingImageGenerator.h
similarity index 59%
copy from Source/core/platform/graphics/test/MockDiscardablePixelRef.h
copy to Source/core/platform/graphics/DecodingImageGenerator.h
index
c1bd7fe21d71d15a94bd881cc323536daf43c256..f23fd4a6fda3f73ef129af20b425fa37e94a601e
100644
--- a/Source/core/platform/graphics/test/MockDiscardablePixelRef.h
+++ b/Source/core/platform/graphics/DecodingImageGenerator.h
@@ -23,46 +23,41 @@
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef MockDiscardablePixelRef_h
+#ifndef DecodingImageGenerator_h
+#define DecodingImageGenerator_h
 
-#include "SkBitmap.h"
-#include "SkPixelRef.h"
+#include "SkImageGenerator.h"
+#include "SkImageInfo.h"
 
-namespace WebCore {
+#include "wtf/RefPtr.h"
 
-class MockDiscardablePixelRef : public SkPixelRef {
-public:
-    MockDiscardablePixelRef() : discarded(false) { setURI("discardable"); }
-    ~MockDiscardablePixelRef() { }
+class SkData;
 
-    void discard()
-    {
-        ASSERT(!m_lockedMemory);
-        discarded = true;
-    }
+namespace WebCore {
 
-    SK_DECLARE_UNFLATTENABLE_OBJECT()
+class ImageFrameGenerator;
 
-protected:
-    // SkPixelRef implementation.
-    virtual void* onLockPixels(SkColorTable**)
-    {
-        if (discarded)
-            return 0;
-        m_lockedMemory = &discarded;
-        return m_lockedMemory;
-    }
+// Implements SkImageGenerator and used by SkPixelRef to populate a discardable
+// memory with decoded pixels.
+//
+// This class does not own an ImageDecode. It does not own encoded data. It
serves
+// as and adapter to ImageFrameGenerator which actually performs decoding.
+class DecodingImageGenerator : public SkImageGenerator {
+public:
+    DecodingImageGenerator(PassRefPtr<ImageFrameGenerator>, const SkImageInfo&,
size_t index);
+    virtual ~DecodingImageGenerator();
 
-    virtual void onUnlockPixels()
-    {
-        m_lockedMemory = 0;
-    }
+    // SkImageGenerator implementation.
+    virtual SkData* refEncodedData();
+    virtual bool getInfo(SkImageInfo*);
+    virtual bool getPixels(const SkImageInfo&, void* pixels, size_t rowBytes);
 
 private:
-    void* m_lockedMemory;
-    bool discarded;
+    RefPtr<ImageFrameGenerator> m_frameGenerator;
+    SkImageInfo m_imageInfo;
+    size_t m_frameIndex;
 };
 
 } // namespace WebCore
 
-#endif // MockDiscardablePixelRef_h
+#endif // DecodingImageGenerator_h_

[commit-bot: I haz the power, 2013-12-08 00:57:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/105773003/40001

[commit-bot: I haz the power, 2013-12-08 01:11:25]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[Alpha Left Google, 2013-12-09 20:55:17]

+jamesr for owners approval for Source/web/tests.

[jamesr, 2013-12-09 21:20:25]

tests lgtm, but you really should move these into Source/platform/graphics

[Alpha Left Google, 2013-12-09 21:24:20]

On 2013/12/09 21:20:25, jamesr wrote:
> tests lgtm, but you really should move these into Source/platform/graphics

It was in Source/platform/graphics but I think schenney@ moved them back to
Source/web last Friday. :(

[commit-bot: I haz the power, 2013-12-09 21:25:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/105773003/40001

[jamesr, 2013-12-09 21:26:37]

Huh? Why?  Unit tests should live next to the code under tests.

[Alpha Left Google, 2013-12-09 21:28:34]

On 2013/12/09 21:26:37, jamesr wrote:
> Huh? Why?  Unit tests should live next to the code under tests.

I don't have much background about the move: 
https://codereview.chromium.org/99103006

[commit-bot: I haz the power, 2013-12-09 23:07:38]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[commit-bot: I haz the power, 2013-12-10 00:18:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/105773003/60001

[commit-bot: I haz the power, 2013-12-10 02:44:07]

Retried try job too often on linux_blink for step(s) blink_platform_unittests,
webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2013-12-12 00:40:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hclam@chromium.org/105773003/110001

[commit-bot: I haz the power, 2013-12-12 02:27:49]

Change committed as 163743

[johnme, 2013-12-12 11:31:55]

A revert of this CL has been created in
https://codereview.chromium.org/105003005/ by johnme@chromium.org.

The reason for reverting is: This patch made
DeferredImageDecoderTest.drawDiscardableIntoSkPicture consistently fail on bot:
WebKit Android (Nexus4).

[reveman, 2013-12-12 16:57:10]

https://codereview.chromium.org/105773003/diff/110001/Source/platform/graphic...
File Source/platform/graphics/DeferredImageDecoder.cpp (right):

https://codereview.chromium.org/105773003/diff/110001/Source/platform/graphic...
Source/platform/graphics/DeferredImageDecoder.cpp:47: bool
DeferredImageDecoder::s_skiaDiscardableMemoryEnabled = false;
Can we just make this true if !defined(OS_ANDROID) for now. I don't think a
setting is necessary as we should make this default on all platforms asap.

[Alpha Left Google, 2013-12-12 19:22:30]

On 2013/12/12 16:57:10, David Reveman wrote:
>
https://codereview.chromium.org/105773003/diff/110001/Source/platform/graphic...
> File Source/platform/graphics/DeferredImageDecoder.cpp (right):
> 
>
https://codereview.chromium.org/105773003/diff/110001/Source/platform/graphic...
> Source/platform/graphics/DeferredImageDecoder.cpp:47: bool
> DeferredImageDecoder::s_skiaDiscardableMemoryEnabled = false;
> Can we just make this true if !defined(OS_ANDROID) for now. I don't think a
> setting is necessary as we should make this default on all platforms asap.

Okay. That's easy to do. I'll upload another patch.