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

2012-03-08 Thread Tanu Kaskinen 
On Sun, 2012-01-08 at 22:04 +0100, Niels Ole Salscheider wrote:
 I have uploaded suitable impulse responses to:
 http://stuff.salscheider-online.de/hrir_kemar.tar.gz
 http://stuff.salscheider-online.de/hrir_listen.tar.gz
 
 The first archive contains an impulse response for the KEMAR dummy head and 
 is 
 quite small. If that impulse response does not work well for you, you can try 
 the listen database. This archive contains a folder demos that should help 
 you to find a suitable impulse response.

Do you have any advice for ranking the demos? Should I choose one that
appears to make the most perfect circle around my head?

-- 
Tanu

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

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

2012-03-05 Thread 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

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

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

2012-02-25 Thread Niels Ole Salscheider 
Hello,

 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 changed it.

 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.
[...]

You are right, the impulse response files did not use the extensible format. 
I changed this for my impulse responses and now it works as expected.
I think we should just require that the impulse response file uses the 
extensible format.

  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.

 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;
 }

Fixed.

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

Fixed.

I have attached a new version of the patch.

Regards,

OleFrom b97e66866ef7a21b874f858ca6a90804ce2470a6 Mon Sep 17 00:00:00 2001
From: Niels Ole Salscheider niels_...@salscheider-online.de
Date: Sun, 8 Jan 2012 21:22:35 +0100
Subject: [PATCH] add module-virtual-surround-sink

It provides a virtual surround sound effect

v2: Normalize hrir to avoid clipping, some cleanups
v3: use fabs, not abs
v4: implement changes proposed by Tanu Kaskinen
v5: likewise
v6: use channel map from hrir file
---
 src/Makefile.am|7 +
 ...rtual-sink.c = module-virtual-surround-sink.c} |  308 +++-
 2 files changed, 249 insertions(+), 66 deletions(-)
 copy src/modules/{module-virtual-sink.c = module-virtual-surround-sink.c} (68%)

diff --git a/src/Makefile.am b/src/Makefile.am
index 603ccc3..9aa31f4 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1019,6 +1019,7 @@ modlibexec_LTLIBRARIES += \
 		module-loopback.la \
 		module-virtual-sink.la \
 		module-virtual-source.la \
+		module-virtual-surround-sink.la \
 		module-switch-on-connect.la \
 		module-filter-apply.la \
 		module-filter-heuristics.la
@@ -1319,6 +1320,7 @@ SYMDEF_FILES = \
 		module-loopback-symdef.h \
 		module-virtual-sink-symdef.h \
 		module-virtual-source-symdef.h \
+		module-virtual-surround-sink-symdef.h \
 		module-switch-on-connect-symdef.h \
 		module-filter-apply-symdef.h \
 		module-filter-heuristics-symdef.h
@@ -1535,6 +1537,11 @@ module_virtual_source_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS)
 module_virtual_source_la_LDFLAGS = $(MODULE_LDFLAGS)
 module_virtual_source_la_LIBADD = $(MODULE_LIBADD)
 
+module_virtual_surround_sink_la_SOURCES = modules/module-virtual-surround-sink.c
+module_virtual_surround_sink_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS)
+module_virtual_surround_sink_la_LDFLAGS = $(MODULE_LDFLAGS)
+module_virtual_surround_sink_la_LIBADD = $(MODULE_LIBADD)
+
 # X11
 
 module_x11_bell_la_SOURCES = modules/x11/module-x11-bell.c
diff --git a/src/modules/module-virtual-sink.c b/src/modules/module-virtual-surround-sink.c
similarity index 68%
copy from src/modules/module-virtual-sink.c
copy to src/modules/module-virtual-surround-sink.c
index cf11ffa..656af7e 100644
--- a/src/modules/module-virtual-sink.c
+++ b/src/modules/module-virtual-surround-sink.c
@@ -3,6 +3,7 @@
 
 Copyright 2010 Intel Corporation
 Contributor: Pierre-Louis Bossart pierre-louis.boss...@intel.com
+Copyright 2012 Niels Ole Salscheider niels_...@salscheider-online.de
 
 PulseAudio is free software; you can redistribute it and/or modify
 it under the terms of the GNU Lesser General Public License as published
@@ -37,11 +38,15 @@
 #include pulsecore/rtpoll.h
 #include pulsecore/sample-util.h
 #include pulsecore/ltdl-helper.h
