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

[Michael Niedermayer]

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 at 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 at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> 
> 
> -- 
> 
> Marcin Gorzel |  Software Engineer |  gorzel at google.com |
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at 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
-------------- 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/20180709/1227db6c/attachment.sig>