diff mbox series

[FFmpeg-devel,1/2] ffmpeg: add disable_all_auto_conversion_filters option.

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.
Related show

Checks

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

Commit Message

Nicolas George Aug. 16, 2020, 11:31 a.m. UTC
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(+)

Comments

Marton Balint Aug. 16, 2020, 3:49 p.m. UTC | #1
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".
Nicolas George Aug. 16, 2020, 4:27 p.m. UTC | #2
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,
Paul B Mahol Aug. 16, 2020, 4:28 p.m. UTC | #3
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.
Nicolas George Aug. 16, 2020, 4:39 p.m. UTC | #4
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?
Gyan Doshi Aug. 16, 2020, 4:42 p.m. UTC | #5
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
Nicolas George Aug. 16, 2020, 4:43 p.m. UTC | #6
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,
Alexander Strasser Aug. 16, 2020, 7:30 p.m. UTC | #7
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
Alexander Strasser Aug. 16, 2020, 7:49 p.m. UTC | #8
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
Nicolas George Aug. 16, 2020, 9:03 p.m. UTC | #9
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,
Nicolas George Aug. 16, 2020, 9:12 p.m. UTC | #10
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,
Alexander Strasser Aug. 17, 2020, 8:26 a.m. UTC | #11
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
Alexander Strasser Aug. 17, 2020, 8:43 a.m. UTC | #12
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
Paul B Mahol Aug. 19, 2020, 6:22 p.m. UTC | #13
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".
Nicolas George Aug. 20, 2020, 5:49 p.m. UTC | #14
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,
Alexander Strasser Aug. 20, 2020, 9:43 p.m. UTC | #15
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 mbox series

Patch

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 |