diff mbox

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

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

Commit Message

Marcin Gorzel July 6, 2018, 2:15 p.m. UTC
Rematrixing supports up to 64 channels but there is only a limited number of channel layouts defined. Currently, in/out channel count is obtained from the channel layout so if the channel layout is undefined (e.g. for 9, 10, 11 channels etc.) the in/out channel count will be 0 and the rematrixing will fail. This change adds a check if the channel layout is non-zero, and if not, prefers to use the in|out_ch_count instead. This seems to be related to ticket #6790.
---
 libswresample/rematrix.c          | 18 ++++++++++++------
 libswresample/x86/rematrix_init.c |  8 ++++++--
 2 files changed, 18 insertions(+), 8 deletions(-)

Comments

Michael Niedermayer July 7, 2018, 1:04 a.m. UTC | #1
On Fri, Jul 06, 2018 at 03:15:58PM +0100, Marcin Gorzel wrote:
> Rematrixing supports up to 64 channels but there is only a limited number of channel layouts defined. Currently, in/out channel count is obtained from the channel layout so if the channel layout is undefined (e.g. for 9, 10, 11 channels etc.) the in/out channel count will be 0 and the rematrixing will fail. This change adds a check if the channel layout is non-zero, and if not, prefers to use the in|out_ch_count instead. This seems to be related to ticket #6790.
> ---
>  libswresample/rematrix.c          | 18 ++++++++++++------
>  libswresample/x86/rematrix_init.c |  8 ++++++--
>  2 files changed, 18 insertions(+), 8 deletions(-)

Iam not completely sure what this is trying to do exactly
but the commit message inadequently describes it.

#6790 is already fixed, the commit message doesnt explain how its related

also the FFMIN is wrong. If the user provided a value outside
the supported range the code must have failed with an error
already or something is not working correctly.

How can the bug this fixes be reproduced ?

Thanks

[...]
Marcin Gorzel July 9, 2018, 12:55 p.m. UTC | #2
Thank you for your comments Michael and apologies if my commit message was
inadequate (I am new to this forum and this is my first patch).

The bug can be reproduced when downmixing audio with more than 8 channels,
for example:

./ffmpeg -i input_9ch.wav -filter:a:0
pan="6c|c0=0.166*c0+0.166*c6|c1=c1|c2=c2|c3=c3|c4=c4|c5=c5" -y
output_6ch.wav

The result is noisy output in the first channel.

#6790 applies the fix to the swr_set_matrix() method but a similar fix
needs to be applied to the swri_rematrix_init()
and swri_rematrix_init_x86() as well.

Since currently the number of in/out channels is determined based on the
channel layout (av_get_channel_layout_nb_channels(s->in_ch_layout)) my
patch first checks if the channel layout is non-zero, and if it 0, then it
falls back to using the (user) channel count instead.

Re. FFMIN: Agreed. I could add checks if the channel count is within
supported range. For example if s->user_out_ch_count < SWR_CH_MAX and
s->user_in_ch_count
< SWR_CH_MAX inside swr_init() method (for example, similarly as is done in
swresample.c:178)?

Thanks,

Marcin

