Created attachment 370277 [details] [review]
rtspclientsink: Fix client ports for the RTCP backchannel

Comment on attachment 370277 [details] [review]
rtspclientsink: Fix client ports for the RTCP backchannel

Please add a FIXME comment explaining the situation before merging

Can you also put a testcase in this bug report to reproduce it, or at least the RTSP messages that go back and forth and are missing the ports?

Created attachment 370346 [details] [review]
rtspclientsink: Fix client ports for the RTCP backchannel

This was broken since the work for delayed transport creation
was merged: the creation of the transports string depends on
calling stream_get_server_port, which only starts returning
something meaningful after a call to stream_allocate_udp_sockets
has been made, this function expects a transport that we parse
from the transport string ...

Significant refactoring is in order, but does not look entirely
trivial, for now we put a band aid on and create a second transport
string after the stream has been completed, to pass it in
the request headers instead of the previous, incomplete one.

Review of attachment 370346 [details] [review]:

::: gst/rtsp-server/rtsp-stream.c
@@ +1511,3 @@
   priv = stream->priv;
 
+  GST_ERROR_OBJECT (stream, "Allocating sockets");

This is not an error

@@ +3747,2 @@
       if (add) {
+        GST_ERROR_OBJECT (stream, "adding %s:%d-%d", dest, min, max);

and neither is this :)

Attachment 370346 [details] pushed as 7f9b8c2 - rtspclientsink: Fix client ports for the RTCP backchannel

backported to 1.14

Why is there a dependency between client ports and server ports? It seems very strange. What if the "server ports" are already allocated on the client side?

(In reply to Patricia Muscalu from comment #8)
> Why is there a dependency between client ports and server ports? It seems
> very strange.

That's because the stream API itself is very strange, gst_rtsp_stream_get_server_port is a bad and confusing name.

> What if the "server ports" are already allocated on the client
> side?

I don't understand that question.

(In reply to Mathieu Duponchelle from comment #9)
> (In reply to Patricia Muscalu from comment #8)
> > Why is there a dependency between client ports and server ports? It seems
> > very strange.
> 
> That's because the stream API itself is very strange,
> gst_rtsp_stream_get_server_port is a bad and confusing name.

There is no need to call this function at all I think. It's just about allocating client ports for receiving RTP and RTCP, so it shouldn't be any dependencies to the server ports.

> > What if the "server ports" are already allocated on the client
> > side?
> 
> I don't understand that question.

The RTP/RTCP sockets are bound to the RTP/RTCP ports (<=> server ports). The client issues a SETUP request with the client_port pair of ports, so I guess that there are the corresponding sockets associated with these ports on the client side. Am I wrong?

(In reply to Patricia Muscalu from comment #10)
> There is no need to call this function at all I think. It's just about
> allocating client ports for receiving RTP and RTCP, so it shouldn't be any
> dependencies to the server ports.
> 

You realize that RTCP goes both ways don't you? In the RECORD case, rtspclientsink will send RTP and RTCP (eg Sender Reports) to the server, but it also needs to inform the server, through "client_ports" in the transports string, about the port it will listen on for RTCP, to receive the Receiver Reports and potential retransmission requests. It obtains these ports through the confusingly named gst_rtsp_stream_get_server_port () API.

> > > What if the "server ports" are already allocated on the client
> > > side?
> > 
> > I don't understand that question.
> 
> The RTP/RTCP sockets are bound to the RTP/RTCP ports (<=> server ports). The
> client issues a SETUP request with the client_port pair of ports, so I guess
> that there are the corresponding sockets associated with these ports on the
> client side. Am I wrong?

See my answer above :)