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 <gor...@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 <mich...@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 >> <mich...@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 | gor...@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 | gor...@google.com | > -- Marcin Gorzel | Software Engineer | gor...@google.com | _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
