srt: Refactor plugin to support non-blocking SRT APIs

Submitted by Seungha Yang

Link to original bug (#796842)

Description

Add support "srt://localhost:port" style uri, and change the
default host to "localhost"

Seungha Yang submitted a patch:

Patch 373098, "srt: Remove white space":
0001-srt-Remove-white-space.patch

Seungha Yang submitted a patch:

Providing simplified _client_connect() is not required for now.

Patch 373099, "[2/4] srt: Reduce useless call depth":
0002-srt-Reduce-useless-call-depth.patch

Seungha Yang submitted a patch:

Patch 373100, "[3/4] srt: Remove duplicated code for setting server socket":
0003-srt-Remove-duplicated-code-for-setting-server-socket.patch

Seungha Yang submitted a patch:

Patch 373101, "[4/4] srt: Allow the host name "localhost"":
0004-srt-Allow-the-host-name-localhost.patch

Olivier Crête @ocrete reviewed patch 373098:

this one looks good

Olivier Crête @ocrete reviewed patch 373099:

looks good too

Olivier Crête @ocrete reviewed patch 373101:

The problem is that name resolution is a blocking operation here.. and gst_*src_start() shouldn't be blocking. That would mean moving the thing to the async api .. with gst_base_src_set_async() and then create a thread if the name resolution is blocking and then call gst_base_src_start_complete() when it's done.

I guess for the sink it's already in the streaming thread, so it doesn't matter as much.

Also, we awnt to merge clientsink/serversink into a srtsink and the same at the source, there is a branch on github that does vaguely that.

Olivier Crête @ocrete reviewed patch 373100:

::: ext/srt/gstsrt.c  
@@ +215,3 @@  
+  /* This is a sink, we're always a sender */  
+  srt_setsockopt (sock, 0, SRTO_SENDER, &(int) {  
+      1}, sizeof (int));  

But you call this form the source also.

Olivier Crête @ocrete reviewed patch 373100:

::: ext/srt/gstsrt.c  
@@ +215,3 @@  
+  /* This is a sink, we're always a sender */  
+  srt_setsockopt (sock, 0, SRTO_SENDER, &(int) {  
+      1}, sizeof (int));  

But you call this form the source also.

Seungha Yang said:

(In reply to Olivier Crête from comment 7)

Review of attachment 373101 [details] [review]:

The problem is that name resolution is a blocking operation here.. and
gst_*src_start() shouldn't be blocking. That would mean moving the thing to
the async api .. with gst_base_src_set_async() and then create a thread if
the name resolution is blocking and then call gst_base_src_start_complete()
when it's done.

I guess for the sink it's already in the streaming thread, so it doesn't
matter as much.

I guess the point which you are worrying about is that source's start()/stop() is most likely app thread and (i.e., triggering set_state()) blocking the app thread might cause problem, right?
If my understanding is correct, I think "blocking" could be ugly and agree with your point, but I'm wondering whether it's really critical or not, because async source element also looks tricky. The problem is,

• I couldn't find any async source element for now, and even some elements which are using name resolving (such as udp/tcp source) are not async. So, there seems no good example for me. And when I saw basesrc's impl., finding proper room for calling gst_base_src_start_complete() seems to be a little tricky, because we should meet following two conditions (if it's wrong, please correct me)

1. gst_base_src_start_complete() shouldn't be called in gst_base_src_get_range() thread (loop thread) since both are taking stream_lock.
2. gst_base_src_start_complete() should be called before gst_base_src_get_range() in order for create()/fill() methods not to return null buffer.

Olivier Crête @ocrete said:

(In reply to Seungha Yang from comment 10)

• I couldn't find any async source element for now, and even some elements
  which are using name resolving (such as udp/tcp source) are not async. So,
  there seems no good example for me. And when I saw basesrc's impl., finding
  proper room for calling gst_base_src_start_complete() seems to be a little
  tricky, because we should meet following two conditions (if it's wrong,
  please correct me)

1. gst_base_src_start_complete() shouldn't be called in
   gst_base_src_get_range() thread (loop thread) since both are taking
   stream_lock.
2. gst_base_src_start_complete() should be called before
   gst_base_src_get_range() in order for create()/fill() methods not to return
   null buffer.

In theory yes, but I can't find any other element that uses it, so maybe it's not useable as-is.. Lets ignore this for now.

Seungha Yang said:

(In reply to Olivier Crête from comment 11)

In theory yes, but I can't find any other element that uses it, so maybe
it's not useable as-is.. Lets ignore this for now.

Ok :). I'm doing refacto srtbasesrc and it's subclasses to make them actually cancellable with unit test code. I'll update them at once in this ticket soon.
(possibly reworked srtbasesink also updated)

Seungha Yang submitted a patch:

Patch 373197, "srtclient: Fix bind-port and rendez-vous property getter":
0001-srtclient-Fix-bind-port-and-rendez-vous-property-get.patch

Seungha Yang submitted a patch:

Patch 373198, "[3/17] srtclient: Fix bind-port and rendez-vous property getter":
0001-srtclient-Fix-bind-port-and-rendez-vous-property-get.patch

Seungha Yang submitted a patch:

Patch 373199, "[4/17] srtclientsink: Do not leak string":
0002-srtclientsink-Do-not-leak-string.patch

Seungha Yang submitted a patch:

... instead of client-added

Patch 373200, "[5/17] srtserversrc: Emit client-closed signal in _stop()":
0003-srtserversrc-Emit-client-closed-signal-in-_stop.patch

Seungha Yang submitted a patch:

client-added and client-{removed,closed} signals were defined
to be emitted with two arguments (socket fd and address)

Patch 373201, "[6/17] srt: Fix signal args mismatch":
0004-srt-Fix-signal-args-mismatch.patch

Seungha Yang submitted a patch:

Patch 373202, "[7/17] srt: Remove duplicated code for setting server socket":
0005-srt-Remove-duplicated-code-for-setting-server-socket.patch

Seungha Yang submitted a patch:

Given "passphrase" shouldn't be modified

Patch 373203, "[8/17] srt: Add "const" keyword to a function argument":
0006-srt-Add-const-keyword-to-a-function-argument.patch

Seungha Yang submitted a patch:

... instead of doing it ourselves. Otherwise, we should add more
logic here (such as checking GstClock and etc) which was already provided by
GstBaseSrc.

Patch 373204, "[9/17] srtbasesrc: Use GstBaseSrc's timestamp impl.":
0007-srtbasesrc-Use-GstBaseSrc-s-timestamp-impl.patch