| Message ID | 20180709154802.42505-1-gorzel@google.com |
|---|---|
| State | New |
| Headers | show |
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,
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 (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,
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.000000 FC:0.000000 LFE:0.000000 BL:0.000000 BR:0.000000 SL:0.000000 SR:0.000000 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] FR: FL:0.000000 FR:0.585786 FC:0.000000 LFE:0.000000 BL:0.000000 BR:0.000000 SL:0.000000 SR:0.000000 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] FC: FL:0.000000 FR:0.000000 FC:0.585786 LFE:0.000000 BL:0.000000 BR:0.000000 SL:0.000000 SR:0.000000 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] LFE: FL:0.000000 FR:0.000000 FC:0.000000 LFE:0.585786 BL:0.000000 BR:0.000000 SL:0.000000 SR:0.000000 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] BL: FL:0.000000 FR:0.000000 FC:0.000000 LFE:0.000000 BL:0.585786 BR:0.000000 SL:0.414214 SR:0.000000 [auto_resampler_0 @ 0x55adfbd6fa00] [SWR @ 0x55adfbd6fec0] BR: FL:0.000000 FR:0.000000 FC:0.000000 LFE:0.000000 BL:0.000000 BR:0.585786 SL:0.000000 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,
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 [...]
diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 59d41ccbb6..728f2b3355 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -674,6 +674,7 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code av_freep(&avctx->subtitle_header); if (avctx->channels > FF_SANE_NB_CHANNELS) { + av_log(avctx, AV_LOG_ERROR, "Unsupported number of channels: %d.\n", avctx->channels); ret = AVERROR(EINVAL); goto free_and_end; } diff --git a/libswresample/rematrix.c b/libswresample/rematrix.c index 8227730056..8c9fbf3804 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) + : s->user_in_ch_count; + nb_out = s->user_out_ch_layout != 0 + ? av_get_channel_layout_nb_channels(s->user_out_ch_layout) + : s->user_out_ch_count; 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) + : s->user_in_ch_count; + int nb_out = s->out_ch_layout != 0 + ? av_get_channel_layout_nb_channels(s->out_ch_layout) + : s->user_out_ch_count; s->mix_any_f = NULL; diff --git a/libswresample/swresample.c b/libswresample/swresample.c index 5bd39caac4..ecb9d97bc9 100644 --- a/libswresample/swresample.c +++ b/libswresample/swresample.c @@ -168,6 +168,16 @@ av_cold int swr_init(struct SwrContext *s){ s-> in.ch_count = s-> user_in_ch_count; s->used_ch_count = s->user_used_ch_count; + if(s->user_out_ch_count > SWR_CH_MAX) { + av_log(s, AV_LOG_ERROR, "Output channel count %d is unsupported.\n", s->user_out_ch_count); + return AVERROR(EINVAL); + } + + if(s->user_in_ch_count > SWR_CH_MAX) { + av_log(s, AV_LOG_ERROR, "Input channel count %d is unsupported.\n", s->user_in_ch_count); + return AVERROR(EINVAL); + } + s-> in_ch_layout = s-> user_in_ch_layout; s->out_ch_layout = s->user_out_ch_layout; diff --git a/libswresample/x86/rematrix_init.c b/libswresample/x86/rematrix_init.c index d71b41a73e..a6ae074926 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) + : s->user_in_ch_count; + int nb_out = s->out_ch_layout != 0 + ? av_get_channel_layout_nb_channels(s->out_ch_layout) + : s->user_out_ch_count; int num = nb_in * nb_out; int i,j;