diff mbox series

[FFmpeg-devel,v2] fftools/cmdutils: don't print build configuration by default

Message ID 20210806211703.9041-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2] fftools/cmdutils: don't print build configuration by default
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer Aug. 6, 2021, 9:17 p.m. UTC
Suggested-by: ffmpeg@fb.com
Signed-off-by: James Almer <jamrial@gmail.com>
---
 fftools/cmdutils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Lynne Aug. 6, 2021, 9:29 p.m. UTC | #1
6 Aug 2021, 23:17 by jamrial@gmail.com:

> Suggested-by: ffmpeg@fb.com
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  fftools/cmdutils.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> index 912e881174..e6ae749167 100644
> --- a/fftools/cmdutils.c
> +++ b/fftools/cmdutils.c
> @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags, int level)
>  static void print_program_info(int flags, int level)
>  {
>  const char *indent = flags & INDENT? "  " : "";
> +    // Show config only if level is verbose or higher
> +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE : level;
>  
>  av_log(NULL, level, "%s version " FFMPEG_VERSION, program_name);
>  if (flags & SHOW_COPYRIGHT)
> @@ -1160,7 +1162,7 @@ static void print_program_info(int flags, int level)
>  av_log(NULL, level, "\n");
>  av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT);
>  
> -    av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
> +    av_log(NULL, config_level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
>  }
>  
>  static void print_buildconf(int flags, int level)
> -- 
> 2.32.0
>

That's even worse. Verbose is too verbose to be useful, especially
when parsing H264 in MP4.
I'm happy with the default, and if we had to hide the build config,
having it in -version and -loglevel verbose both would be better.
But I'd rather keep it as-is.
James Almer Aug. 6, 2021, 9:32 p.m. UTC | #2
On 8/6/2021 6:29 PM, Lynne wrote:
> 6 Aug 2021, 23:17 by jamrial@gmail.com:
> 
>> Suggested-by: ffmpeg@fb.com
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   fftools/cmdutils.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
>> index 912e881174..e6ae749167 100644
>> --- a/fftools/cmdutils.c
>> +++ b/fftools/cmdutils.c
>> @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags, int level)
>>   static void print_program_info(int flags, int level)
>>   {
>>   const char *indent = flags & INDENT? "  " : "";
>> +    // Show config only if level is verbose or higher
>> +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE : level;
>>   
>>   av_log(NULL, level, "%s version " FFMPEG_VERSION, program_name);
>>   if (flags & SHOW_COPYRIGHT)
>> @@ -1160,7 +1162,7 @@ static void print_program_info(int flags, int level)
>>   av_log(NULL, level, "\n");
>>   av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT);
>>   
>> -    av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
>> +    av_log(NULL, config_level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
>>   }
>>   
>>   static void print_buildconf(int flags, int level)
>> -- 
>> 2.32.0
>>
> 
> That's even worse. Verbose is too verbose to be useful, especially
> when parsing H264 in MP4.

I'm not sure i get it. I'm showing the build config only in verbose or 
higher to ensure bug reports don't require extra steps for the reporter. 
What exactly does H264 and MP4 have to do with this?

> I'm happy with the default, and if we had to hide the build config,
> having it in -version and -loglevel verbose both would be better.

This is exactly what this patch does.

