Re: [pulseaudio-discuss] [PATCH] NetBSD: Add missing declaration of struct timespec

[Arun Raghavan]

On 8 December 2015 at 07:22, Kamil Rytarowski  wrote:
> ---
>  src/pulsecore/core-rtclock.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/pulsecore/core-rtclock.h b/src/pulsecore/core-rtclock.h
> index 11bfcc9..a5327e8 100644
> --- a/src/pulsecore/core-rtclock.h
> +++ b/src/pulsecore/core-rtclock.h
> @@ -42,6 +42,7 @@ void pa_rtclock_hrtimer_enable(void);
>  struct timeval* pa_rtclock_from_wallclock(struct timeval *tv);
>
>  #ifdef HAVE_CLOCK_GETTIME
> +struct timespec;
>  pa_usec_t pa_timespec_load(const struct timespec *ts);
>  struct timespec* pa_timespec_store(struct timespec *ts, pa_usec_t v);
>  #endif
> --

Should we not just be including  instead?

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

Re: [pulseaudio-discuss] [PATCH] NetBSD: Add missing declaration of struct timespec

[Arun Raghavan]

On 8 December 2015 at 08:32, Kamil Rytarowski  wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
>
> On 08.12.2015 03:53, Arun Raghavan wrote:
>> Should we not just be including  instead?
>>
>> -- Arun
>
> Including whole header isn't needed.
>
> For the definition of struct timespec we can cherry-pick from a
> variety of headers, for example .
>
> http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html
>
> I'm OK with any solution solving this issue.

Okay, I see now. Thanks, pushing this patch. Modified commit message
slightly for consistency.

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

Re: [pulseaudio-discuss] [PATCH] netbsd: Fix undefined behavior with array subscript of invalid type

[Arun Raghavan]

On 20 November 2015 at 15:52, Kamil Rytarowski  wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
>
> On 20.11.2015 07:35, Arun Raghavan wrote:
>> On 20 November 2015 at 08:50, Kamil Rytarowski 
>> wrote:
>>> From the NetBSD manual:
>>>
>>> The first argument of these functions is of type int, but only a
>>> very restricted subset of values are actually valid.  The
>>> argument must either be the value of the macro EOF (which has a
>>> negative value), or must be a non-negative value within the range
>>> representable as unsigned char. Passing invalid values leads to
>>> undefined behavior.
>>>
>>> --  ctype(3)
>>
>> This is also true for C99 in general, so not a NetBSD-specific
>> thing, it seems.
>>
>
> This is a part of the C Standard. Behind the scenes ctype(3) functions
> are table lookup functions, passing anything out of the range results
> in undefined/invalid memory access.
>
>>> --- src/modules/dbus/iface-core.c |  2 +- src/pulse/proplist.c
>>> | 12 ++-- src/pulsecore/core-util.c |  6 +++---
>>> src/pulsecore/ltdl-helper.c   |  2 +- src/pulsecore/modargs.c
>>> |  8  5 files changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/src/modules/dbus/iface-core.c
>>> b/src/modules/dbus/iface-core.c index 1b14195..88e9030 100644 ---
>>> a/src/modules/dbus/iface-core.c +++
>>> b/src/modules/dbus/iface-core.c @@ -1442,7 +1442,7 @@ static bool
>>> contains_space(const char *string) { pa_assert(string);
>>>
>>> for (p = string; *p; ++p) { -if (isspace(*p)) +if
>>> (isspace((unsigned char)*p)) return true; }
>>
>> I'm not sure how this is better -- we go from undefined behaviour
>> in the library to forcing potentially undefined behaviour ourselves
>> -- non-ASCII values will generate a "random" value in ASCII space.
>>
>
> The isspace(3) function isn't designed to handle non-ASCII characters.
> If we pass anything (except EOF) we always must cast it, no matter
> what the program logic is.
>
>> We should be checking for valid input instead if we care about
>> this (the one place I quickly checked that uses isspace() is
>> preceded by a pa_asci_valid() call).
>>
>
> This is separated task not handled by this commit.

Okay, that sounds fine. Pushing now.

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

Re: [pulseaudio-discuss] [PATCH] netbsd: Revamp NetBSD platform checks

[Arun Raghavan]

Thanks, landing shortly.

On 8 December 2015 at 08:52, Kamil Rytarowski  wrote:
> ---
>  configure.ac | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 86b28d3..b9cd3d1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -137,6 +137,9 @@ case "$host_os" in
>  AC_MSG_RESULT([freebsd])
>  host_has_caps=1
>  ;;
> +netbsd*)
> +AC_MSG_RESULT([netbsd])
> +;;
>  darwin*)
>  AC_MSG_RESULT([darwin])
>  os_is_darwin=1
> @@ -257,6 +260,10 @@ else
>  # HW specific atomic ops stuff
>  AC_MSG_CHECKING([architecture for native atomic operations])
>  case $host in
> +*-netbsd*)
> +AC_MSG_RESULT([yes])
> +need_libatomic_ops=no
> +;;
>  arm*)
>  AC_MSG_RESULT([arm])
>  AC_MSG_CHECKING([whether we can use Linux kernel helpers])
> @@ -292,10 +299,6 @@ else
>  ])
>  fi
>  ;;
> -*-netbsdelf5*)
> -AC_MSG_RESULT([yes])
> -need_libatomic_ops=no
> -;;
>  *-freebsd*)
>  AC_MSG_RESULT([yes])
>  need_libatomic_ops=no
> --
> 2.6.3
>
> ___
> pulseaudio-discuss mailing list
> pulseaudio-discuss@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] caps: Stop warning about missing capabilities

[Arun Raghavan]

On 9 December 2015 at 09:27, Kamil Rytarowski  wrote:
> This part is system specific, if we request it - we will get them,
> otherwise tolerate that it's either unsupported or just disabled.
>
> Caught on NetBSD.
> ---
>  src/daemon/caps.c | 4 
>  1 file changed, 4 deletions(-)
>
> diff --git a/src/daemon/caps.c b/src/daemon/caps.c
> index fd135c0..81c4b25 100644
> --- a/src/daemon/caps.c
> +++ b/src/daemon/caps.c
> @@ -91,9 +91,5 @@ void pa_drop_caps(void) {
>  #else
>  #error "Don't know how to do capabilities on your system.  Please send a 
> patch."
>  #endif /* __linux__ */
> -#else /* HAVE_SYS_CAPABILITY_H */
> -pa_log_warn("Normally all extra capabilities would be dropped now, but "
> -"that's impossible because PulseAudio was built without "
> -"capabilities support.");
>  #endif
>  }
> --

This seems to be legitimate as a warning considering it's emitted when
running as a privileged process. Since running in system mode is a
special case, it seems okay to leave this in.

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

Re: [pulseaudio-discuss] [PATCH v3 2/2] client-conf, daemon-conf: enable .d directories

[Arun Raghavan]

On 10 December 2015 at 17:16, David Henningsson
 wrote:
>
>
> On 2015-12-07 22:22, Tanu Kaskinen wrote:
>>
>> I want to enable client.conf.d, because in OpenEmbedded-core we have
>> a graphical environment called Sato that runs as root. Sato needs to
>> set allow-autospawn-for-root=true in client.conf, but the default
>> configuration in OpenEmbedded-core should not set that option. With
>> this patch, I can create a Sato-specific package that simply installs
>> 50-sato.conf in /etc/pulse/client.conf.d without conflicting with the
>> main client.conf coming from a different package.
>>
>> daemon.conf.d is enabled just because it would be strange to not
>> support it while client.conf.d is supported.
>> ---
>>   man/pulse-client.conf.5.xml.in | 19 +++
>>   man/pulse-daemon.conf.5.xml.in | 25 ++---
>>   src/daemon/daemon-conf.c   |  2 +-
>>   src/pulse/client-conf.c|  2 +-
>>   4 files changed, 35 insertions(+), 13 deletions(-)
>>
>> diff --git a/man/pulse-client.conf.5.xml.in
>> b/man/pulse-client.conf.5.xml.in
>> index 1002dbe..cca2219 100644
>> --- a/man/pulse-client.conf.5.xml.in
>> +++ b/man/pulse-client.conf.5.xml.in
>> @@ -23,15 +23,26 @@ License along with PulseAudio; if not, see
>> .
>>
>> 
>>   ~/.config/pulse/client.conf
>> -
>> +~/.config/pulse/client.conf.d/*.conf
>>   @PA_DEFAULT_CONFIG_DIR@/client.conf
>> +@PA_DEFAULT_CONFIG_DIR@/client.conf.d/*.conf
>
>
> Sorry for not noticing this earlier, but to compare with PAM, which has
> /etc/security/limits.d/*.conf, not /etc/security/limits.conf.d/*.conf
>
> Hence our directory name should be ~/.config/pulse/client.d rather than
> ~/.config/pulse/client.conf.d
>
> OTOH, /usr/share/alsa/alsa.conf includes files from
> /usr/share/alsa/alsa.conf.d, not /usr/share/alsa/alsa.d/
>
> Is there a more common standard? (E g, what does systemd do?)

I prefer .conf.d -- makes the correlation between the two more explicit imo.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] build-sys: Make pulsecore a private library

[Arun Raghavan]

On 14 December 2015 at 17:58, David Henningsson
 wrote:
> My understanding of all this is a bit limited, but it seems we already do
> this for libpulsecommon without issue.
>
> I'm not sure how much it is upstream and how much is packaging, but on my
> Ubuntu system it looks like /usr/bin/pulseaudio (and libpulse.so) both have
> a
>
>   RPATH/usr/lib/x86_64-linux-gnu/pulseaudio
>
> ...that cause us to find libpulsecomon (and presumably, libpulsecore). I
> thought we weren't using RPATH, but apparently I'm mistaken.
>
> Anyhow. If nobody else complains I don't mind this one going in. Anyone with
> more knowledge who can fill in?

This one does look okay to go in from my side. afaik we do use rpath
on Linux (doesn't work with Android/bionic-based stuff, though).

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

Re: [pulseaudio-discuss] [PATCH] echo-cancel: Don't skip canceller when sink is inactive

[Arun Raghavan]

On 11 December 2015 at 19:26, Alexander E. Patrakov  wrote:
> 18.11.2015 07:58, a...@accosted.net wrote:
>>
>> From: Arun Raghavan 
>>
>> This forces the canceller engine to be invoked even if playback is not
>> currently active. We need to do this for cases where the engine provides
>> additional processing that is independent of playback, such as noise
>> suppression and AGC.
>
>
> The commit message should, ideally, mention this bug:
>
> https://bugs.freedesktop.org/show_bug.cgi?id=83557
>
> And it is fixed by the patch - ACK!
>

Done, and thanks.

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

[pulseaudio-discuss] [PATCH v2 16/25] echo-cancel: Use anonymous unions for echo canceller params

[Arun Raghavan]

From: Arun Raghavan 

Makes this part of the code just a little less verbose.
---
 src/modules/echo-cancel/adrian.c  | 18 +-
 src/modules/echo-cancel/echo-cancel.h |  3 +-
 src/modules/echo-cancel/null.c|  4 +--
 src/modules/echo-cancel/speex.c   | 50 +--
 src/modules/echo-cancel/webrtc.cc | 64 +--
 5 files changed, 70 insertions(+), 69 deletions(-)

diff --git a/src/modules/echo-cancel/adrian.c b/src/modules/echo-cancel/adrian.c
index 60a2b66..3c47fae 100644
--- a/src/modules/echo-cancel/adrian.c
+++ b/src/modules/echo-cancel/adrian.c
@@ -78,16 +78,16 @@ bool pa_adrian_ec_init(pa_core *c, pa_echo_canceller *ec,
 
 rate = out_ss->rate;
 *nframes = (rate * frame_size_ms) / 1000;
-ec->params.priv.adrian.blocksize = (*nframes) * pa_frame_size(out_ss);
+ec->params.adrian.blocksize = (*nframes) * pa_frame_size(out_ss);
 
-pa_log_debug ("Using nframes %d, blocksize %u, channels %d, rate %d", 
*nframes, ec->params.priv.adrian.blocksize, out_ss->channels, out_ss->rate);
+pa_log_debug ("Using nframes %d, blocksize %u, channels %d, rate %d", 
*nframes, ec->params.adrian.blocksize, out_ss->channels, out_ss->rate);
 
 /* For now we only support SSE */
 if (c->cpu_info.cpu_type == PA_CPU_X86 && (c->cpu_info.flags.x86 & 
PA_CPU_X86_SSE))
 have_vector = 1;
 
-ec->params.priv.adrian.aec = AEC_init(rate, have_vector);
-if (!ec->params.priv.adrian.aec)
+ec->params.adrian.aec = AEC_init(rate, have_vector);
+if (!ec->params.adrian.aec)
 goto fail;
 
 pa_modargs_free(ma);
@@ -102,17 +102,17 @@ fail:
 void pa_adrian_ec_run(pa_echo_canceller *ec, const uint8_t *rec, const uint8_t 
*play, uint8_t *out) {
 unsigned int i;
 
-for (i = 0; i < ec->params.priv.adrian.blocksize; i += 2) {
+for (i = 0; i < ec->params.adrian.blocksize; i += 2) {
 /* We know it's S16NE mono data */
 int r = *(int16_t *)(rec + i);
 int p = *(int16_t *)(play + i);
-*(int16_t *)(out + i) = (int16_t) 
AEC_doAEC(ec->params.priv.adrian.aec, r, p);
+*(int16_t *)(out + i) = (int16_t) AEC_doAEC(ec->params.adrian.aec, r, 
p);
 }
 }
 
 void pa_adrian_ec_done(pa_echo_canceller *ec) {
-if (ec->params.priv.adrian.aec) {
-AEC_done(ec->params.priv.adrian.aec);
-ec->params.priv.adrian.aec = NULL;
+if (ec->params.adrian.aec) {
+AEC_done(ec->params.adrian.aec);
+ec->params.adrian.aec = NULL;
 }
 }
diff --git a/src/modules/echo-cancel/echo-cancel.h 
b/src/modules/echo-cancel/echo-cancel.h
index b570095..37f99c0 100644
--- a/src/modules/echo-cancel/echo-cancel.h
+++ b/src/modules/echo-cancel/echo-cancel.h
@@ -69,10 +69,11 @@ struct pa_echo_canceller_params {
 bool agc;
 bool trace;
 bool first;
+unsigned int agc_start_volume;
 } webrtc;
 #endif
 /* each canceller-specific structure goes here */
-} priv;
+};
 
 /* Set this if canceller can do drift compensation. Also see set_drift()
  * below */
