[FFmpeg-devel] [PATCH] Use channel count if channel layout is undefined
Marcin Gorzel
gorzel at google.com
Wed Jul 11 22:11:27 EEST 2018
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 at 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 at 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 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
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
>
> --
>
> Marcin Gorzel | Software Engineer | gorzel at google.com |
>
--
Marcin Gorzel | Software Engineer | gorzel at google.com |
More information about the ffmpeg-devel
mailing list