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

Marcin Gorzel gorzel at google.com
Tue Jul 17 14:35:18 EEST 2018


Hi Michael,

Of course we should not check in a change that is incorrect. So thank you
for your thorough review.
However, could you help me understand which part exactly of the below do
you think is incorrect?

Before:

int nb_in  = av_get_channel_layout_nb_channels(s->in_ch_layout);

This is clearly incorrect, since when s->in_ch_layout = 0, nb_in will be 0,
even if the audio has a valid number of channels (<64). So this is the bug
I'm trying to address.

After my change:

 int nb_in = s->in_ch_layout != 0
                  ? av_get_channel_layout_nb_channels(s->user_in_ch_layout)
                  :  s->user_in_ch_count;

In words, "check if the channel layout is valid first. If so, use it to
determine the channel count. If not, use the input channel count".

Could you point out which line of the above is incorrect? Are you
suggesting that this check is unnecessary in general and that we should just
use  the s->user_in_ch_count instead?
What was the reason the av_get_channel_layout_nb_channels(s->user_in_ch_layout)
is used here in the first place?

Also, could you help me understand, why this (approved and merged) change
is more correct (
https://github.com/FFmpeg/FFmpeg/commit/6325bd3717348615adafb52e4da2fd01a3007d0a#diff-5e2d16895082a305b95d127ece942c03
)?

Before:

nb_in  = av_get_channel_layout_nb_channels(s->user_in_ch_layout);

After:

nb_in = (s->user_in_ch_count > 0)
             ? s->user_in_ch_count
              : av_get_channel_layout_nb_channels(s->user_in_ch_layout);

Even if the s->user_in_ch_count fails the check, how is it assured
that the s->user_in_ch_layout
will be valid?

Thanks and apologies if there is some fundamental misunderstanding on my
side - as I said I am new to ffmpeg code but I would love to improve it by
eliminating the problem I've experienced.

Marcin


On Mon, Jul 16, 2018 at 9:23 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> 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
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>


-- 

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


More information about the ffmpeg-devel mailing list