Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
On Fri, Jul 13, 2018 at 12:15 AM Michael Niedermayer 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 > -- Marcin Gorzel | Software Engineer | gor...@google.com | ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
On Mon, Jul 09, 2018 at 04:48:02PM +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. > --- > libavcodec/utils.c| 1 + > libswresample/rematrix.c | 18 -- > libswresample/swresample.c| 10 ++ > libswresample/x86/rematrix_init.c | 8 ++-- > 4 files changed, 29 insertions(+), 8 deletions(-) Changes to libavcodec and libswresample should be in seperate patches a fate test for the case that is fixed would also be good probably thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I do not agree with what you have to say, but I'll defend to the death your right to say it. -- Voltaire signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
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! [...] -- 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. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
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 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 > 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 >> >> > 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 #6
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
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 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 > > > 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 co
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
Thanks for the explanation Nicolas. When the matrix is not explicitly provided, lswr computes it. For that, > it needs the channel layout, because you do not mix a rear channel the > same way as a subwoofer. This patch absolutely needs to be tested under > these circumstances too. > This patch does not affect the behavior when the downmix matrix is not explicitly provided. For example, running: ./ffmpeg -i input_9ch.wav -ac 6 output_6ch.wav Results in: [auto_resampler_0 @ 0x5617a8668c80] [SWR @ 0x5617a8669180] Rematrix is needed between 9 channels and 5.1 but there is not enough information to do it [auto_resampler_0 @ 0x5617a8668c80] Failed to configure output pad on auto_resampler_0 Which I think is a reasonable expectation. Since the channel layout for 9 channel audio is undefined, there is no 'standard' way to downmix it to 5.1 or 2.0 etc. For 'known' channel layouts, the output is correct as well: ./ffmpeg -i input_8ch.wav -ac 6 output_6ch.wav [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] FL: FL:0.585786 FR:0.00 FC:0.00 LFE:0.00 BL:0.00 BR:0.00 SL:0.00 SR:0.00 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] FR: FL:0.00 FR:0.585786 FC:0.00 LFE:0.00 BL:0.00 BR:0.00 SL:0.00 SR:0.00 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] FC: FL:0.00 FR:0.00 FC:0.585786 LFE:0.00 BL:0.00 BR:0.00 SL:0.00 SR:0.00 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] LFE: FL:0.00 FR:0.00 FC:0.00 LFE:0.585786 BL:0.00 BR:0.00 SL:0.00 SR:0.00 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] BL: FL:0.00 FR:0.00 FC:0.00 LFE:0.00 BL:0.585786 BR:0.00 SL:0.414214 SR:0.00 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] BR: FL:0.00 FR:0.00 FC:0.00 LFE:0.00 BL:0.00 BR:0.585786 SL:0.00 SR:0.414214 [auto_resampler_0 @ 0x55adfbd6fa00] ch:8 chl:7.1 fmt:s16 r:48000Hz -> ch:6 chl:5.1 fmt:s16 r:48000Hz Thank you for raising that point and please let me know if you still have concerns. Regards, -- Marcin Gorzel | Software Engineer | gor...@google.com | ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
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 > 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 signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
Marcin Gorzel (2018-07-09): > ./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 > > Without the patch, the output in the first channel (c0) is noise. After > applying the patch, I can verify that two sine tones are mixed together and > scaled properly. Ok, so this is with an explicit matrix provided. > Could you explain what you mean by "the actual layouts are necessary to build > the matrix"? When the matrix is not explicitly provided, lswr computes it. For that, it needs the channel layout, because you do not mix a rear channel the same way as a subwoofer. This patch absolutely needs to be tested under these circumstances too. Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
Hi Nicolas, > Please remember to mention "lswr" or "libswresample" in the first line > of the commit message. > Apologies, I will update the commit message. > > 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. > > I do not understand how it can work: the actual layouts are necessary to > build the matrix, otherwise it is not possible to know which channel > needs to be mixed into which. Can you explain how this patch was tested? I create a 9-channel wav file, with a different sine tone in each channel (100Hz, 200Hz, 300Hz, ...). I downmix it to 6 channels using the following command: ./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 Without the patch, the output in the first channel (c0) is noise. After applying the patch, I can verify that two sine tones are mixed together and scaled properly. Could you explain what you mean by "the actual layouts are necessary to build the matrix"? In the case of input channel counts of 8 or more (16 is an exception) the channel layout is 0, although the matrix is created correctly? For example, based on the above example: [Parsed_pan_0 @ 0x561d8777dbc0] [SWR @ 0x561d87787740] Using s16p internally between filters [Parsed_pan_0 @ 0x561d8777dbc0] o0 = 0.166 i0 + 0 i1 + 0 i2 + 0 i3 + 0 i4 + 0 i5 + 0.166 i6 + 0 i7 + 0 i8 [Parsed_pan_0 @ 0x561d8777dbc0] o1 = 0 i0 + 1 i1 + 0 i2 + 0 i3 + 0 i4 + 0 i5 + 0 i6 + 0 i7 + 0 i8 [Parsed_pan_0 @ 0x561d8777dbc0] o2 = 0 i0 + 0 i1 + 1 i2 + 0 i3 + 0 i4 + 0 i5 + 0 i6 + 0 i7 + 0 i8 [Parsed_pan_0 @ 0x561d8777dbc0] o3 = 0 i0 + 0 i1 + 0 i2 + 1 i3 + 0 i4 + 0 i5 + 0 i6 + 0 i7 + 0 i8 [Parsed_pan_0 @ 0x561d8777dbc0] o4 = 0 i0 + 0 i1 + 0 i2 + 0 i3 + 1 i4 + 0 i5 + 0 i6 + 0 i7 + 0 i8 [Parsed_pan_0 @ 0x561d8777dbc0] o5 = 0 i0 + 0 i1 + 0 i2 + 0 i3 + 0 i4 + 1 i5 + 0 i6 + 0 i7 + 0 i8 Output #0, wav, to 'output_6ch.wav': Metadata: ISFT: Lavf58.17.101 Stream #0:0, 0, 1/48000: Audio: pcm_s16le ([1][0][0][0] / 0x0001), 48000 Hz, 5.1, s16, 4608 kb/s Regards, Marcin -- Marcin Gorzel | Software Engineer | gor...@google.com | ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
Marcin Gorzel (2018-07-09): > Subject: Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is > undefined Please remember to mention "lswr" or "libswresample" in the first line of the commit message. > 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. I do not understand how it can work: the actual layouts are necessary to build the matrix, otherwise it is not possible to know which channel needs to be mixed into which. Can you explain how this patch was tested? Regards, -- Nicolas George signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
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 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
Re: [FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
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 signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
