Remove multi-frame image decoder when the image is completely decoded

Source/platform/graphics/ImageFrameGenerator.cpp

Index: Source/platform/graphics/ImageFrameGenerator.cpp
diff --git a/Source/platform/graphics/ImageFrameGenerator.cpp b/Source/platform/graphics/ImageFrameGenerator.cpp
index 46e281b4f5ad6ca3dbc0987c93883a982d2a18fb..c9cb7b5b093b15d1574a4b4b97d6d802692e529d 100644
--- a/Source/platform/graphics/ImageFrameGenerator.cpp
+++ b/Source/platform/graphics/ImageFrameGenerator.cpp
@@ -90,6 +90,7 @@ ImageFrameGenerator::ImageFrameGenerator(const SkISize& fullSize, PassRefPtr<Sha
     , m_isMultiFrame(isMultiFrame)
     , m_decodeFailedAndEmpty(false)
     , m_decodeCount(0)
+    , m_framesCount(0)

[Alpha Left Google, 2014/10/31 17:17:49]

No need to have this variable.

 {
     setData(data.get(), allDataReceived);
 }
@@ -204,6 +205,9 @@ SkBitmap ImageFrameGenerator::tryToResumeDecode(const SkISize& scaledSize, size_
 
     if (!decoder)
         return SkBitmap();
+    if (index >= m_frameComplete.size())
+        m_frameComplete.resize(index + 1);
+    m_frameComplete[index] = complete;
 
     // If we are not resuming decoding that means the decoder is freshly
     // created and we have ownership. If we are resuming decoding then
@@ -225,7 +229,14 @@ SkBitmap ImageFrameGenerator::tryToResumeDecode(const SkISize& scaledSize, size_
     // If the image generated is complete then there is no need to keep
     // the decoder. The exception is multi-frame decoder which can generate
     // multiple complete frames.
-    const bool removeDecoder = complete && !m_isMultiFrame;
+    size_t completeFrameCount = 0;
+
+    for (Vector<bool>::iterator it = m_frameComplete.begin(); it != m_frameComplete.end(); ++it) {
+        if (*it)
+            completeFrameCount++;
+    }
+
+    const bool removeDecoder = m_framesCount && (completeFrameCount >= m_framesCount);

[Alpha Left Google, 2014/10/31 17:17:49]

It's better to have all the states to compute |removeDecoder| done here:

bool removeDecoder;
if (m_isMultiFrame) {
  int decodedFrameCount = /* .. for loop here to count decoded frames .. */;
  removeDecoder = decoder->isAllDataReceived() && decodedFrameCount ==
decoder->frameCount();
} else {
  removeDecoder = complete;
}

 
     if (resumeDecoding) {
         if (removeDecoder)
@@ -282,6 +293,11 @@ bool ImageFrameGenerator::decode(size_t index, ImageDecoder** decoder, SkBitmap*
     (*decoder)->setData(data, allDataReceived);
 
     ImageFrame* frame = (*decoder)->frameBufferAtIndex(index);
+    if (allDataReceived) {

[Alpha Left Google, 2014/10/31 17:17:48]

I prefer you move this block to "tryToResumeDecode".

[Zhenyu Shan, 2014/11/03 08:19:17]

There is an issue here: decoder->frameCount() and decoder->isAllDataAvailable()
returns correct result only here, between the two decoder->setData().

For frameCount(), GIF decoder needs parse the raw data every time. If (m_data ==
null) it will crash.

For isAllDataAvailable(), decoder->setData(0, false) set it to false. So if we
call it in tryToResumeDecode() it will always be false.

We can workaround the frameCount() issue by checking if (m_data == null) in
GIFImageDecoder::parse, but we still need to know if |isAllDataRecieved| to get
the final total frameCount() for an image.

I agree that moving everything needed to calculate |removeDecoder| to
tryToResumeDecode() is reasonable, but this is impossible for now because of
these issues. So in patchset 2 I leave |m_framesCount| not removed. If there is
any better possibility please tell me.

(e.g. Can we change (*decoder)->setData(0, false) to (*decoder)->setData(0,
allDataReceived)?)

[Alpha Left Google, 2014/11/05 00:31:00]

Okay that makes sense. Please see latest comments.

+        m_framesCount = (*decoder)->frameCount();
+        m_frameComplete.resize(m_framesCount);
+    }
+
     (*decoder)->setData(0, false); // Unref SharedBuffer from ImageDecoder.
     (*decoder)->clearCacheExceptFrame(index);
     (*decoder)->setMemoryAllocator(0);