On Sat, Jul 7, 2018 at 2:04 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, Jul 06, 2018 at 03:15:58PM +0100, Marcin Gorzel wrote:
> > Rematrixing supports up to 64 channels but there is only a limited
> number of channel layouts defined. Currently, in/out channel count is
> obtained from the channel layout so if the channel layout is undefined
> (e.g. for 9, 10, 11 channels etc.) the in/out channel count will be 0 and
> the rematrixing will fail. This change adds a check if the channel layout
> is non-zero, and if not, prefers to use the in|out_ch_count instead. This
> seems to be related to ticket #6790.
> > ---
> >  libswresample/rematrix.c          | 18 ++++++++++++------
> >  libswresample/x86/rematrix_init.c |  8 ++++++--
> >  2 files changed, 18 insertions(+), 8 deletions(-)
>
> Iam not completely sure what this is trying to do exactly
> but the commit message inadequently describes it.
>
> #6790 is already fixed, the commit message doesnt explain how its related
>
> also the FFMIN is wrong. If the user provided a value outside
> the supported range the code must have failed with an error
> already or something is not working correctly.
>
> How can the bug this fixes be reproduced ?
>
> Thanks
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I know you won't believe me, but the highest form of Human Excellence is
> to question oneself and others. -- Socrates
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Michael Niedermayer July 9, 2018, 8:11 p.m. UTC | #3
On Mon, Jul 09, 2018 at 01:55:37PM +0100, Marcin Gorzel wrote:
> Thank you for your comments Michael and apologies if my commit message was
> inadequate (I am new to this forum and this is my first patch).
> 
> The bug can be reproduced when downmixing audio with more than 8 channels,
> for example:
> 
> ./ffmpeg -i input_9ch.wav -filter:a:0
> pan="6c|c0=0.166*c0+0.166*c6|c1=c1|c2=c2|c3=c3|c4=c4|c5=c5" -y
> output_6ch.wav
> 
> The result is noisy output in the first channel.
> 
> #6790 applies the fix to the swr_set_matrix() method but a similar fix
> needs to be applied to the swri_rematrix_init()
> and swri_rematrix_init_x86() as well.
> 

> Since currently the number of in/out channels is determined based on the
> channel layout (av_get_channel_layout_nb_channels(s->in_ch_layout)) my
> patch first checks if the channel layout is non-zero, and if it 0, then it
> falls back to using the (user) channel count instead.

theres the layout and the channel count

before      one is checked and used and if possible and if not the other
afterwards  one is checked and used and if possible and if not the other

the patch seems to just check the other field
I dont know how to match this up with the explanation of what this patch
does.
Quite possibly iam missing something 


> 
> Re. FFMIN: Agreed. I could add checks if the channel count is within
> supported range. For example if s->user_out_ch_count < SWR_CH_MAX and
> s->user_in_ch_count
> < SWR_CH_MAX inside swr_init() method (for example, similarly as is done in
> swresample.c:178)?

Each field has a defined range in libswresample/options.c
the AVOption code checks this when setting the field

If the value in the table is wrong, thats what should be fixed.
If something sets it ignoring the value, that code should be fixed.
i dont think we should add a check without first understanding what
sets it outside the range





> 
> Thanks,
> 
> Marcin
> 
> On Sat, Jul 7, 2018 at 2:04 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Fri, Jul 06, 2018 at 03:15:58PM +0100, Marcin Gorzel wrote:
> > > Rematrixing supports up to 64 channels but there is only a limited
> > number of channel layouts defined. Currently, in/out channel count is
> > obtained from the channel layout so if the channel layout is undefined
> > (e.g. for 9, 10, 11 channels etc.) the in/out channel count will be 0 and
> > the rematrixing will fail. This change adds a check if the channel layout
> > is non-zero, and if not, prefers to use the in|out_ch_count instead. This
> > seems to be related to ticket #6790.
> > > ---
> > >  libswresample/rematrix.c          | 18 ++++++++++++------
> > >  libswresample/x86/rematrix_init.c |  8 ++++++--
> > >  2 files changed, 18 insertions(+), 8 deletions(-)
> >
> > Iam not completely sure what this is trying to do exactly
> > but the commit message inadequently describes it.
> >
> > #6790 is already fixed, the commit message doesnt explain how its related
> >
> > also the FFMIN is wrong. If the user provided a value outside
> > the supported range the code must have failed with an error
> > already or something is not working correctly.
> >
> > How can the bug this fixes be reproduced ?
> >
> > Thanks
> >
> > [...]
> >
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > I know you won't believe me, but the highest form of Human Excellence is
> > to question oneself and others. -- Socrates
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> 
> 
> -- 
> 
> Marcin Gorzel |  Software Engineer |  gorzel@google.com |
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Marcin Gorzel July 10, 2018, 9:34 a.m. UTC | #4
Hi Michael,

I think I know where the misunderstanding could be.

The main changes in my patch are:

rematrix.c:389 and rematrix_int.c:36:

Before:
int nb_in  = av_get_channel_layout_nb_channels(s->in_ch_layout)

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

