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 744668 - GDK: integrate the MasterClock mechanism with GdkFrameClock
GDK: integrate the MasterClock mechanism with GdkFrameClock
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks:
 
 
Reported: 2015-02-17 15:52 UTC by Lionel Landwerlin
Modified: 2015-02-17 19:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
main: Make the MasterClock an interface (51.13 KB, patch)
2015-02-17 15:52 UTC, Lionel Landwerlin
none Details | Review
gdk: implement a MasterClock based on GdkFrameClock (25.41 KB, patch)
2015-02-17 15:52 UTC, Lionel Landwerlin
none Details | Review
main: Make the MasterClock an interface (51.13 KB, patch)
2015-02-17 17:10 UTC, Lionel Landwerlin
needs-work Details | Review
gdk: implement a MasterClock based on GdkFrameClock (25.75 KB, patch)
2015-02-17 17:10 UTC, Lionel Landwerlin
needs-work Details | Review
main: Make the MasterClock an interface (50.89 KB, patch)
2015-02-17 18:15 UTC, Lionel Landwerlin
committed Details | Review
gdk: implement a MasterClock based on GdkFrameClock (25.07 KB, patch)
2015-02-17 18:53 UTC, Lionel Landwerlin
committed Details | Review

Description Lionel Landwerlin 2015-02-17 15:52:39 UTC
Attached are a couple of to implement a special MasterClock for the
GDK backend, so we can be synchronized with the compositor from a
Clutter application.

I left some code in comment in clutter/gdk/clutter-master-clock-gdk.c,
this is because I'm still looking for ideas to support
CLUTTER_DEBUG_CONTINUOUS_REDRAW. Maybe this only option is to have
something similar in GdkFrameClock.
Comment 1 Lionel Landwerlin 2015-02-17 15:52:43 UTC
Created attachment 297031 [details] [review]
main: Make the MasterClock an interface

Move the implementation of the MasterClock into MasterClockDefault, so
backends can provide their own implementation.
Comment 2 Lionel Landwerlin 2015-02-17 15:52:49 UTC
Created attachment 297032 [details] [review]
gdk: implement a MasterClock based on GdkFrameClock
Comment 3 Emmanuele Bassi (:ebassi) 2015-02-17 16:25:52 UTC
fun segfault that happens when closing examples/canvas:

  • #0 _clutter_stage_do_update
    at clutter-stage.c line 1242
  • #1 master_clock_update_stage
    at gdk/clutter-master-clock-gdk.c line 234
  • #2 clutter_master_clock_gdk_update
    at gdk/clutter-master-clock-gdk.c line 319
  • #3 _g_closure_invoke_va
    at gclosure.c line 831
  • #4 g_signal_emit_valist
    at gsignal.c line 3214
  • #5 g_signal_emit_by_name
    at gsignal.c line 3401
  • #6 gdk_frame_clock_paint_idle
    at gdkframeclockidle.c line 380
  • #7 gdk_threads_dispatch
    at gdk.c line 717
  • #8 g_timeout_dispatch
    at gmain.c line 4545
  • #9 g_main_dispatch
    at gmain.c line 3122
  • #10 g_main_context_dispatch
    at gmain.c line 3737
  • #11 g_main_context_iterate
    at gmain.c line 3808
  • #12 g_main_loop_run
    at gmain.c line 4002
  • #13 clutter_main
    at clutter-main.c line 844
  • #14 main
    at canvas.c line 167

the stage pointer is valid, but the private data is garbage:

(gdb) print stage
$1 = (ClutterStage *) 0x216e480
(gdb) print stage->priv
$2 = (ClutterStagePrivate *) 0x630
(gdb) print *stage->priv
Cannot access memory at address 0x630

which means that some reference is leaking, as the frame clock is trying to update a closed stage.
Comment 4 Lionel Landwerlin 2015-02-17 17:10:20 UTC
Created attachment 297035 [details] [review]
main: Make the MasterClock an interface

Move the implementation of the MasterClock into MasterClockDefault, so
backends can provide their own implementation.
Comment 5 Lionel Landwerlin 2015-02-17 17:10:25 UTC
Created attachment 297036 [details] [review]
gdk: implement a MasterClock based on GdkFrameClock
Comment 6 Lionel Landwerlin 2015-02-17 17:13:06 UTC
It was just that the default master clock uses the peek_stages() that allows the list to be modified as run the clock update.
Therefore in the latest step, when updating the stages, the list might have become empty.
The GDK master clock didn't handle that case :(

Thanks!
Comment 7 Emmanuele Bassi (:ebassi) 2015-02-17 17:51:13 UTC
Review of attachment 297035 [details] [review]:

looks generally good; some minor coding style nitpicking.

::: clutter/clutter-master-clock.c
@@ +46,1 @@
+G_DEFINE_INTERFACE (ClutterMasterClock, clutter_master_clock, G_TYPE_OBJECT);

remove the trailing semi-colon.

@@ +56,3 @@
   ClutterMainContext *context = _clutter_context_get_default ();
 
+  if (G_UNLIKELY (context->master_clock == NULL)) {

coding style: braces on a new line.

@@ +129,3 @@
 }
 
+void _clutter_master_clock_set_paused (ClutterMasterClock *master_clock,

coding style: new line between return value and function name.
Comment 8 Lionel Landwerlin 2015-02-17 18:15:18 UTC
Created attachment 297045 [details] [review]
main: Make the MasterClock an interface

Move the implementation of the MasterClock into MasterClockDefault, so
backends can provide their own implementation.
Comment 9 Emmanuele Bassi (:ebassi) 2015-02-17 18:30:16 UTC
Review of attachment 297036 [details] [review]:

looks generally okay; a couple nitpicks.

::: clutter/gdk/clutter-master-clock-gdk.h
@@ +25,3 @@
+#define __CLUTTER_MASTER_CLOCK_GDK_H__
+
+#include <clutter/clutter-timeline.h>

unnecessary include; you can use <glib-object.h> or <clutter/clutter-types.h>.

@@ +42,3 @@
+};
+
+GType _clutter_master_clock_gdk_get_type (void) G_GNUC_CONST;

no need to use a leading underscore.
Comment 10 Emmanuele Bassi (:ebassi) 2015-02-17 18:52:23 UTC
Review of attachment 297045 [details] [review]:

looks good.
Comment 11 Lionel Landwerlin 2015-02-17 18:53:20 UTC
Created attachment 297051 [details] [review]
gdk: implement a MasterClock based on GdkFrameClock
Comment 12 Lionel Landwerlin 2015-02-17 18:54:46 UTC
Removed the commented code and put a g_warning instead, saying continuous-redraw is not supported with the GDK backend.
Comment 13 Emmanuele Bassi (:ebassi) 2015-02-17 19:11:41 UTC
Review of attachment 297051 [details] [review]:

looks good.
Comment 14 Lionel Landwerlin 2015-02-17 19:13:19 UTC
Review of attachment 297045 [details] [review]:

Pushed to master.
Comment 15 Lionel Landwerlin 2015-02-17 19:14:01 UTC
Review of attachment 297051 [details] [review]:

Pushed to master too.
Thanks!