[FFmpeg-devel] [PATCH] incorrect return type for opt_bitrate/type safety

[Michael Niedermayer]

On Wed, Jan 02, 2008 at 12:25:22AM +0100, Morten Hustveit wrote:
> Hello,
> 
> during use I discovered that my -b and -vb options were rejected by ffmpeg.  In
> the `options' array, the `opt_bitrate' function is declared to be a OPT_FUNC2
> handler, yet it void.  OPT_FUNC2 handlers are supposed to return int, and
> negative values for failures.
> 
> This is symptomatic of lack of type safety; the compiler could have warned about
> passing an incorrect function pointer type if given the chance, but in every
> case the pointers were casted to (void*).  I have attached a patch which
> establishes type safety in the `options' array, in addition to removing all the
> warnings this caused.
> 
> -- 
> Morten Hustveit

> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 11364)
> +++ ffmpeg.c	(working copy)
> @@ -125,7 +125,7 @@
>  static float video_rc_qmod_amp=0;
>  static int video_rc_qmod_freq=0;
>  #endif
> -static char *video_rc_override_string=NULL;
> +static const char *video_rc_override_string=NULL;
>  static int video_disable = 0;
>  static int video_discard = 0;
>  static char *video_codec_name = NULL;

adding const to stuff should be a seperate patch


> @@ -204,7 +204,7 @@
>  static int nb_frames_dup = 0;
>  static int nb_frames_drop = 0;
>  static int input_sync;
> -static uint64_t limit_filesize = 0; //
> +static int64_t limit_filesize = 0; //

filesize is not signed


[...]

> @@ -2180,7 +2206,7 @@
>      }
>  }
>  
> -static void opt_bitrate(const char *opt, const char *arg)
> +static int opt_bitrate(const char *opt, const char *arg)
>  {
>      int codec_type = opt[0]=='a' ? CODEC_TYPE_AUDIO : CODEC_TYPE_VIDEO;
>  
> @@ -2188,6 +2214,8 @@
>  
>      if (av_get_int(avctx_opts[codec_type], "b", NULL) < 1000)
>          fprintf(stderr, "WARNING: The bitrate parameter is set too low. It takes bits/s as argument, not kbits/s\n");
> +
> +    return 0;
>  }
>  

fixing the return type should also be a seperate patch


[...]
> @@ -3092,7 +3112,7 @@
>      subtitle_stream_copy = 0;
>  }
>  
> -static void opt_new_audio_stream(void)
> +static void opt_new_audio_stream(const char *arg av_unused)
>  {
>      AVFormatContext *oc;
>      if (nb_output_files <= 0) {
> @@ -3103,7 +3123,7 @@
>      new_audio_stream(oc);
>  }
>  
> -static void opt_new_video_stream(void)
> +static void opt_new_video_stream(const char *arg av_unused)
>  {
>      AVFormatContext *oc;
>      if (nb_output_files <= 0) {
> @@ -3114,7 +3134,7 @@
>      new_video_stream(oc);
>  }
>  
> -static void opt_new_subtitle_stream(void)
> +static void opt_new_subtitle_stream(const char *arg av_unused)
>  {
>      AVFormatContext *oc;
>      if (nb_output_files <= 0) {
> @@ -3308,7 +3328,7 @@
>  #endif
>  }
>  
> -static void opt_show_formats(void)
> +static void opt_show_formats(const char *arg av_unused)
>  {
>      AVInputFormat *ifmt=NULL;
>      AVOutputFormat *ofmt=NULL;
> @@ -3503,7 +3523,7 @@
>      av_opt_show(sws_opts, NULL);
>  }
>  
> -static void opt_show_help(void)
> +static void opt_show_help(const char *arg av_unused)
>  {
>      show_help();
>      exit(0);
> @@ -3660,7 +3680,7 @@
>      vstats_filename=av_strdup (arg);
>  }
>  
> -static void opt_vstats (void)
> +static void opt_vstats (const char *arg av_unused)
>  {
>      char filename[40];
>      time_t today2 = time(NULL);

this is ugly, either way, it should be a sperate patch


[...]
> @@ -3702,125 +3724,125 @@
>  
>  const OptionDef options[] = {
>      /* main options */
> -    { "L", 0, {(void*)opt_show_license}, "show license" },
> -    { "h", 0, {(void*)opt_show_help}, "show help" },
> -    { "version", 0, {(void*)opt_show_version}, "show version" },
> -    { "formats", 0, {(void*)opt_show_formats}, "show available formats, codecs, protocols, ..." },
> -    { "f", HAS_ARG, {(void*)opt_format}, "force format", "fmt" },
> -    { "i", HAS_ARG, {(void*)opt_input_file}, "input file name", "filename" },
> -    { "y", OPT_BOOL, {(void*)&file_overwrite}, "overwrite output files" },
[...]
> +    { "L", 0, {func_arg: opt_show_license}, "show license" },
> +    { "h", 0, {func_arg: opt_show_help}, "show help" },
> +    { "version", 0, {func_arg: opt_show_version}, "show version" },
> +    { "formats", 0, {func_arg: opt_show_formats}, "show available formats, codecs, protocols, ..." },
> +    { "f", HAS_ARG, {func_arg: opt_format}, "force format", "fmt" },
> +    { "i", HAS_ARG, {func_arg: opt_input_file}, "input file name", "filename" },
> +    { "y", OPT_BOOL, {int_arg: &file_overwrite}, "overwrite output files" },

changing (void*) to the nicer stuff is a cosmetic change and MUST be a 
seperate patch


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

There will always be a question for which you do not know the correct awnser.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080102/334757d6/attachment.pgp>