+#include pulsecore/sound-file.h
+#include pulsecore/resampler.h
 
-#include module-virtual-sink-symdef.h
+#include math.h
 
-PA_MODULE_AUTHOR(Pierre-Louis Bossart);
-PA_MODULE_DESCRIPTION(_(Virtual sink));
+#include module-virtual-surround-sink-symdef.h
+
+PA_MODULE_AUTHOR(Niels Ole Salscheider);
+PA_MODULE_DESCRIPTION(_(Virtual surround sink));
 PA_MODULE_VERSION(PACKAGE_VERSION);
 PA_MODULE_LOAD_ONCE(FALSE);
 PA_MODULE_USAGE(
@@ -54,6 +59,7 @@

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

2012-02-23 Thread Tanu Kaskinen 
On Fri, 2012-02-17 at 15:56 +0100, Niels Ole Salscheider wrote:
  I'd put this variable inside the innermost if, because it's not used
  elsewhere in this function.
 
 I think that is not allowed in C89/C90...

I think C89 allows variable declarations at the beginning of any block.
And even if it doesn't, variables are declared inside if blocks all over
Pulseaudio's code, so not doing it here won't achieve anything in terms
of complying with C89.

 That sounds sensible. I think at some point I got confused by
 sink_input being 
 the input for the master sink that accepts the output signal I
 compute.
 
 I hope I got it right now. I have attached a new version of my patch.

Thanks!

One question about the processing: does it add any latency?

 From 56ac4d4a95971818657202677bf0646b3626a2a4 Mon Sep 17 00:00:00 2001
 From: Niels Ole Salscheider niels_...@salscheider-online.de
 Date: Sun, 8 Jan 2012 21:22:35 +0100
 Subject: [PATCH] add module-virtual-surround-sink
 
 It provides a virtual surround sound effect
 
 v2: Normalize hrir to avoid clipping, some cleanups
 v3: use fabs, not abs
 v4: implement changes proposed by Tanu Kaskinen
 ---
  src/Makefile.am|7 +
  ...rtual-sink.c = module-virtual-surround-sink.c} |  344 
 
  2 files changed, 288 insertions(+), 63 deletions(-)
  copy src/modules/{module-virtual-sink.c = module-virtual-surround-sink.c} 
 (63%)
 
 diff --git a/src/Makefile.am b/src/Makefile.am
 index 521bf50..8f942f0 100644
 --- a/src/Makefile.am
 +++ b/src/Makefile.am
 @@ -1009,6 +1009,7 @@ modlibexec_LTLIBRARIES += \
 module-loopback.la \
 module-virtual-sink.la \
 module-virtual-source.la \
 +   module-virtual-surround-sink.la \
 module-switch-on-connect.la \
 module-filter-apply.la \
 module-filter-heuristics.la
 @@ -1309,6 +1310,7 @@ SYMDEF_FILES = \
 module-loopback-symdef.h \
 module-virtual-sink-symdef.h \
 module-virtual-source-symdef.h \
 +   module-virtual-surround-sink-symdef.h \
 module-switch-on-connect-symdef.h \
 module-filter-apply-symdef.h \
 module-filter-heuristics-symdef.h
 @@ -1525,6 +1527,11 @@ module_virtual_source_la_CFLAGS = $(AM_CFLAGS) 
 $(SERVER_CFLAGS)
  module_virtual_source_la_LDFLAGS = $(MODULE_LDFLAGS)
  module_virtual_source_la_LIBADD = $(MODULE_LIBADD)
  
 +module_virtual_surround_sink_la_SOURCES = 
 modules/module-virtual-surround-sink.c
 +module_virtual_surround_sink_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS)
 +module_virtual_surround_sink_la_LDFLAGS = $(MODULE_LDFLAGS)
 +module_virtual_surround_sink_la_LIBADD = $(MODULE_LIBADD)
 +
  # X11
  
  module_x11_bell_la_SOURCES = modules/x11/module-x11-bell.c
 diff --git a/src/modules/module-virtual-sink.c 
 b/src/modules/module-virtual-surround-sink.c
 similarity index 63%
 copy from src/modules/module-virtual-sink.c
 copy to src/modules/module-virtual-surround-sink.c
 index cf11ffa..d3dfe6f 100644
 --- a/src/modules/module-virtual-sink.c
 +++ b/src/modules/module-virtual-surround-sink.c
 @@ -3,6 +3,7 @@
  
  Copyright 2010 Intel Corporation
  Contributor: Pierre-Louis Bossart pierre-louis.boss...@intel.com
 +Copyright 2012 Niels Ole Salscheider niels_...@salscheider-online.de
  
  PulseAudio is free software; you can redistribute it and/or modify
  it under the terms of the GNU Lesser General Public License as published
 @@ -37,11 +38,15 @@
  #include pulsecore/rtpoll.h
  #include pulsecore/sample-util.h
  #include pulsecore/ltdl-helper.h
 +#include pulsecore/sound-file.h
 +#include pulsecore/resampler.h
  
 -#include module-virtual-sink-symdef.h
 +#include math.h
  
 -PA_MODULE_AUTHOR(Pierre-Louis Bossart);
 -PA_MODULE_DESCRIPTION(_(Virtual sink));
 +#include module-virtual-surround-sink-symdef.h
 +
 +PA_MODULE_AUTHOR(Niels Ole Salscheider);
 +PA_MODULE_DESCRIPTION(_(Virtual surround sink));
  PA_MODULE_VERSION(PACKAGE_VERSION);
  PA_MODULE_LOAD_ONCE(FALSE);
  PA_MODULE_USAGE(
 @@ -54,6 +59,8 @@ PA_MODULE_USAGE(
channel_map=channel map 
use_volume_sharing=yes or no 
force_flat_volume=yes or no 
 +  hrir=/path/to/left_hrir.wav 
 +  hrir_channel_map=channel map 
  ));
  
  #define MEMBLOCKQ_MAXLENGTH (16*1024*1024)
 @@ -71,6 +78,23 @@ struct userdata {
  
  pa_bool_t auto_desc;
  unsigned channels;
 +unsigned hrir_channels;
 +unsigned master_channels;
 +
 +unsigned fs, sink_fs;
 +
 +unsigned *mapping_left;
 +unsigned *mapping_right;
 +
 +unsigned output_left, output_right;
 +
 +unsigned hrir_samples;
 +pa_sample_spec hrir_ss;
 +pa_channel_map hrir_map;
 +pa_memchunk hrir_chunk;
 +
 +pa_memchunk input_buffer_chunk;

The hrir data and the input buffer don't really need to be stored inside
memchunks. They could be just

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

2012-02-23 Thread Niels Ole Salscheider 
Hello,

 One question about the processing: does it add any latency?

That depends on the impulse response. In general, the current output sample is 
a weighted sum of the current input sample and some preceding.
Provided that the first values of the impulse response are not equal (or 
approximately) zero there is no added latency.

 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.

 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?

 Pulseaudio's convention is one more indentation level for the cases
 inside the switch.

 You need to check that hrir_file isn't NULL.

 The hrir_temp_chunk memblock needs to be unreffed also in the fail
 section if it's non-NULL to avoid memory leaks. For that reason the
 memblock pointer also has to be set to NULL here and in the beginning of
 the function.

All fixed. I have attached an updated patch.

 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?

Regards,

OleFrom c124c11f82051a835e695b94392eba8b63e27254 Mon Sep 17 00:00:00 2001
From: Niels Ole Salscheider niels_...@salscheider-online.de
Date: Sun, 8 Jan 2012 21:22:35 +0100
Subject: [PATCH] add module-virtual-surround-sink

It provides a virtual surround sound effect

v2: Normalize hrir to avoid clipping, some cleanups
v3: use fabs, not abs
v4: implement changes proposed by Tanu Kaskinen
v5: likewise
---
 src/Makefile.am|7 +
 ...rtual-sink.c = module-virtual-surround-sink.c} |  321 
 2 files changed, 265 insertions(+), 63 deletions(-)
 copy src/modules/{module-virtual-sink.c = module-virtual-surround-sink.c} (66%)

