[PATCH 1/2] resampler: refactor calc_map_table()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2013-02-05 at 20:55 +0100, Stefan Huber wrote:
> - Separate the cases with PA_RESAMPLER_NO_REMAP or PA_RESAMPLER_NO_REMIX
>   set and remove redundant if-conditions.
> - Fix C90 compiler warning due to mixing code and variable declaration.
> - Do not repeatedly count number of left, right and center channels in
>   the input channel map.
> 
> The logic of calc_map_table() remains unaltered.
> ---
>  src/pulsecore/resampler.c |  370 +++++++++++++++++++++------------------------
>  1 file changed, 174 insertions(+), 196 deletions(-)

Thanks, this is a good change. A few comments below.

> diff --git a/src/pulsecore/resampler.c b/src/pulsecore/resampler.c
> index f0b3fd4..0d21158 100644
> --- a/src/pulsecore/resampler.c
> +++ b/src/pulsecore/resampler.c
> @@ -653,6 +653,14 @@ static void calc_map_table(pa_resampler *r) {
>      pa_strbuf *s;
>      char *t;
>      pa_remap_t *m;
> +    unsigned
> +        ic_left = 0,
> +        ic_right = 0,
> +        ic_center = 0,
> +        ic_unconnected_left = 0,
> +        ic_unconnected_right = 0,
> +        ic_unconnected_center = 0,
> +        ic_unconnected_lfe = 0;
>  
>      pa_assert(r);
>  
> @@ -668,228 +676,201 @@ static void calc_map_table(pa_resampler *r) {
>      memset(m->map_table_i, 0, sizeof(m->map_table_i));
>  
>      memset(ic_connected, 0, sizeof(ic_connected));
> -    remix = (r->flags & (PA_RESAMPLER_NO_REMAP|PA_RESAMPLER_NO_REMIX)) == 0;
> +    remix = (r->flags & (PA_RESAMPLER_NO_REMAP | PA_RESAMPLER_NO_REMIX)) == 0;
>  
> -    for (oc = 0; oc < n_oc; oc++) {
> -        pa_bool_t oc_connected = FALSE;
> -        pa_channel_position_t b = r->o_cm.map[oc];
> -
> -        for (ic = 0; ic < n_ic; ic++) {
> -            pa_channel_position_t a = r->i_cm.map[ic];
> +    if (r->flags & PA_RESAMPLER_NO_REMAP) {
> +        pa_assert(!remix);
> +        assert(n_oc == n_ic);

This assertion is not correct. The number of channels can be different.
In that case, only the first min(n_ic, n_oc) channels should be
connected.

>  
> -            if (r->flags & PA_RESAMPLER_NO_REMAP) {
> -                /* We shall not do any remapping. Hence, just check by index */
> +        for (oc = 0; oc < n_oc; oc++)
> +            m->map_table_f[oc][oc] = 1.0;
>  
> -                if (ic == oc)
> -                    m->map_table_f[oc][ic] = 1.0;
> +    } else if (r->flags & PA_RESAMPLER_NO_REMIX) {
> +        pa_assert(!remix);
> +        for (oc = 0; oc < n_oc; oc++) {
> +            pa_channel_position_t b = r->o_cm.map[oc];
>  
> -                continue;
> -            }
> +            for (ic = 0; ic < n_ic; ic++) {
> +                pa_channel_position_t a = r->i_cm.map[ic];
>  
> -            if (r->flags & PA_RESAMPLER_NO_REMIX) {
>                  /* We shall not do any remixing. Hence, just check by name */
> -
>                  if (a == b)
>                      m->map_table_f[oc][ic] = 1.0;
> -
> -                continue;
> -            }
> -
> -            pa_assert(remix);
> -
> -            /* OK, we shall do the full monty: upmixing and
> -             * downmixing. Our algorithm is relatively simple, does
> -             * not do spacialization, delay elements or apply lowpass
> -             * filters for LFE. Patches are always welcome,
> -             * though. Oh, and it doesn't do any matrix
> -             * decoding. (Which probably wouldn't make any sense
> -             * anyway.)
> -             *
> -             * This code is not idempotent: downmixing an upmixed
> -             * stereo stream is not identical to the original. The
> -             * volume will not match, and the two channels will be a
> -             * linear combination of both.
> -             *
> -             * This is loosely based on random suggestions found on the
> -             * Internet, such as this:
> -             * http://www.halfgaar.net/surround-sound-in-linux and the
> -             * alsa upmix plugin.
> -             *
> -             * The algorithm works basically like this:
> -             *
> -             * 1) Connect all channels with matching names.
> -             *
> -             * 2) Mono Handling:
> -             *    S:Mono: Copy into all D:channels
> -             *    D:Mono: Avg all S:channels
> -             *
> -             * 3) Mix D:Left, D:Right:
> -             *    D:Left: If not connected, avg all S:Left
> -             *    D:Right: If not connected, avg all S:Right
> -             *
> -             * 4) Mix D:Center
> -             *       If not connected, avg all S:Center
> -             *       If still not connected, avg all S:Left, S:Right
> -             *
> -             * 5) Mix D:LFE
> -             *       If not connected, avg all S:*
> -             *
> -             * 6) Make sure S:Left/S:Right is used: S:Left/S:Right: If
> -             *    not connected, mix into all D:left and all D:right
> -             *    channels. Gain is 0.1, the current left and right
> -             *    should be multiplied by 0.9.
> -             *
> -             * 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.
> -             *
> -             * S: and D: shall relate to the source resp. destination channels.
> -             *
> -             * Rationale: 1, 2 are probably obvious. For 3: this
> -             * copies front to rear if needed. For 4: we try to find
> -             * some suitable C source for C, if we don't find any, we
> -             * avg L and R. For 5: LFE is mixed from all channels. For
> -             * 6: the rear channels should not be dropped entirely,
> -             * however have only minimal impact. For 7: movies usually
> -             * encode speech on the center channel. Thus we have to
> -             * make sure this channel is distributed to L and R if not
> -             * available in the output. Also, LFE is used to achieve a
> -             * greater dynamic range, and thus we should try to do our
> -             * best to pass it to L+R.
> -             */
> -
> -            if (a == b || a == PA_CHANNEL_POSITION_MONO) {
> -                m->map_table_f[oc][ic] = 1.0;
> -
> -                oc_connected = TRUE;
> -                ic_connected[ic] = TRUE;
> -            }
> -            else if (b == PA_CHANNEL_POSITION_MONO) {
> -                if (n_ic)
> -                    m->map_table_f[oc][ic] = 1.0f / (float) n_ic;
> -
> -                oc_connected = TRUE;
> -                ic_connected[ic] = TRUE;
>              }
>          }
> +    } else {
> +        pa_assert(remix);
> +
> +        /* OK, we shall do the full monty: upmixing and downmixing. Our
> +         * algorithm is relatively simple, does not do spacialization, delay
> +         * elements or apply lowpass filters for LFE. Patches are always
> +         * welcome, though. Oh, and it doesn't do any matrix decoding. (Which
> +         * probably wouldn't make any sense anyway.)
> +         *
> +         * This code is not idempotent: downmixing an upmixed stereo stream is
> +         * not identical to the original. The volume will not match, and the
> +         * two channels will be a linear combination of both.
> +         *
> +         * This is loosely based on random suggestions found on the Internet,
> +         * such as this:
> +         * http://www.halfgaar.net/surround-sound-in-linux and the alsa upmix
> +         * plugin.
> +         *
> +         * The algorithm works basically like this:
> +         *
> +         * 1) Connect all channels with matching names.
> +         *
> +         * 2) Mono Handling:
> +         *    S:Mono: Copy into all D:channels
> +         *    D:Mono: Avg all S:channels
> +         *
> +         * 3) Mix D:Left, D:Right:
> +         *    D:Left: If not connected, avg all S:Left
> +         *    D:Right: If not connected, avg all S:Right
> +         *
> +         * 4) Mix D:Center
> +         *    If not connected, avg all S:Center
> +         *    If still not connected, avg all S:Left, S:Right
> +         *
> +         * 5) Mix D:LFE
> +         *    If not connected, avg all S:*
> +         *
> +         * 6) Make sure S:Left/S:Right is used: S:Left/S:Right: If not
> +         *    connected, mix into all D:left and all D:right channels. Gain is
> +         *    0.1, the current left and right should be multiplied by 0.9.
> +         *
> +         * 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.
> +         *
> +         * S: and D: shall relate to the source resp. destination channels.
> +         *
> +         * Rationale: 1, 2 are probably obvious. For 3: this copies front to
> +         * rear if needed. For 4: we try to find some suitable C source for C,
> +         * if we don't find any, we avg L and R. For 5: LFE is mixed from all
> +         * channels. For 6: the rear channels should not be dropped entirely,
> +         * however have only minimal impact. For 7: movies usually encode
> +         * speech on the center channel. Thus we have to make sure this channel
> +         * is distributed to L and R if not available in the output. Also, LFE
> +         * is used to achieve a greater dynamic range, and thus we should try
> +         * to do our best to pass it to L+R.
> +         */
>  
> -        if (!oc_connected && remix) {
> -            /* OK, we shall remix */
> +        for (ic = 0; ic < n_ic; ic++) {
> +            if (on_left(r->i_cm.map[ic]))
> +                ic_left++;
> +            if (on_right(r->i_cm.map[ic]))
> +                ic_right++;
> +            if (on_center(r->i_cm.map[ic]))
> +                ic_center++;
> +        }
>  
> -            /* Try to find matching input ports for this output port */
> +        for (oc = 0; oc < n_oc; oc++) {
> +            pa_bool_t oc_connected = FALSE;

While you're rewriting this function, you could also change every
pa_bool_t to bool, every TRUE to true and every FALSE to false.

> +            pa_channel_position_t b = r->o_cm.map[oc];
>  
> -            if (on_left(b)) {
> -                unsigned n = 0;
> +            for (ic = 0; ic < n_ic; ic++) {
> +                pa_channel_position_t a = r->i_cm.map[ic];
>  
> -                /* We are not connected and on the left side, let's
> -                 * average all left side input channels. */
> +                if (a == b || a == PA_CHANNEL_POSITION_MONO) {
> +                    m->map_table_f[oc][ic] = 1.0;
>  
> -                for (ic = 0; ic < n_ic; ic++)
> -                    if (on_left(r->i_cm.map[ic]))
> -                        n++;
> +                    oc_connected = TRUE;
> +                    ic_connected[ic] = TRUE;
> +                }
> +                else if (b == PA_CHANNEL_POSITION_MONO) {
> +                    if (n_ic)

Redundant check.

-- 
Tanu



[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux