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 749391 - PTP network clock support
PTP network clock support
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-14 17:43 UTC by Sebastian Dröge (slomo)
Modified: 2015-06-09 17:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
clock: Add GST_CLOCK_FLAG_NEEDS_STARTUP_SYNC and related API (8.12 KB, patch)
2015-06-03 11:18 UTC, Sebastian Dröge (slomo)
none Details | Review
clock: Add GST_CLOCK_FLAG_NEEDS_STARTUP_SYNC and related API (8.57 KB, patch)
2015-06-03 11:35 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Sebastian Dröge (slomo) 2015-05-14 17:43:49 UTC
I've implemented a PTP based network clock for libgstnet here: http://cgit.freedesktop.org/~slomo/gstreamer/tree/?h=ptp

Would be great to get some review of the code and API before I merge it into core :)
Comment 1 Sebastian Dröge (slomo) 2015-05-29 10:32:29 UTC
On IRC it was mentioned that the clock waiting and "clock not ready" stuff should be merged into GstClock instead, as we have the same problem with other clocks and something generic to handle this would be nice.

Proposed API:
flag:   GST_CLOCK_FLAG_CAN_BE_NOT_READY = (1<<6)
signal: void ready-state-changed(GstClock *clock, gboolean ready);
method: gboolean gst_clock_wait_ready(GstClock *clock, GstClockTime timeout);
        gboolean gst_clock_is_ready(GstClock *clock);


Comments?
Comment 2 Jan Schmidt 2015-05-29 11:07:59 UTC
I think the flag and 'readiness' is the same as the network clock - at startup time, the clock doesn't know what time to report until it has some observations of the master clock.

Maybe with that in mind, maybe something like

GST_CLOCK_FLAG_NEEDS_STARTUP_SYNC and gst_clock_wait_for_sync() gst_clock_is_synced()

?
Comment 3 Sebastian Dröge (slomo) 2015-05-29 11:08:53 UTC
I like that, let's go for that :)
Comment 4 Sebastian Dröge (slomo) 2015-06-03 11:18:06 UTC
Created attachment 304485 [details] [review]
clock: Add GST_CLOCK_FLAG_NEEDS_STARTUP_SYNC and related API

gst_clock_wait_for_sync(), gst_clock_is_synced() and gst_clock_set_synced()
plus a signal to asynchronously wait for the clock to be synced.

This can be used by clocks to signal that they need initial synchronization
before they can report any time, and that this synchronization can also get
completely lost at some point. Network clocks, like the GStreamer
netclientclock, NTP or PTP clocks are examples for clocks where this is useful
to have as they can't report any time at all before they're synced.
Comment 5 Jan Schmidt 2015-06-03 11:33:03 UTC
Review of attachment 304485 [details] [review]:

Works for me!
Comment 6 Tim-Philipp Müller 2015-06-03 11:33:52 UTC
Comment on attachment 304485 [details] [review]
clock: Add GST_CLOCK_FLAG_NEEDS_STARTUP_SYNC and related API

Looks good to me. Some docs nitpicks:

>+  /**
>+   * GstElement::synced:

GstClock::synced ?

Might be good to mention explicitly that this signal will be emitted from a non-application thread.


>+/**
>+ * gst_clock_wait_for_sync:

Would be good to mention in the docs the 'async' variant (i.e. the signal).

I initially was not so fond of the synced=true/false boolean, but a SyncStatus enum seems to be a bit over the top as well, especially if it only has two values, so I guess it's all good.
Comment 7 Sebastian Dröge (slomo) 2015-06-03 11:35:32 UTC
Created attachment 304495 [details] [review]
clock: Add GST_CLOCK_FLAG_NEEDS_STARTUP_SYNC and related API

gst_clock_wait_for_sync(), gst_clock_is_synced() and gst_clock_set_synced()
plus a signal to asynchronously wait for the clock to be synced.

This can be used by clocks to signal that they need initial synchronization
before they can report any time, and that this synchronization can also get
completely lost at some point. Network clocks, like the GStreamer
netclientclock, NTP or PTP clocks are examples for clocks where this is useful
to have as they can't report any time at all before they're synced.
Comment 8 Sebastian Dröge (slomo) 2015-06-03 11:56:12 UTC
All merged, thanks for the reviews :)
Comment 9 Nicolas Dufresne (ndufresne) 2015-06-03 13:13:06 UTC
Review of attachment 304485 [details] [review]:

Looks good.
Comment 10 Carlos Rafael Giani 2015-06-09 12:36:59 UTC
One addendum: gst_clock_wait_ready() is a good idea, but should be interruptible. Imagine writing a sink bin that internally sets up a netclientclock or a PTP clock, and waits for a long time (because of a large timeout). It should still be possible then to abort the wait, for example during a PAUSED->READY state change, with a call like gst_clock_abort_wait().
Comment 11 Olivier Crête 2015-06-09 17:33:22 UTC
(In reply to Carlos Rafael Giani from comment #10)
> with a call like gst_clock_abort_wait().

That is probably a good case for a GCancellable