diff --git a/src/modules/echo-cancel/null.c b/src/modules/echo-cancel/null.c
index 673b14f..c8ecf27 100644
--- a/src/modules/echo-cancel/null.c
+++ b/src/modules/echo-cancel/null.c
@@ -34,7 +34,7 @@ bool pa_null_ec_init(pa_core *c, pa_echo_canceller *ec,
 char strss_sink[PA_SAMPLE_SPEC_SNPRINT_MAX];
 
 *nframes = 256;
-ec->params.priv.null.out_ss = *out_ss;
+ec->params.null.out_ss = *out_ss;
 
 *rec_ss = *out_ss;
 *rec_map = *out_map;
@@ -49,7 +49,7 @@ bool pa_null_ec_init(pa_core *c, pa_echo_canceller *ec,
 void pa_null_ec_run(pa_echo_canceller *ec, const uint8_t *rec, const uint8_t 
*play, uint8_t *out) {
 /* The null implementation simply copies the recorded buffer to the output
buffer and ignores the play buffer. */
-memcpy(out, rec, 256 * pa_frame_size(&ec->params.priv.null.out_ss));
+memcpy(out, rec, 256 * pa_frame_size(&ec->params.null.out_ss));
 }
 
 void pa_null_ec_done(pa_echo_canceller *ec) {
diff --git a/src/modules/echo-cancel/speex.c b/src/modules/echo-cancel/speex.c
index 11e53b3..08c1027 100644
--- a/src/modules/echo-cancel/speex.c
+++ b/src/modules/echo-cancel/speex.c
@@ -111,26 +111,26 @@ static bool 
pa_speex_ec_preprocessor_init(pa_echo_canceller *ec, pa_sample_spec
 goto fail;
 }
 
-ec->params.priv.speex.pp_state = speex_preprocess_state_init(nframes, 
out_ss->rate);
+ec->params.speex.pp_state = speex_preprocess_state_init(nframes, 
out_ss->rate);
 
 tmp = agc;
-speex_preprocess_ctl(ec->params.priv.speex.pp_state, 
SPEEX_PREPROCESS_SET_AGC, &tmp);
+speex_preprocess_ctl(ec->params.speex.pp_state, 
SPEEX_PREPROCESS_SET_AGC, &tmp);
 
 tmp = 

Re: [pulseaudio-discuss] [PATCH v2 01/25] echo-cancel: Update webrtc-audio-processing usage to new API

[Arun Raghavan]

On 16 December 2015 at 09:38, Tanu Kaskinen  wrote:
> (I'm just glancing through, this is not a proper review.)
>
> On Wed, 2015-12-16 at 09:09 +0530, a...@accosted.net wrote:
>> From: Arun Raghavan 
>>
>> ---
>>  configure.ac  |  2 +-
>>  src/Makefile.am   |  2 +-
>>  src/modules/echo-cancel/webrtc.cc | 54 
>> +--
>>  3 files changed, 31 insertions(+), 27 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index b9cd3d1..26c3e29 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1371,7 +1371,7 @@ AC_ARG_ENABLE([webrtc-aec],
>>  AS_HELP_STRING([--enable-webrtc-aec], [Enable the optional WebRTC-based 
>> echo canceller]))
>>
>>  AS_IF([test "x$enable_webrtc_aec" != "xno"],
>> -[PKG_CHECK_MODULES(WEBRTC, [ webrtc-audio-processing ], 
>> [HAVE_WEBRTC=1], [HAVE_WEBRTC=0])],
>> +[PKG_CHECK_MODULES(WEBRTC, [ webrtc-audio-processing > 0.1 ], 
>> [HAVE_WEBRTC=1], [HAVE_WEBRTC=0])],
>
> I think it would be better to use >= 0.2 (or whatever is the minimum
> required version).

Sure, I can do that. Was easier to test this way before doing a 0.2
release of webrtc-audio-processing.

>>  [HAVE_WEBRTC=0])
>>
>>  AS_IF([test "x$enable_webrtc_aec" = "xyes" && test "x$HAVE_WEBRTC" = "x0"],
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index f1bd38d..533b646 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -50,7 +50,7 @@ AM_CPPFLAGS = \
>>   -DPULSE_LOCALEDIR=\"$(localedir)\"
>>  AM_CFLAGS = \
>>   $(PTHREAD_CFLAGS)
>> -AM_CXXFLAGS = $(AM_CFLAGS)
>> +AM_CXXFLAGS = $(AM_CFLAGS) -std=c++11
>
> This seems like material for a separate patch.

This is needed for the code to compile at all, is why I included it here.

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

Re: [pulseaudio-discuss] [PATCH] sink-input: Don't access resampler to get silence memchunk

[Arun Raghavan]

On 16 December 2015 at 00:36, Tanu Kaskinen  wrote:
> On Tue, 2015-12-15 at 21:49 +0530, a...@accosted.net wrote:
>> From: Arun Raghavan 
>>
>> There doesn't appear to be a good reason to restrict the memchunk length
>> to the resample max block size -- we're going to have the memory around
>> anyway.
>
> I think the reason is to make sure that we don't feed the resampler
> bigger chunks than what it can handle. The resampler has to allocate
> other memblocks during its operation, and those memblocks may be bigger
> than the input block, so if the input block is too large, the
> requirements for the other blocks will grow beyond the mempool max
> block size.
>
> However, pa_sink_input_peek() seems to protect against this anyway when
> doing resampling (it processes the input in smaller pieces if it's
> larger than the resampler max block size), so maybe this change is safe
> anyway.
>
>> Moreover, callers of pa_sink_input_get_silence() don't seem to
>> actually care about the chunk itself, just the memblock for creating
>> their own pa_memblockq.
>
> I don't understand this comment. pa_memblockq cares about the chunk
> itself, not just the memblock.

I misread that code. It does work with the chunk, not the memblock of course.

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

Re: [pulseaudio-discuss] [PATCH v2 01/25] echo-cancel: Update webrtc-audio-processing usage to new API

[Arun Raghavan]

On 16 December 2015 at 09:52, Tanu Kaskinen  wrote:
> On Wed, 2015-12-16 at 09:48 +0530, Arun Raghavan wrote:
>> On 16 December 2015 at 09:38, Tanu Kaskinen  wrote:
>> > (I'm just glancing through, this is not a proper review.)
>> >
>> > On Wed, 2015-12-16 at 09:09 +0530, a...@accosted.net wrote:
>> > > From: Arun Raghavan 
>> > >
>> > > ---
>> > >  configure.ac  |  2 +-
>> > >  src/Makefile.am   |  2 +-
>> > >  src/modules/echo-cancel/webrtc.cc | 54 +--
>> > > 
>> > >  3 files changed, 31 insertions(+), 27 deletions(-)
>> > >
>> > > diff --git a/configure.ac b/configure.ac
>> > > index b9cd3d1..26c3e29 100644
>> > > --- a/configure.ac
>> > > +++ b/configure.ac
>> > > @@ -1371,7 +1371,7 @@ AC_ARG_ENABLE([webrtc-aec],
>> > >  AS_HELP_STRING([--enable-webrtc-aec], [Enable the optional
>> > > WebRTC-based echo canceller]))
>> > >
>> > >  AS_IF([test "x$enable_webrtc_aec" != "xno"],
>> > > -[PKG_CHECK_MODULES(WEBRTC, [ webrtc-audio-processing ],
>> > > [HAVE_WEBRTC=1], [HAVE_WEBRTC=0])],
>> > > +[PKG_CHECK_MODULES(WEBRTC, [ webrtc-audio-processing > 0.1
>> > > ], [HAVE_WEBRTC=1], [HAVE_WEBRTC=0])],
>> >
>> > I think it would be better to use >= 0.2 (or whatever is the
>> > minimum
>> > required version).
>>
>> Sure, I can do that. Was easier to test this way before doing a 0.2
>> release of webrtc-audio-processing.
>>
>> > >  [HAVE_WEBRTC=0])
>> > >
>> > >  AS_IF([test "x$enable_webrtc_aec" = "xyes" && test
>> > > "x$HAVE_WEBRTC" = "x0"],
>> > > diff --git a/src/Makefile.am b/src/Makefile.am
>> > > index f1bd38d..533b646 100644
>> > > --- a/src/Makefile.am
>> > > +++ b/src/Makefile.am
>> > > @@ -50,7 +50,7 @@ AM_CPPFLAGS = \
>> > >   -DPULSE_LOCALEDIR=\"$(localedir)\"
>> > >  AM_CFLAGS = \
>> > >   $(PTHREAD_CFLAGS)
>> > > -AM_CXXFLAGS = $(AM_CFLAGS)
>> > > +AM_CXXFLAGS = $(AM_CFLAGS) -std=c++11
>> >
>> > This seems like material for a separate patch.
>>
>> This is needed for the code to compile at all, is why I included it
>> here.
>
> Ok, makes sense. It would be good to note the reason in the commit
> message, though. Does the requirement for c++11 come from the library
> or from the code changes in this patch?

The library IIRC (will double-check and update the commit message).

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

Re: [pulseaudio-discuss] [PATCH v2 15/25] build-sys: Move to compiling with C11 support

[Arun Raghavan]

On Wed, 2015-12-16 at 07:19 +0200, Tanu Kaskinen wrote:
> On Wed, 2015-12-16 at 09:10 +0530, a...@accosted.net wrote:
> > From: Arun Raghavan 
> > 
> > This is needed for building with anonymous unions. A bunch of calls
> > to
> > fail() that used to mysteriously work need fixing -- fail() is a
> > macro
> > that takes a printf-style message as an argument. Not passing this
> > somehow worked with the previous compiler flags, but breaks with
> > -std=c11.
> > ---
> >  configure.ac | 3 ++-
> >  src/tests/connect-stress.c   | 6 +++---
> >  src/tests/cpu-mix-test.c | 2 +-
> >  src/tests/cpu-remap-test.c   | 4 ++--
> >  src/tests/cpu-sconv-test.c   | 4 ++--
> >  src/tests/cpu-volume-test.c  | 2 +-
> >  src/tests/cpulimit-test.c| 4 ++--
> >  src/tests/extended-test.c| 4 ++--
> >  src/tests/get-binary-name-test.c | 2 +-
> >  src/tests/interpol-test.c| 2 +-
> >  src/tests/mult-s16-test.c| 2 +-
> >  src/tests/sync-playback.c| 4 ++--
> >  12 files changed, 20 insertions(+), 19 deletions(-)
> > 
> > diff --git a/configure.ac b/configure.ac
> > index 26c3e29..6fc77c5 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -80,7 +80,6 @@ AC_PROG_LN_S
> >  # CC
> >  
> >  AC_PROG_CC
> > -AC_PROG_CC_C99
> >  AM_PROG_CC_C_O
> >  # Only required if you want the WebRTC canceller -- no runtime dep
> > on
> >  # libstdc++ otherwise
> > @@ -176,6 +175,8 @@ esac
> >  
> >   Compiler flags 
> >  
> > +AX_APPEND_COMPILE_FLAGS([-std=c11], [], [-pedantic -Werror])
> 
> We discussed this a while back in irc, and my recollection is that we
> agreed that some macro with "CHECK" in its name seemed the best fit.
> AX_APPEND_COMPILE_FLAGS is not good, because it silently drops the
> flag
> if -std=c11 isn't supported.

Ah yes, I'll fix this one up to use AX_CHECK_COMPILE_FLAG instead.

-- Arun


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

[pulseaudio-discuss] Impending freeze

[Arun Raghavan]

Hi folks,
Since we're trying to keep up a 4 month release cycle, we're looking to
freeze for 8.0 on December 24th. If there's stuff you think needs to be
merged before that, please speak up and/or poke people.

For those of you who'd like to pitch in, there's a tracker at: https://
bugs.freedesktop.org/show_bug.cgi?id=91504

Looking at the blocker list, I think we'll end up moving some of that
to 9.0 unfortunately. Or maybe the days will magically become 8 hours
longer. :)

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

Re: [pulseaudio-discuss] [PATCH v2 08/25] echo-cancel: Mark private function as static

[Arun Raghavan]

On Wed, 2015-12-16 at 07:11 +0200, Tanu Kaskinen wrote:
> On Wed, 2015-12-16 at 09:09 +0530, a...@accosted.net wrote:
> > From: Arun Raghavan 
> > 
> > ---
> >  src/modules/echo-cancel/webrtc.cc | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-
> > cancel/webrtc.cc
> > index 3be7fe5..a8b69b5 100644
> > --- a/src/modules/echo-cancel/webrtc.cc
> > +++ b/src/modules/echo-cancel/webrtc.cc
> > @@ -78,9 +78,9 @@ static int routing_mode_from_string(const char
> > *rmode) {
> >  return -1;
> >  }
> >  
> > -void pa_webrtc_ec_fixate_spec(pa_sample_spec *rec_ss,
> > pa_channel_map *rec_map,
> > -  pa_sample_spec *play_ss,
> > pa_channel_map *play_map,
> > -  pa_sample_spec *out_ss,
> > pa_channel_map *out_map)
> > +static void pa_webrtc_ec_fixate_spec(pa_sample_spec *rec_ss,
> > pa_channel_map *rec_map,
> > + pa_sample_spec *play_ss,
> > pa_channel_map *play_map,
> > + pa_sample_spec *out_ss,
> > pa_channel_map *out_map)
> >  {
> >  rec_ss->format = PA_SAMPLE_S16NE;
> >  play_ss->format = PA_SAMPLE_S16NE;
> 
> The pa_ prefix should be dropped for static functions, and "webrtc"
> (maybe also "ec") seems redundant too.

Will fix that and speex' similar usage up in a separate commit.

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

Re: [pulseaudio-discuss] [PATCH] SunOS: Catch up with newer API and blacklist SOUND_PCM_* on SunOS

[Arun Raghavan]

On Fri, 2015-12-11 at 05:30 +0100, Kamil Rytarowski wrote:
> Patch from pkgsrc by Jonathan Perkin (Joyent).
> ---

The two patches should be separated, since they are unrelated.

>  src/modules/module-solaris.c | 12 +---
>  src/utils/padsp.c|  2 ++
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/modules/module-solaris.c b/src/modules/module-
> solaris.c
> index c79918a..2fa0bff 100644
> --- a/src/modules/module-solaris.c
> +++ b/src/modules/module-solaris.c
> @@ -412,10 +412,12 @@ static int sink_process_msg(pa_msgobject *o,
> int code, void *data, int64_t offse
>  pa_smoother_resume(u->smoother,
> pa_rtclock_now(), true);
>  
>  if (!u->source || u->source_suspended) {
> +bool mute;
>  if (unsuspend(u) < 0)
>  return -1;
>  u->sink->get_volume(u->sink);
> -u->sink->get_mute(u->sink);
> +if (u->sink->get_mute(u->sink, &mute) >=
> 0)
> +pa_sink_set_mute(u->sink, mute,
> false);
>  }
>  u->sink_suspended = false;
>  }
> @@ -1033,8 +1035,12 @@ int pa__init(pa_module *m) {
>  
>  if (sink_new_data.muted_is_set)
>  u->sink->set_mute(u->sink);
> -else
> -u->sink->get_mute(u->sink);
> +else {
> +bool mute;
> +
> +if (u->sink->get_mute(u->sink, &mute) >= 0)
> +pa_sink_set_mute(u->sink, mute, false);
> +}
>  
>  pa_sink_put(u->sink);
>  }
> diff --git a/src/utils/padsp.c b/src/utils/padsp.c
> index 5e336bb..2eff0f0 100644
> --- a/src/utils/padsp.c
> +++ b/src/utils/padsp.c
> @@ -2278,6 +2278,7 @@ static int dsp_ioctl(fd_info *i, unsigned long
> request, void*argp, int *_errno)
>  break;
>  }
>  
> +#ifndef __sun
>  case SOUND_PCM_READ_RATE:
>  debug(DEBUG_LEVEL_NORMAL, __FILE__":
> SOUND_PCM_READ_RATE\n");
>  
> @@ -2301,6 +2302,7 @@ static int dsp_ioctl(fd_info *i, unsigned long
> request, void*argp, int *_errno)
>  *(int*) argp = pa_sample_size(&i->sample_spec)*8;
>  pa_threaded_mainloop_unlock(i->mainloop);
>  break;
> +#endif
>  
>  case SNDCTL_DSP_GETOPTR: {
>  count_info *info;

Is this really not supported on Solaris? Do you have a bug report or
some other reference for this (which would ideally go into the commit
message after splitting this off separately).

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

Re: [pulseaudio-discuss] [PATCH] allow-passthrough module

[Arun Raghavan]

On Fri, 2015-12-11 at 17:08 +0100, Guillaume Desmottes wrote:
> Hi there,
> 
> A while ago we (Collabora) implemented a module for Valve changing
> the
> default policy regarding passthrough streams.
> You can find some context on the original thread: http://lists.freede
> sk
> top.org/archives/pulseaudio-discuss/2014-May/020644.html
> 
> This module never reached upstream but is still shipped and used by
> default on SteamOS.
> 
> I'd be interested resurrecting this work and bring it to a state it
> could be merged upstream.
> What would be needed to reach this? Would fixing Arun's comments from
> h
> ttp://lists.freedesktop.org/archives/pulseaudio-discuss/2014
> -May/020745.html be enough?

Re-enumerating those, plus additional comments:

1. You need to deal with the passthrough sink moving to another sink as
well (pretty much the same as the passthrough sink being unlinked)

2. If the sink goes away the corresponding null-sink should too. You
may find a situation where a passthrough stream on this sink moves to
another sink successfully, in which case you might need to set up a
null sink for the new sink and apply the same logic as a new
passthrough stream

3. There's a "fake-passthrough" hack that has a FIXME against it -- not
sure what this is for

4. The (stream != i) condition in passthrough_stream_removed() seems
like a noop? i-> sink will never be equal to null_sink

5. Might be nice to name the null sinks to have the "parent" sink's
name

This isn't a blocker, but I'd also like a more descriptive name than
allow-passthrough, but I can't think of one. :) Maybe someone else has
a suggestion.

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

Re: [pulseaudio-discuss] [PATCH] always-sink: simplify hook management with pa_module_hook_connect()

[Arun Raghavan]

On 20 December 2015 at 15:48, Tanu Kaskinen  wrote:
> ---
>  src/modules/module-always-sink.c | 9 ++---
>  1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/src/modules/module-always-sink.c 
> b/src/modules/module-always-sink.c
> index b5721bf..12f63f7 100644
> --- a/src/modules/module-always-sink.c
> +++ b/src/modules/module-always-sink.c
> @@ -47,7 +47,6 @@ static const char* const valid_modargs[] = {
>  };
>
>  struct userdata {
> -pa_hook_slot *put_slot, *unlink_slot;
>  uint32_t null_module;
>  bool ignore;
>  char *sink_name;
> @@ -162,8 +161,8 @@ int pa__init(pa_module*m) {
>
>  m->userdata = u = pa_xnew(struct userdata, 1);
>  u->sink_name = pa_xstrdup(pa_modargs_get_value(ma, "sink_name", 
> DEFAULT_SINK_NAME));
> -u->put_slot = pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_PUT], 
> PA_HOOK_LATE, (pa_hook_cb_t) put_hook_callback, u);
> -u->unlink_slot = 
> pa_hook_connect(&m->core->hooks[PA_CORE_HOOK_SINK_UNLINK], PA_HOOK_EARLY, 
> (pa_hook_cb_t) unlink_hook_callback, u);
> +pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_PUT], 
> PA_HOOK_LATE, (pa_hook_cb_t) put_hook_callback, u);
> +pa_module_hook_connect(m, &m->core->hooks[PA_CORE_HOOK_SINK_UNLINK], 
> PA_HOOK_EARLY, (pa_hook_cb_t) unlink_hook_callback, u);
>  u->null_module = PA_INVALID_INDEX;
>  u->ignore = false;
>
> @@ -182,10 +181,6 @@ void pa__done(pa_module*m) {
>  if (!(u = m->userdata))
>  return;
>
> -if (u->put_slot)
> -pa_hook_slot_free(u->put_slot);
> -if (u->unlink_slot)
> -pa_hook_slot_free(u->unlink_slot);
>  if (u->null_module != PA_INVALID_INDEX && m->core->state != 
> PA_CORE_SHUTDOWN)
>  pa_module_unload_request_by_index(m->core, u->null_module, true);
>
> --

Ack.

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

Re: [pulseaudio-discuss] [PATCH] sink-input, source-output: don't allow moving of streams connected to moving filter devices

[Arun Raghavan]

On 20 December 2015 at 15:48, Tanu Kaskinen  wrote:
> This fixes a crash that was observed in the following scenario:
>
> A phone applications creates playback and recording streams with
> filter.want=echo-cancel. An echo-cancel sink gets created. The alsa
> card profile has enabled a 4.0 sink, and the user changes the profile
> to have a stereo sink instead. A complex sequence of events happens:
> the echo-cancel sink input gets detached from the alsa sink, the 4.0
> sink gets removed, which triggers loading of a null sink by
> module-always-sink, and that in turn triggers rerouting in
> module-device-manager, which decides to move the phone stream to the
> null sink. Moving a sink input involves sending a message to the IO
> thread of the old sink, but in this case the old sink is the
> echo-cancel sink, which was detached from the now-dead alsa sink.
> Therefore, the echo-cancel sink doesn't have an IO thread associated
> with it, and as can be expected, sending a message to a non-existing
> thread results in a crash.
>
> The crash can be avoided by disallowing moving streams that are
> connected to filter devices that themselves are being moved. The
> profile switch still doesn't work smoothly, though, because the
> echo-cancel streams don't support moving, so when the old alsa sink
> goes away, module-echo-cancel gets unloaded, and since the
> echo-cancel sink input got detached before that, the phone sink input
> isn't movable either and gets killed. But that's a separate issue.
>
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=93443
> ---
>  src/pulsecore/sink-input.c| 26 ++
>  src/pulsecore/source-output.c | 27 +++
>  2 files changed, 53 insertions(+)
>
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 539ae17..0e01893 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -1528,6 +1528,22 @@ static bool find_filter_sink_input(pa_sink_input 
> *target, pa_sink *s) {
>  return false;
>  }
>
> +static bool is_filter_sink_moving(pa_sink_input *i) {
> +pa_sink *sink = i->sink;
> +
> +if (!sink)
> +return false;
> +
> +while (sink->input_to_master) {
> +sink = sink->input_to_master->sink;
> +
> +if (!sink)
> +return true;
> +}
> +
> +return false;
> +}
> +
>  /* Called from main context */
>  bool pa_sink_input_may_move_to(pa_sink_input *i, pa_sink *dest) {
>  pa_sink_input_assert_ref(i);
> @@ -1547,6 +1563,16 @@ bool pa_sink_input_may_move_to(pa_sink_input *i, 
> pa_sink *dest) {
>  return false;
>  }
>
> +/* If this sink input is connected to a filter sink that itself is 
> moving,
> + * then don't allow the move. Moving requires sending a message to the IO
> + * thread of the old sink, and if the old sink is a filter sink that is
> + * moving, there's no IO thread associated to the old sink. */
> +if (is_filter_sink_moving(i)) {
> +pa_log_debug("Can't move input from filter sink %s, because the 
> filter sink itself is currently moving.",
> + i->sink->name);
> +return false;
> +}
> +
>  if (pa_idxset_size(dest->inputs) >= PA_MAX_INPUTS_PER_SINK) {
>  pa_log_warn("Failed to move sink input: too many inputs per sink.");
>  return false;
> diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
> index 9000972..24e9df4 100644
> --- a/src/pulsecore/source-output.c
> +++ b/src/pulsecore/source-output.c
> @@ -1184,6 +1184,22 @@ static bool find_filter_source_output(pa_source_output 
> *target, pa_source *s) {
>  return false;
>  }
>
> +static bool is_filter_source_moving(pa_source_output *o) {
> +pa_source *source = o->source;
> +
> +if (!source)
> +return false;
> +
> +while (source->output_from_master) {
> +source = source->output_from_master->source;
> +
> +if (!source)
> +return true;
> +}
> +
> +return false;
> +}
> +
>  /* Called from main context */
>  bool pa_source_output_may_move_to(pa_source_output *o, pa_source *dest) {
>  pa_source_output_assert_ref(o);
> @@ -1202,6 +1218,17 @@ bool pa_source_output_may_move_to(pa_source_output *o, 
> pa_source *dest) {
>  return false;
>  }
>
> +/* If this source output is connected to a filter source that itself is
> + * moving, then don't allow the move. Moving requires sending a message 
> to
> + * the IO thread of the old source, and if the old source is a filter
> + * source that is moving, there's no IO thread associated to the old
> + * source. */
> +if (is_filter_source_moving(o)) {
> +pa_log_debug("Can't move output from filter source %s, because the 
> filter source itself is currently moving.",
> + o->source->name);
> +return false;
> +}
> +
>  if (pa_idxset_size(dest->outputs) >= PA_MAX_OUTPUTS_PER_SOURCE) {
>  p

[pulseaudio-discuss] [ANNOUNCE] PulseAudio 8.0 RC1

[Arun Raghavan]

Hi folks,
As promised, master has been frozen for the 8.0 release. I've rolled
out the RC1 tarball (7.99.1, as usual). Please test and file bugs if
you see regressions. Get it at:

http://www.freedesktop.org/software/pulseaudio/releases/pulseaudio-7.99.1.tar.xz
MD5: 257bf53e4cb00227d7d1abb964655c7c
SHA1: e5ad51227c4833e7c1955683fd4cf9ec7f4bf8df

In addition to the usual set of bug fixes and misc improvements,
interesting changes include a new balance-based volume API, routing
improvements, and .d directory support.

From a packaging perspective, the last bit is likely interesting (you
can ship distro config as separate files instead of mucking about with
the current file in /etc/pulse). Also of note is the move of
libpulsecore out of libdir since it's a private library.

We'll have a detailed changelog with the final release, of course.

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

Re: [pulseaudio-discuss] [PATCH 1/2] build-sys: Fix install order of libpulsecore

[Arun Raghavan]

On 29 December 2015 at 05:33, Felipe Sateler  wrote:
> On 28 December 2015 at 20:34, Felipe Sateler  wrote:
>> It needs to be installed after libpulse, because of libtool relinking.
>
> Sorry for not noticing earlier. This is fallout from my earlier patch
> to install libpulsecore into pkglibdir. When building on a system that
> does not have libpulse installed, the followin error occurs:
>
> /usr/bin/ld: cannot find -lpulse
> collect2: error: ld returned 1 exit status
> libtool: install: error: relink `libpulsecore-7.99.la' with the above
> command before installing it
> Makefile:5012: recipe for target 'install-pkglibLTLIBRARIES' failed
>
> This patch orders first libpulsecommon, then normal libs, and then
> pkglibs (which is only libpulsecore now).
>
> --

Thanks, pushing this and the Travis one.

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

Re: [pulseaudio-discuss] [PATCH 3/6] stream: Implement a pa_stream_get_volume() API

[Arun Raghavan]

On 29 December 2015 at 18:17, Felipe Sateler  wrote:
> On 29 December 2015 at 00:33,   wrote:
>> diff --git a/PROTOCOL b/PROTOCOL
>> index 3c08fea..45e3c96 100644
>> --- a/PROTOCOL
>> +++ b/PROTOCOL
>> @@ -371,6 +371,22 @@ PA_COMMAND_DISABLE_SRBCHANNEL
>>  Tells the client to stop listening on the additional SHM ringbuffer channel.
>>  Acked by client by sending PA_COMMAND_DISABLE_SRBCHANNEL back.
>>
>> +## v31, implemented by >= 8.0
>
>
> This says 8.0, but other patches say "\since 9.0".

Thanks, I'll fix that locally.

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

Re: [pulseaudio-discuss] [PATCH 2/2] pulse: Bump PA_RATE_MAX to 384 kHz

[Arun Raghavan]

On 31 December 2015 at 11:15, Tanu Kaskinen  wrote:
> On Thu, 2015-12-31 at 09:42 +0530, a...@accosted.net wrote:
>> From: Arun Raghavan 
>>
>> This will likely be needed in the future when we start supporting high
>> bitrate passthrough, and there actually seem to be people 352/384 kHz
>> out there (potentially as an intermediate production step).
>
> What's your source for claiming that there seems to be people with such
> PCM files (I'm assuming you implied PCM files)? If it's only used as an
> intermediate production step, is there any evidence that people would
> want to play that intermediate stuff via pulseaudio?
>
> The mpv bug was about a dsf file (which means a dsd stream from sacd?).

My source is the Internet. :)

https://en.wikipedia.org/wiki/Digital_eXtreme_Definition

> Supposedly the stream contents were non-PCM data containing more than 2
> channels, meant to be played in passthrough mode. And I guess alsa has
> specified that such content should be wrapped in a 2-channel 384 kHz
> float fake-PCM stream? I don't like extending the rate range for PCM
> streams just because alsa's passthrough implementation happens to
> specify such parameters for fake-PCM streams, but maybe there aren't
> any better options...

I certainly don't know of better options. FWIW, some HDA codecs do
report support for this rate as well. Presumably either the DAC
supports it (I'd be curious to find out why), or it's supported for
HBR passthrough. Either way, we'll probably need to end up supporting
this.

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

Re: [pulseaudio-discuss] [PATCH 1/2] format: Make pa_format_info_valid() stricter for PCM

[Arun Raghavan]

On 31 December 2015 at 11:16, Tanu Kaskinen  wrote:
> On Thu, 2015-12-31 at 09:42 +0530, a...@accosted.net wrote:
>> From: Arun Raghavan 
>>
>> We should do stricter validation when we can.
>> ---
>>  src/pulse/format.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/pulse/format.c b/src/pulse/format.c
>> index c2a1552..b07940a 100644
>> --- a/src/pulse/format.c
>> +++ b/src/pulse/format.c
>> @@ -101,7 +101,13 @@ void pa_format_info_free(pa_format_info *f) {
>>  }
>>
>>  int pa_format_info_valid(const pa_format_info *f) {
>> -return (f->encoding >= 0 && f->encoding < PA_ENCODING_MAX && f->plist 
>> != NULL);
>> +pa_sample_spec ss;
>> +
>> +if (pa_format_info_is_pcm(f)) {
>> +pa_format_info_to_sample_spec(f, &ss, NULL);
>> +return pa_sample_spec_valid(&ss);
>> +} else
>> +return (f->encoding >= 0 && f->encoding < PA_ENCODING_MAX && 
>> f->plist != NULL);
>>  }
>>
>>  int pa_format_info_is_pcm(const pa_format_info *f) {
>
> Looks good to me, and this seems appropriate for 8.0 too.

As usual, my preference is to be super-conservative during the freeze
so we can go from RC to final as quickly as possible.

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

Re: [pulseaudio-discuss] [ANNOUNCE] PulseAudio 8.0 RC1

[Arun Raghavan]

On 31 December 2015 at 09:11, Felipe Sateler  wrote:
> On 28 December 2015 at 09:56, Arun Raghavan  wrote:
>> Hi folks,
>> As promised, master has been frozen for the 8.0 release. I've rolled
>> out the RC1 tarball (7.99.1, as usual). Please test and file bugs if
>> you see regressions.
>
> It's now available from debian experimental, if you want to try it out
> but don't want to compile it!

Nice, thanks!

Fedora Rawhide has it as well, and I've got builds based on that
package for Fedora 21/22/23 at:

https://copr.fedoraproject.org/coprs/arunsr/pulseaudio-latest/

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

Re: [pulseaudio-discuss] [PATCH] module-coreaudio-{device, detect}: implement record and playback modargs, curtesy of module-waveout.

[Arun Raghavan]

On Mon, 2015-04-27 at 23:11 +0200, Mihai Moldovan wrote:
> Signed-off-by: Mihai Moldovan 
> ---
>  src/modules/macosx/module-coreaudio-detect.c | 23
> ---
>  src/modules/macosx/module-coreaudio-device.c | 23
> ---
>  2 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/src/modules/macosx/module-coreaudio-detect.c
> b/src/modules/macosx/module-coreaudio-detect.c
> index d9c09da..a23aa2f 100644
> --- a/src/modules/macosx/module-coreaudio-detect.c
> +++ b/src/modules/macosx/module-coreaudio-detect.c
> @@ -39,10 +39,14 @@ PA_MODULE_AUTHOR("Daniel Mack");
>  PA_MODULE_DESCRIPTION("CoreAudio device detection");
>  PA_MODULE_VERSION(PACKAGE_VERSION);
>  PA_MODULE_LOAD_ONCE(true);
> -PA_MODULE_USAGE("ioproc_frames= device> ");
> +PA_MODULE_USAGE("ioproc_frames= device> "
> +"record= "
> +"playback= ");
>  
>  static const char* const valid_modargs[] = {
>  "ioproc_frames",
> +"record",
> +"playback",
>  NULL
>  };
>  
> @@ -58,6 +62,8 @@ struct userdata {
>  int detect_fds[2];
>  pa_io_event *detect_io;
>  unsigned int ioproc_frames;
> +bool record;
> +bool playback;
>  PA_LLIST_HEAD(ca_device, devices);
>  };
>  
> @@ -87,9 +93,9 @@ static int ca_device_added(struct pa_module *m,
> AudioObjectID id) {
>  return 0;
>  
>  if (u->ioproc_frames)
> -args = pa_sprintf_malloc("object_id=%d ioproc_frames=%d",
> (int) id, u->ioproc_frames);
> +args = pa_sprintf_malloc("object_id=%d ioproc_frames=%d
> record=%d playback=%d", (int) id, u->ioproc_frames, (int) u->record,
> (int) u->playback);
>  else
> -args = pa_sprintf_malloc("object_id=%d", (int) id);
> +args = pa_sprintf_malloc("object_id=%d record=%d
> playback=%d", (int) id, (int) u->record, (int) u->playback);
>  
>  pa_log_debug("Loading %s with arguments '%s'",
> DEVICE_MODULE_NAME, args);
>  mod = pa_module_load(m->core, DEVICE_MODULE_NAME, args);
> @@ -212,6 +218,7 @@ int pa__init(pa_module *m) {
>  pa_modargs *ma;
>  
>  pa_assert(m);
> +pa_assert(m->core);
>  
>  m->userdata = u;
>  
> @@ -220,6 +227,16 @@ int pa__init(pa_module *m) {
>  goto fail;
>  }
>  
> +if (pa_modargs_get_value_boolean(ma, "record", &u->record) < 0
> || pa_modargs_get_value_boolean(ma, "playback", &u->playback) < 0) {
> +pa_log("record= and playback= expect boolean argument.");
> +goto fail;
> +}
> +
> +if (!u->playback && !u->record) {
> +pa_log("neither playback nor record enabled for device.");
> +goto fail;
> +}
> +
> 

Sorry about the super late review. Shouldn't u->record and u->playback
be true by default?

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

Re: [pulseaudio-discuss] [PATCH 0/4] Fix startup issues on OS X

[Arun Raghavan]

On Tue, 2015-04-21 at 08:00 +0200, Mihai Moldovan wrote:
> This patchset is based upon the work submitted by Stéphan Kochen,
> Jonathan "springermac" Springer and René J.V. Bertin in https://bugs.
> freedesktop.org/show_bug.cgi?id=62987
> 
> The fame goes to them, I'll take all the blame.
> 
> HAVE_COREAUDIO is added as a substitution to automake and in the code
> through autoconf.
> If HAVE_COREAUDIO is set to 1, module-coreaudio-detect is
> added/loaded in default.pa.in and system.pa.in.
> 
> Additionally, module-console-kit and module-systemd-login are
> disabled if HAVE_COREAUDIO is set to 1, as (on OS X), these modules
> cause PulseAudio to fail to start.
> 
> Mihai Moldovan (4):
>   configure.ac: add HAVE_COREAUDIO to automake and code.
>   default.pa.in: load module-coreaudio-detect if HAVE_COREAUDIO.
>   system.pa.in: load module-coreaudio-detect if HAVE_COREAUDIO.
>   default.pa.in: do not load module-console-kit and module-systemd-
> login
> if HAVE_COREAUDIO. It fails.
> 
>  configure.ac | 3 +++
>  src/daemon/default.pa.in | 6 ++
>  src/daemon/system.pa.in  | 4 
>  3 files changed, 13 insertions(+)

I've picked up the first three. The last seems to have been resolved
via other fixes already.

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

Re: [pulseaudio-discuss] [PATCH 0/2] Two volume factor fixes

[Arun Raghavan]

On 7 January 2016 at 18:55, Tanu Kaskinen  wrote:
> I noticed a strange error message about pa_cvolume_remap() failing in
> a server log, which turned out to be a bug in volume_factor_source
> handling. I found another bug when reviewing the volume factor code.
> Here are fixes for both bugs.
>
> Tanu Kaskinen (2):
>   source-output: do volume_factor_source application before resampling
>   source-output: remap volume_factor_source when starting move
>
>  src/pulsecore/source-output.c | 23 ++-
>  1 file changed, 10 insertions(+), 13 deletions(-)
>
> --

Both look good to me. Since they're non-critical, I'd prefer they went
to 'next'.

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

Re: [pulseaudio-discuss] [PATCH] stream: fix incorrect doc for pa_stream_writable_size()

[Arun Raghavan]

On 11 January 2016 at 18:58, Tanu Kaskinen  wrote:
> The old documentation implied that it wouldn't be possible to write
> more than the returned amount, which was incorrect.
> ---
>  src/pulse/stream.h | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/src/pulse/stream.h b/src/pulse/stream.h
> index 70fa415..802660d 100644
> --- a/src/pulse/stream.h
> +++ b/src/pulse/stream.h
> @@ -588,7 +588,14 @@ int pa_stream_peek(
>   * calling pa_stream_peek(). */
>  int pa_stream_drop(pa_stream *p);
>
> -/** Return the number of bytes that may be written using pa_stream_write(). 
> */
> +/** Return the number of bytes that the server has requested to be written.
> + *
> + * Contrary to what might be expected from the function name, it's usually
> + * possible to write more than the returned amount, but typically it doesn't
> + * make sense to do that, because that will likely make the stream latency
> + * exceed the target latency (which is configured with the tlength parameter 
> in
> + * pa_buffer_attr).
> + */
>  size_t pa_stream_writable_size(pa_stream *p);
>
>  /** Return the number of bytes that may be read using pa_stream_peek(). */
> --

I would rewrite this as:

"
Return the number of bytes requested by the server that have not yet
been written.

It is possible to write more than this amount, up to the stream's
buffer_attr.maxlength bytes. This is usually not desirable, though, as
it would increase stream latency to be higher than requested
(buffer_attr.tlength).
"

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

Re: [pulseaudio-discuss] [PATCH 1/2] SunOS: Catch up with newer API

[Arun Raghavan]

On 11 January 2016 at 19:37, David Henningsson
 wrote:
> Hi,
>
> this patch seems to need further explanation.
>
> I e, what "newer API", and why have we added a set_mute call in some places
> after get_mute and not others?

Probably just needs to reference commit e4a7625ba884c5cce20468d75937857343751c35

http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=e4a7625ba884c5cce20468d75937857343751c35

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

Re: [pulseaudio-discuss] Question about writable data

[Arun Raghavan]

On 11 January 2016 at 14:57, Alexandros Chronopoulos  wrote:
>
> On Fri, Jan 1, 2016 at 10:24 AM, Tanu Kaskinen  wrote:
>>
>>
>> I don't know how sound cards are usually designed, but I would guess
>> that yes, the audio pipelines on the same sound card probably share the
>> clock.
>
>
> Is there an API available to discover/compare the clock of the source/sink
> for each platform?

If there were such API, I'd expect it would need to be exposed by the
driver since that's where you might know about this.

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

Re: [pulseaudio-discuss] [PATCH] bluetooth: Prevent aborts caused by invalid module arguments

[Arun Raghavan]

On Mon, 2016-01-11 at 08:08 -0800, Jason Gerecke wrote:
> Hadn't noticed the lack of goto in module-bluetooth-policy.c... Given
> that it seems to be the usual pattern, would it be preferable to send
> a set of two patches: the first swapping out the "return -1" for a
> "goto fail" in module-bluetooth-policy.c:pa__init and the second
> patch
> being this one?

That's generally the preference but in this case, either could be fine
since it's also okay to group small but logically similar fixes in a
single change.

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

Re: [pulseaudio-discuss] [PATCH] stream: fix incorrect doc for pa_stream_writable_size()

[Arun Raghavan]

Pushed out this wording after an IRC ack from Tanu.

On 12 January 2016 at 08:35, Arun Raghavan  wrote:
> On 11 January 2016 at 18:58, Tanu Kaskinen  wrote:
>> The old documentation implied that it wouldn't be possible to write
>> more than the returned amount, which was incorrect.
>> ---
>>  src/pulse/stream.h | 9 -
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/pulse/stream.h b/src/pulse/stream.h
>> index 70fa415..802660d 100644
>> --- a/src/pulse/stream.h
>> +++ b/src/pulse/stream.h
>> @@ -588,7 +588,14 @@ int pa_stream_peek(
>>   * calling pa_stream_peek(). */
>>  int pa_stream_drop(pa_stream *p);
>>
>> -/** Return the number of bytes that may be written using pa_stream_write(). 
>> */
>> +/** Return the number of bytes that the server has requested to be written.
>> + *
>> + * Contrary to what might be expected from the function name, it's usually
>> + * possible to write more than the returned amount, but typically it doesn't
>> + * make sense to do that, because that will likely make the stream latency
>> + * exceed the target latency (which is configured with the tlength 
>> parameter in
>> + * pa_buffer_attr).
>> + */
>>  size_t pa_stream_writable_size(pa_stream *p);
>>
>>  /** Return the number of bytes that may be read using pa_stream_peek(). */
>> --
>
> I would rewrite this as:
>
> "
> Return the number of bytes requested by the server that have not yet
> been written.
>
> It is possible to write more than this amount, up to the stream's
> buffer_attr.maxlength bytes. This is usually not desirable, though, as
> it would increase stream latency to be higher than requested
> (buffer_attr.tlength).
> "
>
> -- Arun
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [ANNOUNCE] PulseAudio 8.0 RC2

[Arun Raghavan]

Hello,
Things seem to have been fairly stable with RC1, but we had a few small
changes worth an RC2, so here we go. If things continue to be quiet, I
think we can likely roll the release out next.

http://www.freedesktop.org/software/pulseaudio/releases/pulseaudio-7.99.2.tar.xz
MD5: 0028dea2cc88be8e874b63e9d1f4bd17
SHA1: b3b90489161c424202b841639a37c8e2357fa4c8

Changes include re-enabling timer-based scheduling on USB devices, and
fixes for OS X, and some minor build and bug fixes. A detailed
changelog will accompany the release.

As usual, please test and report any problems you find.

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

[pulseaudio-discuss] [ANNOUNCE] PulseAudio 8.0 RC2

[Arun Raghavan]

Hello,
Things seem to have been fairly stable with RC1, but we had a few small
changes worth an RC2, so here we go. If things continue to be quiet, I
think we can likely roll the release out next.

http://www.freedesktop.org/software/pulseaudio/releases/pulseaudio-7.99.2.tar.xz
MD5: 0028dea2cc88be8e874b63e9d1f4bd17
SHA1: b3b90489161c424202b841639a37c8e2357fa4c8

Changes include re-enabling timer-based scheduling on USB devices, and
fixes for OS X, and some minor build and bug fixes. A detailed
changelog will accompany the release.

As usual, please test and report any problems you find.

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

Re: [pulseaudio-discuss] [systemd-devel] user unit blocking login shell from popping out because wanted by default target?

[Arun Raghavan]

On 13 January 2016 at 02:19, Lennart Poettering  wrote:
> On Sun, 10.01.16 22:25, Tom Yan (tom.t...@gmail.com) wrote:
>
>> So I am recently experiencing some issue with pulseaudio (which I
>> already filed a bug report:
>> https://bugs.freedesktop.org/show_bug.cgi?id=93651) that it takes a
>> long time to start.
>>
>> The thing is, I am thinking whether it exposed a problem of systemd as
>> well. For example:
>>
>> Jan 10 21:31:33 localhost systemd[257]: Starting Sound Service...
>> Jan 10 21:31:33 localhost systemd[257]: Started D-Bus User Message Bus.
>> Jan 10 21:31:39 localhost systemd[257]: Started Sound Service.
>> Jan 10 21:31:39 localhost systemd[257]: Reached target Default.
>> Jan 10 21:31:39 localhost systemd[257]: Startup finished in 5.830s.
>>
>> As you can see, because of pulseaudio, it takes about 6 seconds to
>> reach the default target.
>>
>> The reason I realise that pulseaudio is having this issue, is because
>> I can actually "experience" the 6 seconds after I entered my password
>> in the tty, if I have pulseaudio.service enabled. The login shell only
>> pops up after the default target has been reached.
>
> Why would it wait for that?
>
> Also, PA taking 6s to start is quite something...

Yeah, that's clearly a (separate) bug, being tracked at:
https://bugs.freedesktop.org/show_bug.cgi?id=93651

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

[pulseaudio-discuss] Managing Android vs. UCM configuration

[Arun Raghavan]

Hi folks,
I'd spoken briefly about this at the Audio mini-summit at Dublin, and
promised to send out a link. I've been working on a tool to automate
translating Android mixer_paths.xml to UCM that PulseAudion (or CrAS
or anything else) can consume.

I've now written a bit about it at:

  http://arunraghavan.net/2016/01/audio-devices-and-configuration/

The code is up at:

  https://github.com/ford-prefect/xml2ucm

I'll try to provide a quicker way to take this stuff out for a spin
than building it yourself soon.

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

Re: [pulseaudio-discuss] Fwd: [LOW LATENCY] PA_simple

[Arun Raghavan]

On 13 January 2016 at 13:14, Tanu Kaskinen  wrote:
> On Wed, 2016-01-13 at 10:19 +0530, Swapnali Patil wrote:
>> Hello all,
>> we are using Pulse Audio API to playback audio received from network. So
>> far we are using pa_simple_api, with this we get latency of 2+ seconds.
>> From internet documents we got to about PA_STREAM_ADJUST_LATENCY flags. But
>> it seem this flag can be set for PA_STREAM_API.
>
> The PA_STREAM_ADJUST_LATENCY flag is automatically set for all streams
> created with the simple API.
>
>> We tried to set /use
>> PA_STREAM_API as mentioned below
>> . but it fails in
>>
>> pa_stream_new
>> () it self.
>>
>> pa_ml = pa_mainloop_new();
>> pa_mlapi = pa_mainloop_get_api(pa_ml);
>> pa_ctx = pa_context_new(pa_mlapi, "ivcloudapp");
>> pa_context_connect(pa_ctx, NULL,PA_CONTEXT_NOFLAGS, NULL);
>>
>>  if(!s)
>>  {
>> s =
>>
>> pa_stream_new(pa_ctx, "Playback", &ss, NULL);
>>if (!s) {
>>   printf("pa_stream_new failed\n");
>
> This will fail, because pa_context_connect() doesn't immediately
> establish the connection. You need to monitor the connection state by
> setting up a callback with pa_context_set_state_callback().
>
>> Please give direction how we can set low latency with pa_simple_api.
>
> The latency is controlled by the tlength parameter in pa_buffer_attr,
> which you pass to pa_simple_new().

Just to add to hat Tanu said, you can see one of our tests for how
this is supposed to work:

  
http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/tests/sync-playback.c

In general, you're probably going to want the async API anyway to deal
with a low latency application (that way you can provide exactly as
much data as requested when it is requested).

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

Re: [pulseaudio-discuss] Release notes draft completed

[Arun Raghavan]

On 14 January 2016 at 00:48, Kamil Rytarowski  wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
>
> On 13.01.2016 10:10, Tanu Kaskinen wrote:
>> Hi all,
>>
>> I finished the initial draft of the 8.0 release notes:
>> https://wiki.freedesktop.org/www/Software/PulseAudio/Notes/8.0/
>>
>> Is there anything missing or needing changes? Please reply this
>> message or just go ahead and edit the wiki page.
>>
>> Perhaps the NetBSD and/or OS X changes should be mentioned? Kamil
>> and Mihai, how would you summarize the changes you have made? It
>> seems that module-coreaudio-detect wasn't loaded by default in
>> previous versions, and that seems like an important change
>> affecting end users.

Yes, the main change is that our configuration should work
out-of-the-box on OS X.

> I would summarize them as reduced delta between pkgsrc and upstream
> version, pkgsrc is actively used by POSIX-compat systems, notably by
> NetBSD and SunOS.
>
> There are still two small nits I would like to get for 8.0 (besides
> the SunOS pending patch for OSS audio):
> - - cleanup the accept4/paccept(2)/accept(2) usage,
> - - improve handling of processors not handling quickly 64-bit operations.

We've frozen 8.0, so unless these are really trivial, I'd punt them
for merge after (we can still review and push the patches to our
'next' branch when they're ready, though).

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

Re: [pulseaudio-discuss] [PATCH v2] bluetooth: Prevent aborts caused by invalid module arguments

[Arun Raghavan]

On Wed, 2016-01-13 at 20:27 -0800, Jason Gerecke wrote:
> If 'pa_modargs_new' returns a NULL, we need to be careful to not call
> 'pa_modargs_free' in the failure path since it requires that we pass
> it
> a non-null argument. Also updates 'module-bluetooth-
> policy.c:pa__init'
> to follow the standard "goto fail" pattern used everywhere else.
> 
> Signed-off-by: Jason Gerecke 
> ---

Merged this now, thanks!

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

Re: [pulseaudio-discuss] [ANNOUNCE] PulseAudio 8.0 RC2

[Arun Raghavan]

On 12 January 2016 at 09:13, Arun Raghavan  wrote:
> Hello,
> Things seem to have been fairly stable with RC1, but we had a few small
> changes worth an RC2, so here we go. If things continue to be quiet, I
> think we can likely roll the release out next.
>
> http://www.freedesktop.org/software/pulseaudio/releases/pulseaudio-7.99.2.tar.xz
> MD5: 0028dea2cc88be8e874b63e9d1f4bd17
> SHA1: b3b90489161c424202b841639a37c8e2357fa4c8
>
> Changes include re-enabling timer-based scheduling on USB devices, and
> fixes for OS X, and some minor build and bug fixes. A detailed
> changelog will accompany the release.
>
> As usual, please test and report any problems you find.

As usual, there are Fedora RPMs up at:

  https://copr.fedoraproject.org/coprs/arunsr/pulseaudio-latest/

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

Re: [pulseaudio-discuss] [PATCH 1/2] SunOS: Catch up with newer API

[Arun Raghavan]

On Fri, 2016-01-15 at 14:33 +0100, Kamil Rytarowski wrote:
> On 12.01.2016 04:08, Arun Raghavan wrote:
> > On 11 January 2016 at 19:37, David Henningsson 
> >  wrote:
> > > Hi,
> > > 
> > > this patch seems to need further explanation.
> > > 
> > > I e, what "newer API", and why have we added a set_mute call in
> > > some places after get_mute and not others?
> > 
> > Probably just needs to reference commit
> > e4a7625ba884c5cce20468d75937857343751c35
> > 
> > http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=e4a762
> > 5ba
> 884c5cce20468d75937857343751c35
> > 
> > 
> Is there still need to improve the patch?

Not really, though I think I'd prefer to exchange the author and
attribution (i.e. Jonathan as author, and your contribution mentioned
in the commit message). I'll fix that up and mention the earlier commit
if there are no objections.

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

Re: [pulseaudio-discuss] [ANNOUNCE] PulseAudio 8.0 RC2

[Arun Raghavan]

On 15 January 2016 at 22:05, Thomas Martitz  wrote:
> Am 12.01.2016 um 04:43 schrieb Arun Raghavan:
>>
>> Changes include re-enabling timer-based scheduling on USB devices, and
>> fixes for OS X, and some minor build and bug fixes. A detailed
>> changelog will accompany the release.
>
>
> I'm experiencing distorted sound on my X-Fi Surround 5.1 USB sound card with
> realtime scheduling, under PA v7.1. Can I expect improvements with that
> change? Can I delete the "realtime-scheduling = no" from my
> ~/.config/pulse/daemon.conf?

That sounds unrelated. Could you provide logs for when that happens?
I've not seen this on my USB card.

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

[pulseaudio-discuss] Configuration system proposal (was: Toggling hardware volume for a card or port in pavucontrol)

[Arun Raghavan]

Hi folks,
Tanu already brought this up, posting some context:

On Fri, 2016-01-15 at 19:52 +0200, Tanu Kaskinen wrote:
> On Wed, 2016-01-13 at 16:19 +0200, Tanu Kaskinen wrote:
> > Hi,
> > 
> > Related to previous discussions[1][2][3] about dealing with volume
> > on
> > certain type of usb sound cards, I'd like to make it possible to
> > enable
> > and disable hardware volume for individual cards and/or ports in
> > pavucontrol.
> > 
> > Here's the specific use case: the Terratec Aureon Dual USB sound
> > card
> > has a single output jack that can used in both analog and digital
> > modes
> > (there are other similar usb sound cards too). PulseAudio has no
> > way to
> > know which mode is in use. Hardware volume works only in the analog
> > mode. Currently that means that PulseAudio's volume control has no
> > effect in the digital mode, because PulseAudio controls a mixer
> > element
> > that doesn't do anything.
> > 
> > I want to disable hardware volume for that card by default, because
> > that's the only safe default. That reduces the audio quality a bit,
> > however, so it would be good to allow concerned users to enable
> > hardware volume for the card without too much hassle.
> > 
> > I'd like some feedback to guide the design:
> > 
> > 0) Is this feature worth the maintenance burden the new code
> > causes, or
> > should I just forget about this? (My answer: it's worth doing.)
> > 
> > 1) Should the client API be specific to alsa, or should it be added
> > to
> > the core interface? A third option exists too: don't make it
> > specific
> > to alsa, but implement the client API in a module with a protocol
> > extension, and hook to the new module from alsa code. (My answer:
> > core.)
> 
> Arun commented in IRC that another alternative would be to use the
> "configuration system"[1] that Arun would like to implement. Although
> I
> had some reservations at first, I now think that the proposed
> configuration system fits this use case well, so I have nothing
> against
> doing the hardware volume toggle implementation that way. The recent
> discussion about the configuration system has been only between Arun
> and me in IRC, however, so it may be that others disagree about the
> desirability of the new system. Feedback about that, too, would be
> very
> welcome.
> 
> [1] 
> https://wiki.freedesktop.org/www/Software/PulseAudio/Documentation/Developer/ConfigurationSystem/

Comments would indeed be welcome, and I'm happy to expand on any of the
bits that aren't clear as well.

I'm not sure exactly when I'll be able to get to this, but having the
basic ideas hammered out should make it easier to get started whenever
time allows (or for anyone else to jump in as well).

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

Re: [pulseaudio-discuss] How to change the sink's priority in PulseAudio?

[Arun Raghavan]

On 16 January 2016 at 22:42, Michal Suchanek  wrote:
> Hello,
>
> On 15 January 2016 at 18:28, Лежанкин Иван  wrote:
>> Hi,
>>
>> I wonder on how to set the sink's priority in PulseAudio. I want some
>> devices to override others when they get connected. But the source code
>> inspection shows that the priority is calculated by hardcoded heuristics and
>> can't be modified from outside (cli, dbus, ...). Is it true, or do I miss
>> something?
>
> There does not seem to be any setting for this.
>
> Also even when the priority is higher existing streams are not going
> to be redirected. You either have to restart the application or use
> some pulseaudio foo to redirect the stream by hand.
>
> So in the end you probably want the scripts that switch playback
> between different sinks manually regardless of priority

You can look at module-device-manager which does have a way to
maintain per-device and per-role priorities for devices, and route
based on this.

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

Re: [pulseaudio-discuss] How to change the sink's priority in PulseAudio?

[Arun Raghavan]

On 15 January 2016 at 22:58, Лежанкин Иван  wrote:
[...]
> P.S. Is the G+ Community alive?

It is, somewhat, but the mailing list is more likely to get a quicker answer.

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

Re: [pulseaudio-discuss] [PATCH DEBUG] pulse: Enable memfd for client palyback

[Arun Raghavan]

On 17 January 2016 at 02:05, Ahmed S. Darwish  wrote:
> On Mon, Sep 21, 2015 at 03:20:01AM +0200, Ahmed S. Darwish wrote:
>> Hi,
>>
>> This is a debugging patch I'm sending to ask for help for a bug
>> I'm currently facing when enabling memfd for client playback audio.
>>
>> In this patch, I simply transform the client playback mempool
>> (context->mempool) to be backed by memfd shared memory instead of
>> posix SHM one.
>>
> [...]
>> when doing so for the playback mempool, sometimes pstreams code
>> reach do_read() with a flag PA_FLAG_MEMFD_FD (implying that this is
>> indeed a marshalled memfd-block frame), but with a reset ancillary
>> data field (nfd = 0) and no memfd descriptors
>>
>
> woot, bug cause discovered.
>
> This was an fd leak that, in interaction with UNIX sockets fd passing,
> manifested itself in a very weird manner!
>
> ## Situation:
>
> - Over time, PA daemon reaches its open fd limit (due to fd leak)
> - a client sends a memfd fd, as ancil data, over the socket
>
> ## Expected result:
>
> - Kernel fails at the recvmsg() system call, complaining with
>   something like -EMFILE
>
> ## What actually happens:
>
> - Kernel won't complain at all, or even return any kind of error. It
>   will just _silently_ strip the file descriptor from the passed ancil
>   message.
>
> - Naturally, this leads to broken expectations from the pstream side.
>   (pstream code expecting a passed file descriptor but finds nothing)
>
> - /me wondering how the passed fd could silently vanish in this manner
>   (originally blamed on a misunderstanding of pstreams behavior)
>
> Anyway, this was a blocker for this patch series that is now fixed :)

Yeeesh, nice catch. :-)

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

Re: [pulseaudio-discuss] [PATCH] sink-input: Don't access resampler to get silence memchunk

[Arun Raghavan]

On 16 January 2016 at 13:08, Tanu Kaskinen  wrote:
> On Wed, 2015-12-16 at 09:52 +0530, Arun Raghavan wrote:
>> On 16 December 2015 at 00:36, Tanu Kaskinen  wrote:
>> > On Tue, 2015-12-15 at 21:49 +0530, a...@accosted.net wrote:
>> > > From: Arun Raghavan 
>> > >
>> > > There doesn't appear to be a good reason to restrict the memchunk length
>> > > to the resample max block size -- we're going to have the memory around
>> > > anyway.
>> >
>> > I think the reason is to make sure that we don't feed the resampler
>> > bigger chunks than what it can handle. The resampler has to allocate
>> > other memblocks during its operation, and those memblocks may be bigger
>> > than the input block, so if the input block is too large, the
>> > requirements for the other blocks will grow beyond the mempool max
>> > block size.
>> >
>> > However, pa_sink_input_peek() seems to protect against this anyway when
>> > doing resampling (it processes the input in smaller pieces if it's
>> > larger than the resampler max block size), so maybe this change is safe
>> > anyway.
>> >
>> > > Moreover, callers of pa_sink_input_get_silence() don't seem to
>> > > actually care about the chunk itself, just the memblock for creating
>> > > their own pa_memblockq.
>> >
>> > I don't understand this comment. pa_memblockq cares about the chunk
>> > itself, not just the memblock.
>>
>> I misread that code. It does work with the chunk, not the memblock of course.
>
> Do you plan to send v2 of this patch some time soon?

Do you see a need to change anything other than the commit message? I
can just drop the last sentence.

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

Re: [pulseaudio-discuss] [PATCH v2 01/25] echo-cancel: Update webrtc-audio-processing usage to new API

[Arun Raghavan]

On 16 January 2016 at 14:03, Tanu Kaskinen  wrote:
> On Wed, 2015-12-16 at 09:09 +0530, a...@accosted.net wrote:
>> From: Arun Raghavan 
>>
>> ---
>>  configure.ac  |  2 +-
>>  src/Makefile.am   |  2 +-
>>  src/modules/echo-cancel/webrtc.cc | 54 
>> +--
>>  3 files changed, 31 insertions(+), 27 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index b9cd3d1..26c3e29 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1371,7 +1371,7 @@ AC_ARG_ENABLE([webrtc-aec],
>>  AS_HELP_STRING([--enable-webrtc-aec], [Enable the optional WebRTC-based 
>> echo canceller]))
>>
>>  AS_IF([test "x$enable_webrtc_aec" != "xno"],
>> -[PKG_CHECK_MODULES(WEBRTC, [ webrtc-audio-processing ], 
>> [HAVE_WEBRTC=1], [HAVE_WEBRTC=0])],
>> +[PKG_CHECK_MODULES(WEBRTC, [ webrtc-audio-processing > 0.1 ], 
>> [HAVE_WEBRTC=1], [HAVE_WEBRTC=0])],
>>  [HAVE_WEBRTC=0])
>>
>>  AS_IF([test "x$enable_webrtc_aec" = "xyes" && test "x$HAVE_WEBRTC" = "x0"],
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index f1bd38d..533b646 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -50,7 +50,7 @@ AM_CPPFLAGS = \
>>   -DPULSE_LOCALEDIR=\"$(localedir)\"
>>  AM_CFLAGS = \
>>   $(PTHREAD_CFLAGS)
>> -AM_CXXFLAGS = $(AM_CFLAGS)
>> +AM_CXXFLAGS = $(AM_CFLAGS) -std=c++11
>>  SERVER_CFLAGS = -D__INCLUDED_FROM_PULSE_AUDIO
>>
>>  AM_LIBADD = $(PTHREAD_LIBS) $(INTLLIBS)
>> diff --git a/src/modules/echo-cancel/webrtc.cc 
>> b/src/modules/echo-cancel/webrtc.cc
>> index 511c7ee..4a23377 100644
>> --- a/src/modules/echo-cancel/webrtc.cc
>> +++ b/src/modules/echo-cancel/webrtc.cc
>> @@ -33,8 +33,8 @@ PA_C_DECL_BEGIN
>>  #include "echo-cancel.h"
>>  PA_C_DECL_END
>>
>> -#include
>> -#include
>> +#include
>> +#include
>>
>>  #define BLOCK_SIZE_US 1
>>
>> @@ -80,6 +80,7 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
>> pa_sample_spec *out_ss, pa_channel_map *out_map,
>> uint32_t *nframes, const char *args) {
>>  webrtc::AudioProcessing *apm = NULL;
>> +webrtc::ProcessingConfig pconfig;
>>  bool hpf, ns, agc, dgc, mobile, cn;
>>  int rm = -1;
>>  pa_modargs *ma;
>> @@ -153,7 +154,7 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
>>  }
>>  }
>>
>> -apm = webrtc::AudioProcessing::Create(0);
>> +apm = webrtc::AudioProcessing::Create();
>>
>>  out_ss->format = PA_SAMPLE_S16NE;
>>  *play_ss = *out_ss;
>> @@ -163,22 +164,19 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller 
>> *ec,
>>  *rec_ss = *out_ss;
>>  *rec_map = *out_map;
>>
>> -apm->set_sample_rate_hz(out_ss->rate);
>> -
>> -apm->set_num_channels(out_ss->channels, out_ss->channels);
>> -apm->set_num_reverse_channels(play_ss->channels);
>> +pconfig = {
>> +webrtc::StreamConfig(out_ss->rate, out_ss->channels, false), /* 
>> input stream */
>> +webrtc::StreamConfig(out_ss->rate, out_ss->channels, false), /* 
>> output stream */
>> +webrtc::StreamConfig(out_ss->rate, out_ss->channels, false), /* 
>> reverse input stream */
>> +webrtc::StreamConfig(out_ss->rate, out_ss->channels, false), /* 
>> reverse output stream */
>> +};
>> +apm->Initialize(pconfig);
>>
>>  if (hpf)
>>  apm->high_pass_filter()->Enable(true);
>>
>>  if (!mobile) {
>> -if (ec->params.drift_compensation) {
>> -
>> apm->echo_cancellation()->set_device_sample_rate_hz(out_ss->rate);
>> -apm->echo_cancellation()->enable_drift_compensation(true);
>> -} else {
>> -apm->echo_cancellation()->enable_drift_compensation(false);
>> -}
>> -
>> +
>> apm->echo_cancellation()->enable_drift_compensation(ec->params.drift_compensation);
>>  apm->echo_cancellation()->Enable(true);
>>  } else {
>>  apm->echo_control_mobile()->set_routing_mode(static_cast(rm));
>> @@ -225,7 +223,7 @@ fail:
>>  if (ma)
>>  pa_modargs_free(ma);
>>  if (apm)
>> -webrtc::AudioProcessing::D

Re: [pulseaudio-discuss] [PATCH 1/2] SunOS: Catch up with newer API

[Arun Raghavan]

On Mon, 2016-01-18 at 08:04 +0100, Kamil Rytarowski wrote:
> On 18.01.2016 05:21, Arun Raghavan wrote:
> > On Fri, 2016-01-15 at 14:33 +0100, Kamil Rytarowski wrote:
> > > On 12.01.2016 04:08, Arun Raghavan wrote:
> > > > On 11 January 2016 at 19:37, David Henningsson 
> > > >  wrote:
> > > > > Hi,
> > > > > 
> > > > > this patch seems to need further explanation.
> > > > > 
> > > > > I e, what "newer API", and why have we added a set_mute call
> > > > > in some places after get_mute and not others?
> > > > 
> > > > Probably just needs to reference commit 
> > > > e4a7625ba884c5cce20468d75937857343751c35
> > > > 
> > > > http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=e4
> > > > a762
> > > > 
> > > > 
> 5ba
> > > 884c5cce20468d75937857343751c35
> > > > 
> > > > 
> > > Is there still need to improve the patch?
> > 
> > Not really, though I think I'd prefer to exchange the author and 
> > attribution (i.e. Jonathan as author, and your contribution
> > mentioned in the commit message). I'll fix that up and mention the
> > earlier commit if there are no objections.
> 
> Well,
> 
> Jonathan's added original patches to pkgsrc regarding this platform.
> SunOS 1/2 is unmodified his work.
> SunOS 2/2 was reworked by myself, I changed the code from #ifdef
> __sunos #endif to autoconf checks.
> 
> I'm OK with your proposal as long as it will be merged and delta
> between pkgsrc and upstream reduced.

Done on both counts.

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

Re: [pulseaudio-discuss] Using module-combine-sink with intermittent devices

[Arun Raghavan]

On 18 January 2016 at 03:20, Matt Feifarek  wrote:
> Hi there.
>
> Thanks for the fantastic PA; I'm enjoying its elegance and power, and have
> been for years. I've been trying to search the web and the mailing list for
> answers to my questions, but the docs are few and the mailing list is hard
> to search.
>
> ...
>
> Lately, I'm setting up a sort of a house-wide hifi setup. I'm driving
> various audio cables from a HEADLESS computer. I'm having a couple of
> challenges, and some questions:
>
> 1. In this situation (no actual users, headless) is it "ok" to use system
> mode? I've done lots of work-arounds to get PA to run from an account (the
> one for mpd). The docs are quite strenuous that it's basically never ok to
> use system mode, but seems like I'm pretty much in system land. No?

This should be okay. I thought systemd has (or was going to have) a
mechanism for per-user services to come up at boot, but I might've
been imagining this.

> 2. I want to use module-combine-sink to play through several sound sinks at
> the same time. It works, but things break down when one of the sinks
> disappears, which it does because it's a USB interface to a DAC in a
> receiver.

Breaks down?

> 3. module-combine seems to suffer with the idle detection, should I just
> turn that off?

Suffer in what way?

> 4. What's the difference between adding a "card" and adding a "sink"? Why
> would one use card?

A card is a representation of the physical device that provides audio
(so the sound card, for example). The theory is that a card provides a
profile for each configuration it supports, and a sink for each
playback stream it supports when a given profile is activated.

> I know that you can configure PA to move streams to newly appearing sinks,
> but I don't know how to reconfigure an existing sink (combined sink). I
> looked at updating property lists, but I don't think that's what it's for.
>
> And since there's no "script language" per-se, I have a similar problem at
> start-up: I can't refer to a missing sink when I want to declare the combine
> module... and if I wrap it in .nofail tags, it mostly works, but then I have
> to manually remove the module and re-init later when the USB device comes
> online.
>
> In short, I have two run-states: one with one soundcard, one with two; I
> want to support both transparently, playing the same source streams, without
> having to log into the server and run pacmd. Is this possible?

Are there more sound cards on this system that you do not want added
to the combine sink? If not, when you don't specify the "slaves"
argument to the module, it will automatically combine in all new
sinks.

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

Re: [pulseaudio-discuss] pulseaudio rtp setup for a specific IP and port

[Arun Raghavan]

Hi Abhiruchi,

On 18 January 2016 at 11:32, Gupta Abhiruchi (RBEI/ECF4)
 wrote:
> Hi,
> I am trying to setup rtp audio routing using pulseaudio rtp modules.
> Pulseaudio rtp setup works fine as multicast using rtp send and recv modules
> but when I am trying to pass a specific IP and port number while loading the
> rtp send module, I could not hear audio in my destination system.
> My requirement is to route audio to a specific IP and port using  rtp.
>
> What setup do I need to follow to achieve the same.

Are you able to see the audio packets go out on the sender side, and
if yes, are they coming in on the receiver side? (you could use
tcpdump/wireshark to check).

You need to make sure that both the SAP packets and RTP packets are
being received.

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

Re: [pulseaudio-discuss] [ANNOUNCE] PulseAudio 8.0 RC2

[Arun Raghavan]

On 19 January 2016 at 08:19, Felipe Sateler  wrote:
> On 15 January 2016 at 08:09, Arun Raghavan  wrote:
>> On 12 January 2016 at 09:13, Arun Raghavan  wrote:
>>> Hello,
>>> Things seem to have been fairly stable with RC1, but we had a few small
>>> changes worth an RC2, so here we go. If things continue to be quiet, I
>>> think we can likely roll the release out next.
>>>
>>> http://www.freedesktop.org/software/pulseaudio/releases/pulseaudio-7.99.2.tar.xz
>>> MD5: 0028dea2cc88be8e874b63e9d1f4bd17
>>> SHA1: b3b90489161c424202b841639a37c8e2357fa4c8
>>>
>>> Changes include re-enabling timer-based scheduling on USB devices, and
>>> fixes for OS X, and some minor build and bug fixes. A detailed
>>> changelog will accompany the release.
>>>
>>> As usual, please test and report any problems you find.
>>
>> As usual, there are Fedora RPMs up at:
>>
>>   https://copr.fedoraproject.org/coprs/arunsr/pulseaudio-latest/
>>
>
> I have also just uploaded the package to debian experimental.

Great! I'd like to try to roll the release out tomorrow unless
something comes up at the last minute.

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

Re: [pulseaudio-discuss] [RESEND] [PATCH v3 1/2] bluetooth: Add support for automatic switch between hsp and a2dp profiles

[Arun Raghavan]

On Wed, 2016-01-13 at 13:32 +0100, Pali Rohár wrote:
> With this patch module-bluetooth-policy automatically switch from
> a2dp profile
> to hsp profile if some VOIP application with media.role=phone wants
> to start
> recording audio.
> 
> By default a2dp profile is used for listening music, but for VOIP
> calls is
> needed profile with microphone support (hsp). So this patch will
> switch to
> hsp profile if some application want to use microphone (and specify
> it in
> media.role as "phone). After recording is stopped profile is switched
> back
> to a2dp. So this patch allows to use bluetooth microphone for VOIP
> applications
> with media.role=phone automatically without need of user interaction.

Thanks, I haven't tested this yet, but some quick comments.

> Signed-off-by: Pali Rohár 
> ---
>  src/modules/bluetooth/module-bluetooth-policy.c |  200
> ++-
>  1 file changed, 198 insertions(+), 2 deletions(-)
> 
> diff --git a/src/modules/bluetooth/module-bluetooth-policy.c
> b/src/modules/bluetooth/module-bluetooth-policy.c
> index 860868f..200fc84 100644
> --- a/src/modules/bluetooth/module-bluetooth-policy.c
> +++ b/src/modules/bluetooth/module-bluetooth-policy.c
> @@ -35,16 +35,18 @@
>  
>  #include "module-bluetooth-policy-symdef.h"
>  
> -PA_MODULE_AUTHOR("Frédéric Dalleau");
> -PA_MODULE_DESCRIPTION("When a bluetooth sink or source is added,
> load module-loopback");
> +PA_MODULE_AUTHOR("Frédéric Dalleau, Pali Rohár");
> +PA_MODULE_DESCRIPTION("Automatically switch between bluetooth hsp
> and a2dp profiles and automatically load module-loopback when a
> bluetooth sink (ag) or source (ag, a2dp_source) is added");

I'd just rewrite this to be "Policy module to make using bluetooth
devices out-of-the-box easier".

>  PA_MODULE_VERSION(PACKAGE_VERSION);
>  PA_MODULE_LOAD_ONCE(true);
>  PA_MODULE_USAGE(
> +"switch= "

Maybe call this "autoswitch".

>  "a2dp_source=
> "
>  "ag= role)?> "
>  "hfgw=
> DEPRECATED");
>  
>  static const char* const valid_modargs[] = {
> +"switch",
>  "a2dp_source",
>  "ag",
>  "hfgw",
> @@ -56,6 +58,10 @@ struct userdata {
>  bool enable_ag;
>  pa_hook_slot *source_put_slot;
>  pa_hook_slot *sink_put_slot;
> +pa_hook_slot *source_output_put_slot;
> +pa_hook_slot *source_output_unlink_slot;
> +pa_hook_slot *card_new_slot;
> +pa_hook_slot *card_unlink_slot;
>  pa_hook_slot *profile_available_changed_slot;
>  };
>  
> @@ -139,6 +145,163 @@ static pa_hook_result_t
> sink_put_hook_callback(pa_core *c, pa_sink *sink, void *
>  return PA_HOOK_OK;
>  }
>  
> +/* Switch profile for one card */
> +static void switch_profile(pa_card *card, bool revert) {

From a readability perspective, I think it makes sense to call this
"a2dp" rather than "revert".

> +const char *from_profile;
> +const char *to_profile;
> +pa_card_profile *profile;
> +const char *s;
> +void *state;
> +
> +if (revert) {
> +from_profile = "hsp";
> +to_profile = "a2dp";
> +} else {
> +from_profile = "a2dp";
> +to_profile = "hsp";
> +}
> +
> +/* Only consider bluetooth cards */
> +s = pa_proplist_gets(card->proplist, PA_PROP_DEVICE_BUS);
> +if (!s || !pa_streq(s, "bluetooth"))
> +return;
> +
> +if (revert) {
> +/* In revert phase only consider cards with revert flag */
> +s = pa_proplist_gets(card->proplist, "bluez-revert");
> +if (!s || !pa_streq(s, "true"))
> +return;
> +
> +/* Remove revert flag */
> +pa_proplist_unset(card->proplist, "bluez-revert");
> +}
> +
> +/* Skip card if does not have active from_profile */
> +if (!pa_streq(card->active_profile->name, from_profile))
> +return;
> +
> +/* Skip card if already has active profile to_profile */
> +if (pa_streq(card->active_profile->name, to_profile))
> +return;
> +
> +/* Find available to_profile and activate it */
> +PA_HASHMAP_FOREACH(profile, card->profiles, state) {
> +if (!pa_streq(profile->name, to_profile))
> +continue;
> +
> +if (profile->available == PA_AVAILABLE_NO)
> +continue;
> +
> +pa_log_debug("Setting card '%s' to profile '%s'", card-
> >name, to_profile);
> +
> +if (pa_card_set_profile(card, profile, false) != 0) {
> +pa_log_warn("Could not set profile '%s'", to_profile);
> +continue;
> +}
> +
> +/* When we are not in revert phase flag this card for revert
> */
> +if (!revert)
> +pa_proplist_sets(card->proplist, "bluez-revert",
> "true");
> +
> +        break;
> +}
> +}
> +
> +/* Return true if we should ignore this source output */
> +static bool ignore_output(pa_source_output *source_output) {
> +const char *s;
> +
> +/* New applications could set media.role for identifing streams
> */
> +/* We are iterested only in media.role=phone 

Re: [pulseaudio-discuss] A2DP AAC streaming Decoder

[Arun Raghavan]

On 21 January 2016 at 17:49, Hong-Rong Lin  wrote:
> Hi All,
> I would like to decode AAC frames from A2DP packets, and feed AAC data into 
> AAC audio decoder to output audio, ie, Bluetooth speaker function supporting 
> AAC streaming.
> I have found there are sbc_decode() source code in pulseaudio-2.1, but I 
> cannot find aac_decode().
> Could all of you tell me how to get the aac_decode() source code in the 
> internet? Thanks a lot.

My ideal solution for this would be to introduce an AAC format that
PulseAudio can directly provide, and clients can then deal with
decoding.

The challenge here is that you would need to deal with variable
bytes<->time conversion, which will potentially throw off latency
calculations. This should be solvable, though.

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

[pulseaudio-discuss] [ANNOUNCE] PulseAudio 8.0 is out!

[Arun Raghavan]

Hello,
I'm pleased to announce the availability of PulseAudio 8.0. A short
summary of the changes in this release:

 * Automatic routing more likely to change profile
 * OS X and NetBSD support improvements
 * Systemd journal logging for clients
 * New LFE balance programming interface
 * Module-dbus-protocol improvements
 * More flexible configuration file handling
 * pulsecore-8.0.so moved to a private directory
 * New script for measuring memory consumption
 * Various bug fixes and small improvements

Of note to packages is the availability of a more flexible
configuration for distribution-specific config, and the change in
installation path of the libpulsecore.

A more detailed changelog is up at:

  http://www.freedesktop.org/wiki/Software/PulseAudio/Notes/8.0/

The release is at the usual place:

http://www.freedesktop.org/software/pulseaudio/releases/pulseaudio-8.0.tar.xz
MD5: 8678442ba0bb4b4c33ac6f62542962df
SHA1: 1399a2f6288ad743184b6c2192129fef033343ac

To all 31 contributors in the git logs, and everyone else who's been
actively helping on IRC and Bugzilla -- thank you!

Cheers and a good weekend,
Arun
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v3 02/24] echo-cancel: Allow enabling the extended filter in webrtc AEC

[Arun Raghavan]

On 23 January 2016 at 20:36, Tanu Kaskinen  wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>> From: Arun Raghavan 
>>
>> This creates a longer filter that is more complex and less sensitive to
>> incorrect delay reporting from the hardware. There is also a
>> delay-agnostic mode that can eventually be enabled if required.
>>
>> In some very quick testing, not enabling this seems to provide better
>> results during double-talk.
>> ---
>>  src/modules/echo-cancel/webrtc.cc | 16 ++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/modules/echo-cancel/webrtc.cc 
>> b/src/modules/echo-cancel/webrtc.cc
>> index 3e5a144..f4f1395 100644
>> --- a/src/modules/echo-cancel/webrtc.cc
>> +++ b/src/modules/echo-cancel/webrtc.cc
>> @@ -46,6 +46,7 @@ PA_C_DECL_END
>>  #define DEFAULT_ROUTING_MODE "speakerphone"
>>  #define DEFAULT_COMFORT_NOISE true
>>  #define DEFAULT_DRIFT_COMPENSATION false
>> +#define DEFAULT_EXTENDED_FILTER false
>>
>>  static const char* const valid_modargs[] = {
>>  "high_pass_filter",
>> @@ -56,6 +57,7 @@ static const char* const valid_modargs[] = {
>>  "routing_mode",
>>  "comfort_noise",
>>  "drift_compensation",
>> +"extended_filter",
>>  NULL
>>  };
>>
>> @@ -81,7 +83,8 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
>> uint32_t *nframes, const char *args) {
>>  webrtc::AudioProcessing *apm = NULL;
>>  webrtc::ProcessingConfig pconfig;
>> -bool hpf, ns, agc, dgc, mobile, cn;
>> +webrtc::Config config;
>> +bool hpf, ns, agc, dgc, mobile, cn, ext_filter;
>>  int rm = -1;
>>  pa_modargs *ma;
>>
>> @@ -154,7 +157,16 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller 
>> *ec,
>>  }
>>  }
>>
>> -apm = webrtc::AudioProcessing::Create();
>> +ext_filter = DEFAULT_EXTENDED_FILTER;
>> +if (pa_modargs_get_value_boolean(ma, "extended_filter", &ext_filter) < 
>> 0) {
>> +pa_log("Failed to parse extended_filter value");
>> +goto fail;
>> +}
>> +
>> +if (ext_filter)
>> +config.Set(new 
>> webrtc::ExtendedFilter(true));
>
> Could these last two lines be replaced with
>
> config.Set(new 
> webrtc::ExtendedFilter(ext_filter));
>
> ?
>
> Otherwise the patch looks good.

Sure, I can fix that up.

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

Re: [pulseaudio-discuss] [PATCH v3 03/24] echo-cancel: Add support for the webrtc intelligibility enhancer

[Arun Raghavan]

On 24 January 2016 at 22:00, Tanu Kaskinen  wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>> From: Arun Raghavan 
>>
>> Just exposing this, disabled by default. It's not used by Chromium at
>> the moment.
>> ---
>>  src/modules/echo-cancel/webrtc.cc | 14 --
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/modules/echo-cancel/webrtc.cc 
>> b/src/modules/echo-cancel/webrtc.cc
>> index f4f1395..bbfa43f 100644
>> --- a/src/modules/echo-cancel/webrtc.cc
>> +++ b/src/modules/echo-cancel/webrtc.cc
>> @@ -47,6 +47,7 @@ PA_C_DECL_END
>>  #define DEFAULT_COMFORT_NOISE true
>>  #define DEFAULT_DRIFT_COMPENSATION false
>>  #define DEFAULT_EXTENDED_FILTER false
>> +#define DEFAULT_INTELLIGIBILITY_ENHANCER false
>>
>>  static const char* const valid_modargs[] = {
>>  "high_pass_filter",
>> @@ -58,6 +59,7 @@ static const char* const valid_modargs[] = {
>>  "comfort_noise",
>>  "drift_compensation",
>>  "extended_filter",
>> +"intelligibility_enhancer",
>>  NULL
>>  };
>>
>> @@ -84,7 +86,7 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
>>  webrtc::AudioProcessing *apm = NULL;
>>  webrtc::ProcessingConfig pconfig;
>>  webrtc::Config config;
>> -bool hpf, ns, agc, dgc, mobile, cn, ext_filter;
>> +bool hpf, ns, agc, dgc, mobile, cn, ext_filter, intelligibility;
>>  int rm = -1;
>>  pa_modargs *ma;
>>
>> @@ -163,8 +165,16 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller 
>> *ec,
>>  goto fail;
>>  }
>>
>> +intelligibility = DEFAULT_INTELLIGIBILITY_ENHANCER;
>> +if (pa_modargs_get_value_boolean(ma, "intelligibility_enhancer", 
>> &intelligibility) < 0) {
>> +pa_log("Failed to parse intelligibility_enhancer value");
>> +goto fail;
>> +}
>> +
>>  if (ext_filter)
>>  config.Set(new webrtc::ExtendedFilter(true));
>> +if (intelligibility)
>> +config.Set(new 
>> webrtc::Intelligibility(true));
>
> The same comment as in the previous patch: could the if statement be
> eliminated?

Ack.

>>
>>  apm = webrtc::AudioProcessing::Create(config);
>>
>> @@ -253,7 +263,7 @@ void pa_webrtc_ec_play(pa_echo_canceller *ec, const 
>> uint8_t *play) {
>>  pa_assert(play_frame.samples_per_channel_ <= 
>> webrtc::AudioFrame::kMaxDataSizeSamples);
>>  memcpy(play_frame.data_, play, ec->params.priv.webrtc.blocksize);
>>
>> -apm->AnalyzeReverseStream(&play_frame);
>> +apm->ProcessReverseStream(&play_frame);
>
> This looks like a potentially unrelated change. Why is this change
> done?

It is needed for the intelligibility enhancer to work, but I later
realised we don't actually support this -- unlike
AnalyzeReverseStream(), ProcessReverseStream() modifies the playback
samples. This is something that needs to be done in the future
(there's a later commit that disables this that I could not squash due
to some intermediate changes).

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

Re: [pulseaudio-discuss] [PATCH v3 03/24] echo-cancel: Add support for the webrtc intelligibility enhancer

[Arun Raghavan]

On 25 January 2016 at 13:30, Tanu Kaskinen  wrote:
> On Mon, 2016-01-25 at 09:36 +0530, Arun Raghavan wrote:
>> On 24 January 2016 at 22:00, Tanu Kaskinen  wrote:
>> > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>> > > @@ -253,7 +263,7 @@ void pa_webrtc_ec_play(pa_echo_canceller *ec, const 
>> > > uint8_t *play) {
>> > >  pa_assert(play_frame.samples_per_channel_ <= 
>> > > webrtc::AudioFrame::kMaxDataSizeSamples);
>> > >  memcpy(play_frame.data_, play, ec->params.priv.webrtc.blocksize);
>> > >
>> > > -apm->AnalyzeReverseStream(&play_frame);
>> > > +apm->ProcessReverseStream(&play_frame);
>> >
>> > This looks like a potentially unrelated change. Why is this change
>> > done?
>>
>> It is needed for the intelligibility enhancer to work, but I later
>> realised we don't actually support this -- unlike
>> AnalyzeReverseStream(), ProcessReverseStream() modifies the playback
>> samples. This is something that needs to be done in the future
>> (there's a later commit that disables this that I could not squash due
>> to some intermediate changes).
>
> Does that mean that the intelligibility enhancer feature doesn't work
> at all? In that case this patch should be dropped.

I was thinking of leaving this and the corresponding disable-patch in
as documentation of what doesn't work, but Ic an just drop it if
that's the preference.

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

Re: [pulseaudio-discuss] [PATCH v3 04/24] echo-cancel: Use correct sample spec parameters for webrtc AEC init

[Arun Raghavan]

On 25 January 2016 at 13:56, Tanu Kaskinen  wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>
>> From: Arun Raghavan 
>>
>> ---
>>  src/modules/echo-cancel/webrtc.cc | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/modules/echo-cancel/webrtc.cc 
>> b/src/modules/echo-cancel/webrtc.cc
>> index bbfa43f..ec63e26 100644
>> --- a/src/modules/echo-cancel/webrtc.cc
>> +++ b/src/modules/echo-cancel/webrtc.cc
>> @@ -187,10 +187,10 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller 
>> *ec,
>>  *rec_map = *out_map;
>>
>>  pconfig = {
>> -webrtc::StreamConfig(out_ss->rate, out_ss->channels, false), /* 
>> input stream */
>> +webrtc::StreamConfig(rec_ss->rate, rec_ss->channels, false), /* 
>> input stream */
>>  webrtc::StreamConfig(out_ss->rate, out_ss->channels, false), /* 
>> output stream */
>> -webrtc::StreamConfig(out_ss->rate, out_ss->channels, false), /* 
>> reverse input stream */
>> -webrtc::StreamConfig(out_ss->rate, out_ss->channels, false), /* 
>> reverse output stream */
>> +webrtc::StreamConfig(play_ss->rate, play_ss->channels, false), /* 
>> reverse input stream */
>> +webrtc::StreamConfig(play_ss->rate, play_ss->channels, false), /* 
>> reverse output stream */
>>  };
>>  apm->Initialize(pconfig);
>
> Shouldn't this be fixed in patch 1?

Not sure why I didn't while squashing the others. Will do so.

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

Re: [pulseaudio-discuss] [PATCH v3 03/24] echo-cancel: Add support for the webrtc intelligibility enhancer

[Arun Raghavan]

On Mon, 2016-01-25 at 13:01 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-25 at 15:03 +0530, Arun Raghavan wrote:
> > On 25 January 2016 at 13:30, Tanu Kaskinen  wrote:
> > > On Mon, 2016-01-25 at 09:36 +0530, Arun Raghavan wrote:
> > > > On 24 January 2016 at 22:00, Tanu Kaskinen 
> > > > wrote:
> > > > > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > > > > > @@ -253,7 +263,7 @@ void
> > > > > > pa_webrtc_ec_play(pa_echo_canceller *ec, const uint8_t
> > > > > > *play) {
> > > > > >  pa_assert(play_frame.samples_per_channel_ <=
> > > > > > webrtc::AudioFrame::kMaxDataSizeSamples);
> > > > > >  memcpy(play_frame.data_, play, ec-
> > > > > > >params.priv.webrtc.blocksize);
> > > > > > 
> > > > > > -apm->AnalyzeReverseStream(&play_frame);
> > > > > > +apm->ProcessReverseStream(&play_frame);
> > > > > 
> > > > > This looks like a potentially unrelated change. Why is this
> > > > > change
> > > > > done?
> > > > 
> > > > It is needed for the intelligibility enhancer to work, but I
> > > > later
> > > > realised we don't actually support this -- unlike
> > > > AnalyzeReverseStream(), ProcessReverseStream() modifies the
> > > > playback
> > > > samples. This is something that needs to be done in the future
> > > > (there's a later commit that disables this that I could not
> > > > squash due
> > > > to some intermediate changes).
> > > 
> > > Does that mean that the intelligibility enhancer feature doesn't
> > > work
> > > at all? In that case this patch should be dropped.
> > 
> > I was thinking of leaving this and the corresponding disable-patch
> > in
> > as documentation of what doesn't work, but Ic an just drop it if
> > that's the preference.
> 
> I'd prefer not merging broken patches. If the echo-cancel module has
> options for all webrtc features, then adding the warning here would
> be
> ok (instead of a separate patch), but if the feature coverage is
> partial anyway, then just drop the broken options.

I'll double-check, but I think I've covered all the options by the end
of the patchset.

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

Re: [pulseaudio-discuss] [PATCH] device-manager: improve logging about non-existing data

[Arun Raghavan]

On 26 January 2016 at 18:27, Tanu Kaskinen  wrote:
> Previously a missing key would cause this kind of log output:
>
> D: [pulseaudio] module-device-manager.c: Database contains invalid data for 
> key: sink:auto_null (probably pre-v1.0 data)
> D: [pulseaudio] module-device-manager.c: Attempting to load legacy (pre-v1.0) 
> data for key: sink:auto_null
> D: [pulseaudio] module-device-manager.c: Size does not match.
> D: [pulseaudio] module-device-manager.c: Unable to load legacy (pre-v1.0) 
> data for key: sink:auto_null. Ignoring.
>
> That is now replaced with
>
> D: [pulseaudio] module-device-manager.c: Database contains no data for key: 
> sink:auto_null
> ---
>  src/modules/module-device-manager.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/modules/module-device-manager.c 
> b/src/modules/module-device-manager.c
> index f125bdd..1a0a53e 100644
> --- a/src/modules/module-device-manager.c
> +++ b/src/modules/module-device-manager.c
> @@ -292,8 +292,10 @@ static struct entry* entry_read(struct userdata *u, 
> const char *name) {
>
>  pa_zero(data);
>
> -if (!pa_database_get(u->database, &key, &data))
> -goto fail;
> +if (!pa_database_get(u->database, &key, &data)) {
> +pa_log_debug("Database contains no data for key: %s", name);
> +return NULL;
> +}
>
>  t = pa_tagstruct_new_fixed(data.data, data.size);
>  e = entry_new();
> --

+1

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

Re: [pulseaudio-discuss] [PATCH v3 05/24] echo-cancel: Canceller may use different spec for playback and capture

[Arun Raghavan]

On Tue, 2016-01-26 at 13:42 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > From: Arun Raghavan 
> > 
> > This sets up the default sink sample spec to match what we expect,
> > rather than assuming that the canceller will set this up (our
> > assumption
> > is that we'll use 32 kHz mono unless someone explicitly overrides
> > this).
> 
> I have trouble understanding this description. Maybe rewording is
> needed?
> 
> What does "what we expect" mean? You set the rate and channels to 32
> kHz mono, why is that our "expectation"? Does something break if this
> expectation is violated via user configuration? The sink and source
> rates seem to be configurable (matching rates between source and sink
> is enforced, though). The sink channel setup seems to be forced to be
> mono, while the source channel map can be configured. What are the
> reasons behind this particular mix of configurability and hardcoding?
> Some comments about this in the sample spec initialization code might
> be a good idea.
> 
> I'm not sure why you describe the choice of using 32 kHz mono as an
> "assumption". Defaulting to 32 kHz mono is what the code does, in
> what
> way is that an assumption?
> 
> Overall, it remains unclear why this change is done.

In the original code, I picked 32 kHz mono as the format to keep
computational complexity at something that seemed would work well on
low-end hardware (basically netbooks) without losing too much in the
way of quality.

Prior to this change, the code assumes that the canceller will pick
something similar for us on the sink side, based on what we're
requesting on the source side (which is that fixed format).

Does that make more sense now?

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

Re: [pulseaudio-discuss] [PATCH v3 07/24] echo-cancel: Add a modarg to use sink/source master format and spec

[Arun Raghavan]

On Thu, 2016-01-28 at 09:40 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > From: Arun Raghavan 
> > 
> > This allows us to inherit the sample spec parameters from the sink
> > and
> > source master (rather than forcing 32 kHz / mono). It is still
> > possible
> > to override some of the parameters for the source side with
> > modargs.
> 
> What reasons do you see for using or not using parameters from the
> master devices? Should use_master_format perhaps be enabled by
> default?

My original testing showed that these parameters provided a decent
perf/quality trade-off on lower end hardware (which I no longer have
access to). I figure it makes sense to continue with that for now, and
in the future this can be relaxed (use_master_format=yes could be the
default, and resource-constrained systems can disable it).

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

Re: [pulseaudio-discuss] [PATCH v3 11/24] echo-cancel: Start capture at a sane value if we're doing webrtc AGC

[Arun Raghavan]

On Mon, 2016-02-01 at 13:18 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > From: Arun Raghavan 
> > 
> > This is required to make sure the capture output has sufficient
> > energy
> > for the AGC to do its job.
> 
> I think the word "value" in the first line of the commit message
> should
> be replaced with "volume". After reading just the commit message, I
> didn't know what the patch was about.
> 
> > ---
> >  src/modules/echo-cancel/echo-cancel.h |  1 +
> >  src/modules/echo-cancel/webrtc.cc | 14 +-
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/modules/echo-cancel/echo-cancel.h
> > b/src/modules/echo-cancel/echo-cancel.h
> > index 142f8ac..b570095 100644
> > --- a/src/modules/echo-cancel/echo-cancel.h
> > +++ b/src/modules/echo-cancel/echo-cancel.h
> > @@ -68,6 +68,7 @@ struct pa_echo_canceller_params {
> >  pa_sample_spec sample_spec;
> >  bool agc;
> >  bool trace;
> > +bool first;
> >  } webrtc;
> >  #endif
> >  /* each canceller-specific structure goes here */
> > diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-
> > cancel/webrtc.cc
> > index c2585a8..6431651 100644
> > --- a/src/modules/echo-cancel/webrtc.cc
> > +++ b/src/modules/echo-cancel/webrtc.cc
> > @@ -52,6 +52,7 @@ PA_C_DECL_END
> >  #define DEFAULT_TRACE false
> >  
> >  #define WEBRTC_AGC_MAX_VOLUME 255
> > +#define WEBRTC_AGC_START_VOLUME 85
> >  
> >  static const char* const valid_modargs[] = {
> >  "high_pass_filter",
> > @@ -297,6 +298,7 @@ bool pa_webrtc_ec_init(pa_core *c,
> > pa_echo_canceller *ec,
> >  ec->params.priv.webrtc.sample_spec = *out_ss;
> >  ec->params.priv.webrtc.blocksize =
> > (uint64_t)pa_bytes_per_second(out_ss) * BLOCK_SIZE_US /
> > PA_USEC_PER_SEC;
> >  *nframes = ec->params.priv.webrtc.blocksize /
> > pa_frame_size(out_ss);
> > +ec->params.priv.webrtc.first = true;
> >  
> >  pa_modargs_free(ma);
> >  return true;
> > @@ -354,7 +356,17 @@ void pa_webrtc_ec_record(pa_echo_canceller
> > *ec, const uint8_t *rec, uint8_t *out
> >  apm->ProcessStream(&out_frame);
> >  
> >  if (ec->params.priv.webrtc.agc) {
> > -new_volume = apm->gain_control()->stream_analog_level();
> > +if (PA_UNLIKELY(ec->params.priv.webrtc.first)) {
> > +/* We start at a sane default volume (taken from the
> > Chromium
> > + * condition on the experimental AGC in
> > audio_processing.h). This is
> > + * needed to make sure that there's enough energy in
> > the capture
> > + * signal for the AGC to work */
> > +ec->params.priv.webrtc.first = false;
> > +new_volume = WEBRTC_AGC_START_VOLUME;
> 
> This is done only on initial loading of the module. Does AGC work ok
> after resuming a suspended source?

After suspend/resume is not a problem since the source volume won't get
reset from whatever it was set to before. This is mostly to make sure
that when the AGC first starts, it is able to actually get going.

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

Re: [pulseaudio-discuss] [PATCH v3 09/24] echo-cancel: Allow enabling tracing output from the webrtc canceller

[Arun Raghavan]

On Sat, 2016-01-30 at 13:51 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > From: Arun Raghavan 
> > 
> > ---
> >  src/modules/echo-cancel/echo-cancel.h |  1 +
> >  src/modules/echo-cancel/webrtc.cc | 34
> > ++
> >  2 files changed, 35 insertions(+)
> > 
> > diff --git a/src/modules/echo-cancel/echo-cancel.h
> > b/src/modules/echo-cancel/echo-cancel.h
> > index 29d1574..142f8ac 100644
> > --- a/src/modules/echo-cancel/echo-cancel.h
> > +++ b/src/modules/echo-cancel/echo-cancel.h
> > @@ -67,6 +67,7 @@ struct pa_echo_canceller_params {
> >  uint32_t blocksize;
> >  pa_sample_spec sample_spec;
> >  bool agc;
> > +bool trace;
> >  } webrtc;
> >  #endif
> >  /* each canceller-specific structure goes here */
> > diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-
> > cancel/webrtc.cc
> > index 4dbd2fc..53ab7e1 100644
> > --- a/src/modules/echo-cancel/webrtc.cc
> > +++ b/src/modules/echo-cancel/webrtc.cc
> > @@ -35,6 +35,7 @@ PA_C_DECL_END
> >  
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #define BLOCK_SIZE_US 1
> >  
> > @@ -48,6 +49,7 @@ PA_C_DECL_END
> >  #define DEFAULT_DRIFT_COMPENSATION false
> >  #define DEFAULT_EXTENDED_FILTER false
> >  #define DEFAULT_INTELLIGIBILITY_ENHANCER false
> > +#define DEFAULT_TRACE false
> >  
> >  static const char* const valid_modargs[] = {
> >  "high_pass_filter",
> > @@ -60,6 +62,7 @@ static const char* const valid_modargs[] = {
> >  "drift_compensation",
> >  "extended_filter",
> >  "intelligibility_enhancer",
> > +"trace",
> >  NULL
> >  };
> >  
> > @@ -78,6 +81,20 @@ static int routing_mode_from_string(const char
> > *rmode) {
> >  return -1;
> >  }
> >  
> > +class PaWebrtcTraceCallback : public webrtc::TraceCallback {
> > +void Print(webrtc::TraceLevel level, const char *message, int
> > length)
> 
> I hope the presence of a length parameter doesn't mean that message
> isn't null-terminated.

I don't know why the length is there, but it is null-terminated.

> > +{
> > +if (level & webrtc::kTraceError || level &
> > webrtc::kTraceCritical)
> > +pa_log(message);
> > +else if (level & webrtc::kTraceWarning)
> > +pa_log_warn(message);
> > +else if (level & webrtc::kTraceInfo)
> > +pa_log_info(message);
> > +else
> > +pa_log_debug(message);
> > +}
> > +};
> > +
> >  static void pa_webrtc_ec_fixate_spec(pa_sample_spec *rec_ss,
> > pa_channel_map *rec_map,
> >   pa_sample_spec *play_ss,
> > pa_channel_map *play_map,
> >   pa_sample_spec *out_ss,
> > pa_channel_map *out_map)
> > @@ -201,6 +218,18 @@ bool pa_webrtc_ec_init(pa_core *c,
> > pa_echo_canceller *ec,
> >  if (intelligibility)
> >  config.Set(new webrtc::Intelligibility(true));
> >  
> > +ec->params.priv.webrtc.trace = DEFAULT_TRACE;
> > +if (pa_modargs_get_value_boolean(ma, "trace", &ec-
> > >params.priv.webrtc.trace) < 0) {
> > +pa_log("Failed to parse trace value");
> > +goto fail;
> > +}
> > +
> > +if (ec->params.priv.webrtc.trace) {
> > +webrtc::Trace::CreateTrace();
> > +webrtc::Trace::set_level_filter(webrtc::kTraceAll);
> > +webrtc::Trace::SetTraceCallback(new
> > PaWebrtcTraceCallback());
> 
> Does webrtc::Trace delete the object that you allocate here?

It does not. I've now redone this and will send out the update.

> > +}
> > +
> >  pa_webrtc_ec_fixate_spec(rec_ss, rec_map, play_ss, play_map,
> > out_ss, out_map);
> >  
> >  apm = webrtc::AudioProcessing::Create(config);
> > @@ -263,6 +292,8 @@ bool pa_webrtc_ec_init(pa_core *c,
> > pa_echo_canceller *ec,
> >  fail:
> >  if (ma)
> >  pa_modargs_free(ma);
> > +if (ec->params.priv.webrtc.trace)
> > +webrtc::Trace::ReturnTrace();
> 
> What does this do? Is it safe to call this if you haven't yet set up
> the tracing?

Drops the reference on the Trace objected created by CreateTrace().
Also fixed this up to be safe against initialisation.

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

Re: [pulseaudio-discuss] [RESEND] [PATCH v3 1/2] bluetooth: Add support for automatic switch between hsp and a2dp profiles

[Arun Raghavan]

On Tue, 2016-01-19 at 09:29 +0100, Pali Rohár wrote:
> On Tuesday 19 January 2016 11:12:40 Arun Raghavan wrote:
> > On Wed, 2016-01-13 at 13:32 +0100, Pali Rohár wrote:
> > > With this patch module-bluetooth-policy automatically switch from
> > > a2dp profile
> > > to hsp profile if some VOIP application with media.role=phone
> > > wants
> > > to start
> > > recording audio.
> > > 
> > > By default a2dp profile is used for listening music, but for VOIP
> > > calls is
> > > needed profile with microphone support (hsp). So this patch will
> > > switch to
> > > hsp profile if some application want to use microphone (and
> > > specify
> > > it in
> > > media.role as "phone). After recording is stopped profile is
> > > switched
> > > back
> > > to a2dp. So this patch allows to use bluetooth microphone for
> > > VOIP
> > > applications
> > > with media.role=phone automatically without need of user
> > > interaction.
> > 
> > Thanks, I haven't tested this yet, but some quick comments.
> 
> Thanks for review, this is just resent v3 version (originally sent
> year
> and half ago) which was without response.

Sorry about that. It's been a challenge trying to keep on top of the
patch inflow.

> > > Signed-off-by: Pali Rohár 
> > > ---
> > >  src/modules/bluetooth/module-bluetooth-policy.c |  200
> > > ++-
> > >  1 file changed, 198 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/modules/bluetooth/module-bluetooth-policy.c
> > > b/src/modules/bluetooth/module-bluetooth-policy.c
> > > index 860868f..200fc84 100644
> > > --- a/src/modules/bluetooth/module-bluetooth-policy.c
> > > +++ b/src/modules/bluetooth/module-bluetooth-policy.c
> > > @@ -35,16 +35,18 @@
> > >  
> > >  #include "module-bluetooth-policy-symdef.h"
> > >  
> > > -PA_MODULE_AUTHOR("Frédéric Dalleau");
> > > -PA_MODULE_DESCRIPTION("When a bluetooth sink or source is added,
> > > load module-loopback");
> > > +PA_MODULE_AUTHOR("Frédéric Dalleau, Pali Rohár");
> > > +PA_MODULE_DESCRIPTION("Automatically switch between bluetooth
> > > hsp
> > > and a2dp profiles and automatically load module-loopback when a
> > > bluetooth sink (ag) or source (ag, a2dp_source) is added");
> > 
> > I'd just rewrite this to be "Policy module to make using bluetooth
> > devices out-of-the-box easier".
> 
> Personally I'm fine with both descriptions so its up to other
> pulseaudio
> reviewers/maintainers.

I prefer the shorter name in the description, so let's go with that.

> > >  PA_MODULE_VERSION(PACKAGE_VERSION);
> > >  PA_MODULE_LOAD_ONCE(true);
> > >  PA_MODULE_USAGE(
> > > +"switch= "
> > 
> > Maybe call this "autoswitch".
> 
> "autoswitch" or "auto_switch"?

I'm not a huge fan of the underscores in our modarg names, but I'm fine
with either.

> > >  "a2dp_source= > > role)?>
> > > "
> > >  "ag= > > role)?> "
> > >  "hfgw=
> > > DEPRECATED");
> > >  
> > >  static const char* const valid_modargs[] = {
> > > +"switch",
> > >  "a2dp_source",
> > >  "ag",
> > >  "hfgw",
> > > @@ -56,6 +58,10 @@ struct userdata {
> > >  bool enable_ag;
> > >  pa_hook_slot *source_put_slot;
> > >  pa_hook_slot *sink_put_slot;
> > > +pa_hook_slot *source_output_put_slot;
> > > +pa_hook_slot *source_output_unlink_slot;
> > > +pa_hook_slot *card_new_slot;
> > > +pa_hook_slot *card_unlink_slot;
> > >  pa_hook_slot *profile_available_changed_slot;
> > >  };
> > >  
> > > @@ -139,6 +145,163 @@ static pa_hook_result_t
> > > sink_put_hook_callback(pa_core *c, pa_sink *sink, void *
> > >  return PA_HOOK_OK;
> > >  }
> > >  
> > > +/* Switch profile for one card */
> > > +static void switch_profile(pa_card *card, bool revert) {
> > 
> > From a readability perspective, I think it makes sense to call this
> > "a2dp" rather than "revert".
> 
> This whole patch is doing autoswitch to hsp mode. And revert param is
> reverting back from hsp back to a2dp.

Re: [pulseaudio-discuss] [RESEND] [PATCH v3 1/2] bluetooth: Add support for automatic switch between hsp and a2dp profiles

[Arun Raghavan]

On 2 February 2016 at 14:39, Pali Rohár  wrote:
> On Tuesday 02 February 2016 14:24:07 Arun Raghavan wrote:
>> > > > @@ -35,16 +35,18 @@
>> > > >
>> > > >  #include "module-bluetooth-policy-symdef.h"
>> > > >
>> > > > -PA_MODULE_AUTHOR("Frédéric Dalleau");
>> > > > -PA_MODULE_DESCRIPTION("When a bluetooth sink or source is added,
>> > > > load module-loopback");
>> > > > +PA_MODULE_AUTHOR("Frédéric Dalleau, Pali Rohár");
>> > > > +PA_MODULE_DESCRIPTION("Automatically switch between bluetooth
>> > > > hsp
>> > > > and a2dp profiles and automatically load module-loopback when a
>> > > > bluetooth sink (ag) or source (ag, a2dp_source) is added");
>> > >
>> > > I'd just rewrite this to be "Policy module to make using bluetooth
>> > > devices out-of-the-box easier".
>> >
>> > Personally I'm fine with both descriptions so its up to other
>> > pulseaudio
>> > reviewers/maintainers.
>>
>> I prefer the shorter name in the description, so let's go with that.
>
> Ok.
>
>> > > >  PA_MODULE_VERSION(PACKAGE_VERSION);
>> > > >  PA_MODULE_LOAD_ONCE(true);
>> > > >  PA_MODULE_USAGE(
>> > > > +"switch= "
>> > >
>> > > Maybe call this "autoswitch".
>> >
>> > "autoswitch" or "auto_switch"?
>>
>> I'm not a huge fan of the underscores in our modarg names, but I'm fine
>> with either.
>
> Ok.
>
>> > > > +/* Flag this card for revert */
>> > > > +pa_proplist_sets(new_data->proplist, "bluez-revert",
>> > > > "true");
>> > > > +return PA_HOOK_OK;
>> > >
>> > > What is the reasoning behind having this property?
>> >
>> > For each card I need to know if card needs to be "reverted" in
>> > future.
>>
>> Is this for a case where a device is manually kept in the hsp profile
>> and you don't want to touch that?
>
> Yes. When user chose manually to use hsp profile.
>
>> > > In general, I prefer the module doing its book-keeping locally
>> > > rather
>> > > than (ab)using the proplist for state.
>> >
>> > Ok, what to use then? How to store for each private boolean flag
>> > needed
>> > just by this module?
>>
>> Usually you'd do something like have a hashmap in the module's
>> userdata, and then you could add your pa_card to that hashmap if it
>> needs a revert (and remove when you're done with the revert). The
>> userdata is available in all the hook callbacks.
>
> But then I need to deal with removing card (e.g. somebody unplug usb
> bluetooth adapter when voice call via hsp profile is active)... And
> maybe other scenarios?

Yes. You just need to use PA_CORE_HOOK_CARD_UNLINK.

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

Re: [pulseaudio-discuss] [PATCH v3 05/24] echo-cancel: Canceller may use different spec for playback and capture

[Arun Raghavan]

On 2 February 2016 at 18:06, Tanu Kaskinen  wrote:
> On Tue, 2016-02-02 at 09:53 +0530, Arun Raghavan wrote:
>> On Tue, 2016-01-26 at 13:42 +0200, Tanu Kaskinen wrote:
>> > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>> > > From: Arun Raghavan 
>> > >
>> > > This sets up the default sink sample spec to match what we expect,
>> > > rather than assuming that the canceller will set this up (our
>> > > assumption
>> > > is that we'll use 32 kHz mono unless someone explicitly overrides
>> > > this).
>> >
>> > I have trouble understanding this description. Maybe rewording is
>> > needed?
>> >
>> > What does "what we expect" mean? You set the rate and channels to 32
>> > kHz mono, why is that our "expectation"? Does something break if this
>> > expectation is violated via user configuration? The sink and source
>> > rates seem to be configurable (matching rates between source and sink
>> > is enforced, though). The sink channel setup seems to be forced to be
>> > mono, while the source channel map can be configured. What are the
>> > reasons behind this particular mix of configurability and hardcoding?
>> > Some comments about this in the sample spec initialization code might
>> > be a good idea.
>> >
>> > I'm not sure why you describe the choice of using 32 kHz mono as an
>> > "assumption". Defaulting to 32 kHz mono is what the code does, in
>> > what
>> > way is that an assumption?
>> >
>> > Overall, it remains unclear why this change is done.
>>
>> In the original code, I picked 32 kHz mono as the format to keep
>> computational complexity at something that seemed would work well on
>> low-end hardware (basically netbooks) without losing too much in the
>> way of quality.
>>
>> Prior to this change, the code assumes that the canceller will pick
>> something similar for us on the sink side, based on what we're
>> requesting on the source side (which is that fixed format).
>>
>> Does that make more sense now?
>
> Maybe. Is this correct: The intention has always been to configure low
> enough parameters to keep CPU consumption down. Prior to this change,
> we assumed that the EC backend would override the sink parameters based
> on the source parameters to achieve this goal, and with this change we
> remove that assumption by forcing the default parameters for the sink
> to be low enough.
>
> I hope you rewrite the commit message.

Yep, I'll rewrite the message, probably just reusing this text.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v3 11/24] echo-cancel: Start capture at a sane value if we're doing webrtc AGC

[Arun Raghavan]

On 2 February 2016 at 18:12, Tanu Kaskinen  wrote:
> On Tue, 2016-02-02 at 09:59 +0530, Arun Raghavan wrote:
>> On Mon, 2016-02-01 at 13:18 +0200, Tanu Kaskinen wrote:
>> > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>> > > From: Arun Raghavan 
>> > >
>> > > This is required to make sure the capture output has sufficient
>> > > energy
>> > > for the AGC to do its job.
>> >
>> > I think the word "value" in the first line of the commit message
>> > should
>> > be replaced with "volume". After reading just the commit message, I
>> > didn't know what the patch was about.
>> >
>> > > ---
>> > >  src/modules/echo-cancel/echo-cancel.h |  1 +
>> > >  src/modules/echo-cancel/webrtc.cc | 14 +-
>> > >  2 files changed, 14 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/src/modules/echo-cancel/echo-cancel.h
>> > > b/src/modules/echo-cancel/echo-cancel.h
>> > > index 142f8ac..b570095 100644
>> > > --- a/src/modules/echo-cancel/echo-cancel.h
>> > > +++ b/src/modules/echo-cancel/echo-cancel.h
>> > > @@ -68,6 +68,7 @@ struct pa_echo_canceller_params {
>> > >  pa_sample_spec sample_spec;
>> > >  bool agc;
>> > >  bool trace;
>> > > +bool first;
>> > >  } webrtc;
>> > >  #endif
>> > >  /* each canceller-specific structure goes here */
>> > > diff --git a/src/modules/echo-cancel/webrtc.cc b/src/modules/echo-
>> > > cancel/webrtc.cc
>> > > index c2585a8..6431651 100644
>> > > --- a/src/modules/echo-cancel/webrtc.cc
>> > > +++ b/src/modules/echo-cancel/webrtc.cc
>> > > @@ -52,6 +52,7 @@ PA_C_DECL_END
>> > >  #define DEFAULT_TRACE false
>> > >
>> > >  #define WEBRTC_AGC_MAX_VOLUME 255
>> > > +#define WEBRTC_AGC_START_VOLUME 85
>> > >
>> > >  static const char* const valid_modargs[] = {
>> > >  "high_pass_filter",
>> > > @@ -297,6 +298,7 @@ bool pa_webrtc_ec_init(pa_core *c,
>> > > pa_echo_canceller *ec,
>> > >  ec->params.priv.webrtc.sample_spec = *out_ss;
>> > >  ec->params.priv.webrtc.blocksize =
>> > > (uint64_t)pa_bytes_per_second(out_ss) * BLOCK_SIZE_US /
>> > > PA_USEC_PER_SEC;
>> > >  *nframes = ec->params.priv.webrtc.blocksize /
>> > > pa_frame_size(out_ss);
>> > > +ec->params.priv.webrtc.first = true;
>> > >
>> > >  pa_modargs_free(ma);
>> > >  return true;
>> > > @@ -354,7 +356,17 @@ void pa_webrtc_ec_record(pa_echo_canceller
>> > > *ec, const uint8_t *rec, uint8_t *out
>> > >  apm->ProcessStream(&out_frame);
>> > >
>> > >  if (ec->params.priv.webrtc.agc) {
>> > > -new_volume = apm->gain_control()->stream_analog_level();
>> > > +if (PA_UNLIKELY(ec->params.priv.webrtc.first)) {
>> > > +/* We start at a sane default volume (taken from the
>> > > Chromium
>> > > + * condition on the experimental AGC in
>> > > audio_processing.h). This is
>> > > + * needed to make sure that there's enough energy in
>> > > the capture
>> > > + * signal for the AGC to work */
>> > > +ec->params.priv.webrtc.first = false;
>> > > +new_volume = WEBRTC_AGC_START_VOLUME;
>> >
>> > This is done only on initial loading of the module. Does AGC work ok
>> > after resuming a suspended source?
>>
>> After suspend/resume is not a problem since the source volume won't get
>> reset from whatever it was set to before. This is mostly to make sure
>> that when the AGC first starts, it is able to actually get going.
>
> Ok. Is the problem that if the initial volume is too low, AGC won't
> work, but too high initial volume is fine? What if the user sets the

Yes (but too high does mean the far end gets a burst of loud audio to
start with).

> source volume manually? Does AGC stop working? If not, why is that
> different from the initialization phase? (I'm just trying to understand
> the problem. If manual volume changes mess up AGC, I don't think that's
> something that this patch needs to care about.)

If the user modifies the volume, it does indeed stop working.

This actually is a bit of a regression in the library. I'm hoping to
do an update again in the coming month so will test again to see if
that fixes things.

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

Re: [pulseaudio-discuss] [PATCH v3 12/24] echo-cancel: Allow enabling of the webrtc experimental AGC mechanism

[Arun Raghavan]

On Tue, 2016-02-02 at 15:51 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > From: Arun Raghavan 
> > 
> > ---
> >  src/modules/echo-cancel/webrtc.cc | 12 +++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> How does this interact with "regular" AGC? Can both be enabled at the
> same time?

It's a modifier -- if AGC is enabled and experimental_agc is enabled,
the experimental code gets selected. If AGC is selected, the
experimental_agc setting is ignored.

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

Re: [pulseaudio-discuss] [PATCH v3 14/24] build-sys: Move to compiling with C11 support

[Arun Raghavan]

On Thu, 2016-02-04 at 06:04 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > @@ -176,6 +175,8 @@ esac
> >  
> >   Compiler flags 
> >  
> > +AX_CHECK_COMPILE_FLAG([-std=c11], [CFLAGS="$CFLAGS -std=c11"], [],
> > [-pedantic -Werror])
> 
> This does nothing if the compile flag isn't supported. Shouldn't we
> fail in configure if -std=c11 doesn't work?

Right, I'm rewriting this line as:

AX_CHECK_COMPILE_FLAG([-std=c11],
   [CFLAGS="$CFLAGS -std=c11"],
   [AC_MSG_ERROR([*** Compiler does not support -std=c11])],
   [-pedantic -Werror])

> Also, I don't think we should set CFLAGS. Adding -std=c11 to
> AM_CFLAGS
> in src/Makefile.am seems like the right thing to do. See
> https://www.gnu.org/software/automake/manual/html_node/User-Variables
> .html#User-Variables

This would make it inconsistent with the rest of configure.ac, though.
It'd be nice to change everything to use AM_CFLAGS but that should be a
separate change.

One more thing that's missing in this change is an addition
of ax_check_compile_flag.m4. I'll squash that in too.

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

Re: [pulseaudio-discuss] [PATCH v3 14/24] build-sys: Move to compiling with C11 support

[Arun Raghavan]

On 4 February 2016 at 10:22, Arun Raghavan  wrote:
> On Thu, 2016-02-04 at 06:04 +0200, Tanu Kaskinen wrote:
>> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>> > @@ -176,6 +175,8 @@ esac
>> >
>> >   Compiler flags 
>> >
>> > +AX_CHECK_COMPILE_FLAG([-std=c11], [CFLAGS="$CFLAGS -std=c11"], [],
>> > [-pedantic -Werror])
>>
>> This does nothing if the compile flag isn't supported. Shouldn't we
>> fail in configure if -std=c11 doesn't work?
>
> Right, I'm rewriting this line as:
>
> AX_CHECK_COMPILE_FLAG([-std=c11],
>[CFLAGS="$CFLAGS -std=c11"],
>[AC_MSG_ERROR([*** Compiler does not support -std=c11])],
>[-pedantic -Werror])
>
>> Also, I don't think we should set CFLAGS. Adding -std=c11 to
>> AM_CFLAGS
>> in src/Makefile.am seems like the right thing to do. See
>> https://www.gnu.org/software/automake/manual/html_node/User-Variables
>> .html#User-Variables
>
> This would make it inconsistent with the rest of configure.ac, though.
> It'd be nice to change everything to use AM_CFLAGS but that should be a
> separate change.
>
> One more thing that's missing in this change is an addition
> of ax_check_compile_flag.m4. I'll squash that in too.

The AX_* macros also seem to work with CFLAGS, btw.

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

Re: [pulseaudio-discuss] [PATCH v3 14/24] build-sys: Move to compiling with C11 support

[Arun Raghavan]

On Thu, 2016-02-04 at 07:13 +0200, Tanu Kaskinen wrote:
> On Thu, 2016-02-04 at 10:30 +0530, Arun Raghavan wrote:
> > On 4 February 2016 at 10:22, Arun Raghavan 
> > wrote:
> > > On Thu, 2016-02-04 at 06:04 +0200, Tanu Kaskinen wrote:
> > > > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > > > > @@ -176,6 +175,8 @@ esac
> > > > > 
> > > > >   Compiler flags 
> > > > > 
> > > > > +AX_CHECK_COMPILE_FLAG([-std=c11], [CFLAGS="$CFLAGS
> > > > > -std=c11"], [],
> > > > > [-pedantic -Werror])
> > > > 
> > > > This does nothing if the compile flag isn't supported.
> > > > Shouldn't we
> > > > fail in configure if -std=c11 doesn't work?
> > > 
> > > Right, I'm rewriting this line as:
> > > 
> > > AX_CHECK_COMPILE_FLAG([-std=c11],
> > >    [CFLAGS="$CFLAGS -std=c11"],
> > >    [AC_MSG_ERROR([*** Compiler does not support -std=c11])],
> > >    [-pedantic -Werror])
> > > 
> > > > Also, I don't think we should set CFLAGS. Adding -std=c11 to
> > > > AM_CFLAGS
> > > > in src/Makefile.am seems like the right thing to do. See
> > > > https://www.gnu.org/software/automake/manual/html_node/User-Var
> > > > iables
> > > > .html#User-Variables
> > > 
> > > This would make it inconsistent with the rest of configure.ac,
> > > though.
> > > It'd be nice to change everything to use AM_CFLAGS but that
> > > should be a
> > > separate change.
> 
> I don't buy this argument. There's no consistency in the first place.
> We set some flags via CFLAGS and some via AM_CFLAGS.

Looking at this again, I don't see any place in the configure checks
where AM_CFLAGS is set (and it looks to me like that this would not be
appropriate anyway -- AM_CFLAGS seems meant to be used withing
Makefile.am, not configure.ac).

> > > One more thing that's missing in this change is an addition
> > > of ax_check_compile_flag.m4. I'll squash that in too.
> > 
> > The AX_* macros also seem to work with CFLAGS, btw.
> 
> What do you mean by that?

If you look at what AX_APPEND_COMPILE_FLAGS does, for example, it sets
CFLAGS directly.

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

Re: [pulseaudio-discuss] [PATCH v3 14/24] build-sys: Move to compiling with C11 support

[Arun Raghavan]

On Thu, 2016-02-04 at 12:45 +0200, Tanu Kaskinen wrote:
> On Thu, 2016-02-04 at 10:57 +0530, Arun Raghavan wrote:
> > On Thu, 2016-02-04 at 07:13 +0200, Tanu Kaskinen wrote:
> > > I don't buy this argument. There's no consistency in the first
> > > place.
> > > We set some flags via CFLAGS and some via AM_CFLAGS.
> > 
> > Looking at this again, I don't see any place in the configure
> > checks
> > where AM_CFLAGS is set (and it looks to me like that this would not
> > be
> > appropriate anyway -- AM_CFLAGS seems meant to be used withing
> > Makefile.am, not configure.ac).
> 
> Yes, configure doesn't set AM_CFLAGS. I didn't suggest setting
> AM_CFLAGS from the configure script.
> 
> For example, gcov flags are set in AM_CFLAGS in Makefile.am based on
> configure results. So configure can be used to check things, and the
> flags can be set according to the checks to AM_CFLAGS, all without
> touching CFLAGS.
> 
> > > > > One more thing that's missing in this change is an addition
> > > > > of ax_check_compile_flag.m4. I'll squash that in too.
> > > > 
> > > > The AX_* macros also seem to work with CFLAGS, btw.
> > > 
> > > What do you mean by that?
> > 
> > If you look at what AX_APPEND_COMPILE_FLAGS does, for example, it
> > sets
> > CFLAGS directly.
> 
> AX_APPEND_COMPILE_FLAGS takes as a parameter the variable to which
> the
> flags are appended. Only when the parameter is omitted the macro will
> modify CFLAGS. We could save the flags to a separate variable that is
> added to AM_CFLAGS in Makefile.am.
> 
> We only use AX_APPEND_COMPILE_FLAGS to set non-mandatory flags,
> though.
> Nothing breaks if the user overrides those flags.

Fair enough. I'll drop the success clause in AX_CHECK_COMPILE_FLAG and
add -std=c11 to AM_CFLAGS in Makefile.am.

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

Re: [pulseaudio-discuss] [PATCH v3 17/24] echo-cancel: Fix webrtc canceller when rec channels != play channels

[Arun Raghavan]

On 8 February 2016 at 13:32, Tanu Kaskinen  wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>> From: Arun Raghavan 
>>
>> The calculations around how many samples were sent to the canceller
>> engine was not updated when we started supporting different channel
>> counts for playback and capture.
>> ---
>>  src/modules/echo-cancel/echo-cancel.h |  4 ++--
>>  src/modules/echo-cancel/webrtc.cc | 25 +
>>  2 files changed, 15 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/modules/echo-cancel/echo-cancel.h 
>> b/src/modules/echo-cancel/echo-cancel.h
>> index 37f99c0..4693516 100644
>> --- a/src/modules/echo-cancel/echo-cancel.h
>> +++ b/src/modules/echo-cancel/echo-cancel.h
>> @@ -64,8 +64,8 @@ struct pa_echo_canceller_params {
>>  /* This is a void* so that we don't have to convert this whole 
>> file
>>   * to C++ linkage. apm is a pointer to an AudioProcessing 
>> object */
>>  void *apm;
>> -uint32_t blocksize;
>> -pa_sample_spec sample_spec;
>> +int32_t blocksize; /* in frames */
>
> Why is the type changed from unsigned to signed? It doesn't look like
> you need negative values.

I meant to change that to just an int, rather than specify the size,
which is unnecessary here.

>> +pa_sample_spec rec_ss, play_ss;
>>  bool agc;
>>  bool trace;
>>  bool first;
>> diff --git a/src/modules/echo-cancel/webrtc.cc 
>> b/src/modules/echo-cancel/webrtc.cc
>> index ec0a383..2732b38 100644
>> --- a/src/modules/echo-cancel/webrtc.cc
>> +++ b/src/modules/echo-cancel/webrtc.cc
>> @@ -327,9 +327,11 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller 
>> *ec,
>>  apm->voice_detection()->Enable(true);
>>
>>  ec->params.webrtc.apm = apm;
>> -ec->params.webrtc.sample_spec = *out_ss;
>> -ec->params.webrtc.blocksize = (uint64_t)pa_bytes_per_second(out_ss) * 
>> BLOCK_SIZE_US / PA_USEC_PER_SEC;
>> -*nframes = ec->params.webrtc.blocksize / pa_frame_size(out_ss);
>> +ec->params.webrtc.rec_ss = *rec_ss;
>> +ec->params.webrtc.play_ss = *play_ss;
>> +ec->params.webrtc.blocksize =
>> +(uint64_t) (pa_bytes_per_second(out_ss) / pa_frame_size(out_ss)) * 
>> BLOCK_SIZE_US / PA_USEC_PER_SEC;
>
> pa_bytes_per_second(out_ss) / pa_frame_size(out_ss) calculates the
> sample rate, so it can be replaced with out_ss->rate.

Ack.

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

Re: [pulseaudio-discuss] [PATCH v3 17/24] echo-cancel: Fix webrtc canceller when rec channels != play channels

[Arun Raghavan]

On 8 February 2016 at 14:52, Arun Raghavan  wrote:
> On 8 February 2016 at 13:32, Tanu Kaskinen  wrote:
>> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>>> From: Arun Raghavan 
>>>
>>> The calculations around how many samples were sent to the canceller
>>> engine was not updated when we started supporting different channel
>>> counts for playback and capture.
>>> ---
>>>  src/modules/echo-cancel/echo-cancel.h |  4 ++--
>>>  src/modules/echo-cancel/webrtc.cc | 25 +
>>>  2 files changed, 15 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/modules/echo-cancel/echo-cancel.h 
>>> b/src/modules/echo-cancel/echo-cancel.h
>>> index 37f99c0..4693516 100644
>>> --- a/src/modules/echo-cancel/echo-cancel.h
>>> +++ b/src/modules/echo-cancel/echo-cancel.h
>>> @@ -64,8 +64,8 @@ struct pa_echo_canceller_params {
>>>  /* This is a void* so that we don't have to convert this whole 
>>> file
>>>   * to C++ linkage. apm is a pointer to an AudioProcessing 
>>> object */
>>>  void *apm;
>>> -uint32_t blocksize;
>>> -pa_sample_spec sample_spec;
>>> +int32_t blocksize; /* in frames */
>>
>> Why is the type changed from unsigned to signed? It doesn't look like
>> you need negative values.
>
> I meant to change that to just an int, rather than specify the size,
> which is unnecessary here.

That was meant to be unsigned int, which is what I'm changing it to.

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

Re: [pulseaudio-discuss] [PATCH 4/5] rtpoll: Implement pa_mainloop_api support

[Arun Raghavan]

On Fri, 2015-01-02 at 15:04 +0200, Tanu Kaskinen wrote:
> The new tunnel sink and source use libpulse to talk to the remote
> server, and libpulse requires a pa_mainloop_api implementation for
> interacting with the event loop. The tunnel sink and source have so
> far been using pa_mainloop, but there are some modules that assume
> that all sinks and sources use pa_rtpoll in their IO threads, and
> trying to use those modules together with the pa_mainloop based
> tunnel sinks and sources will result in crashes (see [1]).
> 
> I will switch the event loop implementation in the tunnel modules
> to pa_rtpoll, but that requires a pa_mainloop_api implementation in
> pa_rtpoll first - that's implemented here.
> 
> pa_rtpoll_run() is changed so that it processes all defer events
> first, and then all expired time events. The rest of pa_rtpoll_run()
> works as before, except the poll timeout calculation now has to also
> take the time events into account. IO events use the existing
> pa_rtpoll_item interface.
> 
> The time events are handled separately from the old timer
> functionality, which is somewhat ugly. It might be a good idea to
> remove the old timer functionality and only use the time events.
> I didn't attempt to do that at this time, because I feared that
> adapting the pa_rtpoll users to the new system would be difficult.
> 
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=73429
> ---

As a first comment, I'd like to ask that there be a test for this. It
should be quite easy to refactor mainloop-test to run the same set of
tests on different mainloop implementations.

>  src/pulsecore/rtpoll.c | 504
> -
>  src/pulsecore/rtpoll.h |   4 +
>  2 files changed, 500 insertions(+), 8 deletions(-)
> 
> diff --git a/src/pulsecore/rtpoll.c b/src/pulsecore/rtpoll.c
> index 5f3ca8b..2e1907c 100644
> --- a/src/pulsecore/rtpoll.c
> +++ b/src/pulsecore/rtpoll.c
> @@ -32,6 +32,7 @@
>  #include 
>  #include 
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -65,6 +66,18 @@ struct pa_rtpoll {
>  #endif
>  
>  PA_LLIST_HEAD(pa_rtpoll_item, items);
> +
> +pa_mainloop_api mainloop_api;
> +
> +pa_dynarray *io_events;
> +
> +pa_dynarray *time_events;
> +pa_dynarray *enabled_time_events;
> +pa_dynarray *expired_time_events;
> +pa_time_event *cached_next_time_event;
> +
> +pa_dynarray *defer_events;
> +pa_dynarray *enabled_defer_events;
>  };
>  
>  struct pa_rtpoll_item {
> @@ -84,8 +97,321 @@ struct pa_rtpoll_item {
>  PA_LLIST_FIELDS(pa_rtpoll_item);
>  };
>  
> +struct pa_io_event {
> +pa_rtpoll *rtpoll;
> +pa_rtpoll_item *rtpoll_item;
> +pa_io_event_flags_t events;
> +pa_io_event_cb_t callback;
> +pa_io_event_destroy_cb_t destroy_callback;
> +void *userdata;
> +};
> +
> +static void io_event_enable(pa_io_event *event, pa_io_event_flags_t
> events);
> +
> +struct pa_time_event {
> +pa_rtpoll *rtpoll;
> +pa_usec_t time;
> +bool use_rtclock;
> +bool enabled;
> +pa_time_event_cb_t callback;
> +pa_time_event_destroy_cb_t destroy_callback;
> +void *userdata;
> +};
> +
> +static void time_event_restart(pa_time_event *event, const struct
> timeval *tv);
> +
> +struct pa_defer_event {
> +pa_rtpoll *rtpoll;
> +bool enabled;
> +pa_defer_event_cb_t callback;
> +pa_defer_event_destroy_cb_t destroy_callback;
> +void *userdata;
> +};
> +
> +static void defer_event_enable(pa_defer_event *event, int enable);
> +
>  PA_STATIC_FLIST_DECLARE(items, 0, pa_xfree);
>  
> +static short map_flags_to_libc(pa_io_event_flags_t flags) {
> +return (short)
> +((flags & PA_IO_EVENT_INPUT ? POLLIN : 0) |
> + (flags & PA_IO_EVENT_OUTPUT ? POLLOUT : 0) |
> + (flags & PA_IO_EVENT_ERROR ? POLLERR : 0) |
> + (flags & PA_IO_EVENT_HANGUP ? POLLHUP : 0));
> +}
> +
> +static pa_io_event_flags_t map_flags_from_libc(short flags) {
> +return
> +(flags & POLLIN ? PA_IO_EVENT_INPUT : 0) |
> +(flags & POLLOUT ? PA_IO_EVENT_OUTPUT : 0) |
> +(flags & POLLERR ? PA_IO_EVENT_ERROR : 0) |
> +(flags & POLLHUP ? PA_IO_EVENT_HANGUP : 0);
> +}

Can we dump these two in a util.c and have this reused with mainloop.c?

> +static int io_event_work_cb(pa_rtpoll_item *item) {
> +pa_io_event *event;
> +struct pollfd *pollfd;
> +
> +pa_assert(item);
> +
> +event = pa_rtpoll_item_get_userdata(item);
> +pollfd = pa_rtpoll_item_get_pollfd(item, NULL);
> +event->callback(&event->rtpoll->mainloop_api, event, pollfd->fd, 
> map_flags_from_libc(pollfd->revents), event->userdata);
> +
> +return 0;
> +}
> +
> +static pa_io_event* io_event_new(pa_mainloop_api *api, int fd,
> pa_io_event_flags_t events, pa_io_event_cb_t callback,
> + void *userdata) {
> +pa_rtpoll *rtpoll;
> +pa_io_event *event;
> +struct pollfd *pollfd;
> +
> +pa_assert(api);
> +pa_assert(api->userdata);
> + 

Re: [pulseaudio-discuss] [PATCH 5/5] tunnel-new: Replace pa_mainloop with pa_rtpoll

[Arun Raghavan]

On Fri, 2015-01-02 at 15:04 +0200, Tanu Kaskinen wrote:
> This fixes crashes when trying to use module-rtp-recv or
> module-combine-sink together with module-tunnel-sink-new.
> module-rtp-recv and module-combine-sink assume that all IO threads
> use
> pa_rtpoll.
> 
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=73429
> ---

Looks good to me.

-- Arun

>  src/modules/module-tunnel-sink-new.c   | 35 --
> 
>  src/modules/module-tunnel-source-new.c | 35 --
> 
>  2 files changed, 32 insertions(+), 38 deletions(-)
> 
> diff --git a/src/modules/module-tunnel-sink-new.c
> b/src/modules/module-tunnel-sink-new.c
> index 7d3bd99..cbb721f 100644
> --- a/src/modules/module-tunnel-sink-new.c
> +++ b/src/modules/module-tunnel-sink-new.c
> @@ -74,7 +74,7 @@ struct userdata {
>  pa_sink *sink;
>  pa_thread *thread;
>  pa_thread_mq *thread_mq;
> -pa_mainloop *thread_mainloop;
> +pa_rtpoll *rtpoll;
>  pa_mainloop_api *thread_mainloop_api;
>  
>  pa_context *context;
> @@ -169,13 +169,6 @@ static void thread_func(void *userdata) {
>  for (;;) {
>  int ret;
>  
> -if (pa_mainloop_iterate(u->thread_mainloop, 1, &ret) < 0) {
> -if (ret == 0)
> -goto finish;
> -else
> -goto fail;
> -}
> -
>  if (PA_UNLIKELY(u->sink->thread_info.rewind_requested))
>  pa_sink_process_rewind(u->sink, 0);
>  
> @@ -212,6 +205,14 @@ static void thread_func(void *userdata) {
>  
>  }
>  }
> +
> +if ((ret = pa_rtpoll_run(u->rtpoll)) < 0)
> +goto fail;
> +
> +/* ret is zero only when the module is being unloaded, i.e.
> we're doing
> + * clean shutdown. */
> +if (ret == 0)
> +goto finish;
>  }
>  fail:
>  pa_asyncmsgq_post(u->thread_mq->outq, PA_MSGOBJECT(u->module-
> >core), PA_CORE_MESSAGE_UNLOAD_MODULE, u->module, 0, NULL, NULL);
> @@ -480,18 +481,13 @@ int pa__init(pa_module *m) {
>  u->module = m;
>  m->userdata = u;
>  u->remote_server = pa_xstrdup(remote_server);
> -u->thread_mainloop = pa_mainloop_new();
> -if (u->thread_mainloop == NULL) {
> -pa_log("Failed to create mainloop");
> -goto fail;
> -}
> -u->thread_mainloop_api = pa_mainloop_get_api(u-
> >thread_mainloop);
> +u->rtpoll = pa_rtpoll_new();
> +u->thread_mq = pa_xnew0(pa_thread_mq, 1);
> +pa_thread_mq_init(u->thread_mq, m->core->mainloop, u->rtpoll);
> +u->thread_mainloop_api = pa_rtpoll_get_mainloop_api(u->rtpoll);
>  u->cookie_file = pa_xstrdup(pa_modargs_get_value(ma, "cookie",
> NULL));
>  u->remote_sink_name = pa_xstrdup(pa_modargs_get_value(ma,
> "sink", NULL));
>  
> -u->thread_mq = pa_xnew0(pa_thread_mq, 1);
> -pa_thread_mq_init_thread_mainloop(u->thread_mq, m->core-
> >mainloop, u->thread_mainloop_api);
> -
>  /* Create sink */
>  pa_sink_new_data_init(&sink_data);
>  sink_data.driver = __FILE__;
> @@ -530,6 +526,7 @@ int pa__init(pa_module *m) {
>  
>  /* set thread message queue */
>  pa_sink_set_asyncmsgq(u->sink, u->thread_mq->inq);
> +pa_sink_set_rtpoll(u->sink, u->rtpoll);
>  
>  if (!(u->thread = pa_thread_new("tunnel-sink", thread_func, u)))
> {
>  pa_log("Failed to create thread.");
> @@ -575,8 +572,8 @@ void pa__done(pa_module *m) {
>  pa_xfree(u->thread_mq);
>  }
>  
> -if (u->thread_mainloop)
> -pa_mainloop_free(u->thread_mainloop);
> +if (u->rtpoll)
> +pa_rtpoll_free(u->rtpoll);
>  
>  if (u->cookie_file)
>  pa_xfree(u->cookie_file);
> diff --git a/src/modules/module-tunnel-source-new.c
> b/src/modules/module-tunnel-source-new.c
> index c6580eb..64c1066 100644
> --- a/src/modules/module-tunnel-source-new.c
> +++ b/src/modules/module-tunnel-source-new.c
> @@ -72,7 +72,7 @@ struct userdata {
>  pa_source *source;
>  pa_thread *thread;
>  pa_thread_mq *thread_mq;
> -pa_mainloop *thread_mainloop;
> +pa_rtpoll *rtpoll;
>  pa_mainloop_api *thread_mainloop_api;
>  
>  pa_context *context;
> @@ -227,15 +227,16 @@ static void thread_func(void *userdata) {
>  for (;;) {
>  int ret;
>  
> -if (pa_mainloop_iterate(u->thread_mainloop, 1, &ret) < 0) {
> -if (ret == 0)
> -goto finish;
> -else
> -goto fail;
> -}
> -
>  if (u->new_data)
>  read_new_samples(u);
> +
> +if ((ret = pa_rtpoll_run(u->rtpoll)) < 0)
> +goto fail;
> +
> +/* ret is zero only when the module is being unloaded, i.e.
> we're doing
> + * clean shutdown. */
> +if (ret == 0)
> +goto finish;
>  }
>  fail:
>  pa_asyncmsgq_post(u->thread_mq->outq, PA_MSGOBJECT(u->module-
> >core), PA_CORE_MESSAGE_UNLOAD_MODULE, u->module, 0, NULL, NULL);
> @@ -488,18 +489,13 @@ int pa__init(pa_module *

Re: [pulseaudio-discuss] [PATCH] default.pa: remove X11 module examples

[Arun Raghavan]

On 6 February 2016 at 19:05, Tanu Kaskinen  wrote:
> Loading X stuff from default.pa is a bad idea, since it doesn't work
> if there are multiple X sessions, or PulseAudio is started outside the
> X session. Since it's a bad idea, don't encourage it by including
> examples that do so.
>
> I also removed the sample loading examples. I don't think the examples
> are particularly useful, since nothing uses the loaded samples once
> module-x11-bell is removed from the configuration.
>
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=93109
> ---

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

Re: [pulseaudio-discuss] [PATCH v6 00/37] raop2 support for module-raop-sink

[Arun Raghavan]

On Sun, 2016-01-31 at 22:15 -0600, Hajime Fujita wrote:
> From: Hajime Fujita 
> 
> This is basically same as the previous patch series,
> with several patches squashed for conciseness, rebased
> on the latest master.
> http://lists.freedesktop.org/archives/pulseaudio-discuss/2015-October
> /024547.html
> 
> This patch set adds a support for UDP version of RAOP (so called
> raop2). Most of the RAOP devices (e.g. AppleTV, AirportExpress,
> third party AV receivers) today use UDP version, so this patch
> set is expected to support those devices.
> 
> Also the patches can be seen from here:
> https://github.com/hfujita/pulseaudio-raop2/compare/323dc5bf...hf/rao
> p2-v4-v8.0
> 

Thank you for your perseverance in getting this through. I'm going to
try to do a pass through the list in the coming days.

I'm using the raop2-v4-v8.0 branch. Please let me know if I should be
looking at something else.

Regards,
Arun
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v6 04/37] raop: Add pulsecore/core-utils a pa_str_in_list function

[Arun Raghavan]

On Sun, 2016-01-31 at 22:16 -0600, Hajime Fujita wrote:
> From: Martin Blanchard 
> 
> ---
>  src/pulsecore/core-util.c | 20 
>  src/pulsecore/core-util.h |  5 +++--
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/src/pulsecore/core-util.c b/src/pulsecore/core-util.c
> index 19c89a9..9fd4301 100644
> --- a/src/pulsecore/core-util.c
> +++ b/src/pulsecore/core-util.c
> @@ -2977,6 +2977,26 @@ bool pa_in_system_mode(void) {
>  return !!atoi(e);
>  }
>  
> +/* Checks a delimiters-separated list of words in haystack for needle */
> +bool pa_str_in_list(const char *haystack, const char *delimiters, const char 
> *needle) {
> +char *s;
> +const char *state = NULL;
> +
> +if (!haystack || !needle)
> +return false;
> +
> +while ((s = pa_split(haystack, delimiters, &state))) {

Would be nicer to use pa_split_in_place() here.

-- Arun

> +if (pa_streq(needle, s)) {
> +pa_xfree(s);
> +return true;
> +}
> +
> +pa_xfree(s);
> +}
> +
> +return false;
> +}
> +
>  /* Checks a whitespace-separated list of words in haystack for needle */
>  bool pa_str_in_list_spaces(const char *haystack, const char *needle) {
>  char *s;
> diff --git a/src/pulsecore/core-util.h b/src/pulsecore/core-util.h
> index d5a2d39..56b527f 100644
> --- a/src/pulsecore/core-util.h
> +++ b/src/pulsecore/core-util.h
> @@ -109,8 +109,8 @@ static inline const char *pa_strna(const char *x) {
>  return x ? x : "n/a";
>  }
>  
> -char *pa_split(const char *c, const char*delimiters, const char **state);
> -const char *pa_split_in_place(const char *c, const char*delimiters, int *n, 
> const char **state);
> +char *pa_split(const char *c, const char *delimiters, const char **state);
> +const char *pa_split_in_place(const char *c, const char *delimiters, int *n, 
> const char **state);
>  char *pa_split_spaces(const char *c, const char **state);
>  
>  char *pa_strip_nl(char *s);
> @@ -228,6 +228,7 @@ static inline bool pa_safe_streq(const char *a, const 
> char *b) {
>  }
>  
>  bool pa_str_in_list_spaces(const char *needle, const char *haystack);
> +bool pa_str_in_list(const char *haystack, const char *delimiters, const char 
> *needle);
>  
>  char *pa_get_host_name_malloc(void);
>  char *pa_get_user_name_malloc(void);
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v6 05/37] raop: Parse server capabilities on discovery

[Arun Raghavan]

On Sun, 2016-01-31 at 22:16 -0600, Hajime Fujita wrote:
> From: Martin Blanchard 
> 
> During the discovery phase, raop servers send theirs capabilities
> (supported encryption, audio codec...). These should be passed to the
> raop sink via module's arguments.
> ---
>  src/modules/raop/module-raop-discover.c | 100
> 
>  1 file changed, 90 insertions(+), 10 deletions(-)
> 
> diff --git a/src/modules/raop/module-raop-discover.c
> b/src/modules/raop/module-raop-discover.c
> index 1ced777..f174da3 100644
> --- a/src/modules/raop/module-raop-discover.c
> +++ b/src/modules/raop/module-raop-discover.c
> @@ -138,7 +138,10 @@ static void resolver_cb(
>  void *userdata) {
>  struct userdata *u = userdata;
>  struct tunnel *tnl;
> -char *device = NULL, *nicename, *dname, *vname, *args;
> +char *nicename, *dname, *vname, *args;
> +char *tp = NULL, *et = NULL, *cn = NULL;
> +char *ch = NULL, *ss = NULL, *sr = NULL;
> +char *t = NULL;
>  char at[AVAHI_ADDRESS_STR_MAX];
>  AvahiStringList *l;
>  pa_module *m;
> @@ -166,8 +169,54 @@ static void resolver_cb(
>  pa_assert_se(avahi_string_list_get_pair(l, &key, &value,
> NULL) == 0);
>  
>  pa_log_debug("Found key: '%s' with value: '%s'", key,
> value);
> -if (pa_streq(key, "device")) {
> -device = value;

Is there a reason the device is dropped here. Ideally this should be a
separate commit, but for now, if we have a reason we can put in the
commit message, that'd be fine too.

-- Arun

> +
> +if (pa_streq(key, "tp")) {
> +/* Transport protocol:
> + *  - TCP = only TCP,
> + *  - UDP = only UDP,
> + *  - TCP,UDP = both supported (UDP should be prefered)
> */
> + if (pa_str_in_list(value, ",", "UDP"))
> + tp = strdup("UDP");
> +else if (pa_str_in_list(value, ",", "TCP"))
> +tp = strdup("TCP");
> +else
> +tp = strdup(value);
> +} else if (pa_streq(key, "et")) {
> +/* Supported encryption types:
> + *  - 0 = none,
> + *  - 1 = RSA,
> + *  - 2 = FairPlay,
> + *  - 3 = MFiSAP,
> + *  - 4 = FairPlay SAPv2.5. */
> + if (pa_str_in_list(value, ",", "1"))
> + et = strdup("RSA");
> + else
> + et = strdup("none");
> +} else if (pa_streq(key, "cn")) {
> +/* Suported audio codecs:
> + *  - 0 = PCM,
> + *  - 1 = ALAC,
> + *  - 2 = AAC,
> + *  - 3 = AAC ELD. */
> +cn = strdup("PCM");
> +} else if (pa_streq(key, "md")) {
> +/* Supported metadata types:
> + *  - 0 = text,
> + *  - 1 = artwork,
> + *  - 2 = progress. */
> +} else if (pa_streq(key, "pw")) {
> +/* Requires password ? (true/false) */
> +} else if (pa_streq(key, "ch")) {
> +/* Number of channels */
> +ch = value;
> +value = NULL;
> +} else if (pa_streq(key, "ss")) {
> +/* Sample size */
> +ss = value;
> +value = NULL;
> +} else if (pa_streq(key, "sr")) {
> +/* Sample rate */
> +sr = value;
>  value = NULL;
>  }
>  
> @@ -175,19 +224,13 @@ static void resolver_cb(
>  avahi_free(value);
>  }
>  
> -if (device)
> -dname = pa_sprintf_malloc("raop.%s.%s", host_name, device);
> -else
> -dname = pa_sprintf_malloc("raop.%s", host_name);
> -
> +dname = pa_sprintf_malloc("raop_output.%s", host_name);
>  if (!(vname = pa_namereg_make_valid_name(dname))) {
>  pa_log("Cannot construct valid device name from '%s'.",
> dname);
> -avahi_free(device);
>  pa_xfree(dname);
>  goto finish;
>  }
>  
> -avahi_free(device);
>  pa_xfree(dname);
>  
>  if (nicename) {
> @@ -205,6 +248,43 @@ static void resolver_cb(
>   vname);
>  }
>  
> +if (tp != NULL) {
> +t = args;
> +args = pa_sprintf_malloc("%s protocol=%s", args, tp);
> +avahi_free(tp);
> +pa_xfree(t);
> +}
> +if (et != NULL) {
> +t = args;
> +args = pa_sprintf_malloc("%s encryption=%s", args, et);
> +pa_xfree(et);
> +pa_xfree(t);
> +}
> +if (cn != NULL) {
> +t = args;
> +args = pa_sprintf_malloc("%s codec=%s", args, cn);
> +pa_xfree(cn);
> +pa_xfree(t);
> +}
> +if (ch != NULL) {
> +t = args;
> +args = pa_sprintf_malloc("%s channels=%s", args, ch);
> +avahi_free(ch);
> +pa_xfree(t);
> +}
> +if (ss != NULL) {
> +t = args;
> +args = pa_sprintf_malloc("%s format=%s", args, ss);
> +avahi_free(ss);
> +pa_xfree(

Re: [pulseaudio-discuss] [PATCH v6 09/37] raop: Add UDP protocol handling

[Arun Raghavan]

On Sun, 2016-01-31 at 22:16 -0600, Hajime Fujita wrote:
> From: Hajime Fujita 
> 
> There are two versions in the RAOP protocol; one uses TCP and the
> other uses UDP. Current raop implementation only supports TCP
> version.
> 
> This patch adds an initial UDP protocol support for RAOP.
> It is based on Martin Blanchard's work
> (http://repo.or.cz/w/pulseaudio-raopUDP.git/shortlog/refs/heads/raop)
> which is inspired by Christophe Fergeau's work
> (https://github.com/zx2c4/pulseaudio-raop2).
> 
> Matrin's modifications were edited by Hajime Fujita, so that it
> would support both TCP and UDP protocol in a single module.
> 
> Also this patch includes a fix that was found thanks to Matthias,
> who reported that his ALAC
> codec support fixed the issue.
> https://bugs.freedesktop.org/show_bug.cgi?id=42804#c30
> ---

Ideally this patch should come before "raop: Parse server capabilities
on discovery" as it introduces modargs that the previous commit uses.
If there are conflicts on rebase though, I'm okay with not reordering.

>  src/modules/raop/module-raop-sink.c |  457 +--
>  src/modules/raop/raop_client.c  | 1063 
> +++
>  src/modules/raop/raop_client.h  |   39 +-
>  3 files changed, 1400 insertions(+), 159 deletions(-)
> 
> diff --git a/src/modules/raop/module-raop-sink.c 
> b/src/modules/raop/module-raop-sink.c
> index 6fc3d94..1eadde2 100644
> --- a/src/modules/raop/module-raop-sink.c
> +++ b/src/modules/raop/module-raop-sink.c
> @@ -66,12 +66,13 @@ PA_MODULE_USAGE(
>  "sink_name= "
>  "sink_properties= "
>  "server=  "
> +"protocol= "
> +"encryption= "
> +"codec= "
>  "format= "
>  "rate= "
>  "channels=");
>  
> -#define DEFAULT_SINK_NAME "raop"
> -
>  struct userdata {
>  pa_core *core;
>  pa_module *module;
> @@ -82,6 +83,8 @@ struct userdata {
>  pa_rtpoll_item *rtpoll_item;
>  pa_thread *thread;
>  
> +pa_raop_protocol_t protocol;
> +
>  pa_memchunk raw_memchunk;
>  pa_memchunk encoded_memchunk;
>  
> @@ -97,7 +100,6 @@ struct userdata {
>  int32_t rate;
>  
>  pa_smoother *smoother;
> -int fd;
>  
>  int64_t offset;
>  int64_t encoding_overhead;
> @@ -107,12 +109,26 @@ struct userdata {
>  pa_raop_client *raop;
>  
>  size_t block_size;
> +
> +/* Members only for the TCP protocol */
> +int tcp_fd;
> +
> +/* Members only for the UDP protocol */
> +int udp_control_fd;
> +int udp_timing_fd;
> +
> +/* For UDP thread wakeup clock calculation */
> +pa_usec_t udp_playback_start;
> +uint32_t  udp_sent_packets;
>  };
>  
>  static const char* const valid_modargs[] = {
>  "sink_name",
>  "sink_properties",
>  "server",
> +"protocol",
> +"encryption",
> +"codec",
>  "format",
>  "rate",
>  "channels",
> @@ -120,23 +136,26 @@ static const char* const valid_modargs[] = {
>  };
>  
>  enum {
> -SINK_MESSAGE_PASS_SOCKET = PA_SINK_MESSAGE_MAX,
> -SINK_MESSAGE_RIP_SOCKET
> +SINK_MESSAGE_TCP_PASS_SOCKET = PA_SINK_MESSAGE_MAX,
> +SINK_MESSAGE_TCP_RIP_SOCKET,
> +SINK_MESSAGE_UDP_SETUP,
> +SINK_MESSAGE_UDP_RECORD,
> +SINK_MESSAGE_UDP_DISCONNECTED,
>  };
>  
>  /* Forward declarations: */
>  static void sink_set_volume_cb(pa_sink *);
>  
> -static void on_connection(int fd, void *userdata) {
> +static void tcp_on_connection(int fd, void *userdata) {
>  int so_sndbuf = 0;
>  socklen_t sl = sizeof(int);
>  struct userdata *u = userdata;
>  pa_assert(u);
>  
> -pa_assert(u->fd < 0);
> -u->fd = fd;
> +pa_assert(u->tcp_fd < 0);
> +u->tcp_fd = fd;
>  
> -if (getsockopt(u->fd, SOL_SOCKET, SO_SNDBUF, &so_sndbuf, &sl) < 0)
> +if (getsockopt(u->tcp_fd, SOL_SOCKET, SO_SNDBUF, &so_sndbuf, &sl) < 0)
>  pa_log_warn("getsockopt(SO_SNDBUF) failed: %s", pa_cstrerror(errno));
>  else {
>  pa_log_debug("SO_SNDBUF is %zu.", (size_t) so_sndbuf);
> @@ -148,19 +167,28 @@ static void on_connection(int fd, void *userdata) {
>  
>  pa_log_debug("Connection authenticated, handing fd to IO thread...");
>  
> -pa_asyncmsgq_post(u->thread_mq.inq, PA_MSGOBJECT(u->sink), 
> SINK_MESSAGE_PASS_SOCKET, NULL, 0, NULL, NULL);
> +pa_asyncmsgq_post(u->thread_mq.inq, PA_MSGOBJECT(u->sink), 
> SINK_MESSAGE_TCP_PASS_SOCKET, NULL, 0, NULL, NULL);
>  }
>  
> -static void on_close(void*userdata) {
> +static void tcp_on_close(void*userdata) {
>  struct userdata *u = userdata;
>  pa_assert(u);
>  
>  pa_log_debug("Connection closed, informing IO thread...");
>  
> -pa_asyncmsgq_post(u->thread_mq.inq, PA_MSGOBJECT(u->sink), 
> SINK_MESSAGE_RIP_SOCKET, NULL, 0, NULL, NULL);
> +pa_asyncmsgq_post(u->thread_mq.inq, PA_MSGOBJECT(u->sink), 
> SINK_MESSAGE_TCP_RIP_SOCKET, NULL, 0, NULL, NULL);
> +}
> +
> +static pa_usec_t sink_get_latency(const struct userdata *u) {
> +pa_usec_t w, r;
> +
> +r = pa_smoot

Re: [pulseaudio-discuss] [PATCH v6 16/37] raop: Add the core implementation of RAOP authentication

[Arun Raghavan]

On Sun, 2016-01-31 at 22:16 -0600, Hajime Fujita wrote:
> From: Martin Blanchard 
> 
> RAOP authentication (password) is based on BA and DA HTTP authentication
> schemes. This patch adds the RSTP client the ability to specify the caller
> of server response status. Tracking the '401 Unauthorized' status allow
> the RAOP client to respond the server challenge authentication request.
> This patch adds the core implementation but does not introduce the
> authentication scheme in the RAOP connection process yet.
> ---
>  src/modules/raop/raop_client.c  |  245 +-
>  src/modules/raop/raop_client.c.orig | 1495 
> +++

Please remove this file from this commit rather than adding it and
removing it later.

-- Arun

>  src/modules/raop/raop_client.h  |4 +
>  src/modules/rtp/rtsp_client.c   |   83 +-
>  src/modules/rtp/rtsp_client.h   |   26 +-
>  5 files changed, 1810 insertions(+), 43 deletions(-)
>  create mode 100644 src/modules/raop/raop_client.c.orig
> 
> diff --git a/src/modules/raop/raop_client.c b/src/modules/raop/raop_client.c
> index a2ec91e..1323cd0 100644
> --- a/src/modules/raop/raop_client.c
> +++ b/src/modules/raop/raop_client.c
> @@ -65,6 +65,9 @@
>  #define VOLUME_MIN -144
>  #define VOLUME_MAX 0
>  
> +#define USER_AGENT "iTunes/11.0.4 (Windows; N)"
> +#define USER_NAME "iTunes"
> +
>  #define DEFAULT_RAOP_PORT 5000
>  #define UDP_DEFAULT_AUDIO_PORT 6000
>  #define UDP_DEFAULT_CONTROL_PORT 6001
> @@ -85,13 +88,15 @@ struct pa_raop_client {
>  pa_core *core;
>  char *host;
>  uint16_t port;
> -char *sid;
>  pa_rtsp_client *rtsp;
> -pa_raop_protocol_t protocol;
> +char *sci, *sid;
> +char *pwd;
>  
>  uint8_t jack_type;
>  uint8_t jack_status;
>  
> +pa_raop_protocol_t protocol;
> +
>  int encryption; /* Enable encryption? */
>  pa_raop_secret *aes;
>  
> @@ -125,6 +130,9 @@ struct pa_raop_client {
>  uint32_t udp_sync_interval;
>  uint32_t udp_sync_count;
>  
> +pa_raop_client_auth_cb_t udp_auth_callback;
> +void *udp_auth_userdata;
> +
>  pa_raop_client_setup_cb_t udp_setup_callback;
>  void *udp_setup_userdata;
>  
> @@ -576,7 +584,7 @@ static void do_rtsp_announce(pa_raop_client *c) {
>  pa_xfree(sdp);
>  }
>  
> -static void tcp_rtsp_cb(pa_rtsp_client *rtsp, pa_rtsp_state state, 
> pa_headerlist* headers, void *userdata) {
> +static void tcp_rtsp_cb(pa_rtsp_client *rtsp, pa_rtsp_state state, 
> pa_rtsp_status status, pa_headerlist* headers, void *userdata) {
>  pa_raop_client* c = userdata;
>  pa_assert(c);
>  pa_assert(rtsp);
> @@ -675,12 +683,13 @@ static void tcp_rtsp_cb(pa_rtsp_client *rtsp, 
> pa_rtsp_state state, pa_headerlist
>  }
>  }
>  
> -static void udp_rtsp_cb(pa_rtsp_client *rtsp, pa_rtsp_state state, 
> pa_headerlist *headers, void *userdata) {
> +static void udp_rtsp_cb(pa_rtsp_client *rtsp, pa_rtsp_state state, 
> pa_rtsp_status status, pa_headerlist *headers, void *userdata) {
>  pa_raop_client *c = userdata;
>  
>  pa_assert(c);
>  pa_assert(rtsp);
>  pa_assert(rtsp == c->rtsp);
> +pa_assert(STATUS_OK == status);
>  
>  switch (state) {
>  case STATE_CONNECT: {
> @@ -982,6 +991,178 @@ static void udp_rtsp_cb(pa_rtsp_client *rtsp, 
> pa_rtsp_state state, pa_headerlist
>  }
>  }
>  
> +static void rtsp_authentication_cb(pa_rtsp_client *rtsp, pa_rtsp_state 
> state, pa_rtsp_status status, pa_headerlist *headers, void *userdata) {
> +pa_raop_client *c = userdata;
> +
> +pa_assert(c);
> +pa_assert(rtsp);
> +pa_assert(rtsp == c->rtsp);
> +
> +switch (state) {
> +case STATE_CONNECT: {
> +char *sci = NULL, *sac = NULL;
> +uint16_t rac;
> +struct {
> +uint32_t ci1;
> +uint32_t ci2;
> +} rci;
> +
> +pa_random(&rci, sizeof(rci));
> +/* Generate a random Client-Instance number */
> +sci = pa_sprintf_malloc("%08x%08x",rci.ci1, rci.ci2);
> +pa_rtsp_add_header(c->rtsp, "Client-Instance", sci);
> +
> +pa_random(&rac, sizeof(rac));
> +/* Generate a random Apple-Challenge key */
> +pa_raop_base64_encode(&rac, 8*sizeof(rac), &sac);
> +pa_log_debug ("APPLECHALLENGE >> %s | %d", sac, (int) 
> sizeof(rac));
> +rtrimchar(sac, '=');
> +pa_rtsp_add_header(c->rtsp, "Apple-Challenge", sac);
> +
> +pa_rtsp_options(c->rtsp);
> +
> +pa_xfree(sac);
> +pa_xfree(sci);
> +break;
> +}
> +
> +case STATE_OPTIONS: {
> +static bool waiting = false;
> +const char *current = NULL;
> +char space[] = " ";
> +char *token,*ath = NULL;
> +char *publ, *wath, *mth, *val;
> +char *realm = NULL, *nonce = NULL, *response = NULL;
> +char comma[] = ",";
> +
> +pa_l

Re: [pulseaudio-discuss] [PATCH v6 04/37] raop: Add pulsecore/core-utils a pa_str_in_list function

[Arun Raghavan]

On 10-Feb-2016 8:59 AM, "Hajime Fujita"  wrote:
>
> Arun Raghavan wrote:
> > On Sun, 2016-01-31 at 22:16 -0600, Hajime Fujita wrote:
> >> From: Martin Blanchard 
> >>
> >> ---
> >>  src/pulsecore/core-util.c | 20 
> >>  src/pulsecore/core-util.h |  5 +++--
> >>  2 files changed, 23 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/pulsecore/core-util.c b/src/pulsecore/core-util.c
> >> index 19c89a9..9fd4301 100644
> >> --- a/src/pulsecore/core-util.c
> >> +++ b/src/pulsecore/core-util.c
> >> @@ -2977,6 +2977,26 @@ bool pa_in_system_mode(void) {
> >>  return !!atoi(e);
> >>  }
> >>
> >> +/* Checks a delimiters-separated list of words in haystack for needle
*/
> >> +bool pa_str_in_list(const char *haystack, const char *delimiters,
const char *needle) {
> >> +char *s;
> >> +const char *state = NULL;
> >> +
> >> +if (!haystack || !needle)
> >> +return false;
> >> +
> >> +while ((s = pa_split(haystack, delimiters, &state))) {
> >
> > Would be nicer to use pa_split_in_place() here.
>
> Sure. Actually there is another function pa_str_in_list_spaces() right
after this, which has almost the same structure including use of pa_split.
I think pa_str_in_list() was written based on pa_str_in_list_spaces().
>
> Do you also want to modify pa_str_in_list_spaces() so it uses
pa_split_in_places()? Perhaps in a separate commit.
>

Yep, that would be great, and should be in a different commit.

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

Re: [pulseaudio-discuss] [PATCH v3 18/24] echo-cancel: Improve webrtc canceller error handling a bit

[Arun Raghavan]

On 9 February 2016 at 23:41, Tanu Kaskinen  wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
>
>> From: Arun Raghavan 
>>
>> ---
>>  src/modules/echo-cancel/webrtc.cc | 14 ++
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/modules/echo-cancel/webrtc.cc 
>> b/src/modules/echo-cancel/webrtc.cc
>> index 2732b38..5741f45 100644
>> --- a/src/modules/echo-cancel/webrtc.cc
>> +++ b/src/modules/echo-cancel/webrtc.cc
>> @@ -284,7 +284,10 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller 
>> *ec,
>>  webrtc::StreamConfig(play_ss->rate, play_ss->channels, false), /* 
>> reverse input stream */
>>  webrtc::StreamConfig(play_ss->rate, play_ss->channels, false), /* 
>> reverse output stream */
>>  };
>> -apm->Initialize(pconfig);
>> +if (apm->Initialize(pconfig) != webrtc::AudioProcessing::kNoError) {
>> +pa_log("Error initialising audio processing module");
>> +goto fail;
>> +}
>>
>>  if (hpf)
>>  apm->high_pass_filter()->Enable(true);
>> @@ -313,7 +316,8 @@ bool pa_webrtc_ec_init(pa_core *c, pa_echo_canceller *ec,
>>  ec->params.webrtc.agc = false;
>>  } else {
>>  
>> apm->gain_control()->set_mode(webrtc::GainControl::kAdaptiveAnalog);
>> -if (apm->gain_control()->set_analog_level_limits(0, 
>> WEBRTC_AGC_MAX_VOLUME) != apm->kNoError) {
>> +if (apm->gain_control()->set_analog_level_limits(0, 
>> WEBRTC_AGC_MAX_VOLUME) !=
>> +webrtc::AudioProcessing::kNoError) {
>>  pa_log("Failed to initialise AGC");
>>  goto fail;
>>  }
>> @@ -361,7 +365,8 @@ void pa_webrtc_ec_play(pa_echo_canceller *ec, const 
>> uint8_t *play) {
>>  pa_assert(play_frame.samples_per_channel_ <= 
>> webrtc::AudioFrame::kMaxDataSizeSamples);
>>  memcpy(play_frame.data_, play, ec->params.webrtc.blocksize * 
>> pa_frame_size(ss));
>>
>> -apm->ProcessReverseStream(&play_frame);
>> +if (apm->ProcessReverseStream(&play_frame) != 
>> webrtc::AudioProcessing::kNoError)
>> +pa_log("Failed to process playback stream");
>>  }
>>
>>  void pa_webrtc_ec_record(pa_echo_canceller *ec, const uint8_t *rec, uint8_t 
>> *out) {
>> @@ -387,7 +392,8 @@ void pa_webrtc_ec_record(pa_echo_canceller *ec, const 
>> uint8_t *rec, uint8_t *out
>>  }
>>
>>  apm->set_stream_delay_ms(0);
>> -apm->ProcessStream(&out_frame);
>> +if (apm->ProcessStream(&out_frame) != webrtc::AudioProcessing::kNoError)
>> +pa_log("Failed to process capture stream");
>
> I don't like error messages that can spam the syslog. Would a hard
> failure be acceptable here (that is, unload the module, or change to a
> failure state where the module stays around but does no processing)?

This particular case is extremely low probability, so I'd like to not
change it if possible. I don't think the extra code to add a noop mode
is justified since we're very unlikely to hit this.

Maybe we could/should change the run/play/record vmethods to return an
error code, but I'd prefer to do that separately if it's done.

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

Re: [pulseaudio-discuss] [PATCH v6 00/37] raop2 support for module-raop-sink

[Arun Raghavan]

Hi Hajime,
I'm still working on this, it's slow going since I have other things to
do in parallel.

It's a bit hard to review this work properly per-commit, so I'm going
to try to summarise the issues I've found at one go. 

* There are some style issues that I'm fixing up as I find, and I'll
just have that as a commit on top of your tree.

* While testing, I see there are two instances loaded every time (one
for IPv4 and one for IPv6) -- wouldn't it be more user friendly to make
sure we load only one? This doesn't appear to be a new issue, so not a
blocker on this series

* I tried this against Airserver on a Mac (worked) and a current
generation Apple TV (did not work) -- just got stuck trying to start
playback

Will reply again once I'm through the patch set.

Regards,
Arun

On Wed, 2016-02-10 at 18:50 -0600, Hajime Fujita wrote:
> Hi Arun,
> 
> Are you still going through the patches?
> If you are done, I'll submit a new series of patches that address
> your comments.
> 
> 
> Thanks,
> Hajime
> 
> Hajime Fujita wrote:
> > Arun Raghavan wrote:
> > > On Sun, 2016-01-31 at 22:15 -0600, Hajime Fujita wrote:
> > > > From: Hajime Fujita 
> > > > 
> > > > This is basically same as the previous patch series,
> > > > with several patches squashed for conciseness, rebased
> > > > on the latest master.
> > > > http://lists.freedesktop.org/archives/pulseaudio-discuss/2015-O
> > > > ctober
> > > > /024547.html
> > > > 
> > > > This patch set adds a support for UDP version of RAOP (so
> > > > called
> > > > raop2). Most of the RAOP devices (e.g. AppleTV, AirportExpress,
> > > > third party AV receivers) today use UDP version, so this patch
> > > > set is expected to support those devices.
> > > > 
> > > > Also the patches can be seen from here:
> > > > https://github.com/hfujita/pulseaudio-raop2/compare/323dc5bf...
> > > > hf/rao
> > > > p2-v4-v8.0
> > > > 
> > > 
> > > Thank you for your perseverance in getting this through. I'm
> > > going to
> > > try to do a pass through the list in the coming days.
> > > 
> > > I'm using the raop2-v4-v8.0 branch. Please let me know if I
> > > should be
> > > looking at something else.
> > 
> > Hi Arun,
> > 
> > Thank you for taking a look at this.
> > Yes, that is the right one. I'll start addressing your comments.
> > 
> > 
> > Best,
> > Hajime
> > 
> > > 
> > > Regards,
> > > Arun
> > > ___
> > > pulseaudio-discuss mailing list
> > > pulseaudio-discuss@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
> > > 
> > 
> > ___
> > pulseaudio-discuss mailing list
> > pulseaudio-discuss@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
> > 
> 
> ___
> pulseaudio-discuss mailing list
> pulseaudio-discuss@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH 3/3] coreaudio: Catch devices with more channels than we support

[Arun Raghavan]

On 11 February 2016 at 06:52, Mihai Moldovan  wrote:
> Regarding this, I submitted another patch a year ago that doesn't skip devices
> with more channels than supported, but only exposes as many channels as
> supported by PulseAudio.
>
> I guess it would be beneficial to decide how to proceed on that front: either
> drop devices with an unsupported number of channels completely, or restrict 
> the
> channels so that PA can still handle the device (and leave the other channels
> "empty")?
>
> My original idea of still using the device might not make most sense or be
> ideal, but I felt that completely dropping it would be weird as well. What's
> your take?

I think dropping them makes sense. Either the device works as intended
(all channels) or it doesn't. Sounds weird to me that someone would be
okay with some but not all channels working.

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

Re: [pulseaudio-discuss] [PATCH] default.pa.in: do not load module-console-kit and module-systemd-login if HAVE_COREAUDIO. It fails.

[Arun Raghavan]

On 11 February 2016 at 07:11, Mihai Moldovan  wrote:
> Just a resubmit to let Patchwork catch it.

These are now hidden behind .ifexists, so it should make your patch redundant.

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

Re: [pulseaudio-discuss] [PATCH v6 00/37] raop2 support for module-raop-sink

[Arun Raghavan]

On 12 February 2016 at 12:03, Hajime Fujita  wrote:
> Hi Arun,
>
> Arun Raghavan wrote:
>> Hi Hajime,
>> I'm still working on this, it's slow going since I have other things to
>> do in parallel.
>
> Thank you for your reply. Yeah that's totally fine. Just wanted make sure if 
> it was appropriate to submit a new series.
>
>> It's a bit hard to review this work properly per-commit, so I'm going
>> to try to summarise the issues I've found at one go.
>>
>> * There are some style issues that I'm fixing up as I find, and I'll
>> just have that as a commit on top of your tree.
>>
>> * While testing, I see there are two instances loaded every time (one
>> for IPv4 and one for IPv6) -- wouldn't it be more user friendly to make
>> sure we load only one? This doesn't appear to be a new issue, so not a
>> blocker on this series
>
> I've been aware of this for a while. I think my thought was that some user 
> might want to explicitly choose IPv4 or v6 address. I'm not sure if it is a 
> real need though.
>
>>
>> * I tried this against Airserver on a Mac (worked) and a current
>> generation Apple TV (did not work) -- just got stuck trying to start
>> playback
>
> Hmm you mean the latest Apple TV which came out last year? I currently don't 
> have that one so it'll be a bit hard to debug... let me check the price :)

That's the one. I might be able to try again and get you a packet
capture if that helps.

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

Re: [pulseaudio-discuss] [PATCH v3 18/24] echo-cancel: Improve webrtc canceller error handling a bit

[Arun Raghavan]

On Thu, 2016-02-11 at 12:17 +0200, Tanu Kaskinen wrote:
> On Wed, 2016-02-10 at 09:56 +0530, Arun Raghavan wrote:
> > On 9 February 2016 at 23:41, Tanu Kaskinen  wrote:
> > > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > > > @@ -387,7 +392,8 @@ void pa_webrtc_ec_record(pa_echo_canceller
> > > > *ec, const uint8_t *rec, uint8_t *out
> > > >  }
> > > > 
> > > >  apm->set_stream_delay_ms(0);
> > > > -apm->ProcessStream(&out_frame);
> > > > +if (apm->ProcessStream(&out_frame) !=
> > > > webrtc::AudioProcessing::kNoError)
> > > > +pa_log("Failed to process capture stream");
> > > 
> > > I don't like error messages that can spam the syslog. Would a
> > > hard
> > > failure be acceptable here (that is, unload the module, or change
> > > to a
> > > failure state where the module stays around but does no
> > > processing)?
> > 
> > This particular case is extremely low probability, so I'd like to
> > not
> > change it if possible. I don't think the extra code to add a noop
> > mode
> > is justified since we're very unlikely to hit this.
> > 
> > Maybe we could/should change the run/play/record vmethods to return
> > an
> > error code, but I'd prefer to do that separately if it's done.
> 
> So what are the extremely rare conditions where this error is
> triggered? I'll assume that this error doesn't indicate a bug in our
> code or in the webrtc library, and can really happen, so an assertion
> is not appropriate here.

The only failure case that I see currently is where we send a different
block size fro mwhat we promised.

> Adding a noop mode was just one idea, and I understand if that feels
> overkill. But what about unloading the module? Other ideas: lower the
> message level to info or debug, or prevent the error message being
> printed more than once.
> 
> Flooding syslog is never a good error handling strategy. This looks
> like if there is one error, there will probably be thousand.

We shouldn't spam the log, since ratelimit will stop that from
happening. Does that not suffice? If not, I'll drop it to debug level.

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

Re: [pulseaudio-discuss] [PATCH v3 18/24] echo-cancel: Improve webrtc canceller error handling a bit

[Arun Raghavan]

On Wed, 2016-02-17 at 13:12 +0200, Tanu Kaskinen wrote:
> On Wed, 2016-02-17 at 16:23 +0530, Arun Raghavan wrote:
> > On Thu, 2016-02-11 at 12:17 +0200, Tanu Kaskinen wrote:
> > > On Wed, 2016-02-10 at 09:56 +0530, Arun Raghavan wrote:
> > > > This particular case is extremely low probability, so I'd like
> > > > to
> > > > not
> > > > change it if possible. I don't think the extra code to add a
> > > > noop
> > > > mode
> > > > is justified since we're very unlikely to hit this.
> > > > 
> > > > Maybe we could/should change the run/play/record vmethods to
> > > > return
> > > > an
> > > > error code, but I'd prefer to do that separately if it's done.
> > > 
> > > So what are the extremely rare conditions where this error is
> > > triggered? I'll assume that this error doesn't indicate a bug in
> > > our
> > > code or in the webrtc library, and can really happen, so an
> > > assertion
> > > is not appropriate here.
> > 
> > The only failure case that I see currently is where we send a
> > different
> > block size fro mwhat we promised.
> 
> That should never happen, right? So an assertion is appropriate after
> all.

I picked the log message over the assert since it's based on external
code, so we don't want to crash if the webrtc code changes. That said,
it's enough of corner case for this to not be a big deal, so I'll just
make it an assert so we can move on.

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

Re: [pulseaudio-discuss] [PATCH v3 22/24] echo-cancel: Use webrtc's deinterleaved API

[Arun Raghavan]

On Tue, 2016-02-16 at 14:20 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > This is required to have unequal channel counts on capture in and out
> > streams, which is needed for beamforming to work.
> 
> The commit message should mention why the sample format was changed to
> float.

Okay.

> >  void pa_webrtc_ec_play(pa_echo_canceller *ec, const uint8_t *play) {
> >  webrtc::AudioProcessing *apm = 
> > (webrtc::AudioProcessing*)ec->params.webrtc.apm;
> > -webrtc::AudioFrame play_frame;
> >  const pa_sample_spec *ss = &ec->params.webrtc.play_ss;
> > +int n = ec->params.webrtc.blocksize;
> > +float **buf = ec->params.webrtc.play_buffer;
> > +webrtc::StreamConfig config(ss->rate, ss->channels, false);
> >  
> > -play_frame.num_channels_ = ss->channels;
> > -play_frame.sample_rate_hz_ = ss->rate;
> > -play_frame.interleaved_ = true;
> > -play_frame.samples_per_channel_ = ec->params.webrtc.blocksize;
> > -
> > -pa_assert(play_frame.samples_per_channel_ <= 
> > webrtc::AudioFrame::kMaxDataSizeSamples);
> > -memcpy(play_frame.data_, play, ec->params.webrtc.blocksize * 
> > pa_frame_size(ss));
> > +pa_deinterleave(play, (void **) buf, ss->channels, pa_sample_size(ss), 
> > n);
> >  
> > -if (apm->ProcessReverseStream(&play_frame) != 
> > webrtc::AudioProcessing::kNoError)
> > +if (apm->ProcessReverseStream(buf, config, config, buf) != 
> > webrtc::AudioProcessing::kNoError)
> >  pa_log("Failed to process playback stream");
> > +
> > +/* FIXME: we need to be able to modify playback samples */
> 
> This comment should say also why we need to be able to modify them (and
> I also don't understand what's preventing us from doing so now).

Reworded this as:

/* FIXME: we need to be able to modify playback samples, which we can't
 * currently do. This is because module-echo-cancel processes playback
 * frames in the source thread, and just stores playback chunks as they
 * pass through the sink. */

> > @@ -573,4 +575,9 @@ void pa_webrtc_ec_done(pa_echo_canceller *ec) {
> >  delete (webrtc::AudioProcessing*)ec->params.webrtc.apm;
> >  ec->params.webrtc.apm = NULL;
> >  }
> > +
> > +for (i = 0; i < ec->params.webrtc.rec_ss.channels; i++)
> > +pa_xfree(ec->params.webrtc.rec_buffer[i]);
> > +for (i = 0; i < ec->params.webrtc.play_ss.channels; i++)
> > +pa_xfree(ec->params.webrtc.play_buffer[i]);
> 
> I think it's not guaranteed that rec_ss and play_ss are initialized at
> this point. You should check that rec_buffer and play_buffer are non-
> NULL before accessing them.

There should not be a path where rec_buffer and play_buffer are not
initialised before done, but I'll add that check in case this becomes
possible in the future.

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

Re: [pulseaudio-discuss] [PATCH v3 22/24] echo-cancel: Use webrtc's deinterleaved API

[Arun Raghavan]

On Wed, 2016-02-17 at 18:36 +0530, Arun Raghavan wrote:
> On Tue, 2016-02-16 at 14:20 +0200, Tanu Kaskinen wrote:
> > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > > This is required to have unequal channel counts on capture in and
> > > out
> > > streams, which is needed for beamforming to work.
> > 
> > The commit message should mention why the sample format was changed
> > to
> > float.
> 
> Okay.
> 
> > >  void pa_webrtc_ec_play(pa_echo_canceller *ec, const uint8_t
> > > *play) {
> > >  webrtc::AudioProcessing *apm = (webrtc::AudioProcessing*)ec-
> > > >params.webrtc.apm;
> > > -webrtc::AudioFrame play_frame;
> > >  const pa_sample_spec *ss = &ec->params.webrtc.play_ss;
> > > +int n = ec->params.webrtc.blocksize;
> > > +float **buf = ec->params.webrtc.play_buffer;
> > > +webrtc::StreamConfig config(ss->rate, ss->channels, false);
> > >  
> > > -play_frame.num_channels_ = ss->channels;
> > > -play_frame.sample_rate_hz_ = ss->rate;
> > > -play_frame.interleaved_ = true;
> > > -play_frame.samples_per_channel_ = ec-
> > > >params.webrtc.blocksize;
> > > -
> > > -pa_assert(play_frame.samples_per_channel_ <=
> > > webrtc::AudioFrame::kMaxDataSizeSamples);
> > > -memcpy(play_frame.data_, play, ec->params.webrtc.blocksize *
> > > pa_frame_size(ss));
> > > +pa_deinterleave(play, (void **) buf, ss->channels,
> > > pa_sample_size(ss), n);
> > >  
> > > -if (apm->ProcessReverseStream(&play_frame) !=
> > > webrtc::AudioProcessing::kNoError)
> > > +if (apm->ProcessReverseStream(buf, config, config, buf) !=
> > > webrtc::AudioProcessing::kNoError)
> > >  pa_log("Failed to process playback stream");
> > > +
> > > +/* FIXME: we need to be able to modify playback samples */
> > 
> > This comment should say also why we need to be able to modify them
> > (and
> > I also don't understand what's preventing us from doing so now).
> 
> Reworded this as:
> 
> /* FIXME: we need to be able to modify playback samples, which we
> can't
>  * currently do. This is because module-echo-cancel processes
> playback
>  * frames in the source thread, and just stores playback chunks as
> they
>  * pass through the sink. */
> 
> > > @@ -573,4 +575,9 @@ void pa_webrtc_ec_done(pa_echo_canceller *ec)
> > > {
> > >  delete (webrtc::AudioProcessing*)ec->params.webrtc.apm;
> > >  ec->params.webrtc.apm = NULL;
> > >  }
> > > +
> > > +for (i = 0; i < ec->params.webrtc.rec_ss.channels; i++)
> > > +pa_xfree(ec->params.webrtc.rec_buffer[i]);
> > > +for (i = 0; i < ec->params.webrtc.play_ss.channels; i++)
> > > +pa_xfree(ec->params.webrtc.play_buffer[i]);
> > 
> > I think it's not guaranteed that rec_ss and play_ss are initialized
> > at
> > this point. You should check that rec_buffer and play_buffer are
> > non-
> > NULL before accessing them.
> 
> There should not be a path where rec_buffer and play_buffer are not
> initialised before done, but I'll add that check in case this becomes
> possible in the future.

Shouldn't be necessary, actually. pa_xfree() is NULL-safe.

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

Re: [pulseaudio-discuss] [PATCH v3 20/24] echo-cancel: Add beamforming support in the webrtc canceller

[Arun Raghavan]

On Mon, 2016-02-15 at 13:12 +0200, Tanu Kaskinen wrote:
> On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > From: Arun Raghavan 
> > 
> > ---
> >  src/modules/echo-cancel/webrtc.cc | 138 
> > +-
> >  1 file changed, 137 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/modules/echo-cancel/webrtc.cc 
> > b/src/modules/echo-cancel/webrtc.cc
> > index 5f00286..9ae5d77 100644
> > --- a/src/modules/echo-cancel/webrtc.cc
> > +++ b/src/modules/echo-cancel/webrtc.cc
> > @@ -52,6 +52,7 @@ PA_C_DECL_END
> >  #define DEFAULT_INTELLIGIBILITY_ENHANCER false
> >  #define DEFAULT_EXPERIMENTAL_AGC false
> >  #define DEFAULT_AGC_START_VOLUME 85
> > +#define DEFAULT_BEAMFORMING false
> >  #define DEFAULT_TRACE false
> >  
> >  #define WEBRTC_AGC_MAX_VOLUME 255
> > @@ -70,6 +71,9 @@ static const char* const valid_modargs[] = {
> >  "intelligibility_enhancer",
> >  "experimental_agc",
> >  "agc_start_volume",
> > +"beamforming",
> > +"mic_geometry", /* documented in parse_mic_geometry() */
> > +"target_direction", /* documented in parse_mic_geometry() */
> >  "trace",
> >  NULL
> >  };
> > @@ -138,6 +142,76 @@ static void pa_webrtc_ec_fixate_spec(pa_sample_spec 
> > *rec_ss, pa_channel_map *rec
> >  play_ss->rate = rec_ss->rate;
> >  }
> >  
> > +static bool parse_point(const char **point, float (&f)[3]) {
> > +int ret, length;
> > +
> > +ret = sscanf(*point, "%g,%g,%g%n", &f[0], &f[1], &f[2], &length);
> > +if (ret != 3)
> > +return false;
> > +
> > +/* Consume the bytes we've read so far */
> > +*point += length;
> > +
> > +/* Consume trailing comma if there is one */
> > +if (**point == ',')
> > +(*point)++;
> 
> I don't think this should be done in this function. When expecting only
> one point, we should fail on "1,2,3," but this code accepts that
> string.

Okay.

> > +
> > +return true;
> > +}
> > +
> > +static bool parse_mic_geometry(const char **mic_geometry, std::vector& 
> > geometry) {
> > +/* The microphone geometry is expressed as cartesian point form:
> > + *   x1,y1,z1,x2,y2,z2,...
> > + *
> > + * Where x1,y1,z1 is the position of the first microphone with regards 
> > to
> > + * the array's "center", x2,y2,z2 the position of the second, and so 
> > on.
> > + *
> > + * 'x' is the horizontal coordinate, with positive values being to the
> > + * right from the mic array's perspective.
> > + *
> > + * 'y' is the depth coordinate, with positive values being in front of 
> > the
> > + * array.
> > + *
> > + * 'z' is the vertical coordinate, with positive values being above the
> > + * array.
> > + *
> > + * All distances are in meters.
> > + */
> 
> It's a bit unclear what "the mic array's perspective" means. Should I
> imagine the array as a person standing upright, looking straight ahead?
> Then positive x is to the right from the "person's" point of view,
> positive y is in front of the array guy and positive z is above the
> array.

Yes.

> > +
> > +/* The target direction is expected to be in spherical point form:
> > + *   a,e,r
> > + *
> > + * Where 'a is the azimuth of the first mic channel, 'e' its elevation,
> > + * and 'r' the radius.
> > + *
> > + * 0 radians azimuth is to the right of the array, and positive angles
> > + * move in a counter-clockwise direction.
> > + *
> > + * 0 radians elevation is horizontal w.r.t. the array, and positive
> > + * angles go upwards.
> > + *
> > + * radius is distance from the array center in meters.
> > + */
> > +
> > +int i;
> > +float f[3];
> > +
> > +for (i = 0; i < geometry.size(); i++) {
> > +if (!parse_point(mic_geometry, f)) {
> > +pa_log("Failed to parse channel %d in mic_geometry", i);
> > +return false;
> > +}
> > +
> > +pa_log_debug("Got mic #%d position: (%g, %g, %g)", i, f[0], f[1], 
> > f[2]);
> > +
> > +geometry[i].c[

Re: [pulseaudio-discuss] [PATCH v3 22/24] echo-cancel: Use webrtc's deinterleaved API

[Arun Raghavan]

On Wed, 2016-02-17 at 15:54 +0200, Tanu Kaskinen wrote:
> On Wed, 2016-02-17 at 18:40 +0530, Arun Raghavan wrote:
> > On Wed, 2016-02-17 at 18:36 +0530, Arun Raghavan wrote:
> > > On Tue, 2016-02-16 at 14:20 +0200, Tanu Kaskinen wrote:
> > > > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > > > > +/* FIXME: we need to be able to modify playback samples */
> > > > 
> > > > This comment should say also why we need to be able to modify them
> > > > (and
> > > > I also don't understand what's preventing us from doing so now).
> > > 
> > > Reworded this as:
> > > 
> > > /* FIXME: we need to be able to modify playback samples, which we can't
> > >  * currently do. This is because module-echo-cancel processes playback
> > >  * frames in the source thread, and just stores playback chunks as they
> > >  * pass through the sink. */
> 
> Mmmh, that still doesn't answer the question why we need to be able to
> modify playback samples. What processing do we want to do that we can't
> currently do?

ProcessReverseStream() *may* change samples. Currently, this only
happens when you enable the intelligibility enhancer.

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

Re: [pulseaudio-discuss] [PATCH v3 20/24] echo-cancel: Add beamforming support in the webrtc canceller

[Arun Raghavan]

On Wed, 2016-02-17 at 15:57 +0200, Tanu Kaskinen wrote:
> On Wed, 2016-02-17 at 18:47 +0530, Arun Raghavan wrote:
> > On Mon, 2016-02-15 at 13:12 +0200, Tanu Kaskinen wrote:
> > > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > > > +pa_log("The beamformer currently only supports
> > > > targeting along the azimuth");
> > > > +goto fail;
> > > > +}
> > > > +
> > > > +direction.s[0] = f[0];
> > > > +direction.s[1] = f[1];
> > > > +direction.s[2] = f[2];
> > > > +}
> > > > +
> > > > +/* The beamformer gives us a single channel */
> > > > +out_ss->channels = 1;
> > > > +pa_channel_map_init_mono(out_map);
> > > 
> > > Could this be moved to fixate_spec()? I don't like the audio
> > > format
> > > parameters being calculated all over the place.
> > 
> > I wanted to do that, but we can't because we need the capture
> > channel
> > count for parsing the mic geometry (there is a comment explaining
> > that
> > where if (beamforming) { ... } is).
> 
> I meant moving only the out_ss and out_map assignments. Those don't
> depend on the capture channel count. fixate_spec() will then need to
> take the beamforming bool as a parameter, but I think that's fine.

Ah, sure, that's doable.

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

Re: [pulseaudio-discuss] [PATCH v3 01/24] echo-cancel: Update webrtc-audio-processing usage to new API

[Arun Raghavan]

On Sat, 2016-02-13 at 11:39 +0200, Tanu Kaskinen wrote:
> On Thu, 2016-02-11 at 19:49 +0100, Mihai Moldovan wrote:
> > On 22.01.2016 04:06 PM, Tanu Kaskinen wrote:
> > > On Mon, 2016-01-18 at 13:06 +0530, a...@accosted.net wrote:
> > > > From: Arun Raghavan 
> > > > 
> > > > The code now needs C++11 support to compile with the updated
> > > > webrtc-audio-processing library.
> > > > ---
> > > >  configure.ac  |  2 +-
> > > >  src/Makefile.am   |  2 +-
> > > >  src/modules/echo-cancel/webrtc.cc | 54 
> > > > +--
> > > >  3 files changed, 31 insertions(+), 27 deletions(-)
> > > 
> > > Looks good to me!
> > 
> > Actually... doesn't this deserve a check in configure if the compiler 
> > actually
> > supports C++11? If it doesn't, why make C++11 mandatory for everything? It
> > should only be used for the webrtc-audio-processing lib and otherwise leave
> > CXXFLAGS untouched (with webrtc-audio-processing being optional, as far as 
> > I've
> > seen.)
> 
> Ah, indeed, there should be a check in configure. Leaving CXXFLAGS
> untouched doesn't seem necessary, though. I don't think it makes sense
> to use different C++ standards for different parts of the code, so
> setting the standard globally to C++11 seems fine to me, unless there
> are some practical problems with that. (And since WebRTC is currently
> the only thing in PulseAudio using C++, I don't think it's possible to
> have practical problems.)

Yes, that's the only C++ code, so it's fine. I'll add the same AX macro
we have in pavucontrol for this.

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

Re: [pulseaudio-discuss] ASoC and pulseaudio

[Arun Raghavan]

On Wed, 2016-02-17 at 16:27 +0100, Carlo Caione wrote:
> Hi,
> In our daily work we are seeing more and more laptops coming in with
> SoC audio (ASoC). In the most recent case, we are working with a new
> consumer laptop based on Intel Cherry Trail. As you probably know to
> have audio working on these laptops / codecs a fairly lengthy hw
> specific mixer configuration is needed.
> The problem we are facing is how to ship a single PA / ALSA package
> containing a working configuration for several of those laptops and we
> were wondering what is the most correct / upstream-friendly way to do
> that.
> 
> I'm already excluding here some trivial workarounds like: shipping a
> different PA package for each hw or using an hw-tailored script to
> load the correct ALSA configuration.
> 
> The first solution we came up with is just to write a profile-set
> configuration file loaded by udev for each cards / laptop with the
> correct mixer configuration already embedded in the path configuration
> files. This looks a bit like an overkill when you have to change tens
> of controls most of which just one time (once you have setup the codec
> selecting the main path is usually just matter of changing a few
> controls to change the output / input path).

With PA 8.0, you have the ability to ship distro overrides via .d
directories, etc. And we have the ability to have includes already, so
maybe this is one way to solve this problem.

> Another possibility is to use UCM and let PA create automatically from
> that profile and path configuration files. This looks promising but we
> were a bit worried about the UCM support in PA and how stable/adopted
> is UCM itself.
> 
> So, what is the suggested way to accomplish this and in general how PA
> is trying to address this problem? I expect in the future to see many
> more ASoC coming into the laptops world, how will the community make
> it so that you install a distro and then sound "just works"?

If you are able to express what you need with UCM, it should be fine
for your use case. ChromeOS uses it, and our support is meant to just
work.

The idea thus far has been to collate UCM configuration that works with
mainline kernel in a single ALSA repository, so that'd be the right
place to put such config.

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