diff --git a/src/Makefile.am b/src/Makefile.am
index 603ccc3..9aa31f4 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1019,6 +1019,7 @@ modlibexec_LTLIBRARIES += \
 		module-loopback.la \
 		module-virtual-sink.la \
 		module-virtual-source.la \
+		module-virtual-surround-sink.la \
 		module-switch-on-connect.la \
 		module-filter-apply.la \
 		module-filter-heuristics.la
@@ -1319,6 +1320,7 @@ SYMDEF_FILES = \
 		module-loopback-symdef.h \
 		module-virtual-sink-symdef.h \
 		module-virtual-source-symdef.h \
+		module-virtual-surround-sink-symdef.h \
 		module-switch-on-connect-symdef.h \
 		module-filter-apply-symdef.h \
 		module-filter-heuristics-symdef.h
@@ -1535,6 +1537,11 @@ module_virtual_source_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS)
 module_virtual_source_la_LDFLAGS = $(MODULE_LDFLAGS)
 module_virtual_source_la_LIBADD = $(MODULE_LIBADD)
 
+module_virtual_surround_sink_la_SOURCES = modules/module-virtual-surround-sink.c
+module_virtual_surround_sink_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS)
+module_virtual_surround_sink_la_LDFLAGS = $(MODULE_LDFLAGS)
+module_virtual_surround_sink_la_LIBADD = $(MODULE_LIBADD)
+
 # X11
 
 module_x11_bell_la_SOURCES = modules/x11/module-x11-bell.c
