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

[Martin Storsjö]

On Thu, 7 Oct 2010, Diego Biurrun wrote:

> On Thu, Oct 07, 2010 at 01:01:16PM +0300, Martin Storsj? wrote:
> > 
> > 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
> > 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()
> 
> > Then there's of course the question of how to name all the files. Perhaps 
> > it would be better to rename rtsp.c to rtspproto.c, and rtspcommon.c to 
> > rtsp.c.
> 
> Yes.
> 
> > --- a/libavformat/Makefile
> > +++ b/libavformat/Makefile
> > @@ -7,6 +7,7 @@ HEADERS = avformat.h avio.h
> >  
> >  OBJS = allformats.o         \
> >         cutils.o             \
> > +       id3v2.o              \
> >         metadata.o           \
> >         metadata_compat.o    \
> >         options.o            \
> 
> This looks unrelated.

This was to fix build errors due to missing dependencies from rev 25378, 
but is unrelated to the rest, yes.

> > --- /dev/null
> > +++ b/libavformat/rtprecv.c
> > @@ -0,0 +1,564 @@
> > +
> > +#include "libavutil/avstring.h"
> > +#include "avformat.h"
> > +
> > +#include <sys/time.h>
> > +#if HAVE_SYS_SELECT_H
> > +#include <sys/select.h>
> > +#endif
> > +#include <strings.h>
> > +#include "internal.h"
> > +#include "network.h"
> > +#include "os_support.h"
> > +#include "rtsp.h"
> 
> Place system headers before local ones and this is missing config.h.

This file, and all others, are actually just copies of the current rtsp.c 
with most lines removed, so I've not reordered them (yet). This also helps 
diffing to make sure I haven't done any other unrelated changes.

> > --- /dev/null
> > +++ b/libavformat/rtspcommon.c
> > @@ -0,0 +1,198 @@
> > +
> > +#include "libavutil/avstring.h"
> > +#include "avformat.h"
> > +
> > +#include <sys/time.h>
> > +#include "internal.h"
> > +#include "network.h"
> > +#include "os_support.h"
> > +#include "rtsp.h"
> 
> again
> 
> > +void get_word_sep(char *buf, int buf_size, const char *sep,
> > +                         const char **pp)
> 
> indentation
> 
> > +{
> > +    if (**pp == '/') (*pp)++;
> 
> Please break the line.

Again, this is just a patch that moves code around without actually 
touching it, I'd avoid doing other reformatting at the same time.

> > --- /dev/null
> > +++ b/libavformat/rtspdec.c
> > @@ -0,0 +1,361 @@
> > +int tcp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
> > +                           uint8_t *buf, int buf_size)
> 
> indentation

This, and the other case, was me just removing the preceding static when 
splitting, with touching as little as possible of the rest. This needs a 
ff prefix, too.

I guess the main question is to Ronald and Luca B, whether they see this 
kind of split as something they'd want me to continue on, or if they 
prefer it as it is now.

// Martin