[FFmpeg-devel] [PATCH v2] fftools/cmdutils: add support for level flag in loglevel option parser

[Tobias Rapp]

On 22.03.2018 00:59, Michael Niedermayer wrote:
> On Wed, Mar 21, 2018 at 03:20:30PM +0100, Tobias Rapp wrote:
>> Allows to manage the AV_LOG_PRINT_LEVEL flag as a prefix to the loglevel
>> option value, similar to the existing AV_LOG_SKIP_REPEATED flag.
>>
>> Signed-off-by: Tobias Rapp <t.rapp at noa-archive.com>
>> ---
>>   doc/fftools-common-opts.texi | 11 +++++++----
>>   fftools/cmdutils.c           | 26 +++++++++++++++++++-------
>>   2 files changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/doc/fftools-common-opts.texi b/doc/fftools-common-opts.texi
>> index 185ec21..9b6bc44 100644
>> --- a/doc/fftools-common-opts.texi
>> +++ b/doc/fftools-common-opts.texi
>> @@ -168,14 +168,17 @@ The returned list cannot be assumed to be always complete.
>>   ffmpeg -sinks pulse,server=192.168.0.4
>>   @end example
>>   
>> - at item -loglevel [repeat+]@var{loglevel} | -v [repeat+]@var{loglevel}
>> + at item -loglevel [repeat+][level+]@var{loglevel} | -v [repeat+][level+]@var{loglevel}
>>   Set the logging level used by the library.
>>   Adding "repeat+" indicates that repeated log output should not be compressed
>>   to the first line and the "Last message repeated n times" line will be
>>   omitted. "repeat" can also be used alone.
>> -If "repeat" is used alone, and with no prior loglevel set, the default
>> -loglevel will be used. If multiple loglevel parameters are given, using
>> -'repeat' will not change the loglevel.
>> +Adding "level+" 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.
>> +If "repeat" and/or "level" is used alone, and with no prior loglevel set, the
>> +default loglevel will be used. If multiple loglevel parameters are given, using
>> +'repeat'/'level' will not change the loglevel.
>>   @var{loglevel} is a string or a number containing one of the following values:
>>   @table @samp
>>   @item quiet, -8
>> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
>> index 708a849..51fa88c 100644
>> --- a/fftools/cmdutils.c
>> +++ b/fftools/cmdutils.c
>> @@ -888,16 +888,28 @@ int opt_loglevel(void *optctx, const char *opt, const char *arg)
>>   
>>       flags = av_log_get_flags();
>>       tail = strstr(arg, "repeat");
>> -    if (tail)
>> +    if (tail == arg) {
>>           flags &= ~AV_LOG_SKIP_REPEATED;
>> -    else
>> +        arg += 6 + (arg[6] == '+');
>> +        if (!*arg) {
>> +            av_log_set_flags(flags);
>> +            return 0;
>> +        }
>> +    } else {
>>           flags |= AV_LOG_SKIP_REPEATED;
>> -
>> +    }
>> +    tail = strstr(arg, "level");
>> +    if (tail == arg) {
>> +        flags |= AV_LOG_PRINT_LEVEL;
>> +        arg += 5 + (arg[5] == '+');
>> +        if (!*arg) {
>> +            av_log_set_flags(flags);
>> +            return 0;
>> +        }
>> +    } else {
>> +        flags &= ~AV_LOG_PRINT_LEVEL;
>> +    }
>>       av_log_set_flags(flags);
>> -    if (tail == arg)
>> -        arg += 6 + (arg[6]=='+');
>> -    if(tail && !*arg)
>> -        return 0;
> 
> might be simpler to use av_strtok()
> 
> also this code should idealy be moved into libavutil so other applications
> do not need to duplicate it

A useful helper function would allow to update one flag without 
affecting the other. Implementation would end up similar to parsing 
"normal" option flags with the level number handling on-top.

Do you have some suggestion on how to do this with least code 
duplication? Maybe checking the right-hand-side of the string for 
matching a level name or being a number string and then passing the rest 
to av_opt_eval_flags?

Regards,
Tobias