GNOME Bugzilla – Bug 657434
Add a GDK backend to Clutter
Last modified: 2011-11-03 18:04:43 UTC
Some applications (my case is mutter) want to use both Clutter and GDK/Gtk, including processing events consistently from both libraries. So far the gluing has been done by libX11, but in the future we want to move away from X11, therefore there must be a single library abstracting all the window system interaction (with a special attention to event retrieval).
Created attachment 194823 [details] [review] Add a new GDK backend This commit introduces a new flavour for Clutter, that uses GDK for handling all window system specific interactions (except for creating the cogl context, as cogl does not know about GDK), including in particular events. This is not compatible with the X11 (glx) flavour, and this is reflected by the different soname (libclutter-gdk-1.0.so), as all X11 specific functions and classes are not available. If you wish to be compatible, you should check for CLUTTER_WINDOWING_X11. Other than that, this backend should be on feature parity with X11, including XInput 2, XSettings and EMWH (with much, much less code)
Review of attachment 194823 [details] [review]: first of all, this looks pretty cool! thanks for doing this :-) I have a design issue, though; I don't want the Cogl backend to use the GDK one — i.e. I don't want the GDK backend to be like the X11 backend. it makes maintaining code much more complicated through all the ifdefs. what I'd like is for the GDK backend to be a fully separate backend, using GDK and Cogl together; the major advantage for that would be that we would have runtime detection of both the GL implementation (through Cogl) and the windowing system (through GDK), and thus have a single shared object (libclutter-1.0.so). I think we can work this out incrementally for the next cycle.
(In reply to comment #2) > Review of attachment 194823 [details] [review]: > > first of all, this looks pretty cool! thanks for doing this :-) > > I have a design issue, though; I don't want the Cogl backend to use the GDK one > — i.e. I don't want the GDK backend to be like the X11 backend. it makes > maintaining code much more complicated through all the ifdefs. > > what I'd like is for the GDK backend to be a fully separate backend, using GDK > and Cogl together; the major advantage for that would be that we would have > runtime detection of both the GL implementation (through Cogl) and the > windowing system (through GDK), and thus have a single shared object > (libclutter-1.0.so). > > I think we can work this out incrementally for the next cycle. But still, there is a lot of code in the Cogl backend, especially that handling clipped redraws, that would be duplicated if the GDK backend. My idea was to have a stub "native" backend, taking out from cogl what is currently used in the drm+evdev/tslib cases. This way, ClutterBackendCogl and ClutterStageCogl can unconditionally chain up to the parent, removing many of the ifdefs.
Created attachment 194889 [details] [review] Rework the interaction between the Cogl and GDK / X11 backends. Previously, the Cogl backend was at times a subclass of the X11 backend, and at times a standalone one. Now it is the other way round, with GDK and X11 backends providing the concrete classes, layered on top of the generic Cogl backend. A new EglNative backend was introduced for direct to framebuffer rendering. This greatly simplifies the API design (at the expense of some casts needed) and reduces the amount of #ifdefs, without duplicating code.
Review of attachment 194889 [details] [review]: this looks *a ton* better, thanks! ::: clutter/egl/clutter-backend-eglnative.c @@ +243,3 @@ + +/* FIXME we should have a CLUTTER_ define for this */ +#ifdef COGL_HAS_EGL_PLATFORM_GDL_SUPPORT the API is public, so it should not disappear if Cogl doesn't have support for GDL. the #ifdef should be moved inside the body of the function. ::: clutter/gdk/clutter-stage-gdk.c @@ +294,3 @@ { ClutterStageGdk *stage_gdk = CLUTTER_STAGE_GDK (stage_window); + ClutterStage *stage = CLUTTER_STAGE_COGL (stage_window)->wrapper; this looks wrong @@ +500,2 @@ if (stage_gdk != NULL && CLUTTER_IS_STAGE_GDK (stage_gdk)) + return CLUTTER_STAGE_COGL (stage_gdk)->wrapper; this looks wrong too. ::: clutter/x11/clutter-backend-x11.c @@ +690,3 @@ + CoglOnscreenTemplate *onscreen_template = NULL; + +{ please, use an explicit check against NULL - here and everywhere you're doing a NULL pointer check.
(In reply to comment #5) > Review of attachment 194889 [details] [review]: > > this looks *a ton* better, thanks! > > ::: clutter/egl/clutter-backend-eglnative.c > @@ +243,3 @@ > + > +/* FIXME we should have a CLUTTER_ define for this */ > +#ifdef COGL_HAS_EGL_PLATFORM_GDL_SUPPORT > > the API is public, so it should not disappear if Cogl doesn't have support for > GDL. the #ifdef should be moved inside the body of the function. The same happens in clutter master (including the FIXME), I just copied the code. > ::: clutter/gdk/clutter-stage-gdk.c > @@ +294,3 @@ > { > ClutterStageGdk *stage_gdk = CLUTTER_STAGE_GDK (stage_window); > + ClutterStage *stage = CLUTTER_STAGE_COGL (stage_window)->wrapper; > > this looks wrong No, it's right: I moved the wrapper and backend backpointers from ClutterStageGdk / ClutterStageX11 to ClutterStageCogl, so to obtain the wrapper I need to cast the ClutterStageWindow. > @@ +500,2 @@ > if (stage_gdk != NULL && CLUTTER_IS_STAGE_GDK (stage_gdk)) > + return CLUTTER_STAGE_COGL (stage_gdk)->wrapper; > > this looks wrong too. Same as before, this is correct. > ::: clutter/x11/clutter-backend-x11.c > @@ +690,3 @@ > + CoglOnscreenTemplate *onscreen_template = NULL; > + > +{ > > please, use an explicit check against NULL - here and everywhere you're doing a > NULL pointer check. Ok.
(In reply to comment #6) > (In reply to comment #5) > > Review of attachment 194889 [details] [review] [details]: > > > > this looks *a ton* better, thanks! > > > > ::: clutter/egl/clutter-backend-eglnative.c > > @@ +243,3 @@ > > + > > +/* FIXME we should have a CLUTTER_ define for this */ > > +#ifdef COGL_HAS_EGL_PLATFORM_GDL_SUPPORT > > > > the API is public, so it should not disappear if Cogl doesn't have support for > > GDL. the #ifdef should be moved inside the body of the function. > > The same happens in clutter master (including the FIXME), I just copied the > code. I know, and the current master is wrong too - I just noticed through your patch.
Created attachment 195118 [details] [review] Rework the interaction between the Cogl and GDK / X11 backends. Previously, the Cogl backend was at times a subclass of the X11 backend, and at times a standalone one. Now it is the other way round, with GDK and X11 backends providing the concrete classes, layered on top of the generic Cogl backend. A new EglNative backend was introduced for direct to framebuffer rendering. This greatly simplifies the API design (at the expense of some casts needed) and reduces the amount of #ifdefs, without duplicating code.
these patches have been merged (and cleaned up) in the multi-backend branch of the Clutter repository: http://git.gnome.org/browse/clutter/log/?h=multi-backend just in case people want to track it. the multi-backend branch also tries to solve this bug as well: http://bugzilla.clutter-project.org/show_bug.cgi?id=2276 by compiling multiple backends into the same SO, and doing run-time selection.
(In reply to comment #8) > Created an attachment (id=195118) [details] [review] > Rework the interaction between the Cogl and GDK / X11 backends. > > Previously, the Cogl backend was at times a subclass of the X11 > backend, and at times a standalone one. Now it is the other way > round, with GDK and X11 backends providing the concrete classes, > layered on top of the generic Cogl backend. A new EglNative backend > was introduced for direct to framebuffer rendering. This greatly > simplifies the API design (at the expense of some casts needed) > and reduces the amount of #ifdefs, without duplicating code. thanks for looking at this stuff. One thing I'm not sure about is the new "EglNative" platform. Clutter ideally shouldn't need to know anything about EGL, it's an implementation detail in Cogl so it doesn't seem right to have a clutter backend that explicitly implies it's tied to EGL. GDL interaction as far as clutter is concerned isn't implicitly tied to EGL, the GDL api is just an api for Sodaville / set-top-box platforms for configuring display planes, it's an internal detail for Cogl that it has integration with EGL but Clutter shouldn't need to care or know about that. Similarly tslib and evdev usage have nothing to do with EGL as far as clutter is concerned they are just alternative ways of getting input events. I think at this point it makes sense to map the new backend names simply to the input backend choice, which would imply splitting the EglNative backend into Evdev and Tslib backends. All the CEX100 stuff in clutter should be handled by the base cogl backend and clutter shouldn't assume it has anything to do with EGL.
(In reply to comment #10) I pushed the multi-backend branch to master because it was becoming absolutely unwieldy to handle, and it can be used as a trampoline to get the EGL situation under control along with Wayland's compositor branch. I'm already working on moving the overall Cogl-related code into ClutterBackend instead of the platform-specific classes. > One thing I'm not sure about is the new "EglNative" platform. Clutter ideally > shouldn't need to know anything about EGL, it's an implementation detail in > Cogl so it doesn't seem right to have a clutter backend that explicitly implies > it's tied to EGL. agree; right now, the > GDL interaction as far as clutter is concerned isn't implicitly tied to EGL, > the GDL api is just an api for Sodaville / set-top-box platforms for > configuring display planes, it's an internal detail for Cogl that it has > integration with EGL but Clutter shouldn't need to care or know about that. sadly, we expose API for that, so I don't think we can get away with just asking Cogl to do this. so I think we should have a CEX100 windowing backend in Clutter which effectively only works in terms of exposing the GDL API for selecting the plane. > Similarly tslib and evdev usage have nothing to do with EGL as far as clutter > is concerned they are just alternative ways of getting input events. regarding tslib: I think it's time to remove support for it from Clutter; it surprises me it even works, given that it doesn't have any support for current event handling. > I think at this point it makes sense to map the new backend names simply to the > input backend choice, which would imply splitting the EglNative backend into > Evdev and Tslib backends. agree; I already started to split them up. > All the CEX100 stuff in clutter should be handled by the base cogl backend and > clutter shouldn't assume it has anything to do with EGL. we still need a symbol for detecting that we are on a GDL-capable platform, so that you can call clutter_cex100_set_buffering_mode() and clutter_cex100_set_plane().
(In reply to comment #11) > > One thing I'm not sure about is the new "EglNative" platform. Clutter ideally > > shouldn't need to know anything about EGL, it's an implementation detail in > > Cogl so it doesn't seem right to have a clutter backend that explicitly implies > > it's tied to EGL. > agree; right now, the eglnative backend is just selecting the input backend, which we can effectively do in the generic code paths.
okay, I've been making progress on the whole "let's get rid of a bunch of backend code" task. since I merged the multi-backend branch, and the GDK backend is in master, let's close this bug and start tracking the removal of the EGL backend on bug 663344.
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.