[FFmpeg-devel] [PATCH] Add an RTSP muxer

[Martin Storsjö]

Hi Ronald,

On Mon, 4 Jan 2010, Ronald S. Bultje wrote:

> 0001 - inet_ntop()
> I'd prefer to drop all usage of it and use getaddrinfo() with the
> proper flags instead.

That'd be getnameinfo, not getaddrinfo, but that would be equally good for 
this case.

A good getnameinfo wrapper (which is needed in the same way as for 
getaddrinfo) would use inet_ntop internall, though, but since it probably 
just should handle IPv4 in that case, I guess it wouldn't be needed.

> 0002 - resolve address
> Adds usage of a deprecated function, I'd say I'm against this.
> 
> Yes, that sucks, we need to convert rtsp.c to IPv6 (and its API), and
> badly so. But we need to do it some time.

Well, it isn't deprecated yet - those patches aren't in SVN yet. The 
instant the getaddrinfo patches hit SVN, I'll convert this to use that API 
instead.

> 0003-5 is for Luca (A).
> 0006-0007 appear to touch upon a deeper issue, I'd just add the
> AVFormatContext (that's not ic, right?) to the struct instead. This is
> a bit ugly. You're removing ic in 0014, which I think is a bad idea.
> I'd drop these two and convert ic into being the AVFormatContext that
> you take pb from and use as a log context. (That's how the RTP/RTSP
> demuxer parts work.)

That unfortunately doesn't really work like that (straight away in my 
current design, at least) for the rtp muxers. In the RTSP/RTP demuxers, 
the RTSP demuxer owns URLContexts where it reads packets, and if packets 
are received, they are fed to RTPDemuxContexts for parsing. The 
RTPDemuxContexts thus never actually touch the actual RTP streams that are 
owned by the RTSP demuxer. (Or did I understand this incorrectly?)

The RTP muxers on the other hand send out the actual rtp packets deep 
inside the packetization routines. So they're not exactly symmetrical in 
their design... Calling the RTP muxers with the RTSP AVFormatContext won't 
work.

As Luca pointed out, perhaps I should open fullblown AVFormatContexes for 
each of the output RTP muxers instead. That would make most (all?) of the 
rtpenc patches unnecessary, and would even remove the need for some of the 
sdp patches.

> 0008-0009, this is the AVFC that I'm talking about one line above, if
> you add the whole AVFC then they aren't needed.
> 
> 0010 - yes

Ok, so this one could basically be applied (preceded by 0005)?

> 0011 - recipee for disaster if you ask me... (See 0006-0009/0014 comment.)
> 
> 0017 - hmm? I wonder if some flag in AVFormatContext doesn't already
> specify this...

Doesn't seem so...

> 0018/0019 - bad idea, I don't want rtsp.c to become a "if muxer ...
> else ...", if this is it then the common code should be split and left
> in rtsp.c and the rest in rtspdec/enc.c.
> 
> I'm wondering if you can't use the transport property for this.
> RDT_IN, RTP_IN or RTP_OUT or so. Then patch 0019 makes more sense. I
> left the rest of the patches because they're doomed to get similar
> resonses from me. My basic question: is this the right way to add a
> RTSP muxer (i.e. using common code and then do if (output) { do one
> thing } else { ... })? I'd say no.

For the common setup/connection routine, I guess it could be separated 
into two similar-looking functions, splitting out larger chunks of shared 
code into common functions.

For the mode= parameter in make_setup_request, I think output/input is the 
correct distinction, but perhaps the mode could be given as a parameter to 
make_setup_request instead?

That leaves the transport distinction. I'm not totally convinced that 
adding in/out versions of the different transport mechanisms, but I'll 
look into it.

// Martin