diff --git a/src/modules/module-virtual-sink.c b/src/modules/module-virtual-surround-sink.c
similarity index 66%
copy from src/modules/module-virtual-sink.c
copy to src/modules/module-virtual-surround-sink.c
index cf11ffa..78c354e 100644
--- a/src/modules/module-virtual-sink.c
+++ b/src/modules/module-virtual-surround-sink.c
@@ -3,6 +3,7 @@
 
 Copyright 2010 Intel Corporation
 Contributor: Pierre-Louis Bossart pierre-louis.boss...@intel.com
+Copyright 2012 Niels Ole Salscheider niels_...@salscheider-online.de
 
 PulseAudio is free software; you can redistribute it and/or modify
 it under the terms of the GNU Lesser General Public License as published
@@ -37,11 +38,15 @@
 #include pulsecore/rtpoll.h
 #include pulsecore/sample-util.h
 #include pulsecore/ltdl-helper.h
+#include pulsecore/sound-file.h
+#include pulsecore/resampler.h
 
-#include module-virtual-sink-symdef.h
+#include math.h
 
-PA_MODULE_AUTHOR(Pierre-Louis Bossart);
-PA_MODULE_DESCRIPTION(_(Virtual sink));
+#include module-virtual-surround-sink-symdef.h
+
+PA_MODULE_AUTHOR(Niels Ole Salscheider);
+PA_MODULE_DESCRIPTION(_(Virtual surround sink));

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

2012-02-17 Thread Niels Ole Salscheider 
Hello,

 First complaint: doesn't your git nag about trailing whitespace?

It seems that this was not enabled in my git config (although I cannot remember 
disabling it). I have fixed the mentioned lines.

 current_latency is in module-virtual-sink only for instructive purposes.
 If it's not needed in the filter, it should be removed from the code.

 I think a switch would be nicer than if here.

Both fixed.

 I'd put this variable inside the innermost if, because it's not used
 elsewhere in this function.

I think that is not allowed in C89/C90...

[...]
 My preferences for selecting the channel map and sample spec for the
 sink input and sink are the following:
 
 Sink input:
 Channel map: Always stereo.
 Sample format: Always float32ne.
 Sample rate: Always match the filter sink.
 
 Sink:
 Channel map: Default to the impulse response file channel map. Allow the
 user to override that default with the channels and/or channel_map
 module arguments. pa_modargs_get_sample_spec_and_channel_map() will take
 care of initializing the channel map appropriately.
 Sample format: Always float32ne.
 Sample rate: I'm not quite sure about this. I guess defaulting to the
 master sink rate would be best. The other alternative would be
 defaulting to the impulse response file's rate. The user should be able
 to override the default with the rate module argument.

That sounds sensible. I think at some point I got confused by sink_input being 
the input for the master sink that accepts the output signal I compute.

I hope I got it right now. I have attached a new version of my patch.

Regards,

OleFrom 56ac4d4a95971818657202677bf0646b3626a2a4 Mon Sep 17 00:00:00 2001
From: Niels Ole Salscheider niels_...@salscheider-online.de
Date: Sun, 8 Jan 2012 21:22:35 +0100
Subject: [PATCH] add module-virtual-surround-sink

It provides a virtual surround sound effect

v2: Normalize hrir to avoid clipping, some cleanups
v3: use fabs, not abs
v4: implement changes proposed by Tanu Kaskinen
---
 src/Makefile.am|7 +
 ...rtual-sink.c = module-virtual-surround-sink.c} |  344 
 2 files changed, 288 insertions(+), 63 deletions(-)
 copy src/modules/{module-virtual-sink.c = module-virtual-surround-sink.c} (63%)

