Closed Bug 722975 Opened 12 years ago Closed 11 years ago

--enable-system-cairo build is broken after Bug 715658 fixed

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: ojab, Assigned: ojab)

References

Details

Attachments

(10 files, 4 obsolete files)

882 bytes, patch
jrmuizel
: review+
ted
: review+
Details | Diff | Splinter Review
1.04 KB, patch
Details | Diff | Splinter Review
14.08 KB, patch
Details | Diff | Splinter Review
16.42 KB, patch
Details | Diff | Splinter Review
1.94 KB, patch
Details | Diff | Splinter Review
130 bytes, text/plain
Details
550 bytes, text/plain
Details
118.51 KB, image/png
Details
2.19 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.46 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
/sources/mozilla-inbound/gfx/thebes/gfxPlatform.cpp: In member function ‘virtual mozilla::RefPtr<mozilla::gfx::SourceSurface> gfxPlatform::GetSourceSurfaceForSurface(mozilla::gfx::DrawTarget*, gfxASurface*)’:
/sources/mozilla-inbound/gfx/thebes/gfxPlatform.cpp:516:53: error: ‘cairo_null_surface_create’ was not declared in this scope
/sources/mozilla-inbound/gfx/thebes/gfxPlatform.cpp:521:95: error: ‘cairo_surface_attach_snapshot’ was not declared in this scope
make[7]: *** [gfxPlatform.o] Error 1
Blocks: 715658
Confirmed, I'm also seeing this bug. If I revert the patch attached to bug 715658 then the build succeeds, another option is to remove --enable-system-cairo from my mozconfig; otherwise the build fails with the error above.
When I was reviewing bug 715658 I took the fact that it would break system cairo into account. System-cairo is not really a supported configuration and I decided that a quick, and reliably correct solution was better than trying to avoid exposing the new api to cairo. That being said, our reliance on this API should be temporary and when we don't need it anymore I'd like to get system cairo working again. I expect, I'd also take a patch that removed our dependency on this API. In the meantime we should probably make configure fail when --enable-system-cairo is specified.
This should be the patch suggested by jrmuizel.
Attachment #594066 - Flags: review?(jmuizelaar)
Comment on attachment 594066 [details] [diff] [review]
Fail in configure when --enable-system-cairo is used

