Re: [pulseaudio-discuss] [announce] Rust bindings!

[David Henningsson]

On 2018-06-13 09:29, Tanu Kaskinen wrote:

On Tue, 2018-06-12 at 19:03 +0100, jnq...@gmail.com wrote:

On Tue, 2018-06-12 at 11:22 +0300, Tanu Kaskinen wrote:

On Mon, 2018-05-28 at 19:37 +0100, jnq...@gmail.com wrote:

Hi everyone!

Back in February I released 'binding' and 'sys' crates for using
pulseaudio from Rust code. I had intended to make an announcement
here
at the time, but I failed to do so, so I'm doing it now.

fyi, the 'sys' crates provide a simple description of the C
interface;
The 'binding' crates take this further, providing a cleaner/safer
Rust
interface.

I have provided separate crates for each PA system library (per
Rust
guidelines for 'sys' crates), thus totalling six in all:

[binding crates]
  - libpulse-binding: https://crates.io/crates/libpulse-binding
  - libpulse-simple-binding: https://crates.io/crates/libpulse-simpl
e-bi
nding
  - libpulse-mainloop-glib-binding: https://crates.io/crates/libpuls
e-gl
ib-binding
[sys crates]
  - libpulse-sys: https://crates.io/crates/libpulse-sys
  - libpulse-simple-sys: https://crates.io/crates/libpulse-simple-sy
s
  - libpulse-mainloop-glib-sys: https://crates.io/crates/libpulse-ma
inlo
op-glib-sys

The 'binding' crates include plenty of documentation (taken from
the C
API). This can be built locally of course (cargo doc), but is also
available online at docs.rs, example: https://docs.rs/libpulse-bind
ing/

Long term I hope that the owners of the PA project itself would
like to
take over ownership and maintenance. Even longer term hopefully we
will
see PA itself converting to Rust - fyi the PA projects has my full
consent to use this work of mine in converting PA itself.

Cool, thanks for the bindings! I'm afraid you'll have to keep
maintaining the bindings yourself for the foreseeable future - I
don't
really want to take more work for myself at this point (I can of
course
only talk only for myself, but I don't expect the other maintainers
to
be enthusiastically adopting the bindings either). That said,
converting PA to Rust might very well be a good idea. From what I've
heard about combining Rust with C, such conversion could be done
gradually.

Ok no problem :)

I am very glad to hear that you are open to a Rust conversion. I'm very
busy at the moment, but I have given a little thought to it over the
past few days; perhaps I will try to tackle it at some point.

No hurry :) Note that I can't alone make the decision to start
converting to Rust. Not everyone might have as good perception of the
language as me (and that perception isn't based on actually trying to
use the language), and not everyone might want to learn it. Arun and
Georg, how do you feel about the prospect of gradually converting the
codebase to Rust some day?


If I had the time and engagement to start working on a new sound server 
tomorrow, I would write it in Rust.
But just porting PulseAudio to Rust without solving any issues with 
PulseAudio's existing design, does not seem like the best use of time to me.


But should you make the step over to Rust, you're more than welcome to 
use my dbus and alsa bindings :-)


// David

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

Re: [pulseaudio-discuss] [PATCH 1/3] alsa: fix infinite loop with Intel HDMI LPE

[David Henningsson]

On 2017-12-30 13:03, Georg Chini wrote:

On 30.12.2017 10:54, Alexander E. Patrakov wrote:

2017-12-29 20:37 GMT+08:00 Tanu Kaskinen :

On Fri, 2017-12-29 at 11:46 +0800, Alexander E. Patrakov wrote:

2017-12-28 18:09 GMT+08:00 Tanu Kaskinen :
The Intel HDMI LPE driver works in a peculiar way when the HDMI 
cable is
not plugged in: any written audio is immediately discarded and 
underrun
is reported. That resulted in an infinite loop, because PulseAudio 
tried
to keep the buffer filled, which was futile since the written 
audio was

immediately consumed/discarded.

This patch adds special handling for the LPE driver: if the active 
port

of the sink is unavailable, the sink suspends itself. A new suspend
cause is added: PA_SUSPEND_UNAVAILABLE.

I think this is not a complete fix. There was a case in the past where
some other card started eating samples too quickly (some Radeon?
unfortunately, can't find it now). While blacklisting one known bad
driver helps, I think it would be better to detect the misbehavior at
runtime, based on the number of samples written and the wall-clock
time. If the card stalls or eats samples too quickly, set a flag that
it misbehaves, accept the xrun, and then set it to off when
convenient.

Sorry, I have no time to help with the code :(

Did that other sample-eating sound card exhibit the behaviour only when
unplugged? With the LPE driver we know that we can resume once the
cable is plugged in again. If we don't take the jack state into
consideration, then we don't know when we should try resuming.

Yes, it was only when unplugged. The thread (found it!) starts here:

http://mailman.alsa-project.org/pipermail/alsa-devel/2014-September/081365.html 



David: do you know whether it has been fixed in the kernel in that case?


I can change the patch so that the sink is suspended when both of the
conditions are fulfilled: too fast sample consumption and jack
unplugged.

I think an "or" condition would be more appropriate here, for
robustness. The real bug here is the CPU overuse (in the worst case,
infinite loop) due to too-fast sample consumption by a misbehaving
soundcard driver (but we accept that this misbehavior happens and we
have to deal with it). The fact that the cable is unplugged is just
one known trigger.


If the sample rate significantly deviates from the nominal value when
the jack is plugged, would that not mean that the card (or the driver)
is broken and the card is therefore unusable?
I think the only situation where the misbehavior is acceptable is when
the jack is unplugged, so force-suspending the sink on unplug seems
sufficient to me.


Btw, if there are buffers that need to be filled up beyond our own ALSA 
buffer, it would be possible that the card fills those buffers first, 
which might look to us like a "too high sample rate".
There is already a workaround for this problem in alsa-sink.c (see 
comment "USB devices on ALSA seem to hit a buffer
underrun during the first iterations much quicker then we calculate 
here, probably due to the transport latency.").


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

Re: [pulseaudio-discuss] [PATCH 1/3] alsa: fix infinite loop with Intel HDMI LPE

[David Henningsson]

On 2017-12-30 10:54, Alexander E. Patrakov wrote:

2017-12-29 20:37 GMT+08:00 Tanu Kaskinen :

On Fri, 2017-12-29 at 11:46 +0800, Alexander E. Patrakov wrote:

2017-12-28 18:09 GMT+08:00 Tanu Kaskinen :

The Intel HDMI LPE driver works in a peculiar way when the HDMI cable is
not plugged in: any written audio is immediately discarded and underrun
is reported. That resulted in an infinite loop, because PulseAudio tried
to keep the buffer filled, which was futile since the written audio was
immediately consumed/discarded.

This patch adds special handling for the LPE driver: if the active port
of the sink is unavailable, the sink suspends itself. A new suspend
cause is added: PA_SUSPEND_UNAVAILABLE.

I think this is not a complete fix. There was a case in the past where
some other card started eating samples too quickly (some Radeon?
unfortunately, can't find it now). While blacklisting one known bad
driver helps, I think it would be better to detect the misbehavior at
runtime, based on the number of samples written and the wall-clock
time. If the card stalls or eats samples too quickly, set a flag that
it misbehaves, accept the xrun, and then set it to off when
convenient.

Sorry, I have no time to help with the code :(

Did that other sample-eating sound card exhibit the behaviour only when
unplugged? With the LPE driver we know that we can resume once the
cable is plugged in again. If we don't take the jack state into
consideration, then we don't know when we should try resuming.

Yes, it was only when unplugged. The thread (found it!) starts here:

http://mailman.alsa-project.org/pipermail/alsa-devel/2014-September/081365.html

David: do you know whether it has been fixed in the kernel in that case?


No idea. And I've switched graphics card since, so I can't test either.

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

[pulseaudio-discuss] [PATCH] module-switch-on-port-available: Add some basic properties

[David Henningsson]

Apparently I forgot about this for like, five years, but better
late than never!

Signed-off-by: David Henningsson 
---
 src/modules/module-switch-on-port-available.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/modules/module-switch-on-port-available.c 
b/src/modules/module-switch-on-port-available.c
index 4020987..b28e60b 100644
--- a/src/modules/module-switch-on-port-available.c
+++ b/src/modules/module-switch-on-port-available.c
@@ -29,6 +29,11 @@
 
 #include "module-switch-on-port-available-symdef.h"
 
+PA_MODULE_AUTHOR("David Henningsson");
+PA_MODULE_DESCRIPTION("Switches ports and profiles when devices are 
plugged/unplugged");
+PA_MODULE_LOAD_ONCE(true);
+PA_MODULE_VERSION(PACKAGE_VERSION);
+
 struct card_info {
 struct userdata *userdata;
 pa_card *card;
-- 
2.7.4

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

[pulseaudio-discuss] [PATCH] module-switch-on-port-available: Add some basic properties

[David Henningsson]

Test...did it arrive on the mailinglist when posted as an attachment?

// David
>From a1b358615e2f9081f832164e9b91d177845554ca Mon Sep 17 00:00:00 2001
From: David Henningsson 
Date: Fri, 22 Sep 2017 16:20:14 +0200
Subject: [PATCH] module-switch-on-port-available: Add some basic properties

Apparently I forgot about this for like, five years, but better
late than never!

Signed-off-by: David Henningsson 
---
 src/modules/module-switch-on-port-available.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/modules/module-switch-on-port-available.c b/src/modules/module-switch-on-port-available.c
index 4020987..b28e60b 100644
--- a/src/modules/module-switch-on-port-available.c
+++ b/src/modules/module-switch-on-port-available.c
@@ -29,6 +29,11 @@
 
 #include "module-switch-on-port-available-symdef.h"
 
+PA_MODULE_AUTHOR("David Henningsson");
+PA_MODULE_DESCRIPTION("Switches ports and profiles when devices are plugged/unplugged");
+PA_MODULE_LOAD_ONCE(true);
+PA_MODULE_VERSION(PACKAGE_VERSION);
+
 struct card_info {
 struct userdata *userdata;
 pa_card *card;
-- 
2.7.4

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

Re: [pulseaudio-discuss] [PATCH] Add env vars for PA_ALSA_PATHS_DIR and PA_ALSA_PROFILE_SETS_DIR

[David Henningsson]

On 2016-11-25 11:28, Albert Astals Cid wrote:

On Thu, Nov 24, 2016 at 9:51 PM, Tanu Kaskinen  wrote:

On Wed, 2016-11-23 at 17:10 +0100, Albert Astals Cid wrote:

Helps making pulseaudio relocatable


Could you provide more information in the commit message about what use
case requires pulseaudio to be relocatable?


In this particular case is making pulseaudio part of a snap file [1]

But it seems somehow the problem we were having has been solved in a
different way by a colleague, i need to find out what he did and if we
still want to pursue this patch approach or not.


JFTR, if that colleague happens to be me, I'm still lurking around here 
and can answer questions, to the extent that I remember what I actually 
did with that snap thing :-)


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

Re: [pulseaudio-discuss] [PATCH 2/2] pacat: Use a ringbuffer for playback mode

[David Henningsson]

On 2016-11-28 05:01, Ahmed S. Darwish wrote:

Current pacat code reads whatever available from STDIN and writes
it directly to the playback stream. A minimal buffer is created
for each read operation; no further reads are then allowed unless
earlier read buffer has been fully consumed by a stream write.

While quite simple, this model breaks upon the new requirements of
writing only frame-aligned data to the stream (commits #1 and #2).
The kernel read syscall can return a length much smaller than the
frame-aligned size requested, leading to invalid unaligned writes.


Sorry, but I don't understand why you need a ringbuffer to resolve this?

And now that I think of it, I'm not sure that the pa_xmalloc (used to 
allocated the ringbuffer's memory) it guaranteed to be aligned either, 
it just happens to be that way in practice.


Wouldn't it be better if we had something like:

 1) Call pa_stream_begin_write to get a buffer.
 2) If we have half a frame from previous iteration at 4), put that 
half frame first in the buffer.
 3) Call read / pa_read to read data from a file into the rest of the 
buffer.
 4) If the last frame read is not complete, store that half frame in a 
local variable.

 5) Call pa_stream_write with the number of complete frames.
 6) Repeat from 1).




This can easily be reproduced by choosing a starved STDIN backend:

  pacat /dev/randompa_stream_write() failed: EINVAL
  echo 1234 | pacatpa_stream_write() failed: EINVAL

So guard against incomplete kernel reads by using a ringbuffer.
Meanwhile leave the simple recording mode buffering as is: it just
writes to plain STDOUT without any special needs.

CommitReference #1: 22827a5e1e62
CommitReference #2: 150ace90f380
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=98475
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=77595
Signed-off-by: Ahmed S. Darwish 
---
 src/utils/pacat.c | 89 +++
 1 file changed, 57 insertions(+), 32 deletions(-)

diff --git a/src/utils/pacat.c b/src/utils/pacat.c
index 68362ec..39c002e 100644
--- a/src/utils/pacat.c
+++ b/src/utils/pacat.c
@@ -38,10 +38,12 @@
 #include 
 #include 

+#include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 

@@ -56,6 +58,11 @@ static pa_context *context = NULL;
 static pa_stream *stream = NULL;
 static pa_mainloop_api *mainloop_api = NULL;

+/* Ring buffer for playback mode */
+static pa_ringbuffer ringbuffer = { 0, };
+static pa_atomic_t rb_count = PA_ATOMIC_INIT(0);
+
+/* Flat buffer for recording mode */
 static void *buffer = NULL;
 static size_t buffer_length = 0, buffer_index = 0;

@@ -155,29 +162,35 @@ static void start_drain(void) {

 /* Write some data to the stream */
 static void do_stream_write(size_t length) {
-size_t l;
-pa_assert(length);
+int rb_freelen;
+void *buf;
+size_t w;

-if (!buffer || !buffer_length)
+if (!ringbuffer.memory)
 return;

-l = length;
-if (l > buffer_length)
-l = buffer_length;
+while (length > 0) {

-if (pa_stream_write(stream, (uint8_t*) buffer + buffer_index, l, NULL, 0, 
PA_SEEK_RELATIVE) < 0) {
-pa_log(_("pa_stream_write() failed: %s"), 
pa_strerror(pa_context_errno(context)));
-quit(1);
-return;
-}
+buf = pa_ringbuffer_peek(&ringbuffer, &rb_freelen);

-buffer_length -= l;
-buffer_index += l;
+/* Frame-align our audio packets or the server will happily ignore
+ * them upon arrival. Also only write as much data as requested: do
+ * not overflow the stream by emptying the whole ringbuffer. */
+w = PA_MIN(pa_frame_align(length, &sample_spec),
+   pa_frame_align(rb_freelen, &sample_spec));
+if (!w)
+break;

-if (!buffer_length) {
-pa_xfree(buffer);
-buffer = NULL;
-buffer_index = buffer_length = 0;
+if (pa_stream_write(stream, buf, w, NULL, 0, PA_SEEK_RELATIVE) < 0) {
+pa_log(_("pa_stream_write() failed: %s"), 
pa_strerror(pa_context_errno(context)));
+quit(1);
+return;
+}
+
+pa_ringbuffer_drop(&ringbuffer, w);
+
+pa_assert(w <= length);
+length -= w;
 }
 }

@@ -192,9 +205,6 @@ static void stream_write_callback(pa_stream *s, size_t 
length, void *userdata) {
 if (stdio_event)
 mainloop_api->io_enable(stdio_event, PA_IO_EVENT_INPUT);

-if (!buffer)
-return;
-
 do_stream_write(length);

 } else {
@@ -540,25 +550,42 @@ fail:

 /* New data on STDIN **/
 static void stdin_callback(pa_mainloop_api*a, pa_io_event *e, int fd, 
pa_io_event_flags_t f, void *userdata) {
-size_t l, w = 0;
-ssize_t r;
-bool stream_not_ready;
+int rb_freelen;
+void *buf;
+size_t writable, r;

 pa_assert(a == mainloop_api);
 pa_assert(e);
 pa_assert(stdio_event == e);

-stream_not_ready = !stream || pa_stream_ge

Re: [pulseaudio-discuss] [PATCH] iochannel: Strictly specify PF_UNIX ancillary data boundaries

[David Henningsson]

Interesting bug :-)

Just so I get this right - this could just as well be fixed with instead 
changing msg_controllen to this:


mh.msg_controllen = CMSG_SPACE(sizeof(int) * nfd);

That said, your version works equally well and is slightly cleaner. I 
don't recall we have any problems with VLAs.


Perhaps adding a comment to your finding and a registered kernel 
bug/patch/etc would be helpful.


Anyhow, good work, so:

Acked-by: David Henningsson 


On 2016-11-15 17:48, Ahmed S. Darwish wrote:

Users reported audio breakage for 32-bit pulse clients connected
to a 64-bit server over memfds. Investigating the issue further,
the problem is twofold:

1. iochannel's file-descriptor passing code is liberal in what it
   issues: produced ancillary data object's "data" section exceeds
   length field. How such an extra space is handled is a grey area
   in the POSIX.1g spec, the IETF RFC #2292 "Advanced Sockets API
   for IPv6" memo, and the cmsg(3) manpage.

2. A 64-bit kernel handling of such extra space differs by whether
   the app is 64-bit or 32-bit. For 64-bit apps, the kernel
   smartly ducks the issue. For 32-bit apps, an -EINVAL is
   directly returned; that's due to a kernel CMSG header traversal
   bug in the networking stack "32-bit sockets emulation layer".

   Compare Linux Kernel's socket.h cmsg_nxthdr() code and the
   32-bit emulation layer version of it at net/compat.c
   cmsg_compat_nxthdr() for further info. Notice how the former
   graciously ignores incomplete CMSGs while the latter _directly_
   complains about them -- as of kernel version 4.9-rc5.

   (A kernel patch is to be submitted)

Details:

iochannel typically uses sendmsg() for passing FDs & credentials.
From RFC 2292, sendmsg() control data is just a heterogeneous
array of embedded ancillary objects that can differ in length.
Linguistically, a "control message" is an ancillary data object.

For example, below is a sendmsg() "msg_control" containing two
ancillary objects:

|<-- msg_controllen-->|
| |
|<--- ancillary data object -->|<- ancillary data object->|
|<--- CMSG_SPACE() --->|<--- CMSG_SPACE() --->|
|  |  |
|< cmsg_len --->|  |< cmsg_len --->|  |
|<--- CMSG_LEN() -->|  |<--- CMSG_LEN() -->|  |
|   |  |   |  |
+-+-+-+--+--+--+-+-+-+--+--+--+
|cmsg_|cmsg_|cmsg_|XX|cmsg_ |XX|cmsg_|cmsg_|cmsg_|XX|cmsg_ |XX|
|len  |level|type |XX|data[]|XX|len  |level|type |XX|data[]|XX|
+-+-+-+--+--+--+-+-+-+--++-+--+
 ^^^ Ancil Object #1^^^ Ancil Object #2
 (control message)  (control message)
^
|
+--- sendmsg() "msg_control" points here

Problem is, while passing FDs, iochannel's code try to avoid
variable-length arrays by creating a single cmsg object that can
fit as much FDs as possible:

  union {
struct cmsghdr hdr;
uint8_t data[CMSG_SPACE(sizeof(int) * MAX_ANCIL_DATA_FDS)];
  } cmsg; ^^

Most of the time though the number of FDs to be passed is less
than the maximum above, thus "cmsg_len" is set to the _actual_ FD
array size:

  cmsg.hdr.cmsg_len = CMSG_LEN(sizeof(int) * nfd);
 ^^^
This inconsistency tricks the kernel into thinking that we have 2
ancillay data objects instead of one! First cmsg is valid as
intended, but the second is instantly _corrupt_ since it has a
cmsg_len size of 0 -- thus failing kernel's CMSG_OK() tests.

For 32-bit apps on a 32-bit kernel, and 64-bit apps over a 64-bit
one, the kernel's own CMSG header traversal macros just ignore the
second "incomplete" cmsg. For 32-bit apps over a 64-bit kernel
though, the kernel 32-bit socket emulation macros does not forgive
such incompleteness and directly complains of invalid args (due to
a subtle bug).

Avoid this ugly problem, which can also bite us in a pure 64-bit
environment if MAX_ANCIL_DATA_FDS got extended to 5 FDs, by
setting "cmsg_data[]" array size to "cmsg_len".

BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=97769

Signed-off-by: Ahmed S. Darwish 
---

Hopefully variable-length arrays won't be problematic in esoteric
build environments?

 src/pulsecore/iochannel.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/pulsecore/iochannel.c b/src/pulsecore/iochannel.c
index e62750b..8ace297 100644
--- a/src/pulsecore/iochannel.c
+++ b/src/pulsecore/iochannel.c
@@ -346,6 +346,8 @@ ssize_t pa_iochannel_write_with_creds(pa_iochannel*io, 
const void*data, size_t l
 return r;
 }

Re: [pulseaudio-discuss] update-card-properties? Possible?

[David Henningsson]

(Sorry if this email reaches you twice, some SMTP problems here)

On 2016-08-17 17:11, Tanu Kaskinen wrote:

On Wed, 2016-08-17 at 12:55 +1000, Bernd Wechner wrote:

But if I've helped spot a pulseaudio bug (failure to copy the HDMI
product name to the card description in some way), that's that's nice. I
do think it would awesome and seemingly an easy fix to see those HDMI
device identifiers in the Sound settings through the card
device.description.


Note that the HDMI product name shouldn't be used as the card
description, at least in most cases, because often a card that provides
HDMI has multiple devices - there might be analog devices on the same
card, or multiple HDMI ports. It makes more sense as a port
description.

The Gnome sound settings output selection list, for example, uses this
format in the labels:

 - 

If HDMI is provided by the built-in sound card, I guess using the HDMI
product name as the port description would look like this:

 - Built-in Audio

I'd imagine the "Built-in Audio" part in combination with the monitor
name would look a bit strange to many users.


Certainly e g "HDMI / DisplayPort - Samsung TV" would look better to me.


Maybe that's why we
haven't gone as far as replacing the port description with the product
name? I'll add David Henningsson to Cc in case he wants to comment.
He's the one who implemented the monitor name reading.


I would like the monitor name somewhere in the Gnome/Unity UI, and 
perhaps somewhere in pavucontrol as well.


If I try to remember why that never happened, I can think of a few reasons:

 1) Changing UI things (in both Gnome and Unity) require design 
decisions etc which is a heavy-weight process


 2) I was hoping someone else would do it :-)

 3) Somebody warned that the monitor name would be wrong due to 
hardware and/or driver issues


 4) This dynamic HDMI PCM stuff (reprobing etc) that never got 
implemented either - e g, if we want volumes stored per connected 
monitor, then monitor name would become part of the ID somehow, etc, 
maybe then the connected monitor name would actually end up as port (or 
card?) name, and then Gnome/Unity would get it automatically instead...





As to the simple use case and why someone would like to change the name
that is easily answered.

1) USB devices identify themselves with weird or unclear names. HDMI
devices similarly. They are techie and device descriptive.

2) These names used as well in the System tray sound apps that allow
selection of output device for example.

3) I have family and others using a multimedia machine, as I suspect
many do who want to be able to select output device, pump sound into one
attached unit or another (I have quite a few).

4) For that reason I'd like freedom to name these devices, in a way that
sticks, and ideally survives unplugging and plugging in again, to be
meaningful to end users. Names like "Wireless headphones", "Surround
sound system", "Bedroom speakers", "FM transmitter" etc, by way of
example. As it is essentially with complex sound systems and multiple
devices served any user has to try them one by one until they detect
sound where they want it. That is neither practical fro all devices nor
friendly so most steer clear of it.

5) Further, it'd be nice to name them in the Sound Settings GUI and not
have to lay around with config files in /etc if possible.


All these use cases seem like they deal only with the descriptions. You
made earlier a distinction between card names and card descriptions,
and it's unclear where you'd need to care about the card name.


I migrated from Windows a while back and there I could name the sound
devices. And I suspect it's managed at a level higher than ALSA, maybe
at a level like pulseaudio, and possible even at a higher level still.

Getting my head around where this might fit. As I may be in a position
to contribute code of course once I know where is most appropriate and
how to contribute to the relevant community.


If you decide to start working on the code, feel free to ask any
questions on this mailing list or in IRC (#pulseaudio at freenode.net).

--
Tanu


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

Re: [pulseaudio-discuss] Access control

[David Henningsson]

On 2016-07-15 11:43, Wim Taymans wrote:



On 15 July 2016 at 11:14, David Henningsson <mailto:di...@ubuntu.com>> wrote:




On 2016-07-15 11:05, Wim Taymans wrote:

Hi guys,

I'm having another look at the access control patches. I
revived my old
patches and found some trouble with the async stuff that I
fixed here:

https://cgit.freedesktop.org/~wtay/pulseaudio/log/?h=access-hooks
<https://cgit.freedesktop.org/%7Ewtay/pulseaudio/log/?h=access-hooks>
<https://cgit.freedesktop.org/%7Ewtay/pulseaudio/log/?h=access-hooks>


FWIW, I also remember fixing a bug or two before I added your
patches to Ubuntu. The way it looks in Ubuntu now is here:


http://anonscm.debian.org/cgit/pkg-pulseaudio/pulseaudio.git/tree/debian/patches?h=ubuntu

(see patches 0406, 0407 and 0408)


It looks ok, you fixed the length in _copy and the reading of the 
command and tag when resuming the async operation.


Interestingly you added the pa_creds to the pa_client. Any reason not 
to upstream this?


Feel free to upstream whatever makes sense. I don't remember exactly why 
I ended up doing things the way I did - all I remember is that I was 
reviewing your set and then waiting for you to submit a v2 (or perhaps 
further discussion?), and then that didn't happen before I needed the 
patches in Ubuntu.


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

Re: [pulseaudio-discuss] Access control

[David Henningsson]

On 2016-07-15 11:05, Wim Taymans wrote:

Hi guys,

I'm having another look at the access control patches. I revived my old
patches and found some trouble with the async stuff that I fixed here:

https://cgit.freedesktop.org/~wtay/pulseaudio/log/?h=access-hooks 



FWIW, I also remember fixing a bug or two before I added your patches to 
Ubuntu. The way it looks in Ubuntu now is here:


http://anonscm.debian.org/cgit/pkg-pulseaudio/pulseaudio.git/tree/debian/patches?h=ubuntu

(see patches 0406, 0407 and 0408)



There is also an example on how to start and complete an async access
check for starting a recording. I believe Ahmed Darwish is building on
top of that so it might be useful to get it working.

Now I'm taking a look at the info in pa_client that is available to decide
what access checks we need to do for each client.

Ideally we would need the pid of the process with we can currently find
in the pa_proplist of the client. Unfortunately this pid is whatever 
the client

sends us in a proplist in the set_client_name command so we need something
more secure.

We do send the pid and gid with the SCM_CREDENTIALS ancillary data in
the AUTH command. Since the kernel checks things, we can be guaranteed
that when we get the credentials, they are correct.

What I would like to do is make these credentials available somewhere. I
would like to make a new key in the client proplist with the verified 
pid from

the credentials but the problem is that we then need to make sure that a
set_client_name command can't overwrite the value, which involves some
filtering or keys.

Alternatively we could make a new pa_client field to store the 
verified pid

and gid.. Does this sound better or worse?

Wim









___
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 0/5] Add access control to protocol-native

[David Henningsson]

On 2016-07-01 03:30, Ahmed S. Darwish wrote:

Hi David,

[ Thanks a lot for chiming back in! ]


Anytime :-)



On Fri, Jun 24, 2016 at 05:21:53PM +0200, David Henningsson wrote:


On 2016-06-24 02:30, Ahmed S. Darwish wrote:

Hi,

Apologies for resurrecting this year-old thread. [*] Hopefully we
can finish this, with Flatpak integration as an access-control
module, before v10.0 ;-) [1] [2] [3]

Some questions below..

On 2015-07-17 13:02, David Henningsson wrote:


On 2015-04-07 17:13, Wim Taymans wrote:

This is a new patch series that preplaces the previous patches regarding
access control.


...


There is an example access control module that shows how one could implement
client specific access control checks.




...


A few design thoughts that I think we need to resolve first before
reviewing details:

1. Coupling between core and native-protocol. Right now the hooks are in
core, and mapped 1-to-1 (or? are there exceptions?) to PA_COMMAND_*.

Other options would be:
- Just have a "pa_hook access[PA_COMMAND_MAX]" in pa_core instead?
Then we could skip the long list of PA_ACCESS_HOOK_ constants. However,
the increased dependency between the native protocol and the pa_core
object might be undesirable.
- On the other side, we could have the "pa_hook
access[PA_COMMAND_MAX]" in pa_native_protocol instead. However, native
protocol instances could - at least in theory - come and go, so they
probably need stored somewhere more reachable anyhow...

To sum up, I feel that since the hooks are specific to the native
protocol, they should be put closer to that protocol, but I can't point
to a better place to put it.



After looking at this, isn't protocol-native a right place for
the hooks? We're only protecting the protocol-native commands
anyway.

There's also a precedent in having hooks in pa_protocol_native.
Namely at protocol-native.c:

 struct pa_native_protocol {
 PA_REFCNT_DECLARE;
 pa_core *core;
 ...
 pa_hook hooks[PA_NATIVE_HOOK_MAX];  // <-- here
 ...
 };

 pa_hook *pa_native_protocol_hooks(pa_native_protocol *p) {
 ...
 return p->hooks;
 }

And modules can get access to the hooks directly using the
pa_native_protocol_hooks() accessor above. This is how it's done
in module  device-manager, module device-restore, and module
stream-restore, etc.

Anything blocking us from doing the same for the access control
hooks? This would indeed save us from the 1-to-1 mapping table.

_@diwic_: You mention that native protocol instances could at
least in theory come and go. But if that happened, what would be
the dangers of freeing the hooks along with protocol-native and
be done with it? In a sense, the access-control hooks themselves
won't make any sense without protocol-native anyway?


Suppose the following chain of events:

  1. Protocol-native loads
  2. Access control module loads, puts hooks into existing protocol-native
instance
  3. Protocol-native unloads
  4. Another Protocol-native loads

At this point the new protocol-native is unprotected, which is probably not
what you want?