diff --git a/src/Makefile.am b/src/Makefile.am
index 521bf50..8f942f0 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1009,6 +1009,7 @@ modlibexec_LTLIBRARIES += \
 		module-loopback.la \
 		module-virtual-sink.la \
 		module-virtual-source.la \
+		module-virtual-surround-sink.la \
 		module-switch-on-connect.la \
 		module-filter-apply.la \
 		module-filter-heuristics.la
@@ -1309,6 +1310,7 @@ SYMDEF_FILES = \
 		module-loopback-symdef.h \
 		module-virtual-sink-symdef.h \
 		module-virtual-source-symdef.h \
+		module-virtual-surround-sink-symdef.h \
 		module-switch-on-connect-symdef.h \
 		module-filter-apply-symdef.h \
 		module-filter-heuristics-symdef.h
@@ -1525,6 +1527,11 @@ module_virtual_source_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS)
 module_virtual_source_la_LDFLAGS = $(MODULE_LDFLAGS)
 module_virtual_source_la_LIBADD = $(MODULE_LIBADD)
 
+module_virtual_surround_sink_la_SOURCES = modules/module-virtual-surround-sink.c
+module_virtual_surround_sink_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS)
+module_virtual_surround_sink_la_LDFLAGS = $(MODULE_LDFLAGS)
+module_virtual_surround_sink_la_LIBADD = $(MODULE_LIBADD)
+
 # X11
 
 module_x11_bell_la_SOURCES = modules/x11/module-x11-bell.c
diff --git a/src/modules/module-virtual-sink.c b/src/modules/module-virtual-surround-sink.c
similarity index 63%
copy from src/modules/module-virtual-sink.c
copy to src/modules/module-virtual-surround-sink.c
index cf11ffa..d3dfe6f 100644
--- a/src/modules/module-virtual-sink.c
+++ b/src/modules/module-virtual-surround-sink.c
@@ -3,6 +3,7 @@
 
 Copyright 2010 Intel Corporation
 Contributor: Pierre-Louis Bossart pierre-louis.boss...@intel.com
+Copyright 2012 Niels Ole Salscheider niels_...@salscheider-online.de
 
 PulseAudio is free software; you can redistribute it and/or modify
 it under the terms of the GNU Lesser General Public License as published
@@ -37,11 +38,15 @@
 #include pulsecore/rtpoll.h
 #include pulsecore/sample-util.h
 #include pulsecore/ltdl-helper.h
+#include pulsecore/sound-file.h
+#include pulsecore/resampler.h
 
-#include module-virtual-sink-symdef.h
+#include math.h
 
-PA_MODULE_AUTHOR(Pierre-Louis Bossart);
-PA_MODULE_DESCRIPTION(_(Virtual sink));
+#include module-virtual-surround-sink-symdef.h
+
+PA_MODULE_AUTHOR(Niels Ole Salscheider);
+PA_MODULE_DESCRIPTION(_(Virtual surround sink));
 PA_MODULE_VERSION(PACKAGE_VERSION);
 PA_MODULE_LOAD_ONCE(FALSE);
 PA_MODULE_USAGE(
@@ -54,6 +59,8 @@ PA_MODULE_USAGE(
   channel_map=channel map 
   use_volume_sharing=yes or no 
   force_flat_volume=yes or no 
+  hrir=/path/to/left_hrir.wav 
+  hrir_channel_map=channel map 
 ));
 
 #define MEMBLOCKQ_MAXLENGTH

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

2012-02-12 Thread Tanu Kaskinen 
On Sun, 2012-01-22 at 10:53 +0100, Niels Ole Salscheider wrote:
  2) Why should we do this advanced downmixing in an optional module?
  I.e., why not just replace the default downmixing algorighm without
  creating a module for that?
 
 Replacing the default downmixing algorithm is not a good idea since this one 
 only works for headphones. And you might want to use a simpler one on a low 
 end machine since folding needs some cpu cycles.
 But if there is a better place to implement this, I am fine with it, too.

For now, a separate filter sink is probably the best solution (or at
least the easiest one). It would be nice to have configurable downmixing
algorithms, though.

 I forgot to add a small fix to my last patch; I have attached a new version.