However, the change you are referring to (rematrix.c:72) has been made just
to match the above changes (although you are right that functionally this
particular one change should be the same).
I just thought that
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.
Please let me know if that makes sense.

Each field has a defined range in libswresample/options.c
> the AVOption code checks this when setting the field


> If the value in the table is wrong, thats what should be fixed.
> If something sets it ignoring the value, that code should be fixed.
> i dont think we should add a check without first understanding what
> sets it outside the range


I believe the check if the number of channels is valid is made in
libavcodec/utils.c:676.
However, the output error message is quite general and possibly not very
helpful:

Error while opening decoder for input stream #0:0 : Invalid argument

That's why in this patch I've added an av_log in the utils.c so that the
output error message is more useful:

[pcm_s16le @ 0x55fd9950d5c0] Unsupported number of channels: 72.
Stream mapping:
  Stream #0:0 -> #0:0 (pcm_s16le (native) -> pcm_s16le (native))
Error while opening decoder for input stream #0:0 : Invalid argument

Re. the additional checks in the swresample.c, if you think they are
unnecessary, I will happily remove them from this patch. Please let me know!

Regards,

Marcin

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

> On Mon, Jul 09, 2018 at 01:55:37PM +0100, Marcin Gorzel wrote:
> > Thank you for your comments Michael and apologies if my commit message
> was
> > inadequate (I am new to this forum and this is my first patch).
> >
> > The bug can be reproduced when downmixing audio with more than 8
> channels,
> > for example:
> >
> > ./ffmpeg -i input_9ch.wav -filter:a:0
> > pan="6c|c0=0.166*c0+0.166*c6|c1=c1|c2=c2|c3=c3|c4=c4|c5=c5" -y
> > output_6ch.wav
> >
> > The result is noisy output in the first channel.
> >
> > #6790 applies the fix to the swr_set_matrix() method but a similar fix
> > needs to be applied to the swri_rematrix_init()
> > and swri_rematrix_init_x86() as well.
> >
>
> > Since currently the number of in/out channels is determined based on the
> > channel layout (av_get_channel_layout_nb_channels(s->in_ch_layout)) my
> > patch first checks if the channel layout is non-zero, and if it 0, then
> it
> > falls back to using the (user) channel count instead.
>
> theres the layout and the channel count
>
> before      one is checked and used and if possible and if not the other
> afterwards  one is checked and used and if possible and if not the other
>
> the patch seems to just check the other field
> I dont know how to match this up with the explanation of what this patch
> does.
> Quite possibly iam missing something
>
>
> >
> > Re. FFMIN: Agreed. I could add checks if the channel count is within
> > supported range. For example if s->user_out_ch_count < SWR_CH_MAX and
> > s->user_in_ch_count
> > < SWR_CH_MAX inside swr_init() method (for example, similarly as is done
> in
> > swresample.c:178)?
>
> Each field has a defined range in libswresample/options.c
> the AVOption code checks this when setting the field
>
> If the value in the table is wrong, thats what should be fixed.
> If something sets it ignoring the value, that code should be fixed.
> i dont think we should add a check without first understanding what
> sets it outside the range
>
>
>
>
>
> >
> > Thanks,
> >
> > Marcin
> >
> > On Sat, Jul 7, 2018 at 2:04 AM Michael Niedermayer
> <michael@niedermayer.cc>
> > wrote:
> >
> > > On Fri, Jul 06, 2018 at 03:15:58PM +0100, Marcin Gorzel wrote:
> > > > Rematrixing supports up to 64 channels but there is only a limited
> > > number of channel layouts defined. Currently, in/out channel count is
> > > obtained from the channel layout so if the channel layout is undefined
> > > (e.g. for 9, 10, 11 channels etc.) the in/out channel count will be 0
> and
> > > the rematrixing will fail. This change adds a check if the channel
> layout
> > > is non-zero, and if not, prefers to use the in|out_ch_count instead.
> This
> > > seems to be related to ticket #6790.
> > > > ---
> > > >  libswresample/rematrix.c          | 18 ++++++++++++------
> > > >  libswresample/x86/rematrix_init.c |  8 ++++++--
> > > >  2 files changed, 18 insertions(+), 8 deletions(-)
> > >
> > > Iam not completely sure what this is trying to do exactly
> > > but the commit message inadequently describes it.
> > >
> > > #6790 is already fixed, the commit message doesnt explain how its
> related
> > >
> > > also the FFMIN is wrong. If the user provided a value outside
> > > the supported range the code must have failed with an error
> > > already or something is not working correctly.
> > >
> > > How can the bug this fixes be reproduced ?
> > >
> > > Thanks
> > >
> > > [...]
> > >
> > > --
> > > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> > >
> > > I know you won't believe me, but the highest form of Human Excellence
> is
> > > to question oneself and others. -- Socrates
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> >
> >
> > --
> >
> > Marcin Gorzel |  Software Engineer |  gorzel@google.com |
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Everything should be made as simple as possible, but not simpler.
> -- Albert Einstein
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Marcin Gorzel July 11, 2018, 7:11 p.m. UTC | #5
Michael, Nicolas,

