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

Source/platform/graphics/ImageFrameGenerator.cpp

 /*
  * 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 72 matching lines @@
     size = decoder->decodedYUVSize(2, sizeType);
     componentSizes[2].set(size.width(), size.height());
     return true;
 }
 
 ImageFrameGenerator::ImageFrameGenerator(const SkISize& fullSize, PassRefPtr<SharedBuffer> data, bool allDataReceived, bool isMultiFrame)
     : m_fullSize(fullSize)
     , 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);
 }
 
 ImageFrameGenerator::~ImageFrameGenerator()
 {
     ImageDecodingStore::instance()->removeCacheIndexedByGenerator(this);
 }
 
 void ImageFrameGenerator::setData(PassRefPtr<SharedBuffer> data, bool allDataReceived)
@@ skipping 94 matching lines @@
 
     ImageDecoder* decoder = 0;
     const bool resumeDecoding = ImageDecodingStore::instance()->lockDecoder(this, m_fullSize, &decoder);
     ASSERT(!resumeDecoding || decoder);
 
     SkBitmap fullSizeImage;
     bool complete = decode(index, &decoder, &fullSizeImage);
 
     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
     // the decoder is owned by ImageDecodingStore.
     OwnPtr<ImageDecoder> decoderContainer;
     if (!resumeDecoding)
         decoderContainer = adoptPtr(decoder);
 
     if (fullSizeImage.isNull()) {
         // If decode has failed and resulted an empty image we can save work
         // in the future by returning early.
         m_decodeFailedAndEmpty = !m_isMultiFrame && decoder->failed();
 
         if (resumeDecoding)
             ImageDecodingStore::instance()->unlockDecoder(this, decoder);
         return SkBitmap();
     }
 
     // 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)
             ImageDecodingStore::instance()->removeDecoder(this, decoder);

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

Need to clear m_frameComplete() here.

         else
             ImageDecodingStore::instance()->unlockDecoder(this, decoder);
     } else if (!removeDecoder) {
         ImageDecodingStore::instance()->insertDecoder(this, decoderContainer.release());
     }
     return fullSizeImage;
 }
 
 void ImageFrameGenerator::setHasAlpha(size_t index, bool hasAlpha)
 {
@@ skipping 32 matching lines @@
 
     if (!m_isMultiFrame && newDecoder && allDataReceived) {
         // If we're using an external memory allocator that means we're decoding
         // directly into the output memory and we can save one memcpy.
         ASSERT(m_externalAllocator.get());
         (*decoder)->setMemoryAllocator(m_externalAllocator.get());
     }
     (*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);
 
     if (!frame || frame->status() == ImageFrame::FrameEmpty)
         return false;
 
     // A cache object is considered complete if we can decode a complete frame.
     // Or we have received all data. The image might not be fully decoded in
     // the latter case.
@@ skipping 36 matching lines @@
 
     // Setting a dummy ImagePlanes object signals to the decoder that we want to do YUV decoding.
     decoder->setData(data, allDataReceived);
     OwnPtr<ImagePlanes> dummyImagePlanes = adoptPtr(new ImagePlanes);
     decoder->setImagePlanes(dummyImagePlanes.release());
 
     return updateYUVComponentSizes(decoder.get(), componentSizes, ImageDecoder::SizeForMemoryAllocation);
 }
 
 } // namespace blink