REGRESSION(r198782, r201043): [image-decoders] Flickering with some animated gif

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

There's some flickering when loading big enough animated gifs running the animation in a loop, you can try this one for example:

https://media0.giphy.com/media/l3UceJyj8Wwlkve8M/200.gif

The first time it loads everything is fine, but after the first loop iteration there are several flickering effects, once every time the animation finishes and some others happening in the middle of the animation loop. This doesn't affect mac ports, only ports using image-decoders implementation. The flickering happens because we fail to render some of the frames, and it has two diferent causes:

 - In r198782, ImageDecoder::createFrameImageAtIndex(), was modified to check first if the image is empty to return early, and then try to get the frame image from the decoder. This is the aone causing flickerin always on the first frame after one iteration. It happens because ImageDecoder::size() is always empty at that point. The first doesn't happen because the gif is loaded and BitmapImage calls isSizeAvailable() from BitmapImage::dataChanged(). The isSizeAvailable call makes the gif decoder calculate the size. But for the next iterations, frames are cached and BitmapImage already has the decoded data so isSizeAvailable is not called again. When createFrameImageAtIndex() is called again for the first frame, size is empty, until the gif decoder creates the reader again, which happens when frameBufferAtIndex() si called, so after that the size is available. So, we could call isSizeAvailable before checking the size, or simply do the check after the frameBufferAtIndex() as we used to do.

 - In r201043 BitmapImage::destroyDecodedDataIfNecessary() was fixed to use the actual bytes used by the frame in order to decide whether to destroy decoded data or not. This actually revealed a bug, it didn't happen before because we were never destroying frames before. The bug is in the gif decoder that doesn't correctly handle the case of destroying only some of the frames from the buffer. The gif decoder is designed to always process the frames in order, the reader keeps an index of the currently processed frame, when some frames are read from the cache, and then we ask the decoder for a not cached frame, the currently processed frame is not in sync with the actual frame we are asking for, and we end do not processing any frame at all.

[Carlos Garcia Campos]

Created attachment 281971 [details]
Patch

[Antonio Gomes]

Comment on attachment 281971 [details]
Patch

I am confident to review it, given the logical changelog explanation.

[Said Abou-Hallawa]

Comment on attachment 281971 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281971&action=review

> Source/WebCore/platform/image-decoders/ImageDecoder.cpp:319
> +    if (!buffer || buffer->status() == ImageFrame::FrameEmpty || size().isEmpty())

I don't like this kind of dependency and implicit hidden calculation. How do I know or remember that to get the size of image in the ImageDecoder I have to call frameBufferAtIndex() first? Can't we fix this by changing the base class function ImageDecoder::setSize() to be like this:

virtual IntSize size() { return frameBufferAtIndex(0) ? m_size : IntSize(); }

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 281971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281971&action=review
> 
> > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:319
> > +    if (!buffer || buffer->status() == ImageFrame::FrameEmpty || size().isEmpty())
> 
> I don't like this kind of dependency and implicit hidden calculation. How do
> I know or remember that to get the size of image in the ImageDecoder I have
> to call frameBufferAtIndex() first?

Yes, I agree, that's why I added the comment there. But note that it's not required to call frameBufferAtIndex specifically, it just happens that frameBufferAtIndex ensures the size is retrieved from the image data. In this case, we just take advantage that we are calling this method to avoid any other check.

> Can't we fix this by changing the base
> class function ImageDecoder::setSize() to be like this:
> 
> virtual IntSize size() { return frameBufferAtIndex(0) ? m_size : IntSize(); }

I thought about that, although not using frameBufferAtIndex, but isSizeAvailable() instead, that decodes enough data to get the size. However, I thought there was a reason why we have isSizeAvailable() and size() public, instead of just making size() do the isSizeAvailable() implicitly. In this particular case, we would end up decoding twice, first to get the size and then to get the buffer. We could avoid that by checking the size after frameBufferAtIndex (because the isSizeAvailable is cached), but we would be in the same situation.

