[FFmpeg-devel] [PATCH] add MicroDVD muxer and demuxer

[Aurelien Jacobs]

On Wed, Mar 30, 2011 at 12:03:08AM +0200, Michael Niedermayer wrote:
> On Tue, Mar 29, 2011 at 11:37:51PM +0200, Aurelien Jacobs wrote:
> > On Tue, Mar 29, 2011 at 09:33:12AM +0200, Tomas Härdin wrote:
> > > Aurelien Jacobs skrev 2011-03-29 00:42:
> > > >--- /dev/null
> > > >+++ b/libavformat/microdvddec.c
> > > >+static int microdvd_probe(AVProbeData *p)
> > > >+{
> > > >+    unsigned char c, *ptr = p->buf;
> > > >+    int i;
> > > >+
> > > >+    if (AV_RB24(ptr) == 0xEFBBBF)
> > > >+        ptr += 3;  /* skip UTF-8 BOM */
> > > >+
> > > >+    for (i=0; i<3; i++) {
> > > >+        if (sscanf(ptr, "{%*d}{}%c",&c) != 1&&
> > > >+            sscanf(ptr, "{%*d}{%*d}%c",&c) != 1&&
> > > >+            sscanf(ptr, "{DEFAULT}{}%c",&c) != 1)
> > > 
> > > Using sscanf() looks unsafe - IIRC the probe buffer isn't null
> > > terminated.
> > 
> > The probe buffer is properly null terminated (look at AVPROBE_PADDING_SIZE).
> > 
> > > Also nit: space before &&
> > 
> > The space (and proper alignment) is present in the patch I sent.
> > Maybe your mail client doesn't display it correctly ??
> > 
> > > >+    }
> > > >+    return AVPROBE_SCORE_MAX;
> > > >+}
> > > >+
> > > >+static int microdvd_read_header(AVFormatContext *s, AVFormatParameters *ap)
> > > >+{
> > > >+    AVRational pts_info = (AVRational){ 2997, 125 };  /* default: 23.976 fps */
> > > >+    MicroDVDContext *microdvd = s->priv_data;
> > > >+    AVStream *st = av_new_stream(s, 0);
> > > >+    int i, frame;
> > > >+    float fps;
> > > >+    char c;
> > > >+
> > > >+    if (!st)
> > > >+        return -1;
> > > >+    for (i=0; i<FF_ARRAY_ELEMS(microdvd->lines); i++) {
> > > >+        microdvd->pos[i] = avio_tell(s->pb);
> > > >+        ff_get_line(s->pb, microdvd->lines[i], sizeof(microdvd->lines[i]));
> > > >+        if ((sscanf(microdvd->lines[i], "{%d}{}%5f",&frame,&fps) == 2 ||
> > > >+             sscanf(microdvd->lines[i], "{%d}{%*d}%5f",&frame,&fps) == 2)
> > > >+&&  frame<= 1&&  fps>  3&&  fps<  100)
> > > 
> > > Uhm, really odd indent..
> > 
> > ...
> > 
> > > Also, why not frame > 1 or slightly strange frame rates?
> > 
> > AFAIK, there is no specification about how to store FPS in MicroDVD
> > files. It seems it became a common practice to store it at the begining
> > of the file as a normal subtitle line restricted to frame 1. It looks
> > like this:
> > 
> > {1}{1}23.976
> > 
> > So I made the parsing a bit strict to avoid miss-interpretting something
> > else as a FPS value.
> > In practice it works fine.
> > 
> > > >+            pts_info = av_d2q(fps, 100000);
> > > 
> > > Does this always do "the right thing", considering the limited
> > > precision of float? Maybe I'm nitpicking, but parsing %d.%d seems
> > > more stable.
> > 
> > This does the right thing for the various values I encounter in various
> > sample files, even using float. eg:
> > 
> >   MicroDVD    =>   pts_info
> > {1}{1}23.976  =>  2997 / 125
> > {1}{1}25.000  =>  25 / 1
> > {1}{1}29.970  =>  2997 / 100
> > 
> > But for extra security, I've just switched it to double.
> > Here it is, attached this time, so hopefully formatting will be OK for
> > you.
> > Any other comment ?
> 
> only "LGTM" from me

Pushed.

Aurel