[Bug 787358] debugutils: Added new jitterer element

[GStreamer (GNOME Bugzilla)]

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

Nicolas Dufresne (stormer) <nicolas at ndufresne.ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #362417|none                        |needs-work
             status|                            |

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

::: gst/debugutils/gstjitterer.c
@@ +70,3 @@
+
+#define parent_class gst_jitterer_parent_class
+G_DEFINE_TYPE (GstJitterer, gst_jitterer, GST_TYPE_ELEMENT);

I think this should be a GstBaseTransform running "in-place" mode. This way
everything will be properly forwarded. It's far in my memory, but I think there
is strong motivation why "identity" element is base on that too.

@@ +103,3 @@
+  g_object_class_install_property (object_class, PROP_JITTER_AMPL,
+      g_param_spec_uint64 ("jitter-amplitude", "Jitter amplitude",
+          "Amplitude of the jitter to apply",

For extra clarity, can you add the time unit, I assume it's nanosecond all over
right ?

@@ +158,3 @@
+  self->jitter_avg = 0;
+  self->drift_ampl = 0;
+  self->drift_avg = 0;

Note, this was optional, gobjects allocation is set to zero. (informative
comment)

@@ +183,3 @@
+{
+  GstJitterer *self = GST_JITTERER (object);
+

Take the object lock here maybe ?

@@ +241,3 @@
+
+static gint64
+gst_jitterer_rand_uint64_range (GRand * rand, gint64 min, gint64 max)

For readability, this function could benefit some white lines.

@@ +250,3 @@
+  if (G_LIKELY (dist < G_MAXINT32)) {
+    return g_rand_int_range (rand, 0, dist) + min;
+  } else {

Drop this else here please, it's not needed, previous case has a return
statement.

@@ +251,3 @@
+    return g_rand_int_range (rand, 0, dist) + min;
+  } else {
+    /* This code is based on g_rand_int_range source */

Worth bringing over some copyright ?

@@ +272,3 @@
+      lowrand = g_rand_int (rand);
+      ret = ((((gint64) highrand) << 32) | lowrand);
+    } while (ret > maxvalue);

I'm not fan of this pattern, if unlucky, this can spin and waste a lot of CPU.
Why not scaling down the highrand to the appropriate range ?

@@ +298,3 @@
+{
+  GstJitterer *self = GST_JITTERER (parent);
+

I think object lock would be appropriate, so that we don't get mixed property
values being used.

@@ +306,3 @@
+    max = self->jitter_avg + self->jitter_ampl / 2;
+
+    if (self->change_pts && GST_BUFFER_PTS (inbuf) != GST_CLOCK_TIME_NONE) {

Maybe you could use GST_CLOCK_TIME_IS_VALID() instead of !=
GST_CLOCK_TIME_NONE. This is repeated.

@@ +309,3 @@
+      jitter = gst_jitterer_rand_uint64_range (self->rand, min, max);
+      if (GST_BUFFER_PTS (inbuf) + jitter > 0)
+        GST_BUFFER_PTS (inbuf) = GST_BUFFER_PTS (inbuf) + jitter;

You didn't call gst_buffer_make_writable(), yet you modify the buffer.

@@ +325,3 @@
+  if (self->prev_pts != GST_CLOCK_TIME_NONE) {
+    GstClockTimeDiff drift_avg_per_frame;
+    GstClockTime pts_diff = GST_BUFFER_PTS (inbuf) - self->prev_pts;

You are assuming PTS is monotonic, which may lead to overflow. You should check
the prev_pts is smaller, or use GStClockTimeDiff and the associated DIFF macro.
Then you have to deal with positive and negative values later.

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