Looks fine to me but I don't read autoconf very well. If you're not sure about it's correctness please get a review from a better reviewer.
Attachment #594066 - Flags: review?(jmuizelaar) → review+
Attachment #594066 - Attachment is patch: true
Attachment #594066 - Flags: review?(ted.mielczarek)
(In reply to Bas Schouten (:bas) from comment #3)
> Created attachment 594066 [details] [diff] [review]
> Fail in configure when --enable-system-cairo is used
> 
> This should be the patch suggested by jrmuizel.

Isn't this a bit drastic, when a few simple ifdefs in gfx/thebes/gfxPlatform.cpp
would be sufficient to fix this problem?
(In reply to Octoploid from comment #5)
> (In reply to Bas Schouten (:bas) from comment #3)
> Isn't this a bit drastic, when a few simple ifdefs in
> gfx/thebes/gfxPlatform.cpp
> would be sufficient to fix this problem?

Simple? I can do simple! This patch fixes compiling with --enable-system-cairo for me
(In reply to Andrew Benton from comment #6)
> Created attachment 595000 [details] [diff] [review]
> Ad 2 #ifdefs to gfx/thebes/gfxPlatform.cpp
> 
> (In reply to Octoploid from comment #5)
> > (In reply to Bas Schouten (:bas) from comment #3)
> > Isn't this a bit drastic, when a few simple ifdefs in
> > gfx/thebes/gfxPlatform.cpp
> > would be sufficient to fix this problem?
> 
> Simple? I can do simple! This patch fixes compiling with
> --enable-system-cairo for me

It means this bug exists again on linux (once we switch Azure content rendering on there), so it doesn't actually help.
(In reply to Bas Schouten (:bas) from comment #7)
> It means this bug exists again on linux (once we switch Azure content
> rendering on there), so it doesn't actually help.

It means I can compile firefox again which is a step in the right direction. The alternative is to stick with Firefox-10 for the foreseeable future.
(In reply to Andrew Benton from comment #8)
> (In reply to Bas Schouten (:bas) from comment #7)
> > It means this bug exists again on linux (once we switch Azure content
> > rendering on there), so it doesn't actually help.
> 
> It means I can compile firefox again which is a step in the right direction.
> The alternative is to stick with Firefox-10 for the foreseeable future.

Why can't you compile firefox without system cairo?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> Why can't you compile firefox without system cairo?

Pure stubborn bloodymindedness ;)
I prefer to link to system libraries wherever possible
(In reply to Andrew Benton from comment #10)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> > Why can't you compile firefox without system cairo?
> 
> Pure stubborn bloodymindedness ;)
> I prefer to link to system libraries wherever possible

If that's the reason, you can always apply this patch yourself :)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> (In reply to Andrew Benton from comment #8)
> > (In reply to Bas Schouten (:bas) from comment #7)
> > > It means this bug exists again on linux (once we switch Azure content
> > > rendering on there), so it doesn't actually help.
> > 
> > It means I can compile firefox again which is a step in the right direction.
> > The alternative is to stick with Firefox-10 for the foreseeable future.
> 
> Why can't you compile firefox without system cairo?

There was a time when Firefox, compiled without system cairo, looked 
**** (color fringes), because the in-tree cairo only included the awful 
LCD_FILTER_LEGACY algorithm. 
I've been using --enable-system-cairo since that time.
But today I've build Firefox without --enable-system-cairo and it actually looks OK now.
So --enable-system-cairo isn't necessary anymore.
Well if the mountain will not come to Mohammed, perhaps we can patch cairo to have the new functions? This patch adds the new functions into cairo-1.10.2. With the new functions in the system cairo I can compile firefox without patching the firefox code
Comment on attachment 594066 [details] [diff] [review]
Fail in configure when --enable-system-cairo is used

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

::: configure.in
@@ +8066,5 @@
>  
>  MOZ_TREE_CAIRO=1
>  MOZ_ARG_ENABLE_BOOL(system-cairo,
>  [  --enable-system-cairo   Use system cairo (located with pkgconfig)],
> +AC_MSG_ERROR([ --enable-system-cairo is no longer supported.]),

Please file a bug on making system cairo work again, and list that bug number in a comment here (and possibly in the error message).
Attachment #594066 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → bas.schouten
doesn't mozilla like to push changes into upstream? 
this way the fix is for real rather than trying to apply workarounds in packages from distros or switching to internal cairo.
(In reply to Ionut Biru from comment #15)
> doesn't mozilla like to push changes into upstream? 
> this way the fix is for real rather than trying to apply workarounds in
> packages from distros or switching to internal cairo.

I've been hesitant to push this upstream because I'm not really sure it's appropriate.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16)
> (In reply to Ionut Biru from comment #15)
> > doesn't mozilla like to push changes into upstream? 
> > this way the fix is for real rather than trying to apply workarounds in
> > packages from distros or switching to internal cairo.
> 
> I've been hesitant to push this upstream because I'm not really sure it's
> appropriate.

only one way to find out. let upstream review the changes and try harder to push it there if the review is not favorable. from that, mozilla has more benefits along with it,other communities as well.
Updated the system cairo patch for cairo-1.12.0
(In reply to Andrew Benton from comment #18)
> Created attachment 609898 [details] [diff] [review]
> patch the new functions into cairo-1.12.0
> 
> Updated the system cairo patch for cairo-1.12.0

Ugh, that patch doesn't work, don't use it. Too much has changed between cairo-1.10.2 and 1.12.0. I don't understand enough to make it work.
Attachment #595138 - Attachment is obsolete: true
Attachment #609898 - Attachment is obsolete: true
Improved patch for cairo-1.12.0. This patch works in that I can compile Firefox with --enable-system-cairo. cairo-1.12.0 is pretty buggy though https://bugs.freedesktop.org/show_bug.cgi?id=47266
https://bugs.gentoo.org/show_bug.cgi?id=409593
Attachment #595138 - Attachment is obsolete: false
@comment 20: for the moment, it seems most of the problem (in regard of those bugs) lies in xorg-server, not cairo.
Oh noes, not the "let's ditch --enable-system-cairo" argument again... i'll use https://bugzilla.mozilla.org/attachment.cgi?id=595000 in the meantime if it works, but it'd be good to have this properly fixed, as lots of distributors are using that switch. Fx 12.0b5 is broken for me as-is. Jeff, you're saying "our reliance on this API should be temporary", what's the timeframe/consequences ?
(In reply to Andrew Benton from comment #20)
> Created attachment 611180 [details] [diff] [review]
> patch the new functions into cairo-1.12.0
> 
> Improved patch for cairo-1.12.0. This patch works in that I can compile
> Firefox with --enable-system-cairo. cairo-1.12.0 is pretty buggy though
> https://bugs.freedesktop.org/show_bug.cgi?id=47266
> https://bugs.gentoo.org/show_bug.cgi?id=409593

Note that the bugs lie on the X server side, not on the cairo side. Which means that whenever we upgrade cairo in the mozilla codebase, we'll hit those X server bugs :-/
(In reply to Landry Breuil from comment #22)
> Jeff,
> you're saying "our reliance on this API should be temporary", what's the
> timeframe/consequences ?

Basically, we need to port the some of the image code to use Azure directly. Once, that happens we won't need to use private APIs anymore and can re-enable system-cairo support. I'm not sure if we'll be able to do this before we switch everything over to Azure or not. The timeframe depends largely on this.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> System-cairo is not really a supported configuration
Wow, and I thought it was only Chromium guys being crazy enough to fork anything they touch with Firefox team being more prudent in terms of effort and output... hope it's still so.
The breakage of system-cairo support breaks out GTK3 Firefox port too, because it requires to use system cairo (all GTK3 widgets are rendered by system cairo). Can we help somehow with the changes mentioned in comment 24? "to port the some of the image code to use Azure directly".
Anyway, there's another regression with --enable-system-cairo:

gmake[6]: Entering directory `/home/komat/tmp646-trunk/src/objdir/layout/media'
/home/komat/tmp646-trunk/src/config/rules.mk:745: *** SHARED_LIBRARY_LIBS must contain .a files only.  Stop.

because with --enable-system-cairo the SHARED_LIBRARY_LIBS contains cairo dynamic libraries.
(In reply to Martin Stránský from comment #27)
> Anyway, there's another regression with --enable-system-cairo:
> 
> gmake[6]: Entering directory
> `/home/komat/tmp646-trunk/src/objdir/layout/media'
> /home/komat/tmp646-trunk/src/config/rules.mk:745: *** SHARED_LIBRARY_LIBS
> must contain .a files only.  Stop.
> 
> because with --enable-system-cairo the SHARED_LIBRARY_LIBS contains cairo
> dynamic libraries.

It's the same as bug 751521. You may want to kill the two birds with the same stone.
This patch makes mozilla build here with system cairo again. It replaces usage of cairo_surface_attach_snapshot() and _create_nil_surface() with public cairo API (cairo_surface_set_mime_data()). Mime data gets detached at the same time that snapshots get detached (see _cairo_surface_begin_modification()).
Attachment #624394 - Flags: review?(bas.schouten)
Comment on attachment 624394 [details] [diff] [review]
Make system cairo work again

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

As I see, your patch breaks the correct rendering of the following page (press End): http://rohejorszaga.blog.hu/2012/06/03/erre_egyszeruen_nem_talalok_szavakat_601?fullcommentlist=1#comments
@comment 30:
Well, it seems even holding PageDown for awhile triggers the problem (it probably reverts to the state of bug 715658).
So, anyone up to asking *cairo* upstream how would they solve the original problem within existing *public* API ?
Or is the attachment 624394 [details] [diff] [review] the correct solution and it's firefox, that needs a bit stricter resource management ?
(In reply to Rafał Mużyło from comment #31)
> @comment 30:
> Well, it seems even holding PageDown for awhile triggers the problem (it
> probably reverts to the state of bug 715658).
> So, anyone up to asking *cairo* upstream how would they solve the original
> problem within existing *public* API ?
> Or is the attachment 624394 [details] [diff] [review] the correct solution
> and it's firefox, that needs a bit stricter resource management ?

I don't know what specific version of firefox and cairo you use, but it works fine for me with cairo 1.12 and firefox 14. However, cairo 1.12 is known to expose X.org driver problems.
@comment 30: most of it is a miss.
I *am* using cairo 1.12, but I'm also using the drivers with the fix for that problem (or at least that part of it that was easily exposed in firefox - basic acceleration of solid pictures via scratch 1x1 pixmaps). I'm using (as of yesterday) firefox 13 though, so perhaps it's fixed in 14 already.

Well, that is unless you're talking about a different set of cairo/X driver problems.
(In reply to Rafał Mużyło from comment #33)
> @comment 30: most of it is a miss.
> I *am* using cairo 1.12, but I'm also using the drivers with the fix for
> that problem (or at least that part of it that was easily exposed in firefox
> - basic acceleration of solid pictures via scratch 1x1 pixmaps). I'm using
> (as of yesterday) firefox 13 though, so perhaps it's fixed in 14 already.

Works fine here in firefox 13 as well.

> Well, that is unless you're talking about a different set of cairo/X driver
> problems.

AFAIK, there are several different problems, not all of which are fixed ATM.
(In reply to Mike Hommey [:glandium] from comment #34)
> (In reply to Rafał Mużyło from comment #33)
> > Well, that is unless you're talking about a different set of cairo/X driver
> > problems.
> 
> AFAIK, there are several different problems, not all of which are fixed ATM.

Can you share the bug numbers in upstream bugzilla or ... ?
(In reply to Rafał Mużyło from comment #35)
> (In reply to Mike Hommey [:glandium] from comment #34)
> > (In reply to Rafał Mużyło from comment #33)
> > > Well, that is unless you're talking about a different set of cairo/X driver
> > > problems.
> > 
> > AFAIK, there are several different problems, not all of which are fixed ATM.
> 
> Can you share the bug numbers in upstream bugzilla or ... ?

I did in comment #20
(In reply to Andrew Benton from comment #36)
> (In reply to Rafał Mużyło from comment #35)
> > Can you share the bug numbers in upstream bugzilla or ... ?
> 
> I did in comment #20
Your info is out of date (and there were a few of my comments in both of those bugs).
The Gentoo bug was exactly the same problem as the freedesktop one and the later one has long been fix in all three major driver branches (intel, ati and nouveau).
Attached file minimal testcase - html (obsolete) —
...and on a related note: I do notice an odd case of display corruption, though I'm not quite sure whether it's related to cairo 1.12.

I've managed to reduce it into a nearly trivial test case.
To trigger the corruption put html and css files in one folder, and hover the mouse over those two links. As you switch a few times between them, backrground of the links becomes corrupted.
Attached file minimal testcase - css
(In reply to Rafał Mużyło from comment #38)
> Created attachment 630531 [details]
> minimal testcase - html
> 
> ...and on a related note: I do notice an odd case of display corruption,
> though I'm not quite sure whether it's related to cairo 1.12.
> 
> I've managed to reduce it into a nearly trivial test case.
> To trigger the corruption put html and css files in one folder, and hover
> the mouse over those two links. As you switch a few times between them,
> backrground of the links becomes corrupted.

Still no problem here. Can we just stop chasing cairo or Xorg bugs here anyways?
Minor correction of the testcase (now it validates as xhtml).
Attachment #630531 - Attachment is obsolete: true
This bug became a bit of a mess, so lets summarize:
While using patch from comment 29:
- that link problem happens only in cairo built with '--enable-xlib-xcb', so my mistake there (that option is disabled by default)
- problem in bug 715658 happens regardless of cairo version, so patch might be incomplete
- problem from comment 30 happens only with cairo 1.12 - I'll see what upstream bugzilla has to say
(In reply to Rafał Mużyło from comment #42)
> - problem in bug 715658 happens regardless of cairo version, so patch might
> be incomplete

With https://bugzilla.mozilla.org/attachment.cgi?id=624394, it doesn't. The patch is no incomplete.
With both cairo 1.10.2 and 1.12.2, http://scoop.today.msnbc.msn.com/_news/2012/01/05/9977921-rooney-mara-girl-with-the-style-we-want-to-steal looks like on the attached screenshot while using the mentioned patch.

I don't think that black rectangle is the correct display.
(In reply to Rafał Mużyło from comment #44)
> Created attachment 630607 [details]
> a screenshot of the site
> 
> With both cairo 1.10.2 and 1.12.2,
> http://scoop.today.msnbc.msn.com/_news/2012/01/05/9977921-rooney-mara-girl-
> with-the-style-we-want-to-steal looks like on the attached screenshot while
> using the mentioned patch.
> 
> I don't think that black rectangle is the correct display.

And again, this has nothing to do with this bug, and works fine here. Can we stop here?
Attached patch Updated patch (obsolete) — Splinter Review
Guys, can it be committed?
Attachment #648007 - Flags: review?(bas.schouten)
ping?
Every EXA driver is different but in my case, text looks much better with --enable-system-cairo. If a program is designed to use a version of cairo that has 91 patches applied, this doesn't seem right to me.
Four month since there is a working patch with review request, even longer build with system cairo is broken.
Can anybody review/commit the patch?
This patch is being pushed by people who think it is inefficient for mozilla to try to fork cairo. However, mozilla has now decided to go with the even more inefficient route of writing a vector graphics library from scratch to replace cairo. Maybe that's why they aren't committing this patch.
Blocks: 795737
Also status "UNCONFIRMED" is obviously false...
Attached patch Updated patchSplinter Review
And, after nine month since the bug was filled, the patch has bitrotted once again.
:/
Attachment #648007 - Attachment is obsolete: true
Attachment #648007 - Flags: review?(bas.schouten)
Attachment #669619 - Flags: review?(bas.schouten)
Comment on attachment 669619 [details] [diff] [review]
Updated patch

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

If this works lets land it. We need to keep a very close eye if it doesn't break anything though.
Attachment #669619 - Flags: review?(bas.schouten) → review+
Hooray!
Keywords: checkin-needed
I don't see any Try results here, so I've gone ahead and started a run myself.
https://tbpl.mozilla.org/?tree=Try&rev=034a386f0a57

Uli, sorry for the extra delay, but given Bas' comments, I want to make sure it at least passes Mozilla's internal testsuite before pushing it. I will push it as soon as the Try comes back OK.
Green on Try.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2231b8e778a3

Uli, thanks again for the patch! Just so you know where things are in the process, this patch has landed on mozilla-inbound, which is an intermediate integration branch. It is merged to mozilla-central (the official repo) once or twice daily. Once it lands on m-c, this bug will be resolved as FIXED. At this point, it will land for Firefox 19. If you want, you can request approval on the patch for aurora/beta to get it in an earlier release, but I can't promise you that the request will be accepted. Thanks again!
Assignee: bas.schouten → ojab
Flags: in-testsuite-
Keywords: checkin-needed
No clue what the heck is wrong on windows. cairo-rename.h handles cairo_surface_set_mime_data() the same way it handles other cairo functions and there is nothing windows-specific in there. So why does this only happen on windows?

For the OS X crashes:
No good idea here either. The stack trace looks bogous. _cairo_mime_data_destroy is supposed to call the function pointer given to cairo_surface_set_mime_data(), so this is supposed to end up in SourceSnapshotDetached. However, for some weird reason, _cairo_user_data_array_set_data() is called instead. Weird memory corruption?
Looks like windows build is broken because of https://hg.mozilla.org/integration/mozilla-inbound/rev/27c51f14c3ed
https://hg.mozilla.org/mozilla-central/rev/05006509aa53
Status: UNCONFIRMED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
This was backed out, no?
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Target Milestone: mozilla19 → ---
Ah crap, that changeset had the wrong bug number in it. The changeset in comment #60 belongs to bug 779209.
Blocks: 798926
Comment on attachment 624394 [details] [diff] [review]
Make system cairo work again

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

Afaiui this doesn't work yet.
Attachment #624394 - Flags: review?(bas)
Which build is broken on Windows? The --enable-system-cairo one that didn't work before or --disable-system-cairo? If it's the latter, the problem can still be fixed temporarily using ifdefs right? This would beat having to wait hours for a -Wl,--no-keep-memory build to finish.
I's like bring a little corner case to your attention, that comes from using patch from attachment 624394 [details] [diff] [review] - bug 862422. It might be hard to tell whether the problem lies on cairo or libxul side. Also, the conditions to trigger the case are very specific, but it's not being established what they exactly are.
Attached patch Patch with ifdefs (obsolete) — Splinter Review
Attachment #753419 - Flags: review?
(In reply to Connor Behan from comment #66)
> Created attachment 753419 [details] [diff] [review]
> Patch with ifdef

The ifdefs should not be against linux but rather against MOZ_TREE_CAIRO.
Comment on attachment 753419 [details] [diff] [review]
Patch with ifdefs

Reviews without a person attached to them are likely to be ignored, best to choose someone, even if you aren't sure they can redirect the review request.
Attachment #753419 - Flags: review? → review?(bas)
Yeah, that makes more sense.
Attachment #753419 - Attachment is obsolete: true
Attachment #753419 - Flags: review?(bas)
Attachment #753610 - Flags: review?(bas)
Attachment #753610 - Flags: review?(bas) → review+
(In reply to Connor Behan from comment #69)
BTW, switching by tree or not is a good idea?
If this is depended by the version of cairo, we should use it.
https://hg.mozilla.org/mozilla-central/rev/52b02042b27f
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 1034064
No longer blocks: 1034064
You need to log in before you can comment on or make changes to this bug.