Do you think this patch is now ready to be applied? Or would you like me to
make any further changes?

Thanks,

Marcin

On Tue, Jul 10, 2018 at 10:34 AM Marcin Gorzel <gorzel@google.com> wrote:

> Hi Michael,
>
> I think I know where the misunderstanding could be.
>
> The main changes in my patch are:
>
> rematrix.c:389 and rematrix_int.c:36:
>
> Before:
> int nb_in  = av_get_channel_layout_nb_channels(s->in_ch_layout)
>
> After:
> int nb_in  = s->in_ch_layout != 0
>         ? av_get_channel_layout_nb_channels(s->in_ch_layout)
>         : s->user_in_ch_count;
>
> However, the change you are referring to (rematrix.c:72) has been made
> just to match the above changes (although you are right that functionally
> this particular one change should be the same).
> I just thought that
> 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.
> Please let me know if that makes sense.
>
> Each field has a defined range in libswresample/options.c
>> the AVOption code checks this when setting the field
>
>
>> If the value in the table is wrong, thats what should be fixed.
>> If something sets it ignoring the value, that code should be fixed.
>> i dont think we should add a check without first understanding what
>> sets it outside the range
>
>
> I believe the check if the number of channels is valid is made in
> libavcodec/utils.c:676.
> However, the output error message is quite general and possibly not very
> helpful:
>
> Error while opening decoder for input stream #0:0 : Invalid argument
>
> That's why in this patch I've added an av_log in the utils.c so that the
> output error message is more useful:
>
> [pcm_s16le @ 0x55fd9950d5c0] Unsupported number of channels: 72.
> Stream mapping:
>   Stream #0:0 -> #0:0 (pcm_s16le (native) -> pcm_s16le (native))
> Error while opening decoder for input stream #0:0 : Invalid argument
>
> Re. the additional checks in the swresample.c, if you think they are
> unnecessary, I will happily remove them from this patch. Please let me know!
>
> Regards,
>
> Marcin
>
> On Mon, Jul 9, 2018 at 9:11 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
>
>> On Mon, Jul 09, 2018 at 01:55:37PM +0100, Marcin Gorzel wrote:
>> > Thank you for your comments Michael and apologies if my commit message
>> was
>> > inadequate (I am new to this forum and this is my first patch).
>> >
>> > The bug can be reproduced when downmixing audio with more than 8
>> channels,
>> > for example:
>> >
>> > ./ffmpeg -i input_9ch.wav -filter:a:0
>> > pan="6c|c0=0.166*c0+0.166*c6|c1=c1|c2=c2|c3=c3|c4=c4|c5=c5" -y
>> > output_6ch.wav
>> >
>> > The result is noisy output in the first channel.
>> >
>> > #6790 applies the fix to the swr_set_matrix() method but a similar fix
>> > needs to be applied to the swri_rematrix_init()
>> > and swri_rematrix_init_x86() as well.
>> >
>>
>> > Since currently the number of in/out channels is determined based on the
>> > channel layout (av_get_channel_layout_nb_channels(s->in_ch_layout)) my
>> > patch first checks if the channel layout is non-zero, and if it 0, then
>> it
>> > falls back to using the (user) channel count instead.
>>
>> theres the layout and the channel count
>>
>> before      one is checked and used and if possible and if not the other
>> afterwards  one is checked and used and if possible and if not the other
>>
>> the patch seems to just check the other field
>> I dont know how to match this up with the explanation of what this patch
>> does.
>> Quite possibly iam missing something
>>
>>
>> >
>> > Re. FFMIN: Agreed. I could add checks if the channel count is within
>> > supported range. For example if s->user_out_ch_count < SWR_CH_MAX and
>> > s->user_in_ch_count
>> > < SWR_CH_MAX inside swr_init() method (for example, similarly as is
>> done in
>> > swresample.c:178)?
>>
>> Each field has a defined range in libswresample/options.c
>> the AVOption code checks this when setting the field
>>
>> If the value in the table is wrong, thats what should be fixed.
>> If something sets it ignoring the value, that code should be fixed.
>> i dont think we should add a check without first understanding what
>> sets it outside the range
>>
>>
>>
>>
>>
>> >
>> > Thanks,
>> >
>> > Marcin
>> >
>> > On Sat, Jul 7, 2018 at 2:04 AM Michael Niedermayer
>> <michael@niedermayer.cc>
>> > wrote:
>> >
>> > > On Fri, Jul 06, 2018 at 03:15:58PM +0100, Marcin Gorzel wrote:
>> > > > Rematrixing supports up to 64 channels but there is only a limited
>> > > number of channel layouts defined. Currently, in/out channel count is
>> > > obtained from the channel layout so if the channel layout is undefined
>> > > (e.g. for 9, 10, 11 channels etc.) the in/out channel count will be 0
>> and
>> > > the rematrixing will fail. This change adds a check if the channel
>> layout
>> > > is non-zero, and if not, prefers to use the in|out_ch_count instead.
>> This
>> > > seems to be related to ticket #6790.
>> > > > ---
>> > > >  libswresample/rematrix.c          | 18 ++++++++++++------
>> > > >  libswresample/x86/rematrix_init.c |  8 ++++++--
>> > > >  2 files changed, 18 insertions(+), 8 deletions(-)
>> > >
>> > > Iam not completely sure what this is trying to do exactly
>> > > but the commit message inadequently describes it.
>> > >
>> > > #6790 is already fixed, the commit message doesnt explain how its
>> related
>> > >
>> > > also the FFMIN is wrong. If the user provided a value outside
>> > > the supported range the code must have failed with an error
>> > > already or something is not working correctly.
>> > >
>> > > How can the bug this fixes be reproduced ?
>> > >
>> > > Thanks
>> > >
>> > > [...]
>> > >
>> > > --
>> > > Michael     GnuPG fingerprint:
>> 9FF2128B147EF6730BADF133611EC787040B0FAB
>> > >
>> > > I know you won't believe me, but the highest form of Human Excellence
>> is
>> > > to question oneself and others. -- Socrates
>> > > _______________________________________________
>> > > ffmpeg-devel mailing list
>> > > ffmpeg-devel@ffmpeg.org
>> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > >
>> >
>> >
>> > --
>> >
>> > Marcin Gorzel |  Software Engineer |  gorzel@google.com |
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Everything should be made as simple as possible, but not simpler.
>> -- Albert Einstein
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
>
> --
>
> Marcin Gorzel |  Software Engineer |  gorzel@google.com |
>
Michael Niedermayer July 12, 2018, 11:15 p.m. UTC | #6
On Tue, Jul 10, 2018 at 10:34:51AM +0100, Marcin Gorzel wrote:
> Hi Michael,
> 
> I think I know where the misunderstanding could be.
> 
> The main changes in my patch are:
> 
> rematrix.c:389 and rematrix_int.c:36:
> 
> Before:
> int nb_in  = av_get_channel_layout_nb_channels(s->in_ch_layout)
> 
> After:
> int nb_in  = s->in_ch_layout != 0
>         ? av_get_channel_layout_nb_channels(s->in_ch_layout)
>         : s->user_in_ch_count;
> 

