Created attachment 276128 [details] [review]
Suggestion for fix

What exactly does this fix?

You override ct->client_port.min, which:

 - is not used to make the server transport in TCP mode
 - will send the wrong port back to the client in UDP mode

Okay, not exactly the intention :).
I wanted to set the port for tcp connections so it would be readable later (without needing to extract it from the socket). This would be used for identifying active streams for statistics collection.

I guess I should put it after the TCP check?

if (ct->lower_transport & GST_RTSP_LOWER_TRANS_TCP) {
...

Setting back to unconfirmed.

You want the port number that the client used to connect to the server? When do you need this information?

I use the info for identifying a stream by its socketaddress (source & dest ip & port) outside gstreamer. It can be used for collecting uncooked stats via for example libpcap. I use it already for the udp case and it would be very convenient to be able to use it for tcp aswell. Would it be possible to add this functionality?

It's possible to add but I think it's already available in the client GstRTSPConnection object. That's why I ask how and when you get this information.

For UDP you get this information from GstRTSPTransport object via RTSPSessionMedia->RTSPStreamTransport->RTSPTransport. You do this? Do you want to get the remote and local ports in RTSPTransport as well for TCP?

For TCP you can iterate the RTSPClient objects and get the RTSPConnection from it and look at the ports.

Yes that is correct, that is how I fetch it in the UDP case.

Not sure how you mean by iterating the client?
I use the "play-request" signal on the client to get a callback from where I iterate the streams in the media. 
Perhaps a dumb question, but are all connections same for all streams within a client?
I can get the Connection from the client with gst_rtsp_client_get_connection, and then use gst_rtsp_connection_get_write_socket to get the writing socket, and then extract the sockaddr from the sockets fd? Seems a bit more complicated than for the UDP case :), or have I missed something?
Wouldnt it be more consistent to set it so It can be readable for TCP as we do for UDP?

(In reply to comment #8)
> Perhaps a dumb question, but are all connections same for all streams within a
> client?

The Client object is the TCP connection that a client has with a server, if the RTSP data is send with TCP, it is also sent over this connection.

> I can get the Connection from the client with gst_rtsp_client_get_connection,
> and then use gst_rtsp_connection_get_write_socket to get the writing socket,
> and then extract the sockaddr from the sockets fd? Seems a bit more complicated
> than for the UDP case :), or have I missed something?

No, it's not as easy.

> Wouldnt it be more consistent to set it so It can be readable for TCP as we do
> for UDP?

It would be good but it seems a little weird to abuse the min/max values of the Transport for that. I guess if we document this properly, It would be acceptable. Let me take a closer look.

Ok great, let me know what you come up with!

There could be 4 port:

 - port used by client to send RTP/RTCP
 - port used by client to receive RTP/RTCP
 - port used by server to send RTP/RTCP
 - port used by server to receive RTP/RTCP

Sender and receiver ports can be different when tunnelling over HTTP. In practice we always send RTP and RTCP over the same TCP connection, there is nothing defined in RTSP to split that up.

We could use:
 
 client_port.min and client_port.max for the client sender/receiver ports respectively and
 server_port.min and server_port.max for the server sender/receiver ports respectively.

Yes, that sounds good!

commit 5aa06b8058423e7ecd1f656d6d420cab6d581d1e
Author: Wim Taymans <wtaymans@redhat.com>
Date:   Tue May 20 15:57:30 2014 +0200

    client: store TCP ports in transport
    
    Store the TCP ports in the transport when we are doing RTSP over TCP.
    This way, we can easily get to the ports from the transport.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=729776