[pulseaudio-discuss] [PATCH 1/2] resampler: refactor calc_map_table()
Tanu Kaskinen
tanuk at iki.fi
Thu Feb 7 00:30:26 PST 2013
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
More information about the pulseaudio-discuss
mailing list