RESOLVED FIXED Bug 36818
[Qt] Animated GIF images does not animate 10x as expected by default. 
https://bugs.webkit.org/show_bug.cgi?id=36818
Summary [Qt] Animated GIF images does not animate 10x as expected by default.
nokiabugz
Reported 2010-03-30 06:20:57 PDT
Created attachment 52027 [details] Animated GIF STEPS TO REPRODUCE: The code below loads an animated GIF that consists of 2 frames; and each image in the animation should be displayed 10 times and stop (the smiley face will be shown 10 times and stop) ACTUAL RESULTS: The smiley face is displayed only 8 times. EXPECTED RESULTS: Verify the browser remains stable and she smiley face is shown 10 times. CODE: <html> <head> <title>10_iterations</title> </head> <body> <p> Test 119.01.40<br/><br/> </p> <p> smily face should display 10 times<br/><br/> </p> <p> <img alt="200 B" src="10times.gif" align="bottom" hspace="0" vspace="0"/> </p> </body> </html>
Attachments
Animated GIF (1.22 KB, image/gif)
2010-03-30 06:20 PDT, nokiabugz 		no flags
Details
patch file (4.43 KB, patch)
2010-04-30 07:47 PDT, Jay Tucker 		no flags
DetailsFormatted DiffDiff
patch (1.50 KB, patch)
2010-12-10 16:24 PST, Yi Shen 		no flags
DetailsFormatted DiffDiff
Gif loop count information (ms gif animator) (111.84 KB, image/jpeg)
2010-12-13 05:27 PST, Yi Shen 		no flags
Details
non-animation gif (1.60 KB, image/gif)
2010-12-13 05:28 PST, Yi Shen 		no flags
Details
Gif whose loop count is 1 (2.26 KB, image/gif)
2010-12-13 05:29 PST, Yi Shen 		no flags
Details
Add test for GIF which anmiate 2 times (4.66 KB, patch)
2010-12-30 10:38 PST, Yi Shen 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Jay Tucker
Comment 1 2010-04-30 07:31:21 PDT
The report is somewhat invalid. The original attached animated GIF states in its header that the repetition count is 9, not 10. (See http://www.netsw.org/graphic/bitmap/formats/gif/gifmerge/docs/gifabout.htm for details on GIF header format.) The bug is still valid: 8 != 9.
Jay Tucker
Comment 2 2010-04-30 07:47:24 PDT
Created attachment 54802 [details] patch file
Holger Freyther
Comment 3 2010-05-03 21:53:44 PDT
Comment on attachment 54802 [details] patch file > =================================================================== > --- WebCore/platform/graphics/qt/ImageDecoderQt.cpp (revision 58574) > +++ WebCore/platform/graphics/qt/ImageDecoderQt.cpp (working copy) > @@ -123,12 +123,14 @@ int ImageDecoderQt::repetitionCount() co > // Qt | WebCore | description > // -1 | 0 | infinite animation > // 0 | cAnimationLoopOnce | show every frame once > - // n | n | no idea if that is supported > + // n | n+1 | Qt returns the # of iterations - 1 Okay, as a follow up patch for Qt. Could you verify that MNG and GIF behave the same in Qt? In the past I was adding unit tests to Qt to verify the Qt column. I could not test loop n-times as GIMP didn't allow to save an image like this.
WebKit Commit Bot
Comment 4 2010-05-04 02:15:14 PDT
Comment on attachment 54802 [details] patch file Clearing flags on attachment: 54802 Committed r58744: <http://trac.webkit.org/changeset/58744>
WebKit Commit Bot
Comment 5 2010-05-04 02:15:19 PDT
All reviewed patches have been landed. Closing bug.
Peter Kasting
Comment 6 2010-05-28 11:17:16 PDT
(In reply to comment #1) > The report is somewhat invalid. The original attached animated GIF states in its header that the repetition count is 9, not 10. (See http://www.netsw.org/graphic/bitmap/formats/gif/gifmerge/docs/gifabout.htm for details on GIF header format.) The bug is still valid: 8 != 9. Actually, the originally attached GIF is correct. A loop count of 9 means "show 10 cycles". See bug 39857. Jay or Holger, can you please check after I land that patch whether Qt behaves correctly (per the sentence above)? It may be that either ImageDecoderQt or the Qt image decoding routines themselves will need a further patch to fix bug 39857.
Simon Hausmann
Comment 7 2010-06-16 01:31:35 PDT
Revision r58744 cherry-picked into qtwebkit-2.0 with commit de1558931f4390d32c452616d60046de08315a6f
Yi Shen
Comment 8 2010-12-10 13:42:09 PST
We need to reopen this bug since it can be reproduced on the latest trunk build.
Yi Shen
Comment 9 2010-12-10 13:54:17 PST
(In reply to comment #8) > We need to reopen this bug since it can be reproduced on the latest trunk build. Test on linux with qt 4.7.1
Yi Shen
Comment 10 2010-12-10 13:55:17 PST
(In reply to comment #9) > (In reply to comment #8) > > We need to reopen this bug since it can be reproduced on the latest trunk build. > > Test on linux with qt 4.7.1 Sorry, forgot the test page http://TestSuite.nokia-boston.com/content/Animated_GIF/animated_gifs_40v00/anim_gifs_40v00.html
Yi Shen
Comment 11 2010-12-10 16:24:01 PST
Created attachment 76282 [details] patch
Peter Kasting
Comment 12 2010-12-10 16:27:22 PST
It would be preferable to fix the consumer of repetitionCount() to have the same understanding everywhere else in WebKit does, compared to making Qt's repetitionCount() mean something slightly different -- that's just asking for subtle bugs later.
Yi Shen
Comment 13 2010-12-12 20:49:18 PST
(In reply to comment #12) > It would be preferable to fix the consumer of repetitionCount() to have the same understanding everywhere else in WebKit does, compared to making Qt's repetitionCount() mean something slightly different -- that's just asking for subtle bugs later. Thanks, I think there is a Qt bug related to the repetition count, which needed to be fixed first. Looks like the change for http://bugreports.qt.nokia.com/browse/QTBUG-7037 is incorrect, since 4.6.2, the QImageReader::loopCount for gif images is returning 0 for animated gif whose loop count is 1 And non-animated gif. In stead of initializing loopCnt with '1', it should be initialized to -1. Then QImageReader::loopCount returns -2 for non loop, -1 for infinite loop, and 0 for 1 loop, n-1 for n loop. After fix Qt's bug, we can address ImageDecoderQt::repetitionCount(). Please let me know your thoughts. Thanks
Peter Kasting
Comment 14 2010-12-12 22:39:03 PST
(In reply to comment #13) > Then QImageReader::loopCount returns -2 for non loop, -1 for infinite loop, and 0 for 1 loop, n-1 for n loop. I can't figure out how to see the change on the bug you mention, but this sounds offhand like the correct end state, so if you make Qt do/expect that, I would imagine you'd get correct behavior out of the existing WebKit code (I think?).
Yi Shen
Comment 15 2010-12-13 05:25:42 PST
(In reply to comment #14) > (In reply to comment #13) > > Then QImageReader::loopCount returns -2 for non loop, -1 for infinite loop, and 0 for 1 loop, n-1 for n loop. > > I can't figure out how to see the change on the bug you mention, but this sounds offhand like the correct end state, so if you make Qt do/expect that, I would imagine you'd get correct behavior out of the existing WebKit code (I think?). The problem is that in the QGifHandler the loopCnt is now initialized to 1 and the loopCount implementation is doing loopCnt - 1; For the no animation loopCnt will never be set from within the decoding, which means QImageReader::loopCount returns 0 (1 - 1) for no animation. See my attached test gif.
Yi Shen
Comment 16 2010-12-13 05:27:47 PST
Created attachment 76372 [details] Gif loop count information (ms gif animator)
Yi Shen
Comment 17 2010-12-13 05:28:42 PST
Created attachment 76373 [details] non-animation gif
Yi Shen
Comment 18 2010-12-13 05:29:46 PST
Created attachment 76374 [details] Gif whose loop count is 1
Yi Shen
Comment 19 2010-12-13 05:37:57 PST
(In reply to comment #14) > (In reply to comment #13) > > Then QImageReader::loopCount returns -2 for non loop, -1 for infinite loop, and 0 for 1 loop, n-1 for n loop. > > I can't figure out how to see the change on the bug you mention, but this sounds offhand like the correct end state, so if you make Qt do/expect that, I would imagine you'd get correct behavior out of the existing WebKit code (I think?). The existing WebKit code may still need to be changed since Qt may still want to return loop count as real_loop_count - 1. The change for ImageDecoderQt::repetitionCount may look like following (increase the loop count by one except it is infinite loop or non-animation) int ImageDecoderQt::repetitionCount() const { - if (m_reader && m_reader->supportsAnimation()) + if (m_reader && m_reader->supportsAnimation()) { m_repetitionCount = m_reader->loopCount(); + if ((m_repetitionCount != cAnimationLoopInfinite) && (m_repetitionCount != cAnimationNone)) + ++m_repetitionCount; + } return m_repetitionCount; }
Peter Kasting
Comment 20 2010-12-13 19:08:37 PST
(In reply to comment #19) > The existing WebKit code may still need to be changed since Qt may still want to return loop count as real_loop_count - 1. > > The change for ImageDecoderQt::repetitionCount may look like following (increase the loop count by one except it is infinite loop or non-animation) Your paste looks exactly like the patch file that I initially responded negatively to (see comment 12).
Yi Shen
Comment 21 2010-12-14 05:13:06 PST
(In reply to comment #20) > (In reply to comment #19) > > The existing WebKit code may still need to be changed since Qt may still want to return loop count as real_loop_count - 1. > > > > The change for ImageDecoderQt::repetitionCount may look like following (increase the loop count by one except it is infinite loop or non-animation) > > Your paste looks exactly like the patch file that I initially responded negatively to (see comment 12). The difference is that my proposed new patch will increase repetition count for animation loop once gif, see old, + if ((m_repetitionCount != cAnimationLoopInfinite) && (m_repetitionCount != cAnimationLoopOnce)) new, + if ((m_repetitionCount != cAnimationLoopInfinite) && (m_repetitionCount != cAnimationNone))
Yi Shen
Comment 22 2010-12-14 07:07:46 PST
(In reply to comment #20) > (In reply to comment #19) > > The existing WebKit code may still need to be changed since Qt may still want to return loop count as real_loop_count - 1. > > > > The change for ImageDecoderQt::repetitionCount may look like following (increase the loop count by one except it is infinite loop or non-animation) > > Your paste looks exactly like the patch file that I initially responded negatively to (see comment 12). Also, Peter, I understand your point. We will also try to fix everything in the Qt and leave webkit no change if possible. Thanks for your comments:)
Yi Shen
Comment 23 2010-12-30 10:38:33 PST
Created attachment 77688 [details] Add test for GIF which anmiate 2 times
WebKit Commit Bot
Comment 24 2011-01-05 09:27:23 PST
Comment on attachment 77688 [details] Add test for GIF which anmiate 2 times Clearing flags on attachment: 77688 Committed r75071: <http://trac.webkit.org/changeset/75071>
WebKit Commit Bot
Comment 25 2011-01-05 09:27:29 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.