Last Comment Bug 623797 - Build with system cairo is broken
: Build with system cairo is broken
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla2.0b12
Assigned To: Jory A. Pratt
:
Mentors:
Depends on:
Blocks: 363861
  Show dependency treegraph
 
Reported: 2011-01-06 18:04 PST by ojab
Modified: 2011-02-07 10:57 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
ifdef out features not avaliable in system cairo yet (romaxa's original patch) (2.15 KB, patch)
2011-01-07 21:47 PST, Jory A. Pratt
no flags Details | Diff | Review
system-cairo-fixup v1.1 (3.67 KB, patch)
2011-01-09 11:24 PST, Jory A. Pratt
jmuizelaar: review-
Details | Diff | Review
system-cairo-fixup v1.2 (2.31 KB, patch)
2011-01-10 10:26 PST, Jory A. Pratt
no flags Details | Diff | Review
system-cairo-fixup v1.3 (3.08 KB, patch)
2011-01-10 11:38 PST, Jory A. Pratt
jmuizelaar: review+
Details | Diff | Review
Check that cairo was built with tee backend support (1.13 KB, patch)
2011-01-16 09:42 PST, Jory A. Pratt
no flags Details | Diff | Review
system-cairo-fixup v1.4 (3.22 KB, patch)
2011-01-16 09:55 PST, Jory A. Pratt
anarchy: review+
joe: approval2.0+
Details | Diff | Review
Check that cairo was built with tee backend support v1.1 (958 bytes, patch)
2011-01-17 07:46 PST, Jory A. Pratt
jmuizelaar: review+
joe: approval2.0+
Details | Diff | Review

Description ojab 2011-01-06 18:04:07 PST
User-Agent:       Uzbl (Webkit 1.3.9) (Linux x86_64 [x86_64]) (Commit 505cdd73111b51934ad382264e14b401996fcd4d)
Build Identifier: 

https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c132

Reproducible: Always
Comment 1 Jory A. Pratt 2011-01-07 21:47:50 PST
Created attachment 502197 [details] [diff] [review]
ifdef out features not avaliable in system cairo yet (romaxa's original patch)

I have tested the patch from romaxa on numerous systems ppc x86 x86_64 and all are able to use system-cairo again without any problems.
Comment 2 Ilja Sekler 2011-01-08 06:51:04 PST
(In reply to comment #1)
> Created attachment 502197 [details] [diff] [review]
> ifdef out features not avaliable in system cairo yet (romaxa's original patch)
> 
> I have tested the patch from romaxa on numerous systems ppc x86 x86_64 and all
> are able to use system-cairo again without any problems.

This fails for me on Ubuntu 10.10 (x86), see <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c143>, but <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c144> works.
Comment 3 Jory A. Pratt 2011-01-08 16:54:00 PST
(In reply to comment #2)
> (In reply to comment #1)
> > Created attachment 502197 [details] [diff] [review]
> > ifdef out features not avaliable in system cairo yet (romaxa's original patch)
> > 
> > I have tested the patch from romaxa on numerous systems ppc x86 x86_64 and all
> > are able to use system-cairo again without any problems.
> 
> This fails for me on Ubuntu 10.10 (x86), see
> <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c143>, but
> <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c144> works.

It would be nice if you specified which version of gcc your using. The error you speak of looks to be a gcc issue not an actual error on patches part.
Comment 4 Ilja Sekler 2011-01-09 02:07:08 PST
(In reply to comment #3)

> It would be nice if you specified which version of gcc your using. The error
> you speak of looks to be a gcc issue not an actual error on patches part.

I use Ubuntu 10.10, 'c++ --version' reports

c++ --version
g++-4.4.real (Ubuntu/Linaro 4.4.4-14ubuntu5) 4.4.5

where /usr/bin/c++ points after a chain of symlinks to /usr/bin/hardened-c++ from the package <http://packages.ubuntu.com/maverick/hardening-wrapper>.
Comment 5 Hussam Al-Tayeb 2011-01-09 10:21:16 PST
after applying the patch, I get the following build error:

/home/hussam/packages/firefox/gfx/thebes/gfxTeeSurface.cpp: In member function ‘virtual const gfxIntSize gfxTeeSurface::GetSize() const’:
/home/hussam/packages/firefox/gfx/thebes/gfxTeeSurface.cpp:67:1: error: no return statement in function returning non-void
make[5]: *** [gfxTeeSurface.o] Error 1
make[5]: Leaving directory `/home/hussam/packages/firefox/obj-i686-pc-linux-gnu/gfx/thebes'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/home/hussam/packages/firefox/obj-i686-pc-linux-gnu/gfx'
make[3]: *** [libs_tier_platform] Error 2
make[3]: Leaving directory `/home/hussam/packages/firefox/obj-i686-pc-linux-gnu'
make[2]: *** [tier_platform] Error 2
make[2]: Leaving directory `/home/hussam/packages/firefox/obj-i686-pc-linux-gnu'
make[1]: *** [default] Error 2
make[1]: Leaving directory `/home/hussam/packages/firefox/obj-i686-pc-linux-gnu'
make: *** [build] Error 2
Comment 6 Hussam Al-Tayeb 2011-01-09 10:38:19 PST
nevermind, the fix in the other bug helped. sorry for the spam
Comment 7 Jory A. Pratt 2011-01-09 11:24:10 PST
Created attachment 502370 [details] [diff] [review]
system-cairo-fixup v1.1

Updated to include the return for broken gcc versions.
Comment 8 Jeff Muizelaar [:jrmuizel] 2011-01-10 08:48:10 PST
Comment on attachment 502370 [details] [diff] [review]
system-cairo-fixup v1.1


> PRBool
> gfxASurface::GetSubpixelAntialiasingEnabled()
> {
>     if (!mSurfaceValid)
>       return PR_FALSE;
>+#ifdef MOZ_TREE_CAIRO
>     return cairo_surface_get_subpixel_antialiasing(mSurface) == CAIRO_SUBPIXEL_ANTIALIASING_ENABLED;
>+#else
>+    return PR_FALSE;
>+#endif
> }

This should probably return PR_TRUE if not MOZ_TREE_CAIRO as that is the default.

>diff --git a/gfx/thebes/gfxTeeSurface.cpp b/gfx/thebes/gfxTeeSurface.cpp

I don't feel too good about disabling this code. We don't currently use it on Linux but the plan is to do so. It's probably better to get the tee backend enabled in distros.
Comment 9 Jory A. Pratt 2011-01-10 10:26:44 PST
Created attachment 502539 [details] [diff] [review]
system-cairo-fixup v1.2

../../staticlib/libthebes.a(gfxASurface.o): In function `gfxASurface::Init(_cairo_surface*, int)':
gfxASurface.cpp:(.text+0x440): undefined reference to `cairo_surface_status'
../../staticlib/libthebes.a(gfxASurface.o): In function `gfxASurface::CreateSimilarSurface(gfxASurface::gfxContentType, gfxIntSize const&)':
gfxASurface.cpp:(.text+0x65c): undefined reference to `cairo_surface_status'
../../staticlib/libthebes.a(gfxASurface.o): In function `gfxASurface::CairoStatus()':
gfxASurface.cpp:(.text+0x971): undefined reference to `cairo_surface_status'
../../staticlib/libthebes.a(gfxTeeSurface.o): In function `gfxTeeSurface::GetSize() const':
gfxTeeSurface.cpp:(.text+0x1d): undefined reference to `cairo_tee_surface_index'
../../staticlib/libthebes.a(gfxTeeSurface.o): In function `gfxTeeSurface::gfxTeeSurface(gfxASurface**, int)':
gfxTeeSurface.cpp:(.text+0x12e): undefined reference to `cairo_tee_surface_create'
gfxTeeSurface.cpp:(.text+0x15f): undefined reference to `cairo_tee_surface_add'
../../staticlib/libthebes.a(gfxTeeSurface.o): In function `gfxTeeSurface::GetSurfaces(nsTArray<nsRefPtr<gfxASurface>, nsTArrayDefaultAllocator>*)':
gfxTeeSurface.cpp:(.text+0x200): undefined reference to `cairo_tee_surface_index'
gfxTeeSurface.cpp:(.text+0x20b): undefined reference to `cairo_surface_status'
/usr/lib/gcc/x86_64-pc-linux-gnu/4.5.2/../../../../x86_64-pc-linux-gnu/bin/ld: libxul.so: hidden symbol `cairo_tee_surface_index' isn't defined
/usr/lib/gcc/x86_64-pc-linux-gnu/4.5.2/../../../../x86_64-pc-linux-gnu/bin/ld: final link failed: Nonrepresentable section on output

Adressed issue as far as PR_TRUE and system cairo having tee support but now we show link issues I will attach a complete build.log
Comment 10 Karl Tomlinson (ni?:karlt) 2011-01-10 11:03:56 PST
> /usr/lib/gcc/x86_64-pc-linux-gnu/4.5.2/../../../../x86_64-pc-linux-gnu/bin/ld:
> libxul.so: hidden symbol `cairo_tee_surface_index' isn't defined

Thank you, Jory.
cairo-tee.h will need to be added to config/system-headers,
and for some reason our tests require that js/src/config/system-headers is kept in line with config/system-headers.
Comment 11 Jory A. Pratt 2011-01-10 11:17:20 PST
(In reply to comment #10)
> > /usr/lib/gcc/x86_64-pc-linux-gnu/4.5.2/../../../../x86_64-pc-linux-gnu/bin/ld:
> > libxul.so: hidden symbol `cairo_tee_surface_index' isn't defined
> 
> Thank you, Jory.
> cairo-tee.h will need to be added to config/system-headers,
> and for some reason our tests require that js/src/config/system-headers is kept
> in line with config/system-headers.

Kal thanks for reminding me of the need to add to system-headers. I will finish working up this patch so I can test and we can get it reviewed.
Comment 12 Jory A. Pratt 2011-01-10 11:38:13 PST
Created attachment 502556 [details] [diff] [review]
system-cairo-fixup v1.3

Karl's comments are addresssed for adding cairo-tee.h to both system-headers. I am still digging through the code to see why I am ending up with failure.
Comment 13 Jory A. Pratt 2011-01-10 13:06:03 PST
Comment on attachment 502556 [details] [diff] [review]
system-cairo-fixup v1.3

This is only the first part, we still need to add checks to make sure when system cairo is used that tee support is avaliable.
Comment 14 Jeff Muizelaar [:jrmuizel] 2011-01-10 19:38:13 PST
Comment on attachment 502556 [details] [diff] [review]
system-cairo-fixup v1.3

>+#ifdef MOZ_TREE_CAIRO
> #include "cairo.h"

Add a comment here about removing the MOZ_TREE_CAIRO once we switch cairo 1.10

>+#else
>+#include "cairo-tee.h"
>+#endif

Other then that and the checks for cairo-tee support this looks good.
Comment 15 Mike Hommey [:glandium] 2011-01-12 00:09:10 PST
Comment on attachment 502556 [details] [diff] [review]
system-cairo-fixup v1.3

> void
> gfxASurface::SetSubpixelAntialiasingEnabled(PRBool aEnabled)
> {
>     if (!mSurfaceValid)
>         return;
>+#ifdef MOZ_TREE_CAIRO
>     cairo_surface_set_subpixel_antialiasing(mSurface,
>         aEnabled ? CAIRO_SUBPIXEL_ANTIALIASING_ENABLED : CAIRO_SUBPIXEL_ANTIALIASING_DISABLED);
>+#endif
> }

Why keep the (!mSurfaceValid) test ?

> #include "gfxTeeSurface.h"
> 
>+#ifdef MOZ_TREE_CAIRO
> #include "cairo.h"
>+#else
>+#include "cairo-tee.h"
>+#endif

To be future-proof, it might be better to create a dummy cairo-tee.h in the cairo directory that either receives the corresponding definition or simply includes cairo.h... That would avoid surprises when the in-tree cairo is updated to a version that doesn't include cairo-tee.h from cairo.h.
Comment 16 Hussam Al-Tayeb 2011-01-12 10:01:48 PST
using system-cairo-fixup v1.3 patch
I get
../../dist/system_wrappers/cairo-tee.h:3:28: fatal error: cairo-tee.h: No such file or directory
if I build against system cairo 1.10.2
Comment 17 Mike Hommey [:glandium] 2011-01-12 10:26:30 PST
(In reply to comment #16)
> using system-cairo-fixup v1.3 patch
> I get
> ../../dist/system_wrappers/cairo-tee.h:3:28: fatal error: cairo-tee.h: No such
> file or directory
> if I build against system cairo 1.10.2

You most probably are building against a cairo that wasn't built with --enable-tee. You need bug 624684.
Comment 18 Jeremy Huddleston 2011-01-15 13:03:41 PST
Even if cairo is built with --enable-tee, it will still fail to build without this patch because _cairo_subpixel_antialiasing_t is not in a released cairo (nor is it even in cairo git master).  Are their plans to get these mozilla-specific changes into mainline cairo?
Comment 19 Jory A. Pratt 2011-01-16 09:42:07 PST
Created attachment 504266 [details] [diff] [review]
Check that cairo was built with tee backend support

This will ensure that cairo has proper tee support, if not fail in configure instead of build.
Comment 20 Jory A. Pratt 2011-01-16 09:55:38 PST
Created attachment 504271 [details] [diff] [review]
system-cairo-fixup v1.4

Add comment to ensure we fix up header when cairo is updated in the tree.
Comment 21 Mike Hommey [:glandium] 2011-01-16 23:38:33 PST
Comment on attachment 504266 [details] [diff] [review]
Check that cairo was built with tee backend support

>+   # Ensure cairo-tee.h is present
>+   AC_CHECK_HEADER(cairo/cairo-tee.h,, [AC_MSG_ERROR([
>+        ERROR: System cairo must be built with tee backend enabled. ])])

You don't need to add ERROR to the message. AC_MSG_ERROR already will display that.

You should also do that AFTER the main PKG_CHECK_MODULES(CAIRO, cairo...), and IMHO, you should use PKG_CHECK_MODULES(CAIRO_TEE, cairo-tee >= $CAIRO_VERSION) instead of AC_CHECK_HEADER.
Comment 22 Jory A. Pratt 2011-01-17 07:46:50 PST
Created attachment 504470 [details] [diff] [review]
Check that cairo was built with tee backend support v1.1

As Mike pointed out we really should check the pkg-config files presence, less complicated and much saner approach.
Comment 23 ojab 2011-01-29 13:36:26 PST
Can i haz approval2.0?
Comment 24 Joe Drew (not getting mail) 2011-01-31 13:44:37 PST
I'm not fully convinced that we'd be OK with shipping Firefox without these patches to our Linux users.

I'd be much happier if these patches allowed for distros to apply our subpixel AA patch, and fall back if they haven't.
Comment 25 Joe Drew (not getting mail) 2011-01-31 13:49:17 PST
Also, what will happen if someone does --enable-system-cairo and has only cairo 1.8 on their system? It'd be nice if this failed in configure instead of at build time.
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-31 13:50:15 PST
I'm OK with taking these patches. The only effect of disabling the subpixel-AA calls, with the cairo backends we use on Linux, is uglier rendering of subpixel-AA text to the transparent parts of transparent surfaces. That will really only affect canvas and it'll be the same as pre-FF4. Suboptimal, but not worth making a fuss over.

The tee surface is only used with the GL layers backend, so it would be possible to create patches to build without the tee surface as long as GL is disabled.
Comment 27 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-31 13:52:14 PST
(In reply to comment #18)
> Even if cairo is built with --enable-tee, it will still fail to build without
> this patch because _cairo_subpixel_antialiasing_t is not in a released cairo
> (nor is it even in cairo git master).  Are their plans to get these
> mozilla-specific changes into mainline cairo?

We discussed these changes on the cairo mailing list. Behdad rejected them and suggested instead supporting full component-alpha surfaces in cairo. That would be a lot more work, and slow; we don't want to go there. So it's an impasse.
Comment 28 Mike Hommey [:glandium] 2011-01-31 23:39:44 PST
(In reply to comment #25)
> Also, what will happen if someone does --enable-system-cairo and has only cairo
> 1.8 on their system?

That's already taken care of by PKG_CHECK_MODULES(CAIRO, cairo >= $CAIRO_VERSION ...), since CAIRO_VERSION is 1.10
Comment 29 Joe Drew (not getting mail) 2011-02-03 19:55:28 PST
Comment on attachment 504271 [details] [diff] [review]
system-cairo-fixup v1.4

I still am really not in favour of encouraging the use of system cairo when it provides a worse experience for users, but I do recognize that it only affects a small subset of content. I will approve this patch as-is, as long as it's passed try, but I strongly recommend you rework to allow distros to patch their cairo with our subpixel AA patches.
Comment 30 Mounir Lamouri (:mounir) 2011-02-04 07:14:36 PST
Sent to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=a2e34a35282e

I will try to push that this week-end if it's green.
Comment 31 Mounir Lamouri (:mounir) 2011-02-07 10:57:14 PST
One test (config matching betweeen mozilla-central one and js/ one) was failing. I fixed that and land the two patches:
http://hg.mozilla.org/mozilla-central/rev/a92182a3c71f
http://hg.mozilla.org/mozilla-central/rev/366d47bce128

Note You need to log in before you can comment on or make changes to this bug.