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

[Tanu Kaskinen]

On Thu, 2012-02-23 at 21:56 +0100, Niels Ole Salscheider wrote:
> > The hrir data and the input buffer don't really need to be stored inside
> > memchunks. They could be just arrays of floats.
> 
> I fixed this for the input buffer but "pa_resampler_run" stores the hrir data 
> inside a memchunk.

The resampling is only done at initialization time, so you could use the
memchunk only locally in pa__init() and copy the data to a plain float
array in userdata. But I'm not saying that that is necessarily better -
if you prefer storing the memchunk in userdata, that's ok.

> > I guess master_channels is unneeded now that it's hardcoded to be 2. And
> > this piece of code can be simplified to be just two assignments to dst.
> 
> Fixed. Is it ok to assume that the first channel is left and the second is 
> right or do I need to get this information from the sink input sample spec?

The sink input channel map is controlled by you, and you initialize it
with pa_channel_map_init_stereo(), which sets the first channel to left
and the second channel to right. So yes, you can assume that the first
channel is left.

> > Wouldn't it be better to default to the hrir file channel map? In that
> > case the file reading needs to be done before initializing the sink
> > channel map.
> 
> > Shouldn't the default hrir channel map be hrir_temp_map, i.e. the
> > channel map of the file? 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?
> 
> That is what I would like to do, too. But the channel map that is returned by 
> "pa_sound_file_load" is wrong. I am not even sure if there is a way to store 
> the channel map in the wav header.
> Because of that I provided a way to specify the channel map of the hrir wav. 
> Is there a better solution?

I had a look at some WAVE documentation[1], and it seems that the file
may or may not have the channel map information. The channel map
information is included with files that use the "extensible format". The
documentation says that the extensible format should be used whenever
the file contains more than 2 channels. It's still not a strict
requirement.

If you're getting an incorrect channel map from pa_sound_file_load(), it
would be nice to check whether the file contains the channel map (i.e.
uses the extensible format). If the file contains the channel map, then
pa_sound_file_load() (or libsndfile) contains a bug that needs to be
fixed.

In any case, I think you should in general trust pa_sound_file_load() to
provide a correct channel map, and use that as the default for the sink
channel map. If it really is so that the common case is that the correct
channel map is left,right,center,lfe,rear-left,rear-right and
pa_sound_file_load() can't detect that, then I guess it's ok to
overwrite the channel map with the hardcoded values. Figuring out the
channel map (i.e. loading the file and maybe overriding the channel map
with the hardcoded values) should, IMO, in any case be done before the
sink channel map setup, so that the hrir channel map can be used as the
default channel map for the sink.

(Interesting fact that I didn't know: WAVE files can contain audio in
pretty much any format, e.g. mp3.)

[1] http://www-mmsp.ece.mcgill.ca/Documents/AudioFormats/WAVE/WAVE.html

> +    /* Initialize hrir and input buffer */
> +    /* this is the hrir file for the left ear! */
> +    hrir_file = pa_modargs_get_value(ma, "hrir", NULL);
> +
> +    if (hrir_file && pa_sound_file_load(master->core->mempool, hrir_file, &hrir_temp_ss, &hrir_temp_map, &hrir_temp_chunk, NULL) < 0) {
> +        pa_log("Cannot load hrir file.");
> +        goto fail;
> +    }

This isn't really good enough for handling NULL hrir_file. If hrir_file
is NULL, pa_sound_file_load() won't be called, and that's not good.
There should be something like this:

if (!(hrir_file = pa_modargs_get_value(ma, "hrir", NULL))) {
    pa_log("The mandatory 'hrir' module argument is missing.");
    goto fail;
}

> +    /* resample hrir */
> +    resampler = pa_resampler_new(u->sink->core->mempool, &hrir_temp_ss, &u->hrir_map, &u->hrir_ss, &u->hrir_map,
> +                                 PA_RESAMPLER_SRC_SINC_BEST_QUALITY, PA_RESAMPLER_NO_REMAP);
> +    pa_resampler_run(resampler, &hrir_temp_chunk, &u->hrir_chunk);
> +    pa_resampler_free(resampler);
> +    pa_memblock_unref(hrir_temp_chunk.memblock);

hrir_temp_chunk.memblock needs to be set to NULL here, otherwise in case
of failure it will be unreffed twice.

-- 
Tanu