[pulseaudio-discuss] [PATCH] add module-virtual-surround-sink

[Tanu Kaskinen]

Hi!

Sorry for the delay in reviewing...

One minor thing that could be fixed is the naming (sink name and
description, and sink input description). Currently they are copied from
module-virtual-sink, and they could be modified to include the word
"surround" in some way.

On Sat, 2012-02-25 at 13:32 +0100, Niels Ole Salscheider wrote:
> > > > And aren't hrir_ss and hrir_map redundant
> > > > anyway - shouldn't the hrir sample spec and channel map be the same as
> > > > what the sink has?
> > 
> > I do not think that they are redundant. The sink's sample spec and channel map 
> > default to the hrir's but they still can be overwritten by the user.
> > 
> > However, I could modify the hrir_data afterwards so that they match if you 
> > prefer that solution.

That was my idea, but I'm not so sure about that anymore. I'm not
confident that it's safe to remap the hrir contents. But what is the use
case for having a different channel map for the sink than what is in the
hrir file? To me it would sound sensible to just use the hrir file
channel map for the sink, and drop the configurability. I know that I
recommended earlier to have the channel map configurable...

Hmm... The main motivation for me to push for matching sink and hrir
channel maps has been that I'd like to get rid of the mapping_left and
mapping_right variables, but actually I think it's not possible to
achieve that, because whatever the channel map, it still has to be
treated differently for the left and the right ear... So, in conclusion,
feel free to keep the code as is, if you want to keep the
configurability.

A couple of comments still below.

> +    if (hrir_map.channels != u->hrir_channels) {
> +        pa_log("number of channels in hrir file does not match number of channels in hrir channel map");
> +        goto fail;
> +    }

I don't think this error can happen anymore. Both hrir_map.channels and
u->hrir_channels seem to originate from pa_sound_file_load(), and that
function shouldn't return inconsistent sample specs and channel maps
(otherwise it's buggy).

> +    /* create mapping between hrir and input */
> +    u->mapping_left = (unsigned *) pa_xnew0(unsigned, u->channels);
> +    u->mapping_right = (unsigned *) pa_xnew0(unsigned, u->channels);
> +    for (i = 0; i < map.channels; i++) {
> +        found_channel = 0;
> +
> +        for (j = 0; j < hrir_map.channels; j++) {
> +            if (hrir_map.map[j] == map.map[i]) {
> +                u->mapping_left[i] = j;
> +                found_channel = 1;
> +            }
> +
> +            if (hrir_map.map[j] == mirror_channel(map.map[i]))
> +                u->mapping_right[i] = j;
> +        }
> +
> +        if (!found_channel) {
> +            pa_log("Cannot find mapping for channel %s", pa_channel_position_to_string(map.map[i]));
> +            goto fail;
> +        }
> +    }

I think this is buggy. I think that even if you successfully find a
channel for each item in mapping_left, some channels can be left
uninitialized in mapping_right, if the hrir file doesn't have a
"symmetrical" channel map. So, it should be checked that a channel has
been found for both mappings.

-- 
Tanu