[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 #5 from Roman Diouskine <rdiouskine at haivision.com> ---
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]:
> 
> ::: 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.

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