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

[Stefano Sabatini]

On date Tuesday 2008-08-19 20:57:55 +0200, Stefano Sabatini encoded:
> On date Monday 2008-08-11 16:05:35 +0200, Stefano Sabatini encoded:
> > 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.
> 
> So what to do with this patch?

Ping? (trying to clean my mailbox...)
-- 
FFmpeg = Frightening Fast Multipurpose Portable Empowered Gymnast