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

[Tobias Rapp]

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?

> @@ -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.

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.

Minor nit: In my personal opinion putting parentheses around the 
condition part of the ternary operator would improve readability.

Regards,
Tobias