Animated GIFs loop before they have finished decoding

VERIFIED FIXED in Firefox 27

Status

()

Core
ImageLib
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: billm)

Tracking

({regression})

Trunk
mozilla29
x86
Windows XP
regression
Points:
---

Firefox Tracking Flags

(firefox26 wontfix, firefox27 fixed, firefox28 fixed, firefox29 verified)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
I happened to scroll down a page to reveal a large (10MB) animated GIF. At this point the GIF had not even started decoding (I didn't have the network panel open so I don't know whether it had been downloaded). As it decoded, the already decoded frames would loop (so each loop showed slightly more). It was only once the GIF had fully decoded that I was able to watch it correctly. (By comparison Internet Explorer simply plays the GIF slowly as it downloads and/or decodes.)

You can find the GIF itself at http://cheezburger.com/7836871424 although that page will open with the GIF already in view; I don't know whether that uses a different code path. (The page I was using varies over time so is unsuitable.) Note: my build is 10 days old so apologies if this has since been fixed.
Taking this.
Assignee: nobody → seth

Comment 2

5 years ago
It looks like this is a duplicate of Bug 762752 (as is Bug 909257), though that one is unclaimed and this one may have the clearer title.

All 3 describe the same behavior of large GIF images beginning to play back before they have completely downloaded/decoded.

Comment 3

5 years ago
Yes, they both look like duplicates. I'm going to forward dupe them to here, since this bug is assigned.

Updated

5 years ago
Duplicate of this bug: 762752

Updated

5 years ago
Duplicate of this bug: 909257

Comment 6

5 years ago
This affects a significant amount of content on Tumblr and the like (meme sites, imgur, etc) and makes viewing animated gifs on mobile connections pretty frustrating (as well as being inconsistent with all prior releases and other browsers). Any progress on this?
The behavior change is due to a recent decision to stop playing GIFs as fast as possible and instead pay better attention to the actual timeline.  And, synchronizing multiple ones to improve performance.  The side effect are some of these regressions, as some of the existing behavior was counting on the opposite assumptions.  Any interest in contributing to the solution?
Created attachment 8344130 [details] [diff] [review]
gif-patch

One benefit of my crappy internet connection is that it's really easy to reproduce this bug.

When we're loading the GIF, new frames are only added as they come in from the network. So if we've received only the first 10 frames out of a total of 20, GetNumFrames() will return 10. Consequently, the mFrameBlender.RawGetFrame(nextFrameIndex) check in the patch will return NULL even though we haven't reached the end of the GIF.
Assignee: seth → wmccloskey
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8344130 - Flags: review?(seth)
Comment on attachment 8344130 [details] [diff] [review]
gif-patch

Review of attachment 8344130 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/FrameAnimator.cpp
@@ +90,5 @@
>    // If we aren't, we only display fully-downloaded frames; everything else
>    // gets delayed.
>    bool needToWait = !mDoneDecoding &&
> +                    (!mFrameBlender.RawGetFrame(nextFrameIndex) ||
> +                     !mFrameBlender.RawGetFrame(nextFrameIndex)->ImageComplete());

Looks good, Bill!

I think the reason this bug wasn't blindingly obvious in the original code is because of excessive negation. Let's apply De Morgan's laws a bit here and rewrite this to be:

```
  bool canDisplay = mDoneDecoding ||
                    (mFrameBlender.RawGetFrame(nextFrameIndex) &&
                     mFrameBlender.RawGetFrame(nextFrameIndex)->ImageComplete());

  if (!canDisplay) {
    // Uh oh ...
    return ret;
  } else {
    ...
```

To me, this formulation seems much clearer.
(Reporter)

Comment 10

5 years ago
(In reply to Seth Fowler from comment #9)
>   if (!canDisplay) {
>     // Uh oh ...
>     return ret;
>   } else {
>     ...
> ```
> 
> To me, this formulation seems much clearer.

Except for the else after return, of course ;-)
Created attachment 8345505 [details] [diff] [review]
gif-fix v2

Wasn't sure if you meant to r+ this. Here's the updated patch.
Attachment #8344130 - Attachment is obsolete: true
Attachment #8344130 - Flags: review?(seth)
Attachment #8345505 - Flags: review?(seth)
Created attachment 8345506 [details] [diff] [review]
gif-fix v3

Oops. That had some extra junk in it.
Attachment #8345505 - Attachment is obsolete: true
Attachment #8345505 - Flags: review?(seth)
Attachment #8345506 - Flags: review?(seth)
Comment on attachment 8345506 [details] [diff] [review]
gif-fix v3

Review of attachment 8345506 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice!
Attachment #8345506 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/d9f6b387a49a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Duplicate of this bug: 949936
Comment on attachment 8345506 [details] [diff] [review]
gif-fix v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 717872?
User impact if declined: Weird behavior for animated GIFs
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Low, it's a simple fix.
String or IDL/UUID changes made by this patch: None.
Attachment #8345506 - Flags: approval-mozilla-beta?
Attachment #8345506 - Flags: approval-mozilla-aurora?
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → fixed
Attachment #8345506 - Flags: approval-mozilla-beta?
Attachment #8345506 - Flags: approval-mozilla-beta+
Attachment #8345506 - Flags: approval-mozilla-aurora?
Attachment #8345506 - Flags: approval-mozilla-aurora+
This has conflicts on Aurora. Please post a branch-specific patch for uplift.
status-firefox26: affected → wontfix
Flags: needinfo?(wmccloskey)
Keywords: branch-patch-needed
Created attachment 8348293 [details] [diff] [review]
animated-gifs patch for branches

This patch should work on Aurora and Beta.
Flags: needinfo?(wmccloskey)
https://hg.mozilla.org/releases/mozilla-aurora/rev/2c962cec8c3c
https://hg.mozilla.org/releases/mozilla-beta/rev/df7c72278682
status-firefox27: affected → fixed
status-firefox28: affected → fixed
Keywords: branch-patch-needed
Neil, please verify this is fixed in the latest Firefox 27 (beta), 28 (aurora), and 29 (nightly) builds.
Flags: needinfo?(neil)
(Reporter)

Comment 22

5 years ago
I couldn't get the animated GIF from my original comment to load. I kept on finding GIFs that demonstrated the problem in a 26 build only to find that somehow they weren't big enough to demonstrate the problem in an unpatched 29 build. (This is in a private window so there shouldn't have been any caching to worry about.) Eventually I found http://www.pretto.com/funny4.gif which clearly demonstrated the problem in an unpatched 29 build but not in patched 27 or 27 builds. I didn't go to the trouble of trying an unpatched 27 or 28 build though.
Flags: needinfo?(neil)
Thanks for trying, Neil.
Status: RESOLVED → VERIFIED
status-firefox29: fixed → verified
Whiteboard: [qa-]

Updated

4 years ago
Duplicate of this bug: 953233
You need to log in before you can comment on or make changes to this bug.