The default bug view has changed. See this FAQ.

Build with system cairo is broken

RESOLVED FIXED in mozilla2.0b12

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ojab, Assigned: Jory A. Pratt)

Tracking

Trunk
mozilla2.0b12
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Blocks: 363861
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #502197 - Flags: review?(jmuizelaar)

Comment 2

6 years ago
(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.
(Assignee)

Comment 3

6 years ago
(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

6 years ago
(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

6 years ago
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

6 years ago
nevermind, the fix in the other bug helped. sorry for the spam
(Assignee)

Comment 7

6 years ago
Created attachment 502370 [details] [diff] [review]
system-cairo-fixup v1.1

Updated to include the return for broken gcc versions.
Attachment #502197 - Attachment is obsolete: true
Attachment #502370 - Flags: review?
Attachment #502197 - Flags: review?(jmuizelaar)
(Assignee)

Updated

6 years ago
Attachment #502370 - Flags: review? → review?(jmuizelaar)
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.
Attachment #502370 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 9

6 years ago
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
Attachment #502370 - Attachment is obsolete: true
> /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.
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
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.
Attachment #502556 - Flags: review?(jmuizelaar)
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.
Attachment #502556 - Flags: review?(jmuizelaar) → review+
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

6 years ago
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
(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

6 years ago
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?
(Assignee)

Comment 19

6 years ago
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.
Attachment #502539 - Attachment is obsolete: true
Attachment #504266 - Flags: review?(jmuizelaar)
(Assignee)

Comment 20

6 years ago
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.
Attachment #502556 - Attachment is obsolete: true
Attachment #504271 - Flags: review+
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.
(Assignee)

Comment 22

6 years ago
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.
Attachment #504266 - Attachment is obsolete: true
Attachment #504470 - Flags: review?(jmuizelaar)
Attachment #504266 - Flags: review?(jmuizelaar)
Attachment #504470 - Flags: review?(jmuizelaar) → review+
(Assignee)

Updated

6 years ago
Attachment #504271 - Flags: approval2.0?
(Assignee)

Updated

6 years ago
Attachment #504470 - Flags: approval2.0?
(Reporter)

Comment 23

6 years ago
Can i haz approval2.0?
Assignee: nobody → anarchy
Status: NEW → ASSIGNED
Whiteboard: [needs approval]
Version: unspecified → Trunk
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.
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.
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.
(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.
(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
Attachment #504470 - Flags: approval2.0? → approval2.0+
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.
Attachment #504271 - Flags: approval2.0? → approval2.0+
Sent to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=a2e34a35282e

I will try to push that this week-end if it's green.
Whiteboard: [needs approval] → [needs try]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [needs try]
Target Milestone: --- → mozilla2.0b12
You need to log in before you can comment on or make changes to this bug.