Also we have to make sure we don't get use-after-free errors, no matter
which order the modules are unloaded. But that's certainly solvable, just an
additional thing to remember.



Can we solve the above two points by by introducing a pulse module
dependencies feature, just like what the linux kernel does? [1]

If so, we can do the following:

(a) Introduce module dependencies

(b) Put the access-control hooks inside protocol-native itself,
 and thus remove the 1-to-1 mappings [2] and avoid making core
 dependent on protocol-native (even implicitly)

(c) Make all policy modules, which contains all the hooks
 implementations, hard-dependent on protocol-native

In the first point raised, to unload protocol-native, user will
have to unload the dependent protocol-native-policy-X first. So if
the user loads protocol-native again, he/she will know that the
policy module will need to be reloaded as well ..

In the second point raised, hopefully module dependencies will
simplify the relationship: use-after-free errors can be eliminated
given proper cleanup by the policy modules ..

Any thoughts on this?



Just looking at core.h there seems to be both PA_CORE_HOOK_MODULE_NEW 
and PA_CORE_HOOK_MODULE_UNLINK hooks, perhaps it could be as simple as 
just requiring any access control module to listen to these and take the 
appropriate action?


As for whether or not a generic "module dependencies" feature would be 
good or bad: I would find it more user friendly myself if the user would 
not have to manually reload access control modules, but since I'm not 
that involved anymore, I don't think I should have decision power on 
that matter. :-)



thanks,

[1] http://man7.org/linux/man-pages/man5/modules.dep.5.html

[2] For details on the 1-to-1 mapping, ch

Re: [pulseaudio-discuss] [PATCH 0/5] Add access control to protocol-native

[David Henningsson]

On 2016-06-24 02:30, Ahmed S. Darwish wrote:

Hi,

Apologies for resurrecting this year-old thread. [*] Hopefully we
can finish this, with Flatpak integration as an access-control
module, before v10.0 ;-) [1] [2] [3]

Some questions below..

On 2015-07-17 13:02, David Henningsson wrote:


On 2015-04-07 17:13, Wim Taymans wrote:

This is a new patch series that preplaces the previous patches regarding
access control.


...


There is an example access control module that shows how one could implement
client specific access control checks.




...


A few design thoughts that I think we need to resolve first before
reviewing details:

1. Coupling between core and native-protocol. Right now the hooks are in
core, and mapped 1-to-1 (or? are there exceptions?) to PA_COMMAND_*.

Other options would be:
   - Just have a "pa_hook access[PA_COMMAND_MAX]" in pa_core instead?
Then we could skip the long list of PA_ACCESS_HOOK_ constants. However,
the increased dependency between the native protocol and the pa_core
object might be undesirable.
   - On the other side, we could have the "pa_hook
access[PA_COMMAND_MAX]" in pa_native_protocol instead. However, native
protocol instances could - at least in theory - come and go, so they
probably need stored somewhere more reachable anyhow...

To sum up, I feel that since the hooks are specific to the native
protocol, they should be put closer to that protocol, but I can't point
to a better place to put it.



After looking at this, isn't protocol-native a right place for
the hooks? We're only protecting the protocol-native commands
anyway.

There's also a precedent in having hooks in pa_protocol_native.
Namely at protocol-native.c:

struct pa_native_protocol {
PA_REFCNT_DECLARE;
pa_core *core;
...
pa_hook hooks[PA_NATIVE_HOOK_MAX];  // <-- here
...
};

pa_hook *pa_native_protocol_hooks(pa_native_protocol *p) {
...
return p->hooks;
}

And modules can get access to the hooks directly using the
pa_native_protocol_hooks() accessor above. This is how it's done
in module  device-manager, module device-restore, and module
stream-restore, etc.

Anything blocking us from doing the same for the access control
hooks? This would indeed save us from the 1-to-1 mapping table.

_@diwic_: You mention that native protocol instances could at
least in theory come and go. But if that happened, what would be
the dangers of freeing the hooks along with protocol-native and
be done with it? In a sense, the access-control hooks themselves
won't make any sense without protocol-native anyway?


Suppose the following chain of events:

 1. Protocol-native loads
 2. Access control module loads, puts hooks into existing 
protocol-native instance

 3. Protocol-native unloads
 4. Another Protocol-native loads

At this point the new protocol-native is unprotected, which is probably 
not what you want?


Also we have to make sure we don't get use-after-free errors, no matter 
which order the modules are unloaded. But that's certainly solvable, 
just an additional thing to remember.





Regards,

[*] Full thread originally archived at http://goo.gl/8h7876

[1] Simple kernel based mechanism to know if a client is inside
 a Flatpak, from a PID which we can get through the unix
 socket credentials passing, is here: https://goo.gl/OCfmBW

[2] Initial repo here
 https://github.com/a-darwish/pulseaudio/commits/access-control-v1

[3] Wim Tay was kind enough to welcome building on top of this
 patchset! Thanks a lot Wim :-)


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

Re: [pulseaudio-discuss] Should we ban Raymond from bugzilla?

[David Henningsson]

On 2016-05-04 06:49, Alexander E. Patrakov wrote:
Raymond produces too many irrelevant comments and seems not to 
understand what he is talking about or what was already debugged. May 
I request a ban in order to increase our signal-to-noise ratio?




First; I think it's inappropriate to ask this 1) in a public forum and 
2) without involving Raymond in the conversation.


But since you asked in public, I'm inclined to answer in public. I've 
seen a lot of Raymond's comments over the years both here and in the 
Ubuntu bug tracker, sometimes he's spot on and very helpful, and 
sometimes he's discussing things that are irrelevant to the bug, which 
can be confusing. Maybe we have a tendency to notice the latter but not 
the former?


But it also strikes me that we've been talking about Raymond rather than 
to Raymond, and I'd like to change that - better late than never.


So, Raymond,

Hi! Would you like to introduce yourself a bit? Tell us where you're 
from, what your motivations are, and how we can help you to focus on 
solving what the bug is about, rather than getting side tracked on other 
potential issues?


// David

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

Re: [pulseaudio-discuss] Rethinking how we do reviews

[David Henningsson]

On 2016-04-06 10:25, Arun Raghavan wrote:

On Tue, 2016-04-05 at 19:28 +0300, Tanu Kaskinen wrote:

On Mon, 2016-04-04 at 13:35 +0530, Arun Raghavan wrote:

On 4 April 2016 at 13:01, David Henningsson  wrote:




On 2016-04-04 04:51, Arun Raghavan wrote:



On Sat, 2016-04-02 at 17:19 +0300, Tanu Kaskinen wrote:



On Thu, 2016-03-31 at 10:55 +0530, Arun Raghavan wrote:




We have two opposing axes to optimise along. The first is code quality,
as you point out. The other is developer motivation.

My feeling is that being able to work on things and push them out
without a long wait would help us feel more productive and motivated to
work on all aspects of the project.

With the current system, I think we're spending more and more time
reviewing things and waiting for reviews.

Having the ability to work on things and push them out provides a much
shorter path between "this is something I'd like to do" and "this thing
is done". This allows for a positive feedback loop, and I think that
will bolster our motivation to perform reviews, bug triaging, and
everything else.

That sounds like a plausible theory, although I don't personally feel
demotivated by the large amount of time it takes for my own patches to
get merged. If you really think that your review bandwidth increases if
your own patches go through faster, it seems to me that I could
prioritize reviewing your patches over others, and that way get more
reviews from you. What do you think?

I think that a this would help, but I fear that this does not do
enough. My preference is still the relaxed (but still existent) review
process that I outlined before, so that we have /some/ hope of catching
up on the patch and bug backlog over time.

Perhaps we can give it a try what I'd proposed for a couple of release
cycles and evaluate how things stand?



For me, the demotivator has been when we disagree on things, if someone
wants my patch written in another way: I can then choose either to rewrite
my code to someone else's liking even though I like it better the way it is
(which is demotivating), or I can choose to respond back which can lead to
long discussions, which is in itself demotivating and time consuming.
I don't know if there exists a good solution to that problem, but I've been
thinking along the lines of having areas of responsibility, where one is
effectively dictator for some pieces of the code, and someone else is
dictator for other pieces, etc.

I think there are ways to short-circuit long discussions, such as
setting aside some time for real-time discussion on IRC.

And perhaps it does make sense to take on more autonomy for certain
areas if that helps us work better. Note that my review proposal does
overlap with your suggestion in that non-external-facing changes can
be done more autonomously.

I'm not sure David's proposal overlaps with your proposal. David didn't
explicitly say whether these dictators themselves should still have
their own patches in their own area reviewed. I understood David's
proposal only as a tool to make it easier to decide things when there
are disagreements, so it wouldn't imply a right to skip reviews.

You're right, and it's quite possible I interpreted David's suggestion
differently from what he meant.


It's not a finished thought, but maybe the amount of review dictators 
need for their own area is something like: simple patches can go 
straight in, medium patches can have what we have today for simple 
patches (i e wait a week, if there is no reviews then pushing can be 
okay), and difficult world-changing patches should have a pair of extra 
eyes.


And what patches are "simple", "medium" and "difficult" would be a 
learning / gut feeling thing.





I quite like the general idea of having a dictator to resolve
disagreements, but it's not obvious how the code base could best be
divided. I don't myself have any particular desire to be the dictator
for any part of the code, but I don't have a problem with taking that
role either if it's given to me.

I guess the idea isn't different from subsystem maintainers in Linux,
but I'm not sure we're a large enough project to be able to divide up
responsibilities quite like that. Maybe there's a way to do it in a way
that makes sense, though.


As for Tanu's suggestion (to prioritise Arun and see if that helps) vs
Arun's (every committer is free to push their own stuff), I prefer
evaluating Tanu's, as I see a risk with Arun's proposal that committers will
spend more time on their own stuff rather than reviewing, if the former is
what gives that positive feedback loop.

To me, the latter gives a stronger positive feedback loop than the
former, because you're still waiting for a few days at least for each
patch set.

Does it really demotivate you if your patches take a few 

Re: [pulseaudio-discuss] Rethinking how we do reviews

[David Henningsson]

On 2016-04-04 04:51, Arun Raghavan wrote:

On Sat, 2016-04-02 at 17:19 +0300, Tanu Kaskinen wrote:

On Thu, 2016-03-31 at 10:55 +0530, Arun Raghavan wrote:


We have two opposing axes to optimise along. The first is code quality,
as you point out. The other is developer motivation.

My feeling is that being able to work on things and push them out
without a long wait would help us feel more productive and motivated to
work on all aspects of the project.

With the current system, I think we're spending more and more time
reviewing things and waiting for reviews.

Having the ability to work on things and push them out provides a much
shorter path between "this is something I'd like to do" and "this thing
is done". This allows for a positive feedback loop, and I think that
will bolster our motivation to perform reviews, bug triaging, and
everything else.

That sounds like a plausible theory, although I don't personally feel
demotivated by the large amount of time it takes for my own patches to
get merged. If you really think that your review bandwidth increases if
your own patches go through faster, it seems to me that I could
prioritize reviewing your patches over others, and that way get more
reviews from you. What do you think?

I think that a this would help, but I fear that this does not do
enough. My preference is still the relaxed (but still existent) review
process that I outlined before, so that we have /some/ hope of catching
up on the patch and bug backlog over time.

Perhaps we can give it a try what I'd proposed for a couple of release
cycles and evaluate how things stand?


For me, the demotivator has been when we disagree on things, if someone 
wants my patch written in another way: I can then choose either to 
rewrite my code to someone else's liking even though I like it better 
the way it is (which is demotivating), or I can choose to respond back 
which can lead to long discussions, which is in itself demotivating and 
time consuming.
I don't know if there exists a good solution to that problem, but I've 
been thinking along the lines of having areas of responsibility, where 
one is effectively dictator for some pieces of the code, and someone 
else is dictator for other pieces, etc.


As for Tanu's suggestion (to prioritise Arun and see if that helps) vs 
Arun's (every committer is free to push their own stuff), I prefer 
evaluating Tanu's, as I see a risk with Arun's proposal that committers 
will spend more time on their own stuff rather than reviewing, if the 
former is what gives that positive feedback loop.


// David

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

Re: [pulseaudio-discuss] [PATCH v3 00/11] Introduce memfd support

[David Henningsson]

Hello!

I've now merged and pushed patches 1 to 9 after some quick checks. When 
looking at patch 10 I thought that was part of the final enablement 
sequence (after all, it bumps the protocol so it's not as easy to revert 
as the rest of the patches, in case something is wrong), so I figured 
both 10 and 11 could be part of your next patch set.


I added a trivial patch on top of your patch set that fixes two compiler 
warnings I saw.


Thanks!

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

Re: [pulseaudio-discuss] [PATCH v3 00/11] Introduce memfd support

[David Henningsson]

On 2016-03-22 01:56, Ahmed S. Darwish wrote:

On Mon, Mar 21, 2016 at 10:01:14AM +0100, David Henningsson wrote:


On 2016-03-12 23:35, Ahmed S. Darwish wrote:

Hello!

The simplified memfd patch series ;-)


Hi!

Good work!

I've read through the patch series and think they look pretty much okay, and
your amount of testing is exemplary.



Thanks a lot .. couldn't do it without everyone's help here :-)

Also, regarding the testing, I really did not want to increase
anyone's Launchpad bugfixing workload ;-)


Surely there are nitpicks here and there but at this point I think maybe
we're better off merging the patch series as it is to get wider testing from
developers using it in practice.



Great, I'll be around if there's anything in need of a fix.


My only concern or thought is about the global mempool. By turning that into
a memfd by default, we would potentially slow down clients which support
posix shm but not memfd. I'm not sure if this is a problem in practice
though - even an old closed-source client would (hopefully?) bind to a new
libpulse library and thus gain memfd support that way, and I hope nobody
links libpulse statically. Thoughts?



Alexander mentioned the SteamOS case, where "they don't link
statically, but have a 'known-good' copy of the library packaged
and put into LD_LIBRARY_PATH, and it may be next to impossible to
explain to the users how to upgrade it."

I'm sure though that even if they have a 'known-good' copy, they
keep the daemon version in-sync with the library version? I can't
think of any sane reason not to do so..

Anyway, if that case is indeed problematic, maybe we can lobby
them a bit to do a proper distribution of PA? If anyone has some
contacts, I'll be glad to open a communication channel..


Aaaand...there we stalled. I could use a second opinion on:

 1) Whether to go ahead and merge, despite not promising to be around 
much to test it


 2) If that merge should include all 11 patches or not.

Given the fact that the possible performance regression only affects a) 
input data and b) only old copies of libpulse, I think the chances are 
fairly small.


One option could be to merge all patches as-is but on top of that set 
"enable-memfd" to default to false for one release cycle; to give Steam 
and others a chance to update. That will also give pioneers some extra 
time for testing, and report bugs if there are any.


Not to mention the fact that if you play a Steam game, rendering 
triangles will probably be a bigger performance concern than 
transferring audio data over unix pipes. :-) That said; in the future 
packaging libs with the app might be more popular, e g ubuntu-snappy 
would to that. Not sure how xdg-app would work in this regard.


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

Re: [pulseaudio-discuss] Rethinking how we do reviews

[David Henningsson]

On 2016-03-28 09:13, Arun Raghavan wrote:

Hello,
I'm sure it's not shocking or surprising to state that our patch review
bandwidth is significantly lower than the rate at which contributions
are coming in.

It is also quite demotivating to send patches to the list and have to
wait weeks to hear about them, and kudos to all our patient
contributors and everyone who's been pitching in on the review process,
especially Tanu who's taken up the bulk of the heavy-lifting on this
front.

While having Patchwork in place helps keep track of patches so we don't
lose some through the cracks, it does not help decrease our review
turnaround time by a whole lot.

To this end, I propose that we ease our review policy a bit. My
proposal is that:

* Committers should have the ability to go ahead and push out their own
changes without review, except ...

* User-facing changes should have some announcement and/or discussion
(changing dependencies, new modules, etc.)

* Changes to API or protocol should undergo review at least to the
extent of the API/protocol change

* Large infrastructure changes should go through a full review
(slightly subjective, but I think we can leave this to individual
judgment)

Our current way of doing things is good for keeping up code quality,
but I think over time, with such a large patch backlog, we end up
spending more and more time performing reviews, and less and less time
working on features. This becomes quite draining and drops our overall
productivity in contributing to the project.

At this point, I guess this is mostly for Tanu to buy into, and maybe
David if he'd like to continue contributing at least on the ALSA side.
Thoughts and suggestions from others are still welcome, of course.


I agree in large with the problem statement, but I'm not really seeing 
how your suggestion addresses the problem. Giving committers a fast path 
seems to rather encourage committers to work on their own stuff rather 
than reviewing other people's patches.


Also, it's not the trivial "fix-a-typo" patches that are the problem, 
those are easy to review and commit - it's the large patchsets (such as 
memfd, RAOP2, etc) that take time. And those would still require manual 
review and design thinking.


My preferences is that we should instead be less picky about patches. 
And then I mean less picky about things that don't cause bugs; like 
coding style, variable names, that sort of stuff. If the overall 
architecture of a patch (or patch set) is good and does not cause 
regressions, then let it go in. If then someone wants to rename a 
variable or fix some coding style issue, that can then be done as a 
separate patch.


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

Re: [pulseaudio-discuss] Stepping down; future thoughts

[David Henningsson]

On 2016-03-29 01:31, Ahmed S. Darwish wrote:

Hi David :-)

On Mon, Mar 28, 2016 at 06:36:30AM +0200, David Henningsson wrote:

I've decided to stop working for Canonical, and with that, I intend to ramp
down my contributions to PulseAudio, on both upstream and Ubuntu levels.



As a user, I send my gratitude for all of your work for a better
Linux audio. And as a recent contributor, _big_ _thanks_ for your
very solid reviews and IRC discussions .. I've definitely learned
a lot (summary: "keep it simple")


Thanks! You've been very receptive to feedback, and I'd encourage you to 
continue contributing to PulseAudio!



I could have stayed as a volunteer, but in fact I've been a bit frustrated
over some of PulseAudio's design choices for quite some time. Maybe some of
those choices were right at the time when they were made, but they make us a
not good enough fit for tomorrow.

 From one end comes the embedded ASoC drivers, who have never heard of
timer-based scheduling, but can reconfigure themselves to decode mp3 in
hardware.

 From the other end come sandboxed apps, and the security requirements that
come with it.



Memfds + per-client global mempool + policy + protections against
malicious clients + a fuzzer, and we should then be OK - right? :)


Indeed your memfds work is a step in that direction, but as you say, 
lots of steps remain - e g the part that makes sure clients cannot mute, 
or introspect, other clients, maybe that falls under "policy"?



And routing - we tried, but we never got that right. It's been difficult,
not only due to all use cases and different demands and ideas from different
groups of people, but also due to the way PulseAudio is built up internally.

In software nothing is impossible, but to re-architecture PulseAudio to
support all of these requirements in a good way (rather than to "build
another layer on top", which in the long run would make the PulseAudio even
more difficult to maintain) would be very difficult, so my judgment is that
it would be easier to write something new from scratch.



Would the Linux userspace be ready for yet another audio API?


It is not fair to ask for thousands of applications to rewrite their 
audio backend just because someone invented another API. Certainly one 
has to write some kind of compatibility APIs; exactly how to do this for 
PulseAudio I'm not certain yet.


That would actually be an interesting discussion to have, suppose 
someone writes a new audio server and wants to provide compatibility for 
existing PulseAudio applications; what approach does upstream PulseAudio 
recommend?


(Side note: This has already happened - roaraudio does this, using 
LD_LIBRARY_PATH hacks to inject a new libpulse. It's the only app I know 
of that tries to provide PulseAudio application compatibility.)


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

Re: [pulseaudio-discuss] Stepping down; future thoughts

[David Henningsson]

On 2016-03-28 08:55, Arun Raghavan wrote:

Hi David,

On Mon, 2016-03-28 at 06:36 +0200, David Henningsson wrote:

I've decided to stop working for Canonical, and with that, I intend to
ramp down my contributions to PulseAudio, on both upstream and Ubuntu
levels.

I could have stayed as a volunteer, but in fact I've been a bit
frustrated over some of PulseAudio's design choices for quite some time.
Maybe some of those choices were right at the time when they were made,
but they make us a not good enough fit for tomorrow.


I'm definitely sad to see you step down from contributing to
PulseAudio, but I understand and respect your decision.

If you do decide to come back, or even contribute occasionally, you will of 
course be welcome.


Thanks. I intend to "ramp down" rather than "run away" - even if I won't 
read every email on the PA list, feel free to cc me if there's anything 
that you'd like my opinion on, questions about any code I've written, etc.


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

[pulseaudio-discuss] Stepping down; future thoughts

[David Henningsson]

I've decided to stop working for Canonical, and with that, I intend to 
ramp down my contributions to PulseAudio, on both upstream and Ubuntu 
levels.


I could have stayed as a volunteer, but in fact I've been a bit 
frustrated over some of PulseAudio's design choices for quite some time. 
Maybe some of those choices were right at the time when they were made, 
but they make us a not good enough fit for tomorrow.


From one end comes the embedded ASoC drivers, who have never heard of 
timer-based scheduling, but can reconfigure themselves to decode mp3 in 
hardware.


From the other end come sandboxed apps, and the security requirements 
that come with it.


And routing - we tried, but we never got that right. It's been 
difficult, not only due to all use cases and different demands and ideas 
from different groups of people, but also due to the way PulseAudio is 
built up internally.


In software nothing is impossible, but to re-architecture PulseAudio to 
support all of these requirements in a good way (rather than to "build 
another layer on top", which in the long run would make the PulseAudio 
even more difficult to maintain) would be very difficult, so my judgment 
is that it would be easier to write something new from scratch.


And I do think it would be possible to write something that took the 
best from PulseAudio, JACK, and AudioFlinger, and get something that 
would work well for both mobile and desktop; for pro-audio, gaming, 
low-power music playback, etc. I have been tempted to try; but knowing 
that it would be a huge undertaking, and I don't know any company who 
would like to provide funding for such a project, I'm not likely to get 
very far. Which is a shame, because I think we, as an open source 
community, could have great use for such a sound server.


All the best,
  David

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

Re: [pulseaudio-discuss] [PATCH v3 00/11] Introduce memfd support

[David Henningsson]

On 2016-03-12 23:35, Ahmed S. Darwish wrote:

Hello!

The simplified memfd patch series ;-)


Hi!

Good work!

I've read through the patch series and think they look pretty much okay, 
and your amount of testing is exemplary.


Surely there are nitpicks here and there but at this point I think maybe 
we're better off merging the patch series as it is to get wider testing 
from developers using it in practice.


My only concern or thought is about the global mempool. By turning that 
into a memfd by default, we would potentially slow down clients which 
support posix shm but not memfd. I'm not sure if this is a problem in 
practice though - even an old closed-source client would (hopefully?) 
bind to a new libpulse library and thus gain memfd support that way, and 
I hope nobody links libpulse statically. Thoughts?


Arun, Tanu, should I go ahead and commit this or do you want to review 
it as well?


--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] card-restore: save the database when shutting down

[David Henningsson]

Looks good to me.

Acked-by: David Henningsson 

On 2016-03-04 14:16, Tanu Kaskinen wrote:

If u->save_time_event is non-NULL when the module is being unloaded,
it means that there are some changes to the database that haven't
yet been flushed to the disk.
---
  src/modules/module-card-restore.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/modules/module-card-restore.c 
b/src/modules/module-card-restore.c
index f906843..2660a2b 100644
--- a/src/modules/module-card-restore.c
+++ b/src/modules/module-card-restore.c
@@ -608,8 +608,10 @@ void pa__done(pa_module*m) {
  if (!(u = m->userdata))
  return;

-if (u->save_time_event)
+if (u->save_time_event) {
  u->core->mainloop->time_free(u->save_time_event);
+pa_database_sync(u->database);
+}

  if (u->database)
  pa_database_close(u->database);



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Recording from USB devices without channel mixing

[David Henningsson]

On 2016-03-01 15:44, Tanu Kaskinen wrote:

On Tue, 2016-03-01 at 14:40 +0100, Klaus Jaensch wrote:

Am 01.03.2016 um 11:19 schrieb Tanu Kaskinen:

On Mon, 2016-02-29 at 17:33 +0100, Klaus Jaensch wrote:

I'm wondering why it is the default to mix the channels.

If there's a device with channel map "front-left,front-right", and a
capture stream appears that has channel map "mono", do you wish that
pulseaudio would by default take audio only from the left channel? Why
would that be more likely what the user wants, rather than taking audio
from all channels the device has, or only from the right channel?

Yes. If the user wants only the left channel he has no chance to do so.
(Without changing the configuration.)
You can't split the mixed channels later. The only way is to record
stereo and split the channels later.

If you want a mix of both channels  it is always possible to record
stereo and mix it later.

Another problem with mixing is that the level meter of applications like
audacity show the amplitude of the mix. If you do not know that the
level meter shows only half of the dynamic range (-6dB) if only one
microphone is plugged to the left channel it is likely that you overmodulate
the recording.

And as far as I know it is the default on Windows and Mac OS X to record
only the left channel. I've checked this with some of our USB audio devices.


It could be also argued that if the recording application wants to pick
just one specific channel, it should set the stream channel map
accordingly. But after pondering this for a bit, I think I agree it
would probably good to map mono recording streams only to the first
channel (usually left) of the device by default.


But - now every mono recording of, e g, line in, will only record the 
left channel instead of both.


Also, what about dual-channel internal laptop microphones with high 
background noise? By not mixing the channels, won't your S/N ratio 
decrease by 6 dB?


This sounds like fixing one problem but causing other problems.

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Design constraints for per-client mempools

[David Henningsson]

On 2016-03-01 11:25, Ahmed S. Darwish wrote:

Hi :-)

On Tue, Feb 23, 2016 at 11:45:41AM +0200, Tanu Kaskinen wrote:

On Tue, 2016-02-23 at 11:19 +0200, Tanu Kaskinen wrote:

On Tue, 2016-02-23 at 07:55 +0200, Ahmed S. Danish wrote:

Hello everyone,

As you probably know by now, the second patch in the memfd series
was created to transform the global, shared by all clients,
srbchannel mempool to a "per-client" one. [1]

Reason of this transition is to avoid data leaks between clients.
In a follow-up patch series, it's even hoped that the global core
mempool will also be transformed to the per-client model. [2]


==> Current problem:


By injecting faults at the system-call level, it was discovered
that current design of per-client mempools leads to a reproducible
memory fault on certain error-recovery paths.

The fault is caused by a design issue, rather than a small coding
error.


==> Why are you dedicating a full mail to this?
---

The problematic issue discovered implies that a bigger design
change is required; your input is very much needed :-)


==> What was the current design?


To do the per-client mempool transformation, a simple decision
was taken: let 'pa_native_connection' own the per-client mempool
in question.

Why? Because when a client connects, a 'native connection' object
is created for it by default. Inside this object is the pstream
pipe and other per client resources. So this seemed like the best
place for a per-client mempool:

   /* protocol-native.c - Called for each client connection */
   void pa_native_protocol_connect(...) {
   pa_native_connection *c;

   ...
   c = pa_msgobject_new(pa_native_connection);
   c->pstream = pa_pstream_new(...);
   c->mempool = pa_mempool_new(...);// per-client mempool
   ...
   }

And 'naturally', the pool should be deallocated when the
connection closes:

   /* protocol-native.c */
   native_connection_free(pa_object *o) {
   ...
   pa_pdispatch_unref(c->pdispatch);
   pa_pstream_unref(c->pstream);

   // Srbchannels allocates from this per-client pool,
   // so free it only after the pstream is freed
   pa_mempool_free(c->mempool);
   ...
   }

All looked fine and dandy ..


==> And where is the problem exactly?
-

By injecting failures in sendmsg(2), thus reaching up to the
iochannel and pstream layers, the following leak is shown:

   E: Memory pool destroyed but not all mem blocks freed! 1 remain

and a segfault is then produced due to a use-after-free operation
on the leaked memblock.

The sequence of failure, bottom-up, is as follows:

   -> sendmsg(2) failes -EACCES
 -> pa_iochannel_write_with_creds() return failure
   -> pstream do_write() fails
 -> do_pstream_read_write() fails, jumps to error recovery

And the error recovery goes as this:

   -> pstream die callback (p->callback) is called
   -> That's protocol-native.c pstream_die_callback()
 -> native_connection_unlink// Stops srbchannel, etc.
   -> pa_pstream_unlink // Reset all pstream callbacks
   -> native_connection_free// Per-client mempool _freed!_
   -> pa_pstream_unlink // Second time, does nothing..
   -> pa_pstram_free
 -> pa_queue_free(p->send_queue, item_free)
   -> 1. PACKET item found .. safely removed
   -> 2. MEMBLOCK item found .. from freed srbchannel mempool
 BOOM!!! segfault

SUMMARY : As seen above, a per-client mempool's lifetime must be
a superset of the pstream's lifetime. Putting the per-client pool
in the native_connection object provided only an _illusion_ of
such a superset. [*]

During error recovery, stale memblocks remain in pstrem's send
queue long after __all the per-client objects__ has been removed.

[*] Check native_connection_free() under "What was the current
 design?" for why this illusion was maintained.


==> Suggested solutions?


The situation a is bit tricky.

Where can we put the per-client pool while insuring a correct
lifecycle – especially during error recovery?

The safest option, it seems, is to let the per-client pool be
owned by the pstream object, thus the pool can be freed at the
very end of pstream_free() itself. Is such a solution acceptable?


My first reaction is that why is the pstream object reference counted?



Seems this was just a regular convention rather than a conscious
design decision. This is evidenced by the fact of having only
__two__ pa_pstream_ref() calls in the entire tree. At pstream.c
do_pstream_read_write() and in the same file at srb_callback().
In both places they're just a ref/unref couple done at local
context.


I recently added the one in srb_callba

[pulseaudio-discuss] [PATCH 2/2] alsa-mixer: refactor element_probe

[David Henningsson]

By refactoring volume probing into its own function, we can reduce
indentation a lot. Also, if an error occurs during the volume probe,
that volume element is now always skipped (instead of taking down
the entire path with it).

