[Bug 796963] Rework SRT plugin to unify client/server and add features

[GStreamer (GNOME Bugzilla)]

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

--- Comment #7 from Roman Diouskine <rdiouskine at haivision.com> ---
(In reply to Nicolas Dufresne (ndufresne) from comment #6)
> Review of attachment 373338 [details] [review]:
> 
> I must admit it's really hard to figure-out what new and what's old. I feel
> like I'm reviewing code that was already like that before.
> 

Yes, I know what you mean. As I mentioned, I don't feel particularly proud
about the "big patch" but at this point slitting it up into smaller (working)
patches is probably a lot harder than reviewing the code as-is. Thank you for
going with it.

> ::: ext/srt/gstsrt.c
> @@ +34,3 @@
> +#if !GLIB_CHECK_VERSION(2, 54, 0)
> +/* gboolean g_ascii_string_to_signed() and g_ascii_string_to_unsigned()
> + * have been borrowed from glib 2.54 as-is minus the formatting
> 
> You need to copy over the copyright.
> 

OK. I will probably move the whole function to separate .c and add the
appropriate copyright there.

> ::: ext/srt/gstsrtsink.c
> @@ +385,3 @@
> +  priv->loop = g_main_loop_new (priv->context, TRUE);
> +
> +  priv->thread = g_thread_try_new ("srtsink", thread_func, self, &error);
> 
> This thread won't be catched through GST_MESSAGE_STREAM_STATUS / ENTER /
> LEAVE etc. As you do custom threading, you need to implement this yourself.
> 

Hmm, not too sure. The custom thread in question does not, in fact, touch the
stream data at all. What it does is managing the list of remote SRT "clients"
or "destinations" allowing them to dynamically connect/disconnect. The stream
data is then iteratively sent to that list when 'render' callback is invoked.
Do you think that qualifies for STREAM_STATUS logic? Not too sure what that
would do...

> ::: ext/srt/gstsrtsink.h
> @@ +61,3 @@
> +
> +	void (*client_added)      (GstSRTSink *self, int sock, struct sockaddr
> *addr, int addr_len);
> +	void (*client_removed)    (GstSRTSink *self, int sock, struct sockaddr
> *addr, int addr_len);
> 
> Indentation is wrong.

Will fix, thank you.

> 
> ::: ext/srt/gstsrtsrc.c
> @@ +543,3 @@
> +
> +    if (srt_epoll_wait (pollid, &rsock, &(int) {
> +            1}, 0, 0, self->poll_timeout, 0, 0, 0, 0) < 0) {
> 
> Maybe use a variable to hold the structure, it's pretty hard to read.
>
Will fix, thank you.

> @@ +544,3 @@
> +    if (srt_epoll_wait (pollid, &rsock, &(int) {
> +            1}, 0, 0, self->poll_timeout, 0, 0, 0, 0) < 0) {
> +      // TODO: Maybe add a 'yield' here in case epoll starts thrashing?
> 
> g_thread_yield() instead of the TODO ? Seems like less characters.
>

Good point :) will fix, thank you.

> @@ +664,3 @@
> +   * GstSRTSrc:caps:
> +   *
> +   * The Caps used by the source pad.
> 
> Are the caps used or produced.
> 
I would think they are produced... Probably a redundant comment anyway since
there is a description of the property just below where it is spec'ed. I will
remove this and the equally redundant "uri" comment just above.

> @@ +672,3 @@
> +  properties[PROP_POLL_TIMEOUT] =
> +      g_param_spec_int ("poll-timeout", "Poll Timeout",
> +      "Return poll wait after timeout miliseconds (-1 = infinite)",
> 
> Can that be rephrased ?

Yes. To be honest this property is pretty useless since it is unlikely anyone
will want to change this value outside of dev cycle. And -1 value is likely to
break stuff. I think I will just remove the property unless someone (Olivier?)
objects.

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