[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 #6 from Nicolas Dufresne (ndufresne) <nicolas at ndufresne.ca> ---
Review of attachment 373338:
 --> (https://bugzilla.gnome.org/review?bug=796963&attachment=373338)

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.

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

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

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

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

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

@@ +664,3 @@
+   * GstSRTSrc:caps:
+   *
+   * The Caps used by the source pad.

Are the caps used or produced.

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

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