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.

Created attachment 54802 [details]
patch file

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.

Comment on attachment 54802 [details]
patch file

Clearing flags on attachment: 54802

Committed r58744: <http://trac.webkit.org/changeset/58744>

All reviewed patches have been landed.  Closing bug.

(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.

Revision r58744 cherry-picked into qtwebkit-2.0 with commit de1558931f4390d32c452616d60046de08315a6f

We need to reopen this bug since it can be reproduced on the latest trunk build.

(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

(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

Created attachment 76282 [details]
patch

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.

(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

(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?).

(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.

Created attachment 76372 [details]
Gif loop count information (ms gif animator)

Created attachment 76373 [details]
non-animation gif

Created attachment 76374 [details]
Gif whose loop count is 1

(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;
 }

(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).

(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))

(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:)

Created attachment 77688 [details]
Add test for GIF which anmiate 2 times

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>

All reviewed patches have been landed.  Closing bug.