> However, the change you are referring to (rematrix.c:72) has been made just
> to match the above changes (although you are right that functionally this
> particular one change should be the same).

This should be documented in the commit message


> I just thought that
> 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.
> Please let me know if that makes sense.
> 
> Each field has a defined range in libswresample/options.c
> > the AVOption code checks this when setting the field
> 
> 
> > If the value in the table is wrong, thats what should be fixed.
> > If something sets it ignoring the value, that code should be fixed.
> > i dont think we should add a check without first understanding what
> > sets it outside the range
> 
> 
> I believe the check if the number of channels is valid is made in
> libavcodec/utils.c:676.

you are changing libswresample, the check in libavcodec looks unrelated
to libswresample.


> However, the output error message is quite general and possibly not very
> helpful:

Clearer error messages are certainly better, improvments to error
messages should be welcome!

[...]
Marcin Gorzel July 13, 2018, 11:34 a.m. UTC | #7
On Fri, Jul 13, 2018 at 12:15 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Tue, Jul 10, 2018 at 10:34:51AM +0100, Marcin Gorzel wrote:
> > Hi Michael,
> >
> > I think I know where the misunderstanding could be.
> >
> > The main changes in my patch are:
> >
> > rematrix.c:389 and rematrix_int.c:36:
> >
> > Before:
> > int nb_in  = av_get_channel_layout_nb_channels(s->in_ch_layout)
> >
> > After:
> > int nb_in  = s->in_ch_layout != 0
> >         ? av_get_channel_layout_nb_channels(s->in_ch_layout)
> >         : s->user_in_ch_count;
> >
>
> > However, the change you are referring to (rematrix.c:72) has been made
> just
> > to match the above changes (although you are right that functionally this
> > particular one change should be the same).
>
> This should be documented in the commit message
>

