[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 21:09
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [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.
> > I actually intended my patch
> (https://ffmpeg.org/pipermail/ffmpeg-devel/2021-August/283439.html)
> which deals with the legacy flags to be applied first, so there will
> be
> no mashup.

You could have done this after merging my patch and do everything exactly
as you want it to have. I have oriented my patch based on the existing
code and when you find that incorrect, you'll have to change it anyway,
so you could have also changed the parts that I'm adding afterwards.
Instead you force me to wait for your patch, rebase and adapt my patch
based on your change. 
I'm afraid, the number of hoops I'm ready to jump through is exhausted.

Kind regards,
softworkz