> But I'd rather keep it as-is.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Soft Works Aug. 6, 2021, 9:35 p.m. UTC | #3
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Lynne
> Sent: Friday, 6 August 2021 23:30
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print build
> configuration by default
> 
> 6 Aug 2021, 23:17 by jamrial@gmail.com:
> 
> > Suggested-by: ffmpeg@fb.com
> > Signed-off-by: James Almer <jamrial@gmail.com>
> > ---
> >  fftools/cmdutils.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index
> > 912e881174..e6ae749167 100644
> > --- a/fftools/cmdutils.c
> > +++ b/fftools/cmdutils.c
> > @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags, int
> > level)  static void print_program_info(int flags, int level)  {  const
> > char *indent = flags & INDENT? "  " : "";
> > +    // Show config only if level is verbose or higher
> > +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE : level;
> >
> >  av_log(NULL, level, "%s version " FFMPEG_VERSION, program_name);  if
> > (flags & SHOW_COPYRIGHT) @@ -1160,7 +1162,7 @@ static void
> > print_program_info(int flags, int level)  av_log(NULL, level, "\n");
> > av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT);
> >
> > -    av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n",
> indent);
> > +    av_log(NULL, config_level, "%sconfiguration: "
> > + FFMPEG_CONFIGURATION "\n", indent);
> >  }
> >
> >  static void print_buildconf(int flags, int level)
> > --
> > 2.32.0
> >
> 
> That's even worse. Verbose is too verbose to be useful, especially
> when parsing H264 in MP4.

You are right. There are two or three annoying outputs with loglevel
VERBOSE. I had changed those to TRACE and VERBOSE is now very
useful and actually my preferred level when testing things.

