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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=159089

--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160629/4d21a729/attachment.html>