Thanks for the patch! Sounds like a very cool feature, if it works well
(I haven't tested yet). Code review below. Note that I'm not good at
signal processing algorithms, and I couldn't quite understand what the
actual processing code does (I'm pretty sure that it does a thing called
convolution, but I couldn't verify that it does it correctly or well).
I trust that it's perfect :)

First complaint: doesn't your git nag about trailing whitespace? When
trying to commit the patch, I get:

src/modules/module-virtual-surround-sink.c:232: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:521: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:524: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:527: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:530: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:533: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:536: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:539: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:542: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:545: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:548: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:551: trailing whitespace.
+
src/modules/module-virtual-surround-sink.c:554: trailing whitespace.
+

 From 4e8a09d44ca8df7a7cc32662d0a784c77f09523c Mon Sep 17 00:00:00 2001
 From: Niels Ole Salscheider niels_...@salscheider-online.de
 Date: Sun, 8 Jan 2012 21:22:35 +0100
 Subject: [PATCH] add module-virtual-surround-sink
 
 It provides a virtual surround sound effect
 
 v2: Normalize hrir to avoid clipping, some cleanups
 v3: use fabs, not abs
 ---
  src/Makefile.am|7 +
  ...rtual-sink.c = module-virtual-surround-sink.c} |  272
 +---
  2 files changed, 246 insertions(+), 33 deletions(-)
  copy src/modules/{module-virtual-sink.c =
 module-virtual-surround-sink.c} (69%)
 
 diff --git a/src/Makefile.am b/src/Makefile.am
 index 521bf50..8f942f0 100644
 --- a/src/Makefile.am
 +++ b/src/Makefile.am
 @@ -1009,6 +1009,7 @@ modlibexec_LTLIBRARIES += \
 module-loopback.la \
 module-virtual-sink.la \
 module-virtual-source.la \
 +   module-virtual-surround-sink.la \
 module-switch-on-connect.la \
 module-filter-apply.la \
 module-filter-heuristics.la
 @@ -1309,6 +1310,7 @@ SYMDEF_FILES = \
 module-loopback-symdef.h \
 module-virtual-sink-symdef.h \
 module-virtual-source-symdef.h \
 +   module-virtual-surround-sink-symdef.h \
 module-switch-on-connect-symdef.h \
 module-filter-apply-symdef.h \
 module-filter-heuristics-symdef.h
 @@ -1525,6 +1527,11 @@ module_virtual_source_la_CFLAGS = $(AM_CFLAGS)
 $(SERVER_CFLAGS)
  module_virtual_source_la_LDFLAGS = $(MODULE_LDFLAGS)
  module_virtual_source_la_LIBADD = $(MODULE_LIBADD)
  
 +module_virtual_surround_sink_la_SOURCES =
 modules/module-virtual-surround-sink.c
 +module_virtual_surround_sink_la_CFLAGS = $(AM_CFLAGS)
 $(SERVER_CFLAGS)
 +module_virtual_surround_sink_la_LDFLAGS = $(MODULE_LDFLAGS)
 +module_virtual_surround_sink_la_LIBADD = $(MODULE_LIBADD)
 +
  # X11
  
  module_x11_bell_la_SOURCES = modules/x11/module-x11-bell.c
 diff --git a/src/modules/module-virtual-sink.c
 b/src/modules/module-virtual-surround-sink.c
 similarity index 69%
 copy from src/modules/module-virtual-sink.c
 copy to src/modules/module-virtual-surround-sink.c
 index e7c476e..b3100e4 100644
 --- a/src/modules/module-virtual-sink.c
 +++ b/src/modules/module-virtual-surround-sink.c
 @@ -3,6 +3,7 @@
  
  Copyright 2010 Intel Corporation
  Contributor: Pierre-Louis Bossart
 pierre-louis.boss...@intel.com
 +Copyright 2012 Niels Ole Salscheider
 niels_...@salscheider-online.de
  
  PulseAudio is free software; you can redistribute it and/or
 modify
  it under the terms of the GNU Lesser General Public License as
 published
 @@

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

2012-01-22 Thread Alexander E. Patrakov 
Niels Ole Salscheider niels_...@salscheider-online.de wrote:

 Hello,
 
 this version normalizes the hrir to avoid clipping and contains some
 minor cleanups.

Some stupid questions:

1) Is the license of the HRTF dataset that you use compatible with GPL?

2) Why should we do this advanced downmixing in an optional module?
I.e., why not just replace the default downmixing algorighm without
creating a module for that?

-- 
Alexander E. Patrakov

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

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

2012-01-22 Thread Niels Ole Salscheider 
Hello,

 1) Is the license of the HRTF dataset that you use compatible with GPL?

