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 692901 - Add clutter_stage_set_sync_delay()
Add clutter_stage_set_sync_delay()
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks: 685463
 
 
Reported: 2013-01-30 21:10 UTC by Owen Taylor
Modified: 2013-02-13 14:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add clutter_stage_set_sync_delay() (26.51 KB, patch)
2013-01-30 21:10 UTC, Owen Taylor
none Details | Review
Add clutter_stage_set_sync_delay() (26.36 KB, patch)
2013-02-07 17:14 UTC, Owen Taylor
reviewed Details | Review
Add clutter_stage_set_sync_delay() (26.67 KB, patch)
2013-02-13 06:14 UTC, Owen Taylor
accepted-commit_now Details | Review

Description Owen Taylor 2013-01-30 21:10:35 UTC
New experimental API is added to allow changing the way that redraws
are timed for a stage to include a "sync delay" - a period after
the vertical blanking period where Clutter simply waits for updates.

In detail, the algorithm is that when the master clock is restarted
after drawing a frame (in the case where there are timelines running)
or started fresh in response to a queued redraw or relayout, the
start is scheduled at the next sync point (sync_delay ms after the
predicted vblank period) rather than done immediately.
Comment 1 Owen Taylor 2013-01-30 21:10:37 UTC
Created attachment 234869 [details] [review]
Add clutter_stage_set_sync_delay()
Comment 2 Owen Taylor 2013-01-30 21:12:56 UTC
http://blog.fishsoup.net/2012/11/28/avoiding-jitter-in-composited-frame-display/ is probably useful background material in understanding the motivation.
Comment 3 Owen Taylor 2013-02-07 17:14:23 UTC
Created attachment 235425 [details] [review]
Add clutter_stage_set_sync_delay()

Minor change for final Cogl API.
Comment 4 Emmanuele Bassi (:ebassi) 2013-02-07 18:27:16 UTC
Review of attachment 235425 [details] [review]:

the patch looks good to me, but I still need to test it (I've been traveling for the past two weeks and am about to go home); I also need to double check that Clutter builds on other backends, though it shouldn't be a problem. I just have a couple of questions on the documentation, though.

::: clutter/clutter-master-clock.c
@@ +569,2 @@
   /* We need to protect ourselves against stages being destroyed during
    * event handling

the comment should be modified to say that list_ready_stages() returns a list of ref'd stage pointers, given that the explicit foreach(ref()) is not there any more.

::: clutter/clutter-stage.c
@@ +4508,3 @@
+ * @sync_delay: number of milliseconds after frame presentation to wait
+ *   before painting the next frame. If less than zero, redraw is throttled
+ *   to the refresh rate but not synchronized to it.

is there a way to "undo" the sync delay and allow switching back to vblank synchronized drawing? the sync_delay argument documentation seems to imply that it's impossible, once set_sync_delay() is called, to go back to restore the default behaviour of ClutterStage.

if not: it should be more explicit in the documentation.

@@ +4522,3 @@
+ * and 8 ms (up to one-half of a typical 60hz frame rate) is appropriate.
+ * using a larger value will reduce latency but risks skipping a frame if
+ * drawing the stage takes too long.

should we cap the sync_delay so that it's impossible to set a nonsensical value?

@@ +4523,3 @@
+ * using a larger value will reduce latency but risks skipping a frame if
+ * drawing the stage takes too long.
+ */

missing "Since: 1.14" and "Stability: unstable" annotations for gtk-doc

@@ +4540,3 @@
+ * possible, ignoring any delay that clutter_stage_set_sync_delay()
+ * would normally cause.
+ */

as above, missing Since and Stability annotations

::: clutter/clutter-stage.h
@@ +204,3 @@
 
+#ifdef CLUTTER_ENABLE_EXPERIMENTAL_API
+void            clutter_stage_set_sync_delay                    (ClutterStage          *stage,

missing CLUTTER_AVAILABLE_IN_1_14 annotation.

@@ +206,3 @@
+void            clutter_stage_set_sync_delay                    (ClutterStage          *stage,
+                                                                 gint                   sync_delay);
+void            clutter_stage_skip_sync_delay                   (ClutterStage          *stage);

missing CLUTTER_AVAILABLE_IN_1_14 annotation.
Comment 5 Owen Taylor 2013-02-13 06:12:49 UTC
(In reply to comment #4)
> Review of attachment 235425 [details] [review]:
> 
> the patch looks good to me, but I still need to test it (I've been traveling
> for the past two weeks and am about to go home); I also need to double check
> that Clutter builds on other backends, though it shouldn't be a problem. I just
> have a couple of questions on the documentation, though.
> 
> ::: clutter/clutter-master-clock.c
> @@ +569,2 @@
>    /* We need to protect ourselves against stages being destroyed during
>     * event handling
> 
> the comment should be modified to say that list_ready_stages() returns a list
> of ref'd stage pointers, given that the explicit foreach(ref()) is not there
> any more.

Fixed.

> ::: clutter/clutter-stage.c
> @@ +4508,3 @@
> + * @sync_delay: number of milliseconds after frame presentation to wait
> + *   before painting the next frame. If less than zero, redraw is throttled
> + *   to the refresh rate but not synchronized to it.
> 
> is there a way to "undo" the sync delay and allow switching back to vblank
> synchronized drawing? the sync_delay argument documentation seems to imply that
> it's impossible, once set_sync_delay() is called, to go back to restore the
> default behaviour of ClutterStage.
> 
> if not: it should be more explicit in the documentation.

Changed the text slightly to say:

 * @sync_delay: number of milliseconds after frame presentation to wait                                                                       
 *   before painting the next frame. If less than zero, restores the                                                                          
 *   default behavior where redraw is throttled to the refresh rate but                                                                       
 *   not synchronized to it.  

> @@ +4522,3 @@
> + * and 8 ms (up to one-half of a typical 60hz frame rate) is appropriate.
> + * using a larger value will reduce latency but risks skipping a frame if
> + * drawing the stage takes too long.
> 
> should we cap the sync_delay so that it's impossible to set a nonsensical
> value?

I don't think so. The exact range of meaningful will depend on the frame rate - whether the display is 50hz or 120hz, and I don't think we want to have property rangges depend on something like that which isn't even fixed in the lifetime of the program. And ruling out values which are clearly bad like 1s doesn't really help the programmer any.

> @@ +4523,3 @@
> + * using a larger value will reduce latency but risks skipping a frame if
> + * drawing the stage takes too long.
> + */
> 
> missing "Since: 1.14" and "Stability: unstable" annotations for gtk-doc

Added.
 
> @@ +4540,3 @@
> + * possible, ignoring any delay that clutter_stage_set_sync_delay()
> + * would normally cause.
> + */
> 
> as above, missing Since and Stability annotations

and added.

> ::: clutter/clutter-stage.h
> @@ +204,3 @@
> 
> +#ifdef CLUTTER_ENABLE_EXPERIMENTAL_API
> +void            clutter_stage_set_sync_delay                    (ClutterStage 
>         *stage,
> 
> missing CLUTTER_AVAILABLE_IN_1_14 annotation.
> 
> @@ +206,3 @@
> +void            clutter_stage_set_sync_delay                    (ClutterStage 
>         *stage,
> +                                                                 gint         
>          sync_delay);
> +void            clutter_stage_skip_sync_delay                   (ClutterStage 
>         *stage);
> 
> missing CLUTTER_AVAILABLE_IN_1_14 annotation.

Added. Will attach a new version.
Comment 6 Owen Taylor 2013-02-13 06:14:25 UTC
Created attachment 235870 [details] [review]
Add clutter_stage_set_sync_delay()

New version with fixes.
Comment 7 Emmanuele Bassi (:ebassi) 2013-02-13 06:29:14 UTC
Review of attachment 235870 [details] [review]:

looks good. please, push to master and to the clutter-1.14 branch.
Comment 8 Owen Taylor 2013-02-13 14:49:10 UTC
Pushed to master and clutter-1.14