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

-- 
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/20160630/f1a68d76/attachment.html>