Closed Bug 899861 Opened 12 years ago Closed 12 years ago

Gif playback begins when Gif is fully downloaded, doesn't start at beginning

Categories

(Core :: Graphics: ImageLib, defect)

25 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla26
Tracking Status
firefox24 --- unaffected
firefox25 + verified
firefox26 --- verified

People

(Reporter: BenWa, Assigned: milan)

References

Details

(Keywords: regression)

Attachments

(2 files)

STR: 1) On win7 nightly. Open a gif: http://i.imgur.com/Lwl04Sz.gif 2) Shift+Reload to force a redownload Playback only begins once the gif is fully downloaded but it doesn't start at the beginning.
Regression winodw(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/5976b9c673f8 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130715 Firefox/25.0 ID:20130715211256 Bad(animated GIF won't start till fully downloaded with Shift+click reload button): http://hg.mozilla.org/mozilla-central/rev/41ed26826acb Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130716 Firefox/25.0 ID:20130716015911 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5976b9c673f8&tochange=41ed26826acb Regression winodw(m-c) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/bf847b1a3776 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130715 Firefox/25.0 ID:20130715113446 Bad(animated GIF won't start till fully downloaded with Shift+click reload button): http://hg.mozilla.org/integration/mozilla-inbound/rev/ea8d855c4edb Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20130715 Firefox/25.0 ID:20130715113946 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bf847b1a3776&tochange=ea8d855c4edb Regressed by ea8d855c4edb Joe Drew — Bug 717872 - Move all image animation logic into a new class, FrameAnimator, and use it from RasterImage. r=seth This patch moves the logic of moving from one frame to another (and tracking what frame is current, etc) to a separate class, FrameAnimator. Deciding *whether* to animate, and actually calling that animation code, is left to RasterImage, but the animation itself is driven by FrameAnimator.
Version: unspecified → 25 Branch
I noticed this too and thought it was an intentional change (I kind of like it).
(In reply to silverwind from comment #2) > I noticed this too and thought it was an intentional change (I kind of like > it). It could be turned into a feature but it would need some kind of loading progress bar and it would have to start at the first page of the animation which it doesn't always. In any case we would have to do a bit of research to decide if it's better instead of changing it unintentionally.
Most of the web will not see this as a feature :) Tracking.
Actually, this seems to work in the latest nightly. Anyone wants to claim credit?
Maybe not, I have a feeling the image got cached somewhere, so that the full download takes a lot less time, and it just becomes more difficult to tell.
Doesn't look fixed for me here either. (I'm hovering over entries at https://pay.reddit.com/r/gifs/ using https://userscripts.org/scripts/show/109262)
(In reply to Milan Sreckovic [:milan] from comment #6) > Maybe not, I have a feeling the image got cached somewhere, so that the full > download takes a lot less time, and it just becomes more difficult to tell. It's significantly more noticeable with a slower internet/remote web server. I couldn't convince myself that the issue was still there from the office.
I clearly saw it. Then tried it enough times on different versions that it stopped showing up :( I tried throttling with "network link conditioner", but that didn't help; I'll try a few other tools.
Back out the relevant parts of https://bug717872.bugzilla.mozilla.org/attachment.cgi?id=774685 from bug 717872, hiding it behind #ifdef with the plan to actually fix it in the FrameAnimator.
Attachment #789862 - Flags: review?(bgirard)
Comment on attachment 789862 [details] [diff] [review] Stop using FrameAnimator (behind #ifdef) getting back to the original code. Review of attachment 789862 [details] [diff] [review]: ----------------------------------------------------------------- Should we also disable compiling of FrameAnimator.cpp?
Attachment #789862 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #11) > Comment on attachment 789862 [details] [diff] [review] > Stop using FrameAnimator (behind #ifdef) getting back to the original code. > > Review of attachment 789862 [details] [diff] [review]: > ----------------------------------------------------------------- > > Should we also disable compiling of FrameAnimator.cpp? If we can't get the "real" fix that uses FrameAnimator, we should, and perhaps even back out FrameAnimator.{h,cpp} as well.
Pushed with the bug # in the commit message fixed. https://hg.mozilla.org/integration/mozilla-inbound/rev/e054c33d3417
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13) > Pushed with the bug # in the commit message fixed. Thanks. The old "off by 1000" error.
Assignee: nobody → milan
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment on attachment 789862 [details] [diff] [review] Stop using FrameAnimator (behind #ifdef) getting back to the original code. >+ NS_ASSERTION(aTime <= TimeStamp::Now(), >+ "Given time appears to be in the future"); I'm surprised that I'm hitting this assertion all the time in my 2-processor Windows XP VM, given that this is supposed to be a backout and I don't remember hitting it before.
Neil, setting a need info on you as I have a small request - while this is a backout of this class, a number of other changes landed in the two weeks this was enabled, perhaps in the code that was calling it or being called from it (and certainly in this class) so it may have masked the actual problem - could you open another bug with this and give us a steps to reproduce? Please CC me on the bug so that I see it right away. Thanks.
Flags: needinfo?(neil)
I'm not sure I can give you steps to reproduce, but I can give you a stack, and I can also link to the source of the image which is actually part of a theme: http://mxr.mozila.org/comm-central/source/suite/themes/classic/communicator/icons/loading.gif
Flags: needinfo?(neil)
Depends on: 909258
I filed bug 909635 for a different gif playback issue.
Hey Milan, worth uplifting to Aurora 25?
Flags: needinfo?(milan)
(In reply to Alex Keybl [:akeybl] from comment #21) > Hey Milan, worth uplifting to Aurora 25? Didn't think the original problem made it into 25.
Flags: needinfo?(milan)
Comment 1 and the status-firefox25 flag suggest otherwise. Are those wrong?
Flags: needinfo?(milan)
Comment on attachment 789862 [details] [diff] [review] Stop using FrameAnimator (behind #ifdef) getting back to the original code. Note: this is already in Aurora. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Perceived performance. Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): There are a lot of animated gifs; they will start playing later and that will look like a performance problem. String or IDL/UUID changes made by this patch: n/a
Attachment #789862 - Flags: approval-mozilla-beta?
Comment on attachment 789862 [details] [diff] [review] Stop using FrameAnimator (behind #ifdef) getting back to the original code. Think of the GIFs. a+
Attachment #789862 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: verifyme
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0 Reproduced this issue with Nightly 25.0a1 (2013-08-05). Verified as fixed with Firefox 25 beta 2 (Build ID: 20130923194050).
Mozilla/5.0 (Windows NT 6.1; rv:26.0) Gecko/20100101 Firefox/26.0 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 Verified as fixed with Aurora 26.0a2 buildID 20130925004004
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: