| Message ID | 20200816113139.360344-1-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] ffmpeg: add disable_all_auto_conversion_filters option. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Sun, 16 Aug 2020, Nicolas George wrote: > Signed-off-by: Nicolas George <george@nsup.org> > --- > doc/ffmpeg.texi | 8 ++++++++ > fftools/ffmpeg.h | 1 + > fftools/ffmpeg_filter.c | 2 ++ > fftools/ffmpeg_opt.c | 3 +++ > 4 files changed, 14 insertions(+) > > diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi > index 267ddfe8b5..e38e55573e 100644 > --- a/doc/ffmpeg.texi > +++ b/doc/ffmpeg.texi > @@ -1746,6 +1746,14 @@ this buffer, in packets, for the matching output stream. > The default value of this option should be high enough for most uses, so only > touch this option if you are sure that you need it. > > +@item -disable_all_auto_conversion_filters (@emph{global}) Can we find a shorter name for the option? E.g. simply call the option auto_conversion_filters and the user can use -noauto_conversion_filters to disable it... In the next patch with the current name you have -nodisable_auto_coversion_filters which is really ugly IMHO. Regards, Marton > +Disable automatically inserting format conversion filters in all filter > +graphs, including those defined by @option{-vf}, @option{-af}, > +@option{-filter_complex} and @option{-lavfi}. If filter format negotiation > +requires a conversion, the initialization of the filters will fail. > +Conversions can still be performed by inserting the relevant conversion > +filter (scale, aresample) in the graph. > + > @end table > > As a special exception, you can use a bitmap subtitle stream as input: it > diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h > index 6e3f2545c7..acd297c8e8 100644 > --- a/fftools/ffmpeg.h > +++ b/fftools/ffmpeg.h > @@ -613,6 +613,7 @@ extern char *videotoolbox_pixfmt; > extern int filter_nbthreads; > extern int filter_complex_nbthreads; > extern int vstats_version; > +extern int disable_all_auto_conversion_filters; > > extern const AVIOInterruptCB int_cb; > > diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c > index 4784e8a575..8721403d7a 100644 > --- a/fftools/ffmpeg_filter.c > +++ b/fftools/ffmpeg_filter.c > @@ -1104,6 +1104,8 @@ int configure_filtergraph(FilterGraph *fg) > configure_output_filter(fg, fg->outputs[i], cur); > avfilter_inout_free(&outputs); > > + if (disable_all_auto_conversion_filters) > + avfilter_graph_set_auto_convert(fg->graph, AVFILTER_AUTO_CONVERT_NONE); > if ((ret = avfilter_graph_config(fg->graph, NULL)) < 0) > goto fail; > > diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c > index 853550a142..7c69cca10d 100644 > --- a/fftools/ffmpeg_opt.c > +++ b/fftools/ffmpeg_opt.c > @@ -172,6 +172,7 @@ float max_error_rate = 2.0/3; > int filter_nbthreads = 0; > int filter_complex_nbthreads = 0; > int vstats_version = 2; > +int disable_all_auto_conversion_filters = 0; > > > static int intra_only = 0; > @@ -3545,6 +3546,8 @@ const OptionDef options[] = { > "create a complex filtergraph", "graph_description" }, > { "filter_complex_script", HAS_ARG | OPT_EXPERT, { .func_arg = opt_filter_complex_script }, > "read complex filtergraph description from a file", "filename" }, > + { "disable_all_auto_conversion_filters", OPT_BOOL | OPT_EXPERT, { &disable_all_auto_conversion_filters }, > + "disable all automatic conversion filters" }, > { "stats", OPT_BOOL, { &print_stats }, > "print progress report during encoding", }, > { "attach", HAS_ARG | OPT_PERFILE | OPT_EXPERT | > -- > 2.28.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".
Marton Balint (12020-08-16): > Can we find a shorter name for the option? E.g. simply call the option > auto_conversion_filters and the user can use -noauto_conversion_filters to > disable it... In the next patch with the current name you have > -nodisable_auto_coversion_filters which is really ugly IMHO. As I explained there: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267954.html the very long name is on purpose, it is meant to express this option is only for rare technical use. I would prefer to stick with it if that is fine with the others. Regards,
On 8/16/20, Nicolas George <george@nsup.org> wrote: > Marton Balint (12020-08-16): >> Can we find a shorter name for the option? E.g. simply call the option >> auto_conversion_filters and the user can use -noauto_conversion_filters >> to >> disable it... In the next patch with the current name you have >> -nodisable_auto_coversion_filters which is really ugly IMHO. > > As I explained there: > > https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267954.html > > the very long name is on purpose, it is meant to express this option is > only for rare technical use. I would prefer to stick with it if that is > fine with the others. I do not like long name either.
Paul B Mahol (12020-08-16):
> I do not like long name either.
And do you have a better reason than the fact that it comes from me?
On 16-08-2020 09:57 pm, Nicolas George wrote: > Marton Balint (12020-08-16): >> Can we find a shorter name for the option? E.g. simply call the option >> auto_conversion_filters and the user can use -noauto_conversion_filters to >> disable it... In the next patch with the current name you have >> -nodisable_auto_coversion_filters which is really ugly IMHO. > As I explained there: > > https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267954.html > > the very long name is on purpose, it is meant to express this option is > only for rare technical use. I would prefer to stick with it if that is > fine with the others. Then make it a bit cryptic rather than verbose. e.g. an abbreviation. Gyan
Gyan Doshi (12020-08-16):
> Then make it a bit cryptic rather than verbose. e.g. an abbreviation.
Urgh, no. Making it cryptic would be hostile to people who read the
tests that use it. Cryptic is a very bad idea for almost everything.
Regards,
On 2020-08-16 18:39 +0200, Nicolas George wrote: > Paul B Mahol (12020-08-16): > > I do not like long name either. > > And do you have a better reason than the fact that it comes from me? This was uncalled for and not nice at all! It's neither good for you nor the community. There is nothing wrong with supporting another opinion like Paul did support the opinion voiced by Marton. Alexander
On 2020-08-16 18:27 +0200, Nicolas George wrote: > Marton Balint (12020-08-16): > > Can we find a shorter name for the option? E.g. simply call the option > > auto_conversion_filters and the user can use -noauto_conversion_filters to > > disable it... In the next patch with the current name you have > > -nodisable_auto_coversion_filters which is really ugly IMHO. I dislike the negative name too, because like mentioned by Marton it doesn't work well with overriding the option to turn it off. On one hand for this option in particular it wouldn't be that important, on the other hand it will be something (new) developers will see when writing tests and scratch their heads about it. > As I explained there: > > https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267954.html > > the very long name is on purpose, it is meant to express this option is > only for rare technical use. I would prefer to stick with it if that is > fine with the others. I'm not convinced that using the long name on purpose is good here or in general. 1. It would not be so great to have to invent convenient names for every option that shouldn't be used "normally" 2. If users want to use an option they will use it no matter how long the name. (This is from experience, we had a very longish and worse named option in MPlayer for a similar reason.) Alexander
Alexander Strasser (12020-08-16): > This was uncalled for and not nice at all! I will be correct, but I'm done being nice with him. > It's neither good for you nor the community. Do you honestly consider wrongs are balanced in this whole mess? Because if wrongs are not balanced, there is only one thing good for the community: saying it clearly and acting on it. > There is nothing wrong with supporting another opinion like Paul did > support the opinion voiced by Marton. There is nothing right, either, in replying to a message that gives reasons for exactly the same objection without giving new arguments. Case in point: you replied, but you gave new reasons. Your mail was useful. Regards,
Alexander Strasser (12020-08-16): > I dislike the negative name too, because like mentioned by Marton it > doesn't work well with overriding the option to turn it off. > > On one hand for this option in particular it wouldn't be that important, > on the other hand it will be something (new) developers will see when > writing tests and scratch their heads about it. But I want new developers writing tests to see it and scratch their head! I want to scratch their heads and find a better way if implementing their test. It is one of the points of the patch: find where tests are inefficients and give an incentive to make them more efficient. And I want reviewers to see the option, and make a comment about it; tests lines are frequently long, a short option is easy to overlook. > I'm not convinced that using the long name on purpose is good here > or in general. > > 1. It would not be so great to have to invent convenient names for > every option that shouldn't be used "normally" > 2. If users want to use an option they will use it no matter how > long the name. (This is from experience, we had a very longish > and worse named option in MPlayer for a similar reason.) At least, it will make Carl Eugen's work easier, or whoever deals with user questions on the mailing list somewhat easier: if somebody uses the option and break their command line cluelessly, it will be easy to spot, even in the middle of half a page of scale= arithmetic formulas and unrelated encoding options. I am not adamant on the name. If somebody suggests something better and there is a consensus, I will change it, of course. But I think these are good points for a very visible name. Regards,
Hi Nicolas! On 2020-08-16 23:03 +0200, Nicolas George wrote: > Alexander Strasser (12020-08-16): > > This was uncalled for and not nice at all! > > I will be correct, but I'm done being nice with him. It was an unneeded accusation. Thanks for trying to not repeat those. > > It's neither good for you nor the community. > > Do you honestly consider wrongs are balanced in this whole mess? Because > if wrongs are not balanced, there is only one thing good for the > community: saying it clearly and acting on it. I really don't have a score on this and I don't think it matters in this particular message I have sent. > > There is nothing wrong with supporting another opinion like Paul did > > support the opinion voiced by Marton. > > There is nothing right, either, in replying to a message that gives > reasons for exactly the same objection without giving new arguments. Yes. It's weaker than giving new arguments, but it shows there are more persons supporting the opinion of Marton or say not in favor of a long name. That's a bit of useful information. And naturally it's usually not required that every person reading this, voices their support of a particular opinion. Though in this case there weren't many voices yet, so it's not one of those cases. > Case in point: you replied, but you gave new reasons. Your mail was > useful. Alexander
On 2020-08-16 23:12 +0200, Nicolas George wrote: > Alexander Strasser (12020-08-16): > > I dislike the negative name too, because like mentioned by Marton it > > doesn't work well with overriding the option to turn it off. > > > > On one hand for this option in particular it wouldn't be that important, > > on the other hand it will be something (new) developers will see when > > writing tests and scratch their heads about it. > > But I want new developers writing tests to see it and scratch their > head! I want to scratch their heads and find a better way if > implementing their test. It is one of the points of the patch: find where > tests are inefficients and give an incentive to make them more > efficient. > > And I want reviewers to see the option, and make a comment about it; > tests lines are frequently long, a short option is easy to overlook. I need to differentiate here. I agree on adding an option to the tests where there are still autoconversions happening. I'm not in favor of adding an option with an unwieldy name and double negation (nodisable). I think the pendulum can swing in both direction here. So the overall effect is not clear to me. E.g. one developer may think "hey what's this -> i need to fix it" another might think "hey what's this -> better just copy and not look into it" and a third might think "hey what's this -> just another idiosyncrasy :(" > > I'm not convinced that using the long name on purpose is good here > > or in general. > > > > 1. It would not be so great to have to invent convenient names for > > every option that shouldn't be used "normally" > > 2. If users want to use an option they will use it no matter how > > long the name. (This is from experience, we had a very longish > > and worse named option in MPlayer for a similar reason.) > > At least, it will make Carl Eugen's work easier, or whoever deals with > user questions on the mailing list somewhat easier: if somebody uses the > option and break their command line cluelessly, it will be easy to spot, > even in the middle of half a page of scale= arithmetic formulas and > unrelated encoding options. Might be. I suspect it will not help much, but sure I might be wrong. I didn't do lots of bug wrangling in the last years. > I am not adamant on the name. If somebody suggests something better and > there is a consensus, I will change it, of course. But I think these are > good points for a very visible name. I'm neither insisting on anything. I like this patch set. Here are some suggestions in no particular order: * auto_conversion_filters (from Marton) * lavfi_auto_conversion * lavfi_autoconv * lavfi_sample_format_conversion * lavfi_fmt_conversion (in reference to pix_fmt and sample_fmt) * lavfi_fmt_conv Best regards, Alexander
On 8/17/20, Alexander Strasser <eclipse7@gmx.net> wrote: > On 2020-08-16 23:12 +0200, Nicolas George wrote: >> Alexander Strasser (12020-08-16): >> > I dislike the negative name too, because like mentioned by Marton it >> > doesn't work well with overriding the option to turn it off. >> > >> > On one hand for this option in particular it wouldn't be that important, >> > on the other hand it will be something (new) developers will see when >> > writing tests and scratch their heads about it. >> >> But I want new developers writing tests to see it and scratch their >> head! I want to scratch their heads and find a better way if >> implementing their test. It is one of the points of the patch: find where >> tests are inefficients and give an incentive to make them more >> efficient. >> >> And I want reviewers to see the option, and make a comment about it; >> tests lines are frequently long, a short option is easy to overlook. > > I need to differentiate here. I agree on adding an option to the tests > where there are still autoconversions happening. > > I'm not in favor of adding an option with an unwieldy name and double > negation (nodisable). > > I think the pendulum can swing in both direction here. So the overall > effect is not clear to me. E.g. one developer may think > > "hey what's this -> i need to fix it" > > another might think > > "hey what's this -> better just copy and not look into it" > > and a third might think > > "hey what's this -> just another idiosyncrasy :(" > > >> > I'm not convinced that using the long name on purpose is good here >> > or in general. >> > >> > 1. It would not be so great to have to invent convenient names for >> > every option that shouldn't be used "normally" >> > 2. If users want to use an option they will use it no matter how >> > long the name. (This is from experience, we had a very longish >> > and worse named option in MPlayer for a similar reason.) >> >> At least, it will make Carl Eugen's work easier, or whoever deals with >> user questions on the mailing list somewhat easier: if somebody uses the >> option and break their command line cluelessly, it will be easy to spot, >> even in the middle of half a page of scale= arithmetic formulas and >> unrelated encoding options. > > Might be. I suspect it will not help much, but sure I might be wrong. > I didn't do lots of bug wrangling in the last years. > > >> I am not adamant on the name. If somebody suggests something better and >> there is a consensus, I will change it, of course. But I think these are >> good points for a very visible name. > > I'm neither insisting on anything. I like this patch set. > > Here are some suggestions in no particular order: > > * auto_conversion_filters (from Marton) This is very good one. The double negation is making user and developers think about it several times upon reading. > * lavfi_auto_conversion > * lavfi_autoconv > * lavfi_sample_format_conversion > * lavfi_fmt_conversion (in reference to pix_fmt and sample_fmt) > * lavfi_fmt_conv > > > Best regards, > Alexander > _______________________________________________ > 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".
Alexander Strasser (12020-08-17): > I think the pendulum can swing in both direction here. So the overall > effect is not clear to me. E.g. one developer may think > > "hey what's this -> i need to fix it" > > another might think > > "hey what's this -> better just copy and not look into it" > > and a third might think > > "hey what's this -> just another idiosyncrasy :(" Fortunately, patches are not accepted without review: they will do that, the reviewer will ask them to fix the test and re-submit. > Here are some suggestions in no particular order: > > * auto_conversion_filters (from Marton) I can be ok with this one. I really dislike boolean options that default to yes and have to be disabled with no, because it requires remembering what the default is, but if that is what everybody else prefers. > * lavfi_auto_conversion > * lavfi_autoconv > * lavfi_sample_format_conversion > * lavfi_fmt_conversion (in reference to pix_fmt and sample_fmt) > * lavfi_fmt_conv The last three lack the "auto" bit. Explicit conversions are still supported. Regards,
On 2020-08-20 19:49 +0200, Nicolas George wrote: > Alexander Strasser (12020-08-17): > > I think the pendulum can swing in both direction here. So the overall > > effect is not clear to me. E.g. one developer may think > > > > "hey what's this -> i need to fix it" > > > > another might think > > > > "hey what's this -> better just copy and not look into it" > > > > and a third might think > > > > "hey what's this -> just another idiosyncrasy :(" > > Fortunately, patches are not accepted without review: they will do that, > the reviewer will ask them to fix the test and re-submit. True, that might work for the second developer, the third might get silently fed up. > > Here are some suggestions in no particular order: > > > > * auto_conversion_filters (from Marton) > > I can be ok with this one. I really dislike boolean options that default > to yes and have to be disabled with no, because it requires remembering > what the default is, but if that is what everybody else prefers. > > > * lavfi_auto_conversion > > * lavfi_autoconv > > * lavfi_sample_format_conversion > > * lavfi_fmt_conversion (in reference to pix_fmt and sample_fmt) > > * lavfi_fmt_conv > > The last three lack the "auto" bit. Explicit conversions are still > supported. I wonder if the auto (automatic) part is really needed for the name. AFAIU explicit conversions are just explicit use of filters which change the sample formats. I guess it would really far fetched to assume that those could be disabled, since it would be like disabling all filters or like allowing no conversions in the filter graph at all. I agree it's possible to misunderstand, though I would have said not very likely. After you said it out loud, it feels a little bit more likely to me now ;-) Alexander
diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi index 267ddfe8b5..e38e55573e 100644 --- a/doc/ffmpeg.texi +++ b/doc/ffmpeg.texi @@ -1746,6 +1746,14 @@ this buffer, in packets, for the matching output stream. The default value of this option should be high enough for most uses, so only touch this option if you are sure that you need it. +@item -disable_all_auto_conversion_filters (@emph{global}) +Disable automatically inserting format conversion filters in all filter +graphs, including those defined by @option{-vf}, @option{-af}, +@option{-filter_complex} and @option{-lavfi}. If filter format negotiation +requires a conversion, the initialization of the filters will fail. +Conversions can still be performed by inserting the relevant conversion +filter (scale, aresample) in the graph. + @end table As a special exception, you can use a bitmap subtitle stream as input: it diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h index 6e3f2545c7..acd297c8e8 100644 --- a/fftools/ffmpeg.h +++ b/fftools/ffmpeg.h @@ -613,6 +613,7 @@ extern char *videotoolbox_pixfmt; extern int filter_nbthreads; extern int filter_complex_nbthreads; extern int vstats_version; +extern int disable_all_auto_conversion_filters; extern const AVIOInterruptCB int_cb; diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c index 4784e8a575..8721403d7a 100644 --- a/fftools/ffmpeg_filter.c +++ b/fftools/ffmpeg_filter.c @@ -1104,6 +1104,8 @@ int configure_filtergraph(FilterGraph *fg) configure_output_filter(fg, fg->outputs[i], cur); avfilter_inout_free(&outputs); + if (disable_all_auto_conversion_filters) + avfilter_graph_set_auto_convert(fg->graph, AVFILTER_AUTO_CONVERT_NONE); if ((ret = avfilter_graph_config(fg->graph, NULL)) < 0) goto fail; diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c index 853550a142..7c69cca10d 100644 --- a/fftools/ffmpeg_opt.c +++ b/fftools/ffmpeg_opt.c @@ -172,6 +172,7 @@ float max_error_rate = 2.0/3; int filter_nbthreads = 0; int filter_complex_nbthreads = 0; int vstats_version = 2; +int disable_all_auto_conversion_filters = 0; static int intra_only = 0; @@ -3545,6 +3546,8 @@ const OptionDef options[] = { "create a complex filtergraph", "graph_description" }, { "filter_complex_script", HAS_ARG | OPT_EXPERT, { .func_arg = opt_filter_complex_script }, "read complex filtergraph description from a file", "filename" }, + { "disable_all_auto_conversion_filters", OPT_BOOL | OPT_EXPERT, { &disable_all_auto_conversion_filters }, + "disable all automatic conversion filters" }, { "stats", OPT_BOOL, { &print_stats }, "print progress report during encoding", }, { "attach", HAS_ARG | OPT_PERFILE | OPT_EXPERT |
Signed-off-by: Nicolas George <george@nsup.org> --- doc/ffmpeg.texi | 8 ++++++++ fftools/ffmpeg.h | 1 + fftools/ffmpeg_filter.c | 2 ++ fftools/ffmpeg_opt.c | 3 +++ 4 files changed, 14 insertions(+)