[FFmpeg-devel] [PATCH] Use the actual RTSP peer IP for RTP sessions

[Martin Storsjö]

Hi Luca,

On Tue, 9 Mar 2010, Luca Abeni wrote:

> Martin Storsj? wrote:
> > On Mon, 8 Mar 2010, Ronald S. Bultje wrote:
> > 
> > > On Mon, Mar 8, 2010 at 11:53 AM, Martin Storsj? <martin at martin.st> wrote:
> > > > On Mon, 8 Mar 2010, Ronald S. Bultje wrote:
> > > > > On Mon, Mar 8, 2010 at 5:08 AM, Martin Storsj? <martin at martin.st>
> > > > > wrote:
> > > > > > The IP address is also present in the SDP, so the
> > > > > > AVFormatContext->filename field must be temporarily changed to
> > > > > > contain an
> > > > > > URL with the numerical IP, but later changed back to the originally
> > > > > > requested url. This feels a bit messy, though.
> > > > > Better not change that, imo. Can you make it a local buffer? Also, we
> > > > > already have RTSPState->filename (or so) as a copy (for control:
> > > > > purposes), maybe you should use that, see the comments in the code for
> > > > > that.
> > > > No, unfortunately, I can't give the url directly to avf_sdp_create, it
> > > > only reads the AVFormatContext->filename field. Unless we improve it, of
> > > > course. :-)
> > > Improve it, indeed.
> 
> Sorry, I missed the beginning of this thread...
> 
> 
> > Ok, here's the first attempt.
> > 
> > If we update avf_sdp_create to take new parameters, what other flexibility
> > may we need? These ones were the ones that I felt needed at the moment. As a
> > bonus, this lets us avoid hardcoding the fact that the control uri in the
> > SDP is exactly "streamid".
> 
> I am all for improving avf_sdp_create(), but:
> 1) I am not convinced that the "url" parameter is a good idea: in my opinion,
>    the AVFormatContexts passed to avf_sdp_create() should be the ones actually
>    used for the RTP streams. So, having a destination address different from
>    ac[]->filename does not make too much sense to me.

In the RTSP muxer case, we don't have the RTP muxers available at that 
point yet. First we create an SDP that we announce, then we do the setup 
of each individual stream, and create each RTP muxer at that point.

>    It seems to me that the right solution in this case would be to create
>    a proper AVFormatContext (or array of AVFormatContexts) before calling
>    avf_sdp_create() (you will have to do it anyway, for outputing the RTP
>    streams... No?)

Yes, it is possible to change things that way, but it gets very messy IMO. 
Currently, either we don't have an AVFormatContext for the RTP muxer at 
all, or we have a fully initialized AVFormatContext, all handled within 
rtsp_rtp_mux_open. Instead, we now may have an AVFormatContext that is 
allocated but may or may not have av_write_header called (which changes 
what should be done in the cleanup procedure).

Attached patch series that shows what this would look like - messy IMO.

> So, I think you and Ronald will have to convince me about the need for these
> changes in the avf_sdp_create() interface (for point 2, I think I can be
> convinced... But for point 1 you will have to show me a case where the
> destination address for the SDP really needs to be different from the RTP
> destination :).

The SDP destination shouldn't be different from the RTP destination, but 
the issue is that it gets complicated to construct the SDP from the RTP 
muxers and we can accomplish 99% of it by simply constructing the SDP from 
the original RTSP muxer and the AVStream the user has set up.

The only thing missing is that we need to specify a slightly modified URL, 
with the numerical peer IP instead of the original host name that the user 
specified.

> (as a side note, I am still not convinced about the fact that using a
> non-numeric destination address for RTP is a good idea)

For raw RTP, I guess a non-numerical destination could make sense in some 
scenarios, but in this scenario, the internal URLs within the RTSP muxer 
should of course use the numerical address of the peer we're communicating 
with - that's the whole point of this thread.

The problem simply is that the user-supplied RTSP URL most definitely 
should be allowed to use a non-numerical host name.

// Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Initialize-the-RTSPStreams-before-doing-the-RTSP-ANN.patch
Type: text/x-diff
Size: 2160 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100309/abb9d994/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Initialize-the-output-AVFormatContext-in-rtsp_setup_.patch
Type: text/x-diff
Size: 5727 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100309/abb9d994/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Use-the-RTP-AVFormatContexts-for-creating-the-SDP.patch
Type: text/x-diff
Size: 2993 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100309/abb9d994/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Resolve-and-use-the-actual-IP-address-of-the-peer-we.patch
Type: text/x-diff
Size: 1678 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100309/abb9d994/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Use-the-peer-IP-address-in-the-RTP-URLs.patch
Type: text/x-diff
Size: 1491 bytes
Desc: 
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100309/abb9d994/attachment-0004.patch>