Last Comment Bug 641198 - Rollover animated gif show only one frame.
: Rollover animated gif show only one frame.
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 All
: -- normal with 1 vote (vote)
: ---
Assigned To: Alon Zakai (:azakai)
:
Mentors:
http://www.riflessivo.eu/intro/home_i...
Depends on: 1120144
Blocks: 646987 359608 644855
  Show dependency treegraph
 
Reported: 2011-03-12 03:19 PST by Mario
Modified: 2015-01-10 18:44 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This test does not involve JS (701 bytes, text/html)
2011-03-12 03:20 PST, Mario
no flags Details
patch (665 bytes, patch)
2011-03-18 17:00 PDT, Alon Zakai (:azakai)
no flags Details | Diff | Review
patch v2 (4.64 KB, patch)
2011-03-22 14:35 PDT, Alon Zakai (:azakai)
joe: review+
Details | Diff | Review

Description Mario 2011-03-12 03:19:15 PST
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0) Gecko/20100101 Firefox/4.0

Starting from the first FF4b I installed this problem is present. This animation works fine on ALL other browser (including FF3) on all platform (WIN, MAC and LINUX). When you use 2 animated gif, with rollover, they animate the first time and then you only see ONE frame for all other over and out activity.

Reproducible: Always




Attached is a simple testcase (no javascript involved) that works in Fx3.6 and other but not in Fx4

Rollover gif should animate not only show first frame.
Comment 1 Mario 2011-03-12 03:20:26 PST
Created attachment 518931 [details]
This test does not involve JS
Comment 2 Mario 2011-03-12 03:23:34 PST
The first beta I installed was FF4b11. To try a rollback I downloaded some older versions. I installed 4b3 pre and all animations work fine except for the one on the right top, where there are 2 animations, one in the background and one in the foreground. Then I installed 4b6pre and animations work ONLY for the hover activity, when I move the mouse out I only see the first frame of the animated gif. Then I was not able to find other releases to install... I confirm again that "my site" works perfectly with all other browser I tested (FF3 up to the last 3.6.15 - Opera, IE 7 - IE 8 - Safari - Chrome) on different OS (WINXP - WIN 7 - VISTA - OSX - LINUX) where the browsers work.
Comment 3 Alice0775 White 2011-03-12 03:51:18 PST
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/75d2145d1bd3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100907 Firefox/4.0b6pre ID:20100907194131
Fails:
http://hg.mozilla.org/mozilla-central/rev/4bb022d84a31
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100907 Firefox/4.0b6pre ID:20100907182227
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75d2145d1bd3&tochange=4bb022d84a31

Suspected bug:
Bug 359608 - Animated GIFs are animated even when user navigates to another page
Comment 4 Alice0775 White 2011-03-12 17:00:42 PST
In local build(from m-c repository),
build from fd33aa85de93 : fails
build from d35c0d22fa71 : works
build from 459a07b6bdee : works
build from 75d2145d1bd3 : works

Triggered by:
fd33aa85de93	Alon Zakai — Bug 359608 - Animated GIFs are animated even when user navigates to another page.r=bholley,bz;sr=bz;a=blocker
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-13 13:04:44 PDT
Alon, can you take a look at this please?
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-13 13:06:57 PDT
I'm guessing the issue is that the nsImageLoader::OnStartContainer call to StartAnimation would restart the animation from the beginning if it had already finished.

The new setup doesn't seem to have a way of restarting finite animations: it assumes all animations are infinite....
Comment 7 Alon Zakai (:azakai) 2011-03-14 13:06:45 PDT
I'll take a look at this very soon.
Comment 8 Alon Zakai (:azakai) 2011-03-14 15:38:05 PDT
On Chrome 10.0.648.114 beta, I don't see animations with the test page. Only a single frame changes when hovering.

FF4 also shows a single frame change on hover, but the opposite of the frames Chrome shows.

So I am confused. Are both browsers wrong here?
Comment 9 Mario 2011-03-15 00:00:02 PDT
The site 
http://www.riflessivo.eu/intro/home_en.html
(which involve preloading JS) works perfectly with chrome (last official release).
The test file only show the last frame of animations that is, at least, a better behaviour compared to FF4. In this case we loose the animation but we can see the open and close actions correctly. Please see also the site that is more complete than the test html.
Comment 10 Josh Matthews [:jdm] 2011-03-15 00:37:42 PDT
Sorry, didn't mean to clear the assignee.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-15 06:45:05 PDT
Alon, Chrome shows the _last_ frame of the animation once the animation is one.

It looks like Firefox 4 shows the _first_ frame of the animation.

Arguably both browsers are "wrong" (in that Fx 3.6 and Opera show the whole animation), but Firefox 4's behavior seems _very_ wrong to me.
Comment 12 Alon Zakai (:azakai) 2011-03-18 17:00:42 PDT
Created attachment 520359 [details] [diff] [review]
patch

I believe it makes sense to reset mAnimationFinished in ResetAnimation(). After all, if we are resetting the animation, presumably we want it to start again even if it already finished.

This fixes the no-JS testcase here and the website itself.

I am pushing to try.
Comment 13 Alon Zakai (:azakai) 2011-03-19 10:16:19 PDT
Comment on attachment 520359 [details] [diff] [review]
patch

