| Message ID | 20180721183112.193543-1-gorzel@google.com |
|---|---|
| State | Superseded |
| Headers | show |
On 21.07.2018 20:31, Marcin Gorzel wrote: > Rematrixing supports up to 64 channels. However, there is only a limited number of channel layouts defined. Since the in/out channel count is currently obtained from the channel layout, for undefined layouts (e.g. for 9, 10, 11 channels etc.) the rematrixing fails. > > This patch changes rematrix init methods to use in/out channel count directly instead of computing it from channel layout. > --- > libswresample/rematrix.c | 4 ++-- > libswresample/x86/rematrix_init.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > index 8227730056..ec1909dc0c 100644 > --- a/libswresample/rematrix.c > +++ b/libswresample/rematrix.c > @@ -384,8 +384,8 @@ av_cold static int auto_matrix(SwrContext *s) > > av_cold int swri_rematrix_init(SwrContext *s){ > int i, j; > - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > + int nb_in = s->in.ch_count; > + int nb_out = s->out.ch_count; > > s->mix_any_f = NULL; > > diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c > index d71b41a73e..1cdf97803f 100644 > --- a/libswresample/x86/rematrix_init.c > +++ b/libswresample/x86/rematrix_init.c > @@ -33,8 +33,8 @@ D(int16, sse2) > av_cold int swri_rematrix_init_x86(struct SwrContext *s){ > #if HAVE_X86ASM > int mm_flags = av_get_cpu_flags(); > - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > + int nb_in = s->in.ch_count; > + int nb_out = s->out.ch_count; > int num = nb_in * nb_out; > int i,j; > > Patch looks good to me. Will leave it to Michael to comment/apply the patch. Regards, Tobias
On Sat, Jul 21, 2018 at 07:31:12PM +0100, Marcin Gorzel wrote: > Rematrixing supports up to 64 channels. However, there is only a limited number of channel layouts defined. Since the in/out channel count is currently obtained from the channel layout, for undefined layouts (e.g. for 9, 10, 11 channels etc.) the rematrixing fails. > > This patch changes rematrix init methods to use in/out channel count directly instead of computing it from channel layout. > --- > libswresample/rematrix.c | 4 ++-- > libswresample/x86/rematrix_init.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > index 8227730056..ec1909dc0c 100644 > --- a/libswresample/rematrix.c > +++ b/libswresample/rematrix.c > @@ -384,8 +384,8 @@ av_cold static int auto_matrix(SwrContext *s) > > av_cold int swri_rematrix_init(SwrContext *s){ > int i, j; > - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > + int nb_in = s->in.ch_count; > + int nb_out = s->out.ch_count; > > s->mix_any_f = NULL; This is much better than the previous patch still, why is this not using used_ch_count ? Lets look at the code in swresample.c, as i dont remember 100% ... First stage is in_convert, its initialized with: s->in_convert = swri_audio_convert_alloc(s->int_sample_fmt, s-> in_sample_fmt, s->used_ch_count, s->channel_map, 0); You can see here that used_ch_count is its output and next stages are resample and rematrix (in either order) how can they have s->in.ch_count as input ? and yes, it seems there is no testcase that catches this, which is unfortunate. a much more minor issue is, please vertically align the "=" as it was before the patch. Thanks [...]
On Mon, Jul 23, 2018 at 7:52 AM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Sat, Jul 21, 2018 at 07:31:12PM +0100, Marcin Gorzel wrote: > > Rematrixing supports up to 64 channels. However, there is only a limited > number of channel layouts defined. Since the in/out channel count is > currently obtained from the channel layout, for undefined layouts (e.g. for > 9, 10, 11 channels etc.) the rematrixing fails. > > > > This patch changes rematrix init methods to use in/out channel count > directly instead of computing it from channel layout. > > --- > > libswresample/rematrix.c | 4 ++-- > > libswresample/x86/rematrix_init.c | 4 ++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > > index 8227730056..ec1909dc0c 100644 > > --- a/libswresample/rematrix.c > > +++ b/libswresample/rematrix.c > > @@ -384,8 +384,8 @@ av_cold static int auto_matrix(SwrContext *s) > > > > av_cold int swri_rematrix_init(SwrContext *s){ > > int i, j; > > - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > > - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > > + int nb_in = s->in.ch_count; > > + int nb_out = s->out.ch_count; > > > > s->mix_any_f = NULL; > > This is much better than the previous patch > still, why is this not using used_ch_count ? > > Lets look at the code in swresample.c, as i dont remember 100% ... > First stage is in_convert, its initialized with: > s->in_convert = swri_audio_convert_alloc(s->int_sample_fmt, > s-> in_sample_fmt, > s->used_ch_count, s->channel_map, 0); > > You can see here that used_ch_count is its output > and next stages are resample and rematrix (in either order) > > how can they have s->in.ch_count as input ? > > and yes, it seems there is no testcase that catches this, which is > unfortunate. > I think you are right, looking at the swresample.c again, used_ch_count is initially obtained from the user_used_ch_count. In the case user_used_ch_count is not set (default: 0) used_ch_count will take the value of in.ch_count. I've changed that. Also, OOC, shouldn't used_ch_count be used for s->resample_first in swresample.c:321 as well in this case? a much more minor issue is, please vertically align the "=" as it was > before the patch. > Done. Thanks > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The educated differ from the uneducated as much as the living from the > dead. -- Aristotle > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Tue, Jul 24, 2018 at 06:55:08AM -0700, Marcin Gorzel wrote: > On Mon, Jul 23, 2018 at 7:52 AM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Sat, Jul 21, 2018 at 07:31:12PM +0100, Marcin Gorzel wrote: > > > Rematrixing supports up to 64 channels. However, there is only a limited > > number of channel layouts defined. Since the in/out channel count is > > currently obtained from the channel layout, for undefined layouts (e.g. for > > 9, 10, 11 channels etc.) the rematrixing fails. > > > > > > This patch changes rematrix init methods to use in/out channel count > > directly instead of computing it from channel layout. > > > --- > > > libswresample/rematrix.c | 4 ++-- > > > libswresample/x86/rematrix_init.c | 4 ++-- > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > > > index 8227730056..ec1909dc0c 100644 > > > --- a/libswresample/rematrix.c > > > +++ b/libswresample/rematrix.c > > > @@ -384,8 +384,8 @@ av_cold static int auto_matrix(SwrContext *s) > > > > > > av_cold int swri_rematrix_init(SwrContext *s){ > > > int i, j; > > > - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); > > > - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); > > > + int nb_in = s->in.ch_count; > > > + int nb_out = s->out.ch_count; > > > > > > s->mix_any_f = NULL; > > > > This is much better than the previous patch > > still, why is this not using used_ch_count ? > > > > Lets look at the code in swresample.c, as i dont remember 100% ... > > First stage is in_convert, its initialized with: > > s->in_convert = swri_audio_convert_alloc(s->int_sample_fmt, > > s-> in_sample_fmt, > > s->used_ch_count, s->channel_map, 0); > > > > You can see here that used_ch_count is its output > > and next stages are resample and rematrix (in either order) > > > > how can they have s->in.ch_count as input ? > > > > and yes, it seems there is no testcase that catches this, which is > > unfortunate. > > > > I think you are right, looking at the swresample.c again, used_ch_count is > initially obtained from the user_used_ch_count. In the case > user_used_ch_count is not set (default: 0) used_ch_count will take the > value of in.ch_count. I've changed that. > > Also, OOC, shouldn't used_ch_count be used for s->resample_first in > swresample.c:321 as well in this case? yes, that looks wrong, patch posted for that thanks [...]
diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c index 8227730056..ec1909dc0c 100644 --- a/libswresample/rematrix.c +++ b/libswresample/rematrix.c @@ -384,8 +384,8 @@ av_cold static int auto_matrix(SwrContext *s) av_cold int swri_rematrix_init(SwrContext *s){ int i, j; - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); + int nb_in = s->in.ch_count; + int nb_out = s->out.ch_count; s->mix_any_f = NULL; diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c index d71b41a73e..1cdf97803f 100644 --- a/libswresample/x86/rematrix_init.c +++ b/libswresample/x86/rematrix_init.c @@ -33,8 +33,8 @@ D(int16, sse2) av_cold int swri_rematrix_init_x86(struct SwrContext *s){ #if HAVE_X86ASM int mm_flags = av_get_cpu_flags(); - int nb_in = av_get_channel_layout_nb_channels(s->in_ch_layout); - int nb_out = av_get_channel_layout_nb_channels(s->out_ch_layout); + int nb_in = s->in.ch_count; + int nb_out = s->out.ch_count; int num = nb_in * nb_out; int i,j;