Created attachment 373338 [details] [review]
Combined patch for the proposed changes

Hi Roman, thanks for your patch. Can you split this patches in reviewable smaller patches please. This is way to large, hence unlikely to be reviewed. We do one logical change per patch in GStreamer project.

Also, what does glib 2.54 compatibility means ?

Hello Nicolas, thanks for taking a look.

"glib 2.54 compatibility" means a very confusing comment :) Sorry, I will remove that.

I do understand very well that the patch is hard to review due to the number of changes introduced. I don't feel proud about that part at all. Unfortunately, it ended up this way after a fairly long internal project with many changes required to make the plugin usable in a product. Reworking this into a smaller patch series will take much work and is error-prone. I asked Olivier's opinion and he suggested to push the "big patch" and see how that goes first.

If you could kindly take a look at the code and see if there is a way the changes can be used without the splitting I would greatly appreciate that as it would save a lot of time on my side. If the conclusion will be that splitting is absolutely necessary I will do my best to get that accomplished.

Review of attachment 373338 [details] [review]:

::: ext/srt/gstsrt.c
@@ +63,2 @@
+static gboolean
+g_ascii_string_to_signed (const gchar * str, guint base,

Do you want ot add a non g_ prefix? like gst_ascii...

@@ +356,3 @@
+gboolean
+gst_srt_init_params_from_uri (const GstElement * elem,
+    GstSRTParams * params, const GstUri * uri)

I think the URI parsing should be added to libsrt, that way we can ensure that all SRT users have the same interpretation of the parameters. Maybe fill a struct with them or something? and have a function to set them all at once on the srt_socket..

::: ext/srt/gstsrtsrc.c
@@ +238,3 @@
+  priv->n_connect_attempts = 0;
+
+  srt_cleanup ();

What does srt_cleanup() do again? there could be multiple instances of srtsrc in parallel...

@@ +256,3 @@
+
+  if (self->uri != NULL) {
+    // gst_srt_init_params_from_uri() will set element error as needed

Please no // style comments, on /* */ style.

::: ext/srt/gstsrtsrc.h
@@ +57,3 @@
+
+  /*< private >*/
+  gpointer _gst_reserved[GST_PADDING];

No need for reserved, this is all private.

@@ +63,3 @@
+  GstPushSrcClass parent_class;
+
+  gpointer _gst_reserved[GST_PADDING_LARGE];

No need for reserved, this is all private.

Hello Nicolas, thanks for taking a look.

"glib 2.54 compatibility" means a very confusing comment :) Sorry, I will remove that.

I do understand very well that the patch is hard to review due to the number of changes introduced. I don't feel proud about that part at all. Unfortunately, it ended up this way after a fairly long internal project with many changes required to make the plugin usable in a product. Reworking this into a smaller patch series will take much work and is error-prone. I asked Olivier's opinion and he suggested to push the "big patch" and see how that goes first.

If you could kindly take a look at the code and see if there is a way the changes can be used without the splitting I would greatly appreciate that as it would save a lot of time on my side. If the conclusion will be that splitting is absolutely necessary I will do my best to get that accomplished.(In reply to Olivier Crête from comment #4)
> Review of attachment 373338 [details] [review] [review]:
> 
> ::: ext/srt/gstsrt.c
> @@ +63,2 @@
> +static gboolean
> +g_ascii_string_to_signed (const gchar * str, guint base,
> 
> Do you want ot add a non g_ prefix? like gst_ascii...
> 

OK. I will implement an internal gst_srt_ascii_string_to_signed() wrapper.
 
GLib version prior to 2.54 are lacking g_ascii_string_to_signed() function. This code provides 'compatibility' implementation for the environments where GLib version is below 2.54 as is, unfortunately, the case for an embedded device I am currently targeting. Perhaps it is better to move this code in a separate .c file, like glibcompat.c?

> @@ +356,3 @@
> +gboolean
> +gst_srt_init_params_from_uri (const GstElement * elem,
> +    GstSRTParams * params, const GstUri * uri)
> 
> I think the URI parsing should be added to libsrt, that way we can ensure
> that all SRT users have the same interpretation of the parameters. Maybe
> fill a struct with them or something? and have a function to set them all at
> once on the srt_socket..
> 

I agree, I will open a ticket in libsrt project. I was thinking of adding that as srt_utils.h to libsrt so that we don't have to change the main API but still make the functionality available. I think it would probably still make sense the leave the current URI parsing here to make the plugin compatible with the current and previous libsrt releases. I can then add conditional libsrt's URI parsing once that becomes available. The URI parsing itself is required in my case because the element properties (except uri) are not directly accessible through the player implementation and that part is out of my control.

> ::: ext/srt/gstsrtsrc.c
> @@ +238,3 @@
> +  priv->n_connect_attempts = 0;
> +
> +  srt_cleanup ();
> 
> What does srt_cleanup() do again? there could be multiple instances of
> srtsrc in parallel...

srt_startup()/srt_cleanup() pair internally implements instance counting. When srt_cleanup() is called for the last instance and count goes to zero, an internal clean up is performed and worker threads are shut down.
> 
> @@ +256,3 @@
> +
> +  if (self->uri != NULL) {
> +    // gst_srt_init_params_from_uri() will set element error as needed
> 
> Please no // style comments, on /* */ style.
> 

Will fix, thank you.

> ::: ext/srt/gstsrtsrc.h
> @@ +57,3 @@
> +
> +  /*< private >*/
> +  gpointer _gst_reserved[GST_PADDING];
> 
> No need for reserved, this is all private.

Are you certain? There is another private structure GstSRTSrcPrivate that gets added to GstSRTSrc in gstsrtsrc.c, I thought I need to reservce some space for this to work properly.
> 
> @@ +63,3 @@
> +  GstPushSrcClass parent_class;
> +
> +  gpointer _gst_reserved[GST_PADDING_LARGE];
> 
> No need for reserved, this is all private.

Same as above. I don't claim to be a GLib expert so if you think reserved field can go even if I am adding another private structure to this class elsewhere I will remove it.

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.

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

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

Created attachment 373399 [details] [review]
Combined patch for the proposed changes (rev2)

Attached the ticket is the second iteration of the patch with all the noted issues fixed except two for which I need some guidance:

> ::: ext/srt/gstsrtsrc.h
> @@ +57,3 @@
> +
> +  /*< private >*/
> +  gpointer _gst_reserved[GST_PADDING];
> 
> No need for reserved, this is all private.

There is another private structure GstSRTSrcPrivate that gets added to GstSRTSrc in gstsrtsrc.c, I think I need to reserve some space for this to work properly. I could be wrong, please advice.

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

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

Created attachment 373408 [details] [review]
Combined patch for the proposed changes (rev3)

Thanks, Olivier, for clarifications on _gst_reserved question.

Attached the ticket is the third iteration of the patch with all the noted issues fixed except for STREAM_STATUS question on which I need some guidance.

-- 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/770.