softworkz
Soft Works Aug. 6, 2021, 9:38 p.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> James Almer
> Sent: Friday, 6 August 2021 23:17
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print build
> configuration by default
> 
> Suggested-by: ffmpeg@fb.com
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  fftools/cmdutils.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index
> 912e881174..e6ae749167 100644
> --- a/fftools/cmdutils.c
> +++ b/fftools/cmdutils.c
> @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags, int level)
> static void print_program_info(int flags, int level)  {
>      const char *indent = flags & INDENT? "  " : "";
> +    // Show config only if level is verbose or higher
> +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE : level;

I don't think it will work this way because level is hardcoded to AV_LOG_INFO
in both upstream calls.

> 
>      av_log(NULL, level, "%s version " FFMPEG_VERSION, program_name);
>      if (flags & SHOW_COPYRIGHT)
> @@ -1160,7 +1162,7 @@ static void print_program_info(int flags, int level)
>      av_log(NULL, level, "\n");
>      av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT);
> 
> -    av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n",
> indent);
> +    av_log(NULL, config_level, "%sconfiguration: "
> FFMPEG_CONFIGURATION
> + "\n", indent);
>  }
> 
>  static void print_buildconf(int flags, int level)
> --
> 2.32.0
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
> with subject "unsubscribe".
James Almer Aug. 6, 2021, 9:49 p.m. UTC | #5
On 8/6/2021 6:38 PM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> James Almer
>> Sent: Friday, 6 August 2021 23:17
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print build
>> configuration by default
>>
>> Suggested-by: ffmpeg@fb.com
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   fftools/cmdutils.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index
>> 912e881174..e6ae749167 100644
>> --- a/fftools/cmdutils.c
>> +++ b/fftools/cmdutils.c
>> @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags, int level)
>> static void print_program_info(int flags, int level)  {
>>       const char *indent = flags & INDENT? "  " : "";
>> +    // Show config only if level is verbose or higher
>> +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE : level;
> 
> I don't think it will work this way because level is hardcoded to AV_LOG_INFO
> in both upstream calls.

I can hardcode it to VERBOSE if that's preferred. I made it this way 
since there could be new callers in the future that could choose a 
different level. Otherwise we could just remove the level parameter 
altogether.

> 
>>
>>       av_log(NULL, level, "%s version " FFMPEG_VERSION, program_name);
>>       if (flags & SHOW_COPYRIGHT)
>> @@ -1160,7 +1162,7 @@ static void print_program_info(int flags, int level)
>>       av_log(NULL, level, "\n");
>>       av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT);
>>
>> -    av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n",
>> indent);
>> +    av_log(NULL, config_level, "%sconfiguration: "
>> FFMPEG_CONFIGURATION
>> + "\n", indent);
>>   }
>>
>>   static void print_buildconf(int flags, int level)
>> --
>> 2.32.0
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org
>> with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Soft Works Aug. 6, 2021, 10:08 p.m. UTC | #6
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> James Almer
> Sent: Friday, 6 August 2021 23:49
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print build
> configuration by default
> 
> On 8/6/2021 6:38 PM, Soft Works wrote:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> James Almer
> >> Sent: Friday, 6 August 2021 23:17
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print
> >> build configuration by default
> >>
> >> Suggested-by: ffmpeg@fb.com
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >>   fftools/cmdutils.c | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index
> >> 912e881174..e6ae749167 100644
> >> --- a/fftools/cmdutils.c
> >> +++ b/fftools/cmdutils.c
> >> @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags, int
> >> level) static void print_program_info(int flags, int level)  {
> >>       const char *indent = flags & INDENT? "  " : "";
> >> +    // Show config only if level is verbose or higher
> >> +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE :
> >> + level;
> >
> > I don't think it will work this way because level is hardcoded to
> > AV_LOG_INFO in both upstream calls.
> 
> I can hardcode it to VERBOSE if that's preferred. I made it this way since there
> could be new callers in the future that could choose a different level.
> Otherwise we could just remove the level parameter altogether.

Yes, that would make it more clear. But another parameter is needed,
because when it's called from show_version, it should always be printed,
regardless of loglevel.

I have it like this:

void show_banner(int argc, char **argv, const OptionDef *options)
{
    int idx = locate_option(argc, argv, options, "version");
    if (hide_banner || idx)
        return;

    print_program_info (INDENT|SHOW_COPYRIGHT, AV_LOG_INFO, 0);  <==
    print_all_libs_info(INDENT|SHOW_CONFIG,  AV_LOG_INFO);
    print_all_libs_info(INDENT|SHOW_VERSION, AV_LOG_DEBUG);
}

int show_version(void *optctx, const char *opt, const char *arg)
{
    av_log_set_callback(log_callback_help);
    print_program_info (SHOW_COPYRIGHT, AV_LOG_INFO, 1) );  <==
    print_all_libs_info(SHOW_VERSION, AV_LOG_INFO);

    return 0;
}
James Almer Aug. 6, 2021, 10:17 p.m. UTC | #7
On 8/6/2021 7:08 PM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> James Almer
>> Sent: Friday, 6 August 2021 23:49
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print build
>> configuration by default
>>
>> On 8/6/2021 6:38 PM, Soft Works wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>>> James Almer
>>>> Sent: Friday, 6 August 2021 23:17
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Subject: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print
>>>> build configuration by default
>>>>
>>>> Suggested-by: ffmpeg@fb.com
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>    fftools/cmdutils.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index
>>>> 912e881174..e6ae749167 100644
>>>> --- a/fftools/cmdutils.c
>>>> +++ b/fftools/cmdutils.c
>>>> @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags, int
>>>> level) static void print_program_info(int flags, int level)  {
>>>>        const char *indent = flags & INDENT? "  " : "";
>>>> +    // Show config only if level is verbose or higher
>>>> +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE :
>>>> + level;
>>>
>>> I don't think it will work this way because level is hardcoded to
>>> AV_LOG_INFO in both upstream calls.
>>
>> I can hardcode it to VERBOSE if that's preferred. I made it this way since there
>> could be new callers in the future that could choose a different level.
>> Otherwise we could just remove the level parameter altogether.
> 
> Yes, that would make it more clear. But another parameter is needed,
> because when it's called from show_version, it should always be printed,
> regardless of loglevel.

Does "ffmpeg -version" not show it for you with this patch? It does for me.

> 
> I have it like this:
> 
> void show_banner(int argc, char **argv, const OptionDef *options)
> {
>      int idx = locate_option(argc, argv, options, "version");
>      if (hide_banner || idx)
>          return;
> 
>      print_program_info (INDENT|SHOW_COPYRIGHT, AV_LOG_INFO, 0);  <==
>      print_all_libs_info(INDENT|SHOW_CONFIG,  AV_LOG_INFO);
>      print_all_libs_info(INDENT|SHOW_VERSION, AV_LOG_DEBUG);
> }
> 
> int show_version(void *optctx, const char *opt, const char *arg)
> {
>      av_log_set_callback(log_callback_help);
>      print_program_info (SHOW_COPYRIGHT, AV_LOG_INFO, 1) );  <==
>      print_all_libs_info(SHOW_VERSION, AV_LOG_INFO);
> 
>      return 0;
> }
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Lynne Aug. 6, 2021, 11:18 p.m. UTC | #8
6 Aug 2021, 23:32 by jamrial@gmail.com:

