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

Martin Storsjö martin
Thu Oct 7 23:13:21 CEST 2010


On Thu, 7 Oct 2010, Martin Storsj? wrote:

> On Thu, 7 Oct 2010, Aurelien Jacobs wrote:
> 
> > On Thu, Oct 07, 2010 at 01:01:16PM +0300, Martin Storsj? 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:
> > > 
> > > [...]
> > > 
> > > I tried doing the split, and the attached diff isn't something that could 
> > > be used as such, but it's an initial proof of concept.
> > > 
> > > Since the common code paths calls out to the use case specific code, 
> > > splitting of the code is a bit trickier and needs function prototypes for 
> > > the use case specific parts, and needs to enclose the callers within small 
> > > #ifdefs within the functions, like this for example:
> > > 
> > > -    if (s->iformat)
> > > +    if (s->iformat) {
> > > +#if CONFIG_RTSP_DEMUXER
> > >          err = rtsp_setup_input_streams(s, reply);
> > > -    else
> > > +#endif
> > > +    } else {
> > > +#if CONFIG_RTSP_MUXER
> > >          err = rtsp_setup_output_streams(s, host);
> > > +#endif
> > > +    }
> > 
> > Would be cleaner like this:
> > 
> >     if (CONFIG_RTSP_DEMUXER && s->iformat)
> >         err = rtsp_setup_input_streams(s, reply);
> >     else if (CONFIG_RTSP_MUXER)
> >         err = rtsp_setup_output_streams(s, host);
> 
> Ah, good point, we rely on that on many other places, too. I'll change it 
> to use that.
> 
> > > This is in the common RTSP connection code, while the setup_*_streams 
> > > functions are in either the demuxer or in the muxer. We already have some 
> > > similar code in rtsp_fetch_packet, like this one:
> > > 
> > >     switch(rt->lower_transport) {
> > >     default:
> > > #if CONFIG_RTSP_DEMUXER
> > >     case RTSP_LOWER_TRANSPORT_TCP:
> > >         len = tcp_read_packet(s, &rtsp_st, rt->recvbuf, RECVBUF_SIZE);
> > >         break;
> > > #endif
> > >     case RTSP_LOWER_TRANSPORT_UDP:
> > > 
> > > The attached patch splits the current code from rtsp.c and rtspenc.c into
> > > the following files:
> > > 
> > > rtsp.c       - handling of RTSP protocol things, shared by the RTSP muxer 
> > >                and demuxer
> > > rtspenc.c    - RTSP muxer only
> > > rtspcommon.c - common code shared by all three components, such as opening 
> > >                of the transport contexts, cleanup of transport contexts, 
> > >                common parsing helpers such as get_word(), get_word_sep(), 
> > >                get_word_until_chars()
> > > rtprecv.c    - functions for receiving RTP packets, shared by the RTSP and 
> > >                SDP demuxers
> > > rtspdec.c    - RTSP demuxer only
> > > sdpdec.c     - SDP demuxer only
> > 
> > This looks like a good plan. I like it.
> > 
> > > Then there's of course the question of how to name all the files.
> > 
> > I have no strong feeling about the naming. Your proposition is not bad.
> > Maybe you should find a better name for rtspcommon. Maybe a name which
> > don't contain "rtsp" ?
> 
> I changed them locally to rtsp.c (containing stuff handling the RTSPState 
> struct which all of them use) and rtspproto.c (which is all the code that 
> talks to a RTSP server).

Ok, I've got an updated patch now. I wasn't able to send it to the 
list, since it was slightly too large, but as Luca B suggested, I've 
pushed it to github: http://github.com/mstorsjo/ffmpeg/tree/rtsp-split

Now I think this actually could be committed as such.

I added ff_ or ff_rtsp_ prefixes to all functions I've made non-static 
(and reindented their parameter lists), and added the needed CONFIG_* to 
if statements, to avoid having to link to unused code (relying on dead 
code eliminatin). Except for that, the code is exactly what we have now, 
lines are not reordered compared to how they're in rtsp.c right now, and I 
do no other cosmetic changes to the code.

Ok to go ahead with this split?

// Martin



More information about the ffmpeg-devel mailing list