[FFmpeg-devel,2/2] avfilter/af_amerge: do not guess an output channel layout

Submitted by Marton Balint on Dec. 15, 2016, 3:39 a.m.

Details

Message ID 20161215033919.2870-2-cus@passwd.hu
State New
Headers show

Commit Message

Marton Balint Dec. 15, 2016, 3:39 a.m.
This is the right thing to do, but I am afraid this will break too many
existing filter chains. How can we implement this properly? Ideas/options:

- change it, break it, users will fix it
- add a guess_output_layout option which will be true for now, false after a
  major bump, and mention this incompatible change in the next release
- add amerge2 filter, deprecate amerge filter

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavfilter/af_amerge.c      | 4 +---
 tests/ref/fate/filter-amerge | 4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)

Comments

Michael Niedermayer Dec. 23, 2016, 1:07 p.m.
On Thu, Dec 15, 2016 at 04:39:19AM +0100, Marton Balint wrote:
> This is the right thing to do, but I am afraid this will break too many
> existing filter chains. How can we implement this properly? Ideas/options:

do you have an example of a filter chain it would break ?

[...]
Nicolas George Dec. 23, 2016, 2:12 p.m.
Le quintidi 25 frimaire, an CCXXV, Marton Balint a écrit :
> This is the right thing to do, but I am afraid this will break too many
> existing filter chains. How can we implement this properly? Ideas/options:
> 
> - change it, break it, users will fix it
> - add a guess_output_layout option which will be true for now, false after a
>   major bump, and mention this incompatible change in the next release
> - add amerge2 filter, deprecate amerge filter

I do not like the option or the amerge2 solution, because that leaves
traces we can not get rid of.

My gut feeling wants me to say: there already was a warning, people who
ignore warnings deserve what they get. If you want to be extra careful
and are not in a hurry, you could reword the warning to notify that it
will change soon, wait some time (next release) and do the change.

> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavfilter/af_amerge.c      | 4 +---
>  tests/ref/fate/filter-amerge | 4 ++--
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/libavfilter/af_amerge.c b/libavfilter/af_amerge.c
> index 3bc7d89..43a2d95 100644
> --- a/libavfilter/af_amerge.c
> +++ b/libavfilter/af_amerge.c
> @@ -114,9 +114,7 @@ static int query_formats(AVFilterContext *ctx)

>                 "output layout will be determined by the number of distinct input channels\n");

I think the warning needs to be reworded anyway.

>          for (i = 0; i < nb_ch; i++)
>              s->route[i] = i;
> -        outlayout = av_get_default_channel_layout(nb_ch);
> -        if (!outlayout && nb_ch)
> -            outlayout = 0xFFFFFFFFFFFFFFFFULL >> (64 - nb_ch);
> +        outlayout = FF_COUNT2LAYOUT(nb_ch);
>      } else {
>          int *route[SWR_CH_MAX];
>          int c, out_ch_number = 0;
> diff --git a/tests/ref/fate/filter-amerge b/tests/ref/fate/filter-amerge
> index b3e5eb5..8118b4e 100644

> --- a/tests/ref/fate/filter-amerge
> +++ b/tests/ref/fate/filter-amerge
> @@ -2,8 +2,8 @@
>  #media_type 0: audio
>  #codec_id 0: pcm_s16le
>  #sample_rate 0: 44100
> -#channel_layout 0: 3
> -#channel_layout_name 0: stereo
> +#channel_layout 0: 0
> +#channel_layout_name 0: 2 channels

Michael: this is an example of filter graph that breaks: the output was
"stereo", it becomes "2 channels". Since there are
av_get_default_channel_layout() all over the place anyway, I think
something else would have caught it and hidden the difference for all
practical purposes.

Regards,
Michael Niedermayer Dec. 23, 2016, 3:48 p.m.
On Fri, Dec 23, 2016 at 03:12:26PM +0100, Nicolas George wrote:
> Le quintidi 25 frimaire, an CCXXV, Marton Balint a écrit :
[...]
> > --- a/tests/ref/fate/filter-amerge
> > +++ b/tests/ref/fate/filter-amerge
> > @@ -2,8 +2,8 @@
> >  #media_type 0: audio
> >  #codec_id 0: pcm_s16le
> >  #sample_rate 0: 44100
> > -#channel_layout 0: 3
> > -#channel_layout_name 0: stereo
> > +#channel_layout 0: 0
> > +#channel_layout_name 0: 2 channels
> 
> Michael: this is an example of filter graph that breaks: the output was
> "stereo", it becomes "2 channels". Since there are
> av_get_default_channel_layout() all over the place anyway, I think
> something else would have caught it and hidden the difference for all
> practical purposes.

I thought so too

thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavfilter/af_amerge.c b/libavfilter/af_amerge.c
index 3bc7d89..43a2d95 100644
--- a/libavfilter/af_amerge.c
+++ b/libavfilter/af_amerge.c
@@ -114,9 +114,7 @@  static int query_formats(AVFilterContext *ctx)
                "output layout will be determined by the number of distinct input channels\n");
         for (i = 0; i < nb_ch; i++)
             s->route[i] = i;
-        outlayout = av_get_default_channel_layout(nb_ch);
-        if (!outlayout && nb_ch)
-            outlayout = 0xFFFFFFFFFFFFFFFFULL >> (64 - nb_ch);
+        outlayout = FF_COUNT2LAYOUT(nb_ch);
     } else {
         int *route[SWR_CH_MAX];
         int c, out_ch_number = 0;
diff --git a/tests/ref/fate/filter-amerge b/tests/ref/fate/filter-amerge
index b3e5eb5..8118b4e 100644
--- a/tests/ref/fate/filter-amerge
+++ b/tests/ref/fate/filter-amerge
@@ -2,8 +2,8 @@ 
 #media_type 0: audio
 #codec_id 0: pcm_s16le
 #sample_rate 0: 44100
-#channel_layout 0: 3
-#channel_layout_name 0: stereo
+#channel_layout 0: 0
+#channel_layout_name 0: 2 channels
 0,          0,          0,     2048,     8192, 0x120efa65
 0,       2048,       2048,     2048,     8192, 0x7b3cebf7
 0,       4096,       4096,     2048,     8192, 0x0fb8ee01