Signed-off-by: David Henningsson 
---
 src/modules/alsa/alsa-mixer.c | 438 --
 1 file changed, 207 insertions(+), 231 deletions(-)

diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index c0ab1ba..3247835 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -1518,288 +1518,264 @@ static int check_required(pa_alsa_element *e, 
snd_mixer_elem_t *me) {
 return 0;
 }
 
-static int element_probe(pa_alsa_element *e, snd_mixer_t *m) {
-snd_mixer_selem_id_t *sid;
-snd_mixer_elem_t *me;
+static int element_ask_vol_dB(snd_mixer_elem_t *me, int dir, long value, long 
*dBvalue) {
+if (dir == PA_ALSA_DIRECTION_OUTPUT)
+return snd_mixer_selem_ask_playback_vol_dB(me, value, dBvalue);
+else
+return snd_mixer_selem_ask_capture_vol_dB(me, value, dBvalue);
+}
 
-pa_assert(m);
-pa_assert(e);
-pa_assert(e->path);
+static bool element_probe_volume(pa_alsa_element *e, snd_mixer_elem_t *me) {
 
-SELEM_INIT(sid, e->alsa_name);
+long min_dB = 0, max_dB = 0;
+int r;
+bool is_mono;
+pa_channel_position_t p;
 
-if (!(me = snd_mixer_find_selem(m, sid))) {
+if (e->direction == PA_ALSA_DIRECTION_OUTPUT) {
+if (!snd_mixer_selem_has_playback_volume(me)) {
+if (e->direction_try_other && 
snd_mixer_selem_has_capture_volume(me))
+e->direction = PA_ALSA_DIRECTION_INPUT;
+else
+return false;
+}
+} else {
+if (!snd_mixer_selem_has_capture_volume(me)) {
+if (e->direction_try_other && 
snd_mixer_selem_has_playback_volume(me))
+e->direction = PA_ALSA_DIRECTION_OUTPUT;
+else
+return false;
+}
+}
 
-if (e->required != PA_ALSA_REQUIRED_IGNORE)
-return -1;
+e->direction_try_other = false;
 
-e->switch_use = PA_ALSA_SWITCH_IGNORE;
-e->volume_use = PA_ALSA_VOLUME_IGNORE;
-e->enumeration_use = PA_ALSA_ENUMERATION_IGNORE;
+if (e->direction == PA_ALSA_DIRECTION_OUTPUT)
+r = snd_mixer_selem_get_playback_volume_range(me, &e->min_volume, 
&e->max_volume);
+else
+r = snd_mixer_selem_get_capture_volume_range(me, &e->min_volume, 
&e->max_volume);
 
-return 0;
+if (r < 0) {
+pa_log_warn("Failed to get volume range of %s: %s", e->alsa_name, 
pa_alsa_strerror(r));
+return false;
 }
 
-if (e->switch_use != PA_ALSA_SWITCH_IGNORE) {
-if (e->direction == PA_ALSA_DIRECTION_OUTPUT) {
-
-if (!snd_mixer_selem_has_playback_switch(me)) {
-if (e->direction_try_other && 
snd_mixer_selem_has_capture_switch(me))
-e->direction = PA_ALSA_DIRECTION_INPUT;
-else
-e->switch_use = PA_ALSA_SWITCH_IGNORE;
-}
+if (e->min_volume >= e->max_volume) {
+pa_log_warn("Your kernel driver is broken: it reports a volume range 
from %li to %li which makes no sense.",
+e->min_volume, e->max_volume);
+return false;
+}
+if (e->volume_use == PA_ALSA_VOLUME_CONSTANT && (e->min_volume > 
e->constant_volume || e->max_volume < e->constant_volume)) {
+pa_log_warn("Constant volume %li configured for element %s, but the 
available range is from %li to %li.",
+e->constant_volume, e->alsa_name, e->min_volume, 
e->max_volume);
+return false;
+}
 
-} else {
 
-if (!snd_mixer_selem_has_capture_switch(me)) {
-if (e->direction_try_other && 
snd_mixer_selem_has_playback_switch(me))
-e->direction = PA_ALSA_DIRECTION_OUTPUT;
-else
-e->switch_use = PA_ALSA_SWITCH_IGNORE;
-}
-}
+if (e->db_fix && ((e->min_volume > e->db_fix->min_step) || (e->max_volume 
< e->db_fix->max_step))) {
+  pa_log_warn("The step range of the decibel fix for element %s 
(%li-%li) doesn't fit to the "
+  "real hardware range (%li-%li). Disabling the decibel 
fix.", e->alsa_name,
+  e->db_fix->min_step, e->db_fix->max_step, e->min_volume, 
e->max_volume);
 
-if (e->switch_use != PA_ALSA_SWITCH_IGNORE)
-e->direction_try_other = false;
+  decibel_fix_free(e->db_fix);
+  e->db_fix = NULL;
 }
 
-if (e-&

[pulseaudio-discuss] [PATCH 1/2] alsa-mixer: Fix reference to too high channel numbers

[David Henningsson]

The volume_use is set to ignore, but we continue the volume parsing
code, potentially referencing somewhere outside the array (which has
max two channels).

Signed-off-by: David Henningsson 
---
 src/modules/alsa/alsa-mixer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index 1fe2a02..c0ab1ba 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -1768,6 +1768,7 @@ static int element_probe(pa_alsa_element *e, snd_mixer_t 
*m) {
  * channels... */
 pa_log_warn("Volume element %s has %u channels. That's 
too much! I can't handle that!", e->alsa_name, e->n_channels);
 e->volume_use = PA_ALSA_VOLUME_IGNORE;
+e->n_channels = 2;
 }
 
 if (!e->override_map) {
-- 
2.7.0

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

[pulseaudio-discuss] [PATCH 0/2] multichannel element_probe

[David Henningsson]

Looking at errors.ubuntu.com, a new crasher has climbed the charts a bit (at 
least
compared to other ones), starting with PA 7.0. The assertion failure occurs when
trying to set ALSA source volume. ( https://bugs.launchpad.net/bugs/1551610 )

While investigating that one, the only thing remotely related, seemed to be my 
change
w r t multi-channel volumes. Found a bug in that code; the first patch is a 
quick bug fix,
but I'm not at all sure it fixes the actual bug. Nevertheless the plan is to 
deploy it on
Ubuntu and see if the crash numbers go down or not. 

But then I was annoyed by the giant element_probe function being unreadable,
so I refactored it as well. I hope you like the change.

David Henningsson (2):
  alsa-mixer: Fix reference to too high channel numbers
  alsa-mixer: refactor element_probe

 src/modules/alsa/alsa-mixer.c | 437 --
 1 file changed, 207 insertions(+), 230 deletions(-)

-- 
2.7.0

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

Re: [pulseaudio-discuss] Design question: how to store the "preferred ports" of a card

[David Henningsson]

On 2016-02-25 17:18, Tanu Kaskinen wrote:

Hi,

Some time ago I submitted a patch[1] for fixing the HDMI routing
regression[2]. The patch introduces two new variables for cards:
preferred_input_port and preferred_output_port. The variables are
internal to module-switch-on-port-available. It turned out that the
patch doesn't fix the bug properly, because the variables are not saved
on disk, so the user preferences are forgotten on reboot.

Now my question is what approach is preferred for implementing the
variable persistence. I'd like to keep the logic of setting the
variables in module-switch-on-port-available, but I'd prefer not to
create a new file for storing the variables on disk. So, I propose that
module-card-restore will take care of saving and restoring the
variables.

How should module-switch-on-port-available and module-card-restore
communicate with each other? My preference would be to add the
variables to pa_card, and add functions
pa_card_set_preferred_input_port() and
pa_card_set_preferred_output_port(). module-switch-on-port-available
would call those when it wants to change the values, and module-card-
restore would call them when restoring the values from disk. There
would also be new hooks PA_CARD_PREFERRED_INPUT_PORT_CHANGED and the
same for output, which module-card-restore would use to update the on-
disk database.

Another alternative would be to use the card proplist for storing the
variables.

Opinions?
Hmm, is there anything in the above that differs from the strategy used 
for the recently added pa_device_port->preferred_profile?


Adding variables to pa_card seems reasonable to me. Perhaps having the 
same hook for both input and output (and perhaps adding a parameter 
telling which one changed) would save a few lines of code here and 
there? But that's mostly a detail.




[1] https://patchwork.freedesktop.org/patch/74014/
[2] https://bugs.freedesktop.org/show_bug.cgi?id=93946

--
Tanu



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

Re: [pulseaudio-discuss] [PATCH v2 09/12] pulsecore: pstream: Support memfd blocks transport

[David Henningsson]

On 2016-02-20 01:19, Ahmed S. Darwish wrote:

Hi!

On Tue, Feb 16, 2016 at 03:58:02PM +0100, David Henningsson wrote:


On 2016-02-12 01:19, Ahmed S. Darwish wrote:

Now that we have the necessary infrastructure for memexporting and
mempimporting a memfd memblock, extend that support higher up in the
chain with pstreams.

A PulseAudio endpoint can now _transparently_ send a memfd memblock
to the other end by simply calling pa_pstream_send_memblock().


...provided the memfds are first registered.



Yup, will add that.

...

@@ -92,6 +96,9 @@ struct item_info {

  /* packet info */
  pa_packet *packet;
+bool shmid_to_memfd_packet:1;


This seems a bit strange to me. When you set up a srbchannel, there is a
special packet sent (PA_COMMAND_ENABLE_SRBCHANNEL) which has two fds as
ancil data.

Would it not make sense (and lead to less code changes, probably) if this
followed a similar scheme, i e, add a new command (PA_COMMAND_SHMID_FD or
something) that would have an memfd as ancil data and the ID as packet
content?



Hmm, I guess this should've been further commented.

Let's first agree to the following:

- to send a command like PA_COMMAND_ENABLE_SRBCHANNEL, a
   tagstruct is built and sent over the pipe using
   pa_pstream_send_tagstruct_with_fds()

   [ protocol_native.c::setup_srbchannel() ]

- pa_pstream_send_tagstruct_with_fds() transform the tagstruct to
   an opaque packet data using a simple memcpy(), then the packet
   with its ancillary is sent over the domain socket.

   [ pstream-util.c::pa_pstream_send_tagstruct_with_fds() ==>
 pstream-util.c::pa_pstream_send_tagstruct_with_ancil_data()
 ==> pstream.c::pa_pstream_send_packet() /* event deferred */
 ==> pstream.c::do_write()   /* packet write */
 ==> iochannel.c::pa_iochannel_write_with_fds()
 ==> POSIX sendmsg()! ]

So, _on the wire_, both srbchannel enablement and SHMID<->memfd
registration mechanisms are actually the same: the latter just
uses pa_pstream_send_packet() with extra little modifications.

Given the above, why not create a PA_COMMAND_SHMID_FD and save us
some code?

Well, the reason is that we want to close the memfd fd directly
after sending the packet and its ancillary using POSIX sendmsg().
pa_pstream_send_tagstruct_**() does not offer us any ability to
do that :-( It _opaques the message_ then defers the write event
over the pstream.

Due to such opaqueness, after a succesful pstream do_write() I
cannot just say: the packet written was a PA_COMMAND_SMID_FD, so
let's close its associated fds.


It seems quite straight forward just to add another parameter "bool 
close_fds" to pa_pstream_send_tagstruct_with_fds and struct 
pa_cmsg_ancil_data, and in pstream.c::do_write, right after the call to 
pa_iochannel_write_with_fds, you close the fds if the parameter is set?


That way you don't need any "cleanup hook" either.


An argument could then be stated: why not wait for the other PA
endpoint to reply/ACK the command and then close the fds? Well,
this would open us to malicous clients .. leading to an fd leak
per each malicious client connection, and thus bringing the whole
server down after a while.

==

Thus the patch, actually simple, solution was created. Let's mark
the SHM_ID<->memfd registration packet with a special color. Now
in pstream.c::do_write() I can see that this is a registration
packet and call up a simple cleanup hook after write(). The hook
does all the necessary validations and then close the fds.

This way, the fds are always safely closed on the sending side.

[ After writing the above, I've just discovered a bug! If sendmsg
   fails, the cleanup hook should also be called but it is not.
   Will fix that ]



As a side note, we could potentially optimise setting up an srbchannel by
sending all three memfds in PA_COMMAND_ENABLE_SRBCHANNEL, it'll be one less
package to send...



I'm really not a big fan of this.. It  will hardcode both the
number, _and nature_, of mempools in the Pulse communication
protocol itslef. Are you sure we want to do that?


Hey, it was just an idea :-)

Indeed that would mean some additional code, probably not worth it at 
this point. There are just these really quick clients, e g if you do 
"pactl list sinks" or something, and we're adding package after package 
just to set the connection up. But thinking about it, this would 
probably be better fixed if, e g, "pactl" could just disable SHM in the 
first place.




So in the future, if we merge the per-client srbchannel mempool
with the envisioned per-client (earlier global) mempool, we will
have to change the protocol semantics. In the far future if we
reached the optimum case of a single mempool per client (not 3),
we'll also change the protocol semantics.

And in the present, we will have to insert lots of if conditions
and such if the client does not support memf

Re: [pulseaudio-discuss] [PATCH v2 06/12] pulsecore: memexport/memimport: Support memfd blocks

[David Henningsson]

On 2016-02-19 20:28, Ahmed S. Darwish wrote:

On Fri, Feb 19, 2016 at 03:54:06PM +0100, David Henningsson wrote:


On 2016-02-16 22:41, Ahmed S. Darwish wrote:

Hi :-)


...

4. The code above is actually for the PA endpoint creating a
memfd mempool and registering it with the other side. That
is, pa_mempool_take_memfd_fd() is just used when registering
a memfd mempool over the pstream.


Ah, so that's only used on the sending side. Maybe I was just
confused when I wrote the above.

But then I have a follow-up question. In shm_attach, which is on the
receiving side, why do we need to save the fd in pa_shm at all? I
mean, we don't need to send it to anyone else (right?), so all we
need is the memory and mmap keeps that open for us. So, we could
just close the memfd in shm_attach?


Yup, this is what exactly happens. No fd is ever cached on the
receiving side at shm_attch(). Similar point was raised in patch #5
review and a more detailed reply was posted here [*] -- in the 2nd
reply hunk.

Given that this issue is now raised twice, I'm sure the code is
badly unclear, possibly due to the per-type stuff now removed.

Anyway here's how shm_attch() now looks like in v3:

   static int shm_attach(pa_shm *m, pa_mem_type_t type, unsigned
 id, int memfd_fd, ..) {
   ..
   /* In case of attaching to memfd areas, _the caller_
* maintains ownership of the passed fd and has the sole
* responsibility of closing it down.. For other types, we
* are the code path  which created the fd in the first
* place and we're thus the ones responsible for closing it
* down */
   if (type != PA_MEM_TYPE_SHARED_MEMFD)
   pa_assert_se(pa_close(fd) == 0);

   m->type = type;
   m->id = id;
   m->size = (size_t) st.st_size;
   m->do_unlink = false;
   m->fd = -1;
   }

I guess this is now much more clear :-)


Okay, so it's mostly a design decision then? Whether to take ownership 
of the memfd and thus close it down when we're done (and simpler code as 
a result?), or let the caller do the same right after the call to 
shm_attach?



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] Design constraints for per-client mempools

[David Henningsson]

On 2016-02-23 06:55, Ahmed S. Danish wrote:

Hello everyone,

As you probably know by now, the second patch in the memfd series
was created to transform the global, shared by all clients,
srbchannel mempool to a "per-client" one. [1]

Reason of this transition is to avoid data leaks between clients.
In a follow-up patch series, it's even hoped that the global core
mempool will also be transformed to the per-client model. [2]


==> Current problem:


By injecting faults at the system-call level, it was discovered
that current design of per-client mempools leads to a reproducible
memory fault on certain error-recovery paths.

The fault is caused by a design issue, rather than a small coding
error.


==> Why are you dedicating a full mail to this?
---

The problematic issue discovered implies that a bigger design
change is required; your input is very much needed :-)


==> What was the current design?


To do the per-client mempool transformation, a simple decision
was taken: let 'pa_native_connection' own the per-client mempool
in question.

Why? Because when a client connects, a 'native connection' object
is created for it by default. Inside this object is the pstream
pipe and other per client resources. So this seemed like the best
place for a per-client mempool:

   /* protocol-native.c - Called for each client connection */
   void pa_native_protocol_connect(...) {
   pa_native_connection *c;

   ...
   c = pa_msgobject_new(pa_native_connection);
   c->pstream = pa_pstream_new(...);
   c->mempool = pa_mempool_new(...);// per-client mempool
   ...
   }

And 'naturally', the pool should be deallocated when the
connection closes:

   /* protocol-native.c */
   native_connection_free(pa_object *o) {
   ...
   pa_pdispatch_unref(c->pdispatch);
   pa_pstream_unref(c->pstream);

   // Srbchannels allocates from this per-client pool,
   // so free it only after the pstream is freed
   pa_mempool_free(c->mempool);
   ...
   }

All looked fine and dandy ..


==> And where is the problem exactly?
-

By injecting failures in sendmsg(2), thus reaching up to the
iochannel and pstream layers, the following leak is shown:

   E: Memory pool destroyed but not all mem blocks freed! 1 remain

and a segfault is then produced due to a use-after-free operation
on the leaked memblock.

The sequence of failure, bottom-up, is as follows:

   -> sendmsg(2) failes -EACCES
 -> pa_iochannel_write_with_creds() return failure
   -> pstream do_write() fails
 -> do_pstream_read_write() fails, jumps to error recovery

And the error recovery goes as this:

   -> pstream die callback (p->callback) is called
   -> That's protocol-native.c pstream_die_callback()
 -> native_connection_unlink// Stops srbchannel, etc.
   -> pa_pstream_unlink // Reset all pstream callbacks
   -> native_connection_free// Per-client mempool _freed!_
   -> pa_pstream_unlink // Second time, does nothing..
   -> pa_pstram_free
 -> pa_queue_free(p->send_queue, item_free)
   -> 1. PACKET item found .. safely removed
   -> 2. MEMBLOCK item found .. from freed srbchannel mempool
 BOOM!!! segfault

SUMMARY : As seen above, a per-client mempool's lifetime must be
a superset of the pstream's lifetime. Putting the per-client pool
in the native_connection object provided only an _illusion_ of
such a superset. [*]

During error recovery, stale memblocks remain in pstrem's send
queue long after __all the per-client objects__ has been removed.

[*] Check native_connection_free() under "What was the current
 design?" for why this illusion was maintained.


==> Suggested solutions?


The situation a is bit tricky.

Where can we put the per-client pool while insuring a correct
lifecycle – especially during error recovery?

The safest option, it seems, is to let the per-client pool be
owned by the pstream object, thus the pool can be freed at the
very end of pstream_free() itself. Is such a solution acceptable?


Having read your detailed problem description and Tanu's answers, I 
think making mempools reference counted seems like the safest option.


From a quick glance it does not look like mempools own anything 
reference counted (right?), so we shouldn't have a problem with cycles 
if we did that refcount.


--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 06/12] pulsecore: memexport/memimport: Support memfd blocks

[David Henningsson]

On 2016-02-16 22:41, Ahmed S. Darwish wrote:

Hi :-)

On Tue, Feb 16, 2016 at 03:08:03PM +0100, David Henningsson wrote:



On 2016-02-12 01:15, Ahmed S. Darwish wrote:

...

diff --git a/src/pulsecore/memblock.c b/src/pulsecore/memblock.c
index 154bd67..959453e 100644
--- a/src/pulsecore/memblock.c
+++ b/src/pulsecore/memblock.c
@@ -100,6 +100,19 @@ struct pa_memimport_segment {
  pa_memtrap *trap;
  unsigned n_blocks;
  bool writable;
+/* If true, this segment's lifetime will not be limited by the
+ * number of active blocks (n_blocks) using its shared memory.
+ * Rather, it will exist for the full lifetime of the memimport.
+ *
+ * This is done to support SHM memfd blocks transport.
+ *
+ * To transfer memfd-backed blocks without passing their fd every
+ * time, thus minimizing overhead and the possibility of fd leaks,
+ * a packet is sent with the memfd fd as ancil data very early on.
+ * This packet has an ID that identifies the memfd region. Once
+ * received, a permanent mapping is added to the memimport's
+ * segments hash. */
+bool permanent;


Do you need to add this? It looks like you're almost never read this
variable, except for in some assertions - and those assertions could perhaps
be replaced with an "pa_assert(seg->memory.type = MEMFD)" instead?



No problem. Variable is now removed and below macro is used
instead:

static bool segment_is_permanent(pa_memimport_segment *seg) {
 pa_assert(seg);
 return seg->memory.type == PA_MEM_TYPE_SHARED_MEMFD;
}

...

+/* Self-locked
+ *
+ * Check the comments over pa_shm->per_type.memfd.fd for context.
+ *
+ * After this method's return, the caller owns the file descriptor
+ * and is responsible for closing it in the appropriate time. This
+ * should only be called once during during a mempool's lifetime. */
+int pa_mempool_get_and_reset_shm_memfd_fd(pa_mempool *p) {


Nitpick: pa_mempool_take_memfd_fd() is a shorter and more obvious name.



That's a much better name indeed. Now used.



But there's something else I don't get here. A memimport has only one pool,
but more than one segment.

Can some of these segments be memfd and others be posix_shm (e g, one
sandboxed app has the new memfd stuff, and an older client outside the
sandbox uses shm)?
If so, why are you taking the memfd out of the pool, rather than out of the
segment?



Unless I'm missing something obvious, here's why the situation
above cannot hold:

1. A memimport is actually owned by the pstream, and not by any
mempool. This is evidinced by the fact that there's only a
single pa_memimport_new() call in the entire PA code base and
that call resides at:

pa_pstream *pa_pstream_new() {
pa_pstream *p;
...
p->memimport = pa_memimport_new(p->mempool, ...);
}

2. As you know there's only one pstream per client connection.
Thus two clients, one memfd and one posix SHM will lead to
_two_ pstreams, which from (1) will lead to two different
memimports

3. The mempool attached to the memimport is actually for cosmetic
purposes only! A memblock cannot stand naked by its own and
it must be attached to a parent mempool -- thus memfd or posix
SHM _imported_ blocks get attached to the mempool attached to
the memimport.. and that's the only use case for that mempool!

This is evidenced by the fact that the only place a memimport
pool is used is when importing a block in pa_memimport_get():

pa_memblock* pa_memimport_get(pa_memimport *i, pa_mem_type_t
  type, uint32_t block_id, uint32_t shm_id, ...) {
...
b = pa_xnew(pa_memblock, 1);
PA_REFCNT_INIT(b);
b->pool = i->pool;
b->type = PA_MEMBLOCK_IMPORTED;
...
}

So I'm not seeing anything significant here. On the client
side, the memimport's mempool is taken from the pstream which
is the context audio data mempool.

And on the server side the memimport's mempool is also taken
from the pstream which is the pa_core's global mempool (yuk!)

4. The code above is actually for the PA endpoint creating a
memfd mempool and registering it with the other side. That
is, pa_mempool_take_memfd_fd() is just used when registering
a memfd mempool over the pstream.


Ah, so that's only used on the sending side. Maybe I was just confused 
when I wrote the above.


But then I have a follow-up question. In shm_attach, which is on the 
receiving side, why do we need to save the fd in pa_shm at all? I mean, 
we don't need to send it to anyone else (right?), so all we need is the 
memory and mmap keeps that open for us. So, we could just close the 
memfd in shm_attach?




It's basically used to extract the fd from the pool, send a
SHM_ID/memfd mapping to the other end, register the

Re: [pulseaudio-discuss] [PATCH v2 09/12] pulsecore: pstream: Support memfd blocks transport

[David Henningsson]

On 2016-02-12 01:19, Ahmed S. Darwish wrote:

Now that we have the necessary infrastructure for memexporting and
mempimporting a memfd memblock, extend that support higher up in the
chain with pstreams.

A PulseAudio endpoint can now _transparently_ send a memfd memblock
to the other end by simply calling pa_pstream_send_memblock().


...provided the memfds are first registered.



If the pipe does not support memfd trannsfers, we fall back to
sending the full block's data instead of just its reference.

# Further details:

A single pstream connection usually transfers blocks from multiple
pools including the server's srbchannel mempool, the client's audio
data mempool, and the server's shared core mempool.

If these mempools are memfd-backed, we now require registering them
with the pstream before sending any blocks they cover. This is done
to minimize fd passing overhead and the possibility of fd leaks.

Moreover, to support all these pools without hard-coding their number
(or nature) in the Pulse communication protocol, a new pstream packet
type is introduced. That special packet can be sent _anytime_ during
the pstrem's lifetime and is used for creating on demand SHM ID to
memfd mappings.

Signed-off-by: Ahmed S. Darwish 
---
  src/pulsecore/pstream.c | 257 +++-
  src/pulsecore/pstream.h |   2 +
  2 files changed, 233 insertions(+), 26 deletions(-)

diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
index ef2bbf9..11ea7f3 100644
--- a/src/pulsecore/pstream.c
+++ b/src/pulsecore/pstream.c
@@ -38,16 +38,20 @@
  #include 
  #include 
  #include 
+#include 
  #include 

  #include "pstream.h"

  /* We piggyback information if audio data blocks are stored in SHM on the 
seek mode */
  #define PA_FLAG_SHMDATA 0x8000LU
+#define PA_FLAG_SHMDATA_MEMFD_BLOCK 0x2000LU
  #define PA_FLAG_SHMRELEASE  0x4000LU
  #define PA_FLAG_SHMREVOKE   0xC000LU
  #define PA_FLAG_SHMMASK 0xFF00LU
  #define PA_FLAG_SEEKMASK0x00FFLU
+#define PA_FLAG_PACKET_MASK 0x0F00LU
+#define PA_FLAG_PACKET_SHMID_TO_MEMFD_FD0x0100LU
  #define PA_FLAG_SHMWRITABLE 0x0080LU

  /* The sequence descriptor header consists of 5 32bit integers: */
@@ -92,6 +96,9 @@ struct item_info {

  /* packet info */
  pa_packet *packet;
+bool shmid_to_memfd_packet:1;


This seems a bit strange to me. When you set up a srbchannel, there is a 
special packet sent (PA_COMMAND_ENABLE_SRBCHANNEL) which has two fds as 
ancil data.


Would it not make sense (and lead to less code changes, probably) if 
this followed a similar scheme, i e, add a new command 
(PA_COMMAND_SHMID_FD or something) that would have an memfd as ancil 
data and the ID as packet content?


As a side note, we could potentially optimise setting up an srbchannel 
by sending all three memfds in PA_COMMAND_ENABLE_SRBCHANNEL, it'll be 
one less package to send...



+bool close_memfd_fd_after_send:1;
+
  #ifdef HAVE_CREDS
  bool with_ancil_data;
  pa_cmsg_ancil_data ancil_data;
@@ -147,6 +154,10 @@ struct pa_pstream {
  pa_memimport *import;
  pa_memexport *export;

+/* This pipe supports sending memfd fd as ancillary data */
+bool use_memfd;
+pa_hashmap *registered_memfd_ids;


This hashmap could use a comment (maps from what to what? does it own 
any fds that should be closed?).


Also, it seems this hashmap is never freed anywhere...

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 08/12] pulsecore: Specially mark global mempools

[David Henningsson]

I probably would just have added a "bool global" directly to 
pa_mempool_new instead of creating two extra functions. But this is 
probably just a matter of taste. Looks good.


Reviewed-by: David Henningsson 

On 2016-02-12 01:18, Ahmed S. Darwish wrote:

Color global mempools with a special mark. Almost all pools are
now created on a per client basis except the pa_core's mempool,
which is shared between all clients.

This special marking is needed for handling memfd-backed pools.

To avoid fd leaks, memfd pools are registered with the connection
pstream to create an ID<->memfd mapping on both PA endpoints.
Such memory regions are then always referenced by their IDs and
never by their fds, and so their fds can be safely closed later.

Unfortunately this scheme cannot work with global pools since the
registration ID<->memfd mechanism needs to happen for each newly
connected client, and thus the need for a more special handling.

Signed-off-by: Ahmed S. Darwish 
---
  src/pulsecore/core.c |  4 +--
  src/pulsecore/memblock.c | 70 +++-
  src/pulsecore/memblock.h |  3 +++
  src/pulsecore/shm.h  |  6 -
  4 files changed, 79 insertions(+), 4 deletions(-)

diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index 4714932..aab82f3 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -69,14 +69,14 @@ pa_core* pa_core_new(pa_mainloop_api *m, bool shared, 
size_t shm_size) {
  pa_assert(m);

  if (shared) {
-if (!(pool = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, shm_size))) {
+if (!(pool = pa_global_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 
shm_size))) {
  pa_log_warn("Failed to allocate shared memory pool. Falling back to a 
normal memory pool.");
  shared = false;
  }
  }

  if (!shared) {
-if (!(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, shm_size))) {
+if (!(pool = pa_global_mempool_new(PA_MEM_TYPE_PRIVATE, shm_size))) {
  pa_log("pa_mempool_new() failed.");
  return NULL;
  }
diff --git a/src/pulsecore/memblock.c b/src/pulsecore/memblock.c
index af27ba5..9c31e53 100644
--- a/src/pulsecore/memblock.c
+++ b/src/pulsecore/memblock.c
@@ -169,6 +169,29 @@ struct pa_mempool {
  } per_type;
  };

+/* Color global mempools with a special mark.
+ *
+ * Mempools are now created on a per client basis by default. The
+ * only exception is the pa_core's mempool, which is shared between
+ * all clients of the system.
+ *
+ * This special mark is needed for handling memfd pools.
+ *
+ * To avoid fd leaks, memfd pools are registered with the connection
+ * pstream to create an ID<->memfd mapping on both PA endpoints.
+ * Such memory regions are then always referenced by their IDs and
+ * never by their fds, and so their fds can be safely closed later.
+ *
+ * Unfortunately this scheme cannot work with global pools since the
+ * registration ID<->memfd mechanism needs to happen for each newly
+ * connected client, and thus the need for a more special handling.
+ *
+ * TODO: Transform the core mempool into a per-client one and remove
+ * global pools support. They conflict with the futuristic xdg-app
+ * model and complicates handling of memfd-based pools.
+ */
+bool global;
+
  size_t block_size;
  unsigned n_blocks;
  bool is_remote_writable;
@@ -767,7 +790,7 @@ static void memblock_replace_import(pa_memblock *b) {
  pa_mutex_unlock(import->mutex);
  }

-pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size) {
+static pa_mempool *mempool_new(pa_mem_type_t type, size_t size, bool global) {
  pa_mempool *p;
  char t1[PA_BYTES_SNPRINT_MAX], t2[PA_BYTES_SNPRINT_MAX];

@@ -802,6 +825,8 @@ pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size) 
{
   pa_bytes_snprint(t2, sizeof(t2), (unsigned) (p->n_blocks * 
p->block_size)),
   (unsigned long) pa_mempool_block_size_max(p));

+p->global = global;
+
  pa_atomic_store(&p->n_init, 0);

  PA_LLIST_HEAD_INIT(pa_memimport, p->imports);
@@ -819,6 +844,15 @@ mem_create_fail:
  return NULL;
  }

+pa_mempool *pa_mempool_new(pa_mem_type_t type, size_t size) {
+return mempool_new(type, size, false);
+}
+
+/* Check comments on top of pa_mempool.global for details */
+pa_mempool *pa_global_mempool_new(pa_mem_type_t type, size_t size) {
+return mempool_new(type, size, true);
+}
+
  void pa_mempool_free(pa_mempool *p) {
  pa_assert(p);

@@ -953,6 +987,18 @@ bool pa_mempool_is_memfd_backed(const pa_mempool *p) {
  (p->per_type.shm.type == PA_MEM_TYPE_SHARED_MEMFD);
  }

+/* No lock necessary */
+bool pa_mempool_is_global(pa_mempool *p) {
+pa_assert(p);
+
+return p->global;
+}
+
+/* No lock necessary */
+static bool pa_mempoo

Re: [pulseaudio-discuss] [PATCH v2 07/12] pulsecore, tests: Use new memexport/memimport API

[David Henningsson]

No comments on this one.

Reviewed-by: David Henningsson 

On 2016-02-12 01:15, Ahmed S. Darwish wrote:

Signed-off-by: Ahmed S. Darwish 
---
  src/pulsecore/memblock.c  | 24 
  src/pulsecore/memblock.h  |  5 +++--
  src/pulsecore/pstream.c   |  4 
  src/tests/memblock-test.c |  9 +
  4 files changed, 16 insertions(+), 26 deletions(-)

diff --git a/src/pulsecore/memblock.c b/src/pulsecore/memblock.c
index 959453e..af27ba5 100644
--- a/src/pulsecore/memblock.c
+++ b/src/pulsecore/memblock.c
@@ -1116,8 +1116,8 @@ finish:
  }

  /* Self-locked */
-static pa_memblock* NEW_API_pa_memimport_get(pa_memimport *i, pa_mem_type_t 
type, uint32_t block_id, uint32_t shm_id,
- size_t offset, size_t size, bool 
writable) {
+pa_memblock* pa_memimport_get(pa_memimport *i, pa_mem_type_t type, uint32_t 
block_id, uint32_t shm_id,
+  size_t offset, size_t size, bool writable) {
  pa_memblock *b = NULL;
  pa_memimport_segment *seg;

@@ -1177,11 +1177,6 @@ finish:
  return b;
  }

-pa_memblock* pa_memimport_get(pa_memimport *i, uint32_t block_id, uint32_t 
shm_id,
-  size_t offset, size_t size, bool writable) {
-return NEW_API_pa_memimport_get(i, PA_MEM_TYPE_SHARED_POSIX, block_id, 
shm_id, offset, size, writable);
-}
-
  int pa_memimport_process_revoke(pa_memimport *i, uint32_t id) {
  pa_memblock *b;
  int ret = 0;
@@ -1338,8 +1333,8 @@ static pa_memblock *memblock_shared_copy(pa_mempool *p, 
pa_memblock *b) {
  }

  /* Self-locked */
-static int NEW_API_pa_memexport_put(pa_memexport *e, pa_memblock *b, 
pa_mem_type_t *type, uint32_t *block_id,
-uint32_t *shm_id, size_t *offset, size_t * 
size) {
+int pa_memexport_put(pa_memexport *e, pa_memblock *b, pa_mem_type_t *type, 
uint32_t *block_id,
+ uint32_t *shm_id, size_t *offset, size_t * size) {
  pa_shm  *memory;
  struct memexport_slot *slot;
  void *data;
@@ -1402,14 +1397,3 @@ static int NEW_API_pa_memexport_put(pa_memexport *e, 
pa_memblock *b, pa_mem_type

  return 0;
  }
-
-int pa_memexport_put(pa_memexport *e, pa_memblock *b, uint32_t *block_id, 
uint32_t *shm_id,
- size_t *offset, size_t *size) {
-pa_mem_type_t type;
-int ret;
-
-if (!(ret = NEW_API_pa_memexport_put(e, b, &type, block_id, shm_id, 
offset, size)))
-pa_assert(type == PA_MEM_TYPE_SHARED_POSIX);
-
-return ret;
-}
diff --git a/src/pulsecore/memblock.h b/src/pulsecore/memblock.h
index 776b017..059538f 100644
--- a/src/pulsecore/memblock.h
+++ b/src/pulsecore/memblock.h
@@ -140,14 +140,15 @@ pa_memimport* pa_memimport_new(pa_mempool *p, 
pa_memimport_release_cb_t cb, void
  void pa_memimport_free(pa_memimport *i);
  int pa_memimport_add_permanent_shmid_to_memfd_mapping(pa_memimport *i, 
uint32_t shm_id, int memfd_fd,
bool writable);
-pa_memblock* pa_memimport_get(pa_memimport *i, uint32_t block_id, uint32_t 
shm_id,
+pa_memblock* pa_memimport_get(pa_memimport *i, pa_mem_type_t type, uint32_t 
block_id, uint32_t shm_id,
size_t offset, size_t size, bool writable);
  int pa_memimport_process_revoke(pa_memimport *i, uint32_t block_id);

  /* For sending blocks to other nodes */
  pa_memexport* pa_memexport_new(pa_mempool *p, pa_memexport_revoke_cb_t cb, 
void *userdata);
  void pa_memexport_free(pa_memexport *e);
-int pa_memexport_put(pa_memexport *e, pa_memblock *b, uint32_t *block_id, 
uint32_t *shm_id, size_t *offset, size_t *size);
+int pa_memexport_put(pa_memexport *e, pa_memblock *b, pa_mem_type_t *type, 
uint32_t *block_id,
+ uint32_t *shm_id, size_t *offset, size_t * size);
  int pa_memexport_process_release(pa_memexport *e, uint32_t id);

  #endif
diff --git a/src/pulsecore/pstream.c b/src/pulsecore/pstream.c
index 98a8382..ef2bbf9 100644
--- a/src/pulsecore/pstream.c
+++ b/src/pulsecore/pstream.c
@@ -536,6 +536,7 @@ static void prepare_next_write_item(pa_pstream *p) {
  flags = (uint32_t) (p->write.current->seek_mode & PA_FLAG_SEEKMASK);

  if (p->use_shm) {
+pa_mem_type_t type;
  uint32_t block_id, shm_id;
  size_t offset, length;
  uint32_t *shm_info = (uint32_t *) 
&p->write.minibuf[PA_PSTREAM_DESCRIPTOR_SIZE];
@@ -550,10 +551,12 @@ static void prepare_next_write_item(pa_pstream *p) {

  if (pa_memexport_put(current_export,
   p->write.current->chunk.memblock,
+ &type,
   &block_id,
   &shm_id,
   &offset,
   &length) >= 0) {
+pa_assert(type == PA_MEM_TYPE_SHARED_POSIX);

   

Re: [pulseaudio-discuss] [PATCH v2 06/12] pulsecore: memexport/memimport: Support memfd blocks

[David Henningsson]

On 2016-02-12 01:15, Ahmed S. Darwish wrote:

Add support for transfering memfd-backed blocks without passing their
file descriptor every time, thus minimizing overhead and the
possibility of fd leaks.

To accomplish this, a new type of 'permanent' segments is introduced.

Suggested-by: David Henningsson 
Signed-off-by: Ahmed S. Darwish 
---
  src/pulsecore/memblock.c | 134 +++
  src/pulsecore/memblock.h |   7 ++-
  src/pulsecore/shm.c  |   7 +--
  src/pulsecore/shm.h  |   2 +-
  4 files changed, 132 insertions(+), 18 deletions(-)

diff --git a/src/pulsecore/memblock.c b/src/pulsecore/memblock.c
index 154bd67..959453e 100644
--- a/src/pulsecore/memblock.c
+++ b/src/pulsecore/memblock.c
@@ -100,6 +100,19 @@ struct pa_memimport_segment {
  pa_memtrap *trap;
  unsigned n_blocks;
  bool writable;
+/* If true, this segment's lifetime will not be limited by the
+ * number of active blocks (n_blocks) using its shared memory.
+ * Rather, it will exist for the full lifetime of the memimport.
+ *
+ * This is done to support SHM memfd blocks transport.
+ *
+ * To transfer memfd-backed blocks without passing their fd every
+ * time, thus minimizing overhead and the possibility of fd leaks,
+ * a packet is sent with the memfd fd as ancil data very early on.
+ * This packet has an ID that identifies the memfd region. Once
+ * received, a permanent mapping is added to the memimport's
+ * segments hash. */
+bool permanent;


Do you need to add this? It looks like you're almost never read this 
variable, except for in some assertions - and those assertions could 
perhaps be replaced with an "pa_assert(seg->memory.type = MEMFD)" instead?



  };

  /* A collection of multiple segments */
@@ -926,12 +939,46 @@ int pa_mempool_get_shm_id(pa_mempool *p, uint32_t *id) {
  }

  /* No lock necessary */
-bool pa_mempool_is_shared(pa_mempool *p) {
+bool pa_mempool_is_shared(const pa_mempool *p) {
  pa_assert(p);

  return p->shared;
  }

+/* No lock necessary */
+bool pa_mempool_is_memfd_backed(const pa_mempool *p) {
+pa_assert(p);
+
+return pa_mempool_is_shared(p) &&
+(p->per_type.shm.type == PA_MEM_TYPE_SHARED_MEMFD);
+}
+
+/* Self-locked
+ *
+ * Check the comments over pa_shm->per_type.memfd.fd for context.
+ *
+ * After this method's return, the caller owns the file descriptor
+ * and is responsible for closing it in the appropriate time. This
+ * should only be called once during during a mempool's lifetime. */
+int pa_mempool_get_and_reset_shm_memfd_fd(pa_mempool *p) {


Nitpick: pa_mempool_take_memfd_fd() is a shorter and more obvious name.

But there's something else I don't get here. A memimport has only one 
pool, but more than one segment.


Can some of these segments be memfd and others be posix_shm (e g, one 
sandboxed app has the new memfd stuff, and an older client outside the 
sandbox uses shm)?
If so, why are you taking the memfd out of the pool, rather than out of 
the segment?



+pa_shm *memory;
+int memfd_fd;
+
+pa_assert(pa_mempool_is_shared(p));
+pa_assert(pa_mempool_is_memfd_backed(p));
+
+pa_mutex_lock(p->mutex);
+
+memory = &p->per_type.shm;
+memfd_fd = memory->per_type.memfd.fd;
+memory->per_type.memfd.fd = -1;
+
+pa_mutex_unlock(p->mutex);
+
+pa_assert(memfd_fd != -1);
+return memfd_fd;
+}
+
  /* For receiving blocks from other nodes */
  pa_memimport* pa_memimport_new(pa_mempool *p, pa_memimport_release_cb_t cb, 
void *userdata) {
  pa_memimport *i;
@@ -957,15 +1004,17 @@ pa_memimport* pa_memimport_new(pa_mempool *p, 
pa_memimport_release_cb_t cb, void
  static void memexport_revoke_blocks(pa_memexport *e, pa_memimport *i);

  /* Should be called locked */
-static pa_memimport_segment* segment_attach(pa_memimport *i, uint32_t shm_id, 
bool writable) {
+static pa_memimport_segment* segment_attach(pa_memimport *i, pa_mem_type_t 
type, uint32_t shm_id,
+int memfd_fd, bool writable) {
  pa_memimport_segment* seg;
+pa_assert(pa_mem_type_is_shared(type));

  if (pa_hashmap_size(i->segments) >= PA_MEMIMPORT_SEGMENTS_MAX)
  return NULL;

  seg = pa_xnew0(pa_memimport_segment, 1);

-if (pa_shm_attach(&seg->memory, shm_id, writable) < 0) {
+if (pa_shm_attach(&seg->memory, type, shm_id, memfd_fd, writable) < 0) {
  pa_xfree(seg);
  return NULL;
  }
@@ -981,6 +1030,7 @@ static pa_memimport_segment* segment_attach(pa_memimport 
*i, uint32_t shm_id, bo
  /* Should be called locked */
  static void segment_detach(pa_memimport_segment *seg) {
  pa_assert(seg);
+pa_assert(seg->n_blocks == ((seg->permanent) ? 1 : 0));


Not sure, but I wonder if this is a good idea. Could we change this to 

Re: [pulseaudio-discuss] [PATCH v2 05/12] pulsecore: SHM: Introduce memfd support

[David Henningsson]

On 2016-02-13 02:58, Ahmed S. Darwish wrote:

Hi!

On Fri, Feb 12, 2016 at 04:49:23PM +0100, David Henningsson wrote:


On 2016-02-12 01:14, Ahmed S. Darwish wrote:

+/* Do not close file descriptors which are not our own */
+if (type != PA_MEM_TYPE_SHARED_MEMFD)
+pa_assert_se(pa_close(fd) == 0);


I believe the fd should be closed regardless of type?

Now you seem to leak an fd if type == PA_MEM_TYPE_SHARED_MEMFD?



Hmm, that comment should've been further clarified..

By "not close file descriptors which are not our own", I meant
not to close the fd behind our caller's back. It was just passed
to us as a parameter and thus we do not have any kind of
ownership to close it.

This is unlike attaching to a Posix SHM  where the fd was created
by us in the same code path [ using shm_open() ] and thus we have
its complete ownership -- and must close it afterwards.

So for memfd attachment, the caller owns the passed fd and is
responsible for closing it when it sees fit. That's also why we
do not cache the passed fd in the callee and set pa_shm.memfd.fd
to -1.


Right, so if you would have instead written something like:

if (type == PA_MEM_TYPE_SHARED_MEMFD)
m->per_type.memfd.fd = fd;
else
pa_assert_se(pa_close(fd) == 0);

...it would be more obvious that the ownership of the fd is transferred 
into the struct at that point, or closed.


--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 03/12] pulsecore: Transform pa_mempool_new() into a factory method

[David Henningsson]

On 2016-02-12 16:55, Ahmed S. Darwish wrote:

On Fri, Feb 12, 2016 at 03:06:59PM +0100, David Henningsson wrote:

Looks good mostly, just a few nitpicks.

On 2016-02-12 01:10, Ahmed S. Darwish wrote:

...

+
+#include 
+
+#include 
+
+typedef enum pa_mem_type {
+PA_MEM_TYPE_SHARED_POSIX, /* Data is shared and created using 
POSIX shm_open() */
+PA_MEM_TYPE_SHARED_MEMFD, /* Data is shared and created using 
Linux memfd_create() */
+PA_MEM_TYPE_PRIVATE,  /* Data is private and created using 
classic memory allocation (malloc, etc.) */


Actually, it's created using either mmap, posix_memallign, or malloc.



will clarify.


+} pa_mem_type_t;
+
+static inline bool pa_mem_type_is_shared(pa_mem_type_t t) {
+return (t == PA_MEM_TYPE_SHARED_POSIX) || (t == PA_MEM_TYPE_SHARED_MEMFD);
+}
+


Is the reason for having this as an inline function just to avoid a mem.c ?



Is the inline problematic in the first place? pa_mem_type_is_shared()
is inherently just a type-safe macro...


I'm okay with a static inline here. Comment withdrawn. :-)



If it's problematic there's gcc __attribute__((unused)), but
that's even worse.

Thanks,



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH 1/2] modules: Fix compiler warning comparing 0 with bool

[David Henningsson]

Ack

On 2016-02-16 00:15, Peter Meerwald wrote:

modules/module-stream-restore.c: In function 'clean_up_db':
modules/module-stream-restore.c:2344:74: warning: comparison of constant '0' 
with boolean expression is always true [-Wbool-compare]
  pa_assert_se(entry_write(u, item->entry_name, item->entry, true) >= 
0);

reported by Ubuntu gcc-6

Signed-off-by: Peter Meerwald 
---
  src/modules/module-stream-restore.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/modules/module-stream-restore.c 
b/src/modules/module-stream-restore.c
index ac51ff3..37555a8 100644
--- a/src/modules/module-stream-restore.c
+++ b/src/modules/module-stream-restore.c
@@ -2341,7 +2341,7 @@ static void clean_up_db(struct userdata *u) {
  PA_LLIST_FOREACH_SAFE(item, next, to_be_converted) {
  pa_log_debug("Upgrading a legacy entry to the current format: %s", 
item->entry_name);

-pa_assert_se(entry_write(u, item->entry_name, item->entry, true) >= 0);
+pa_assert_se(entry_write(u, item->entry_name, item->entry, true));
  trigger_save(u);

  PA_LLIST_REMOVE(struct clean_up_item, to_be_converted, item);



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 00/12] Introduce memfd support

[David Henningsson]

On 2016-02-12 16:04, Ahmed S. Darwish wrote:

On Fri, Feb 12, 2016 at 11:57:00AM +0100, David Henningsson wrote:


On 2016-02-12 00:59, Ahmed S. Darwish wrote:

Hello!


Hi and thanks for working on it!

Having skimmed through the patches, I'm still not convinced w r t auto-send
mechanism of the memfds.

E g, consider we have clients 1 and 2, and client 1 plays back audio to a
sink. Client 2 monitors the sink-input of client 1 (like parec does if you
specify --monitor-stream). Could this cause the memfd used to communicate
with client 1 to also be shared with client 2? If so, that's a security
breach.

The mechanism seems fragile to such breaches but I'm not sure, maybe I'm
wrong and you can explain what would happen instead?



Thanks for the quick response and patch reviews ;-)

We've just discussed this over IRC, so I'll detail things here
for archival purposes. This might also be useful to add somewhere
in the code or in future changelogs..


Thanks for the explanations! This is a good summary.



- We now have 3 mempools in the system: a global mempool, and 2
   per-client mempools. One created by the client for passing
   playback audio. One created by the server for srbchannels.

- For any per-client memfd mempool, the file descriptors are
   instaneously closed after sending it to other side. The
   receiving end also instantaneously close all received file
   descriptors after doing an mmap().

   Thus we have no risks of data leaks in that case. The 'secret'
   shared by two PA endpoints is directly discarded after use.

- A special case is the global server-wide mempool. Its fd is
   kept open by the server, so whenever a new client connects, it
   passes it the fd for memfd-fd<->SHM-ID negotiation.

   Even in this case, communication is then done using IDs and
   no further FDs are passed. The receiving end also does not
   distinguish between per-client and global mempools and directly
   close the fd after doing an mmap().

- A question then arises: as was done with srbchannels, why not
   transform the global mempool to a per-client one?

   This is planned, but is to be done in a follow-up project. The
   global mempool is touched *everywhere* in the system -- in
   quite different manners and usage scenarioes. It's also used
   by a huge set of modules, including quite esoteric ones.

   Touching this will require extensive testing for each affected
   part. So this will be quite a HUGE patch series of it own,
   possibly done in 10 patches by 10 patches chunks.


Hmm. I'm thinking, to get the security without 100 patches first, we can 
start by not sharing the global mempool with the clients. That way, it 
will fallback to going over the socket. Which might mean an extra 
memcpy, even if that socket is an srbchannel. But still, it would be 
secure. Right?


Then, we can work on cleaning the global mempool up, by fixing modules 
one by one, the commonly used ones (such as the ALSA source modules) 
first. Indeed now we'll have to copy memory to each 
source_output->client mempool instead of to just one global mempool, so 
that will be a change.





   But when it's done, we'll have all the necessary infrastructure
   to directly secure it.

   For now, we can completely disable posix SHM and things should
   function as expected. This is a win.. yes, it's incomplete
   until we personalize the global mempool too, but it's still a
   step in the right direction. The memfd code path will also be
   heavily tested in the process.

Thanks,
Darwish



The all-improved memfd patch series ;-)

==> v1 cover letter:

Memfd is a simple memory sharing mechanism, added by the systemd/kdbus
developers, to share pages between processes in an anonymous, no
global registry needed, no mount-point required, relatively secure,
manner.

This patch series introduces memfd support to PulseAudio, laying out
the necessary (but not yet sufficient) groundwork for sandboxing,
protecting PulseAudio from its clients, and protecting clients from
each other.

Memfd support is added in quite a transparent manner, respecting
current PA mechanisms and abstractions. The lower-level layers are
properly refactored and extended: the srbchannel communication path
is transformed to memfds by only changing a single line of code.

==> v2 changes:

- All pools (srbchannel + client audio data mempool + global mempool)
   now use memfds by default. An empty /dev/shm FTW! ;-)

- Design issues pointed by David now fixed, leading to a much smaller
   code.. New memfd implementation now shares almost all of the
   existing posix SHM code paths.

- Heavy focus on not leaking any file descriptors throughout the
   code. Every fd, sent and received, has a clear owner responsible
   for closing it at appropriate times.

- If a mempool is memfd-backed, we now require registering it with
   the pstream before sending any blocks it covers. This is don

Re: [pulseaudio-discuss] [PATCH v2 05/12] pulsecore: SHM: Introduce memfd support

[David Henningsson]

_shm_cleanup(void) {
  if (pa_atou(de->d_name + SHM_ID_LEN, &id) < 0)
  continue;

-if (shm_attach(&seg, id, false, true) < 0)
+if (shm_attach(&seg, PA_MEM_TYPE_SHARED_POSIX, id, -1, false, true) < 
0)
  continue;

-if (seg.mem.size < SHM_MARKER_SIZE) {
+if (seg.mem.size < shm_marker_size(&seg)) {
  pa_shm_free(&seg);
  continue;
  }

-m = (struct shm_marker*) ((uint8_t*) seg.mem.ptr + seg.mem.size - 
SHM_MARKER_SIZE);
+m = (struct shm_marker*) ((uint8_t*) seg.mem.ptr + seg.mem.size - 
shm_marker_size(&seg));

  if (pa_atomic_load(&m->marker) != SHM_MARKER) {
  pa_shm_free(&seg);
diff --git a/src/pulsecore/shm.h b/src/pulsecore/shm.h
index 887f516..1c8ce83 100644
--- a/src/pulsecore/shm.h
+++ b/src/pulsecore/shm.h
@@ -32,7 +32,21 @@ typedef struct pa_shm {
  pa_mem mem; /* Parent; must be first */
  pa_mem_type_t type;
  unsigned id;
-bool do_unlink:1;
+
+union {
+struct {
+bool do_unlink;
+    } posix_shm;
+struct {
+/* To avoid fd leaks, we keep this fd open only until we pass it
+ * to the other PA endpoint over the unix domain socket.
+ *
+ * When we don't have ownership for the memfd fd in question (e.g.
+ * memfd segment attachment), or the file descriptor has now been
+ * closed, this is set to -1. */
+int fd;
+} memfd;
+} per_type;
  } pa_shm;

  int pa_shm_create(pa_shm *m, pa_mem_type_t type, size_t size, mode_t mode);


Regards,



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 04/12] pulsecore: Split pa_shm mempool backend into pa_shm and pa_privatemem

[David Henningsson]

t;= m->mem.size);

  #ifdef MAP_FAILED
-pa_assert(m->ptr != MAP_FAILED);
+pa_assert(m->mem.ptr != MAP_FAILED);
  #endif

  /* You're welcome to implement this as NOOP on systems that don't
   * support it */

  /* Align the pointer up to multiples of the page size */
-ptr = (uint8_t*) m->ptr + offset;
+ptr = (uint8_t*) m->mem.ptr + offset;
  o = (size_t) ((uint8_t*) ptr - (uint8_t*) PA_PAGE_ALIGN_PTR(ptr));

  if (o > 0) {
@@ -310,22 +267,21 @@ static int shm_attach(pa_shm *m, unsigned id, bool 
writable, bool for_cleanup) {
  }

  if (st.st_size <= 0 ||
-st.st_size > (off_t) (MAX_SHM_SIZE+SHM_MARKER_SIZE) ||
+st.st_size > (off_t) (PA_MAX_SHM_SIZE+SHM_MARKER_SIZE) ||
  PA_ALIGN((size_t) st.st_size) != (size_t) st.st_size) {
  pa_log("Invalid shared memory segment size");
  goto fail;
  }

-m->size = (size_t) st.st_size;
+m->mem.size = (size_t) st.st_size;

  prot = writable ? PROT_READ | PROT_WRITE : PROT_READ;
-if ((m->ptr = mmap(NULL, PA_PAGE_ALIGN(m->size), prot, MAP_SHARED, fd, 
(off_t) 0)) == MAP_FAILED) {
+if ((m->mem.ptr = mmap(NULL, PA_PAGE_ALIGN(m->mem.size), prot, MAP_SHARED, 
fd, (off_t) 0)) == MAP_FAILED) {
  pa_log("mmap() failed: %s", pa_cstrerror(errno));
  goto fail;
  }

  m->do_unlink = false;
-m->shared = true;

  pa_assert_se(pa_close(fd) == 0);

@@ -382,12 +338,12 @@ int pa_shm_cleanup(void) {
  if (shm_attach(&seg, id, false, true) < 0)
  continue;

-if (seg.size < SHM_MARKER_SIZE) {
+if (seg.mem.size < SHM_MARKER_SIZE) {
  pa_shm_free(&seg);
  continue;
  }

-m = (struct shm_marker*) ((uint8_t*) seg.ptr + seg.size - 
SHM_MARKER_SIZE);
+    m = (struct shm_marker*) ((uint8_t*) seg.mem.ptr + seg.mem.size - 
SHM_MARKER_SIZE);

  if (pa_atomic_load(&m->marker) != SHM_MARKER) {
  pa_shm_free(&seg);
diff --git a/src/pulsecore/shm.h b/src/pulsecore/shm.h
index d438961..887f516 100644
--- a/src/pulsecore/shm.h
+++ b/src/pulsecore/shm.h
@@ -23,16 +23,19 @@
  #include 

  #include 
+#include 
+
+/* 1 GiB at max */
+#define PA_MAX_SHM_SIZE (PA_ALIGN(1024*1024*1024))

  typedef struct pa_shm {
+pa_mem mem; /* Parent; must be first */
+pa_mem_type_t type;
  unsigned id;
-void *ptr;
-size_t size;
  bool do_unlink:1;
-bool shared:1;
  } pa_shm;

-int pa_shm_create_rw(pa_shm *m, size_t size, bool shared, mode_t mode);
+int pa_shm_create(pa_shm *m, pa_mem_type_t type, size_t size, mode_t mode);
  int pa_shm_attach(pa_shm *m, unsigned id, bool writable);

  void pa_shm_punch(pa_shm *m, size_t offset, size_t size);

Regards,



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 03/12] pulsecore: Transform pa_mempool_new() into a factory method

[David Henningsson]

less((pool = pa_mempool_new(false, 0)) != NULL, NULL);
+fail_unless((pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0)) != NULL, NULL);

  pa_random(samples0, nsamples * sizeof(int16_t));
  c0.memblock = pa_memblock_new_fixed(pool, samples0, nsamples * 
sizeof(int16_t), false);
diff --git a/src/tests/lfe-filter-test.c b/src/tests/lfe-filter-test.c
index 389a2b9..5b81e70 100644
--- a/src/tests/lfe-filter-test.c
+++ b/src/tests/lfe-filter-test.c
@@ -136,7 +136,7 @@ START_TEST (lfe_filter_test) {
  a.format = PA_SAMPLE_S16NE;

  lft.ss = &a;
-pa_assert_se(lft.pool = pa_mempool_new(false, 0));
+pa_assert_se(lft.pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0));

  /* We prepare pseudo-random input audio samples for lfe-filter rewind 
testing*/
  ori_sample_ptr = pa_xmalloc(pa_frame_size(lft.ss) * TOTAL_SAMPLES);
diff --git a/src/tests/mcalign-test.c b/src/tests/mcalign-test.c
index 0d27dfd..57ac01c 100644
--- a/src/tests/mcalign-test.c
+++ b/src/tests/mcalign-test.c
@@ -36,7 +36,7 @@ int main(int argc, char *argv[]) {
  pa_mcalign *a;
  pa_memchunk c;

-p = pa_mempool_new(false, 0);
+p = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0);

  a = pa_mcalign_new(11);

diff --git a/src/tests/memblock-test.c b/src/tests/memblock-test.c
index 2b51108..58eae7b 100644
--- a/src/tests/memblock-test.c
+++ b/src/tests/memblock-test.c
@@ -80,11 +80,11 @@ START_TEST (memblock_test) {

  const char txt[] = "This is a test!";

-pool_a = pa_mempool_new(true, 0);
+pool_a = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
  fail_unless(pool_a != NULL);
-pool_b = pa_mempool_new(true, 0);
+pool_b = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
  fail_unless(pool_b != NULL);
-pool_c = pa_mempool_new(true, 0);
+pool_c = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
  fail_unless(pool_c != NULL);

  pa_mempool_get_shm_id(pool_a, &id_a);
diff --git a/src/tests/memblockq-test.c b/src/tests/memblockq-test.c
index eea6cfa..f9464db 100644
--- a/src/tests/memblockq-test.c
+++ b/src/tests/memblockq-test.c
@@ -108,7 +108,7 @@ START_TEST (memblockq_test) {

  pa_log_set_level(PA_LOG_DEBUG);

-p = pa_mempool_new(false, 0);
+p = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0);

  silence.memblock = pa_memblock_new_fixed(p, (char*) "__", 2, 1);
  fail_unless(silence.memblock != NULL);
diff --git a/src/tests/mix-test.c b/src/tests/mix-test.c
index c8af600..117ad34 100644
--- a/src/tests/mix-test.c
+++ b/src/tests/mix-test.c
@@ -286,7 +286,7 @@ START_TEST (mix_test) {
  if (!getenv("MAKE_CHECK"))
  pa_log_set_level(PA_LOG_DEBUG);

-fail_unless((pool = pa_mempool_new(false, 0)) != NULL, NULL);
+fail_unless((pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0)) != NULL, NULL);

  a.channels = 1;
  a.rate = 44100;
diff --git a/src/tests/remix-test.c b/src/tests/remix-test.c
index 6feb8dc..21e5f48 100644
--- a/src/tests/remix-test.c
+++ b/src/tests/remix-test.c
@@ -51,7 +51,7 @@ int main(int argc, char *argv[]) {

  pa_log_set_level(PA_LOG_DEBUG);

-pa_assert_se(pool = pa_mempool_new(false, 0));
+pa_assert_se(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0));

  for (i = 0; maps[i].channels > 0; i++)
  for (j = 0; maps[j].channels > 0; j++) {
diff --git a/src/tests/resampler-test.c b/src/tests/resampler-test.c
index 9832a31..4796353 100644
--- a/src/tests/resampler-test.c
+++ b/src/tests/resampler-test.c
@@ -404,7 +404,7 @@ int main(int argc, char *argv[]) {
  }

  ret = 0;
-pa_assert_se(pool = pa_mempool_new(false, 0));
+pa_assert_se(pool = pa_mempool_new(PA_MEM_TYPE_PRIVATE, 0));

  if (!all_formats) {

diff --git a/src/tests/srbchannel-test.c b/src/tests/srbchannel-test.c
index cd4d397..59ce1ed 100644
--- a/src/tests/srbchannel-test.c
+++ b/src/tests/srbchannel-test.c
@@ -85,7 +85,7 @@ START_TEST (srbchannel_test) {
  int pipefd[4];

  pa_mainloop *ml = pa_mainloop_new();
-pa_mempool *mp = pa_mempool_new(true, 0);
+pa_mempool *mp = pa_mempool_new(PA_MEM_TYPE_SHARED_POSIX, 0);
  pa_iochannel *io1, *io2;
  pa_pstream *p1, *p2;
  pa_srbchannel *sr1, *sr2;

Regards,



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 02/12] pulsecore: srbchannel: Introduce per-client SHM files

[David Henningsson]

What if c->rw_mempool is already set at this point? Can that happen? If 
not, add an assertion, if it can (consider a malicious client) ...then 
handle it somehow :-)



+if (!(c->rw_mempool = pa_mempool_new(true, c->protocol->core->shm_size))) {
+pa_log_warn("Disabling srbchannel, reason: Failed to allocate shared "
+"writable memory pool.");
  return;
  }
+pa_mempool_set_is_remote_writable(c->rw_mempool, true);

-srb = pa_srbchannel_new(c->protocol->core->mainloop, 
c->protocol->core->rw_mempool);
+srb = pa_srbchannel_new(c->protocol->core->mainloop, c->rw_mempool);
  if (!srb) {
  pa_log_debug("Failed to create srbchannel");
-return;
+goto free_rw_mempool;


If this is the only place you need goto, just instead do

pa_mempool_free(c->rw_mempool);
c->rw_mempool = NULL;
return;


  }
  pa_log_debug("Enabling srbchannel...");
  pa_srbchannel_export(srb, &srbt);
@@ -2634,6 +2648,10 @@ static void setup_srbchannel(pa_native_connection *c) {
  pa_pstream_send_memblock(c->pstream, 0, 0, 0, &mc);

  c->srbpending = srb;
+return;
+
+free_rw_mempool:
+pa_mempool_free(c->rw_mempool);
  }

  static void command_enable_srbchannel(pa_pdispatch *pd, uint32_t command, 
uint32_t tag, pa_tagstruct *t, void *userdata) {
@@ -5125,6 +5143,8 @@ void pa_native_protocol_connect(pa_native_protocol *p, 
pa_iochannel *io, pa_nati
  c->client->send_event = client_send_event_cb;
  c->client->userdata = c;

+c->rw_mempool = NULL;


A preferred way nowadays is to instead use pa_xnew0 when creating the 
struct initially.



+
  c->pstream = pa_pstream_new(p->core->mainloop, io, p->core->mempool);
  pa_pstream_set_receive_packet_callback(c->pstream, 
pstream_packet_callback, c);
  pa_pstream_set_receive_memblock_callback(c->pstream, 
pstream_memblock_callback, c);


Regards,



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 11/12] pulse: client audio: Use memfd transport by default

[David Henningsson]

On 2016-02-12 01:20, Ahmed S. Darwish wrote:

Now that all layers in the stack support memfd blocks, use memfd
pools for client context and audio data by default.

Signed-off-by: Ahmed S. Darwish 
---
  man/pulse-client.conf.5.xml.in |  5 +
  src/pulse/client-conf.c|  1 +
  src/pulse/client-conf.h|  2 +-
  src/pulse/context.c| 11 ++-
  4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/man/pulse-client.conf.5.xml.in b/man/pulse-client.conf.5.xml.in
index cca2219..a04dedc 100644
--- a/man/pulse-client.conf.5.xml.in
+++ b/man/pulse-client.conf.5.xml.in
@@ -107,6 +107,11 @@ License along with PulseAudio; if not, see 
<http://www.gnu.org/licenses/>.
  

  
+  disable-memfd=. Disable memfd shared memory. Takes
+  a boolean argument, defaults to no.


Good idea.


+
+
+
shm-size-bytes= Sets the shared memory segment
size for clients, in bytes. If left unspecified or is set to 0
it will default to some system-specific default, usually 64
diff --git a/src/pulse/client-conf.c b/src/pulse/client-conf.c
index c23aa6b..58259e4 100644
--- a/src/pulse/client-conf.c
+++ b/src/pulse/client-conf.c
@@ -141,6 +141,7 @@ void pa_client_conf_load(pa_client_conf *c, bool 
load_from_x11, bool load_from_e
  { "cookie-file",pa_config_parse_string,   
&c->cookie_file_from_client_conf, NULL },
  { "disable-shm",pa_config_parse_bool, 
&c->disable_shm, NULL },
  { "enable-shm", pa_config_parse_not_bool, 
&c->disable_shm, NULL },
+{ "disable-memfd",  pa_config_parse_bool, 
&c->disable_memfd, NULL },
  { "shm-size-bytes", pa_config_parse_size, &c->shm_size, 
NULL },
  { "auto-connect-localhost", pa_config_parse_bool, 
&c->auto_connect_localhost, NULL },
  { "auto-connect-display",   pa_config_parse_bool, 
&c->auto_connect_display, NULL },
diff --git a/src/pulse/client-conf.h b/src/pulse/client-conf.h
index eac705a..7691ec7 100644
--- a/src/pulse/client-conf.h
+++ b/src/pulse/client-conf.h
@@ -37,7 +37,7 @@ typedef struct pa_client_conf {
  bool cookie_from_x11_valid;
  char *cookie_file_from_application;
  char *cookie_file_from_client_conf;
-bool autospawn, disable_shm, auto_connect_localhost, auto_connect_display;
+bool autospawn, disable_shm, disable_memfd, auto_connect_localhost, 
auto_connect_display;
  size_t shm_size;
  } pa_client_conf;

diff --git a/src/pulse/context.c b/src/pulse/context.c
index 204c3e5..c31dcf4 100644
--- a/src/pulse/context.c
+++ b/src/pulse/context.c
@@ -171,7 +171,10 @@ pa_context *pa_context_new_with_proplist(pa_mainloop_api 
*mainloop, const char *
  c->srb_template.readfd = -1;
  c->srb_template.writefd = -1;

-type = !c->conf->disable_shm ? PA_MEM_TYPE_SHARED_POSIX : 
PA_MEM_TYPE_PRIVATE;
+type = (c->conf->disable_shm) ? PA_MEM_TYPE_PRIVATE :
+   ((c->conf->disable_memfd || !pa_memfd_is_locally_supported()) ?
+   PA_MEM_TYPE_SHARED_POSIX : PA_MEM_TYPE_SHARED_MEMFD);
+
  if (!(c->mempool = pa_mempool_new(type, c->conf->shm_size))) {

  if (!c->conf->disable_shm) {
@@ -464,6 +467,7 @@ int pa_context_handle_error(pa_context *c, uint32_t 
command, pa_tagstruct *t, bo

  static void setup_complete_callback(pa_pdispatch *pd, uint32_t command, 
uint32_t tag, pa_tagstruct *t, void *userdata) {
  pa_context *c = userdata;
+const char *reason;

  pa_assert(pd);
  pa_assert(c);
@@ -536,7 +540,12 @@ static void setup_complete_callback(pa_pdispatch *pd, 
uint32_t command, uint32_t
  c->shm_type = PA_MEM_TYPE_PRIVATE;
  if (c->do_shm) {
  if (c->version >= 31 && memfd_on_remote && 
pa_memfd_is_locally_supported()) {
+
  pa_pstream_enable_memfd(c->pstream);
+if (pa_mempool_is_memfd_backed(c->mempool))
+if (pa_pstream_register_memfd_mempool(c->pstream, 
c->mempool, &reason))
+pa_log("Failed regestering memfd mempool. Reason: 
%s", reason);


Nitpicks: "Failed to register memfd mempool". "const char* reason can 
move to just above "pa_pstream_enable_memfd".



+
  c->shm_type = PA_MEM_TYPE_SHARED_MEMFD;


Is it correct to still have c->shm_type = PA_MEM_TYPE_SHARED_MEMFD even 
if registering the memfd failed? I think it is, but maybe this can be 
indicated better by adding a comment stating that.



  } else
  c->shm_type = PA_MEM_TYPE_SHARED_POSIX;

Regards,



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 01/12] pulsecore: Cache daemon shm size inside pa_core

[David Henningsson]

Ack, looks good.

On 2016-02-12 01:08, Ahmed S. Darwish wrote:

The daemon `shm-size-bytes' configuration value was read, and then
directly used, for creating the initial server-wide SHM files.

This is fine for now, but soon, such server-wide SHMs will be replaced
with per-client SHM files that will be dynamically created and deleted
according to clients open and close. Thus, appropriately cache this
configuration value.

Signed-off-by: Ahmed S. Darwish 
---
  src/pulsecore/core.c | 1 +
  src/pulsecore/core.h | 5 +
  2 files changed, 6 insertions(+)

diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index 43fd30e..b2df7de 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -123,6 +123,7 @@ pa_core* pa_core_new(pa_mainloop_api *m, bool shared, 
size_t shm_size) {
  c->subscription_event_last = NULL;

  c->mempool = pool;
+c->shm_size = shm_size;
  pa_silence_cache_init(&c->silence_cache);

  if (shared && !(c->rw_mempool = pa_mempool_new(shared, shm_size)))
diff --git a/src/pulsecore/core.h b/src/pulsecore/core.h
index aefc1eb..428689c 100644
--- a/src/pulsecore/core.h
+++ b/src/pulsecore/core.h
@@ -181,6 +181,11 @@ struct pa_core {
 The rw_mempool is used for data writable by both server and client (and
 can be NULL in some cases). */
  pa_mempool *mempool, *rw_mempool;
+
+/* Shared memory size, as specified either by daemon configuration
+ * or PA daemon defaults (~ 64 MiB). */
+size_t shm_size;
+
  pa_silence_cache silence_cache;

  pa_time_event *exit_event;


Regards,



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 00/12] Introduce memfd support

[David Henningsson]

  2 +-
  src/tests/lfe-filter-test.c  |   2 +-
  src/tests/mcalign-test.c |   2 +-
  src/tests/memblock-test.c|  15 +--
  src/tests/memblockq-test.c   |   2 +-
  src/tests/mix-test.c |   2 +-
  src/tests/remix-test.c   |   2 +-
  src/tests/resampler-test.c   |   2 +-
  src/tests/srbchannel-test.c  |   2 +-
  38 files changed, 1119 insertions(+), 237 deletions(-)
  create mode 100644 src/pulsecore/mem.h
  create mode 100644 src/pulsecore/memfd-wrappers.h
  create mode 100644 src/pulsecore/privatemem.c
  create mode 100644 src/pulsecore/privatemem.h

Regards,

--
Darwish
http://darwish.chasingpointers.com



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] RFC: Switch to HDMI or not?

[David Henningsson]

On 2016-02-04 13:54, David Henningsson wrote:



On 2016-02-04 12:50, Tanu Kaskinen wrote:

On Thu, 2016-02-04 at 11:35 +0100, David Henningsson wrote:


On 2016-02-04 08:47, Alexander E. Patrakov wrote:

04.02.2016 10:45, David Henningsson пишет:

   6b) It seems non-trivial, and I have a gut feeling it will break
some
other use case, that neither of us is thinking about right now.


Based on our past experience here, I agree.


This same point can equally well be made for David's proposal, though.


Indeed. I was thinking my risk was lower because it only dealt with 
HDMI. But maybe that was wrong...


Anyhow; your proposal involves trying to save the last "user selected" 
port; how do you determine if a port is user selected or not? (If the 
port the user wants to select is already set just by selecting profile, 
then there is no point for the GUI application to call pa_sink_set_port.)





   6c) I realize neither of 6a) or 6b) are particularly strong
arguments
against actually fixing a problem...


I think here you meant "trying to fix".

I also think the real problem here is to convince others that you have
indeed fixed the original problem :) so for me 6b is strong enough.



Internal speakers toggle between "no" and "unknown" today; i e, when you
unplug your headphones, internal speakers go to "unknown" rather than
"yes". This somewhat indicates that speakers are now available, but you
did not make an active choice to use them.


Here's a plausible use case that will break in a very annoying way with
your proposal and doesn't break with my proposal: let's say that the
user prefers to use headphones when they are plugged in, and the
internal speakers when the headphones are not plugged in, and never the
HDMI output. Now whenever headphones are disconnected, the routing
logic that you propose will choose the HDMI output, because it has
higher priority than the internal speakers. With my proposal the system
remembers that the internal speakers are the preferred output port of
the sound card (which is actually a misrepresentation of the user's
intent, because the user prefers the headphones over the internal
speakers, but luckily that issue is masked by the speakers becoming
unavailable when the headphones are plugged in).


Okay, that makes sense. Scratch my proposal.


On second thought; what if the module were to, in addition to setting 
"unknown" instead of "yes", also would set its priority to 1?



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] RFC: Switch to HDMI or not?

[David Henningsson]

On 2016-02-04 12:50, Tanu Kaskinen wrote:

On Thu, 2016-02-04 at 11:35 +0100, David Henningsson wrote:


On 2016-02-04 08:47, Alexander E. Patrakov wrote:

04.02.2016 10:45, David Henningsson пишет:

   6b) It seems non-trivial, and I have a gut feeling it will break some
other use case, that neither of us is thinking about right now.


Based on our past experience here, I agree.


This same point can equally well be made for David's proposal, though.


   6c) I realize neither of 6a) or 6b) are particularly strong arguments
against actually fixing a problem...


I think here you meant "trying to fix".

I also think the real problem here is to convince others that you have
indeed fixed the original problem :) so for me 6b is strong enough.



Internal speakers toggle between "no" and "unknown" today; i e, when you
unplug your headphones, internal speakers go to "unknown" rather than
"yes". This somewhat indicates that speakers are now available, but you
did not make an active choice to use them.


Here's a plausible use case that will break in a very annoying way with
your proposal and doesn't break with my proposal: let's say that the
user prefers to use headphones when they are plugged in, and the
internal speakers when the headphones are not plugged in, and never the
HDMI output. Now whenever headphones are disconnected, the routing
logic that you propose will choose the HDMI output, because it has
higher priority than the internal speakers. With my proposal the system
remembers that the internal speakers are the preferred output port of
the sound card (which is actually a misrepresentation of the user's
intent, because the user prefers the headphones over the internal
speakers, but luckily that issue is masked by the speakers becoming
unavailable when the headphones are plugged in).


Okay, that makes sense. Scratch my proposal.

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] RFC: Switch to HDMI or not?

[David Henningsson]

On 2016-02-04 08:47, Alexander E. Patrakov wrote:

04.02.2016 10:45, David Henningsson пишет:

So, there has been a few bugs related to the new routing, in particular
[1] which while related to whether or not the actual system has an
internal speaker or not, also highlights the use case of letting an HDMI
monitor go to powersave. This will, starting from 8.0, cause sound to be
rerouted to internal speakers, even after the HDMI monitor comes back
online.

Here's a recap.

  1) The old situation of playing back audio to an unconnected monitor
(or powersaved monitor) was not user friendly.


Partially agree. As always with old bugs, there may be people for whom
they are features.


  2) There is no way (AFAIK) we can distinguish between a monitor being
in powersave mode, or permanently unconnected.


No opinion here, ask the graphics guys.


  3) Hence, the proper solution is to switch to analog when the monitor
is off/powersaved/unconnected and then switch back when it comes back
online.


Well, here I also don't 100% agree. You are focusing on the "what
happens when the monitor comes back from powersaving mode" side of
things - which is too late and too passive. My opinion, as a user, is:

1. If audio is playing on HDMI (even if this is a music file), it is a
mistake to apply DPMS. Black out the screen if needed for screensaver -
maybe, DPMS - no. Personally, I have disabled DPMS on my desktop PC, but
this is also due to a different bug, unrelated to audio.

2. An attempt to play audio to a DPMSed monitor should try to wake it
up. I understand that we lack an infrastructure to do this. But,
ultimately, only if that fails or times out, audio should be moved to
analog (if moved at all).

I understand that this is better discussed on something like dri-devel,
but I have no concrete-enough proposal to actually present there.


Right, this is a very valid point. We could instead try to fix graphics 
driver(s) to make sure

 a) monitors appear connected when they are in powersave
 b) to not make them go to powersave when an audio stream is currently 
playing back

 c) to wake them up from powersave when an audio stream is started.

We also do not know (or at least I don't) whether some graphics drivers 
already work this way, but the person in the bug seems to use standard 
Ivybridge graphics, so that probably means that a fair share of existing 
HDMI ports appear disconnected when they are in DPMS.


So, while this seems to be an interesting route forward (especially 
given the i915 component framework which can transfer ELD and PD bits 
without taking a hardware detour), we still have a problem here and now.





  4) The easiest way to do that is, at least after the priority patch
[2] has been applied, to adjust the port priorities so that HDMI ports
have a higher priority than analog-speakers. This is also consistent
with a suggestion from GNOME [3]. However, doing so might instead upset
people who would like to stay on analog-speakers when their HDMI monitor
comes back online.


Let's see whether such people exist.

But I have another class of upset people: those whose monitor supports
HDMI audio and has only headphones output, which is unused (but we can't
know that it is unused, i.e. that the monitor is in fact a glorified
null sink). Which is exactly what the proposal at the end of your email
addresses (better accounting of HDMI port availability).


Indeed. In the best of worlds, the HDMI port availability should be the 
same as whether those headphones are plugged in or not, but I doubt this 
works.




And also the case of two audio-capable monitors. When the graphical
session power-saves them "at the same time", there is a race. Suppose
that the user wants his audio on HDMI2, but there is also HDMI1. So, if
HDMI1 is slower to respond to DPMS, the audio could be rerouted this
way: HDMI2 -> HDMI1 (temporarily) -> analog. When the monitors come back
up: analog -> HDMI2, but then HDMI1 (with a higher priority) comes up.
This is also addressed by your proposal.


  5) We could then tell those people that they should manually increase
the priority or analog-speakers in our configuration files, but it could
be argued that this is not user friendly enough either.


It is indeed not user friendly. But we don't have a 100% user-friendly
solution, because mind-reading technology is not quite there yet. So
some user interaction is definitely needed.


We can also debate whether mind-reading technology in itself is user 
friendly, but that's hardly on topic here :-)





  6) Tanu suggested in [1] to add functionality to remember the latest
user-selected port and/or profile to module-switch-on-port-available.
I'm hesitant to that solution, because


...


  6a) It breaks the main idea of module-switch-on-port-available being
strictly rule/priority-based, leaving the "remember" feature to the not
yet finished module-port-manage

[pulseaudio-discuss] RFC: Switch to HDMI or not?

[David Henningsson]

So, there has been a few bugs related to the new routing, in particular 
[1] which while related to whether or not the actual system has an 
internal speaker or not, also highlights the use case of letting an HDMI 
monitor go to powersave. This will, starting from 8.0, cause sound to be 
rerouted to internal speakers, even after the HDMI monitor comes back 
online.


Here's a recap.

 1) The old situation of playing back audio to an unconnected monitor 
(or powersaved monitor) was not user friendly.


 2) There is no way (AFAIK) we can distinguish between a monitor being 
in powersave mode, or permanently unconnected.


 3) Hence, the proper solution is to switch to analog when the monitor 
is off/powersaved/unconnected and then switch back when it comes back 
online.


 4) The easiest way to do that is, at least after the priority patch 
[2] has been applied, to adjust the port priorities so that HDMI ports 
have a higher priority than analog-speakers. This is also consistent 
with a suggestion from GNOME [3]. However, doing so might instead upset 
people who would like to stay on analog-speakers when their HDMI monitor 
comes back online.


 5) We could then tell those people that they should manually increase 
the priority or analog-speakers in our configuration files, but it could 
be argued that this is not user friendly enough either.


 6) Tanu suggested in [1] to add functionality to remember the latest 
user-selected port and/or profile to module-switch-on-port-available. 
I'm hesitant to that solution, because


 6a) It breaks the main idea of module-switch-on-port-available being 
strictly rule/priority-based, leaving the "remember" feature to the not 
yet finished module-port-manager.


 6b) It seems non-trivial, and I have a gut feeling it will break some 
other use case, that neither of us is thinking about right now.


 6c) I realize neither of 6a) or 6b) are particularly strong arguments 
against actually fixing a problem...


 7) So, this morning I came up with another idea. And this is the RFC 
part of this email. We're not sure whether or not a just 
connected/online monitor has "usable" HDMI audio or not. Hence, maybe 
plugging in a monitor should lead to the HDMI port availability being 
"unknown" rather than "yes"? module-switch-on-port-available will not 
route to something that changes availability from "no" to "unknown".


For this to make sense, we need to add yet another module, which 
determines whether an HDMI port should be "unknown" or "yes" when coming 
back online. If a user manually switches to the HDMI profile then that 
means "yes" from now on, if a user manually switches away then that 
means "unknown" from now on. As a bonus, we could potentially use ELD 
info as a key to a database of "yes" and "unknown" values (this could be 
useful for module-port-manager as well). I'm not sure whether the key 
should be "sink+port", "ELD" or "sink+port+ELD", but probably 
"sink+port+ELD" is the best one considering people with dual monitors. 
What do you think?


 8) Everything said about HDMI applies to DisplayPort as well.


[1] https://bugs.freedesktop.org/show_bug.cgi?id=93946

[2] http://patchwork.freedesktop.org/patch/72053/

[3] https://wiki.gnome.org/Design/SystemSettings/Sound - section 
"Guidelines"


--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] switch-on-port-available: Switch from HDMI to analog; but not the other way around

[David Henningsson]

On 2016-02-01 02:40, Raymond Yau wrote:


 > > If you have headphones plugged in and plug in HDMI; you want sound
 > > to stay on headphones.
 > > If you have HDMI plugged in and you plug in headphones; you want sound
 > > to switch to headphones.
 > >
 > > Hence we need to take priority into account as well when determining
 > > whether to switch to a new profile or not.
 >
 > I think the same logic should apply to input ports too. Otherwise looks
 > good to me.
 >

https://bugs.freedesktop.org/show_bug.cgi?id=93903

But the bug report is about headphone switch to optical digital out and
not switch back, usually hdmi and analog codecs attached to different
hda controller (alsa card)


On Skylake, Braswell and BayTrail they are both on the same controller. 
And some older chips as well, IIRC, but I've forgotten which ones.




what happen when the three input jacks of desktop are not plugged ?

Some hda codecs (via) still have stereo mix as input source but the
other codecs don't



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH] switch-on-port-available: Switch from HDMI to analog; but not the other way around

[David Henningsson]

If you have headphones plugged in and plug in HDMI; you want sound
to stay on headphones.
If you have HDMI plugged in and you plug in headphones; you want sound
to switch to headphones.

Hence we need to take priority into account as well when determining
whether to switch to a new profile or not.

BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=93903
Signed-off-by: David Henningsson 
---

Please wait for it to be tested before committing it. Thanks.

 src/modules/module-switch-on-port-available.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/modules/module-switch-on-port-available.c 
b/src/modules/module-switch-on-port-available.c
index 5dd9786..6f4c895 100644
--- a/src/modules/module-switch-on-port-available.c
+++ b/src/modules/module-switch-on-port-available.c
@@ -29,7 +29,7 @@
 
 #include "module-switch-on-port-available-symdef.h"
 
-static bool profile_good_for_output(pa_card_profile *profile) {
+static bool profile_good_for_output(pa_card_profile *profile, unsigned prio) {
 pa_sink *sink;
 uint32_t idx;
 
@@ -49,7 +49,7 @@ static bool profile_good_for_output(pa_card_profile *profile) 
{
 if (!sink->active_port)
 continue;
 
-if (sink->active_port->available != PA_AVAILABLE_NO)
+if ((sink->active_port->available != PA_AVAILABLE_NO) && 
(sink->active_port->priority >= prio))
 return false;
 }
 
@@ -88,7 +88,7 @@ static int try_to_switch_profile(pa_device_port *port) {
 switch (port->direction) {
 case PA_DIRECTION_OUTPUT:
 name = profile->output_name;
-good = profile_good_for_output(profile);
+good = profile_good_for_output(profile, port->priority);
 break;
 
 case PA_DIRECTION_INPUT:
-- 
1.9.1

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

[pulseaudio-discuss] Fwd: [PATCH] rtkit: Fail with explanation on grsec

[David Henningsson]

Hi Lennart, here's an rtkit patch for you.

Hi Stanisław, I'm forwarding this to Lennart as I don't think any of the 
current PulseAudio developers have access to the rtkit repository.


// David
--- Begin Message ---
In case of grsec kernel, make sure that we can actually see other
processes.  If the system is restricted via "chroot_findtask" sysctl or
CONFIG_GRKERNSEC_PROC, rtkit won't be able to monitor other processes,
but the error message is just a generic "Operation not permitted".

Since this prevents rtkit from working at all, just fail fast and loud
at startup instead.
---
 rtkit-daemon.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/rtkit-daemon.c b/rtkit-daemon.c
index 3ecc1f7..294736c 100644
--- a/rtkit-daemon.c
+++ b/rtkit-daemon.c
@@ -1759,6 +1759,7 @@ static int drop_privileges(void) {
 }
 
 if (do_chroot) {
+FILE* init_stat;
 
 /* Second, chroot() */
 if (chroot("/proc") < 0 ||
@@ -1769,6 +1770,14 @@ static int drop_privileges(void) {
 }
 proc = "/";
 
+init_stat = fopen("/1/stat", "r");
+if (init_stat == NULL) {
+r = -errno;
+syslog(LOG_ERR, "Cannot see other processes in chroot. 
Check 'chroot_findtask' if using grsec, or use --no-chroot.\n");
+return r;
+}
+fclose(init_stat);
+
 syslog(LOG_DEBUG, "Successfully called chroot.\n");
 }
 
-- 
2.7.0

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

Re: [pulseaudio-discuss] Release notes draft completed

[David Henningsson]

On 2016-01-13 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.


I've added a section about the routing changes, as well as a few words 
about PULSE_LOG_JOURNAL and module-dbus-protocol.


I was unsure of where to best add those words though, so feel free to 
review and/or reorder as you see fit.


--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
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()

[David Henningsson]

I like Arun's version better, but I'm not too fuzzed.

On 2016-01-12 04:05, 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



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
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

[David Henningsson]

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?


On 2015-12-21 04:10, Kamil Rytarowski wrote:

Patch from pkgsrc by Jonathan Perkin (Joyent).
---
  src/modules/module-solaris.c | 12 +---
  1 file changed, 9 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);
  }



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
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

[David Henningsson]

Hi!

The module-bluez5-discover.c one is correct, but the 
module-bluetooth-policy.c does not go to the fail label in case of 
modargs failure, so there is no need for the NULL check there.


Do you want to send a new version of the patch or should I just strip 
half of your patch and commit the other half?


On 2016-01-09 08:37, 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.

Signed-off-by: Jason Gerecke 
---
  src/modules/bluetooth/module-bluetooth-policy.c | 3 ++-
  src/modules/bluetooth/module-bluez5-discover.c  | 3 ++-
  2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/modules/bluetooth/module-bluetooth-policy.c 
b/src/modules/bluetooth/module-bluetooth-policy.c
index fc709ec..ede7c5f 100644
--- a/src/modules/bluetooth/module-bluetooth-policy.c
+++ b/src/modules/bluetooth/module-bluetooth-policy.c
@@ -261,7 +261,8 @@ int pa__init(pa_module *m) {
  return 0;

  fail:
-pa_modargs_free(ma);
+if (ma)
+pa_modargs_free(ma);
  return -1;
  }

diff --git a/src/modules/bluetooth/module-bluez5-discover.c 
b/src/modules/bluetooth/module-bluez5-discover.c
index 1ccc1d1..080e5d0 100644
--- a/src/modules/bluetooth/module-bluez5-discover.c
+++ b/src/modules/bluetooth/module-bluez5-discover.c
@@ -137,7 +137,8 @@ int pa__init(pa_module *m) {
  return 0;

  fail:
-pa_modargs_free(ma);
+if (ma)
+pa_modargs_free(ma);
  pa__done(m);
  return -1;
  }



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
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()

[David Henningsson]

Hmm, yes, I think this is correct. You might get errors if you push more 
than maxlength, but not more than tlength, at least from looking quickly 
at the code. Have you actually tried?


Acked regardless.

On 2016-01-11 14:28, 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(). */



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v4] NetBSD: Stop depending upon nonstandard __WORDSIZE

[David Henningsson]

Applied now. Thanks!

On 2015-12-20 01:25, Kamil Rytarowski wrote:

There is no way to check CPU type in a portable way across ABIs.

Assume if pointers are 64-bit that CPU is capable to perform fast
64-bit operations. Add an extra check to handle x32-ABI.

PulseAudio by default builds with -Wundef. If we add -Werror=undef this
missing define is fatal. By default build log is full of entries like:

In file included from ./pulsecore/core.h:47:0,
  from ./pulsecore/module.h:31,
  from ./pulsecore/sink-input.h:31,
  from pulsecore/sound-file-stream.c:36:
./pulsecore/sample-util.h: In function 'pa_mult_s16_volume':
./pulsecore/sample-util.h:58:5: warning: "__WORDSIZE" is not defined [-Wundef]
  #if __WORDSIZE == 64 || ((ULONG_MAX) > (UINT_MAX))
  ^

(NetBSD-7.99.21 with default GCC 4.8.5)

This change fixes build issues on NetBSD.

This also address a bug reported by Shawn Walker from Oracle (possibly Solaris):
Bug 90880 - builds can fail due to non-portable glibc-specific internal macro 
usage
---
  configure.ac| 11 +++
  src/pulsecore/sample-util.h |  2 +-
  src/tests/mult-s16-test.c   |  8 +++-
  3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/configure.ac b/configure.ac
index b9cd3d1..195b265 100644
--- a/configure.ac
+++ b/configure.ac
@@ -463,6 +463,17 @@ AC_TYPE_OFF_T
  AC_TYPE_UID_T
  AC_CHECK_DECLS(environ)

+AC_CHECK_SIZEOF(void*)
+
+fast_64bit_operations="no"
+# This check covers x32-ABI
+AC_CHECK_DECL([__x86_64__], [fast_64bit_operations="yes"], [], [])
+if test "x$fast_64bit_operations" = "xno"; then
+AS_IF([test $ac_cv_sizeof_voidp -ge 8], [fast_64bit_operations="yes"])
+fi
+
+AS_IF([test "x$fast_64bit_operations" = "xyes"], 
AC_DEFINE([HAVE_FAST_64BIT_OPERATIONS], 1, [Have CPU with fast 64-bit operations?]))
+
  # SIGXCPU
  AX_CHECK_DEFINE([signal.h], [SIGXCPU], [HAVE_SIGXCPU=1], [HAVE_SIGXCPU=0])
  AS_IF([test "x$HAVE_SIGXCPU" = "x1"], AC_DEFINE([HAVE_SIGXCPU], 1, [Have 
SIGXCPU?]))
diff --git a/src/pulsecore/sample-util.h b/src/pulsecore/sample-util.h
index c817bc9..3d53ebe 100644
--- a/src/pulsecore/sample-util.h
+++ b/src/pulsecore/sample-util.h
@@ -55,7 +55,7 @@ void pa_deinterleave(const void *src, void *dst[], unsigned 
channels, size_t ss,
  void pa_sample_clamp(pa_sample_format_t format, void *dst, size_t dstr, const 
void *src, size_t sstr, unsigned n);

  static inline int32_t pa_mult_s16_volume(int16_t v, int32_t cv) {
-#if __WORDSIZE == 64 || ((ULONG_MAX) > (UINT_MAX))
+#if HAVE_FAST_64BIT_OPERATIONS
  /* Multiply with 64 bit integers on 64 bit platforms */
  return (v * (int64_t) cv) >> 16;
  #else
diff --git a/src/tests/mult-s16-test.c b/src/tests/mult-s16-test.c
index d2a351c..845e61c 100644
--- a/src/tests/mult-s16-test.c
+++ b/src/tests/mult-s16-test.c
@@ -93,12 +93,10 @@ int main(int argc, char *argv[]) {
  if (!getenv("MAKE_CHECK"))
  pa_log_set_level(PA_LOG_DEBUG);

-#if __WORDSIZE == 64 || ((ULONG_MAX) > (UINT_MAX))
-pa_log_debug("This seems to be 64-bit code.");
-#elif  __WORDSIZE == 32
-pa_log_debug("This seems to be 32-bit code.");
+#if HAVE_FAST_64BIT_OPERATIONS
+pa_log_debug("Detected CPU with fast 64-bit operations.");
  #else
-pa_log_debug("Don't know if this is 32- or 64-bit code.");
+pa_log_debug("Not detected CPU with fast 64-bit operations.");
  #endif

  s = suite_create("Mult-s16");



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH 00/11] Introduce memfd support

[David Henningsson]

On 2016-01-02 21:04, Ahmed S. Darwish wrote:

Hi!


Hi! Long time no see :-)



On Mon, Sep 28, 2015 at 10:47:09AM +0200, David Henningsson wrote:

1)

There was some discussion about how the pa_mem struct should look like. I
think it should just look like this:

struct pa_mem {
   void *ptr;
   size_t size;
   pa_mem_type_t type;
   int fd;
   unsigned id; /* both shm and memfd need an id, see below */
   bool shm_do_unlink;
}

Memfd and shm can then share some methods because they're both fd backed (e
g, closing is to call munmap and then fclose for both).

Plus, it's all much simpler than anonymous unions and structs, IMO.



Indeed. After finishing doing so now in v2, a lot of code duplication
between posix SHM and memfds has been removed and the patch series is
now much smaller. Thanks a lot.



2)

The second big thing that makes me confused is how the memfd is initially
set up and sent over the pipe to the client. Am I reading your code wrong,
or do you actually send an extra memfd packet with the fd in *for every
packet* sent?



You're reading the code right. Definitely a problem to be fixed.


The per-client memfd should be set up by the server at SHM negotiation time
(and sealed, but you've said that's a future patch set). Then send a packet
with the memfd as ancil data (or extend PA_COMMAND_ENABLE_SRBCHANNEL, if
that's simpler). This packet should also have an ID that identifies the
memfd.


I'm having a problem in this part. By doing as said above, aren't we
limiting the pstream connection to send memblocks only from a _single_
memfd-backed pool?

Imagine the following:

// PA node 1
pa_pstream_send_memblock(p, memblock1);// memblock1 is inside pool1
pa_pstream_send_memblock(p, memblock2);// memblock2 is inside pool2

If pool1 and pool2 are backed by different memfd regions, how would the
above suggestion handle that case?


Hmm, to ask a counter question; why would you put them in different 
pools in the first place? Why would you need more than one pool per pstream?



Every memfd and shm now have a unique ID, so you can just put that ID when
you do the memexport, so on the sender side you don't need to distinguish
between the two types.
And on the memimport side you'd look this ID up, and see if it matches a
previously received memfd, if not, it's a posix-shm ID that you need to call
pa_shm_attach on.

Does that make sense to you?



Ah, definitely agree. This is much simpler.

If my claim above is valid though, I might just implement in a slightly
different way:

``Instead of sending the memfd file descriptor at SHM negotiation
   time, do it in pstream.c::prepare_next_write_item().

   To avoid the problem of sending the memfd fd for every packet sent,
   track the memfd-backed blocks earlier sent using their randomly
   generated IDs.

   If this is the first time we encounter this ID, send the memfd file
   descriptor as ancil data with the packet. If this is not the first
   encouter, just send the ID without any ancil data. The other end
   is already tracking this ID and have a memfd socket opened for it
   from earlier communication''

What do you think?

Thanks again, and happy holidays :)



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v3] netbsd: Stop depending upon nonstandard __WORDSIZE

[David Henningsson]

On 2015-12-16 10:40, Kamil Rytarowski wrote:

There is no way to check CPU type in a portable way across ABIs.

Go for pa_mult_s16_volume() version without need for int64_t.
It's actually the same as in src/tests/mult-s16-test.c.

This change fixes build on NetBSD.
---
  src/pulsecore/sample-util.h | 5 -
  src/tests/mult-s16-test.c   | 9 ++---
  2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/src/pulsecore/sample-util.h b/src/pulsecore/sample-util.h
index c817bc9..9862938 100644
--- a/src/pulsecore/sample-util.h
+++ b/src/pulsecore/sample-util.h
@@ -55,10 +55,6 @@ void pa_deinterleave(const void *src, void *dst[], unsigned 
channels, size_t ss,
  void pa_sample_clamp(pa_sample_format_t format, void *dst, size_t dstr, const 
void *src, size_t sstr, unsigned n);
  
  static inline int32_t pa_mult_s16_volume(int16_t v, int32_t cv) {

-#if __WORDSIZE == 64 || ((ULONG_MAX) > (UINT_MAX))
-/* Multiply with 64 bit integers on 64 bit platforms */
-return (v * (int64_t) cv) >> 16;
-#else


Instead of dropping this (which looks like an optimisation), can't you 
just do - as a quick fix if nothing else:


#if (defined(__WORDSIZE) && (__WORDSIZE == 64)) || ((ULONG_MAX) > 
(UINT_MAX))


Will that allow netbsd to build?

But, according to the gcc docs [1], "if  is not defined, it will 
be interpreted as having the value zero".
So I'm still not understanding why the current code breaks on netbsd? 
Are you using some other compiler which behaves differently?


[1] https://gcc.gnu.org/onlinedocs/cpp/Defined.html


  /* Multiplying the 32 bit volume factor with the
   * 16 bit sample might result in an 48 bit value. We
   * want to do without 64 bit integers and hence do
@@ -68,7 +64,6 @@ static inline int32_t pa_mult_s16_volume(int16_t v, int32_t 
cv) {
  int32_t hi = cv >> 16;
  int32_t lo = cv & 0x;
  return ((v * lo) >> 16) + (v * hi);
-#endif
  }
  
  pa_usec_t pa_bytes_to_usec_round_up(uint64_t length, const pa_sample_spec *spec);

diff --git a/src/tests/mult-s16-test.c b/src/tests/mult-s16-test.c
index d2a351c..20ad107 100644
--- a/src/tests/mult-s16-test.c
+++ b/src/tests/mult-s16-test.c
@@ -93,13 +93,8 @@ int main(int argc, char *argv[]) {
  if (!getenv("MAKE_CHECK"))
  pa_log_set_level(PA_LOG_DEBUG);
  
-#if __WORDSIZE == 64 || ((ULONG_MAX) > (UINT_MAX))

-pa_log_debug("This seems to be 64-bit code.");
-#elif  __WORDSIZE == 32
-pa_log_debug("This seems to be 32-bit code.");
-#else
-pa_log_debug("Don't know if this is 32- or 64-bit code.");
-#endif
+pa_log_debug("On this platform, integer size is %zu and long size is %zu",
+sizeof(int), sizeof(long));
  
  s = suite_create("Mult-s16");

  tc = tcase_create("mult-s16");


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

Re: [pulseaudio-discuss] [PATCH] netbsd: Replace nonstandard __WORDSIZE with a more portable solution

[David Henningsson]

On 2015-12-10 06:23, Kamil Rytarowski wrote:

There is no way to check CPU type in a portable way across ABIs.
Checking for sizeof(void*) is reasonable since most platforms will
report correct values. One exception is x32, but since it's halfbaked
never finished and almost not needed any more - we can ignore it.

The check is needed only to print a debug message, no functional
change.


There is also a reference to __WORDSIZE in src/pulsecore/sample-util.h.


This change fixes build on NetBSD.


What error are you getting, and how come it does not affect 
src/pulsecore/sample-util.h as well?



---
  configure.ac  | 3 +++
  src/tests/mult-s16-test.c | 5 +++--
  2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index b9cd3d1..7735081 100644
--- a/configure.ac
+++ b/configure.ac
@@ -463,6 +463,9 @@ AC_TYPE_OFF_T
  AC_TYPE_UID_T
  AC_CHECK_DECLS(environ)

+# Used to deduct CPU word size
+AC_CHECK_SIZEOF(void*)
+
  # SIGXCPU
  AX_CHECK_DEFINE([signal.h], [SIGXCPU], [HAVE_SIGXCPU=1], [HAVE_SIGXCPU=0])
  AS_IF([test "x$HAVE_SIGXCPU" = "x1"], AC_DEFINE([HAVE_SIGXCPU], 1, [Have 
SIGXCPU?]))
diff --git a/src/tests/mult-s16-test.c b/src/tests/mult-s16-test.c
index d2a351c..ac5a43f 100644
--- a/src/tests/mult-s16-test.c
+++ b/src/tests/mult-s16-test.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 

  #include 
  #include 
@@ -93,9 +94,9 @@ int main(int argc, char *argv[]) {
  if (!getenv("MAKE_CHECK"))
  pa_log_set_level(PA_LOG_DEBUG);

-#if __WORDSIZE == 64 || ((ULONG_MAX) > (UINT_MAX))
+#if (SIZEOF_VOIDP * CHAR_BIT) == 64
  pa_log_debug("This seems to be 64-bit code.");
-#elif  __WORDSIZE == 32
+#elif (SIZEOF_VOIDP * CHAR_BIT) == 32
  pa_log_debug("This seems to be 32-bit code.");
  #else
  pa_log_debug("Don't know if this is 32- or 64-bit code.");



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v3 0/2] .d directories for configuration

[David Henningsson]

I've pushed these two now. There is still the open question/issue of 
whether we should merge all four locations, but I think that can be 
discussed/resolved later and this is still a good improvement.


Thanks!

// David

On 2015-12-07 22:22, Tanu Kaskinen wrote:

Earlier discussions:
  - v1: http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/23592
  - v2: http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/24578

Changes in v3:
  - Load the .d files after, not before, the main file.

Changes  v2:
  - Split the original patch into two parts.
  - Added documentation to the man pages.
  - Changed a debug message about non-existent .d directory from
  "scandir(\"%s\") failed: %s", dir_name, pa_cstrerror(errno)
to
  "%s does not exist, ignoring.", dir_name

Tanu Kaskinen (2):
   conf-parser: add support for .d directories
   client-conf, daemon-conf: enable .d directories

  man/pulse-client.conf.5.xml.in  | 19 +++
  man/pulse-daemon.conf.5.xml.in  | 25 ++--
  src/daemon/daemon-conf.c|  2 +-
  src/modules/alsa/alsa-mixer.c   |  4 ++--
  src/modules/module-augment-properties.c |  2 +-
  src/pulse/client-conf.c |  2 +-
  src/pulsecore/conf-parser.c | 42 +++--
  src/pulsecore/conf-parser.h |  8 ++-
  8 files changed, 85 insertions(+), 19 deletions(-)



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
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

[David Henningsson]

On 2015-12-14 13:48, Arun Raghavan wrote:

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).


Ok, pushed now.

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
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

[David Henningsson]

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?



On 2015-12-11 15:00, Felipe Sateler wrote:

It is not meant to be used by third parties, so do not install in a public dir
---
  src/Makefile.am | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

This is a resend, earlier patch was not picked up by patchwork.

diff --git a/src/Makefile.am b/src/Makefile.am
index de975c5..f1bd38d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -918,7 +918,7 @@ libpulsedsp_la_LDFLAGS = $(AM_LDFLAGS) $(AM_LIBLDFLAGS) 
-avoid-version -disable-
  #  Daemon core library#
  ###

-lib_LTLIBRARIES += libpulsecore-@PA_MAJORMINOR@.la
+pkglib_LTLIBRARIES += libpulsecore-@PA_MAJORMINOR@.la

  # Pure core stuff
  libpulsecore_@PA_MAJORMINOR@_la_SOURCES = \



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [ANNOUNCE] Patchwork used to track patches

[David Henningsson]

Hi,

As some of you have noticed, we have started to use Patchwork to track 
patches submitted to PulseAudio.


This means that the recommended way to submit patches is to use "git 
send-email" [1] with pulseaudio-discuss@lists.freedesktop.org as 
recipient, this way they'll automatically end up in Patchwork.


Our patchwork is hosted at:
http://patchwork.freedesktop.org/project/pulseaudio/
and you can see the status of all patches here [2].

With help from some clever scripting by Alexander Patrakov (thanks!) 
I've gone through unreviewed patches between May 2015 and up until the 
day the Patchwork instance was set up. I might have missed something though.


Unfortunately, while a new Patchwork system helps the review process for 
us maintainers, it does not increase the review bandwidth, which 
especially hurts larger, more complex patch sets.


If you feel your patch is being ignored, here are a few things you can 
do to try to help:


 * Resend your patch series (to the mailinglist), preferrably rebased 
on top of git master.


 * Come bug us on IRC (#pulseaudio on freenode), e g the person who you 
think is most qualified to review your particular patch. Most of us are 
in a European timezone.


 * Review other people's patches. Not only does this help us get to 
your patch sooner, it also gives you a warm fuzzy feeling. :-)


--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic


[1] 
http://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/


[2] 
http://patchwork.freedesktop.org/project/pulseaudio/patches/?state=*&archive=both

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

Re: [pulseaudio-discuss] [PATCH] context: continue without srb channel if it fails

[David Henningsson]

On 2015-12-10 16:22, Pierre Ossman wrote:

We might be compiled without eventfd support, or something else
might go wrong. And it's fully possible to continue using the old
channel rather than just disconnecting.

Signed-off-by: Pierre Ossman 
---
  src/pulse/context.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/pulse/context.c b/src/pulse/context.c
index 738ea84..4f084e8 100644
--- a/src/pulse/context.c
+++ b/src/pulse/context.c
@@ -364,7 +364,11 @@ static void handle_srbchannel_memblock(pa_context
*c, pa_memblock *memblock) { pa_memblock_ref(memblock);
  sr = pa_srbchannel_new_from_template(c->mainloop,
&c->srb_template); if (!sr) {


The patch itself is acked, as already written in 
https://bugs.freedesktop.org/show_bug.cgi?id=93285


But the patch appears broken w r t automatic line breaks here. Could you 
resend it as an attachment (or fix your mailer)? Thanks.



-pa_context_fail(c, PA_ERR_PROTOCOL);
+pa_log_warn("Failed to create srbchannel from template");
+c->srb_template.readfd = -1;
+c->srb_template.writefd = -1;
+pa_memblock_unref(c->srb_template.memblock);
+c->srb_template.memblock = NULL;
  return;
  }




--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] pkgsrc: Fix build on systems without Real-Time signals

[David Henningsson]

Thanks for the patch, but would it not make more sense not to build the 
test at all (by modifying src/Makefile.am), rather than having a test 
that always succeeds?


E g, like srbchannel-test is only builds if HAVE_SYS_EVENTFD_H.

On 2015-12-11 05:49, Kamil Rytarowski wrote:

Original patch from pkgsrc by Ryo Ondera 
---
  src/tests/rtpoll-test.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/src/tests/rtpoll-test.c b/src/tests/rtpoll-test.c
index 7db7177..d0398a0 100644
--- a/src/tests/rtpoll-test.c
+++ b/src/tests/rtpoll-test.c
@@ -83,6 +83,7 @@ START_TEST (rtpoll_test) {
  END_TEST

  int main(int argc, char *argv[]) {
+#ifdef SIGRTMIN
  int failed = 0;
  Suite *s;
  TCase *tc;
@@ -103,4 +104,9 @@ int main(int argc, char *argv[]) {
  srunner_free(sr);

  return (failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
+#else
+printf("Real-time signals are not supported on this platform\n");
+
+return EXIT_SUCCESS;
+#endif
  }



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] alsa-mixer: Have valid proplist for synthesized path as well.

[David Henningsson]

Applied. Thanks!

On 2015-12-10 14:09, Juho Hämäläinen wrote:

When synthesized alsa path is freed there is an assert from NULL
proplist. Create empty proplist for the path to fix.

Signed-off-by: Juho Hämäläinen 
---
  src/modules/alsa/alsa-mixer.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index 3f51717..515b285 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -2621,6 +2621,7 @@ pa_alsa_path *pa_alsa_path_synthesize(const char 
*element, pa_alsa_direction_t d
  p = pa_xnew0(pa_alsa_path, 1);
  p->name = pa_xstrdup(element);
  p->direction = direction;
+p->proplist = pa_proplist_new();

  e = pa_xnew0(pa_alsa_element, 1);
  e->path = p;



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
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

[David Henningsson]

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 
<http://www.gnu.org/licenses/>.


  ~/.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?)

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] build-sys: fix PULSE_LOCALEDIR definition

[David Henningsson]

On 2015-09-29 16:49, David Henningsson wrote:



On 2015-09-29 16:34, Tanu Kaskinen wrote:

On Tue, 2015-09-29 at 15:01 +0200, David Henningsson wrote:


On 2015-09-29 10:41, Tanu Kaskinen wrote:

On some systems (at least Arch) DATADIRNAME is not defined. This
caused PULSE_LOCALEDIR to point to a wrong directory. This seemed like
an issue introduced in 7.0, but probably something else was updated in
Arch at the same time, causing DATADIRNAME to become undefined,
because there were no changes between 6.0 and 7.0 that could have
caused this.


I don't know eitehr DATADIRNAME or ${localedir} enough to tell which one
is preferred, but it seems like your change now defines PULSE_LOCALEDIR
even if "enable_nls" == "no", which is a difference to the current
behaviour. Is this intentional?


It's not intentional, but the only place where PULSE_LOCALEDIR is used
is in bindtextdomain() calls, and those are guarded by #ifdef
ENABLE_NLS. So I don't think this is a problem.


Ok, then somewhat of an Ack on my side - wait a week or two and see if
anyone complains, otherwise okay to push IMO.


Pushed now.


--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Run a user specified program on suspend/resume.

[David Henningsson]

ng, suspending ...", 
d->source->name);
+call_external_suspend_handler(true, d);
  pa_source_suspend(d->source, true, PA_SUSPEND_IDLE);
  pa_core_maybe_vacuum(d->userdata->core);
  }
@@ -102,11 +126,13 @@ static void resume(struct device_info *d) {

  if (d->sink) {
  pa_log_debug("Sink %s becomes busy, resuming.", d->sink->name);
+call_external_suspend_handler(false, d);
  pa_sink_suspend(d->sink, false, PA_SUSPEND_IDLE);
  }

  if (d->source) {
  pa_log_debug("Source %s becomes busy, resuming.", d->source->name);
+call_external_suspend_handler(false, d);
  pa_source_suspend(d->source, false, PA_SUSPEND_IDLE);
  }
  }
@@ -439,6 +465,10 @@ int pa__init(pa_module*m) {
  u->timeout = timeout * PA_USEC_PER_SEC;
  u->device_infos = pa_hashmap_new_full(pa_idxset_trivial_hash_func, 
pa_idxset_trivial_compare_func, NULL, (pa_free_cb_t) device_info_free);

+u->external_suspend_handler = pa_modargs_get_value(ma, 
"external_suspend_handler", NULL);
+if (u->external_suspend_handler)
+u->external_suspend_handler = pa_xstrdup(u->external_suspend_handler);
+
  PA_IDXSET_FOREACH(sink, m->core->sinks, idx)
  device_new_hook_cb(m->core, PA_OBJECT(sink), u);

@@ -486,5 +516,8 @@ void pa__done(pa_module*m) {

  pa_hashmap_free(u->device_infos);

+if (u->external_suspend_handler)
+  pa_xfree((char *) u->external_suspend_handler);
+
  pa_xfree(u);
  }



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] core-util: improve comments in pa_machine_id()

[David Henningsson]

Thanks. Looks good to me, pushed now.

On 2015-08-12 11:37, Tanu Kaskinen wrote:

---
  src/pulsecore/core-util.c | 21 +
  1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/pulsecore/core-util.c b/src/pulsecore/core-util.c
index edb9e38..e09e2d2 100644
--- a/src/pulsecore/core-util.c
+++ b/src/pulsecore/core-util.c
@@ -3057,14 +3057,19 @@ char *pa_machine_id(void) {
  char *h;

  /* The returned value is supposed be some kind of ascii identifier
- * that is unique and stable across reboots. */
-
-/* First we try ${sysconfdir}/etc/machine-id, with fallbacks to
- * ${localstatedir}/lib/dbus/machine-id, /etc/machine-id and
- * /var/lib/dbus/machine-id, which are the best option we
- * have, since they fit perfectly our needs and are not as volatile
- * as the hostname which might be set from dhcp. */
-
+ * that is unique and stable across reboots. First we try if the machine-id
+ * file is available. If it's available, that's great, since it provides an
+ * identifier that suits our needs perfectly. If it's not, we fall back to
+ * the hostname, which is not as good, since it can change over time. */
+
+/* We search for the machine-id file from four locations. The first two are
+ * relative to the configured installation prefix, but if we're installed
+ * under /usr/local, for example, it's likely that the machine-id won't be
+ * found there, so we also try the hardcoded paths.
+ *
+ * PA_MACHINE_ID or PA_MACHINE_ID_FALLBACK might exist on a Windows system,
+ * but the last two hardcoded paths certainly don't, hence we don't try
+ * them on Windows. */
  if ((f = pa_fopen_cloexec(PA_MACHINE_ID, "r")) ||
  (f = pa_fopen_cloexec(PA_MACHINE_ID_FALLBACK, "r")) ||
  #if !defined(OS_IS_WIN32)



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] Use freedesktop.org standard icon name

[David Henningsson]

Hi, thanks for the patch, and sorry for the late reply.

I tested this patch on my Ubuntu machine and it makes the icon change 
from a keylock to a shield.


Also, the description of "security-medium" is "The icon used to indicate 
that the security level of a connection is presumed to be secure, using 
strong encryption, and a certificate that could not be automatically 
verified, but which the user has chosen to trust." so it does not claim 
that it has to be a keylock or anything related to a lock.


As the icon represents "lock channels together" and has nothing to do 
with security, I suggest we keep the current icon.


On 2015-07-31 21:36, Paul W. Frields wrote:

---
  src/pavucontrol.glade | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pavucontrol.glade b/src/pavucontrol.glade
index 219f5a2..5efd4b3 100644
--- a/src/pavucontrol.glade
+++ b/src/pavucontrol.glade
@@ -312,7 +312,7 @@

  True
  False
-stock_lock
+security-medium
  1

  
@@ -1474,7 +1474,7 @@

  True
  False
-stock_lock
+security-medium
  1

          



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] netbsd: Fix unportable test(1) construct

[David Henningsson]

Applied. Thanks!

On 2015-11-28 14:59, Kamil Rytarowski wrote:

---
  configure.ac | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index abfb8d9..e4b4d4d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -488,14 +488,14 @@ AS_IF([test "$pulseaudio_cv__Bool" = "yes"], 
AC_DEFINE([HAVE_STD_BOOL], 1, [Have
   Thread support 

  AX_TLS
-AS_IF([test "$ac_cv_tls" == "__thread"],
+AS_IF([test "$ac_cv_tls" = "__thread"],
  AC_DEFINE([SUPPORT_TLS___THREAD], 1, [Define this if the compiler 
supports __thread for Thread-Local Storage]))

  # Win32 build breaks with win32 pthread installed
  AS_IF([test "x$os_is_win32" != "x1"],
[AX_PTHREAD])

-AS_IF([test "x$ax_pthread_ok" == "xyes"],
+AS_IF([test "x$ax_pthread_ok" = "xyes"],
  AC_DEFINE([_POSIX_PTHREAD_SEMANTICS], 1, [Needed on Solaris]))





--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] netbsd: Improve handling of and headers

[David Henningsson]

Applied, thanks.

Btw, when you make a new patch that supersedes another, you can use "git 
send-email"'s -v2 (and -v3 etc) flag to change the subject from [PATCH] 
to [PATCH v2]. Doing so helps both us and patchwork to track that the 
old patch is superseded.


On 2015-11-28 10:01, Kamil Rytarowski wrote:

NetBSD ships with strtod_l(3) in .
Having strtol_l(3) doesn't imply to have .
Generalize inclusion of  and .
---
  configure.ac  | 1 +
  src/pulsecore/core-util.c | 5 -
  2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 003673e..5b3ce7d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -447,6 +447,7 @@ AC_CHECK_HEADERS_ONCE([sys/eventfd.h])
  AC_CHECK_HEADERS_ONCE([execinfo.h])
  AC_CHECK_HEADERS_ONCE([langinfo.h])
  AC_CHECK_HEADERS_ONCE([regex.h pcreposix.h])
+AC_CHECK_HEADERS_ONCE([locale.h xlocale.h])

  AM_CONDITIONAL(HAVE_SYS_EVENTFD_H, test "x$ac_cv_header_sys_eventfd_h" = 
"xyes")

diff --git a/src/pulsecore/core-util.c b/src/pulsecore/core-util.c
index 2099686..ee74280 100644
--- a/src/pulsecore/core-util.c
+++ b/src/pulsecore/core-util.c
@@ -53,9 +53,13 @@
  #endif

  #ifdef HAVE_STRTOD_L
+#ifdef HAVE_LOCALE_H
  #include 
+#endif
+#ifdef HAVE_XLOCALE_H
  #include 
  #endif
+#endif

  #ifdef HAVE_SCHED_H
  #include 
@@ -106,7 +110,6 @@
  #endif

  #ifdef __APPLE__
-#include 
  #include 
  #include 
  #include 



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH v2 1/2] conf-parser: add support for .d directories

[David Henningsson]

On 2015-12-04 13:46, Tanu Kaskinen wrote:

On Fri, 2015-12-04 at 09:29 -0300, Felipe Sateler wrote:

On 4 December 2015 at 02:58, Tanu Kaskinen  wrote:

+ * If use_dot_d is true, then before parsing the file named by the filename
+ * argument, the function will parse all files ending with ".conf" in
+ * alphabetical order from a directory whose name is filename + ".d", if such
+ * directory exists.


This is opposite to how other software (eg systemd) work: first the
main file is read, and then the .d/*.conf files are read to override
the configurations.

So if a distro default has an uncommented line, the .d should override
that line. This would allow (in the future) to extend the search path
to /usr/share/pulseaudio, and ship the defaults (uncommented) there.
Then local configuration can be done via dropin files.


All advice given to users so far has been to modify the main files,
because nothing else has existed. If users put their changes to the
main files and distros put their changes to the .d files, that will be
compatible with existing advice on the internet.


As an additional data point, I looked at pam 
(/etc/security/limits.{conf|.d/*.conf}, which (if I'm reading the code 
correctly) does the same as systemd, i e, /etc/security/limits.conf 
being the default, and then /etc/security/limits.d/*.conf overriding that.


If the config file has been given explicitly (not a problem for us now, 
but would be if we ever replace default.pa with this), then the .d files 
are skipped.


My opinion: In the long term, I think it would be a bigger advantage to 
be consistent with other common software, compared to the advantage of 
being compatible with existing advice. We could also add a comment 
saying this in the main .conf file.


Hence we end up with the following order:

/etc/pulse/daemon.conf
/etc/pulse/daemon.d/*.conf
~/.config/pulse/daemon.conf
~/.config/pulse/daemon.d/*.conf

...current documentation states that "If the version in the user's home
directory does not exist the global configuration file is loaded.", or 
put it in another way, we skip loading the global file completely if the 
user specific file is present. Is this desired behavior or should we 
merge all four files in priority order?


--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] typedefs.h: Move some typedefs to a separate file

[David Henningsson]

On 2015-11-27 12:40, Tanu Kaskinen wrote:

On Thu, 2015-11-26 at 18:29 +0100, David Henningsson wrote:

The relationship between sinks, sources, cards, profiles, and ports
is becoming ever more intertwined, to the point that if you try to
include one file from the other, you're likely to end up with some
weird error somewhere else.


If you add a circular dependency between two headers, you get errors,
which you can fix by moving the typedefs above the #include lines. I
don't think those errors are weird or difficult to deal with (once you
know the fix).


As an example, I was trying to add "#include " 
to card.h. Both pa_card and pa_device_port have their typedefs above the 
#include lines. Still, that leads to this error:


In file included from ../../src/pulsecore/card.h:29:0,
 from ../../src/pulsecore/source.h:43,
 from ../../src/pulsecore/sink.h:38,
 from ../../src/pulsecore/core.h:49,
 from ../../src/pulsecore/module.h:31,
 from ../../src/pulsecore/cli-text.c:28:
../../src/pulsecore/device-port.h:49:5: error: unknown type name 
'pa_available_t'


Maybe that can be worked around some other way, but if I include the 
typedefs.h file, then I don't need to include device-port.h in the first 
place.



This patch has another potential benefit, though: it allows
removing includes between headers. If the unnecessary includes are
removed, that will reduce the need to recompile stuff when changing the
headers. Currently, if you touch e.g. sink.h, that will trigger a
rebuild of pretty much everything.


Indeed.


Work around this by creating a new typedefs.h, which does not depend
on anything else, and just creates a few typedefs.

(Can be expanded with more typedefs in the future if the need arises.)


What's the criteria for deciding whether a typedef should go in
typdefs.h?


Except those who are already there, I don't have a strong opinion.

--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [RFC] [PATCH] alsa-sink: Reduce unnecessarily large max rewind

[David Henningsson]

Ambivalent about this one - I think I agree with the general concept, 
but OTOH it somehow feels like the big rewind is there for a reason too.


Maybe if you dig deeper into the problem about filter sinks that you 
noticed, maybe that can give us some hint to whether or not this is a 
correct patch?


I'm marking it as "changes requested" in patchwork for the time being.

On 2015-11-17 08:40, a...@accosted.net wrote:

From: Arun Raghavan 

This makes us not report more than the hardware buffer size being used
as the max rewind that sink inputs need to support. The rationale is
that we won't ever rewind more than the length of the buffer that we are
currently using, so we don't need to keep the extra data around. On
large HDA buffers, this should save some memory for each sink input.

However, I do notice a problem -- when there is a filter sink attached,
the first stream after a resume from suspend playse fine, but the second
stream has a lag that seems to be in the order of the buffer size. I'm
not sure where the problem lies.

I'll try to take a look at what's going wrong, but if anyone has any
ideas, I'm all ears.
---
  src/modules/alsa/alsa-sink.c | 39 ---
  1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index c5a72c3..94aeff0 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -956,12 +956,14 @@ static int suspend(struct userdata *u) {
  }

  /* Called from IO context */
-static int update_sw_params(struct userdata *u) {
+static int update_sw_params(struct userdata *u, bool rewind) {
  snd_pcm_uframes_t avail_min;
+size_t before;
  int err;

  pa_assert(u);

+before = u->hwbuf_unused;
  /* Use the full buffer if no one asked us for anything specific */
  u->hwbuf_unused = 0;

@@ -1006,9 +1008,21 @@ static int update_sw_params(struct userdata *u) {
  return err;
  }

+if (rewind) {
+/* Let's check whether we now use only a smaller part of the
+   buffer then before. If so, we need to make sure that subsequent
+   rewinds are relative to the new maximum fill level and not to the
+   current fill level. Thus, let's do a full rewind once, to clear
+   things up. */
+if (u->hwbuf_unused > before) {
+pa_log_debug("Requesting rewind due to latency change.");
+pa_sink_request_rewind(u->sink, (size_t) -1);
+}
+}
+
  pa_sink_set_max_request_within_thread(u->sink, u->hwbuf_size - 
u->hwbuf_unused);
- if (pa_alsa_pcm_is_hw(u->pcm_handle))
- pa_sink_set_max_rewind_within_thread(u->sink, u->hwbuf_size);
+if (pa_alsa_pcm_is_hw(u->pcm_handle))
+pa_sink_set_max_rewind_within_thread(u->sink, u->hwbuf_size - 
u->hwbuf_unused);
  else {
  pa_log_info("Disabling rewind_within_thread for device %s", 
u->device_name);
  pa_sink_set_max_rewind_within_thread(u->sink, 0);
@@ -1109,7 +1123,7 @@ static int unsuspend(struct userdata *u) {
  goto fail;
  }

-if (update_sw_params(u) < 0)
+if (update_sw_params(u, false) < 0)
  goto fail;

  if (build_pollfd(u) < 0)
@@ -1505,7 +1519,6 @@ static int sink_set_port_cb(pa_sink *s, pa_device_port 
*p) {

  static void sink_update_requested_latency_cb(pa_sink *s) {
  struct userdata *u = s->userdata;
-size_t before;
  pa_assert(u);
  pa_assert(u->use_tsched); /* only when timer scheduling is used
 * we can dynamically adjust the
@@ -1514,19 +1527,7 @@ static void sink_update_requested_latency_cb(pa_sink *s) 
{
  if (!u->pcm_handle)
  return;

-before = u->hwbuf_unused;
-update_sw_params(u);
-
-/* Let's check whether we now use only a smaller part of the
-buffer then before. If so, we need to make sure that subsequent
-rewinds are relative to the new maximum fill level and not to the
-current fill level. Thus, let's do a full rewind once, to clear
-things up. */
-
-if (u->hwbuf_unused > before) {
-pa_log_debug("Requesting rewind due to latency change.");
-pa_sink_request_rewind(s, (size_t) -1);
-}
+update_sw_params(u, true);
  }

  static pa_idxset* sink_get_formats(pa_sink *s) {
@@ -2360,7 +2361,7 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, 
const char*driver, pa_ca

  reserve_update(u);

-if (update_sw_params(u) < 0)
+if (update_sw_params(u, false) < 0)
  goto fail;

  if (u->ucm_context) {



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] netbsd: Improve handling of and headers

[David Henningsson]

On 2015-11-21 00:38, Kamil Rytarowski wrote:

NetBSD ships with strtod_l(3) in .
Having strtol_l(3) isn't relevant to having .
Generalize inclusion of  and .


Hmm, but we don't need to include either if we don't have strtod_l, 
because that's the only function needing anything from locale.h/xlocale.h.



---
  configure.ac  | 1 +
  src/pulsecore/core-util.c | 6 --
  2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 003673e..5b3ce7d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -447,6 +447,7 @@ AC_CHECK_HEADERS_ONCE([sys/eventfd.h])
  AC_CHECK_HEADERS_ONCE([execinfo.h])
  AC_CHECK_HEADERS_ONCE([langinfo.h])
  AC_CHECK_HEADERS_ONCE([regex.h pcreposix.h])
+AC_CHECK_HEADERS_ONCE([locale.h xlocale.h])


...this seems reasonable, but...



  AM_CONDITIONAL(HAVE_SYS_EVENTFD_H, test "x$ac_cv_header_sys_eventfd_h" = 
"xyes")

diff --git a/src/pulsecore/core-util.c b/src/pulsecore/core-util.c
index 2099686..24babcb 100644
--- a/src/pulsecore/core-util.c
+++ b/src/pulsecore/core-util.c
@@ -52,8 +52,11 @@
  #include 
  #endif

-#ifdef HAVE_STRTOD_L
+#ifdef HAVE_LOCALE_H
  #include 
+#endif
+
+#ifdef HAVE_XLOCALE_H
  #include 
  #endif


...per the reasoning above, I think it should instead change to:

#ifdef HAVE_STRTOD_L
  #ifdef HAVE_LOCALE_H
#include 
  #endif
  #ifdef HAVE_XLOCALE_H
#include 
  #endif
#endif



@@ -106,7 +109,6 @@
  #endif

  #ifdef __APPLE__
-#include 
  #include 
  #include 
  #include 



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] netbsd: NetBSD ships with paccept(2) a superset of Linux-specific accept4()

[David Henningsson]

Pushed this one (after moving paccept to the "bsd" section, that seemed 
appropriate). Thanks.


// David

On 2015-11-21 23:08, Kamil Rytarowski wrote:

---
  configure.ac  | 2 +-
  src/pulsecore/core-util.c | 7 +++
  2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 003673e..24ba7ae 100644
--- a/configure.ac
+++ b/configure.ac
@@ -572,7 +572,7 @@ AC_CHECK_FUNCS_ONCE([strerror_r])
  AC_CHECK_FUNCS_ONCE([lstat])

  # Non-standard
-AC_CHECK_FUNCS_ONCE([setresuid setresgid setreuid setregid seteuid setegid 
ppoll strsignal sig2str strtod_l pipe2 accept4])
+AC_CHECK_FUNCS_ONCE([setresuid setresgid setreuid setregid seteuid setegid 
ppoll strsignal sig2str strtod_l pipe2 accept4 paccept])

  AC_FUNC_ALLOCA

diff --git a/src/pulsecore/core-util.c b/src/pulsecore/core-util.c
index 2099686..b5ddd3d 100644
--- a/src/pulsecore/core-util.c
+++ b/src/pulsecore/core-util.c
@@ -3501,6 +3501,8 @@ finish:
  int pa_accept_cloexec(int sockfd, struct sockaddr *addr, socklen_t *addrlen) {
  int fd;

+errno = 0;
+
  #ifdef HAVE_ACCEPT4
  if ((fd = accept4(sockfd, addr, addrlen, SOCK_CLOEXEC)) >= 0)
  goto finish;
@@ -3510,6 +3512,11 @@ int pa_accept_cloexec(int sockfd, struct sockaddr *addr, 
socklen_t *addrlen) {

  #endif

+#ifdef HAVE_PACCEPT
+if ((fd = paccept(sockfd, addr, addrlen, NULL, SOCK_CLOEXEC)) >= 0)
+goto finish;
+#endif
+
  if ((fd = accept(sockfd, addr, addrlen)) >= 0)
  goto finish;




--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [PATCH] alsa-mixer: sb-omni-surround-5.1.conf: remove analog-surround-21, add Linux 4.3+ support

[David Henningsson]

Ok. Let's hope that in the future somebody actually fixes the driver so 
that 2.1 works and we can add it back, but for now, I've pushed your patch.


Thanks for the contribution!

On 2015-11-26 14:33, Nazar Mokrynskyi wrote:

I was discussing this here once already. In 2.1 mode LFE just doesn't
work. Instead when I test LFE speaker in sound settings there is weird
long noise in one of rare speakers. I have no idea about reason of this,
so instead of 2.1 I'm using 5.1 mode, which works fine.
My system is Ubuntu 16.04 x64, so packages:
alsa-base: 1.0.25+dfsg-0ubuntu5
alsa-utils: 1.0.29-1ubuntu1

If there is any ppa to try newer packages - I can do that.

Sincerely, Nazar Mokrynskyi
github.com/nazar-pc
Skype: nazar-pc
Diaspora: naza...@diaspora.mokrynskyi.com
Tox:
A9D95C9AA5F7A3ED75D83D0292E22ACE84BA40E912185939414475AF28FD2B2A5C8EF5261249


On 26.11.15 13:54, David Henningsson wrote:

Hi and thanks for looking after your sound card :-)

Just a question w r t the 2.1 stuff - what alsa-lib version are you
using? Any release from 2015 should be fine, there was some
improvements during 2014 to the 2.1 stuff in specific.

On 2015-11-25 09:06, Nazar Mokrynskyi wrote:

Just want to remind once again, that without this patch microphone is
not working with Linux kernel 4.3+

Sincerely, Nazar Mokrynskyi
github.com/nazar-pc
Skype: nazar-pc
Diaspora:naza...@diaspora.mokrynskyi.com
Tox:
A9D95C9AA5F7A3ED75D83D0292E22ACE84BA40E912185939414475AF28FD2B2A5C8EF5261249


On 11.10.15 17:52, Nazar Mokrynskyi wrote:

> Do you mean 2.1 mode won't work with all usb audio since surround
5.1 of usb audio is already using route plugin
What do you mean by "all usb audio"? In 4.1/5.1 mode LFE and all other
outputs work properly, it is just for 2.1, which works as stereo.

> If you don't have SPDIF output/input, you need to add your card in
USB-AUDIO.conf
Sound card has S/PDIF output, but not input. Not sure whether output
works properly though. I do not have speakers or any other hardware
with S/PDIF input to test it.
Sincerely, Nazar Mokrynskyi
github.com/nazar-pc
Skype: nazar-pc
Diaspora:naza...@diaspora.mokrynskyi.com
Tox:
A9D95C9AA5F7A3ED75D83D0292E22ACE84BA40E912185939414475AF28FD2B2A5C8EF5261249

On 11.10.15 10:34, Raymond Yau wrote:


>
> In 2.1 mode LFE is not actually working at all, so it is removed.

Do you mean 2.1 mode won't work with all usb audio since surround 5.1
of usb audio is already using route plugin

USB-Audio.pcm.surround51.0 {
@args [ CARD ]
@args.CARD { type string }
@func refer
name {
@func concat
strings [
"cards.USB-Audio."
{ @func card_name card $CARD }
".pcm.surround51:CARD=" $CARD
]
}
default {
type route
ttable.0.0 1
ttable.1.1 1
ttable.2.4 1
ttable.3.5 1
ttable.4.2 1
ttable.5.3 1
slave {
pcm {
type hw
card $CARD
device 0
}
channels 6
}
}
}

http://git.alsa-project.org/?p=alsa-lib.git;a=blobdiff;f=src/conf/cards/USB-Audio.conf;h=ce3ae019f7f6fa91ca224074d12891217f242300;hp=8a6d9cac6ead765108e09c61914a84d290482556;hb=1af088e39b75a0a0897c7036487b143e983cd423;hpb=57b5076c30b3453ee843912c0aeb3df8dbee3f68


> With Linux 4.3-rc1+ Mic/Line are hw:%f,0,0 as it should be:
https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/sound/usb?id=5ee20bc792467d7d612157e0a9962765aa943b08

> So now we support both Linux 4.2.x- and 4.3-rc1+ setups.
> Also in Linux 4.3-rc1 S/PDIF input was detected incorrectly (there
is no such hardware input), so it is not present in config.
> ---

https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/1384952

Seem all usb audio with more than two playback devices are affected
since capture device of  U7 is also device 1 before the patch

If you don't have SPDIF output/input, you need to add your card in
USB-AUDIO.conf

  # If a device does not use the first PCM device for digital data,
the device
  # number for the iec958 device can be changed here.
  USB-Audio.pcm.iec958_device {
 # "NoiseBlaster 3000" 42



___
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




___
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



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH] typedefs.h: Move some typedefs to a separate file

[David Henningsson]

The relationship between sinks, sources, cards, profiles, and ports
is becoming ever more intertwined, to the point that if you try to
include one file from the other, you're likely to end up with some
weird error somewhere else.

Work around this by creating a new typedefs.h, which does not depend
on anything else, and just creates a few typedefs.

(Can be expanded with more typedefs in the future if the need arises.)

Signed-off-by: David Henningsson 
---
 src/pulsecore/card.h  |  7 +++
 src/pulsecore/client.h|  3 +--
 src/pulsecore/core.h  |  3 +--
 src/pulsecore/device-port.h   |  3 +--
 src/pulsecore/sink-input.h|  3 +--
 src/pulsecore/sink.h  |  4 +---
 src/pulsecore/source-output.h |  3 +--
 src/pulsecore/source.h|  3 +--
 src/pulsecore/typedefs.h  | 37 +
 9 files changed, 47 insertions(+), 19 deletions(-)
 create mode 100644 src/pulsecore/typedefs.h

diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
index a72c8c2..30bfc0e 100644
--- a/src/pulsecore/card.h
+++ b/src/pulsecore/card.h
@@ -20,8 +20,7 @@
   along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
 ***/
 
-typedef struct pa_card pa_card;
-
+#include 
 #include 
 #include 
 #include 
@@ -35,7 +34,7 @@ typedef enum pa_available {
 PA_AVAILABLE_YES = 2,
 } pa_available_t;
 
-typedef struct pa_card_profile {
+struct pa_card_profile {
 pa_card *card;
 char *name;
 char *description;
@@ -59,7 +58,7 @@ typedef struct pa_card_profile {
 unsigned max_source_channels;
 
 /* .. followed by some implementation specific data */
-} pa_card_profile;
+};
 
 #define PA_CARD_PROFILE_DATA(d) ((void*) ((uint8_t*) d + 
PA_ALIGN(sizeof(pa_card_profile
 
diff --git a/src/pulsecore/client.h b/src/pulsecore/client.h
index cd55348..eb8173d 100644
--- a/src/pulsecore/client.h
+++ b/src/pulsecore/client.h
@@ -22,8 +22,7 @@
 
 #include 
 
-typedef struct pa_client pa_client;
-
+#include 
 #include 
 #include 
 #include 
diff --git a/src/pulsecore/core.h b/src/pulsecore/core.h
index 6a8affc..aefc1eb 100644
--- a/src/pulsecore/core.h
+++ b/src/pulsecore/core.h
@@ -20,12 +20,11 @@
   along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
 ***/
 
+#include 
 #include 
 #include 
 #include 
 
-typedef struct pa_core pa_core;
-
 /* This is a bitmask that encodes the cause why a sink/source is
  * suspended. */
 typedef enum pa_suspend_cause {
diff --git a/src/pulsecore/device-port.h b/src/pulsecore/device-port.h
index e3224fd..85c41fa 100644
--- a/src/pulsecore/device-port.h
+++ b/src/pulsecore/device-port.h
@@ -22,14 +22,13 @@
   along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
 ***/
 
-typedef struct pa_device_port pa_device_port;
-
 #ifdef HAVE_CONFIG_H
 #include 
 #endif
 
 #include 
 
+#include 
 #include 
 #include 
 #include 
diff --git a/src/pulsecore/sink-input.h b/src/pulsecore/sink-input.h
index 457f018..86deab2 100644
--- a/src/pulsecore/sink-input.h
+++ b/src/pulsecore/sink-input.h
@@ -23,8 +23,7 @@
 
 #include 
 
-typedef struct pa_sink_input pa_sink_input;
-
+#include 
 #include 
 #include 
 #include 
diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
index 3ae8240..5df109e 100644
--- a/src/pulsecore/sink.h
+++ b/src/pulsecore/sink.h
@@ -21,11 +21,9 @@
   along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
 ***/
 
-typedef struct pa_sink pa_sink;
-typedef struct pa_sink_volume_change pa_sink_volume_change;
-
 #include 
 
+#include 
 #include 
 #include 
 #include 
diff --git a/src/pulsecore/source-output.h b/src/pulsecore/source-output.h
index 24555e4..26be484 100644
--- a/src/pulsecore/source-output.h
+++ b/src/pulsecore/source-output.h
@@ -22,8 +22,7 @@
 
 #include 
 
-typedef struct pa_source_output pa_source_output;
-
+#include 
 #include 
 #include 
 #include 
diff --git a/src/pulsecore/source.h b/src/pulsecore/source.h
index 9ee0783..19fb41b 100644
--- a/src/pulsecore/source.h
+++ b/src/pulsecore/source.h
@@ -21,11 +21,10 @@
   along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
 ***/
 
-typedef struct pa_source pa_source;
-typedef struct pa_source_volume_change pa_source_volume_change;
 
 #include 
 
+#include 
 #include 
 #include 
 #include 
diff --git a/src/pulsecore/typedefs.h b/src/pulsecore/typedefs.h
new file mode 100644
index 000..3652f8f
--- /dev/null
+++ b/src/pulsecore/typedefs.h
@@ -0,0 +1,37 @@
+#ifndef footypedefshfoo
+#define footypedefshfoo
+
+/***
+  This file is part of PulseAudio.
+
+  Copyright 2015 Canonical Ltd.
+  Written by David Henningsson 
+
+  PulseAudio is free software; you can redistribute it and/or modify
+  it under the terms of the GNU Lesser General Public License as published
+  by the Free Software Foundation; either version 2.1 of the License,
+  or (at your option) any later version.
+
+  PulseAudio is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY

[pulseaudio-discuss] [PATCH] card: Only update port's preferred profile if profile is saved

[David Henningsson]

In case pa_card_set_profile is called with save=false, then probably
it makes more sense not to update the port's preferred profile as well.

Signed-off-by: David Henningsson 
---
 src/pulsecore/card.c | 27 ++-
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index 0ca78bb..b6cbbf7 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -258,12 +258,22 @@ static const char* profile_name_for_dir(pa_card_profile 
*cp, pa_direction_t dir)
 return cp->name;
 }
 
-int pa_card_set_profile(pa_card *c, pa_card_profile *profile, bool save) {
-int r;
+static void update_port_preferred_profile(pa_card *c) {
 pa_sink *sink;
 pa_source *source;
 uint32_t state;
 
+PA_IDXSET_FOREACH(sink, c->sinks, state)
+if (sink->active_port)
+pa_device_port_set_preferred_profile(sink->active_port, 
profile_name_for_dir(c->active_profile, PA_DIRECTION_OUTPUT));
+PA_IDXSET_FOREACH(source, c->sources, state)
+if (source->active_port)
+pa_device_port_set_preferred_profile(source->active_port, 
profile_name_for_dir(c->active_profile, PA_DIRECTION_INPUT));
+}
+
+int pa_card_set_profile(pa_card *c, pa_card_profile *profile, bool save) {
+int r;
+
 pa_assert(c);
 pa_assert(profile);
 pa_assert(profile->card == c);
@@ -274,7 +284,10 @@ int pa_card_set_profile(pa_card *c, pa_card_profile 
*profile, bool save) {
 }
 
 if (c->active_profile == profile) {
-c->save_profile = c->save_profile || save;
+if (save && !c->save_profile) {
+update_port_preferred_profile(c);
+c->save_profile = true;
+}
 return 0;
 }
 
@@ -288,12 +301,8 @@ int pa_card_set_profile(pa_card *c, pa_card_profile 
*profile, bool save) {
 c->active_profile = profile;
 c->save_profile = save;
 
-PA_IDXSET_FOREACH(sink, c->sinks, state)
-if (sink->active_port)
-pa_device_port_set_preferred_profile(sink->active_port, 
profile_name_for_dir(profile, PA_DIRECTION_OUTPUT));
-PA_IDXSET_FOREACH(source, c->sources, state)
-if (source->active_port)
-pa_device_port_set_preferred_profile(source->active_port, 
profile_name_for_dir(profile, PA_DIRECTION_INPUT));
+if (save)
+update_port_preferred_profile(c);
 
 pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
 
-- 
1.9.1

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

Re: [pulseaudio-discuss] [PATCH] alsa-mixer: sb-omni-surround-5.1.conf: remove analog-surround-21, add Linux 4.3+ support

[David Henningsson]

Hi and thanks for looking after your sound card :-)

Just a question w r t the 2.1 stuff - what alsa-lib version are you 
using? Any release from 2015 should be fine, there was some improvements 
during 2014 to the 2.1 stuff in specific.


On 2015-11-25 09:06, Nazar Mokrynskyi wrote:

Just want to remind once again, that without this patch microphone is
not working with Linux kernel 4.3+

Sincerely, Nazar Mokrynskyi
github.com/nazar-pc
Skype: nazar-pc
Diaspora:naza...@diaspora.mokrynskyi.com
Tox: 
A9D95C9AA5F7A3ED75D83D0292E22ACE84BA40E912185939414475AF28FD2B2A5C8EF5261249

On 11.10.15 17:52, Nazar Mokrynskyi wrote:

> Do you mean 2.1 mode won't work with all usb audio since surround
5.1 of usb audio is already using route plugin
What do you mean by "all usb audio"? In 4.1/5.1 mode LFE and all other
outputs work properly, it is just for 2.1, which works as stereo.

> If you don't have SPDIF output/input, you need to add your card in
USB-AUDIO.conf
Sound card has S/PDIF output, but not input. Not sure whether output
works properly though. I do not have speakers or any other hardware
with S/PDIF input to test it.
Sincerely, Nazar Mokrynskyi
github.com/nazar-pc
Skype: nazar-pc
Diaspora:naza...@diaspora.mokrynskyi.com
Tox: 
A9D95C9AA5F7A3ED75D83D0292E22ACE84BA40E912185939414475AF28FD2B2A5C8EF5261249
On 11.10.15 10:34, Raymond Yau wrote:


>
> In 2.1 mode LFE is not actually working at all, so it is removed.

Do you mean 2.1 mode won't work with all usb audio since surround 5.1
of usb audio is already using route plugin

USB-Audio.pcm.surround51.0 {
@args [ CARD ]
@args.CARD { type string }
@func refer
name {
@func concat
strings [
"cards.USB-Audio."
{ @func card_name card $CARD }
".pcm.surround51:CARD=" $CARD
]
}
default {
type route
ttable.0.0 1
ttable.1.1 1
ttable.2.4 1
ttable.3.5 1
ttable.4.2 1
ttable.5.3 1
slave {
pcm {
type hw
card $CARD
device 0
}
channels 6
}
}
}

http://git.alsa-project.org/?p=alsa-lib.git;a=blobdiff;f=src/conf/cards/USB-Audio.conf;h=ce3ae019f7f6fa91ca224074d12891217f242300;hp=8a6d9cac6ead765108e09c61914a84d290482556;hb=1af088e39b75a0a0897c7036487b143e983cd423;hpb=57b5076c30b3453ee843912c0aeb3df8dbee3f68

> With Linux 4.3-rc1+ Mic/Line are hw:%f,0,0 as it should be:
https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/sound/usb?id=5ee20bc792467d7d612157e0a9962765aa943b08
> So now we support both Linux 4.2.x- and 4.3-rc1+ setups.
> Also in Linux 4.3-rc1 S/PDIF input was detected incorrectly (there
is no such hardware input), so it is not present in config.
> ---

https://bugs.launchpad.net/ubuntu/+source/alsa-driver/+bug/1384952

Seem all usb audio with more than two playback devices are affected
since capture device of  U7 is also device 1 before the patch

If you don't have SPDIF output/input, you need to add your card in
USB-AUDIO.conf

  # If a device does not use the first PCM device for digital data,
the device
  # number for the iec958 device can be changed here.
  USB-Audio.pcm.iec958_device {
 # "NoiseBlaster 3000" 42



___
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




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



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

[pulseaudio-discuss] [PATCH v3 6/8] module-switch-on-port-available: Use input and output names

[David Henningsson]

In case input or output names are filled in, we can use this to
get a better match in the profile_good_for_input/output functions
instead of guessing based on number of sources and channels.

Signed-off-by: David Henningsson 
---
 src/modules/module-switch-on-port-available.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/modules/module-switch-on-port-available.c 
b/src/modules/module-switch-on-port-available.c
index eb8f2d7..8de68a3 100644
--- a/src/modules/module-switch-on-port-available.c
+++ b/src/modules/module-switch-on-port-available.c
@@ -23,6 +23,7 @@
 #endif
 
 #include 
+#include 
 #include 
 #include 
 
@@ -34,6 +35,9 @@ static bool profile_good_for_output(pa_card_profile *profile) 
{
 
 pa_assert(profile);
 
+if (!pa_safe_streq(profile->card->active_profile->input_name, 
profile->input_name))
+return false;
+
 if (profile->card->active_profile->n_sources != profile->n_sources)
 return false;
 
@@ -55,6 +59,9 @@ static bool profile_good_for_output(pa_card_profile *profile) 
{
 static bool profile_good_for_input(pa_card_profile *profile) {
 pa_assert(profile);
 
+if (!pa_safe_streq(profile->card->active_profile->output_name, 
profile->output_name))
+return false;
+
 if (profile->card->active_profile->n_sinks != profile->n_sinks)
 return false;
 
-- 
1.9.1

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

[pulseaudio-discuss] [PATCH v3 2/8] alsa-mixer: Fill in input and output names

[David Henningsson]

Fill in input_name and output_name to make routing easier for
routing modules.

Signed-off-by: David Henningsson 
---
 src/modules/alsa/alsa-mixer.c   | 8 +++-
 src/modules/alsa/alsa-mixer.h   | 3 +++
 src/modules/alsa/module-alsa-card.c | 2 ++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
index af7adea..3f51717 100644
--- a/src/modules/alsa/alsa-mixer.c
+++ b/src/modules/alsa/alsa-mixer.c
@@ -3473,6 +3473,8 @@ static void profile_free(pa_alsa_profile *p) {
 
 pa_xfree(p->name);
 pa_xfree(p->description);
+pa_xfree(p->input_name);
+pa_xfree(p->output_name);
 
 pa_xstrfreev(p->input_mapping_names);
 pa_xstrfreev(p->output_mapping_names);
@@ -4123,6 +4125,7 @@ static void profile_set_add_auto_pair(
 p->name = name;
 
 if (m) {
+p->output_name = pa_xstrdup(m->name);
 p->output_mappings = pa_idxset_new(pa_idxset_trivial_hash_func, 
pa_idxset_trivial_compare_func);
 pa_idxset_put(p->output_mappings, m, NULL);
 p->priority += m->priority * 100;
@@ -4130,6 +4133,7 @@ static void profile_set_add_auto_pair(
 }
 
 if (n) {
+p->input_name = pa_xstrdup(n->name);
 p->input_mappings = pa_idxset_new(pa_idxset_trivial_hash_func, 
pa_idxset_trivial_compare_func);
 pa_idxset_put(p->input_mappings, n, NULL);
 p->priority += n->priority;
@@ -4292,9 +4296,11 @@ void pa_alsa_profile_dump(pa_alsa_profile *p) {
 pa_alsa_mapping *m;
 pa_assert(p);
 
-pa_log_debug("Profile %s (%s), priority=%u, supported=%s 
n_input_mappings=%u, n_output_mappings=%u",
+pa_log_debug("Profile %s (%s), input=%s, output=%s priority=%u, 
supported=%s n_input_mappings=%u, n_output_mappings=%u",
  p->name,
  pa_strnull(p->description),
+ pa_strnull(p->input_name),
+ pa_strnull(p->output_name),
  p->priority,
  pa_yes_no(p->supported),
  p->input_mappings ? pa_idxset_size(p->input_mappings) : 0,
diff --git a/src/modules/alsa/alsa-mixer.h b/src/modules/alsa/alsa-mixer.h
index 621a71b..4ebf192 100644
--- a/src/modules/alsa/alsa-mixer.h
+++ b/src/modules/alsa/alsa-mixer.h
@@ -293,6 +293,9 @@ struct pa_alsa_profile {
 char *description;
 unsigned priority;
 
+char *input_name;
+char *output_name;
+
 bool supported:1;
 bool fallback_input:1;
 bool fallback_output:1;
diff --git a/src/modules/alsa/module-alsa-card.c 
b/src/modules/alsa/module-alsa-card.c
index e173e51..286cfc9 100644
--- a/src/modules/alsa/module-alsa-card.c
+++ b/src/modules/alsa/module-alsa-card.c
@@ -143,6 +143,8 @@ static void add_profiles(struct userdata *u, pa_hashmap *h, 
pa_hashmap *ports) {
 
 cp = pa_card_profile_new(ap->name, ap->description, sizeof(struct 
profile_data));
 cp->priority = ap->priority;
+cp->input_name = pa_xstrdup(ap->input_name);
+cp->output_name = pa_xstrdup(ap->output_name);
 
 if (ap->output_mappings) {
 cp->n_sinks = pa_idxset_size(ap->output_mappings);
-- 
1.9.1

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

[pulseaudio-discuss] [PATCH v3 7/8] module-switch-on-port-available: Route to preferred profile

[David Henningsson]

This makes the routing slightly more aggressive:

 * It will try to route to another profile, if such a profile
   is preferred by the port.

 * It will allow changing profiles on transitions both to
   PA_AVAILABLE_YES and PA_AVAILABLE_NO

To accommodate there is also some refactoring.

Signed-off-by: David Henningsson 
---
 src/modules/module-switch-on-port-available.c | 172 --
 1 file changed, 105 insertions(+), 67 deletions(-)

diff --git a/src/modules/module-switch-on-port-available.c 
b/src/modules/module-switch-on-port-available.c
index 8de68a3..5dd9786 100644
--- a/src/modules/module-switch-on-port-available.c
+++ b/src/modules/module-switch-on-port-available.c
@@ -74,22 +74,25 @@ static bool profile_good_for_input(pa_card_profile 
*profile) {
 static int try_to_switch_profile(pa_device_port *port) {
 pa_card_profile *best_profile = NULL, *profile;
 void *state;
+unsigned best_prio = 0;
 
-pa_log_debug("Finding best profile");
+pa_log_debug("Finding best profile for port %s, preferred = %s",
+ port->name, pa_strnull(port->preferred_profile));
 
 PA_HASHMAP_FOREACH(profile, port->profiles, state) {
 bool good = false;
-
-if (best_profile && best_profile->priority >= profile->priority)
-continue;
+const char *name;
+unsigned prio = profile->priority;
 
 /* We make a best effort to keep other direction unchanged */
 switch (port->direction) {
 case PA_DIRECTION_OUTPUT:
+name = profile->output_name;
 good = profile_good_for_output(profile);
 break;
 
 case PA_DIRECTION_INPUT:
+name = profile->input_name;
 good = profile_good_for_input(profile);
 break;
 }
@@ -97,7 +100,15 @@ static int try_to_switch_profile(pa_device_port *port) {
 if (!good)
 continue;
 
+/* Give a high bonus in case this is the preferred profile */
+if (port->preferred_profile && pa_streq(name ? name : profile->name, 
port->preferred_profile))
+prio += 100;
+
+if (best_profile && best_prio >= prio)
+continue;
+
 best_profile = profile;
+best_prio = prio;
 }
 
 if (!best_profile) {
@@ -113,98 +124,125 @@ static int try_to_switch_profile(pa_device_port *port) {
 return 0;
 }
 
-static void find_sink_and_source(pa_card *card, pa_device_port *port, pa_sink 
**si, pa_source **so) {
-pa_sink *sink = NULL;
-pa_source *source = NULL;
+struct port_pointers {
+pa_device_port *port;
+pa_sink *sink;
+pa_source *source;
+bool is_possible_profile_active;
+bool is_preferred_profile_active;
+bool is_port_active;
+};
+
+static const char* profile_name_for_dir(pa_card_profile *cp, pa_direction_t 
dir) {
+if (dir == PA_DIRECTION_OUTPUT && cp->output_name)
+return cp->output_name;
+if (dir == PA_DIRECTION_INPUT && cp->input_name)
+return cp->input_name;
+return cp->name;
+}
+
+static struct port_pointers find_port_pointers(pa_device_port *port) {
+struct port_pointers pp = { .port = port };
 uint32_t state;
+pa_card *card;
+
+pa_assert(port);
+pa_assert_se(card = port->card);
 
 switch (port->direction) {
 case PA_DIRECTION_OUTPUT:
-PA_IDXSET_FOREACH(sink, card->sinks, state)
-if (port == pa_hashmap_get(sink->ports, port->name))
+PA_IDXSET_FOREACH(pp.sink, card->sinks, state)
+if (port == pa_hashmap_get(pp.sink->ports, port->name))
 break;
 break;
 
 case PA_DIRECTION_INPUT:
-PA_IDXSET_FOREACH(source, card->sources, state)
-if (port == pa_hashmap_get(source->ports, port->name))
+PA_IDXSET_FOREACH(pp.source, card->sources, state)
+if (port == pa_hashmap_get(pp.source->ports, port->name))
 break;
 break;
 }
 
-*si = sink;
-*so = source;
-}
+pp.is_possible_profile_active =
+card->active_profile == pa_hashmap_get(port->profiles, 
card->active_profile->name);
+pp.is_preferred_profile_active = pp.is_possible_profile_active && 
(!port->preferred_profile ||
+pa_safe_streq(port->preferred_profile, 
profile_name_for_dir(card->active_profile, port->direction)));
+pp.is_port_active = (pp.sink && pp.sink->active_port == port) || 
(pp.source && pp.source->active_port == port);
 
-static pa_hook_result_t port_available_hook_callback(pa_core *c, 
pa_device_port *port, void* userdata) {
-pa_card* card;
-pa_sink *sink;
-pa_source *source;
-bool is_active_profile, is_active_port;
+r

[pulseaudio-discuss] [PATCH v3 1/8] card: Add variables for splitting up a profile

[David Henningsson]

It can be useful for routing modules to know a profile's input
and output parts, in order to e g change output profile
while keeping the input profile unchanged.

For now filling in these fields is optional and a routing module
must be able to handle NULL in these fields.

Signed-off-by: David Henningsson 
---
 src/pulsecore/card.c | 2 ++
 src/pulsecore/card.h | 8 
 2 files changed, 10 insertions(+)

diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index c8b97b7..bb21d0f 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -52,6 +52,8 @@ pa_card_profile *pa_card_profile_new(const char *name, const 
char *description,
 void pa_card_profile_free(pa_card_profile *c) {
 pa_assert(c);
 
+pa_xfree(c->input_name);
+pa_xfree(c->output_name);
 pa_xfree(c->name);
 pa_xfree(c->description);
 pa_xfree(c);
diff --git a/src/pulsecore/card.h b/src/pulsecore/card.h
index 3e2c004..a72c8c2 100644
--- a/src/pulsecore/card.h
+++ b/src/pulsecore/card.h
@@ -40,6 +40,14 @@ typedef struct pa_card_profile {
 char *name;
 char *description;
 
+/* Identifiers for the profile's input and output parts, i e, if two 
different profiles
+   have the same input_name string, they have the same source(s).
+   Same for output_name and sink(s).
+   Can be NULL (and in case of an input- or output- only profile, the 
other direction
+   will be NULL). */
+char *input_name;
+char *output_name;
+
 unsigned priority;
 pa_available_t available; /* PA_AVAILABLE_UNKNOWN, PA_AVAILABLE_NO or 
PA_AVAILABLE_YES */
 
-- 
1.9.1

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

[pulseaudio-discuss] [PATCH v3 0/8] Save/restore profile per port

[David Henningsson]

Changes since v2:
  * Added patch to simplify version handling in module-card-restore
  * Don't fill input/output_name for non-auto-profile cases
  * Changed comment for input/output_name

David Henningsson (8):
  card: Add variables for splitting up a profile
  alsa-mixer: Fill in input and output names
  device-port: Add preferred_profile field to pa_device_port
  card: Update preferred_profile for ports when profile changes
  card-restore: Save and restore "preferred profile" of port
  module-switch-on-port-available: Use input and output names
  module-switch-on-port-available: Route to preferred profile
  module-card-restore: Remove "version" from internal entry struct

 src/modules/alsa/alsa-mixer.c |   8 +-
 src/modules/alsa/alsa-mixer.h |   3 +
 src/modules/alsa/module-alsa-card.c   |   2 +
 src/modules/module-card-restore.c |  58 +++--
 src/modules/module-switch-on-port-available.c | 179 --
 src/pulsecore/card.c  |  20 +++
 src/pulsecore/card.h  |   8 ++
 src/pulsecore/device-port.c   |  11 ++
 src/pulsecore/device-port.h   |   2 +
 9 files changed, 212 insertions(+), 79 deletions(-)

-- 
1.9.1

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

[pulseaudio-discuss] [PATCH v3 4/8] card: Update preferred_profile for ports when profile changes

[David Henningsson]

Signed-off-by: David Henningsson 
---
 src/pulsecore/card.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/src/pulsecore/card.c b/src/pulsecore/card.c
index bb21d0f..0ca78bb 100644
--- a/src/pulsecore/card.c
+++ b/src/pulsecore/card.c
@@ -250,8 +250,19 @@ void pa_card_add_profile(pa_card *c, pa_card_profile 
*profile) {
 pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_ADDED], profile);
 }
 
+static const char* profile_name_for_dir(pa_card_profile *cp, pa_direction_t 
dir) {
+if (dir == PA_DIRECTION_OUTPUT && cp->output_name)
+return cp->output_name;
+if (dir == PA_DIRECTION_INPUT && cp->input_name)
+return cp->input_name;
+return cp->name;
+}
+
 int pa_card_set_profile(pa_card *c, pa_card_profile *profile, bool save) {
 int r;
+pa_sink *sink;
+pa_source *source;
+uint32_t state;
 
 pa_assert(c);
 pa_assert(profile);
@@ -277,6 +288,13 @@ int pa_card_set_profile(pa_card *c, pa_card_profile 
*profile, bool save) {
 c->active_profile = profile;
 c->save_profile = save;
 
+PA_IDXSET_FOREACH(sink, c->sinks, state)
+if (sink->active_port)
+pa_device_port_set_preferred_profile(sink->active_port, 
profile_name_for_dir(profile, PA_DIRECTION_OUTPUT));
+PA_IDXSET_FOREACH(source, c->sources, state)
+if (source->active_port)
+pa_device_port_set_preferred_profile(source->active_port, 
profile_name_for_dir(profile, PA_DIRECTION_INPUT));
+
 pa_hook_fire(&c->core->hooks[PA_CORE_HOOK_CARD_PROFILE_CHANGED], c);
 
 return 0;
-- 
1.9.1

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

[pulseaudio-discuss] [PATCH v3 8/8] module-card-restore: Remove "version" from internal entry struct

[David Henningsson]

If we always write entries of the latest version, we can simplify
code a little by only handling old versions in the "entry_read"
function and assume we have the latest version everywhere else.

Suggested-by: Tanu Kaskinen 
Signed-off-by: David Henningsson 
---
 src/modules/module-card-restore.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/src/modules/module-card-restore.c 
b/src/modules/module-card-restore.c
index b30fa36..f906843 100644
--- a/src/modules/module-card-restore.c
+++ b/src/modules/module-card-restore.c
@@ -73,7 +73,6 @@ struct port_info {
 };
 
 struct entry {
-uint8_t version;
 char *profile;
 pa_hashmap *ports; /* Port name -> struct port_info */
 };
@@ -110,7 +109,6 @@ static void port_info_free(struct port_info *p_info) {
 
 static struct entry* entry_new(void) {
 struct entry *r = pa_xnew0(struct entry, 1);
-r->version = ENTRY_VERSION;
 r->ports = pa_hashmap_new_full(pa_idxset_string_hash_func, 
pa_idxset_string_compare_func, NULL, (pa_free_cb_t) port_info_free);
 return r;
 }
@@ -193,15 +191,14 @@ static bool entry_write(struct userdata *u, const char 
*name, const struct entry
 pa_assert(e);
 
 t = pa_tagstruct_new();
-pa_tagstruct_putu8(t, e->version);
+pa_tagstruct_putu8(t, ENTRY_VERSION);
 pa_tagstruct_puts(t, e->profile);
 pa_tagstruct_putu32(t, pa_hashmap_size(e->ports));
 
 PA_HASHMAP_FOREACH(p_info, e->ports, state) {
 pa_tagstruct_puts(t, p_info->name);
 pa_tagstruct_puts64(t, p_info->offset);
-if (e->version >= 3)
-pa_tagstruct_puts(t, p_info->profile);
+pa_tagstruct_puts(t, p_info->profile);
 }
 
 key.data = (char *) name;
@@ -258,6 +255,7 @@ static struct entry* entry_read(struct userdata *u, const 
char *name) {
 struct entry *e = NULL;
 pa_tagstruct *t = NULL;
 const char* profile;
+uint8_t version;
 
 pa_assert(u);
 pa_assert(name);
@@ -275,8 +273,8 @@ static struct entry* entry_read(struct userdata *u, const 
char *name) {
 t = pa_tagstruct_new_fixed(data.data, data.size);
 e = entry_new();
 
-if (pa_tagstruct_getu8(t, &e->version) < 0 ||
-e->version > ENTRY_VERSION ||
+if (pa_tagstruct_getu8(t, &version) < 0 ||
+version > ENTRY_VERSION ||
 pa_tagstruct_gets(t, &profile) < 0) {
 
 goto fail;
@@ -287,7 +285,7 @@ static struct entry* entry_read(struct userdata *u, const 
char *name) {
 
 e->profile = pa_xstrdup(profile);
 
-if (e->version >= 2) {
+if (version >= 2) {
 uint32_t port_count = 0;
 const char *port_name = NULL, *profile_name = NULL;
 int64_t port_offset = 0;
@@ -303,7 +301,7 @@ static struct entry* entry_read(struct userdata *u, const 
char *name) {
 pa_hashmap_get(e->ports, port_name) ||
 pa_tagstruct_gets64(t, &port_offset) < 0)
 goto fail;
-if (e->version >= 3 && pa_tagstruct_gets(t, &profile_name) < 0)
+if (version >= 3 && pa_tagstruct_gets(t, &profile_name) < 0)
 goto fail;
 
 p_info = port_info_new(NULL);
@@ -399,7 +397,6 @@ static void update_profile_for_port(struct entry *entry, 
pa_card *card, pa_devic
 if (!pa_safe_streq(p_info->profile, p->preferred_profile)) {
 pa_xfree(p_info->profile);
 p_info->profile = pa_xstrdup(p->preferred_profile);
-entry->version = ENTRY_VERSION;
 pa_log_info("Storing profile %s for port %s on card %s.", 
p_info->profile, p->name, card->name);
 }
 }
-- 
1.9.1

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

[pulseaudio-discuss] [PATCH v3 3/8] device-port: Add preferred_profile field to pa_device_port

[David Henningsson]

Signed-off-by: David Henningsson 
---
 src/pulsecore/device-port.c | 11 +++
 src/pulsecore/device-port.h |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/src/pulsecore/device-port.c b/src/pulsecore/device-port.c
index 906ab1f..5807d3e 100644
--- a/src/pulsecore/device-port.c
+++ b/src/pulsecore/device-port.c
@@ -21,6 +21,7 @@
 
 #include "device-port.h"
 #include 
+#include 
 
 PA_DEFINE_PUBLIC_CLASS(pa_device_port, pa_object);
 
@@ -65,6 +66,15 @@ void pa_device_port_new_data_done(pa_device_port_new_data 
*data) {
 pa_xfree(data->description);
 }
 
+void pa_device_port_set_preferred_profile(pa_device_port *p, const char 
*new_pp) {
+pa_assert(p);
+
+if (!pa_safe_streq(p->preferred_profile, new_pp)) {
+pa_xfree(p->preferred_profile);
+p->preferred_profile = pa_xstrdup(new_pp);
+}
+}
+
 void pa_device_port_set_available(pa_device_port *p, pa_available_t status) {
 pa_assert(p);
 
@@ -100,6 +110,7 @@ static void device_port_free(pa_object *o) {
 if (p->profiles)
 pa_hashmap_free(p->profiles);
 
+pa_xfree(p->preferred_profile);
 pa_xfree(p->name);
 pa_xfree(p->description);
 pa_xfree(p);
diff --git a/src/pulsecore/device-port.h b/src/pulsecore/device-port.h
index f35d07c..e3224fd 100644
--- a/src/pulsecore/device-port.h
+++ b/src/pulsecore/device-port.h
@@ -43,6 +43,7 @@ struct pa_device_port {
 
 char *name;
 char *description;
+char *preferred_profile;
 
 unsigned priority;
 pa_available_t available; /* PA_AVAILABLE_UNKNOWN, PA_AVAILABLE_NO 
or PA_AVAILABLE_YES */
@@ -80,6 +81,7 @@ pa_device_port *pa_device_port_new(pa_core *c, 
pa_device_port_new_data *data, si
 void pa_device_port_set_available(pa_device_port *p, pa_available_t available);
 
 void pa_device_port_set_latency_offset(pa_device_port *p, int64_t offset);
+void pa_device_port_set_preferred_profile(pa_device_port *p, const char 
*new_pp);
 
 pa_device_port *pa_device_port_find_best(pa_hashmap *ports);
 
-- 
1.9.1

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

[pulseaudio-discuss] [PATCH v3 5/8] card-restore: Save and restore "preferred profile" of port

[David Henningsson]

Signed-off-by: David Henningsson 
---
 src/modules/module-card-restore.c | 49 +++
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/src/modules/module-card-restore.c 
b/src/modules/module-card-restore.c
index 3725d30..b30fa36 100644
--- a/src/modules/module-card-restore.c
+++ b/src/modules/module-card-restore.c
@@ -64,11 +64,12 @@ struct userdata {
 pa_database *database;
 };
 
-#define ENTRY_VERSION 2
+#define ENTRY_VERSION 3
 
 struct port_info {
 char *name;
 int64_t offset;
+char *profile;
 };
 
 struct entry {
@@ -102,6 +103,7 @@ static void trigger_save(struct userdata *u) {
 static void port_info_free(struct port_info *p_info) {
 pa_assert(p_info);
 
+pa_xfree(p_info->profile);
 pa_xfree(p_info->name);
 pa_xfree(p_info);
 }
@@ -117,9 +119,11 @@ static struct port_info *port_info_new(pa_device_port 
*port) {
 struct port_info *p_info;
 
 if (port) {
-p_info = pa_xnew(struct port_info, 1);
+p_info = pa_xnew0(struct port_info, 1);
 p_info->name = pa_xstrdup(port->name);
 p_info->offset = port->latency_offset;
+if (port->preferred_profile)
+p_info->profile = pa_xstrdup(port->preferred_profile);
 } else
 p_info = pa_xnew0(struct port_info, 1);
 
@@ -196,6 +200,8 @@ static bool entry_write(struct userdata *u, const char 
*name, const struct entry
 PA_HASHMAP_FOREACH(p_info, e->ports, state) {
 pa_tagstruct_puts(t, p_info->name);
 pa_tagstruct_puts64(t, p_info->offset);
+if (e->version >= 3)
+pa_tagstruct_puts(t, p_info->profile);
 }
 
 key.data = (char *) name;
@@ -283,7 +289,7 @@ static struct entry* entry_read(struct userdata *u, const 
char *name) {
 
 if (e->version >= 2) {
 uint32_t port_count = 0;
-const char *port_name = NULL;
+const char *port_name = NULL, *profile_name = NULL;
 int64_t port_offset = 0;
 struct port_info *p_info;
 unsigned i;
@@ -297,10 +303,14 @@ static struct entry* entry_read(struct userdata *u, const 
char *name) {
 pa_hashmap_get(e->ports, port_name) ||
 pa_tagstruct_gets64(t, &port_offset) < 0)
 goto fail;
+if (e->version >= 3 && pa_tagstruct_gets(t, &profile_name) < 0)
+goto fail;
 
 p_info = port_info_new(NULL);
 p_info->name = pa_xstrdup(port_name);
 p_info->offset = port_offset;
+if (profile_name)
+p_info->profile = pa_xstrdup(profile_name);
 
 pa_assert_se(pa_hashmap_put(e->ports, p_info->name, p_info) >= 0);
 }
@@ -375,8 +385,30 @@ finish:
 return PA_HOOK_OK;
 }
 
+static void update_profile_for_port(struct entry *entry, pa_card *card, 
pa_device_port *p) {
+struct port_info *p_info;
+
+if (p == NULL)
+return;
+
+if (!(p_info = pa_hashmap_get(entry->ports, p->name))) {
+p_info = port_info_new(p);
+pa_assert_se(pa_hashmap_put(entry->ports, p_info->name, p_info) >= 0);
+}
+
+if (!pa_safe_streq(p_info->profile, p->preferred_profile)) {
+pa_xfree(p_info->profile);
+p_info->profile = pa_xstrdup(p->preferred_profile);
+entry->version = ENTRY_VERSION;
+pa_log_info("Storing profile %s for port %s on card %s.", 
p_info->profile, p->name, card->name);
+}
+}
+
 static pa_hook_result_t card_profile_changed_callback(pa_core *c, pa_card 
*card, struct userdata *u) {
 struct entry *entry;
+pa_sink *sink;
+pa_source *source;
+uint32_t state;
 
 pa_assert(card);
 
@@ -392,6 +424,11 @@ static pa_hook_result_t 
card_profile_changed_callback(pa_core *c, pa_card *card,
 show_full_info(card);
 }
 
+PA_IDXSET_FOREACH(sink, card->sinks, state)
+update_profile_for_port(entry, card, sink->active_port);
+PA_IDXSET_FOREACH(source, card->sources, state)
+update_profile_for_port(entry, card, source->active_port);
+
 if (entry_write(u, card->name, entry))
 trigger_save(u);
 
@@ -508,9 +545,11 @@ static pa_hook_result_t card_new_hook_callback(pa_core *c, 
pa_card_new_data *new
 pa_log_info("Restoring port latency offsets for card %s.", new_data->name);
 
 PA_HASHMAP_FOREACH(p_info, e->ports, state)
-if ((p = pa_hashmap_get(new_data->ports, p_info->name)))
+if ((p = pa_hashmap_get(new_data->ports, p_info->name))) {
 p->latency_offset = p_info->offset;
-
+if (!p->preferred_profile && p_info->profile)
+pa_device_port_set_preferred_profile(p, p_info->profile);
+}
 entry_free(e);
 
 return PA_HOOK_OK;
-- 
1.9.1

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

Re: [pulseaudio-discuss] [PATCH] pulsecore/packet: avoid redefinition of pa_packet structure

[David Henningsson]

Pushed now. Thanks for the contribution!

On 2015-11-17 12:06, Thomas Petazzoni wrote:

packet.h defines:

   typedef struct pa_packet pa_packet;

and packet.c defines:

   typedef struct pa_packet {
 ...
   } pa_packet;

With old versions of gcc (such as gcc 4.5) this causes a redefinition
error at compile time:

pulsecore/packet.c:43:3: error: redefinition of typedef 'pa_packet'
pulsecore/packet.h:26:26: note: previous declaration of 'pa_packet' was here

In order to fix this, this commit changes the definition in packet.c
to just:

   struct pa_packet {
 ...
   };

This way, the contents of the structure remain opaque to users of
pa_packet outside packet.c, and the 'pa_packet' type remains usable.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=91334

Signed-off-by: Thomas Petazzoni 
---
  src/pulsecore/packet.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/pulsecore/packet.c b/src/pulsecore/packet.c
index e275d23..2a61d58 100644
--- a/src/pulsecore/packet.c
+++ b/src/pulsecore/packet.c
@@ -32,7 +32,7 @@

  #define MAX_APPENDED_SIZE 128

-typedef struct pa_packet {
+struct pa_packet {
  PA_REFCNT_DECLARE;
  enum { PA_PACKET_APPENDED, PA_PACKET_DYNAMIC } type;
  size_t length;
@@ -40,7 +40,7 @@ typedef struct pa_packet {
  union {
  uint8_t appended[MAX_APPENDED_SIZE];
  } per_type;
-} pa_packet;
+};

  PA_STATIC_FLIST_DECLARE(packets, 0, pa_xfree);




--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [BUG?] I get a profile with 2 outputs, but if I mute headphone it mutes both ports

[David Henningsson]

On 2015-11-10 20:26, Tanu Kaskinen wrote:

On Tue, 2015-11-10 at 14:52 +0100, David Henningsson wrote:


On 2015-11-10 14:40, Tanu Kaskinen wrote:

On Thu, 2015-11-05 at 09:04 +0100, David Henningsson wrote:


On 2015-11-04 12:09, Tanu Kaskinen wrote:

On Mon, 2015-11-02 at 09:36 +0100, David Henningsson wrote:


On 2015-11-01 13:41, Tanu Kaskinen wrote:


I'm not sure if we can fix this upstream. To me it would seem logical
for the kernel to promise that "Front" only refers to line out, and if
on some machine it also affects the headphone path, then that should be
considered a kernel bug, and the kernel should rename the element.
David, what do you think?


Well, technically what comes out of a headphone are front left and front
right channels, so to some degree it makes sense that "Front" should be
a part of the headphone path.


Sure, "Front" could be defined to affect both line out and headphone
paths. The problem is that currently it seems to mean "line out only"
on some machines and "line out and headphones" on other machines. If
"Front" is defined to affect both paths, then we need some other name
for controls that only affect the line out path.


I believe the logical name for such a control would be "Line Out Front".
(Which we currently do not support in the line out path, btw, and I
haven't seen it in the wild, either.)


I suppose the kernel is usually able to automute line out when
headphones are plugged in?


Yes.


If so, then why doesn't the kernel expose
the mute control to the userspace?


I'm not sure I understand what "the mute control" refers to, but in this
case the mute control for the front line out jack is called "Front" (or
"Front Playback Switch" being the full name).


I meant the general case, where the kernel knows how to automute line
out. You said that you haven't seen "Line Out Front" anywhere. Why is
that, if the kernel knows how to mute line out?


The kernel is fully aware that the jack is a line out jack (as you can 
see on the jack kcontrol). It's just that the algorithm for naming 
volume/mute kcontrols is not trivial; on one hand, you want as short 
name as possible (max 44 characters, minus ' Playback Switch'), but 
still unique and descriptive, taking location, channel map and device 
into account. Then add that a volume/mute might control more than one, 
but not all, of the outputs...some use cases are not handled optimally.


--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Re: [pulseaudio-discuss] [BUG?] I get a profile with 2 outputs, but if I mute headphone it mutes both ports

[David Henningsson]

On 2015-11-10 14:40, Tanu Kaskinen wrote:

On Thu, 2015-11-05 at 09:04 +0100, David Henningsson wrote:


On 2015-11-04 12:09, Tanu Kaskinen wrote:

On Mon, 2015-11-02 at 09:36 +0100, David Henningsson wrote:


On 2015-11-01 13:41, Tanu Kaskinen wrote:

On Sat, 2015-10-31 at 17:37 +0100, GMAIL wrote:


Le 31/10/2015 12:19, Tanu Kaskinen a écrit :


I suspect that the "Front" volume and mute elements can be used to
control the line out without affecting the headphones. The problem is
that PulseAudio's alsa configuration assumes that the "Front" element
affects both line out and headphones (there's a comment saying that "on
some machines Front is actually a part of the Headphone path").

/usr/share/pulseaudio/alsa-mixer/paths/analog-output-headphones.conf
contains this:

 [Element Front]
 switch = mute
 volume = zero

If you change that to

 [Element Front]
 switch = off
 volume = off

does that fix the problem for you?


Indeed this fixes my problem, thanks!

I'm a bit confused by the comment in the .conf file though. So, despite
PA detecting the rear line out port exactly as it states, this "Element
Front" is actually part of the headphones "paths"? that is rear line out
is considered as part of "Element Front"?

I'm not sure what you mean with your last question. The rear line out
isn't a "part" of the "Front" mixer element, it's the other way around.
You have two paths, headphones and line out. PulseAudio considers the
"Front" mixer element to be part of both paths, even though on your
machine it's only a part of the line out path.

I'm not sure if we can fix this upstream. To me it would seem logical
for the kernel to promise that "Front" only refers to line out, and if
on some machine it also affects the headphone path, then that should be
considered a kernel bug, and the kernel should rename the element.
David, what do you think?


Well, technically what comes out of a headphone are front left and front
right channels, so to some degree it makes sense that "Front" should be
a part of the headphone path.


Sure, "Front" could be defined to affect both line out and headphone
paths. The problem is that currently it seems to mean "line out only"
on some machines and "line out and headphones" on other machines. If
"Front" is defined to affect both paths, then we need some other name
for controls that only affect the line out path.


I believe the logical name for such a control would be "Line Out Front".
(Which we currently do not support in the line out path, btw, and I
haven't seen it in the wild, either.)


I suppose the kernel is usually able to automute line out when
headphones are plugged in?


Yes.


If so, then why doesn't the kernel expose
the mute control to the userspace?


I'm not sure I understand what "the mute control" refers to, but in this 
case the mute control for the front line out jack is called "Front" (or 
"Front Playback Switch" being the full name).


In most cases, there is also an "Auto-Mute Mode" control, but that's not 
present here because the kernel cannot do the automuting.



--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss