Bug 26759 – GIFImageDecoder is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Brett Wilson (Google)]

The GIF image decoder in WebCore/platform/image-decoders used by Chromium (and I think some other ports now) is broken. Animated GIFs do not animate.

The problem is a mismatch in the virtual function repetitionCount in ImageDecoder and the derived class, so the GIF version is not properly called.

[Eric Seidel (no email)]

I'm pretty sure this is a dupe.  Peter is your man.

[Brett Wilson (Google)]

Created attachment 31944 [details]
Patch

I didn't see any dupes.

This is
http://code.google.com/p/chromium/issues/detail?id=14805
in Chromium.

[David Levin]

Comment on attachment 31944 [details]
Patch

Won't this break ImageDecoderQt which does have the "const" on this member?

[Peter Kasting]

I think the right thing here is to make repetitionCount() const everywhere and make GIFImageDecoder::m_repetitionCount mutable.

[Brett Wilson (Google)]

Created attachment 31950 [details]
Patch v2

Indeed it does.

[Brett Wilson (Google)]

(In reply to comment #4)
> I think the right thing here is to make repetitionCount() const everywhere and
> make GIFImageDecoder::m_repetitionCount mutable.

I did that first but it seemed somehow wrong to me.

[Peter Kasting]

(In reply to comment #6)
> (In reply to comment #4)
> > I think the right thing here is to make repetitionCount() const everywhere and
> > make GIFImageDecoder::m_repetitionCount mutable.
> 
> I did that first but it seemed somehow wrong to me.

It's not wrong.  GIFImageDecoder::m_repetitionCount is a local cache, an implementation detail; and calling repetitionCount() does not trigger decoding.  Please go this route instead of your patch v1/v2 route.

[Jan Alonzo]

*** Bug 26657 has been marked as a duplicate of this bug. ***

[Brett Wilson (Google)]

Created attachment 31954 [details]
Patch v3

[Peter Kasting]

(In reply to comment #9)
> Created an attachment (id=31954) [review]
> Patch v3

Thumbs up from me!

[David Levin]

Comment on attachment 31954 [details]
Patch v3

Please add a link to the bug and the bug title in your change log.  Something like:

  https://bugs.webkit.org/show_bug.cgi?id=26759
  GIFImageDecoder is broken
  
...

[David Levin]

Thanks much Peter for looking it over!

[Brett Wilson (Google)]

Should be fixed in r45293