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)
Tracking
()
VERIFIED
FIXED
mozilla26
Tracking | Status | |
---|---|---|
firefox24 | --- | unaffected |
firefox25 | + | verified |
firefox26 | --- | verified |
People
(Reporter: BenWa, Assigned: milan)
References
Details
(Keywords: regression)
Attachments
(2 files)
23.72 KB,
patch
|
BenWa
:
review+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
23.72 KB,
patch
|
milan
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Keywords: regressionwindow-wanted
![]() |
||
Comment 1•12 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: omtagif
Keywords: regressionwindow-wanted → regression
![]() |
||
Updated•12 years ago
|
status-firefox24:
--- → unaffected
status-firefox25:
--- → affected
tracking-firefox25:
--- → ?
Version: unspecified → 25 Branch
Comment 2•12 years ago
|
||
I noticed this too and thought it was an intentional change (I kind of like it).
Reporter | ||
Comment 3•12 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.
Assignee | ||
Comment 5•12 years ago
|
||
Actually, this seems to work in the latest nightly. Anyone wants to claim credit?
Assignee | ||
Comment 6•12 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•12 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•12 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•12 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•12 years ago
|
||
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•12 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•12 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•12 years ago
|
Keywords: checkin-needed
Comment 13•12 years ago
|
||
Pushed with the bug # in the commit message fixed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e054c33d3417
Keywords: checkin-needed
Assignee | ||
Comment 14•12 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.
Comment 15•12 years ago
|
||
Assignee: nobody → milan
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Target Milestone: --- → mozilla26
Comment 17•12 years ago
|
||
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•12 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)
Comment 19•12 years ago
|
||
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)
Reporter | ||
Comment 20•12 years ago
|
||
I filed bug 909635 for a different gif playback issue.
Assignee | ||
Comment 22•12 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 23•12 years ago
|
||
Comment 1 and the status-firefox25 flag suggest otherwise. Are those wrong?
Flags: needinfo?(milan)
Assignee | ||
Comment 24•12 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•12 years ago
|
||
Attachment #806107 -
Flags: review+
Flags: needinfo?(milan)
Comment 26•12 years ago
|
||
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•12 years ago
|
Keywords: checkin-needed
Comment 27•12 years ago
|
||
Comment 28•12 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).
![]() |
||
Comment 29•12 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
You need to log in
before you can comment on or make changes to this bug.
Description
•