diff mbox series

[FFmpeg-devel] ffprobe: Add option to allow unknown format private AVOptions

Message ID 20200626131500.526716-1-derek.buitenhuis@gmail.com
State New
Headers show
Series [FFmpeg-devel] ffprobe: Add option to allow unknown format private AVOptions
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Derek Buitenhuis June 26, 2020, 1:15 p.m. UTC
This useful, because by ffprobe's very nature, you use it to probe
a file and find out what it is. Requiring every format private option
to be known to the demuxer forces one to run ffprobe twice, if one
wants to use ffprobe in a generic way.

For example, say one wants to probe all user-uploaded files, while
also ignoring edit lists for any MP4s that are uploaded. Currently,
you'd have to run ffprobe twice: once to identify the format, and
once again to actually probe the metadata you want. After this
patch, you could set -ignore_editlist 1 on every call and only
probe once.

Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
---
 fftools/ffprobe.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Nicolas George June 26, 2020, 1:49 p.m. UTC | #1
Derek Buitenhuis (12020-06-26):
> This useful, because by ffprobe's very nature, you use it to probe
> a file and find out what it is. Requiring every format private option
> to be known to the demuxer forces one to run ffprobe twice, if one
> wants to use ffprobe in a generic way.
> 
> For example, say one wants to probe all user-uploaded files, while
> also ignoring edit lists for any MP4s that are uploaded. Currently,
> you'd have to run ffprobe twice: once to identify the format, and
> once again to actually probe the metadata you want. After this
> patch, you could set -ignore_editlist 1 on every call and only
> probe once.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
>  fftools/ffprobe.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Probably a good idea, but these explanation should probably go in
doc/ffprobe.texi.

And I do not maintain ffprobe.

Regards,
Michael Niedermayer June 27, 2020, 8:27 a.m. UTC | #2
On Fri, Jun 26, 2020 at 02:15:00PM +0100, Derek Buitenhuis wrote:
> This useful, because by ffprobe's very nature, you use it to probe
> a file and find out what it is. Requiring every format private option
> to be known to the demuxer forces one to run ffprobe twice, if one
> wants to use ffprobe in a generic way.
> 
> For example, say one wants to probe all user-uploaded files, while
> also ignoring edit lists for any MP4s that are uploaded. Currently,
> you'd have to run ffprobe twice: once to identify the format, and
> once again to actually probe the metadata you want. After this
> patch, you could set -ignore_editlist 1 on every call and only
> probe once.
> 
> Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com>
> ---
>  fftools/ffprobe.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

You explain why allow_unknown_format_opts = 1 is needed

does allow_unknown_format_opts = 0 have a use case too ?
because if not, this can be simplified

thx

[...]
Derek Buitenhuis June 28, 2020, 12:22 p.m. UTC | #3
On 26/06/2020 14:49, Nicolas George wrote:
> Probably a good idea, but these explanation should probably go in
> doc/ffprobe.texi.

Good point. Will add that during when I send v2.

> And I do not maintain ffprobe.

I have not seen Stefano active in a long time. Do you suggest I CC him on the v2 patch?

I always thought CCing people on patches was a bit rude, so I haven't.

- Derek
Derek Buitenhuis June 28, 2020, 12:24 p.m. UTC | #4
On 27/06/2020 09:27, Michael Niedermayer wrote:
> You explain why allow_unknown_format_opts = 1 is needed
> 
> does allow_unknown_format_opts = 0 have a use case too ?
> because if not, this can be simplified

I thought so too, but I thought a change in the behavior like that
might break peoples' scripts, etc.

I can move it to be the only behavior if you wish.

Up to you guys.

- Derke
Michael Niedermayer June 28, 2020, 6:41 p.m. UTC | #5
On Sun, Jun 28, 2020 at 01:22:58PM +0100, Derek Buitenhuis wrote:
> On 26/06/2020 14:49, Nicolas George wrote:
> > Probably a good idea, but these explanation should probably go in
> > doc/ffprobe.texi.
> 
> Good point. Will add that during when I send v2.
> 
> > And I do not maintain ffprobe.
> 
> I have not seen Stefano active in a long time. Do you suggest I CC him on the v2 patch?
> 
> I always thought CCing people on patches was a bit rude, so I haven't.

