[Bug 787358] debugutils: Added new jitterer element

[GStreamer (GNOME Bugzilla)]

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

--- Comment #12 from Vivia Nikolaidou <vivia at ahiru.eu> ---
Thanks for the review! Implemented your review comments except :

(In reply to Nicolas Dufresne (ndufresne) from comment #10)
> 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.

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

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