[FFmpeg-devel] [PATCH] libswresample: Use channel count if channel layout is undefined

[Marcin Gorzel]

Thanks for your input Tobias!

On Wed, Jul 18, 2018 at 3:23 PM Tobias Rapp <t.rapp at 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 |  gorzel at google.com |