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

VERIFIED FIXED in Firefox 25

Status

()

Core
ImageLib
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: milan)

Tracking

(Blocks: 1 bug, {regression})

25 Branch
mozilla26
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox24 unaffected, firefox25+ verified, firefox26 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
Keywords: regressionwindow-wanted

Comment 1

5 years ago
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.
Blocks: 717872
Keywords: regressionwindow-wanted → regression

Updated

5 years ago
status-firefox24: --- → unaffected
status-firefox25: --- → affected
tracking-firefox25: --- → ?
Version: unspecified → 25 Branch

Comment 2

5 years ago
I noticed this too and thought it was an intentional change (I kind of like it).
(Reporter)

Comment 3

5 years ago
(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.
tracking-firefox25: ? → +
(Assignee)

Comment 5

5 years ago
Actually, this seems to work in the latest nightly.  Anyone wants to claim credit?
(Assignee)

Comment 6

5 years ago
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.

Comment 7

5 years ago
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)
(Reporter)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
Created attachment 789862 [details] [diff] [review]
Stop using FrameAnimator (behind #ifdef) getting back to the original code.

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

Comment 11

5 years ago
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+
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Pushed with the bug # in the commit message fixed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e054c33d3417
Keywords: checkin-needed
(Assignee)

Comment 14

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/e054c33d3417
Assignee: nobody → milan
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Blocks: 905678
(Assignee)

Updated

5 years ago
Duplicate of this bug: 902961
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.
(Assignee)

Comment 18

5 years ago
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)

Updated

5 years ago
Depends on: 909258
(Reporter)

Comment 20

5 years ago
I filed bug 909635 for a different gif playback issue.
Hey Milan, worth uplifting to Aurora 25?
Flags: needinfo?(milan)
(Assignee)

Comment 22

5 years ago
(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)
(Assignee)

Comment 24

5 years ago
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?
(Assignee)

Comment 25

5 years ago
Created attachment 806107 [details] [diff] [review]
Beta (25) patch.  Trivial rebase, just line numbers.  Carry r=bgirard
Attachment #806107 - Flags: review+
Flags: needinfo?(milan)
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+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-beta/rev/ef213b06605e
status-firefox25: affected → fixed
status-firefox26: --- → fixed
Keywords: checkin-needed
Keywords: verifyme

Comment 28

5 years ago
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).
status-firefox25: fixed → verified

Comment 29

5 years ago
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
status-firefox26: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.