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

[Andreas Rheinhardt]

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.

- Andreas