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

Submitted by Marcin Gorzel on July 9, 2018, 3:48 p.m.

Details

Message ID 20180709154802.42505-1-gorzel@google.com
State New
Headers show

Commit Message

Marcin Gorzel July 9, 2018, 3:48 p.m.
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(-)

Comments

Nicolas George July 9, 2018, 3:59 p.m.
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,
Marcin Gorzel July 9, 2018, 5:50 p.m.
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
Nicolas George July 9, 2018, 6:05 p.m.
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,
Marcin Gorzel July 10, 2018, 9:05 a.m.
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,
Michael Niedermayer July 12, 2018, 11:17 p.m.
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

[...]

Patch hide | download patch | download mbox

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;