[Carlos Garcia Campos]

Committed r202616: <http://trac.webkit.org/changeset/202616>

[Said Abou-Hallawa]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 281971 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=281971&action=review
> > 
> > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:319
> > > +    if (!buffer || buffer->status() == ImageFrame::FrameEmpty || size().isEmpty())
> > 
> > I don't like this kind of dependency and implicit hidden calculation. How do
> > I know or remember that to get the size of image in the ImageDecoder I have
> > to call frameBufferAtIndex() first?
> 
> Yes, I agree, that's why I added the comment there. But note that it's not
> required to call frameBufferAtIndex specifically, it just happens that
> frameBufferAtIndex ensures the size is retrieved from the image data. In
> this case, we just take advantage that we are calling this method to avoid
> any other check.
> 
> > Can't we fix this by changing the base
> > class function ImageDecoder::setSize() to be like this:
> > 
> > virtual IntSize size() { return frameBufferAtIndex(0) ? m_size : IntSize(); }
> 
> I thought about that, although not using frameBufferAtIndex, but
> isSizeAvailable() instead, that decodes enough data to get the size.
> However, I thought there was a reason why we have isSizeAvailable() and
> size() public, instead of just making size() do the isSizeAvailable()
> implicitly. In this particular case, we would end up decoding twice, first
> to get the size and then to get the buffer. We could avoid that by checking
> the size after frameBufferAtIndex (because the isSizeAvailable is cached),
> but we would be in the same situation.

I don't think your reasoning about decoding the first frame twice is correct. The parser of the ImageDecoder is state machine. It moves along the encoded data and changes its state when finishing decoding the data for the current state. All the decoded data, either image metadata or images frames are cached. So querying the size from the first frame should not be repeated when querying the frame itself.

[Carlos Garcia Campos]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Comment on attachment 281971 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=281971&action=review
> > > 
> > > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:319
> > > > +    if (!buffer || buffer->status() == ImageFrame::FrameEmpty || size().isEmpty())
> > > 
> > > I don't like this kind of dependency and implicit hidden calculation. How do
> > > I know or remember that to get the size of image in the ImageDecoder I have
> > > to call frameBufferAtIndex() first?
> > 
> > Yes, I agree, that's why I added the comment there. But note that it's not
> > required to call frameBufferAtIndex specifically, it just happens that
> > frameBufferAtIndex ensures the size is retrieved from the image data. In
> > this case, we just take advantage that we are calling this method to avoid
> > any other check.
> > 
> > > Can't we fix this by changing the base
> > > class function ImageDecoder::setSize() to be like this:
> > > 
> > > virtual IntSize size() { return frameBufferAtIndex(0) ? m_size : IntSize(); }
> > 
> > I thought about that, although not using frameBufferAtIndex, but
> > isSizeAvailable() instead, that decodes enough data to get the size.
> > However, I thought there was a reason why we have isSizeAvailable() and
> > size() public, instead of just making size() do the isSizeAvailable()
> > implicitly. In this particular case, we would end up decoding twice, first
> > to get the size and then to get the buffer. We could avoid that by checking
> > the size after frameBufferAtIndex (because the isSizeAvailable is cached),
> > but we would be in the same situation.
> 
> I don't think your reasoning about decoding the first frame twice is
> correct. The parser of the ImageDecoder is state machine. It moves along the
> encoded data and changes its state when finishing decoding the data for the
> current state. All the decoded data, either image metadata or images frames
> are cached. So querying the size from the first frame should not be repeated
> when querying the frame itself.

Well, I didn't say we decode the first frame twice, but that we decode data twice. Normally to get the size you only need to decode some data, and to get the frame you need to decode the same data than for the size and more, so my point was that getting the frame directly we decode once and get both. You are indeed right that after getting the size, the reader is created and then getting the frame just reuses the reader, so this micro optimization is probably not worth it. I'll file a different bug report to make size() always return the right size, then.