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

[Michael Niedermayer]

On Mon, Jul 16, 2018 at 03:37:15PM +0100, Marcin Gorzel wrote:
> 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)

If the number of channels exceed the maximum you should not reach this code.
nothing in your context can ever be out of range because
you never would get beyond checking the users parameters. Such check would
fail and nothing would use the parameters after that.

So you should never reach any check that could use an alternative
Which is what i meant, "why do we need a check here"

This is effectivly saying that the channel count (when the correct field is
used) in the context isnt actually containing the channel count.

I hope this makes it more clear why this change doesnt look correct to me.


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

Adding an error message where one is missing is probably a good idea.



[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180716/46250877/attachment.sig>