Closed Bug 597174 Opened 14 years ago Closed 13 years ago

Firefox 3.6.10 doesn't render gifs correctly, w/ certain system-cairo versions. Displays (flashes) only the first frame.

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Version:
1.9.2 Branch
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: firew4lker, Assigned: galtgendo)

References

Details

Attachments

(1 file)

(mostly a rollup of several backports) fix for animated gif problem
4.30 KB, patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.10) Gecko/20100916 Firefox/3.6.10
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.10) Gecko/20100916 Firefox/3.6.10

Firefox 3.6.10 doesn't render  gifs correctly. Displays (flashes) only the first frame. 

It seems that it works fine with big gifs. Small gifs though (like) smilies doesn't render ok.

E.g.:
http://i.imgur.com/hfZm5.gif
http://imgur.com/Znj5Z.gif
http://comments.deviantart.com/emoticons

Extra infos:
https://bbs.archlinux.org/viewtopic.php?id=105024
https://bugs.archlinux.org/task/20868


Reproducible: Always

Steps to Reproduce:
1. Try to open some of the following gifs.

http://i.imgur.com/hfZm5.gif
http://imgur.com/Znj5Z.gif
http://comments.deviantart.com/emoticons

Actual Results:  
It only shows the first frame of the gif and it flashes it fast.

Expected Results:  
Show a smooth animation of the gif.
Exact same problem here.
No extensions loaded. Using "nouveau" display drivers on xorg 1.9 Linux.
happens when using cairo 1.10.0. with 1.8.10 is fine
I am having the issue with cairo-lcd 1.8.10
Ignore the above. It is a cairo 1.10.0 issue (I had a look to the other laptop of mine with cairo-lcd 1.8.10).
Reports about this bug can be found in various distro bug trackers:

Fedora:
https://bugzilla.redhat.com/show_bug.cgi?id=628331

Gentoo:
http://bugs.gentoo.org/show_bug.cgi?id=337813

Mandriva:
https://qa.mandriva.com/show_bug.cgi?id=60738
I hope mozilla won't suggest to use the bundled cairo for gecko-1.9.x
I think this comment from Benjamin Otte (https://bugzilla.redhat.com/show_bug.cgi?id=628331#c19; a maintainer of Cairo) could be relevant:

> This is most likely related to the image loader loading data into a Cairo image
surface but forgetting to call cairo_surface_mark_dirty() after and/or
cairo_surface_flush() before the modifications. As Cairo 1.10 is much stricter
about enforcing these requirement than 1.8 was, it might indeed be that the bug
does only show up in Cairo 1.10.

> So I'm reassigning it back to Firefox.
Related / dup-candidate: bug 552986 .
I've noticed this bug after switching to Cairo 1.10.0. Interestingly, it shows in Firefox 3.6.12, but Firefox 4.0b7 works fine (with the same Cairo version).
I use both Slackware-current and Slackware64-current. We had a major update to -current recently and I see this problem but ONLY with Slackware64 and not with the 32-bit Slackware. GIF animations are properly display in Slackware (32-bit)

Both have been upgraded to use cairo 1.10.0. After regressing to cairo 1.8.8 in Slackware64 GIF animations display correctly. 

I tested this with firefox-4.0b7 and cairo 1.10.0 with zero problems, GIF animations are properly displayed. 

I also see this behavior in Seamonkey and Thunderbird.
Left out the Firefox version : Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) Gecko/20101028 Firefox/3.6.12
This is (more or less) a backport of 3 commits from http://hg.mozilla.org/releases/mozilla-2.0 at modules/libpr0n/ - I'm not sure if I got all, that's required
and (probably) commit fixing bug 546272 isn't really needed here, but it doesn't seem to hurt and animated gifs work.

Tested with xulrunner-1.9.2.12
Component: General → ImageLib
Product: Firefox → Core
QA Contact: general → imagelib
Attachment #494277 - Flags: review?(jmuizelaar)
Attachment #494277 - Flags: review?(jmuizelaar) → review?(joe)
Rafał, what are the bug numbers you backported?
Assignee: nobody → galtgendo
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #13)
> Created attachment 494277 [details] [diff] [review]
> fix for animated gif problem
> 
> This is (more or less) a backport of 3 commits from
> http://hg.mozilla.org/releases/mozilla-2.0 at modules/libpr0n/ - I'm not sure
> if I got all, that's required
> and (probably) commit fixing bug 546272 isn't really needed here, but it
> doesn't seem to hurt and animated gifs work.
> 
> Tested with xulrunner-1.9.2.12