> On 8/6/2021 6:29 PM, Lynne wrote:
>
>> 6 Aug 2021, 23:17 by jamrial@gmail.com:
>>
>>> Suggested-by: ffmpeg@fb.com
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>  fftools/cmdutils.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
>>> index 912e881174..e6ae749167 100644
>>> --- a/fftools/cmdutils.c
>>> +++ b/fftools/cmdutils.c
>>> @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags, int level)
>>>  static void print_program_info(int flags, int level)
>>>  {
>>>  const char *indent = flags & INDENT? "  " : "";
>>> +    // Show config only if level is verbose or higher
>>> +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE : level;
>>>  av_log(NULL, level, "%s version " FFMPEG_VERSION, program_name);
>>>  if (flags & SHOW_COPYRIGHT)
>>> @@ -1160,7 +1162,7 @@ static void print_program_info(int flags, int level)
>>>  av_log(NULL, level, "\n");
>>>  av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT);
>>>  -    av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
>>> +    av_log(NULL, config_level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
>>>  }
>>>  static void print_buildconf(int flags, int level)
>>> -- 
>>> 2.32.0
>>>
>>
>> That's even worse. Verbose is too verbose to be useful, especially
>> when parsing H264 in MP4.
>>
>
> I'm not sure i get it. I'm showing the build config only in verbose or higher to ensure bug reports don't require extra steps for the reporter. What exactly does H264 and MP4 have to do with this?
>

It's spammy as hell. It prints every single NAL.
Though I just checked, and it's spammy on debug, not verbose.
Still, NAL printing should be absolutely trace level. It really makes
debug a useless level, and everything else we have is quite responsibly
printing what it should on debug.


>> I'm happy with the default, and if we had to hide the build config,
>> having it in -version and -loglevel verbose both would be better.
>>
>
> This is exactly what this patch does.
>

How? Does -version modify the log level?
James Almer Aug. 6, 2021, 11:41 p.m. UTC | #9
On 8/6/2021 8:18 PM, Lynne wrote:
> 6 Aug 2021, 23:32 by jamrial@gmail.com:
> 
>> On 8/6/2021 6:29 PM, Lynne wrote:
>>
>>> 6 Aug 2021, 23:17 by jamrial@gmail.com:
>>>
>>>> Suggested-by: ffmpeg@fb.com
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>   fftools/cmdutils.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
>>>> index 912e881174..e6ae749167 100644
>>>> --- a/fftools/cmdutils.c
>>>> +++ b/fftools/cmdutils.c
>>>> @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags, int level)
>>>>   static void print_program_info(int flags, int level)
>>>>   {
>>>>   const char *indent = flags & INDENT? "  " : "";
>>>> +    // Show config only if level is verbose or higher
>>>> +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE : level;
>>>>   av_log(NULL, level, "%s version " FFMPEG_VERSION, program_name);
>>>>   if (flags & SHOW_COPYRIGHT)
>>>> @@ -1160,7 +1162,7 @@ static void print_program_info(int flags, int level)
>>>>   av_log(NULL, level, "\n");
>>>>   av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT);
>>>>   -    av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
>>>> +    av_log(NULL, config_level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
>>>>   }
>>>>   static void print_buildconf(int flags, int level)
>>>> -- 
>>>> 2.32.0
>>>>
>>>
>>> That's even worse. Verbose is too verbose to be useful, especially
>>> when parsing H264 in MP4.
>>>
>>
>> I'm not sure i get it. I'm showing the build config only in verbose or higher to ensure bug reports don't require extra steps for the reporter. What exactly does H264 and MP4 have to do with this?
>>
> 
> It's spammy as hell. It prints every single NAL.
> Though I just checked, and it's spammy on debug, not verbose.
> Still, NAL printing should be absolutely trace level. It really makes
> debug a useless level, and everything else we have is quite responsibly
> printing what it should on debug.

