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

[Marcin Gorzel]

Hi Michael,

On Sat, Jul 14, 2018 at 4:01 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Fri, Jul 13, 2018 at 12:43:36PM +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
> 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];
>
> > @@ -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;
>
> So this is the core of the change (the other hunk is a "duplicate" and one
> cosmetic)
>

Correct. I hope my corrected commit message makes it clearer now?


>
> The code after this uses C ? A : B;
> this implies that A is wrong in some cases and B is wrong in some cases
> you explained only one of these, that is that the layout is unable to
> represent some cases.
>

B can be wrong if the number of channels exceed the max. allowed value
(currently 64) in which case the re-matrixing fails with the 'invalid
parameter' error message.
As discussed earlier, I can add an error log to the ibavcodec (as a
separate patch) that will inform more clearly when the number of channels
is unsupported.


>
> 2nd question is, are these the ideal fields.
> shouldnt this use s->used_ch_count and s->out.ch_count?
>

I believe so:

s->out.ch_count is set from s-> user_out_ch_count anyway (in
swresample.c:167)
Also, I think s->used_ch_count is only used of input channel count.

These fields were also used in the original change here
<https://github.com/FFmpeg/FFmpeg/commit/6325bd3717348615adafb52e4da2fd01a3007d0a#diff-5e2d16895082a305b95d127ece942c03>
.


>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> "I am not trying to be anyone's saviour, I'm trying to think about the
>  future and not be sad" - Elon Musk
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


-- 

Marcin Gorzel |  Software Engineer |  gorzel at google.com |