After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 755014 - Fix Clutter-Gtk on x11
Fix Clutter-Gtk on x11
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: x11
unspecified
Other All
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks:
 
 
Reported: 2015-09-14 23:54 UTC by Lionel Landwerlin
Modified: 2015-09-15 11:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
master-clock-default: prevent deadlock with GLX_INTEL_swap_event (3.13 KB, patch)
2015-09-14 23:54 UTC, Lionel Landwerlin
none Details | Review
x11: stage window: reset framebuffer on foreign window unrealize (7.79 KB, patch)
2015-09-14 23:54 UTC, Lionel Landwerlin
none Details | Review
cogl: reset pending swaps counter on unrealize (1001 bytes, patch)
2015-09-14 23:54 UTC, Lionel Landwerlin
committed Details | Review
master-clock-default: prevent deadlock with GLX_INTEL_swap_event (3.10 KB, patch)
2015-09-15 11:23 UTC, Lionel Landwerlin
committed Details | Review
x11: stage window: reset framebuffer on foreign window unrealize (7.80 KB, patch)
2015-09-15 11:31 UTC, Lionel Landwerlin
accepted-commit_now Details | Review

Description Lionel Landwerlin 2015-09-14 23:54:14 UTC
Though I did my best to test the X11 backend of Clutter in the process of
getting Clutter-Gtk to work properly with the GDK backend of Clutter, I
still manged to break Clutter-Gtk with the X11 backend of Clutter.

Here are the patches to resolve this issue.
Comment 1 Lionel Landwerlin 2015-09-14 23:54:18 UTC
Created attachment 311313 [details] [review]
master-clock-default: prevent deadlock with GLX_INTEL_swap_event

If we call _clutter_stage_do_update() on a ClutterStage that isn't
mapped/visible, no GL command will be queued, and the Mesa/DRI2
implementation of SwapBuffers will do nothing. This causes
GLX_INTEL_swap_event to not be emitted by the X server because no swapping
has been requested through DRI2 and it eventually leads to a deadlock
situation in ClutterStageCogl because we're waiting for an event before we
start the next draw cycle.

This patch removes the non mapped stages from the list of stages to process.
This is consistent with a previous patch for the ClutterMasterClockGdk [1].

[1] : 5733ad58e5a3989f5cb836d42a1cebf3884e7c36
Comment 2 Lionel Landwerlin 2015-09-14 23:54:24 UTC
Created attachment 311314 [details] [review]
x11: stage window: reset framebuffer on foreign window unrealize

Similarly to 13dbb74c81bec861d3a135fb53966ae5562831a7, we need to reset the
framebuffer in the x11 for foreign windows.
Comment 3 Lionel Landwerlin 2015-09-14 23:54:28 UTC
Created attachment 311315 [details] [review]
cogl: reset pending swaps counter on unrealize

When removing the frame callback on the CoglOnscreen, we loose the ability
to get notified of swap events. This could leave us with a counter != 0
which leads to a deadlock situation after the next realize/draw cycle.
Comment 4 Emmanuele Bassi (:ebassi) 2015-09-15 11:21:31 UTC
Review of attachment 311313 [details] [review]:

Looks good, with two minor style issues.

::: clutter/clutter-master-clock-default.c
@@ +148,3 @@
   for (l = stages; l; l = l->next)
     {
+      if (clutter_actor_is_mapped (CLUTTER_ACTOR (l->data)) &&

There's no need to cast here: l->data is a gpointer.

@@ +238,3 @@
        * vblank and really match the vsync frequency.
        */
+      if (clutter_actor_is_mapped (CLUTTER_ACTOR (l->data)) &&

Same as above.
Comment 5 Lionel Landwerlin 2015-09-15 11:23:54 UTC
Created attachment 311351 [details] [review]
master-clock-default: prevent deadlock with GLX_INTEL_swap_event

If we call _clutter_stage_do_update() on a ClutterStage that isn't
mapped/visible, no GL command will be queued, and the Mesa/DRI2
implementation of SwapBuffers will do nothing. This causes
GLX_INTEL_swap_event to not be emitted by the X server because no swapping
has been requested through DRI2 and it eventually leads to a deadlock
situation in ClutterStageCogl because we're waiting for an event before we
start the next draw cycle.

This patch removes the non mapped stages from the list of stages to process.
This is consistent with a previous patch for the ClutterMasterClockGdk [1].

[1] : 5733ad58e5a3989f5cb836d42a1cebf3884e7c36
Comment 6 Emmanuele Bassi (:ebassi) 2015-09-15 11:24:20 UTC
Review of attachment 311314 [details] [review]:

::: clutter/clutter-backend-private.h
@@ +152,3 @@
 PangoDirection          _clutter_backend_get_keymap_direction           (ClutterBackend         *backend);
 
+void                    _clutter_backend_reset_framebuffer              (ClutterBackend         *backend);

I'd call this `reset_cogl_framebuffer`, to make it clear what it does.

::: clutter/clutter-backend.c
@@ +1453,3 @@
+                                      &internal_error))
+        {
+      CoglError *internal_error = NULL;

g_error() is an abort() wrapper.

If you want to display a critical warning, use g_critical().
Comment 7 Emmanuele Bassi (:ebassi) 2015-09-15 11:24:44 UTC
Review of attachment 311315 [details] [review]:

Nice catch.
Comment 8 Emmanuele Bassi (:ebassi) 2015-09-15 11:26:16 UTC
Review of attachment 311351 [details] [review]:

Okay.
Comment 9 Lionel Landwerlin 2015-09-15 11:31:07 UTC
Created attachment 311352 [details] [review]
x11: stage window: reset framebuffer on foreign window unrealize

Similarly to 13dbb74c81bec861d3a135fb53966ae5562831a7, we need to reset the
framebuffer in the x11 for foreign windows.
Comment 10 Lionel Landwerlin 2015-09-15 11:32:23 UTC
Review of attachment 311315 [details] [review]:

Pushed to master.
Comment 11 Lionel Landwerlin 2015-09-15 11:32:35 UTC
Review of attachment 311351 [details] [review]:

Pushed to master.
Comment 12 Lionel Landwerlin 2015-09-15 11:33:09 UTC
Review of attachment 311352 [details] [review]:

Pushed to master.
Comment 13 Emmanuele Bassi (:ebassi) 2015-09-15 11:33:25 UTC
Review of attachment 311352 [details] [review]:

Looks good.
Comment 14 Lionel Landwerlin 2015-09-15 11:33:42 UTC
Thanks for reviewing patches so fast!