diff mbox

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

Message ID 20180713114336.224847-1-gorzel@google.com
State Superseded
Headers show

Commit Message

Marcin Gorzel July 13, 2018, 11:43 a.m. UTC 
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(-)

Comments

Michael Niedermayer July 14, 2018, 3:01 p.m. UTC | #1 
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)

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.

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


[...]
Marcin Gorzel July 16, 2018, 2:37 p.m. UTC | #2 
Hi Michael,

On Sat, Jul 14, 2018 at 4:01 PM Michael Niedermayer <michael@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer July 16, 2018, 8:23 p.m. UTC | #3 
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@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.



[...]
Marcin Gorzel July 17, 2018, 11:35 a.m. UTC | #4 
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@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@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Tobias Rapp July 18, 2018, 2:23 p.m. UTC | #5 
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
Marcin Gorzel July 18, 2018, 3:22 p.m. UTC | #6 
Thanks for your input Tobias!

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

Patch

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