[FFmpeg-devel] [PATCH] Provide a context to the ffmpeg -r option

[Stefano Sabatini]

On date Monday 2008-08-11 15:29:39 +0200, Michael Niedermayer encoded:
> On Mon, Aug 11, 2008 at 11:16:46AM +0200, Stefano Sabatini wrote:
> > On date Friday 2008-08-08 22:42:19 +0200, Stefano Sabatini encoded:
> > > Hi,
> > > as in subject, consistent with most options and clearer IMO.
> > > 
> > > Regards.
> > > -- 
> > > FFmpeg = Frightening & Forgiving MultiPurpose Erudite Goblin
> > 
> > > Index: ffmpeg.c
> > > ===================================================================
> > > --- ffmpeg.c	(revision 14670)
> > > +++ ffmpeg.c	(working copy)
> > > @@ -2309,12 +2309,13 @@
> > >      return 0;
> > >  }
> > >  
> > > -static void opt_frame_rate(const char *arg)
> > > +static int opt_frame_rate(const char *opt, const char *arg)
> > >  {
> > >      if (av_parse_video_frame_rate(&frame_rate, arg) < 0) {
> > > -        fprintf(stderr, "Incorrect frame rate\n");
> > > +        fprintf(stderr, "Incorrect value for %s: %s\n", opt, arg);
> > >          av_exit(1);
> > >      }
> > > +    return 0;
> > >  }
> > >  
> > >  static int opt_bitrate(const char *opt, const char *arg)
> > > @@ -3581,7 +3582,7 @@
> > >          opt_format("vcd");
> > >  
> > >          opt_frame_size(norm ? "352x240" : "352x288");
> > > -        opt_frame_rate(frame_rates[norm]);
> > > +        opt_frame_rate(NULL, frame_rates[norm]);
> > >          opt_default("gop", norm ? "18" : "15");
> > >  
> > >          opt_default("b", "1150000");
> > > @@ -3609,7 +3610,7 @@
> > >          opt_format("svcd");
> > >  
> > >          opt_frame_size(norm ? "480x480" : "480x576");
> > > -        opt_frame_rate(frame_rates[norm]);
> > > +        opt_frame_rate(NULL, frame_rates[norm]);
> > >          opt_default("gop", norm ? "18" : "15");
> > >  
> > >          opt_default("b", "2040000");
> > > @@ -3631,7 +3632,7 @@
> > >          opt_format("dvd");
> > >  
> > >          opt_frame_size(norm ? "720x480" : "720x576");
> > > -        opt_frame_rate(frame_rates[norm]);
> > > +        opt_frame_rate(NULL, frame_rates[norm]);
> > >          opt_default("gop", norm ? "18" : "15");
> > >  
> > >          opt_default("b", "6000000");
> > > @@ -3652,7 +3653,7 @@
> > >          opt_frame_size(norm ? "720x480" : "720x576");
> > >          opt_frame_pix_fmt(!strncmp(arg, "dv50", 4) ? "yuv422p" :
> > >                                               (norm ? "yuv411p" : "yuv420p"));
> > > -        opt_frame_rate(frame_rates[norm]);
> > > +        opt_frame_rate(NULL, frame_rates[norm]);
> > >  
> > >          audio_sample_rate = 48000;
> > >          audio_channels = 2;
> > > @@ -3798,7 +3799,7 @@
> > >      { "b", OPT_FUNC2 | HAS_ARG | OPT_VIDEO, {(void*)opt_bitrate}, "set bitrate (in bits/s)", "bitrate" },
> > >      { "vb", OPT_FUNC2 | HAS_ARG | OPT_VIDEO, {(void*)opt_bitrate}, "set bitrate (in bits/s)", "bitrate" },
> > >      { "vframes", OPT_INT | HAS_ARG | OPT_VIDEO, {(void*)&max_frames[CODEC_TYPE_VIDEO]}, "set the number of video frames to record", "number" },
> > > -    { "r", HAS_ARG | OPT_VIDEO, {(void*)opt_frame_rate}, "set frame rate (Hz value, fraction or abbreviation)", "rate" },
> > > +    { "r", OPT_FUNC2 | HAS_ARG | OPT_VIDEO, {(void*)opt_frame_rate}, "set frame rate (Hz value, fraction or abbreviation)", "rate" },
> > >      { "s", HAS_ARG | OPT_VIDEO, {(void*)opt_frame_size}, "set frame size (WxH or abbreviation)", "size" },
> > >      { "aspect", HAS_ARG | OPT_VIDEO, {(void*)opt_frame_aspect_ratio}, "set aspect ratio (4:3, 16:9 or 1.3333, 1.7777)", "aspect" },
> > >      { "pix_fmt", HAS_ARG | OPT_EXPERT | OPT_VIDEO, {(void*)opt_frame_pix_fmt}, "set pixel format, 'list' as argument shows all the pixel formats supported", "format" },
> > 
> > If there are no objections I'm going to apply it on Wednesday, regards.
> 
> I object, not strongly but i think this is not good for anything.
> Like if it aint broken dont fix it ...

Improves consistency with most other options behaviour (least surprise
rule), and provides more information to the user (the faulty wrong
value):

"Incorrect value for r: foo"
I think it's better than:
"Incorrect frame rate"

It's a *slight* improvement I admit, nonetheless an improvement.

Regards.
-- 
FFmpeg = Fostering and Frightening Mega Patchable Extravagant God