diff mbox series

[FFmpeg-devel,16/25] avfilter/af_headphone: Fix channel assignment

Message ID 20200908211856.16290-16-andreas.rheinhardt@gmail.com
State Accepted
Commit 0952f8f909fa723d790ceb43e562e316efbf99dd
Headers show
Series [FFmpeg-devel,01/25] avfilter/af_headphone: Don't use uninitialized buffer in log message | expand

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt Sept. 8, 2020, 9:18 p.m. UTC 
The documentation of the map argument of the headphone filter states:

"Set mapping of input streams for convolution. The argument is a
’|’-separated list of channel names in order as they are given as
additional stream inputs for filter."

Yet this has not been honoured at all. Instead for the kth given HRIR
channel pair it was checked whether there was a kth mapping and if the
channel position so given was present in the channel layout of the main
input; if so, then the given HRIR channel pair was matched to the kth
channel of the main input. It should actually have been matched to the
channel given by the kth mapping. A consequence of the current algorithm
is that if N additional HRIR channel pairs are given, a permutation of
the first N entries of the mapping does not affect the output at all.

The old code might even set arrays belonging to streams that don't exist
(i.e. whose index is >= the number of channels of the main input
stream); these parts were not read lateron at all. The new code doesn't
do this any longer and therefore the number of elements of some of the
allocated arrays has been reduced (in case the number of mappings was
bigger than the number of channels of the first input stream).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavfilter/af_headphone.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

Comments

Paul B Mahol Sept. 9, 2020, 1:24 a.m. UTC | #1 
On Tue, Sep 08, 2020 at 11:18:47PM +0200, Andreas Rheinhardt wrote:
> The documentation of the map argument of the headphone filter states:
> 
> "Set mapping of input streams for convolution. The argument is a
> ’|’-separated list of channel names in order as they are given as
> additional stream inputs for filter."
> 
> Yet this has not been honoured at all. Instead for the kth given HRIR
> channel pair it was checked whether there was a kth mapping and if the
> channel position so given was present in the channel layout of the main
> input; if so, then the given HRIR channel pair was matched to the kth
> channel of the main input. It should actually have been matched to the
> channel given by the kth mapping. A consequence of the current algorithm
> is that if N additional HRIR channel pairs are given, a permutation of
> the first N entries of the mapping does not affect the output at all.
> 
> The old code might even set arrays belonging to streams that don't exist
> (i.e. whose index is >= the number of channels of the main input
> stream); these parts were not read lateron at all. The new code doesn't
> do this any longer and therefore the number of elements of some of the
> allocated arrays has been reduced (in case the number of mappings was
> bigger than the number of channels of the first input stream).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavfilter/af_headphone.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 

should be fine if properly tested.
diff mbox series

Patch

diff --git a/libavfilter/af_headphone.c b/libavfilter/af_headphone.c
index fb6af7a966..d6647ff80b 100644
--- a/libavfilter/af_headphone.c
+++ b/libavfilter/af_headphone.c
@@ -117,8 +117,6 @@  static void parse_map(AVFilterContext *ctx)
             continue;
         }
         used_channels        |= out_channel;
-        if (out_channel == AV_CH_LOW_FREQUENCY)
-            s->lfe_channel = s->nb_irs;
         s->mapping[s->nb_irs] = out_channel;
         s->nb_irs++;
     }
@@ -368,7 +366,6 @@  static int convert_coeffs(AVFilterContext *ctx, AVFilterLink *inlink)
 {
     struct HeadphoneContext *s = ctx->priv;
     const int ir_len = s->ir_len;
-    int nb_irs = s->nb_irs;
     int nb_input_channels = ctx->inputs[0]->channels;
     float gain_lin = expf((s->gain - 3 * nb_input_channels) / 20 * M_LN10);
     FFTComplex *fft_in_l = NULL;
@@ -430,15 +427,15 @@  static int convert_coeffs(AVFilterContext *ctx, AVFilterLink *inlink)
         s->temp_src[0] = av_calloc(s->air_len, sizeof(float));
         s->temp_src[1] = av_calloc(s->air_len, sizeof(float));
 
-        s->data_ir[0] = av_calloc(nb_irs * s->air_len, sizeof(*s->data_ir[0]));
-        s->data_ir[1] = av_calloc(nb_irs * s->air_len, sizeof(*s->data_ir[1]));
+        s->data_ir[0] = av_calloc(nb_input_channels * s->air_len, sizeof(*s->data_ir[0]));
+        s->data_ir[1] = av_calloc(nb_input_channels * s->air_len, sizeof(*s->data_ir[1]));
         if (!s->data_ir[0] || !s->data_ir[1] || !s->temp_src[0] || !s->temp_src[1]) {
             ret = AVERROR(ENOMEM);
             goto fail;
         }
     } else {
-        s->data_hrtf[0] = av_calloc(n_fft, sizeof(*s->data_hrtf[0]) * nb_irs);
-        s->data_hrtf[1] = av_calloc(n_fft, sizeof(*s->data_hrtf[1]) * nb_irs);
+        s->data_hrtf[0] = av_calloc(n_fft, sizeof(*s->data_hrtf[0]) * nb_input_channels);
+        s->data_hrtf[1] = av_calloc(n_fft, sizeof(*s->data_hrtf[1]) * nb_input_channels);
         if (!s->data_hrtf[0] || !s->data_hrtf[1]) {
             ret = AVERROR(ENOMEM);
             goto fail;
@@ -459,7 +456,9 @@  static int convert_coeffs(AVFilterContext *ctx, AVFilterLink *inlink)
 
             for (j = 0; j < inlink->channels; j++) {
                 if ((av_channel_layout_extract_channel(inlink->channel_layout, j)) == s->mapping[i]) {
-                    idx = i;
+                    idx = j;
+                    if (s->mapping[i] == AV_CH_LOW_FREQUENCY)
+                        s->lfe_channel = j;
                     break;
                 }
             }
@@ -499,14 +498,16 @@  static int convert_coeffs(AVFilterContext *ctx, AVFilterLink *inlink)
 
                 for (j = 0; j < inlink->channels; j++) {
                     if ((av_channel_layout_extract_channel(inlink->channel_layout, j)) == s->mapping[k]) {
-                        idx = k;
+                        idx = j;
+                        if (s->mapping[k] == AV_CH_LOW_FREQUENCY)
+                            s->lfe_channel = j;
                         break;
                     }
                 }
                 if (idx == -1)
                     continue;
 
-                I = idx * 2;
+                I = k * 2;
                 if (s->type == TIME_DOMAIN) {
                     float *data_ir_l = s->data_ir[0] + idx * s->air_len;
                     float *data_ir_r = s->data_ir[1] + idx * s->air_len;