My question is, what does unrelated output of verbose level have to do 
with this patch? If you prefer to use info because verbose is too 
verbose, then keep doing so?

> 
> 
>>> I'm happy with the default, and if we had to hide the build config,
>>> having it in -version and -loglevel verbose both would be better.
>>>
>>
>> This is exactly what this patch does.
>>
> 
> How? Does -version modify the log level?

No, it overrides the log callback with av_log_set_callback() with a 
function that ignores the level.

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Soft Works Aug. 6, 2021, 11:55 p.m. UTC | #10
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> James Almer
> Sent: Saturday, 7 August 2021 00:17
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print build
> configuration by default
> 
> On 8/6/2021 7:08 PM, Soft Works wrote:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> James Almer
> >> Sent: Friday, 6 August 2021 23:49
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print
> >> build configuration by default
> >>
> >> On 8/6/2021 6:38 PM, Soft Works wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> >>>> James Almer
> >>>> Sent: Friday, 6 August 2021 23:17
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Subject: [FFmpeg-devel] [PATCH v2] fftools/cmdutils: don't print
> >>>> build configuration by default
> >>>>
> >>>> Suggested-by: ffmpeg@fb.com
> >>>> Signed-off-by: James Almer <jamrial@gmail.com>
> >>>> ---
> >>>>    fftools/cmdutils.c | 4 +++-
> >>>>    1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index
> >>>> 912e881174..e6ae749167 100644
> >>>> --- a/fftools/cmdutils.c
> >>>> +++ b/fftools/cmdutils.c
> >>>> @@ -1152,6 +1152,8 @@ static void print_all_libs_info(int flags,
> >>>> int
> >>>> level) static void print_program_info(int flags, int level)  {
> >>>>        const char *indent = flags & INDENT? "  " : "";
> >>>> +    // Show config only if level is verbose or higher
> >>>> +    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE :
> >>>> + level;
> >>>
> >>> I don't think it will work this way because level is hardcoded to
> >>> AV_LOG_INFO in both upstream calls.
> >>
> >> I can hardcode it to VERBOSE if that's preferred. I made it this way
> >> since there could be new callers in the future that could choose a
> different level.
> >> Otherwise we could just remove the level parameter altogether.
> >
> > Yes, that would make it more clear. But another parameter is needed,
> > because when it's called from show_version, it should always be
> > printed, regardless of loglevel.
> 
> Does "ffmpeg -version" not show it for you with this patch? It does for me.

Yes it does. Sorry, I have a few other modifications which required 
a different approach (years ago).

LGTM. 
(tested against the master branch now)

softworkz
diff mbox series

Patch

diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
index 912e881174..e6ae749167 100644
--- a/fftools/cmdutils.c
+++ b/fftools/cmdutils.c
@@ -1152,6 +1152,8 @@  static void print_all_libs_info(int flags, int level)
 static void print_program_info(int flags, int level)
 {
     const char *indent = flags & INDENT? "  " : "";
+    // Show config only if level is verbose or higher
+    int config_level = level <= AV_LOG_INFO ? AV_LOG_VERBOSE : level;
 
     av_log(NULL, level, "%s version " FFMPEG_VERSION, program_name);
     if (flags & SHOW_COPYRIGHT)
@@ -1160,7 +1162,7 @@  static void print_program_info(int flags, int level)
     av_log(NULL, level, "\n");
     av_log(NULL, level, "%sbuilt with %s\n", indent, CC_IDENT);
 
-    av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
+    av_log(NULL, config_level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent);
 }
 
 static void print_buildconf(int flags, int level)