Bug 26759

Summary: GIFImageDecoder is broken
Product: WebKit Reporter: Brett Wilson (Google) <brettw>
Component: PlatformAssignee: Brett Wilson (Google) <brettw>
Status: RESOLVED FIXED    
Severity: Normal CC: creshal, jmalonzo, mculpepper, mrobinson, pkasting
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
levin: review-
Patch v2
none
Patch v3 levin: review+

Brett Wilson (Google)
Reported 2009-06-26 12:38:06 PDT
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.
Attachments
Patch (1.22 KB, patch)
2009-06-26 12:56 PDT, Brett Wilson (Google) 		levin: review-
DetailsFormatted DiffDiff
Patch v2 (2.57 KB, patch)
2009-06-26 14:35 PDT, Brett Wilson (Google) 		no flags
DetailsFormatted DiffDiff
Patch v3 (2.10 KB, patch)
2009-06-26 15:24 PDT, Brett Wilson (Google) 		levin: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2009-06-26 12:51:00 PDT
I'm pretty sure this is a dupe. Peter is your man.
Brett Wilson (Google)
Comment 2 2009-06-26 12:56:14 PDT
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 3 2009-06-26 13:07:08 PDT
Comment on attachment 31944 [details] Patch Won't this break ImageDecoderQt which does have the "const" on this member?
Peter Kasting
Comment 4 2009-06-26 14:19:41 PDT
I think the right thing here is to make repetitionCount() const everywhere and make GIFImageDecoder::m_repetitionCount mutable.
Brett Wilson (Google)
Comment 5 2009-06-26 14:35:23 PDT
Created attachment 31950 [details] Patch v2 Indeed it does.
Brett Wilson (Google)
Comment 6 2009-06-26 14:37:03 PDT
(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
Comment 7 2009-06-26 14:39:22 PDT
(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
Comment 8 2009-06-26 15:15:06 PDT
*** Bug 26657 has been marked as a duplicate of this bug. ***
Brett Wilson (Google)
Comment 9 2009-06-26 15:24:27 PDT
Created attachment 31954 [details] Patch v3
Peter Kasting
Comment 10 2009-06-26 15:27:15 PDT
(In reply to comment #9) > Created an attachment (id=31954) [review] > Patch v3 Thumbs up from me!
David Levin
Comment 11 2009-06-26 15:33:49 PDT
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
Comment 12 2009-06-26 15:40:15 PDT
Thanks much Peter for looking it over!
Brett Wilson (Google)
Comment 13 2009-06-26 16:10:38 PDT
Should be fixed in r45293
Note You need to log in before you can comment on or make changes to this bug.