Bug 12210

commit 0cf3d0364c13ef761e7ddeb83848ff2c8245077f
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Wed Aug 29 12:18:16 2007 -0400

    [configure.in] Make PS backend require zlib too (#12210)
    
    The PS backend uses zlib these days.  Make it depend on zlib being available
    like PDF has been doing.

(In reply to comment #1)
> commit 0cf3d0364c13ef761e7ddeb83848ff2c8245077f
> Author: Behdad Esfahbod <behdad@behdad.org>
> Date:   Wed Aug 29 12:18:16 2007 -0400
> 
>     [configure.in] Make PS backend require zlib too (#12210)
> 
>     The PS backend uses zlib these days.  Make it depend on zlib being
> available
>     like PDF has been doing.

I see. Does cairo/Xlib also require Xrender in the same way now?

In file included from cairo-xlib-surface.c:41:
cairo-xlib-xrender.h:45:36: error: X11/extensions/Xrender.h: No such file or directory

(In reply to comment #2)
> (In reply to comment #1)
> > commit 0cf3d0364c13ef761e7ddeb83848ff2c8245077f
> > Author: Behdad Esfahbod <behdad@behdad.org>
> > Date:   Wed Aug 29 12:18:16 2007 -0400
> > 
> >     [configure.in] Make PS backend require zlib too (#12210)
> > 
> >     The PS backend uses zlib these days.  Make it depend on zlib being
> > available
> >     like PDF has been doing.
> 
> I see. Does cairo/Xlib also require Xrender in the same way now?
> 
> In file included from cairo-xlib-surface.c:41:
> cairo-xlib-xrender.h:45:36: error: X11/extensions/Xrender.h: No such file or
> directory

No, cairo is capable of rendering to X servers without Xrender, but nobody ever did the job of making it compilable without Xrender.  So, in that sense, it's always been broken.  We're waiting for someone motivated enough to send a patch.  I personally started doing it a few times (including today), but it's not trivial and lacking enough motivation, I gave up again.

If you want to give it a try, just start #if CAIRO_HAS_XLIB_XRENDER_SURFACE stuff out until it compiles and works...

(In reply to comment #4)
> If you want to give it a try, just start #if CAIRO_HAS_XLIB_XRENDER_SURFACE
> stuff out until it compiles and works...
> 

I started to look at it, I am thinking about splitting cairo-xlib-surface.c
in two, stuffing everything that requires render extensions
into cairo-xlib-surface-xrender.c, and possibly export
an interface in cairo-xlib-surface-xrender.h to make available to
cairo-xlib-surface.c . Still very cloudy, but the idea is to clutter
with preprocessor conditionals as little as possible. 

ATM I have wrapped part of the struct _cairo_xlib_surface declaration in
cairo-xlib-surface-private.h in an if like this:

#if CAIRO_HAS_XLIB_XRENDER_SURFACE
    int render_major;
    int render_minor;
    Picture dst_picture, src_picture;
    XRenderPictFormat *xrender_format;
    XTransform xtransform;
#endif

but maybe something better will come to mind.
Tomorrow I'll start going through the functions in the .c in more
detail, trying to understand how this thing works.

Btw, I've noticed that functions are referenced in a backend table/structure,
and in some cases some of these functions only have an implementation that
requires render extensions. Should I just put NULL there in these cases
and hope for the best? (that applications still work?)

For example, I singled out

_cairo_xlib_surface_create_similar,
_cairo_xlib_surface_fill_rectangles,
_cairo_xlib_surface_composite_trapezoids,
_cairo_xlib_surface_is_similar

as currently being render-ext-only (or so they look to me).
Does this make any sense? The domain is very unfamiliar to me.

Claudio

(In reply to comment #5)
> (In reply to comment #4)
> > If you want to give it a try, just start #if CAIRO_HAS_XLIB_XRENDER_SURFACE
> > stuff out until it compiles and works...
> > 
> 
> I started to look at it, I am thinking about splitting cairo-xlib-surface.c
> in two, stuffing everything that requires render extensions
> into cairo-xlib-surface-xrender.c, and possibly export
> an interface in cairo-xlib-surface-xrender.h to make available to
> cairo-xlib-surface.c . Still very cloudy, but the idea is to clutter
> with preprocessor conditionals as little as possible. 

I don't think separating them makes much sense.

> ATM I have wrapped part of the struct _cairo_xlib_surface declaration in
> cairo-xlib-surface-private.h in an if like this:
> 
> #if CAIRO_HAS_XLIB_XRENDER_SURFACE
>     int render_major;
>     int render_minor;
>     Picture dst_picture, src_picture;
>     XRenderPictFormat *xrender_format;
>     XTransform xtransform;
> #endif
> 
> but maybe something better will come to mind.
> Tomorrow I'll start going through the functions in the .c in more
> detail, trying to understand how this thing works.
> 
> Btw, I've noticed that functions are referenced in a backend table/structure,
> and in some cases some of these functions only have an implementation that
> requires render extensions. Should I just put NULL there in these cases
> and hope for the best? (that applications still work?)

If you do NULL, cairo will use the fallback code.  So yes.  But you can also just return UNSUPPORTED.  As I said, the current code does work with an X server without Xrender.  So, you already got all the answers in there.  Just follow what happens if Xrender is detected to be unavailable at run-time and make it do the same at compile time.

> For example, I singled out
> 
> _cairo_xlib_surface_create_similar,
> _cairo_xlib_surface_fill_rectangles,
> _cairo_xlib_surface_composite_trapezoids,
> _cairo_xlib_surface_is_similar
> 
> as currently being render-ext-only (or so they look to me).
> Does this make any sense? The domain is very unfamiliar to me.

There's at least _cairo_xlib_show_glyphs too.  show_glyphs and composite_trapezoids should become UNSUPPORTED/NULL with no render I guess, the rest should already have paths working without Xrender.  is_similar definitely should be available still.

> Claudio
> 

(In reply to comment #6)
> (In reply to comment #5)
> > I started to look at it, I am thinking about splitting cairo-xlib-surface.c
> > in two, stuffing everything that requires render extensions
> > into cairo-xlib-surface-xrender.c, and possibly export
> > an interface in cairo-xlib-surface-xrender.h to make available to
> > cairo-xlib-surface.c . Still very cloudy, but the idea is to clutter
> > with preprocessor conditionals as little as possible. 
> 
> I don't think separating them makes much sense.

I still think it would make sense to isolate the xrender stuff,
but I will try to first implement it with a bunch of ifdefs, to see
if I can get it to compile/work first.

> > ATM I have wrapped part of the struct _cairo_xlib_surface declaration in
> > cairo-xlib-surface-private.h in an if like this:
> > 
> > #if CAIRO_HAS_XLIB_XRENDER_SURFACE
> >     int render_major;
> >     int render_minor;
> >     Picture dst_picture, src_picture;
> >     XRenderPictFormat *xrender_format;
> >     XTransform xtransform;
> > #endif
> > 
> > but maybe something better will come to mind.
> > Tomorrow I'll start going through the functions in the .c in more
> > detail, trying to understand how this thing works.
> > 
> > Btw, I've noticed that functions are referenced in a backend table/structure,
> > and in some cases some of these functions only have an implementation that
> > requires render extensions. Should I just put NULL there in these cases
> > and hope for the best? (that applications still work?)
> 
> If you do NULL, cairo will use the fallback code.  So yes.  But you can also
> just return UNSUPPORTED.
> As I said, the current code does work with an X
> server without Xrender.  So, you already got all the answers in there.  Just
> follow what happens if Xrender is detected to be unavailable at run-time and
> make it do the same at compile time.
> 
> > For example, I singled out
> > 
> > _cairo_xlib_surface_create_similar,
> > _cairo_xlib_surface_fill_rectangles,
> > _cairo_xlib_surface_composite_trapezoids,
> > _cairo_xlib_surface_is_similar
> > 
> > as currently being render-ext-only (or so they look to me).
> > Does this make any sense? The domain is very unfamiliar to me.
> 
> There's at least _cairo_xlib_show_glyphs too.  show_glyphs and
> composite_trapezoids should become UNSUPPORTED/NULL with no render I guess, 
> the rest should already have paths working without Xrender.  
> is_similar definitely should be available still.

ok, is_similar without xrender will then just return the
result of _cairo_xlib_surface_same_screen.

I just thought,
since the logic is already there and cairo already works
if xrender is not present at runtime,

what about providing a no-op render extension interface
replacement module if xrender is not found, 
set the xrender_format to NULL,
and let the existing code do the trick?

Would you like this better?

(In reply to comment #8)
> I just thought,
> since the logic is already there and cairo already works
> if xrender is not present at runtime,
> 
> what about providing a no-op render extension interface
> replacement module if xrender is not found, 
> set the xrender_format to NULL,
> and let the existing code do the trick?
> 
> Would you like this better?


I've thought about that before too.  Yes, I think it's a much more maintainable approach.  Probably put those stuff in a cairo-xrender-stub-private.h and be done.

I've done this now.

It can be found in the no-xrender.h branch in my new tree:

git+ssh://annarchy.freedesktop.org/~behdad/public_html/cairo.git
git://annarchy.freedesktop.org/~behdad/cairo.git


I expect it to land soonish.

(In reply to comment #11)
> It can be found in the no-xrender.h branch in my new tree:
> 
> git+ssh://annarchy.freedesktop.org/~behdad/public_html/cairo.git
> git://annarchy.freedesktop.org/~behdad/cairo.git
> 
> 
> I expect it to land soonish.
> 

I was almost done :)
I'll take a look at your changes.

(In reply to comment #12)
> (In reply to comment #11)
> > It can be found in the no-xrender.h branch in my new tree:
> > 
> > git+ssh://annarchy.freedesktop.org/~behdad/public_html/cairo.git
> > git://annarchy.freedesktop.org/~behdad/cairo.git
> > 
> > 
> > I expect it to land soonish.
> > 
> 
> I was almost done :)
> I'll take a look at your changes.

Sorry about that.  It seemed so easy that I just did it :).

Created attachment 11366 [details] [review]
patch

(In reply to comment #14)
> Created an attachment (id=11366) [details]
> patch
> 

thanks for the patch, I was not able to get your tree with git.
It works; the only thing I did differently was that I did not do
a special case for cairo_xlib_surface_create_with_xrender_format
(whose implementation you included in ifdefs),
since if you pass a XRenderPictFormat *format NULL it behaves fine.

Anyway, I think you can close this.
Thanks,

Claudio

(In reply to comment #15)
> (In reply to comment #14)
> > Created an attachment (id=11366) [details] [details]
> > patch
> > 
> 
> thanks for the patch, I was not able to get your tree with git.
> It works; the only thing I did differently was that I did not do
> a special case for cairo_xlib_surface_create_with_xrender_format
> (whose implementation you included in ifdefs),
> since if you pass a XRenderPictFormat *format NULL it behaves fine.

We don't want the cairo_xlib_surface_create_with_xrender_format() symbol in the library.
 
> Anyway, I think you can close this.

Will do after I merge this.

> Thanks,
> 
> Claudio
> 

Updated my branch, adding copyright/license info for the copied files.

In master now.