I would prefer to see bug 546272 brought along but as stated it is not needed. Joe I have tested on windows mac and linux all are fine as far as official builds distrubuted by mozilla would be. I have also tested on linux for system cairo and problem is resolved.
I may have phrased it too a bit badly.
This patch came from parts of following commits:
5aadb09fdc70c05574fb759a51d505d783962789 (modules/libpr0n/src/imgFrame.cpp
 block, so only NS_OK -> NS_ERROR_NOT_AVAILABLE - the rest is already in 1.9.2.12)
58c60f220da285f1847dacc839350c1828b8e40d (except for imgContainer::WriteToDecoder, as it doesn't seem exist yet, and I skipped one assert)
9a8451dad9afd4db856839eb71354e21bfa54019 (everything but comments)
dc5674a71f286c0f68b1cdaff04c8959e797a0fc (this one is that not really related part, but as this also affects animated gifs, I've took it too)

Actually, I've produced that patch with no plans of putting it here, but as it was decided it's upstreamable, I was strongly suggested to drop it here.
(In reply to comment #14)
> Rafał, what are the bug numbers you backported?

bug 545513 546272 534787 are the actual bug numbers.
I'm trying to adapt Rafał's patch to Seamonkey as we speak, but I'm a n00b. Not a C n00b, but a Mozilla code n00b. I may need help.
Summary: Firefox 3.6.10 doesn't render gifs correctly. Displays (flashes) only the first frame. → Firefox 3.6.10 doesn't render gifs correctly, w/ certain system-cairo versions. Displays (flashes) only the first frame.
I never said I really know Mozilla code well.
It was simply that mercurial commits for xulrunner 2.0 are well documented
and changes between 1.9.2 and 2.0 in those parts of code are minimal (well, nearly).

Semonkey 2.0, on the other hand, is xulrunner 1.9.1, and just looking at the commit, that created imgFrame.cpp (http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b94bc4be53ca) makes me unsure *if* such fix can be made in 1.9.1,
much less where it should go.
Joe, are you fine with the backported patch? Or should we produce something else? Or do you suggest to nominate the Bug 545513 Bug 546272 Bug 534787 to 1.9.2 w/o backporting?
Mozilla's behavior in here is just weird to say the least.

"We don't have time to review the patch, and we won't give you permission to use it downstream either"

Why allow building against system libs if you aren't going to keep up with upstream changes ?
(In reply to comment #22)
> Mozilla's behavior in here is just weird to say the least.
> 
> "We don't have time to review the patch, and we won't give you permission to
> use it downstream either"
> 
Who has said you could not use it downstream?

> Why allow building against system libs if you aren't going to keep up with
> upstream changes ?

Mozilla uses bundled versions of the library, they expect distros to use the bundled while most distros do not, it is up to us to fix the breakage, the issue was already addressed in trunk where cairo was updated to 1.10.0.
(In reply to comment #23)
> (In reply to comment #22)
> > Mozilla's behavior in here is just weird to say the least.
> > 
> > "We don't have time to review the patch, and we won't give you permission to
> > use it downstream either"
> > 
> Who has said you could not use it downstream?
> 

From http://www.mozilla.org/foundation/trademarks/policy.html :

"If you compile Mozilla unmodified source code (including code and config files in the installer) and do not charge for it, you do not need additional permission from Mozilla to use the relevant Mozilla Mark(s) for your compiled version."

> > Why allow building against system libs if you aren't going to keep up with
> > upstream changes ?
> 
> Mozilla uses bundled versions of the library, they expect distros to use the
> bundled while most distros do not, it is up to us to fix the breakage, the
> issue was already addressed in trunk where cairo was updated to 1.10.0.

Fixing breakages is subject to the excerpt above.
(In reply to comment #22)
> Mozilla's behavior in here is just weird to say the least.
> 
> "We don't have time to review the patch"

No one has said that, AFAICT.

Here's the review feedback that I'd give, though, for what it's worth -- since the attached patch is mostly a rollup of several backports (per comment 13), I'd suggest splitting it into a separate patch-file for each backported changeset (plus an additional "not-directly-backported changes" patch if necessary).

That would make it easier to see what's different in the trunk patches vs. their backported versions, thereby making review much easier.  It also would make regression-tracking & backouts (if needed) significantly saner/easier.

> and we won't give you permission to
> use it downstream either

(Of course you have permission to apply any arbitrary patches that you like. You're just not allowed to distribute the result as "Firefox" without Mozilla's permission.)
(In reply to comment #25)
> > and we won't give you permission to
> > use it downstream either

(Note also that no one has said "we won't give you permission".  It's rather that "you need to get Mozilla's permission [in order to label a patched build as Firefox]".)

(And for what it's worth, I think the changes I suggested in Comment 25 would make the backported code much saner and the aforementioned permission and/or review much more likely to be forthcoming.)
> (Of course you have permission to apply any arbitrary patches that you like.
> You're just not allowed to distribute the result as "Firefox" without Mozilla's
> permission.)

Then you should tighten your policy about which system libs versions firefox/xulrunner can be linked to.
(In reply to comment #27)
> Then you should tighten your policy about which system libs versions
> firefox/xulrunner can be linked to.

Version checking/restriction is not a proper solution, IMHO. Having a good test suite is, and writing tests for known problems should be done (like, is there a unit test for this one? If not, there really should)
Bug 545513 Bug 534787 have 1.9.2 versions of the patches,  Bug 546272 applies ok to 1.9.2.
Adding dependencies helps keep relationships straight.
Depends on: 545513, 546272, 534787
Comment on attachment 494277 [details] [diff] [review]
(mostly a rollup of several backports) fix for animated gif problem

I'm canceling the pending review-request here, for clarity, since it's better to take individual backported patches instead of a rollup.  (and it looks like Martin is working on getting the individual backports to be landable, on the various bugs)

Martin: Have you confirmed that the combined patches from Comment 29 fix this issue?  (Comment 13's "more or less" seemed to imply that something more might be needed...(?))
Attachment #494277 - Attachment description: fix for animated gif problem → (mostly a rollup of several backports) fix for animated gif problem
Attachment #494277 - Flags: review?(joe)
> Bug 545513 Bug 534787 have 1.9.2 versions of the patches,  Bug 546272 applies
> ok to 1.9.2.

All bugs are approved except for bug 545513, which is waiting on review.(In reply to comment #29). If / when that gets approved, is this bug INVALID?
I don't know why INVALID would be the right resolution -- maybe FIXED by dependencies (or WORKSFORME)?

(I think you're asking the same question that I was asking at the end of Comment 31, though -- Martin, does this bug become fixed when the various backports are applied?)
Version: unspecified → 1.9.2 Branch
Yes, a combination of patches from Bug 545513 Bug 534787 and Bug 546272 fixes this issue on 1.9.2.
shouldn't this be closed now that both 3.6.x and 4.0 contain those fixes?
Actually 3.6.x is not fixed, unless dropping back to an older version of cairo.

Having said that I'd yes it probably should be closed simply because we are at 4.0 now and 3.6.x days are numbered. The problem is fixed in 4.0
3.6.16 should contain this fix.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WORKSFORME
Just tried 3.6.16 in Slackware64, problem still exist.
It actually will be fixed in 3.6.17.  3.6.16 was pushed out with a single fix for the comodo issue, and everything previously slated for .16 is now going to be in .17 instead.
{ Hopefully, this comment automagically re-opens this bug }

I still have that problem with the animated GIFs
in the original bugreport and with other sites.

I'm using Iceweasel (Firefox) 3.5.19-3 with a fresh profile
in Debian testing (AMD64), together with libcairo2-1.10.2-6.
I doubt Mozilla is interested in fixing this on the 3.5 branch.  Encourage your distributor to move to a newer branch.
Version: 1.9.2 Branch → 1.9.1 Branch
Version: 1.9.1 Branch → 1.9.2 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: