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

[Michael Niedermayer]

On Wed, Jan 02, 2008 at 02:20:50AM +0100, Morten Hustveit wrote:
> On Wed, Jan 02, 2008 at 12:46:38AM +0100, Michael Niedermayer wrote:
> > adding const to stuff should be a seperate patch
> [...]
> > fixing the return type should also be a seperate patch
> [...]
> > this is ugly, either way, it should be a sperate patch
> [...]
> > changing (void*) to the nicer stuff is a cosmetic change and MUST be a 
> > seperate patch
> 
> I do not agree with your patch granularity preferences.  The largest patch
> depends on each of the small patches, but was written first.  The small patches
> are mistakes that were discovered by the "cosmetic change" (excluding
> opt_bitrate, which was discovered in advance).
> 
> Either way, I have split the patch in the suggested fashion.

thanks


> 
> > > -static uint64_t limit_filesize = 0; //
> > > +static int64_t limit_filesize = 0; //
> > 
> > filesize is not signed
> 
> Signed is Correct:
> 
>   - The variable is used only in signed contexts.
>   - ffmpeg's offset_t is signed.
>   - In POSIX, off_t is used for file sizes, and off_t is signed. [1]
> 
> Signed is Sufficient[tm]:
> 
>   If you are going to limit the file size, it will be to less
>   than 8.5 billion gigabytes in at least 9 cases out of 10.

Files have a size >= 0 in at least 9 out of 10 cases, in the remaining
cases you better had a backup
Anyway if people want it signed so be it, it doesnt change anything, theres
no bug it fixes ...


>  
> >> -static void opt_new_audio_stream(void)
> >> +static void opt_new_audio_stream(const char *arg av_unused)
> 
> > this is ugly, [...]
> 
> You have four choices:
> 
>   1. Use the current incorrect function signature and hide the fact by emulating
>      implicit casts with an explicit void* cast.
>   2. Make the current function signature correct by adding yet another option
>      type to optutils.h.
>   3. Use the correct function signature and disregard the parameter.
>   4. Ignore compiler warnings.
> 
> I find #1, #2 and #4 uglier than #3.  #1 and #4 makes bugs harder to spot
> (opt_bitrate is an example of such bug).  #2 increases the number of code lines
> without adding functionality or fixing bugs.

#3 will only hide warnings with gcc as compiler, i consider using compiler
specific things like av_unused ugly, especially if its unneeded.
Also i consider it ugly to have options wihout HAS_ARG which then have a
function ptr with an arument.



[...]

[const patch]

ok


> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 11364)
> +++ ffmpeg.c	(working copy)
> @@ -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; //
>  
>  static int pgmyuv_compatibility_hack=0;
>  static float dts_delta_threshold = 10;

see above



> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 11364)
> +++ ffmpeg.c	(working copy)
> @@ -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)
[...]

Ill wait and see if we can come up with a nice solution before approving
this ...


[return type patch]

ok


[...]

> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 11364)
> +++ ffmpeg.c	(working copy)
> @@ -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" },
> -    { "map", HAS_ARG | OPT_EXPERT, {(void*)opt_map}, "set input stream mapping", "file:stream[:syncfile:syncstream]" },
> -    { "map_meta_data", HAS_ARG | OPT_EXPERT, {(void*)opt_map_meta_data}, "set meta data information of outfile from infile", "outfile:infile" },
> -    { "t", HAS_ARG, {(void*)opt_recording_time}, "record or transcode \"duration\" seconds of audio/video", "duration" },
> -    { "fs", HAS_ARG | OPT_INT64, {(void*)&limit_filesize}, "set the limit file size in bytes", "limit_size" }, //
> -    { "ss", HAS_ARG, {(void*)opt_start_time}, "set the start time offset", "time_off" },
> -    { "itsoffset", HAS_ARG, {(void*)opt_input_ts_offset}, "set the input ts offset", "time_off" },
> -    { "title", HAS_ARG | OPT_STRING, {(void*)&str_title}, "set the title", "string" },
> -    { "timestamp", HAS_ARG, {(void*)&opt_rec_timestamp}, "set the timestamp", "time" },
> -    { "author", HAS_ARG | OPT_STRING, {(void*)&str_author}, "set the author", "string" },
> -    { "copyright", HAS_ARG | OPT_STRING, {(void*)&str_copyright}, "set the copyright", "string" },
> -    { "comment", HAS_ARG | OPT_STRING, {(void*)&str_comment}, "set the comment", "string" },
> -    { "album", HAS_ARG | OPT_STRING, {(void*)&str_album}, "set the album", "string" },
> -    { "benchmark", OPT_BOOL | OPT_EXPERT, {(void*)&do_benchmark},
> +    { "L", 0, {func_arg: opt_show_license}, "show license" },

As mans said, its non standard.

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

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/9e466502/attachment.pgp>