The Listen HRIR dataset is public domain so it is GPL compatible.

The HRIR based on the Kemar dummy head is provided free with no restrictions 
on use, provided the authors are cited when the data is used in any research 
or commercial application.
But there is no need to distribute all HRIRs with pulseaudio. I think a few 
good that are GPL compatible are enough.

 2) Why should we do this advanced downmixing in an optional module?
 I.e., why not just replace the default downmixing algorighm without
 creating a module for that?

Replacing the default downmixing algorithm is not a good idea since this one 
only works for headphones. And you might want to use a simpler one on a low 
end machine since folding needs some cpu cycles.
But if there is a better place to implement this, I am fine with it, too.

I forgot to add a small fix to my last patch; I have attached a new version.

Regards,

OleFrom 4e8a09d44ca8df7a7cc32662d0a784c77f09523c Mon Sep 17 00:00:00 2001
From: Niels Ole Salscheider niels_...@salscheider-online.de
Date: Sun, 8 Jan 2012 21:22:35 +0100
Subject: [PATCH] add module-virtual-surround-sink

It provides a virtual surround sound effect

v2: Normalize hrir to avoid clipping, some cleanups
v3: use fabs, not abs
---
 src/Makefile.am|7 +
 ...rtual-sink.c = module-virtual-surround-sink.c} |  272 +---
 2 files changed, 246 insertions(+), 33 deletions(-)
 copy src/modules/{module-virtual-sink.c = module-virtual-surround-sink.c} (69%)

diff --git a/src/Makefile.am b/src/Makefile.am
index 521bf50..8f942f0 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1009,6 +1009,7 @@ modlibexec_LTLIBRARIES += \
 		module-loopback.la \
 		module-virtual-sink.la \
 		module-virtual-source.la \
+		module-virtual-surround-sink.la \
 		module-switch-on-connect.la \
 		module-filter-apply.la \
 		module-filter-heuristics.la
@@ -1309,6 +1310,7 @@ SYMDEF_FILES = \
 		module-loopback-symdef.h \
 		module-virtual-sink-symdef.h \
 		module-virtual-source-symdef.h \
+		module-virtual-surround-sink-symdef.h \
 		module-switch-on-connect-symdef.h \
 		module-filter-apply-symdef.h \
 		module-filter-heuristics-symdef.h
@@ -1525,6 +1527,11 @@ module_virtual_source_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS)
 module_virtual_source_la_LDFLAGS = $(MODULE_LDFLAGS)
 module_virtual_source_la_LIBADD = $(MODULE_LIBADD)
 
+module_virtual_surround_sink_la_SOURCES = modules/module-virtual-surround-sink.c
+module_virtual_surround_sink_la_CFLAGS = $(AM_CFLAGS) $(SERVER_CFLAGS)
+module_virtual_surround_sink_la_LDFLAGS = $(MODULE_LDFLAGS)
+module_virtual_surround_sink_la_LIBADD = $(MODULE_LIBADD)
+
 # X11
 
 module_x11_bell_la_SOURCES = modules/x11/module-x11-bell.c
diff --git a/src/modules/module-virtual-sink.c b/src/modules/module-virtual-surround-sink.c
similarity index 69%
copy from src/modules/module-virtual-sink.c
copy to src/modules/module-virtual-surround-sink.c
index e7c476e..b3100e4 100644
--- a/src/modules/module-virtual-sink.c
+++ b/src/modules/module-virtual-surround-sink.c
@@ -3,6 +3,7 @@
 
 Copyright 2010 Intel Corporation
 Contributor: Pierre-Louis Bossart pierre-louis.boss...@intel.com
+Copyright 2012 Niels Ole Salscheider niels_...@salscheider-online.de
 
 PulseAudio is free software; you can redistribute it and/or modify
 it under the terms of the GNU Lesser General Public License as published
@@ -40,11 +41,15 @@
 #include pulsecore/rtpoll.h
 #include pulsecore/sample-util.h
 #include pulsecore/ltdl-helper.h
+#include pulsecore/sound-file.h
+#include pulsecore/resampler.h
 
-#include module-virtual-sink-symdef.h
+#include math.h
 
