Bug 173403 - ImageDecoder: Gifs with infinite animation only play once very often
Summary: ImageDecoder: Gifs with infinite animation only play once very often
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2017-06-15 00:53 PDT by Carlos Garcia Campos
Modified: 2017-07-12 03:58 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.82 KB, patch)
2017-06-15 00:57 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (10.59 KB, patch)
2017-06-15 01:17 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (2.05 KB, patch)
2017-07-11 06:40 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-06-15 00:53:08 PDT
It doesn't always happen, it's easier to reproduce when loading big files from the network, but it also depends on every file. The problem is that ImageFrameCache is caching the repetition count value always when the size is already available. In the case of gif files, the loop count value can be at any point of the image stream, so having the size available doesn't mean we also have the loop count. So, if the value is queried before it's available, the default value is cached (repeat once) and then always used. So, we need a way to only cache that value when it's really available.
Comment 1 Carlos Garcia Campos 2017-06-15 00:57:49 PDT
Created attachment 312958 [details]
Patch
Comment 2 Carlos Garcia Campos 2017-06-15 01:17:04 PDT
Created attachment 312959 [details]
Patch
Comment 3 Carlos Garcia Campos 2017-06-29 02:02:30 PDT
Ping reviewers.
Comment 4 Michael Catanzaro 2017-06-29 07:36:49 PDT
Said, is there another way you would prefer to do this?
Comment 5 Said Abou-Hallawa 2017-06-29 13:55:53 PDT
(In reply to Michael Catanzaro from comment #4)
> Said, is there another way you would prefer to do this?

I think this issue can be fixed by clearing m_repetitionCount in ImageFrameCache::clearMetadata(). This method is called from ImageSource::dataChanged() every time new data is incrementally added to the image source. ImageFrameCache::clearMetadata() clears all the cached metadata that may change their values with new data added to the image source.
Comment 6 Carlos Garcia Campos 2017-07-11 06:39:30 PDT
(In reply to Said Abou-Hallawa from comment #5)
> (In reply to Michael Catanzaro from comment #4)
> > Said, is there another way you would prefer to do this?
> 
> I think this issue can be fixed by clearing m_repetitionCount in
> ImageFrameCache::clearMetadata(). This method is called from
> ImageSource::dataChanged() every time new data is incrementally added to the
> image source. ImageFrameCache::clearMetadata() clears all the cached
> metadata that may change their values with new data added to the image
> source.

Ah!, good point, I didn't know that, it's indeed a much simpler solution. I'll submit a new patch. Thanks!
Comment 7 Carlos Garcia Campos 2017-07-11 06:40:11 PDT
Created attachment 315102 [details]
Patch
Comment 8 Michael Catanzaro 2017-07-11 08:37:18 PDT
Comment on attachment 315102 [details]
Patch

Much nicer! Thanks, Said.
Comment 9 Carlos Garcia Campos 2017-07-12 03:58:40 PDT
Committed r219389: <http://trac.webkit.org/changeset/219389>