Looks good on try, asking for review.
Comment 14 Mario 2011-03-20 02:13:43 PDT
Hi, I would like to test it but I do not know how.. I'm sorry... I'm not a developer. Anyway, let me thanks Alon for solving this issue... I hope I'll see it in next RC.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-20 14:01:59 PDT
Mario, there is no next rc planned; final release of Firefox 4 will be this Tuesday pending critical stop-ship bugs (which this bug is not).

Assuming the patch is correct it will show up in Firefox 5 in a few months.
Comment 16 Alon Zakai (:azakai) 2011-03-21 13:37:11 PDT
Comment on attachment 520359 [details] [diff] [review]
patch

Found a bug while writing a test.
Comment 17 Alon Zakai (:azakai) 2011-03-22 14:35:51 PDT
Created attachment 521036 [details] [diff] [review]
patch v2

Better patch, with test.
Comment 18 Joe Drew (not getting mail) 2011-03-30 13:44:49 PDT
Comment on attachment 521036 [details] [diff] [review]
patch v2

>diff --git a/modules/libpr0n/src/RasterImage.cpp b/modules/libpr0n/src/RasterImage.cpp

>-  if (ShouldAnimate())
>+  if (ShouldAnimate()) {
>     StartAnimation();
>+    // The animation may not have been running before, if mAnimationFinished
>+    // was false (before we changed it to true in this function). So, mark the
>+    // animation as running.
>+    mAnimating = PR_TRUE;
>+  }

I almost feel like we should somehow use EvaluateAnimation or something like it here. Maybe we can unconditionally stop animation and then evaluate? It would help a lot if StopAnimation/StartAnimating set mAnimating....

>diff --git a/modules/libpr0n/test/reftest/gif/test_bug641198.html b/modules/libpr0n/test/reftest/gif/test_bug641198.html

>+    document.getElementById("animated").setAttribute("class", "animated" + ((counter % 2)+1));
>+    setTimeout(doTimeout, counter == 3 ? 500 : 250); // Wait a bit more for the last one, to prevent oranges
>+  }
>+  counter++;

This test is just *definitely* going to cause intermittent oranges, I know it :(

Ehsan and I tried coming up with a MozAfterPaint method of testing animated images, but it can't be relied upon *either*. I think our only option for reliable animated image tests is to use imgILoader::LoadImage directly like the libpr0n unit tests do.
Comment 19 Alon Zakai (:azakai) 2011-03-30 14:24:27 PDT
> 
> >diff --git a/modules/libpr0n/test/reftest/gif/test_bug641198.html b/modules/libpr0n/test/reftest/gif/test_bug641198.html
> 
> >+    document.getElementById("animated").setAttribute("class", "animated" + ((counter % 2)+1));
> >+    setTimeout(doTimeout, counter == 3 ? 500 : 250); // Wait a bit more for the last one, to prevent oranges
> >+  }
> >+  counter++;
> 
> This test is just *definitely* going to cause intermittent oranges, I know it
> :(
> 
> Ehsan and I tried coming up with a MozAfterPaint method of testing animated
> images, but it can't be relied upon *either*. I think our only option for
> reliable animated image tests is to use imgILoader::LoadImage directly like the
> libpr0n unit tests do.

I pushed this several times to try (I had other problems to work out), and I never saw an orange due to this. So while it is a risk for oranges, I at least think such oranges will be very rare if they do occur.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-30 18:25:03 PDT
> This test is just *definitely* going to cause intermittent oranges

The test also only tests that the image ends up showing the last frame; not that it animates, right?

And it takes a full second to run?
Comment 21 Alon Zakai (:azakai) 2011-03-30 18:35:28 PDT
Yes, it's a reftest, so only the last frame matters. It does test that it animated properly, though, at least in the sense that the last frame is shown and not the first (which is enough to catch the problem in this bug).

It takes a second, yes. I can make it shorter, but am worried about higher risk of oranges. Do you think it is safe to do that?
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-30 18:41:13 PDT
I wonder whether it's better to just do a chrome test and directly listen for the notifications the animation sends....
Comment 23 Alon Zakai (:azakai) 2011-03-31 09:41:35 PDT
We do have some browser chrome tests for animations, yeah, but they have limitations. I believe the safest thing regarding ensuring correct results is an actual reftest. But yes, this approach has definite downsides too...
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-31 09:49:52 PDT
What makes an actual reftest better?

My suggestion was to use a chrome (not browser chrome) test which uses the privileges it has to install an animation listener and then uses those notifications to decide when to end the test.  Then at test end you can screenshot and compare just like reftest does to make sure that no only did the animation happen but you ended up with the right bits.
Comment 25 Alon Zakai (:azakai) 2011-03-31 10:15:41 PDT
Sounds interesting, is there an example of a similar test I can take a look at?

2nd question, should I not land this meanwhile? Or file a followup bug to replace the test with a better one?
Comment 26 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-03-31 10:37:41 PDT
Landing this in the meantime sounds fine.

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/test_bug451286.xul might be ok to crib.  Or the various things in docshell/test/chrome that include WindowSnapshot.js.
Comment 27 Alon Zakai (:azakai) 2011-03-31 14:09:58 PDT
http://hg.mozilla.org/mozilla-central/rev/be4ecb32b875

I will file a followup bug for an improved test.
Comment 28 Alon Zakai (:azakai) 2011-03-31 14:20:21 PDT
Filed bug 646987 for followup to improve the test.

Note You need to log in before you can comment on or make changes to this bug.