-PA_MODULE_AUTHOR(Pierre-Louis Bossart);
-PA_MODULE_DESCRIPTION(_(Virtual sink));
+#include module-virtual-surround-sink-symdef.h
+
+PA_MODULE_AUTHOR(Niels Ole Salscheider);
+PA_MODULE_DESCRIPTION(_(Virtual surround sink));
 PA_MODULE_VERSION(PACKAGE_VERSION);
 PA_MODULE_LOAD_ONCE(FALSE);
 PA_MODULE_USAGE(
@@ -57,6 +62,8 @@ PA_MODULE_USAGE(
   channel_map=channel map 
   use_volume_sharing=yes or no 
   force_flat_volume=yes or no 
+  hrir=/path/to/left_hrir.wav 
+  hrir_channel_map=channel map 
 ));
 
 #define MEMBLOCKQ_MAXLENGTH (16*1024*1024)
@@ -74,6 +81,21 @@ struct userdata {
 
 pa_bool_t auto_desc;
 unsigned channels;
+unsigned hrir_channels;
+unsigned master_channels;
+
+unsigned *mapping_left;
+unsigned *mapping_right;
+
+unsigned output_left, output_right;
+
+unsigned hrir_samples;
+pa_sample_spec hrir_ss;
+pa_channel_map hrir_map;
+pa_memchunk hrir_chunk;
+
+pa_memchunk input_buffer_chunk;
+int input_buffer_offset;
 };
 
 static const char* const valid_modargs[] = {
@@ -86,6 +108,8 @@ static const char* const valid_modargs[] = {
 channel_map,
 use_volume_sharing,
 force_flat_volume,
+hrir,
+hrir_channel_map,

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

2012-01-22 Thread Alexander E. Patrakov 
Niels Ole Salscheider niels_...@salscheider-online.de wrote:

 Hello,
 
  1) Is the license of the HRTF dataset that you use compatible with
  GPL?
 
 The Listen HRIR dataset is public domain so it is GPL compatible.

OK
 
  2) Why should we do this advanced downmixing in an optional module?
  I.e., why not just replace the default downmixing algorighm without
  creating a module for that?
 
 Replacing the default downmixing algorithm is not a good idea since
 this one only works for headphones. And you might want to use a
 simpler one on a low end machine since folding needs some cpu cycles.
 But if there is a better place to implement this, I am fine with it,
 too.

insane requirements follow, feel free to ignore for now

OK, so in the future we need a different algorithm for desktop speakers.
Something like find out stereo sound that, when convolved with the
HRIR and summed for each ear, produces the same result as the original
5.1 sound convolved with the appropriate HRIR, but at high-enough
frequencies only.

As for the complexity - yes, it can be reduced substantially, because
you use the simplest possible implementation of convolution with a
rather long filter. Please try to use FFT-based convolution and
benchmark.

Or, even better, try to approximate one of the available HRIRs with a
combination of an IIR filter of some low (4-6) order and a
fixed-per-channel delay, and hard-code that. As there is no scientific
way of designing IIR filters with arbitrary impulse response yet, the
simplest possible way of doing such approximation is to autogenerate
random IIR filters of a given order, compare their response with the
desired one, and leave the whole thing running for a day or so until it
finds something suitable.

-- 
Alexander E. Patrakov

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

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

2012-01-22 Thread Niels Ole Salscheider 
Hello,

 OK, so in the future we need a different algorithm for desktop speakers.
 Something like find out stereo sound that, when convolved with the
 HRIR and summed for each ear, produces the same result as the original
 5.1 sound convolved with the appropriate HRIR, but at high-enough
 frequencies only.

That is possible with crosstalk cancellation. It requires knowledge of the 
transfer functions from each speaker to each ear, though. I have no idea how 
well it would work if we just estimate the transfer functions but there seem 
to be commercial products that do so.

 As for the complexity - yes, it can be reduced substantially, because
 you use the simplest possible implementation of convolution with a
 rather long filter. Please try to use FFT-based convolution and
 benchmark.
 
 Or, even better, try to approximate one of the available HRIRs with a
 combination of an IIR filter of some low (4-6) order and a
 fixed-per-channel delay, and hard-code that. As there is no scientific
 way of designing IIR filters with arbitrary impulse response yet, the
 simplest possible way of doing such approximation is to autogenerate
 random IIR filters of a given order, compare their response with the
 desired one, and leave the whole thing running for a day or so until it
 finds something suitable.

Sure, my code is O(n^2) while FFT is O(n log n) but it is somewhat simpler and 
has less overhead.
I will try your proposed alternatives when I find the time to do so (exams are 
coming up).

Regards,

Ole

signature.asc
Description: This is a digitally signed message part.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss