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

[Stefano Sabatini]

On date Thursday 2007-04-19 18:03:55 +0200, Benoit Fouet encoded:
> 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:
> [...]
> >> 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 :)

Most cli I know adopt this rule: later settings override previous ones
(I think this is valid for ffmpeg too). So I suppose most users forged
their user interface expectation on this rule, just like me :-).

> > 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;

I preferred the previous one because there are other data initialized
in the code, so I'm keeping the initialized version.

> >  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...

Cleaned out... I'll try to put more attention to these issues the next times...

Cheers
-- 
Stefano Sabatini
Linux user number 337176 (see http://counter.li.org)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vstats_file.patch
Type: text/x-diff
Size: 4328 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070419/4535db81/attachment.patch>