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

[Aurelien Jacobs]

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 ?

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-add-MicroDVD-muxer-and-demuxer.patch
Type: text/x-diff
Size: 11056 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110329/44b6ba5f/attachment.bin>