Re: [pulseaudio-discuss] [PATCH] add module-virtual-surround-sink
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
> 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
> +Copyright 2012 Niels Ole Salscheider
>
> 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
> #include
> #include
> +#include
> +#include
>
> -#include "module-virtual-sink-symdef.h"
> +#include
>
> -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= "
>"use_volume_sharing= "
>"force_flat_volume= "
> + "hrir=/path/to/left_hrir.wav "
> + "hrir_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 arrays of floats.
> +int input_buffer_offset;
> };
>
> static const char* const valid
Re: [pulseaudio-discuss] [PATCH] add module-virtual-surround-sink
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,
Ole>From c124c11f82051a835e695b94392eba8b63e27254 Mon Sep 17 00:00:00 2001
From: Niels Ole Salscheider
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
+Copyright 2012 Niels Ole Salscheider
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
#include
#include
+#include
+#include
-#include "module-virtual-sink-symdef.h"
+#include
-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= "
"use_volume
Re: [pulseaudio-discuss] [PATCH] UCM patches on ubuntu 1:1.1-0ubuntu4
On 02/21/2012 04:34 AM, Feng Wei wrote: Hi Arun, I'm not clear what should I do to upstream patches. I tested them on ubuntu, so that I must follow what David had done in port structure. In my original mind, I will first upstream to ubuntu, then pulseaudio community. My current patches are maintained in bzr according to ubuntu, I want them to be merged in ubuntu branch. Hi Feng, Sorry I haven't responded earlier, but I'm not sure what to do about these patches either. As I see it there are at least three problems that need to be resolved: * The competing implementation problem: We've had multiple implementations posted to the PulseAudio mailinglist, one by Janos and Jaska, one by Alejandro and Margarita (probably discontinued?), and one by yourself. It would be great if the UCM community could give us a hint on why we should choose one over another. * The patches are based on an older version of PulseAudio - the one that uses the input devices for jack detection. Ubuntu 12.04, as well as PulseAudio 2.0, will release with the new kcontrol jack detection interface [1]. In essence, your patches do not apply, and should we consider these for Ubuntu 12.04 and/or PulseAudio 2.0 we're in quite a hurry. Perhaps it's even too late, I don't know. * Verification and testing is difficult, for a variety of reasons (this point is not a real blocker like the other two, just a little cumbersome): 1) Requires special hardware. I could probably get hold of some hardware if that was the only thing keeping me from reviewing it though. 2) Requires special configuration files, which are usually kept private by companies. I assume that the files you have used for imx53 and omap are public though (where can they be found)? 3) I *was* going to complain about the lack of UCM documentation, but it seems like running "make doc" in alsa-lib 1.0.25 does create a UCM page (called "case interface"), and while it does not look perfect everywhere it seems like most things can actually be figured out. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic [1] Assuming my recently posted patches pass the review of my fellow developers on this list :-) ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] UCM patches on ubuntu 1:1.1-0ubuntu4
Hi David, I'm appreciated for your comments. UCM really has a long way to go. 2012/2/24 David Henningsson : > On 02/21/2012 04:34 AM, Feng Wei wrote: >> >> Hi Arun, >> I'm not clear what should I do to upstream patches. I tested them on >> ubuntu, so that I must follow what David had done in port structure. >> In my original mind, I will first upstream to ubuntu, then pulseaudio >> community. >> My current patches are maintained in bzr according to ubuntu, I want >> them to be merged in ubuntu branch. > > > Hi Feng, > > Sorry I haven't responded earlier, but I'm not sure what to do about these > patches either. As I see it there are at least three problems that need to > be resolved: > > * The competing implementation problem: We've had multiple implementations > posted to the PulseAudio mailinglist, one by Janos and Jaska, one by > Alejandro and Margarita (probably discontinued?), and one by yourself. It > would be great if the UCM community could give us a hint on why we should > choose one over another. Exactly. Maybe one reason is my patch is the only one to implement the agreed concepts mapping between PA and alsa-lib. > > * The patches are based on an older version of PulseAudio - the one that > uses the input devices for jack detection. Ubuntu 12.04, as well as > PulseAudio 2.0, will release with the new kcontrol jack detection interface > [1]. In essence, your patches do not apply, and should we consider these for > Ubuntu 12.04 and/or PulseAudio 2.0 we're in quite a hurry. Perhaps it's even > too late, I don't know. It's really too late. I'll follow the new jack detection method and update my patches tho. > > * Verification and testing is difficult, for a variety of reasons (this > point is not a real blocker like the other two, just a little cumbersome): > 1) Requires special hardware. I could probably get hold of some hardware > if that was the only thing keeping me from reviewing it though. Understandable. > 2) Requires special configuration files, which are usually kept private by > companies. I assume that the files you have used for imx53 and omap are > public though (where can they be found)? I hold the configs for linaro release at https://code.launchpad.net/~b34248/+junk/alsa-lib-1.0.24.1 > 3) I *was* going to complain about the lack of UCM documentation, but it > seems like running "make doc" in alsa-lib 1.0.25 does create a UCM page > (called "case interface"), and while it does not look perfect everywhere it > seems like most things can actually be figured out. > > -- > David Henningsson, Canonical Ltd. > http://launchpad.net/~diwic > > [1] Assuming my recently posted patches pass the review of my fellow > developers on this list :-) -- Wei.Feng (irc wei_feng) Linaro Multimedia Team Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