Done.


> > I just thought that
> > 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.
> > Please let me know if that makes sense.
> >
> > Each field has a defined range in libswresample/options.c
> > > the AVOption code checks this when setting the field
> >
> >
> > > If the value in the table is wrong, thats what should be fixed.
> > > If something sets it ignoring the value, that code should be fixed.
> > > i dont think we should add a check without first understanding what
> > > sets it outside the range
> >
> >
> > I believe the check if the number of channels is valid is made in
> > libavcodec/utils.c:676.
>
> you are changing libswresample, the check in libavcodec looks unrelated
> to libswresample.
>

Done. (Removed changes in libavcodec - now this patch only affects
libswrsample).

> However, the output error message is quite general and possibly not very
> > helpful:
>
> Clearer error messages are certainly better, improvments to error
> messages should be welcome!
>

Channel checks (error logs) will be added as separate patch(es), as
suggested above.


>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you fake or manipulate statistics in a paper in physics you will never
> get a job again.
> If you fake or manipulate statistics in a paper in medicin you will get
> a job for life at the pharma industry.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
diff mbox

Patch

diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c
index 8227730056..45e84eb2df 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)
+        : FFMIN(s->user_in_ch_count, SWR_CH_MAX);
+    nb_out = s->user_out_ch_layout != 0
+        ? av_get_channel_layout_nb_channels(s->user_out_ch_layout)
+        : FFMIN(s->user_out_ch_count, SWR_CH_MAX);
     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)
+        : FFMIN(s->user_in_ch_count, SWR_CH_MAX);
+    int nb_out = s->out_ch_layout != 0
+        ? av_get_channel_layout_nb_channels(s->out_ch_layout)
+        : FFMIN(s->user_out_ch_count, SWR_CH_MAX);
 
     s->mix_any_f = NULL;
 
diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c
index d71b41a73e..f3363567cd 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)
+        : FFMIN(s->user_in_ch_count, SWR_CH_MAX);
+    int nb_out = s->out_ch_layout != 0
+        ? av_get_channel_layout_nb_channels(s->out_ch_layout)
+        : FFMIN(s->user_out_ch_count, SWR_CH_MAX);
     int num    = nb_in * nb_out;
     int i,j;