[Ffmpeg-devel] Changing "-vstats" option behaviour

[Benoit Fouet]

Hi,

Stefano Sabatini wrote:
> On date Thursday 2007-04-19 10:38:18 +0200, Benoit Fouet encoded:
>   
>> Hi,
>>
>> Stefano Sabatini wrote:
>>     
>>> On date Wednesday 2007-04-18 09:15:00 +0200, Benoit Fouet encoded:
>>>   
>>>       
>>>> isn't it also possible to keep vstats option, and internally call
>>>> opt_vstats_file with the right filename ?
>>>> that way, we keep what exists and offer a new option...
>>>> any thoughts ?
>>>>     
>>>>         
>>> [...]
>>>
>>> Anyway I find confusing to have two different options for this (rather
>>> than having two ways to do a thing, it's better to keep just one
>>> method, the more general), and the above solution isn't completely
>>> safe (-vstats_file generic-filename -vstats will print to
>>> generic-filename rather than to the vstats_HHMMSS.log file).
>>>
>>>   
>>>       
>> this will just allow to keep what exists.
>> the behavior when -vstats and -vstats_file are used in the same command
>> line could be a warning, an exit, ...
>> anyway, i thought about something like the attached patch.
>> thoughts ?
>>
>>     
>
> In this case:
> ffmpeg <other options> -vstats_file foo -vstats_file bar -vstats
>
> it will print to the first defined file (which isn't the expected
> behaviour for most users).
>
>   
i don't personnaly know most users :)

> A possible solution is showed in the attached patch. In this case the
> vstats_filename plays the role of the global var, while the fvstats is
> defined in the do_video_stats scope, and is opened the first time
> do_video_stats is executed.  
>
> The various opt_vstats_file and opt_vstats simply change the value of
> the global vstats_filename.
>
> A warning message like this:
> Warning: redefining the already defined vstats file from "foo" to "bar"
> Warning: redefining the already defined vstats file from "bar" to "vstats_174600.log"
>
> is issued when redefining the vstats filename.
>
>   
FWIW, i prefer...

> Cheers
>   
> ------------------------------------------------------------------------
>
> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 8759)
> +++ ffmpeg.c	(working copy)
> @@ -166,7 +166,6 @@
>  static int do_hex_dump = 0;
>  static int do_pkt_dump = 0;
>  static int do_psnr = 0;
> -static int do_vstats = 0;
>  static int do_pass = 0;
>  static char *pass_logfilename = NULL;
>  static int audio_stream_copy = 0;
> @@ -177,6 +176,7 @@
>  static int copy_ts= 0;
>  static int opt_shortest = 0; //
>  static int video_global_header = 0;
> +static char* vstats_filename=NULL;
>  
>   
initialisation is unneeded, and i don't know about "coding rules" in
FFmpeg, but i personnaly prefer:
static char *vstats_filename;

>  static int rate_emu = 0;
>  
> @@ -841,28 +841,22 @@
>  static void do_video_stats(AVFormatContext *os, AVOutputStream *ost,
>                             int frame_size)
>  {
> -    static FILE *fvstats=NULL;
> -    char filename[40];
> -    time_t today2;
> -    struct tm *today;
>      AVCodecContext *enc;
>      int frame_number;
>      int64_t ti;
>      double ti1, bitrate, avg_bitrate;
> +    
> +    static FILE* fvstats=NULL;
>  
>   
cosmetics + trailing white spaces (which are forbidden in svn)

> +    /* this is executed just the first time do_video_stats is executed */
>      if (!fvstats) {
> -        today2 = time(NULL);
> -        today = localtime(&today2);
> -        snprintf(filename, sizeof(filename), "vstats_%02d%02d%02d.log", today->tm_hour,
> -                                               today->tm_min,
> -                                               today->tm_sec);
> -        fvstats = fopen(filename,"w");
> -        if (!fvstats) {
> +        fvstats = fopen(vstats_filename, "w");
> +        if (!fvstats) {        
>   
trailing whitespaces

>              perror("fopen");
>              exit(1);
> -        }
> +        }   
>      }
> -
> +    
>   
and here too

>      ti = INT64_MAX;
>      enc = ost->st->codec;
>      if (enc->codec_type == CODEC_TYPE_VIDEO) {
> @@ -1197,7 +1191,7 @@
>                          case CODEC_TYPE_VIDEO:
>                              do_video_out(os, ost, ist, &picture, &frame_size);
>                              video_size += frame_size;
> -                            if (do_vstats && frame_size)
> +                            if (vstats_filename && frame_size)
>                                  do_video_stats(os, ost, frame_size);
>                              break;
>                          case CODEC_TYPE_SUBTITLE:
> @@ -3449,6 +3443,30 @@
>      }
>  }
>  
> +static void opt_vstats_file (const char *arg)
> +{
> +    /* if the vstats_filename global has been already defined */
> +    if (vstats_filename) {
> +        printf ("Warning: redefining the already defined vstats file from \"%s\" to \"%s\"\n",
> +                vstats_filename, arg);
>   
please avoid printf use

> +        av_free (vstats_filename);
> +    }
> +
> +    vstats_filename=av_strdup (arg);
> +}
> +
> +static void opt_vstats (void)
> +{
> +    char filename[40];
> +    time_t today2 = time(NULL);
> +    struct tm *today = localtime(&today2);
> +    
>   
trailing whitespaces

i am of course not ffmpeg maintainer, so this is just my thoughts...

Ben

-- 
Purple Labs S.A.
www.purplelabs.com