iam not who that question was directed to but i think ffprobe needs a
new maintainer as Stefano seems (as you mention) not active ATM.
looking at ffprobe git history, it seemed both myself and marton had
applied patches from people other than themselfs to ffprobe "recently" so
they did some ffprobe maintaince work.
if marton wants to maintain ffprobe, i think noone would mind otherwise
if not and noone else wants to fill that spot, i guess ffprobe is 
"community maintained" ATM.

marton ?

Thanks

[...]
Nicolas George June 28, 2020, 7:41 p.m. UTC | #6
Derek Buitenhuis (12020-06-28):
> I have not seen Stefano active in a long time. Do you suggest I CC him on the v2 patch?

I was just emphasizing that my comment did not have the weight of
maintainership.

Stefano does not seem very interested, probably not very useful to Cc
him.

Regards,
Marton Balint June 28, 2020, 8:23 p.m. UTC | #7
On Sun, 28 Jun 2020, Michael Niedermayer wrote:

> On Sun, Jun 28, 2020 at 01:22:58PM +0100, Derek Buitenhuis wrote:
>> On 26/06/2020 14:49, Nicolas George wrote:
>>> Probably a good idea, but these explanation should probably go in
>>> doc/ffprobe.texi.
>>
>> Good point. Will add that during when I send v2.
>>
>>> And I do not maintain ffprobe.
>>
>> I have not seen Stefano active in a long time. Do you suggest I CC him on the v2 patch?
>>
>> I always thought CCing people on patches was a bit rude, so I haven't.
>
> iam not who that question was directed to but i think ffprobe needs a
> new maintainer as Stefano seems (as you mention) not active ATM.
> looking at ffprobe git history, it seemed both myself and marton had
> applied patches from people other than themselfs to ffprobe "recently" so
> they did some ffprobe maintaince work.
> if marton wants to maintain ffprobe, i think noone would mind otherwise
> if not and noone else wants to fill that spot, i guess ffprobe is
> "community maintained" ATM.
>
> marton ?

I try to keep track of most ffprobe changes, so fine by me if people 
agree.

Regards,
Marton
Derek Buitenhuis June 28, 2020, 8:44 p.m. UTC | #8
On 28/06/2020 21:23, Marton Balint wrote:
> I try to keep track of most ffprobe changes, so fine by me if people 
> agree.

No argument from me.

- Derek
Alexander Strasser June 28, 2020, 8:52 p.m. UTC | #9
On 2020-06-28 22:23 +0200, Marton Balint wrote:
>
>
> On Sun, 28 Jun 2020, Michael Niedermayer wrote:
>
> > On Sun, Jun 28, 2020 at 01:22:58PM +0100, Derek Buitenhuis wrote:
> > > On 26/06/2020 14:49, Nicolas George wrote:
> > > > Probably a good idea, but these explanation should probably go in
> > > > doc/ffprobe.texi.
> > >
> > > Good point. Will add that during when I send v2.
> > >
> > > > And I do not maintain ffprobe.
> > >
> > > I have not seen Stefano active in a long time. Do you suggest I CC him on the v2 patch?
> > >
> > > I always thought CCing people on patches was a bit rude, so I haven't.
> >
> > iam not who that question was directed to but i think ffprobe needs a
> > new maintainer as Stefano seems (as you mention) not active ATM.
> > looking at ffprobe git history, it seemed both myself and marton had
> > applied patches from people other than themselfs to ffprobe "recently" so
> > they did some ffprobe maintaince work.
> > if marton wants to maintain ffprobe, i think noone would mind otherwise
> > if not and noone else wants to fill that spot, i guess ffprobe is
> > "community maintained" ATM.
> >
> > marton ?
>
> I try to keep track of most ffprobe changes, so fine by me if people agree.

