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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110330/0f00a8e2/attachment.asc>