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
