[Ffmpeg-devel] Changing "-vstats" option behaviour
Stefano Sabatini
stefano.sabatini-lala
Thu Apr 19 18:47:44 CEST 2007
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>
More information about the ffmpeg-devel
mailing list