567750 - Build option to use the system's Pixman

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[James Cook]

It would be nice to have a new feature to allow libxul to be linked against a version of Pixman provided by the operating system.

Comment 1

[James Cook]

Attachment: One possible way to do it

This patch will also check for the version of Pixman to be at least 0.17.3.

Comment 2

[(not currently active) Ted Mielczarek]

Joe and Jeff would have to sign off on such a plan.

Comment 3

[Joe Drew (not getting mail)]

Pretty ugly changes to cairo-surface.c in there.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 447081 [details] [diff] [review]
One possible way to do it

I don't want to add this additional kludge to cairo-surface.c to support system pixman. We should either have pixman export the additional api or better yet, fix cairo upstream so that the wrap_image code is not needed.

Comment 5

[Juha Seppänen]

James - please review your patch according to comments and refile.

Comment 6

[James Cook]

(In reply to comment #5)
> James - please review your patch according to comments and refile.

I don't think it makes any sense to revise this patch until http://cgit.freedesktop.org/pixman/commit/?id=78778e5963c948de5ce5f7c5a2a3bb9f279a8eda makes it to a Pixman release.

Comment 7

[Oleg Romashin (:romaxa)]

I guess all related changes should be available in some pixman release like: pixman-0.19.2.tar.bz2  or even earlier 0.19 version...

can we make patch which is just and allowing to compile with system pixman if version is satisfy our requirements?

Comment 8

[Egor Starkov]

Attachment: pixman option

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 473482 [details] [diff] [review]
pixman option

I'm ok with adding this option but I'll let ted review the build changes.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 473482 [details] [diff] [review]
pixman option

This looks pretty good, but there are a few issues I'd like to see fixed.

>+MOZ_SYSTEM_PIXMAN=
>+MOZ_ARG_ENABLE_BOOL(system-pixman,
>+[ --enable-system-pixman Use system pixman (located with pkgconfig)],
>+MOZ_SYSTEM_PIXMAN=1,
>+MOZ_SYSTEM_PIXMAN= )

Please make this MOZ_TREE_PIXMAN (to mirror MOZ_TREE_CAIRO, MOZ_TREE_FREETYPE, etc).

>+
>+if test "$MOZ_SYSTEM_PIXMAN"; then
>+    AC_DEFINE(MOZ_SYSTEM_PIXMAN)
>+fi
>+

You only need to AC_DEFINE it if you need to use it from C++.  I think you can remove this test/define.

>@@ -8481,17 +8491,27 @@ if test "$MOZ_TREE_CAIRO"; then
>+   echo pixman = $MOZ_SYSTEM_PIXMAN
Is this stray debugging code?  It shouldn't be necessary.

>@@ -8503,16 +8523,17 @@ else
>+AC_SUBST(MOZ_SYSTEM_PIXMAN)

You need to add MOZ_TREE_PIXMAN to config/autoconf.mk.in to make this work.

>+CFLAGS += $(MOZ_CAIRO_CFLAGS)
>+CXXFLAGS += $(MOZ_CAIRO_CFLAGS)

ifdef MOZ_TREE_PIXMAN?

Other than reversing the name it looks pretty good, but I'd like to see another patch.

Comment 11

[Egor Starkov]

Attachment: pixman option v2

Comment 12

[Egor Starkov]

checkin-needed

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This patch needs approval2.0 first.

Comment 14

[Oleg Romashin (:romaxa)]

we need this option to link against system pixman with latest optimizations,(otherwise pixman fixes nokia->upstream->mozilla chain take too much time)

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 475067 [details] [diff] [review]
pixman option v2

jeff says he is okay.  build only changes.

Comment 16

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/ae8d6fa8a42e