[FFmpeg-devel] [PATCH 6/8] add SRT muxer and demuxer

[Aurelien Jacobs]

On Fri, Jul 23, 2010 at 12:32:37AM +0200, Michael Niedermayer wrote:
> On Thu, Jul 22, 2010 at 10:27:43PM +0200, Aurelien Jacobs wrote:
> > On Thu, Jul 22, 2010 at 02:18:43PM +0200, Michael Niedermayer wrote:
> > > On Wed, Jul 21, 2010 at 11:53:17AM +0200, Aurelien Jacobs wrote:
> > > [...]
> > > > +static int srt_read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > +{
> > > > +    char buffer[2048], *ptr = buffer, *ptr2 = ptr;
> > > > +    int64_t pos = url_ftell(s->pb);
> > > > +    int res = AVERROR_EOF;
> > > > +
> > > > +    while (!is_eol(*ptr2) && !url_feof(s->pb) && ptr-buffer<sizeof(buffer)-1) {
> > > 
> > > that is_eol looks like its reading from uninited mem
> > 
> > Yes... It looks like it.
> > Fixed now.
> > 
> > > > +        ptr2 = ptr;
> > > > +        ptr += ff_get_line(s->pb, ptr, sizeof(buffer)+buffer-ptr);
> > > > +    }
> > > 
> > > besides why is ff_get_line() in a loop, i would naively have expected
> > > a single call to work?
> > 
> > A single call of ff_get_line() will only return a single line of text.
> > A single SRT subtitle event is composed of multiple lines, hence the
> > loop. Events are separated by an empty line.
> 
> ok, didnt know that
> 
> 
> > 
> > Aurel
> >  Changelog                |    1 
> >  doc/general.texi         |    1 
> >  libavcodec/avcodec.h     |    1 
> >  libavformat/Makefile     |    2 
> >  libavformat/allformats.c |    1 
> >  libavformat/avformat.h   |    2 
> >  libavformat/raw.c        |   12 +++++
> >  libavformat/srtdec.c     |  102 +++++++++++++++++++++++++++++++++++++++++++++++
> >  8 files changed, 121 insertions(+), 1 deletion(-)
> > d99824b873904ab864194d48a41b87d301a3c29c  06-add-srt-muxer-and-demuxer.patch
> > add SRT muxer and demuxer
> 
> looks good, assuming it has been tested with /tools/trasher and it
> doesnt crash end in infinite loops

Tested crash free and loop free.
Committed.

> and a reg/fate test would be usefull too if this is already possible

Might be possible to add a demux -> mux test with -scodec copy, but it
wouldn't be that much useful IMHO. I plan to work on decoder/encoder.
When this will be in, reg test will be much more useful/interesting.

Aurel