[FFmpeg-devel] [PATCH v7 2/2] fftools: Add option to log timing

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Tuesday, 10 August 2021 20:49
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v7 2/2] fftools: Add option to log
> timing
> 
> Soft Works:
> > This commit adds two logging flags: 'time' and 'datetime'.
> >
> > Usage:
> >
> > ffmpeg -loglevel +time
> >
> > or
> >
> > ffmpeg -loglevel +datetime
> >
> > Signed-off-by: softworkz <softworkz at hotmail.com>
> > ---
> >  doc/fftools-common-opts.texi |  4 ++++
> >  fftools/cmdutils.c           | 21 +++++++++++++++++++++
> >  fftools/ffmpeg.c             |  6 +++++-
> >  3 files changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/doc/fftools-common-opts.texi b/doc/fftools-common-
> opts.texi
> > index 7643dd8396..aae26c28d0 100644
> > --- a/doc/fftools-common-opts.texi
> > +++ b/doc/fftools-common-opts.texi
> > @@ -198,6 +198,10 @@ and the "Last message repeated n times" line
> will be omitted.
> >  Indicates that log output should add a @code{[level]} prefix to
> each message
> >  line. This can be used as an alternative to log coloring, e.g.
> when dumping the
> >  log to file.
> > + at item time
> > +Prefixes each log line with the local time.
> > + at item datetime
> > +Same as time but also prints the current date in each line.
> >  @end table
> >  Flags can also be used alone by adding a '+'/'-' prefix to
> set/reset a single
> >  flag without affecting other @var{flags} or changing
> @var{loglevel}. When
> > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> > index 912e881174..7918078766 100644
> > --- a/fftools/cmdutils.c
> > +++ b/fftools/cmdutils.c
> > @@ -918,6 +918,27 @@ int opt_loglevel(void *optctx, const char
> *opt, const char *arg)
> >                  flags |= AV_LOG_PRINT_LEVEL;
> >              }
> >              arg = token + 5;
> > +        } else if (av_strstart(token, "time", NULL)) {
> > +            if (cmd == '-') {
> > +                flags &= ~AV_LOG_PRINT_TIME;
> > +            } else {
> > +                flags |= AV_LOG_PRINT_TIME;
> > +            }
> > +            arg = token + 4;
> > +        } else if (av_strstart(token, "timing", NULL)) {
> 
> Why are you recognizing an undocumented and redundant command? Just
> remove this.

My mistake, this should only go into my fork where I'm having this 
for years already.

> 
> > +            if (cmd == '-') {
> > +                flags &= ~AV_LOG_PRINT_TIME;
> > +            } else {
> > +                flags |= AV_LOG_PRINT_TIME;
> > +            }
> > +            arg = token + 6;
> > +        } else if (av_strstart(token, "datetime", NULL)) {
> > +            if (cmd == '-') {
> > +                flags &= ~AV_LOG_PRINT_DATETIME;
> > +            } else {
> > +                flags |= AV_LOG_PRINT_DATETIME;
> > +            }
> > +            arg = token + 8;
> 
> This still hardcodes the lengths of the strings, resulting in
> potential
> breakage if one is updated and the other is forgotten. See my patch
> for
> how to avoid this.

Yes, but ALL the code is doing like this. Why should I just change 
the new code and mix it up with the existing code, resulting in 
a weird mashup that is much worse IMO than that tiny caveat that you 
are mentioning.


> 
> >          } else {
> >              break;
> >          }
> > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> > index b0ce7c7c32..b0b105be1c 100644
> > --- a/fftools/ffmpeg.c
> > +++ b/fftools/ffmpeg.c
> > @@ -4983,7 +4983,7 @@ static void log_callback_null(void *ptr, int
> level, const char *fmt, va_list vl)
> >
> >  int main(int argc, char **argv)
> >  {
> > -    int i, ret;
> > +    int i, ret, log_flags;
> >      BenchmarkTimeStamps ti;
> >
> >      init_dynload();
> > @@ -5049,6 +5049,10 @@ int main(int argc, char **argv)
> >      if ((decode_error_stat[0] + decode_error_stat[1]) *
> max_error_rate < decode_error_stat[1])
> >          exit_program(69);
> >
> > +    log_flags = av_log_get_flags();
> > +    if (log_flags & AV_LOG_PRINT_TIME || log_flags &
> AV_LOG_PRINT_DATETIME)
> 
> You can check both flags at once (as Nicolas has already pointed
> out).

I had made that change. No idea where it got lost. That ML+Patch
stuff is turning me mad. Sigh..

Thanks for your patience,
softworkz