[pulseaudio-discuss] [PATCH] resampler: generate normalized rows in calc_map_table()

[Tanu Kaskinen]

On Tue, 2013-02-05 at 20:55 +0100, Stefan Huber wrote:
> Remixing one channel map to another is (except for special cases) done
> via a linear mapping between channels, whose corresponding matrix is
> computed by calc_map_table(). The k-th row in this matrix corresponds to
> the coefficients of the linear combination of the input channels that
> result in the k-th output channel. In order to avoid clipping of samples
> we require that the sum of these coefficients is (at most) 1. This
> commit ensures this.
> 
> Prior to this commit tests/remix-test.c gives 52 of 132 matrices that
> violate this property. For example:
> 'front-left,front-right,front-center,lfe' -> 'front-left,front-right'
>            prior this commit                  after this commit
>          I00   I01   I02   I03              I00   I01   I02   I03
>       +------------------------          +------------------------
>   O00 | 0.750 0.000 0.375 0.375      O00 | 0.560 0.000 0.240 0.200
>   O01 | 0.000 0.750 0.375 0.375      O01 | 0.000 0.560 0.240 0.200
> 
> Building the matrix is done in several steps. In order to guarantee
> normalized rows, this patch assures that each step preserves a row-sum
> of 1.0 (or leaves it at 0.0). Most notably, if a center channel is mixed
> onto left and right channels then the weights 0.7 and 0.3 (which sum up
> to 1.0) are used rather than 0.75 and 0.375. Similarly, if the LFE
> channel is mixed onto left and right channels then the weights 0.8 and
> 0.2 are used instead of 0.75 and 0.375.
> ---
>  src/pulsecore/resampler.c |   74 ++++++++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 34 deletions(-)

Sounds like a good change to me. I'll point out a couple of cosmetic
issues below.

> diff --git a/src/pulsecore/resampler.c b/src/pulsecore/resampler.c
> index 0d21158..2022c83 100644
> --- a/src/pulsecore/resampler.c
> +++ b/src/pulsecore/resampler.c
> @@ -742,11 +742,11 @@ static void calc_map_table(pa_resampler *r) {
>           * 7) Make sure S:Center, S:LFE is used:
>           *
>           *    S:Center, S:LFE: If not connected, mix into all D:left, all
> -         *    D:right, all D:center channels, gain is 0.375. The current (as
> -         *    result of 1..6) factors should be multiplied by 0.75. (Alt.
> -         *    suggestion: 0.25 vs. 0.5) If C-front is only mixed into
> -         *    L-front/R-front if available, otherwise into all L/R channels.
> -         *    Similarly for C-rear.
> +         *    D:right, all D:center channels, gain is 0.3. The current (as
> +         *    result of 1..6) factors should be multiplied by 0.7 (in the case
> +         *    of S:Center) and 0.8 (in the case of S:LFE). If C-front is only

On that last quoted line, the "If" doesn't seem to belong there.

> +         *    mixed into L-front/R-front if available, otherwise into all L/R
> +         *    channels. Similarly for C-rear.
>           *
>           * S: and D: shall relate to the source resp. destination channels.
>           *
> @@ -901,13 +901,11 @@ static void calc_map_table(pa_resampler *r) {
>  
>                  for (ic = 0; ic < n_ic; ic++) {
>  
> -                    if (ic_connected[ic]) {
> -                        m->map_table_f[oc][ic] *= .9f;
> -                        continue;
> -                    }
> +                    m->map_table_f[oc][ic] *= .9f;
>  
> -                    if (on_left(r->i_cm.map[ic]))
> +                    if (!ic_connected[ic] && on_left(r->i_cm.map[ic])) {
>                          m->map_table_f[oc][ic] = .1f / (float) ic_unconnected_left;
> +                    }

No braces for single-line ifs, please.

-- 
Tanu