GNOME Bugzilla – Bug 683012
check: add GstTestClock for use in unit testing
Last modified: 2012-11-13 22:44:40 UTC
I recently came across this bug which has some interesting code: https://bugzilla.gnome.org/show_bug.cgi?id=667838 One of the attachments is GstTestClock along with unit test code for this clock. This code appears to be useful outside of the context of gstrtpjitterbuffer (which bug 667838 concerns). So I have take GstTestClock, separated it into two patches: the first patch adds GstTestClock whose time can be advanced deterministically through a function call, the other patch adds the remainder of the original implementation -- namely support for clock notifications. Moreover I have tested it, documented it, valgrind:ed it and lcov:ed it. Without further review comments I don't know how to improve upon it and make it appealing for merging into 0.11. Once this it has been accepted, I'm hoping that I can convince someone to merging this into 0.10 as well.
Created attachment 222903 [details] [review] 1st proposed patch that introduces GstTestClock
Created attachment 222904 [details] [review] 2nd proposed patch that extends GstTestClock
Created attachment 223057 [details] [review] 1st proposed patch that introduces GstTestClock Use GST_DEBUG_FUNCPTR()...
Created attachment 223058 [details] [review] 2nd proposed patch that extends GstTestClock Use GST_DEBUG_FUNCPTR()...
Review of attachment 223057 [details] [review]: Looks great! Only some minor niggles, but generally good to go IMHO. ::: libs/gst/check/gsttestclock.c @@ +60,3 @@ + * arguments. This will highlight any issues with the unit test code itself. + */ + Should still include config.h first thing, with HAVE_CONFIG_H guards. @@ +66,3 @@ +{ + PROP_0, + PROP_START_TIME, No trailing comma please, some compilers really don't like that (yes, really). @@ +83,3 @@ +}; + +#define GST_TEST_CLOCK_GET_PRIVATE(obj) ((GST_TEST_CLOCK_CAST (obj))->priv) I would prefer to drop this macro and just do testclock->priv->foo directly. Easier to read, fewer variables, more in line with how it's used elsewhere. In a few performance-sensitive cases it might make sense to retrieve FooPrivate first and then use priv->foo. @@ +179,3 @@ +{ + G_OBJECT_CLASS (parent_class)->finalize (object); +} Why override and implement dispose and finalize if we don't have anything to do? @@ +186,3 @@ +{ + GstTestClock *test_clock = GST_TEST_CLOCK (object); + GstTestClockPrivate *priv = GST_TEST_CLOCK_GET_PRIVATE (test_clock); See above about test_clock->priv->foo @@ +194,3 @@ + default: + G_OBJECT_CLASS (parent_class)->set_property (object, property_id, value, + pspec); This is not right. {get,set}_property are special vfuncs. GObject maps the property IDs to the right class, so there's no need to "chain up" to the parent class. The default label should contain the standard warning about an unrecognises property_id, which you can copy'n'paste from elsewhere (not that that code is likely to ever be called..). @@ +287,3 @@ + * gst_clock_get_time() is a programming error. + * + * MT safe. Please add a "Since: 1.2" marker, to the other doc chunks as well.
Nevermind about the dispose/finalize, should've read the other patch first.
commit c6cc50e6de5b578e93e27c3ccd61801a96011fbf Author: Tim-Philipp Müller <tim@centricular.net> Date: Tue Nov 13 22:42:05 2012 +0000 testclock: minor cleanups, add since markers for gtk-doc https://bugzilla.gnome.org/show_bug.cgi?id=683012 commit 4eeb471e11b35bdf0e9aa95ea354219036035d76 Author: Sebastian Rasmussen <sebrn@axis.com> Date: Thu Aug 30 01:58:41 2012 +0200 check: allow GstTestClock to handle clock notifications API: gst_test_clock_peek_id_count() API: gst_test_clock_has_id() API: gst_test_clock_peek_next_pending_id() API: gst_test_clock_wait_for_next_pending_id() API: gst_test_clock_wait_for_pending_id_count() API: gst_test_clock_process_next_clock_id() API: gst_test_clock_get_next_entry_time() https://bugzilla.gnome.org/show_bug.cgi?id=683012 commit 77005be192a884fc47ee7250fa6095b7ab2109d9 Author: Tim-Philipp Müller <tim@centricular.net> Date: Tue Nov 13 21:29:01 2012 +0000 check: add dependency on gstcheck header files for exports.sym So exports.sym gets updated correctly, and our new symbols get exported correctly, which makes g-ir-scanner much happier in terms of linking. https://bugzilla.gnome.org/show_bug.cgi?id=683012 commit e58da2a22d5d01cf080466a474aac2ac89198838 Author: Sebastian Rasmussen <sebrn@axis.com> Date: Wed Aug 29 16:11:10 2012 +0200 check: add GstTestClock as a deterministic clock for testing API: GstTestClock API: gst_test_clock_new() API: gst_test_clock_new_with_start_time() API: gst_test_clock_set_time() API: gst_test_clock_advance_time() https://bugzilla.gnome.org/show_bug.cgi?id=683012