Bug 36818

Summary:

[Qt] Animated GIF images does not animate 10x as expected by default.

Product:

WebKit

Reporter:

nokiabugz

Component:

WebKit Qt

Assignee:

Jay Tucker <jay.tucker>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, hausmann, jay.tucker, max.hong.shen, pkasting

Priority:

Keywords:

Version:

528+ (Nightly build)

Hardware:

S60 Hardware

OS:

S60 3rd edition

Attachments:

Description	Flags
Animated GIF	none
patch file	none
patch	none
Gif loop count information (ms gif animator)	none
non-animation gif	none
Gif whose loop count is 1	none
Add test for GIF which anmiate 2 times	none

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> Test 119.01.40 smily face should display 10 times <img alt="200 B" src="10times.gif" align="bottom" hspace="0" vspace="0"/> </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	Details Formatted Diff Diff
patch (1.50 KB, patch) 2010-12-10 16:24 PST, Yi Shen	no flags	Details Formatted Diff Diff
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	Details Formatted Diff Diff
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

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.