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 683012 - check: add GstTestClock for use in unit testing
check: add GstTestClock for use in unit testing
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other All
: Normal enhancement
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-08-30 01:38 UTC by Sebastian Rasmussen
Modified: 2012-11-13 22:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
1st proposed patch that introduces GstTestClock (22.58 KB, patch)
2012-08-30 01:39 UTC, Sebastian Rasmussen
none Details | Review
2nd proposed patch that extends GstTestClock (52.21 KB, patch)
2012-08-30 01:40 UTC, Sebastian Rasmussen
none Details | Review
1st proposed patch that introduces GstTestClock (22.73 KB, patch)
2012-08-31 12:58 UTC, Sebastian Rasmussen
committed Details | Review
2nd proposed patch that extends GstTestClock (52.26 KB, patch)
2012-08-31 12:58 UTC, Sebastian Rasmussen
committed Details | Review

Description Sebastian Rasmussen 2012-08-30 01:38:03 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.
Comment 1 Sebastian Rasmussen 2012-08-30 01:39:36 UTC
Created attachment 222903 [details] [review]
1st proposed patch that introduces GstTestClock
Comment 2 Sebastian Rasmussen 2012-08-30 01:40:36 UTC
Created attachment 222904 [details] [review]
2nd proposed patch that extends GstTestClock
Comment 3 Sebastian Rasmussen 2012-08-31 12:58:11 UTC
Created attachment 223057 [details] [review]
1st proposed patch that introduces GstTestClock

Use GST_DEBUG_FUNCPTR()...
Comment 4 Sebastian Rasmussen 2012-08-31 12:58:28 UTC
Created attachment 223058 [details] [review]
2nd proposed patch that extends GstTestClock

Use GST_DEBUG_FUNCPTR()...
Comment 5 Tim-Philipp Müller 2012-11-13 21:10:42 UTC
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.
Comment 6 Tim-Philipp Müller 2012-11-13 21:13:59 UTC
Nevermind about the dispose/finalize, should've read the other patch first.
Comment 7 Tim-Philipp Müller 2012-11-13 22:44:14 UTC
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