[Bug 793839] audiolatency: New plugin for measuring audio latency

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=793839

--- Comment #10 from Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> ---
Review of attachment 368936:
 --> (https://bugzilla.gnome.org/review?bug=793839&attachment=368936)

I have no doubt this element is really useful, but I don't understand why it's
a bin that contains the audio src (unlike the example). Also, the code is not
thread safe, the getters access various internal variable and is racing with
the streaming thread.

::: gst/audiolatency/gstaudiolatency.c
@@ +20,3 @@
+
+/**
+ * SECTION:element-audiolatency

I don't see any changes to the doc/ folder that would accompany this.

@@ +45,3 @@
+ *
+ * For programmatic use, instead of using 'print-stats', you should read the
+ * 'last-latency' and 'average-latency' properties at most once a second.

Why this polling ? Other elements like this will post a periodic message, with
a configurable timer.

@@ +55,3 @@
+
+#define AUDIOLATENCY_CAPS "audio/x-raw, " \
+    "format = (string) F32LE, " \

A bit limiting, but ok.

@@ +144,3 @@
+      g_param_spec_boolean ("print-latency", "Print latencies",
+          "Print the measured latencies on stdout",
+          DEFAULT_PRINT_LATENCY, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

Maybe GST_PARAM_MUTABLE_READY (we often forget) or add locking.

@@ +182,3 @@
+  self->audiosrc = gst_element_factory_make ("audiotestsrc", NULL);
+  g_object_set (self->audiosrc, "wave", 8, "samplesperbuffer", 240, NULL);
+  gst_bin_add (GST_BIN (self), self->audiosrc);

Wait a minute, your example says "autoaudiosrc ! audiolatency print-stats=true
! ...", but here I see that this is a bind that contains the audiosrc.
Something is miss-aligned. I don't get why this is a bin instead of a audio
filter.

@@ +198,3 @@
+
+static gint64
+gst_audiolatency_get_latency (GstAudioLatency * self)

Which lock is protecting this?

@@ +211,3 @@
+
+static void
+gst_audiolatency_set_latency (GstAudioLatency * self, gint64 latency)

Which lock is protecting this ?

@@ +253,3 @@
+  gboolean ret;
+
+  g_assert_cmpint (1, >=, gst_buffer_n_memory (buffer));

Isn't that bad, why don't you just warn and using gst_buffer_map() instead ? At
least it won't just break if you have multiple memory object being appended.

@@ +256,3 @@
+  memory = gst_buffer_peek_memory (buffer, 0);
+  ret = gst_memory_map (memory, &minfo, GST_MAP_READ);
+  g_assert_true (ret);

That's wrong, handle the error here.

@@ +261,3 @@
+  s = gst_caps_get_structure (caps, 0);
+  ret = gst_structure_get_int (s, "channels", &channels);
+  g_assert_true (ret);

Validate this onces when you get the caps, and remove that assertion.

@@ +270,3 @@
+  duration = GST_BUFFER_DURATION (buffer);
+  /* Read only one channel */
+  for (int ii = 1; ii < fsize; ii += channels) {

This is C99, we don't usually do that. Move out the variable declaration
please. And name it just 'i' maybe ? why ii ?

@@ +302,3 @@
+  pts = g_get_monotonic_time ();
+  /* The ticks are once a second, so we can skip checking most buffers */
+  if (self->send_pts > 0 && pts - self->send_pts <= 950 * 1000)

I'd move these magic numbers as defines. Also, we usually do all our constant
math around G_USEC_PER_SEC.

@@ +311,3 @@
+    goto out;
+
+  pts -= offset / 1000;

Like (G_USEC_PER_SEC / 10), the compiler will do the math at compile time, an
we know this is 1 / 10 of a second.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.