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

2016-12-23 Thread Michael Niedermayer 
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

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2016-12-23 Thread Nicolas George 
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 
> ---
>  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 = 0xULL >> (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,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2016-12-23 Thread Michael Niedermayer 
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 ?

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2016-12-14 Thread Marton Balint 
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 
---
 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");
 for (i = 0; i < nb_ch; i++)
 s->route[i] = i;
-outlayout = av_get_default_channel_layout(nb_ch);
-if (!outlayout && nb_ch)
-outlayout = 0xULL >> (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
-- 
2.10.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel