[FFmpeg-devel] [RFC] Separating the RTSP muxer/demuxer and SDP demuxer

[Martin Storsjö]

Hi Josh,

On Thu, 7 Oct 2010, Josh Allmann wrote:

> On 7 October 2010 03:01, Martin Storsj? <martin at martin.st> wrote:
> > Hi,
> >
> > Currently, the RTSP demuxer, muxer and the SDP demuxer are quite tightly
> > coupled, forcing the dependencies of the SDP demuxer to be built even if
> > only the RTSP muxer is desired, and vice versa.
> >
> > A quick recap of what the three muxers/demuxers do and their actual
> > dependencies:
> >
> > - The RTSP muxer speaks with a RTSP server, using common code for sending
> > ?and parsing RTSP requests. On setup, it creates chained RTP muxers for
> > ?each stream, adding a dependency on the RTP muxers.
> >
> > - The RTSP demuxer speaks with a RTSP server, using common code for
> > ?sending and parsing RTSP requests. When the SDP description is received,
> > ?it parses it and creates RTP depacketizers for this, using the rtpdec*
> > ?code.
> >
> > - The SDP demuxer doesn't speak with any RTSP server, but is given a SDP
> > ?file directly. It parses this and sets the connections up just like the
> > ?RTSP demuxer, though, and uses the rtpdec* code.
> >
> > All of the three use the same RTSPState struct as priv_data.
> >

Thanks for having a look at this!

> The biggest problem, IMO, is that the dependencies specify a
> topological ordering (rtp > sdp > rtsp), but the code does not respect
> that. Mostly this is because of common/shared code, but things are
> getting leaky (eg, we do some RTCP handling in the RTSP layer). This
> suggests a larger refactoring may be useful soon.

Yes, the common code calling out to specific code makes it hard to enforce 
the topology.

And while some refactorings may be needed, I'm no fan of "large 
refactorings" as in tearing up lots of code and rewriting it in some other 
way. I do prefer smaller steps of fixing one small thing at a time. This 
split is of course hard to do in that way :-)

> > The attached patch splits the current code from rtsp.c and rtspenc.c into
> > the following files:
> >
> > [...]
> > rtprecv.c ? ?- functions for receiving RTP packets, shared by the RTSP and
> > ? ? ? ? ? ? ? SDP demuxers
> 
> Somehow I am not keen on the naming, since it is not being used by
> RTP. rtspsdpcommon.c is the best I can think of, but it's wordy.

That's a bit wordy yes, I still think I prefer the current naming 
suggestion.

> Also, I think rtsp_fetch_packet could be factored into the RTP layer
> but that is beyond the scope of this patch. That would also cleanly
> separate the SDP common code from this file.

The problem is that everything in the RTP layer currently only works with 
one single RTP stream at a time. The code in rtprecv.c takes care of 
receiving multiple streams at once and reading from the correct once. So 
in that sense, the code doesn't really fit anywhere in the current RTP 
layer. That's the same for the RTCP handling in the RTSP code currently, 
too - it doesn't belong in the code for handling one RTP stream since it 
coordinates multiple of them.

Given that, that's perhaps what should be refactored out, a layer for 
receiving and coordinating multiple RTP streams at once, which is 
currently mixed into the RTSP and SDP demuxers. This is also why I think 
the rtprecv name isn't really that bad.

> > So, what do you think? Is splitting it this way worthwhile, or does it
> > just separate the code too much, making it harder to follow when the
> > control flow jumps around between all of the files?
> >
> 
> I think this split is a necessary, if woolly, step forward.

Is the actual split into separate files also necessary, or just the fixing 
of conditional compilation? I.e., if the same effect is achieved by just 
adding #ifdefs to the current rtsp.c, would that be better or worse than 
splitting it into many small files?

// Martin