| Message ID | 20161215033919.2870-2-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
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 ? [...]
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,
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 [...]
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
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(-)