Thanks for your input Tobias! On Wed, Jul 18, 2018 at 3:23 PM Tobias Rapp <t.r...@noa-archive.com> wrote:
> On 13.07.2018 13:43, 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 > obtained from the channel layout, for undefined layouts (e.g. for 9, 10, 11 > channels etc.) the rematrixing fails. > > > > In ticket #6790 the problem has been partially addressed by applying a > patch to swr_set_matrix() in rematrix.c:72. > > However, a similar change must be also applied to swri_rematrix_init() > in rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36. > > > > This patch adds the following check to the swri_rematrix_init() in > rematrix.c:389 and swri_rematrix_init_x86() in rematrix_init.c:36: if > channel layout is non-zero, obtain channel count from the channel layout, > otherwise, use channel count instead. > > > > It also modifies the checks in swr_set_matrix() in rematrix.c:72 to > match the above checks. (Since > av_get_channel_layout_nb_channels(s->user_in_ch_layout) was originally > used, there may be preference to rely on the channel layout first (if > available) before falling back to the user channel count). > > --- > > libswresample/rematrix.c | 18 ++++++++++++------ > > libswresample/x86/rematrix_init.c | 8 ++++++-- > > 2 files changed, 18 insertions(+), 8 deletions(-) > > > > diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c > > index 8227730056..8c9fbf3804 100644 > > --- a/libswresample/rematrix.c > > +++ b/libswresample/rematrix.c > > @@ -69,10 +69,12 @@ int swr_set_matrix(struct SwrContext *s, const > double *matrix, int stride) > > return AVERROR(EINVAL); > > memset(s->matrix, 0, sizeof(s->matrix)); > > memset(s->matrix_flt, 0, sizeof(s->matrix_flt)); > > - nb_in = (s->user_in_ch_count > 0) ? s->user_in_ch_count : > > - av_get_channel_layout_nb_channels(s->user_in_ch_layout); > > - nb_out = (s->user_out_ch_count > 0) ? s->user_out_ch_count : > > - av_get_channel_layout_nb_channels(s->user_out_ch_layout); > > + nb_in = s->user_in_ch_layout != 0 > > + ? av_get_channel_layout_nb_channels(s->user_in_ch_layout) > > + : s->user_in_ch_count; > > + nb_out = s->user_out_ch_layout != 0 > > + ? av_get_channel_layout_nb_channels(s->user_out_ch_layout) > > + : s->user_out_ch_count; > > for (out = 0; out < nb_out; out++) { > > for (in = 0; in < nb_in; in++) > > s->matrix_flt[out][in] = s->matrix[out][in] = matrix[in]; > > Sorry for jumping into the discussion late but I don't think this change > is necessary. If only one of the user_*_ch_count / user_*_ch_layout > field pair is set, the outcome will be the same. If both field values > are set they must be consistent or undefined behavior will occur. So if > we assume the field values are consistent, why use the value that has to > be calculated first from the layout mask instead of the explicit count > value? > Makes sense. I am new to this code and I wasn't sure what historical reasons were behind using av_get_channel_layout_nb_channels(layout) to get the channel count in the first place so I thought it would be safer to leave the code so that it is used first, before falling back to the channel count. > @@ -384,8 +386,12 @@ 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_layout != 0 > > + ? av_get_channel_layout_nb_channels(s->in_ch_layout) > > + : s->user_in_ch_count; > > + int nb_out = s->out_ch_layout != 0 > > + ? av_get_channel_layout_nb_channels(s->out_ch_layout) > > + : s->user_out_ch_count; > > > > s->mix_any_f = NULL; > > > > diff --git a/libswresample/x86/rematrix_init.c > b/libswresample/x86/rematrix_init.c > > index d71b41a73e..a6ae074926 100644 > > --- a/libswresample/x86/rematrix_init.c > > +++ b/libswresample/x86/rematrix_init.c > > @@ -33,8 +33,12 @@ 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_layout != 0 > > + ? av_get_channel_layout_nb_channels(s->in_ch_layout) > > + : s->user_in_ch_count; > > + int nb_out = s->out_ch_layout != 0 > > + ? av_get_channel_layout_nb_channels(s->out_ch_layout) > > + : s->user_out_ch_count; > > int num = nb_in * nb_out; > > int i,j; > > > > > > Like said above I think the same effect can be achieved with less CPU > cycles by using a "(count > 0) ? count : > av_get_channel_layout_nb_channels(layout)" code pattern. > OK, happy to change that back in int swr_set_matrix() (just use in_ch_layout instead of user_in_ch_layout) and propagate a similar change to swri_rematrix_init() and swri_rematrix_init_x86(). Also not sure what is the difference between the "in_ch_layout" field > used here and the "user_in_ch_layout" field used in function > swr_set_matrix. > in_ch_layout is set from user_in_ch_layout in swresample.c:171. However, based on further checks in the init method, in_ch_layout can be modified so I think it should be used instead. Minor nit: In my personal opinion putting parentheses around the > condition part of the ternary operator would improve readability. > Will do! Regards, > Tobias > > -- Marcin Gorzel | Software Engineer | gor...@google.com | _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel