Closed Bug 924366 Opened 6 years ago Closed 6 years ago

Animated GIFs loop before they have finished decoding

Categories

(Core :: ImageLib, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed
firefox29 --- verified

People

(Reporter: neil, Assigned: billm)

References

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(2 files, 2 obsolete files)

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
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.
Yes, they both look like duplicates. I'm going to forward dupe them to here, since this bug is assigned.
Duplicate of this bug: 762752
Duplicate of this bug: 909257
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?
Attached patch gif-patch (obsolete) — Splinter Review
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.
(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 ;-)
Attached patch gif-fix v2 (obsolete) — Splinter Review
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)
Attached patch gif-fix v3Splinter Review
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
Closed: 6 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?
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.
Flags: needinfo?(wmccloskey)
This patch should work on Aurora and Beta.
Flags: needinfo?(wmccloskey)
Neil, please verify this is fixed in the latest Firefox 27 (beta), 28 (aurora), and 29 (nightly) builds.
Flags: needinfo?(neil)
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
Whiteboard: [qa-]
Duplicate of this bug: 953233
You need to log in before you can comment on or make changes to this bug.