623797 - Build with system cairo is broken

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[ojab]

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 [:anarchy]]

Attachment: 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]

(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 [:anarchy]]

(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]

(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]

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]

nevermind, the fix in the other bug helped. sorry for the spam

Comment 7

[Jory A. Pratt [:anarchy]]

Attachment: system-cairo-fixup v1.1

Updated to include the return for broken gcc versions.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

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 [:anarchy]]

Attachment: 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 (:karlt)]

> /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 [:anarchy]]

(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 [:anarchy]]

Attachment: 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 [:anarchy]]

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]]

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]]

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]

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]]

(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]

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 [:anarchy]]

Attachment: 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 [:anarchy]]

Attachment: 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]]

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 [:anarchy]]

Attachment: 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]

Can i haz approval2.0?

Comment 24

[Joe Drew (not getting mail)]

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)]

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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

(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]]

(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)]

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)]

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)]

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