Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
On 8/8/2021 12:31 AM, Gyan Doshi wrote: On 2021-08-06 11:34 pm, James Almer wrote: From: Matthieu Patou Suggested-by: ffm...@fb.com Signed-off-by: James Almer --- fftools/cmdutils.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 912e881174..fc58277df7 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -1160,7 +1160,8 @@ 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); + if (flags & SHOW_CONFIG) + av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); I like it. If most users pasted the full log on their own initiative in bug reports or Q then this would have been a negative but they don't. I suggest to add an else clause that prints something like "Add -buildconf to view configuration". Fun fact, the original -buildconf patchset included¹ that as an option, by letting users opt to disable the configuration: line entirely, whereby it would notify them to use -buildconf. I have kept it rebased over the years in my github repo². ¹http://ffmpeg.org/pipermail/ffmpeg-devel/2013-November/151075.html ²https://github.com/qyot27/FFmpeg/commit/ce087ae0b72cf70fff29300b528122b7451cb4b7 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
On 2021-08-06 11:34 pm, James Almer wrote: From: Matthieu Patou Suggested-by: ffm...@fb.com Signed-off-by: James Almer --- fftools/cmdutils.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index 912e881174..fc58277df7 100644 --- a/fftools/cmdutils.c +++ b/fftools/cmdutils.c @@ -1160,7 +1160,8 @@ 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); +if (flags & SHOW_CONFIG) +av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n", indent); I like it. If most users pasted the full log on their own initiative in bug reports or Q then this would have been a negative but they don't. I suggest to add an else clause that prints something like "Add -buildconf to view configuration". Regards, Gyan ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
Am Sa., 7. Aug. 2021 um 18:14 Uhr schrieb James Almer : > > On 8/7/2021 1:06 PM, Derek Buitenhuis wrote: > > On 8/6/2021 7:04 PM, James Almer wrote: > >> From: Matthieu Patou > >> > >> Suggested-by: ffm...@fb.com > >> Signed-off-by: James Almer > >> --- > >> fftools/cmdutils.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > > > > What exactly is the point of this? > > > > You've provided a patch with no explanation for its value. > > > > If it's "security", then that's kind of dumb, since if you > > have access to run ffmpeg on something, you have access to > > `strings $(which ffmpeg)` too. > > It's just to reduce the amount of information printed by default. Most > command line utilities don't bother showing build time configuration > options outside of --version or --help output, if at all. > And the string is obviously in the binary (duplicated on every library > at that), so it's not about hiding anything. > > I made the latest version print it on verbose level or higher (and > always with --version), which may be preferable. But i nonetheless > don't have a strong opinion about this change. FWIW, the change is a very bad idea imo. Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
On 8/7/2021 1:17 PM, Derek Buitenhuis wrote: On 8/7/2021 5:13 PM, James Almer wrote: It's just to reduce the amount of information printed by default. Most command line utilities don't bother showing build time configuration options outside of --version or --help output, if at all. And the string is obviously in the binary (duplicated on every library at that), so it's not about hiding anything. I'm going to have to say this is actually negative value, and say I dislike it. I find it quite useful to print by default, in terms of debugging, finding issues in logs, etc. It also means (as noted elsewhere) that bug reports no longer include this info by default. Bug reports are usually required to use debug loglevel, so they should not be affected with the latest version. So, I'm going to have to say this is not a good enough reason, in my opinion. Alright. I made the latest version print it on verbose level or higher (and always with --version), which may be preferable. But i nonetheless don't have a strong opinion about this change. See above. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
On 8/7/2021 5:13 PM, James Almer wrote: > It's just to reduce the amount of information printed by default. Most > command line utilities don't bother showing build time configuration > options outside of --version or --help output, if at all. > And the string is obviously in the binary (duplicated on every library > at that), so it's not about hiding anything. I'm going to have to say this is actually negative value, and say I dislike it. I find it quite useful to print by default, in terms of debugging, finding issues in logs, etc. It also means (as noted elsewhere) that bug reports no longer include this info by default. So, I'm going to have to say this is not a good enough reason, in my opinion. > I made the latest version print it on verbose level or higher (and > always with --version), which may be preferable. But i nonetheless don't > have a strong opinion about this change. See above. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
On 8/7/2021 1:06 PM, Derek Buitenhuis wrote: On 8/6/2021 7:04 PM, James Almer wrote: From: Matthieu Patou Suggested-by: ffm...@fb.com Signed-off-by: James Almer --- fftools/cmdutils.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) What exactly is the point of this? You've provided a patch with no explanation for its value. If it's "security", then that's kind of dumb, since if you have access to run ffmpeg on something, you have access to `strings $(which ffmpeg)` too. It's just to reduce the amount of information printed by default. Most command line utilities don't bother showing build time configuration options outside of --version or --help output, if at all. And the string is obviously in the binary (duplicated on every library at that), so it's not about hiding anything. I made the latest version print it on verbose level or higher (and always with --version), which may be preferable. But i nonetheless don't have a strong opinion about this change. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
On 8/6/2021 7:04 PM, James Almer wrote: > From: Matthieu Patou > > Suggested-by: ffm...@fb.com > Signed-off-by: James Almer > --- > fftools/cmdutils.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) What exactly is the point of this? You've provided a patch with no explanation for its value. If it's "security", then that's kind of dumb, since if you have access to run ffmpeg on something, you have access to `strings $(which ffmpeg)` too. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
> -Original Message-
> From: ffmpeg-devel On Behalf Of
> James Almer
> Sent: Friday, 6 August 2021 20:27
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build
> configuration by default
>
> On 8/6/2021 3:08 PM, Andreas Rheinhardt wrote:
> > James Almer:
> >> From: Matthieu Patou
> >>
> >> Suggested-by: ffm...@fb.com
> >> Signed-off-by: James Almer
> >> ---
> >> fftools/cmdutils.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c index
> >> 912e881174..fc58277df7 100644
> >> --- a/fftools/cmdutils.c
> >> +++ b/fftools/cmdutils.c
> >> @@ -1160,7 +1160,8 @@ 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);
> >> +if (flags & SHOW_CONFIG)
> >> +av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION
> >> + "\n", indent);
> >> }
> >>
> >> static void print_buildconf(int flags, int level) @@ -1203,7
> >> +1204,7 @@ void show_banner(int argc, char **argv, const OptionDef
> *options)
> >> 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);
> >> +print_program_info (SHOW_COPYRIGHT|SHOW_CONFIG,
> AV_LOG_INFO);
> >> print_all_libs_info(SHOW_VERSION, AV_LOG_INFO);
> >>
> >> return 0;
> >>
> > This ensures that the configuration will be missing from most bugreports.
>
> Yes, that was my main concern as well. But ffmpeg -version will still show the
> build configuration, in any case.
In my fork, I have a similar change, but I'm also printing the build config when
loglevel >= VERBOSE. This largely addresses the bugreport concern as for those
cases it will usually require a higher loglevel than INFO anyway.
softworkz
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
On 8/6/2021 3:08 PM, Andreas Rheinhardt wrote:
James Almer:
From: Matthieu Patou
Suggested-by: ffm...@fb.com
Signed-off-by: James Almer
---
fftools/cmdutils.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
index 912e881174..fc58277df7 100644
--- a/fftools/cmdutils.c
+++ b/fftools/cmdutils.c
@@ -1160,7 +1160,8 @@ 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);
+if (flags & SHOW_CONFIG)
+av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n",
indent);
}
static void print_buildconf(int flags, int level)
@@ -1203,7 +1204,7 @@ void show_banner(int argc, char **argv, const OptionDef
*options)
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);
+print_program_info (SHOW_COPYRIGHT|SHOW_CONFIG, AV_LOG_INFO);
print_all_libs_info(SHOW_VERSION, AV_LOG_INFO);
return 0;
This ensures that the configuration will be missing from most bugreports.
Yes, that was my main concern as well. But ffmpeg -version will still
show the build configuration, in any case.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
James Almer:
> From: Matthieu Patou
>
> Suggested-by: ffm...@fb.com
> Signed-off-by: James Almer
> ---
> fftools/cmdutils.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
> index 912e881174..fc58277df7 100644
> --- a/fftools/cmdutils.c
> +++ b/fftools/cmdutils.c
> @@ -1160,7 +1160,8 @@ 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);
> +if (flags & SHOW_CONFIG)
> +av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n",
> indent);
> }
>
> static void print_buildconf(int flags, int level)
> @@ -1203,7 +1204,7 @@ void show_banner(int argc, char **argv, const OptionDef
> *options)
> 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);
> +print_program_info (SHOW_COPYRIGHT|SHOW_CONFIG, AV_LOG_INFO);
> print_all_libs_info(SHOW_VERSION, AV_LOG_INFO);
>
> return 0;
>
This ensures that the configuration will be missing from most bugreports.
- Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH] fftools/cmdutils: don't print build configuration by default
From: Matthieu Patou
Suggested-by: ffm...@fb.com
Signed-off-by: James Almer
---
fftools/cmdutils.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
index 912e881174..fc58277df7 100644
--- a/fftools/cmdutils.c
+++ b/fftools/cmdutils.c
@@ -1160,7 +1160,8 @@ 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);
+if (flags & SHOW_CONFIG)
+av_log(NULL, level, "%sconfiguration: " FFMPEG_CONFIGURATION "\n",
indent);
}
static void print_buildconf(int flags, int level)
@@ -1203,7 +1204,7 @@ void show_banner(int argc, char **argv, const OptionDef
*options)
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);
+print_program_info (SHOW_COPYRIGHT|SHOW_CONFIG, AV_LOG_INFO);
print_all_libs_info(SHOW_VERSION, AV_LOG_INFO);
return 0;
--
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-requ...@ffmpeg.org with subject "unsubscribe".
