GNOME Bugzilla – Bug 361155
Add monotonic clock
Last modified: 2009-02-03 17:08:09 UTC
Add a GstPosixClock (based on GstSystemClock) that uses POSIX monotonic clock. The POSIX clock ought to be the default clock on all platforms that supports POSIX monotonic clock.
Created attachment 74406 [details] [review] Patch that add GstPosixClock and makes it the default clock on supported platforms. The check test is a copy of the GstSystemClock test.
Created attachment 74470 [details] [review] Gunzipped patch gunzipped 74406
after some discussion on IRC: 1) an enum property "clock-type" to GstSystemClock with all the different (available, check _POSIX_MONOTONIC_CLOCK, etc..) posix clocks would be better: - CLOCK_REALTIME - CLOCK_MONOTONIC - CLOCK_PROCESS_CPUTIME_ID - CLOCK_THREAD_CPUTIME_ID 2) if no posix clocks are available, only CLOCK_REALTIME is available and it uses the portable g_get_current_time(). 3) The default clock-type would be CLOCK_REALTIME for now.
Created attachment 74633 [details] [review] Adds clock-type property to GstSystemClock The check test is just "hacked" to use CLOCK_MONOTONIC for now. It ought to exercise all supported clocks.
Created attachment 75080 [details] [review] Adds posix timers support to GstSystemClock via clock-type property. Uses a glib main context to wait. Padding in GstSystemClock is probably still wrong.
Created attachment 75288 [details] [review] Updated patch Updated patch: - add some new defines in gstclock.h - fix padding - rework the main clock_wait loop
At the end of _wait_jitter_unlocked the source is _destroy:ed and _unref:ed. I think _destroy actually _unref:s as well. Cosmetics: s/((TimeoutSource *)source/TIMEOUT_SOURCE_CAST (source)/ in _wait_jitter_unlocked()
_destroy() unrefs but we still hold a ref (g_source_attach() does not take a _ref).
Created attachment 75434 [details] [review] updated patch previous patch contained a race, we need to recheck if the entry was unscheduled when we acquire the clock lock.
Created attachment 75731 [details] [review] alternative patch This patch uses ne mainloop in a thread and uses just one GSource to signal a GCond when an entry times out.
Msecs needs to be rounded up to pass the unittests.
Created attachment 76762 [details] [review] Updated alternative patch Made the unittest and valgrind happy: * Added rounding of msecs to GST_TIME_AS_MSECONDS * Freed entry in timeout_dispatch
Why hasn't this patch been committed yet and why is this bug still UNCONFIRMED after 8 months of the last patch/comment?
I guess because it's not so important? Marking it CONFIRMED all the same considering there's been work on it. Zeeshan, ping the authors to know what their feeling is about it.
It is important. Not for everyone, but for those that cannot have a running pipeline locking up when the system clock is adjusted (like some embedded systems). Wim was about to "port" this patch to current CVS some weeks ago, but I think he has had more important things on his mind lately.
Which of the 3 attached patches is the latest/best?
(In reply to comment #16) > Which of the 3 attached patches is the latest/best? > None of them applies to the latest cvs, afaik but i would recommend looking into the latest one attached.
Maybe the patch should also be splitted to have the implementation and the tests separate. I've just cleaned up the test for the clock, so that part would not apply cleanly anymore :/
Created attachment 99092 [details] [review] Updated alternative patch with bugfixes Updated to match latest CVS. Bugfixes: * Sync wait will not replace dispatch func of clock entry. * Unscheduling an entry will broadcast the cond so that unscheduled events can return from wait.
I wonderif we should get this in step by step. Imho the configure check can be applied immediately after freeze. Then the monotonic clock can/should be used for he debug-log times stamps too.
* docs/gst/gstreamer-sections.txt: * gst/gstclock.h: * tests/check/gst/gstsystemclock.c: (GST_START_TEST), (gst_systemclock_suite): Start merging in the easy bits of #361155, the monotonic clock patch. This one adds a few handy macros with docs and a testsuite.
Note that I changed the macro to round down to the whole msecond. You might want to change the other code that needs rounding up. I don't think the macro should round up by default.
Created attachment 99776 [details] [review] Updated alternative patch with bugfixes after partial merge
_unschedule() must broadcast, but not wakeup the main context, unconditionally to wake entries in sync wait that are not first in list.
Created attachment 123755 [details] [review] Alternative patch using gst_poll_wait New attempt using gst_poll_wait, based on Wim's pseudocode http://webcvs.freedesktop.org/gstreamer/gstreamer/docs/random/wtay/poll-timeout?revision=1.3
Created attachment 124929 [details] [review] Updated alternative patch using gst_poll_wait Updated alternative patch using gst_poll_wait: changed enum value names.
Created attachment 127408 [details] [review] GstPoll patch to implement timers this patch adds some methods to GstPoll to make it work for the timer stuff better.
Created attachment 127755 [details] [review] Using gst_poll_wait with GstPoll as timer All in one patch: Alternative patch using gst_poll_wait with GstPoll as timer + the GstPoll patch
commit abffdb2ac384740831352eb5957ecfad15428cd7 Author: Jonas Holmberg <jonas.holmberg@axis.com> Date: Tue Feb 3 18:04:46 2009 +0100 Implement the systemclock with gstpoll Add a property to select the clock type, currently REALTIME and MONOTONIC when posix timers are available. Implement the systemclock with GstPoll instead of GCond. This allows us to schedule timeouts with nanosecond precission on newer kernels and with ppoll support. It's also resilient to changes to the systemclock because of NTP or similar. commit 5cfb02af4a939e44a72cfffc8b94d0c5ec56d61e Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Tue Feb 3 17:49:02 2009 +0100 GstPoll: add methods to use gstpoll for timeouts Add a special timer mode in GstPoll that makes it only use the control socket with a timeout to schedule timeouts. Also add a pair of methods to wakeup the timeout thread. API: GstPoll::gst_poll_new_timer() API: GstPoll::gst_poll_write_control() API: GstPoll::gst_poll_read_control()