No disagreement here.

If you are going forward with this, I think communication with Stefano
would be a good idea. Not sure what others think, but if someone is
listed as a maintainer I think contacting him about those areas is fine.


  Alexander
mypopy@gmail.com June 29, 2020, 5:11 a.m. UTC | #10
On Mon, Jun 29, 2020 at 4:52 AM Alexander Strasser <eclipse7@gmx.net> wrote:
>
> On 2020-06-28 22:23 +0200, Marton Balint wrote:
> >
> >
> > On Sun, 28 Jun 2020, Michael Niedermayer wrote:
> >
> > > On Sun, Jun 28, 2020 at 01:22:58PM +0100, Derek Buitenhuis wrote:
> > > > On 26/06/2020 14:49, Nicolas George wrote:
> > > > > Probably a good idea, but these explanation should probably go in
> > > > > doc/ffprobe.texi.
> > > >
> > > > Good point. Will add that during when I send v2.
> > > >
> > > > > And I do not maintain ffprobe.
> > > >
> > > > I have not seen Stefano active in a long time. Do you suggest I CC him on the v2 patch?
> > > >
> > > > I always thought CCing people on patches was a bit rude, so I haven't.
> > >
> > > iam not who that question was directed to but i think ffprobe needs a
> > > new maintainer as Stefano seems (as you mention) not active ATM.
> > > looking at ffprobe git history, it seemed both myself and marton had
> > > applied patches from people other than themselfs to ffprobe "recently" so
> > > they did some ffprobe maintaince work.
> > > if marton wants to maintain ffprobe, i think noone would mind otherwise
> > > if not and noone else wants to fill that spot, i guess ffprobe is
> > > "community maintained" ATM.
> > >
> > > marton ?
> >
> > I try to keep track of most ffprobe changes, so fine by me if people agree.
>
> No disagreement here.
>
> If you are going forward with this, I think communication with Stefano
> would be a good idea. Not sure what others think, but if someone is
> listed as a maintainer I think contacting him about those areas is fine.
>
No disagreement here.  I think it is better contact the Stefano before we have
new ffprobe maintainer.
diff mbox series

Patch

diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index 5515e1b31b..cf66e5b0a2 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -115,6 +115,7 @@  static int use_value_prefix             = 0;
 static int use_byte_value_binary_prefix = 0;
 static int use_value_sexagesimal_format = 0;
 static int show_private_data            = 1;
+static int allow_unknown_format_opts    = 0;
 
 static char *print_format;
 static char *stream_specifier;
@@ -2880,8 +2881,12 @@  static int open_input_file(InputFile *ifile, const char *filename,
     if (scan_all_pmts_set)
         av_dict_set(&format_opts, "scan_all_pmts", NULL, AV_DICT_MATCH_CASE);
     if ((t = av_dict_get(format_opts, "", NULL, AV_DICT_IGNORE_SUFFIX))) {
-        av_log(NULL, AV_LOG_ERROR, "Option %s not found.\n", t->key);
-        return AVERROR_OPTION_NOT_FOUND;
+        if (allow_unknown_format_opts) {
+            av_log(NULL, AV_LOG_WARNING, "Option %s skipped - not known to demuxer.\n", t->key);
+        } else {
+            av_log(NULL, AV_LOG_ERROR, "Option %s not found.\n", t->key);
+            return AVERROR_OPTION_NOT_FOUND;
+        }
     }
 
     if (find_stream_info) {
@@ -3573,6 +3578,8 @@  static const OptionDef real_options[] = {
     { "print_filename", HAS_ARG, {.func_arg = opt_print_filename}, "override the printed input filename", "print_file"},
     { "find_stream_info", OPT_BOOL | OPT_INPUT | OPT_EXPERT, { &find_stream_info },
         "read and decode the streams to fill missing information with heuristics" },
+    { "allow_unknown_format_opts", OPT_BOOL, { &allow_unknown_format_opts },
+        "allow unknown format options to be set without failing" },
     { NULL, },
 };