Created attachment 359260 [details] [review]
debugutils: Added new jitterer element

This element can be configured to add jitter and/or drift to incoming
buffers' PTS, DTS, or both. Amplitude and average of jitter and drift
are configurable.

How does this differ from the functionality provided by or use case targeted by the netsim element?

Functionality:
This can provide both jitter and drift - so there can be either per-buffer jitter or accumulated drift, or both. Plus, jitterer can be configured to change only PTS or only DTS.

Use case:
The netsim element seems to focus on network-induced problems. In jitterer we are emulating problems caused usually by faulty drivers.

I think netsim would ideally be able to affect the TS (just like this element). Real network jitter will affect the TS since we use do-timestamp as an approximation of the arrival time (we don't have HW timestamps). Though I agree netsim is not really about drift.

Review of attachment 359260 [details] [review]:

I think this element is useful and different enough from netsim. It has a different use case

::: gst/debugutils/gstjitterer.c
@@ +119,3 @@
+          "Average of the drift to apply",
+          -G_MAXINT64 / 2, G_MAXINT64 / 2, 0,
+          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

This is drift per buffer, which seems a bit difficult to control. Better would be something like drift per second, but not sure how to implement that easily just from the buffer timestamps. One could calculate a running average of buffer durations (as calculated from timestamps) probably and base it on that.

@@ +247,3 @@
+    highrand = g_rand_int_range (rand, highmin, highmax);
+    lowrand = g_rand_int (rand);
+    return ((((gint64) highrand) << 32) | lowrand);

This does not allow to get a range between e.g. [2**32, and 2**32 + 5]. Best to take the code from GLib here and adjust the types

I don't mind another element for similar functionality. Jumping straight to the most difficult problem in software engineering: would 'jittersim' be a better name? ;) (I'm undecided myself)

(In reply to Sebastian Dröge (slomo) from comment #5)

> ::: gst/debugutils/gstjitterer.c
> @@ +119,3 @@
> +          "Average of the drift to apply",
> +          -G_MAXINT64 / 2, G_MAXINT64 / 2, 0,
> +          G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> 
> This is drift per buffer, which seems a bit difficult to control. Better
> would be something like drift per second, but not sure how to implement that
> easily just from the buffer timestamps. One could calculate a running
> average of buffer durations (as calculated from timestamps) probably and
> base it on that.

Even easier actually. No drift on first buffer, drift on second buffer is scaled by the timestamp diff between current and previous buffer, drift on third by current and previous again, etc. Just need to remember the last buffer timestamp for this.

Created attachment 362417 [details] [review]
debugutils: Added new jitterer element

This element can be configured to add jitter and/or drift to incoming
buffers' PTS, DTS, or both. Amplitude and average of jitter and drift
are configurable.

Thanks, fixed. Please review again.

Review of attachment 362417 [details] [review]:

::: 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.

Created attachment 371754 [details] [review]
debugutils: Added new jitterer element

This element can be configured to add jitter and/or drift to incoming
buffers' PTS, DTS, or both. Amplitude and average of jitter and drift
are configurable.

Thanks for the review! Implemented your review comments except :

(In reply to Nicolas Dufresne (ndufresne) from comment #10)
> Review of attachment 362417 [details] [review] [review]:
> 
> ::: 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.

Not worth it for this specific simple element, as discussed in person during the hackfest.


> @@ +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 ?

Not anymore after revamping the function, see point below
 
> @@ +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 ?

Refactored this by creating a random integer from 0 to G_MAXUINT64 and scaling+moving it to the appropriate range.
 
> @@ +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.

Oops, forgot this point. Feel free to review the rest of the patch if you want, I'll work on this when we're not getting kicked out of the hackfest room. :)

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/606.