Re: [pulseaudio-discuss] [PATCHv2 19/60] build: Make the build of bluetooth modules BlueZ 4 specific
On Tue, 2013-08-13 at 01:53 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
configure.ac| 28
src/Makefile.am | 23 ++-
2 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/configure.ac b/configure.ac
index 616a990..1c023dd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -981,21 +981,25 @@ AX_DEFINE_DIR(PA_MACHINE_ID_FALLBACK,
PA_MACHINE_ID_FALLBACK,
BlueZ support (optional, dependent on D-Bus)
-AC_ARG_ENABLE([bluez],
-AS_HELP_STRING([--disable-bluez],[Disable optional BlueZ support]))
+AC_ARG_ENABLE([bluez4],
+AS_HELP_STRING([--disable-bluez4],[Disable optional BlueZ 4 support]))
-AS_IF([test x$enable_bluez != xno],
-[PKG_CHECK_MODULES(BLUEZ, [ bluez = 4.99 ], HAVE_BLUEZ=1,
HAVE_BLUEZ=0)],
-HAVE_BLUEZ=0)
-AS_IF([test x$enable_bluez != xno],
+AS_IF([test x$enable_bluez4 != xno],
+[PKG_CHECK_MODULES(BLUEZ_4, [ bluez = 4.99 bluez 5.0 ],
HAVE_BLUEZ_4=1, HAVE_BLUEZ_4=0)],
+HAVE_BLUEZ_4=0)
This disables bluez4 support if bluez5 version of the .pc file is
installed. I thought earlier that bluez5 wouldn't install the file at
all, but I learned today that it does install it if bluez is built with
--enable-library. Let's not have the PKG_CHECK_MODULES check at all,
since we aren't using libbluetooth. Support for both versions shall be
built by default, regardless of the installed .pc file version.
+AS_IF([test x$enable_bluez4 != xno],
[PKG_CHECK_MODULES(SBC, [ sbc = 1.0 ], HAVE_SBC=1, HAVE_SBC=0)],
HAVE_SBC=0)
-AS_IF([test x$HAVE_SBC != x1], HAVE_BLUEZ=0)
-AS_IF([test x$HAVE_DBUS != x1], HAVE_BLUEZ=0)
+AS_IF([test x$HAVE_SBC != x1], HAVE_BLUEZ_4=0)
+AS_IF([test x$HAVE_DBUS != x1], HAVE_BLUEZ_4=0)
-AS_IF([test x$enable_bluez = xyes test x$HAVE_BLUEZ = x0],
-[AC_MSG_ERROR([*** BLUEZ support not found (requires BlueZ, sbc, and
D-Bus)])])
+AS_IF([test x$enable_bluez4 = xyes test x$HAVE_BLUEZ_4 = x0],
+[AC_MSG_ERROR([*** BLUEZ 4 support not found (requires bluez = 4.99 and
5.0, sbc, and D-Bus)])])
+AC_SUBST(HAVE_BLUEZ_4)
+AM_CONDITIONAL([HAVE_BLUEZ_4], [test x$HAVE_BLUEZ_4 = x1])
+
+AS_IF([test x$HAVE_BLUEZ_4 = x1], HAVE_BLUEZ=1)
AC_SUBST(HAVE_BLUEZ)
AM_CONDITIONAL([HAVE_BLUEZ], [test x$HAVE_BLUEZ = x1])
@@ -1389,7 +1393,7 @@ AS_IF([test x$HAVE_XEN = x1], ENABLE_XEN=yes,
ENABLE_XEN=no)
AS_IF([test x$HAVE_DBUS = x1], ENABLE_DBUS=yes, ENABLE_DBUS=no)
AS_IF([test x$HAVE_UDEV = x1], ENABLE_UDEV=yes, ENABLE_UDEV=no)
AS_IF([test x$HAVE_SYSTEMD = x1], ENABLE_SYSTEMD=yes, ENABLE_SYSTEMD=no)
-AS_IF([test x$HAVE_BLUEZ = x1], ENABLE_BLUEZ=yes, ENABLE_BLUEZ=no)
+AS_IF([test x$HAVE_BLUEZ_4 = x1], ENABLE_BLUEZ_4=yes, ENABLE_BLUEZ_4=no)
AS_IF([test x$HAVE_HAL_COMPAT = x1], ENABLE_HAL_COMPAT=yes,
ENABLE_HAL_COMPAT=no)
AS_IF([test x$HAVE_TCPWRAP = x1], ENABLE_TCPWRAP=yes, ENABLE_TCPWRAP=no)
AS_IF([test x$HAVE_LIBSAMPLERATE = x1], ENABLE_LIBSAMPLERATE=yes,
ENABLE_LIBSAMPLERATE=no)
@@ -1440,7 +1444,7 @@ echo
Enable LIRC: ${ENABLE_LIRC}
Enable Xen PV driver: ${ENABLE_XEN}
Enable D-Bus: ${ENABLE_DBUS}
- Enable BlueZ:${ENABLE_BLUEZ}
+ Enable BlueZ 4: ${ENABLE_BLUEZ_4}
Enable udev: ${ENABLE_UDEV}
Enable HAL-udev compat: ${ENABLE_HAL_COMPAT}
Enable systemd login: ${ENABLE_SYSTEMD}
diff --git a/src/Makefile.am b/src/Makefile.am
index ff37877..0e94725 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1320,8 +1320,12 @@ endif
if HAVE_BLUEZ
modlibexec_LTLIBRARIES += \
+ module-bluetooth-policy.la
+endif
+
+if HAVE_BLUEZ_4
+modlibexec_LTLIBRARIES += \
module-bluetooth-proximity.la \
- module-bluetooth-policy.la \
libbluez4-util.la \
module-bluez4-discover.la \
module-bluez4-device.la
@@ -2012,10 +2016,16 @@ module_bluetooth_proximity_la_LIBADD =
$(MODULE_LIBADD) $(DBUS_LIBS)
module_bluetooth_proximity_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
-DPA_BT_PROXIMITY_HELPER=\$(pulselibexecdir)/proximity-helper\
proximity_helper_SOURCES = modules/bluetooth/proximity-helper.c
-proximity_helper_LDADD = $(AM_LDADD) $(BLUEZ_LIBS)
-proximity_helper_CFLAGS = $(AM_CFLAGS) $(BLUEZ_CFLAGS)
+proximity_helper_LDADD = $(AM_LDADD) $(BLUEZ_4_LIBS)
+proximity_helper_CFLAGS = $(AM_CFLAGS) $(BLUEZ_4_CFLAGS)
Uh, so we do use libbluetooth after all.
I did some investigation, and it seems that module-bluetooth-proximity
uses BlueZ 3 APIs, meaning that it hasn't worked for a while. Nobody has
complained, so nobody is using the module. Let's remove the proximity
code.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
Re: [pulseaudio-discuss] [PATCH] bluetooth: Remove module-bluetooth-proximity
On Fri, 2013-08-16 at 09:30 -0300, jprv...@gmail.com wrote: From: João Paulo Rechi Vita jprv...@openbossa.org module-bluetooth-proximity has not worked for quite a while, since it uses pre-BlueZ4 APIs. Nobody complained since then, which is a good indication that it doesn't have much users. Even the original commit message refers to it more as a toy than as something of great use: add new fun module that automatically mutes your audio devices when you leave with your bluetooth phone, and unmutes when you come back Removing it we completely remove the dependency on libbluetooth. --- LICENSE| 20 +- po/POTFILES.in | 2 - src/.gitignore | 1 - src/Makefile.am| 20 - src/modules/bluetooth/module-bluetooth-proximity.c | 480 - src/modules/bluetooth/proximity-helper.c | 202 - 6 files changed, 10 insertions(+), 715 deletions(-) delete mode 100644 src/modules/bluetooth/module-bluetooth-proximity.c delete mode 100644 src/modules/bluetooth/proximity-helper.c Thanks, pushed to the bluez5 branch. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 21/60] bluetooth: Create stub for module-bluez5-discover
On Tue, 2013-08-13 at 01:53 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/Makefile.am| 12 +++
src/modules/bluetooth/module-bluez5-discover.c | 43
++
2 files changed, 55 insertions(+)
create mode 100644 src/modules/bluetooth/module-bluez5-discover.c
+int pa__init(pa_module* m) {
+void pa__done(pa_module* m) {
Cosmetic: should be pa_module *m. I'll fix this myself.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 23/60] bluetooth: Track org.bluez for BlueZ 5
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/bluez5-util.c | 94
+
1 file changed, 94 insertions(+)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 0f23bff..d60f63c 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -69,6 +151,18 @@ void pa_bluetooth_discovery_unref(pa_bluetooth_discovery
*y) {
if (PA_REFCNT_DEC(y) 0)
return;
+if (y-connection) {
+pa_dbus_remove_matches(pa_dbus_connection_get(y-connection),
+
type='signal',sender='org.freedesktop.DBus',interface='org.freedesktop.DBus',member='NameOwnerChanged'
+,arg0=' BLUEZ_SERVICE ',
+NULL);
The match rules should only be removed if they have been added, so you
need to keep track of that. There can be multiple instances of the same
match rule, so if bluez5-util removes a match rule that it hasn't added,
it may remove someone else's match rule (a real example is bluez4-util,
because it tracks org.bluez name owner too).
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 27/60] bluetooth: Create pa_bluetooth_transport for BlueZ 5 support
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
Create the pa_bluetooth_transport structure to store information about
the bluetooth transport and utility functions to manipulate this
structure. The acquire() and release() operations are function pointers
in the pa_bluetooth_transport structure to make possible for different
transport backends to provide different implementations of these
operations. Thre is also a userdata field for the transport backend
provide data for the acquire/release functions.
---
src/modules/bluetooth/bluez5-util.c | 145
src/modules/bluetooth/bluez5-util.h | 43 +++
2 files changed, 188 insertions(+)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 1972a92..69eb759 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -36,6 +36,7 @@
#include bluez5-util.h
#define BLUEZ_SERVICE org.bluez
+#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE .MediaTransport1
struct pa_bluetooth_discovery {
PA_REFCNT_DECLARE;
@@ -45,8 +46,131 @@ struct pa_bluetooth_discovery {
bool filter_added;
pa_hook hooks[PA_BLUETOOTH_HOOK_MAX];
pa_hashmap *devices;
+pa_hashmap *transports;
};
+pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device *d,
const char *owner, const char *path,
+
pa_bluetooth_profile_t p, const uint8_t *config, size_t size) {
Unaligned parameter list.
There is some precedence for wrapping long parameter lists like this:
pa_bluetooth_transport *pa_bluetooth_transport_new(
pa_bluetooth_device *d,
const char *owner,
const char *path,
pa_bluetooth_profile_t p,
const uint8_t *config,
size_t size) {
+pa_bluetooth_transport *t;
+
+t = pa_xnew0(pa_bluetooth_transport, 1);
+t-device = d;
+t-owner = pa_xstrdup(owner);
+t-path = pa_xstrdup(path);
+t-profile = p;
+t-config_size = size;
+
+if (size 0) {
+t-config = pa_xnew(uint8_t, size);
+memcpy(t-config, config, size);
+}
+
+pa_assert_se(pa_hashmap_put(d-discovery-transports, t-path, t) = 0);
+
+return t;
+}
+
+static void transport_state_changed(pa_bluetooth_transport *t,
pa_bluetooth_transport_state_t state) {
+bool old_any_connected;
+
+if (t-state == state)
+return;
+
+old_any_connected =
pa_bluetooth_device_any_transport_connected(t-device);
+
+pa_log_debug(Transport %s state changed from %d to %d, t-path,
t-state, state);
Human-readable states, please.
+
+t-state = state;
+
pa_hook_fire(t-device-discovery-hooks[PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED],
t);
+
+if (old_any_connected !=
pa_bluetooth_device_any_transport_connected(t-device))
undefined reference to `pa_bluetooth_device_any_transport_connected'
+
pa_hook_fire(t-device-discovery-hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED],
t-device);
+}
+
+void pa_bluetooth_transport_put(pa_bluetooth_transport *t) {
+transport_state_changed(t, PA_BLUETOOTH_TRANSPORT_STATE_IDLE);
+}
+
+void pa_bluetooth_transport_free(pa_bluetooth_transport *t) {
+pa_assert(t);
+
+pa_hashmap_remove(t-device-discovery-transports, t-path);
+pa_xfree(t-owner);
+pa_xfree(t-path);
+pa_xfree(t-config);
+pa_xfree(t);
+}
+
+static int bluez5_transport_acquire_cb(pa_bluetooth_transport *t, bool
optional, size_t *imtu, size_t *omtu) {
+DBusMessage *m, *r;
+DBusError err;
+int ret;
+uint16_t i, o;
+const char *method = optional ? TryAcquire : Acquire;
+
+pa_assert(t);
+pa_assert(t-device);
+pa_assert(t-device-discovery);
+
+pa_assert_se(m = dbus_message_new_method_call(t-owner, t-path,
BLUEZ_MEDIA_TRANSPORT_INTERFACE, method));
+
+dbus_error_init(err);
+
+r =
dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t-device-discovery-connection),
m, -1, err);
+if (!r) {
+if (optional pa_streq(err.name, org.bluez.Error.NotAvailable))
+pa_log_info(Failed optional acquire of unavailable transport
%s, t-path);
+else
+pa_log_error(Transport %s() failed for transport %s (%s),
method, t-path, err.message);
+
+dbus_error_free(err);
+return -1;
+}
+
+if (!dbus_message_get_args(r, err, DBUS_TYPE_UNIX_FD, ret,
DBUS_TYPE_UINT16, i, DBUS_TYPE_UINT16, o,
+ DBUS_TYPE_INVALID)) {
+pa_log_error(Failed to parse %s() reply: %s, method, err.message);
+dbus_error_free(err);
+ret = -1;
+goto finish;
+}
+
+if (imtu)
+*imtu = i;
+
+if (omtu)
+*omtu = o;
+
+finish:
+
Re: [pulseaudio-discuss] [PATCHv2 27/60] bluetooth: Create pa_bluetooth_transport for BlueZ 5 support
On Fri, 2013-08-16 at 18:32 +0300, Tanu Kaskinen wrote:
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
static void device_free(pa_bluetooth_device *d) {
+unsigned i;
+
pa_assert(d);
+for (i = 0; i PA_BLUETOOTH_PROFILE_COUNT; i++) {
+pa_bluetooth_transport *t;
+
+if (!(t = d-transports[i]))
+continue;
+
+d-transports[i] = NULL;
+transport_state_changed(t,
PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
+/* TODO: check if there is no risk of calling
+ * transport_connection_changed_cb() when the transport is already
freed */
+pa_bluetooth_transport_free(t);
IMO pa_bluetooth_transport_free() should be called by the transport
code, because the backend also creates the transport. The backend may
need a callback for getting notified about the device going away. Or
perhaps there should be pa_bluetooth_transport_kill(), with a kill()
callback in the backend. This would be similar to how sink inputs are
killed if the sink they're connected to goes away.
If the backend is responsible for calling pa_bluetooth_transport_free(),
the core should still ensure that t-device is set to NULL and that the
transport is removed from discovery-transports (feel free to create
pa_bluetooth_transport_unlink() for this purpose, if you want).
I'll add that it would be good to have the bluez transport backend in a
separate file, so that there would be clear separation with the
bluetooth core code and the transport backends.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 25/60] bluetooth: Create pa_bluetooth_device for BlueZ 5 support
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
+struct pa_bluetooth_device {
+pa_bluetooth_discovery *discovery;
+
+int device_info_valid; /* 0: no results yet; 1: good results; -1:
bad results ... */
+
+/* Device information */
+char *path;
+char *alias;
+char *remote;
+char *local;
I now noticed that you create adapter objects in later patches. I think
the local address should be stored in pa_bluetooth_adapter, and
pa_bluetooth_device should point to to the adapter object.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 31/60] bluetooth: Implement org.bluez.MediaEndpoint1.SelectConfiguration()
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/Makefile.am | 3 +-
src/modules/bluetooth/bluez5-util.c | 156
+++-
2 files changed, 156 insertions(+), 3 deletions(-)
diff --git a/src/Makefile.am b/src/Makefile.am
index ff8bb42..3759b3c 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2055,7 +2055,8 @@ module_bluez4_device_la_CFLAGS = $(AM_CFLAGS)
$(DBUS_CFLAGS) $(SBC_CFLAGS)
# Bluetooth BlueZ 5 sink / source
libbluez5_util_la_SOURCES = \
modules/bluetooth/bluez5-util.c \
- modules/bluetooth/bluez5-util.h
+ modules/bluetooth/bluez5-util.h \
+ modules/bluetooth/a2dp-codecs.h
libbluez5_util_la_LDFLAGS = -avoid-version
libbluez5_util_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS)
libbluez5_util_la_CFLAGS = $(AM_CFLAGS) $(DBUS_CFLAGS)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index d0428a9..1194503 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -361,6 +361,51 @@ fail:
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
+static uint8_t a2dp_default_bitpool(uint8_t freq, uint8_t mode) {
You didn't respond to my previous comment: I think this function should
have a comment explaining on what basis the return values are chosen. As
it stands, they're just random numbers to me.
+
+switch (freq) {
+case SBC_SAMPLING_FREQ_16000:
+case SBC_SAMPLING_FREQ_32000:
+return 53;
+
+case SBC_SAMPLING_FREQ_44100:
+
+switch (mode) {
+case SBC_CHANNEL_MODE_MONO:
+case SBC_CHANNEL_MODE_DUAL_CHANNEL:
+return 31;
+
+case SBC_CHANNEL_MODE_STEREO:
+case SBC_CHANNEL_MODE_JOINT_STEREO:
+return 53;
+
+default:
+pa_log_warn(Invalid channel mode %u, mode);
+return 53;
+}
+
+case SBC_SAMPLING_FREQ_48000:
+
+switch (mode) {
+case SBC_CHANNEL_MODE_MONO:
+case SBC_CHANNEL_MODE_DUAL_CHANNEL:
+return 29;
+
+case SBC_CHANNEL_MODE_STEREO:
+case SBC_CHANNEL_MODE_JOINT_STEREO:
+return 51;
+
+default:
+pa_log_warn(Invalid channel mode %u, mode);
+return 51;
+}
+
+default:
+pa_log_warn(Invalid sampling freq %u, freq);
+return 53;
+}
+}
+
const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
switch(profile) {
case PROFILE_A2DP_SINK:
@@ -536,11 +581,118 @@ fail2:
}
static DBusMessage *endpoint_select_configuration(DBusConnection *conn,
DBusMessage *m, void *userdata) {
+pa_bluetooth_discovery *y = userdata;
+a2dp_sbc_t *cap, config;
+uint8_t *pconf = (uint8_t *) config;
+int i, size;
DBusMessage *r;
+DBusError err;
-pa_assert_se(r = dbus_message_new_error(m,
BLUEZ_MEDIA_ENDPOINT_INTERFACE .Error.NotImplemented,
-Method not implemented));
+static const struct {
+uint32_t rate;
+uint8_t cap;
+} freq_table[] = {
+{ 16000U, SBC_SAMPLING_FREQ_16000 },
+{ 32000U, SBC_SAMPLING_FREQ_32000 },
+{ 44100U, SBC_SAMPLING_FREQ_44100 },
+{ 48000U, SBC_SAMPLING_FREQ_48000 }
+};
+
+dbus_error_init(err);
+
+if (!dbus_message_get_args(m, err, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE,
cap, size, DBUS_TYPE_INVALID)) {
Passing cap may cause alignment issues when accessing cap later. A byte
array pointer should be passed instead, and the memory should be copied
to an a2dp_sbc_t variable.
+pa_log_error(Endpoint SelectConfiguration(): %s, err.message);
+dbus_error_free(err);
+goto fail;
+}
+
+if (size != sizeof(config)) {
+pa_log_error(Capabilities array has invalid size);
+goto fail;
+}
+pa_zero(config);
+
+/* Find the lowest freq that is at least as high as the requested
sampling rate */
+for (i = 0; (unsigned) i PA_ELEMENTSOF(freq_table); i++)
+if (freq_table[i].rate = y-core-default_sample_spec.rate
(cap-frequency freq_table[i].cap)) {
+config.frequency = freq_table[i].cap;
+break;
+}
+
+if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
+for (--i; i = 0; i--) {
+if (cap-frequency freq_table[i].cap) {
+config.frequency = freq_table[i].cap;
+break;
+}
+}
+
+if (i 0) {
+pa_log_error(Not suitable sample rate);
+goto fail;
+
Re: [pulseaudio-discuss] [PATCHv2 32/60] bluetooth: Implement org.bluez.MediaEndpoint1.ClearConfiguration()
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/bluez5-util.c | 28 ++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 1194503..1d98174 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -697,11 +697,35 @@ fail:
}
static DBusMessage *endpoint_clear_configuration(DBusConnection *conn,
DBusMessage *m, void *userdata) {
+pa_bluetooth_discovery *y = userdata;
+pa_bluetooth_transport *t;
DBusMessage *r;
+DBusError err;
+const char *path;
-pa_assert_se(r = dbus_message_new_error(m,
BLUEZ_MEDIA_ENDPOINT_INTERFACE .Error.NotImplemented,
-Method not implemented));
+dbus_error_init(err);
+if (!dbus_message_get_args(m, err, DBUS_TYPE_OBJECT_PATH, path,
DBUS_TYPE_INVALID)) {
+pa_log_error(Endpoint ClearConfiguration(): %s, err.message);
+dbus_error_free(err);
+goto fail;
+}
+
+if ((t = pa_hashmap_get(y-transports, path))) {
+pa_log_debug(Clearing transport %s profile %s, t-path,
pa_bluetooth_profile_to_string(t-profile));
+transport_state_changed(t,
PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
+t-device-transports[t-profile] = NULL;
+/* TODO: check if there is no risk of calling
+ * transport_connection_changed_cb() when the transport is already
freed */
+pa_bluetooth_transport_free(t);
Similar to what I said about device_free(), the core code shouldn't call
pa_bluetooth_transport_free() here either. I think
pa_bluetooth_transport should have a configuration_cleared() callback,
and the transport backend can then call pa_bluetooth_transport_free() if
it wants.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 37/60] bluetooth: Parse BlueZ 5 adapter properties
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
+static int parse_adapter_properties(pa_bluetooth_discovery *y, const char
*adapter, DBusMessageIter *i) {
+DBusMessageIter element_i;
+
+pa_assert(y);
+pa_assert(adapter);
+
+dbus_message_iter_recurse(i, element_i);
+
+while (dbus_message_iter_get_arg_type(element_i) ==
DBUS_TYPE_DICT_ENTRY) {
+DBusMessageIter dict_i, variant_i;
+const char *key;
+
+dbus_message_iter_recurse(element_i, dict_i);
+
+key = check_variant_property(dict_i);
+if (key == NULL) {
+pa_log_error(Received invalid property for adapter %s,
adapter);
+return -1;
+}
+
+dbus_message_iter_recurse(dict_i, variant_i);
+
+if (dbus_message_iter_get_arg_type(variant_i) == DBUS_TYPE_STRING
pa_streq(key, Address)) {
+char *value;
+pa_bluetooth_adapter *a = pa_xnew(pa_bluetooth_adapter, 1);
+
+dbus_message_iter_get_basic(variant_i, value);
+a-path = pa_xstrdup(adapter);
+a-address = pa_xstrdup(value);
+pa_hashmap_put(y-adapters, a-path, a);
This leaks pa_bluetooth_adapter objects if bluetoothd sends duplicate
addresses. pa_bluetooth_adapter creation should be handled in the same
way as pa_bluetooth_device creation.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 39/60] bluetooth: Parse BlueZ 5 device properties
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
This code is based on previous work by Mikel Astiz.
---
src/modules/bluetooth/bluez5-util.c | 150
+++-
src/modules/bluetooth/bluez5-util.h | 1 +
2 files changed, 150 insertions(+), 1 deletion(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 1118bd0..94e3c35 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -282,6 +282,7 @@ static pa_bluetooth_device*
device_create(pa_bluetooth_discovery *y, const char
d = pa_xnew0(pa_bluetooth_device, 1);
d-discovery = y;
d-path = pa_xstrdup(path);
+d-uuids = pa_hashmap_new(pa_idxset_string_hash_func,
pa_idxset_string_compare_func);
pa_hashmap_put(y-devices, d-path, d);
@@ -336,6 +337,9 @@ static void device_free(pa_bluetooth_device *d) {
pa_bluetooth_transport_free(t);
}
+if (d-uuids)
+pa_hashmap_free(d-uuids, NULL);
+
d-discovery = NULL;
pa_xfree(d-path);
pa_xfree(d-alias);
@@ -379,6 +383,145 @@ static void adapter_remove_all(pa_bluetooth_discovery
*y) {
}
}
+static int parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i,
bool is_property_change) {
+const char *key;
+DBusMessageIter variant_i;
+
+pa_assert(d);
+
+key = check_variant_property(i);
+if (key == NULL) {
+pa_log_error(Received invalid property for device %s, d-path);
+return -1;
+}
+
+dbus_message_iter_recurse(i, variant_i);
+
+switch (dbus_message_iter_get_arg_type(variant_i)) {
+
+case DBUS_TYPE_STRING: {
+const char *value;
+dbus_message_iter_get_basic(variant_i, value);
+
+if (pa_streq(key, Alias)) {
+pa_xfree(d-alias);
+d-alias = pa_xstrdup(value);
+pa_log_debug(%s: %s, key, value);
+} else if (pa_streq(key, Address)) {
+if (is_property_change) {
+pa_log_error(Device property 'Address' expected to be
constant but changed for %s, d-path);
+return -1;
+}
+
+if (d-remote) {
+pa_log_error(Device %s: Received a duplicate 'Address'
property., d-path);
+return -1;
+}
+
+d-remote = pa_xstrdup(value);
+pa_log_debug(%s: %s, key, value);
+}
+
+break;
+}
+
+case DBUS_TYPE_OBJECT_PATH: {
+const char *value;
+dbus_message_iter_get_basic(variant_i, value);
+
+if (pa_streq(key, Adapter)) {
+pa_bluetooth_adapter *a;
+
+if (is_property_change) {
+pa_log_error(Device property 'Adapter' expected to be
constant but changed for %s, d-path);
+return -1;
+}
+
+if (d-local) {
+pa_log_error(Device %s: Received a duplicate 'Adapter'
property., d-path);
+return -1;
+}
+
+a = pa_hashmap_get(d-discovery-adapters, value);
+d-local = pa_xstrdup(a-address);
There's no guarantee that a is non-NULL. bluetoothd may send a bogus
adapter path, or since the initial information about adapters and
devices is sent in the same message, it's possible that we haven't yet
parsed the adapter object information.
This is a bit hairy situation, because we don't know whether we will see
the referenced adapter later or not, if it's missing at this point. I
think we will have to save the adapter path here, and attempt to resolve
it to a pa_bluetooth_adapter pointer after all interfaces have been
parsed.
+pa_log_debug(%s: %s, key, value);
+}
+
+break;
+}
+
+case DBUS_TYPE_UINT32: {
+uint32_t value;
+dbus_message_iter_get_basic(variant_i, value);
+
+if (pa_streq(key, Class)) {
+d-class_of_device = (int) value;
Why is class_of_device signed if the Class property is unsigned? I would
understand it if uninitialized class was signalled with a negative
value, but that doesn't seem to be done either.
+pa_log_debug(%s: %d, key, value);
+}
+
+break;
+}
+
+case DBUS_TYPE_ARRAY: {
+DBusMessageIter ai;
+dbus_message_iter_recurse(variant_i, ai);
+
+if (dbus_message_iter_get_arg_type(ai) == DBUS_TYPE_STRING
pa_streq(key, UUIDs)) {
+/* bluetoothd never removes UUIDs from a device object so
there
+ * is no need to handle it here. */
+while
Re: [pulseaudio-discuss] [PATCHv2 44/60] bluetooth: Get BlueZ 5 device object
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
Get the remote device information stored in pa_bluetooth_discovery. This
also creates the mandatory parameter 'path' for module-bluez5-device,
which is used to inform the object path of the remote device in BlueZ on
the module load.
---
src/modules/bluetooth/module-bluez5-device.c | 65
1 file changed, 65 insertions(+)
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index 890f7e4..c1c3477 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -25,6 +25,7 @@
#endif
#include pulsecore/module.h
+#include pulsecore/modargs.h
#include bluez5-util.h
@@ -34,10 +35,74 @@ PA_MODULE_AUTHOR(João Paulo Rechi Vita);
PA_MODULE_DESCRIPTION(BlueZ 5 Bluetooth audio sink and source);
PA_MODULE_VERSION(PACKAGE_VERSION);
PA_MODULE_LOAD_ONCE(false);
+PA_MODULE_USAGE(path=device object path);
+
+static const char* const valid_modargs[] = {
+path,
+NULL
+};
+
+struct userdata {
+pa_module *module;
+pa_core *core;
+pa_modargs *modargs;
The modargs field is not needed in userdata.
+
+pa_bluetooth_discovery *discovery;
+pa_bluetooth_device *device;
+};
int pa__init(pa_module* m) {
+struct userdata *u;
+pa_modargs *ma;
+const char *path;
+
+pa_assert(m);
+
+if (!(ma = pa_modargs_new(m-argument, valid_modargs))) {
+pa_log_error(Failed to parse module arguments);
+goto fail;
+}
+
+if (!(path = pa_modargs_get_value(ma, path, NULL))) {
+pa_log_error(Failed to get device path from module arguments);
+goto fail;
+}
ma is leaked if parsing the path argument fails.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 27/60] bluetooth: Create pa_bluetooth_transport for BlueZ 5 support
On Mon, 2013-08-19 at 13:44 -0300, João Paulo Rechi Vita wrote:
On Fri, Aug 16, 2013 at 12:32 PM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
Create the pa_bluetooth_transport structure to store information about
the bluetooth transport and utility functions to manipulate this
structure. The acquire() and release() operations are function pointers
in the pa_bluetooth_transport structure to make possible for different
transport backends to provide different implementations of these
operations. Thre is also a userdata field for the transport backend
provide data for the acquire/release functions.
---
src/modules/bluetooth/bluez5-util.c | 145
src/modules/bluetooth/bluez5-util.h | 43 +++
2 files changed, 188 insertions(+)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 1972a92..69eb759 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -36,6 +36,7 @@
#include bluez5-util.h
#define BLUEZ_SERVICE org.bluez
+#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE .MediaTransport1
struct pa_bluetooth_discovery {
PA_REFCNT_DECLARE;
@@ -45,8 +46,131 @@ struct pa_bluetooth_discovery {
bool filter_added;
pa_hook hooks[PA_BLUETOOTH_HOOK_MAX];
pa_hashmap *devices;
+pa_hashmap *transports;
};
+pa_bluetooth_transport *pa_bluetooth_transport_new(pa_bluetooth_device
*d, const char *owner, const char *path,
+
pa_bluetooth_profile_t p, const uint8_t *config, size_t size) {
Unaligned parameter list.
There is some precedence for wrapping long parameter lists like this:
pa_bluetooth_transport *pa_bluetooth_transport_new(
pa_bluetooth_device *d,
const char *owner,
const char *path,
pa_bluetooth_profile_t p,
const uint8_t *config,
size_t size) {
Ok. Maybe we should add this to the official coding style guidelines
in the wiki, although I personally dislike having the function
parameters aligned in the same level as the function implementation.
My preference would be to have them aligned after the opening
parenthesis.
The parameters in my suggestion aren't aligned in the same level as the
function implementation - the indentation for the parameters is 8
spaces, while the function implementation is indented by 4 spaces.
I'll discuss with Arun and David about standardizing this or some other
style.
static void device_free(pa_bluetooth_device *d) {
+unsigned i;
+
pa_assert(d);
+for (i = 0; i PA_BLUETOOTH_PROFILE_COUNT; i++) {
+pa_bluetooth_transport *t;
+
+if (!(t = d-transports[i]))
+continue;
+
+d-transports[i] = NULL;
+transport_state_changed(t,
PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
+/* TODO: check if there is no risk of calling
+ * transport_connection_changed_cb() when the transport is
already freed */
+pa_bluetooth_transport_free(t);
IMO pa_bluetooth_transport_free() should be called by the transport
code, because the backend also creates the transport. The backend may
need a callback for getting notified about the device going away. Or
perhaps there should be pa_bluetooth_transport_kill(), with a kill()
callback in the backend. This would be similar to how sink inputs are
killed if the sink they're connected to goes away.
If the backend is responsible for calling pa_bluetooth_transport_free(),
the core should still ensure that t-device is set to NULL and that the
transport is removed from discovery-transports (feel free to create
pa_bluetooth_transport_unlink() for this purpose, if you want).
I'll answer this comment in the next message in this thread due to the
additional comment in that message.
As for the TODO item, I don't know what it means. There is no
transport_connection_changed_cb() function.
transport_connection_changed_cb() is the callback in
module-bluez5-device for the hook
PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED, which is fired by the
transport_state_changed() call right before the TODO line. Depending
on the way hook callbacks are called by the mainloop t may be already
freed by the pa_bluetooth_transport_free() call after the TODO line.
Ok, so you mean transport_state_changed_cb(), not
transport_connection_changed_cb(). If I understood correctly, you worry
that the hook callbacks might get called asynchronously. Don't worry,
they're always synchronous.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio
Re: [pulseaudio-discuss] [PATCHv2 30/60] bluetooth: Implement org.bluez.MediaEndpoint1.SetConfiguration()
On Mon, 2013-08-19 at 13:57 -0300, João Paulo Rechi Vita wrote:
On Sun, Aug 18, 2013 at 6:03 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/bluez5-util.c | 171
+++-
src/modules/bluetooth/bluez5-util.h | 5 ++
2 files changed, 174 insertions(+), 2 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 9687997..d0428a9 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -33,6 +33,8 @@
#include pulsecore/refcnt.h
#include pulsecore/shared.h
+#include a2dp-codecs.h
+
#include bluez5-util.h
#define BLUEZ_SERVICE org.bluez
@@ -359,12 +361,177 @@ fail:
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
+const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t
profile) {
+switch(profile) {
+case PROFILE_A2DP_SINK:
+return a2dp_sink;
+case PROFILE_A2DP_SOURCE:
+return a2dp_source;
+case PROFILE_OFF:
+return off;
+}
+
+return NULL;
+}
+
static DBusMessage *endpoint_set_configuration(DBusConnection *conn,
DBusMessage *m, void *userdata) {
+pa_bluetooth_discovery *y = userdata;
+pa_bluetooth_device *d;
+pa_bluetooth_transport *t;
+const char *sender, *path, *endpoint_path, *dev_path = NULL, *uuid =
NULL;
+uint8_t *config = NULL;
config should be marked as const, because it will point to memory owned
by the DBusMessage object.
Ok.
+int size = 0;
+pa_bluetooth_profile_t p = PROFILE_OFF;
+DBusMessageIter args, props;
DBusMessage *r;
-pa_assert_se(r = dbus_message_new_error(m,
BLUEZ_MEDIA_ENDPOINT_INTERFACE .Error.NotImplemented,
-Method not implemented));
+if (!dbus_message_iter_init(m, args) ||
!pa_streq(dbus_message_get_signature(m), oa{sv})) {
+pa_log_error(Invalid signature for method SetConfiguration());
+goto fail2;
+}
+
+dbus_message_iter_get_basic(args, path);
+
+if (pa_hashmap_get(y-transports, path)) {
+pa_log_error(Endpoint SetConfiguration(): Transport %s is
already configured., path);
+goto fail2;
+}
+
+pa_assert_se(dbus_message_iter_next(args));
+
+dbus_message_iter_recurse(args, props);
+if (dbus_message_iter_get_arg_type(props) != DBUS_TYPE_DICT_ENTRY)
+goto fail;
+
+/* Read transport properties */
+while (dbus_message_iter_get_arg_type(props) ==
DBUS_TYPE_DICT_ENTRY) {
+const char *key;
+DBusMessageIter value, entry;
+int var;
+
+dbus_message_iter_recurse(props, entry);
+dbus_message_iter_get_basic(entry, key);
+
+dbus_message_iter_next(entry);
+dbus_message_iter_recurse(entry, value);
+
+var = dbus_message_iter_get_arg_type(value);
+
+if (pa_streq(key, UUID)) {
+if (var != DBUS_TYPE_STRING) {
+pa_log_error(Property %s of wrong type %c, key,
(char)var);
+goto fail;
+}
+
+dbus_message_iter_get_basic(value, uuid);
+} else if (pa_streq(key, Device)) {
+if (var != DBUS_TYPE_OBJECT_PATH) {
+pa_log_error(Property %s of wrong type %c, key,
(char)var);
+goto fail;
+}
+
+dbus_message_iter_get_basic(value, dev_path);
+} else if (pa_streq(key, Configuration)) {
+DBusMessageIter array;
+a2dp_sbc_t *c;
+
+if (var != DBUS_TYPE_ARRAY) {
+pa_log_error(Property %s of wrong type %c, key,
(char)var);
+goto fail;
+}
+
+dbus_message_iter_recurse(value, array);
+var = dbus_message_iter_get_arg_type(array);
+if (var != DBUS_TYPE_BYTE) {
+pa_log_error(%s is an array of wrong type %c, key,
(char)var);
+goto fail;
+}
+
+dbus_message_iter_get_fixed_array(array, config, size);
+c = (a2dp_sbc_t *) config;
You should check that the config size is what you expect it to be.
Also, casting the byte array to an a2dp_sbc_t pointer seems hazardous
from memory alignment point of view. I think it would be best to copy
the config to a real a2dp_sbc_t variable before accessing the config.
Why it is hazardous? The BlueZ' media API documentation explicitly
states that we should the binary blob as-is. From doc/media-api.txt:
Configuration blob, it is used as it is so the size and byte order
must match.. Additionally, the a2dp_sbc_t
Re: [pulseaudio-discuss] [PATCH] module-tunnel-sink-new: add a rewrite of module-tunnel using libpulse
On Mon, 2013-08-19 at 07:41 +0200, Alexander Couzens wrote:
Old module-tunnel shares duplicated functionality with libpulse because
it is implementing pulse protocol again.
module-tunnel-sink-new uses libpulse to connect to the remote server
Signed-off-by: Alexander Couzens lyn...@fe80.eu
---
src/Makefile.am | 6 +
src/modules/module-tunnel-sink-new.c | 526
+++
2 files changed, 532 insertions(+)
create mode 100644 src/modules/module-tunnel-sink-new.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 6de6e96..27477e9 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1097,6 +1097,7 @@ modlibexec_LTLIBRARIES += \
module-remap-sink.la \
module-remap-source.la \
module-ladspa-sink.la \
+ module-tunnel-sink-new.la \
module-tunnel-sink.la \
module-tunnel-source.la \
module-position-event-sounds.la \
@@ -1368,6 +1369,7 @@ SYMDEF_FILES = \
module-ladspa-sink-symdef.h \
module-equalizer-sink-symdef.h \
module-match-symdef.h \
+ module-tunnel-sink-new-symdef.h \
module-tunnel-sink-symdef.h \
module-tunnel-source-symdef.h \
module-null-sink-symdef.h \
@@ -1638,6 +1640,10 @@ module_match_la_SOURCES = modules/module-match.c
module_match_la_LDFLAGS = $(MODULE_LDFLAGS)
module_match_la_LIBADD = $(MODULE_LIBADD)
+module_tunnel_sink_new_la_SOURCES = modules/module-tunnel-sink-new.c
+module_tunnel_sink_new_la_LDFLAGS = $(MODULE_LDFLAGS)
+module_tunnel_sink_new_la_LIBADD = $(MODULE_LIBADD)
+
module_tunnel_sink_la_SOURCES = modules/module-tunnel.c
module_tunnel_sink_la_CFLAGS = -DTUNNEL_SINK=1 $(AM_CFLAGS)
module_tunnel_sink_la_LDFLAGS = $(MODULE_LDFLAGS)
diff --git a/src/modules/module-tunnel-sink-new.c
b/src/modules/module-tunnel-sink-new.c
new file mode 100644
index 000..18448c2
--- /dev/null
+++ b/src/modules/module-tunnel-sink-new.c
@@ -0,0 +1,526 @@
+/***
+This file is part of PulseAudio.
+
+Copyright 2013 Alexander Couzens
+
+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; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU Lesser General Public License
+along with PulseAudio; if not, write to the Free Software
+Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
+USA.
+***/
+
+#ifdef HAVE_CONFIG_H
+#include config.h
+#endif
+
+#include pulse/context.h
+#include pulse/timeval.h
+#include pulse/xmalloc.h
+#include pulse/stream.h
+#include pulse/mainloop.h
+#include pulse/introspect.h
+#include pulse/error.h
+
+#include pulsecore/core.h
+#include pulsecore/core-util.h
+#include pulsecore/i18n.h
+#include pulsecore/sink.h
+#include pulsecore/modargs.h
+#include pulsecore/log.h
+#include pulsecore/thread.h
+#include pulsecore/thread-mq.h
+#include pulsecore/poll.h
+#include pulsecore/proplist-util.h
+
+#include module-tunnel-sink-new-symdef.h
+
+PA_MODULE_AUTHOR(Alexander Couzens);
+PA_MODULE_DESCRIPTION(Create a network sink which connects via a stream to
a remote PulseAudio server);
+PA_MODULE_VERSION(PACKAGE_VERSION);
+PA_MODULE_LOAD_ONCE(false);
+PA_MODULE_USAGE(
+server=address
+sink=name of the remote sink
+sink_name=name for the local sink
+sink_properties=properties for the local sink
+format=sample format
+channels=number of channels
+rate=sample rate
+channel_map=channel map
+);
+
+#define TUNNEL_THREAD_FAILED_MAINLOOP 1
+
+static void stream_state_cb(pa_stream *stream, void *userdata);
+static void stream_buffer_attr_cb(pa_stream *stream, void *userdata);
+static void context_state_cb(pa_context *c, void *userdata);
+static void sink_update_requested_latency_cb(pa_sink *s);
+
+struct userdata {
+pa_module *module;
+pa_sink *sink;
+pa_thread *thread;
+pa_thread_mq thread_mq;
+pa_mainloop *thread_mainloop;
+pa_mainloop_api *thread_mainloop_api;
+
+pa_context *context;
+pa_stream *stream;
+
+pa_buffer_attr bufferattr;
I think you don't really need this field in userdata.
+
+bool connected;
+
+char *remote_server;
+char *remote_sink_name;
+};
+
+static const char* const valid_modargs[] = {
+sink_name,
+sink_properties,
+server,
+sink,
+format,
+channels,
+
Re: [pulseaudio-discuss] [PATCH] add module-tunnel-sink-new v4
On Mon, 2013-08-19 at 07:30 +0200, Alexander Couzens wrote: I hope i did'nt forget too much. Not much, but something. I recommend copying my review to a text file, and removing those pieces of feedback that you have addressed. When the file is empty, you know you have addressed everything. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/2] client-conf-x11|client-conf: refactor cookie loaders
client-conf is sufficient for the commit prefix, instead of
client-conf-x11|client-conf. (Sorry, I know there's no way for you to
know what the prefix should be, other than trial and error...)
On Tue, 2013-08-13 at 11:36 +0200, Alexander Couzens wrote:
Signed-off-by: Alexander Couzens lyn...@fe80.eu
---
src/pulse/client-conf-x11.c | 12 +---
src/pulse/client-conf.c | 33 ++---
src/pulse/client-conf.h | 5 -
3 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/src/pulse/client-conf-x11.c b/src/pulse/client-conf-x11.c
index 99265c5..f520619 100644
--- a/src/pulse/client-conf-x11.c
+++ b/src/pulse/client-conf-x11.c
@@ -91,20 +91,10 @@ int pa_client_conf_from_x11(pa_client_conf *c, const char
*dname) {
}
if (pa_x11_get_prop(xcb, screen, PULSE_COOKIE, t, sizeof(t))) {
-uint8_t cookie[PA_NATIVE_COOKIE_LENGTH];
-
-if (pa_parsehex(t, cookie, sizeof(cookie)) != sizeof(cookie)) {
+if(pa_client_conf_load_cookie_from_hex(c, t) 0) {
Missing space after if.
pa_log(_(Failed to parse cookie data));
goto finish;
}
-
-pa_assert(sizeof(cookie) == sizeof(c-cookie));
-memcpy(c-cookie, cookie, sizeof(cookie));
-
-c-cookie_valid = true;
-
-pa_xfree(c-cookie_file);
-c-cookie_file = NULL;
}
ret = 0;
diff --git a/src/pulse/client-conf.c b/src/pulse/client-conf.c
index 8301981..f881751 100644
--- a/src/pulse/client-conf.c
+++ b/src/pulse/client-conf.c
@@ -66,6 +66,8 @@ static const pa_client_conf default_conf = {
.auto_connect_display = false
};
+static int pa_client_conf_parse_cookie_file(pa_client_conf* c);
+
pa_client_conf *pa_client_conf_new(void) {
pa_client_conf *c = pa_xmemdup(default_conf, sizeof(default_conf));
@@ -130,7 +132,7 @@ int pa_client_conf_load(pa_client_conf *c, const char
*filename) {
r = f ? pa_config_parse(fn, f, table, NULL, NULL) : 0;
if (!r)
-r = pa_client_conf_load_cookie(c);
+r = pa_client_conf_parse_cookie_file(c);
finish:
pa_xfree(fn);
@@ -171,13 +173,13 @@ int pa_client_conf_env(pa_client_conf *c) {
pa_xfree(c-cookie_file);
c-cookie_file = pa_xstrdup(e);
-return pa_client_conf_load_cookie(c);
+return pa_client_conf_parse_cookie_file(c);
You could use pa_client_conf_load_cookie_from_file() here.
}
return 0;
}
-int pa_client_conf_load_cookie(pa_client_conf* c) {
+static int pa_client_conf_parse_cookie_file(pa_client_conf* c) {
Static functions shouldn't be prefixed, so the function name should be
parse_cookie_file.
int k;
pa_assert(c);
@@ -203,3 +205,28 @@ int pa_client_conf_load_cookie(pa_client_conf* c) {
c-cookie_valid = true;
return 0;
}
+
+int pa_client_conf_load_cookie_from_hex(pa_client_conf* c, const char
*cookie_in_hex) {
+uint8_t cookie[PA_NATIVE_COOKIE_LENGTH];
+
+if (pa_parsehex(cookie_in_hex, cookie, sizeof(cookie)) !=
sizeof(cookie)) {
+pa_log(_(Failed to parse cookie data));
+return -PA_ERR_INVALID;
+}
+
+pa_assert(sizeof(cookie) == sizeof(c-cookie));
+memcpy(c-cookie, cookie, sizeof(cookie));
+
+c-cookie_valid = true;
+
+pa_xfree(c-cookie_file);
+c-cookie_file = NULL;
+
+return 0;
+}
+
+int pa_client_conf_load_cookie_from_file(pa_client_conf *c, const char
*cookie_file_path) {
+pa_xfree(c-cookie_file);
+c-cookie_file = pa_xstrdup(cookie_file_path);
+return pa_client_conf_parse_cookie_file(c);
+}
diff --git a/src/pulse/client-conf.h b/src/pulse/client-conf.h
index 9c509f7..7b9c0c1 100644
--- a/src/pulse/client-conf.h
+++ b/src/pulse/client-conf.h
@@ -49,6 +49,9 @@ int pa_client_conf_load(pa_client_conf *c, const char
*filename);
int pa_client_conf_env(pa_client_conf *c);
/* Load cookie data from c-cookie_file into c-cookie */
-int pa_client_conf_load_cookie(pa_client_conf* c);
+int pa_client_conf_load_cookie_from_file(pa_client_conf* c, const char*
cookie_file_path);
The comment above the function needs to be updated too.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 2/2] context: add cookie loaders + setter
On Tue, 2013-08-13 at 11:41 +0200, Alexander Couzens wrote:
I'm not sure if these 3 seperate calls for cookie loading are better than
creating a pa_context_get_conf() call and
operating on it.
I think it's better to keep pa_client_conf as a private interface.
Signed-off-by: Alexander Couzens lyn...@fe80.eu
---
src/pulse/context.c | 15 +++
src/pulse/context.h | 9 +
2 files changed, 24 insertions(+)
diff --git a/src/pulse/context.c b/src/pulse/context.c
index 1ba2672..654284e 100644
--- a/src/pulse/context.c
+++ b/src/pulse/context.c
@@ -1448,3 +1448,18 @@ size_t pa_context_get_tile_size(pa_context *c, const
pa_sample_spec *ss) {
mbs = PA_ROUND_DOWN(pa_mempool_block_size_max(c-mempool), fs);
return PA_MAX(mbs, fs);
}
+
+int pa_context_set_cookie(pa_context *c, uint8_t *cookie, size_t
cookie_size) {
The pointers should be checked with pa_assert(). And there should be
some sanity checks:
PA_CHECK_VALIDITY(c, !pa_detect_fork(), PA_ERR_FORKED);
PA_CHECK_VALIDITY(c, c-state == PA_CONTEXT_UNCONNECTED, PA_ERR_BADSTATE);
(The same goes for the other functions.)
+if(cookie_size != PA_NATIVE_COOKIE_LENGTH)
Missing space after if.
+return -PA_ERR_INVALID;
+memcpy(c-conf-cookie, cookie, cookie_size);
c-conf-cookie_valid should be set to true. Or actually, there should
be pa_client_conf_set_cookie() that sets the cookie_valid field -
modifying the pa_client_conf state should be done from client-conf.c
only.
+return 0;
+}
+
+int pa_context_load_cookie_from_hex(pa_context *c, const char
*cookie_as_hex) {
+return pa_client_conf_load_cookie_from_hex(c-conf, cookie_as_hex);
+}
+
+int pa_context_load_cookie_file(pa_context *c, const char *cookie_file_path)
{
+return pa_client_conf_load_cookie_from_file(c-conf, cookie_file_path);
+}
diff --git a/src/pulse/context.h b/src/pulse/context.h
index 6e615f6..20fa1ff 100644
--- a/src/pulse/context.h
+++ b/src/pulse/context.h
@@ -280,6 +280,15 @@ void pa_context_rttime_restart(pa_context *c,
pa_time_event *e, pa_usec_t usec);
* pa_stream_get_sample_spec(ss)); \since 0.9.20 */
size_t pa_context_get_tile_size(pa_context *c, const pa_sample_spec *ss);
+/** Set the authentication cookie */
The documentation is missing a \since 5.0 tag, and explanation about
what the cookie size should be.
We need to expose the cookie size to clients somehow. A simple #define
should be good enough. The problem with a #define in the public API is
that it can't be changed without breaking compatibility, but I don't
think we can change the cookie size anyway without breaking
compatibility between old server and new libpulse or vice versa.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] module-tunnel-sink-new: add a rewrite of module-tunnel using libpulse
On Wed, 2013-08-21 at 13:58 +0200, Alexander Couzens wrote:
Old module-tunnel shares duplicated functionality with libpulse because
it is implementing pulse protocol again. It is also not
as much tested as libpulse it.
Signed-off-by: Alexander Couzens lyn...@fe80.eu
---
src/Makefile.am | 6 +
src/modules/module-tunnel-sink-new.c | 546
+++
2 files changed, 552 insertions(+)
create mode 100644 src/modules/module-tunnel-sink-new.c
Still some small issues, but they are trivial enough for me to fix them
myself. Patch applied, thank you!
+static void thread_func(void *userdata) {
+struct userdata *u = userdata;
+pa_proplist *proplist;
+pa_assert(u);
+
+pa_log_debug(Thread starting up);
+pa_thread_mq_install(u-thread_mq);
+
+proplist = tunnel_new_proplist(u);
+u-context = pa_context_new_with_proplist(u-thread_mainloop_api,
+ PulseAudio,
+ proplist);
+pa_proplist_free(proplist);
+
+if (!u-context) {
+pa_log(Failed to create libpulse context);
+goto fail;
+}
+
+pa_context_set_state_callback(u-context, context_state_cb, u);
+if (pa_context_connect(u-context,
+ u-remote_server,
+ PA_CONTEXT_NOAUTOSPAWN,
+ NULL) 0) {
+pa_log(Failed to connect libpulse context);
+goto fail;
+}
+
+for (;;) {
+int ret;
+const void *p;
+pa_memchunk memchunk;
+
+size_t writable = 0;
+
+if (pa_mainloop_iterate(u-thread_mainloop, 1, ret) 0) {
+if (ret == 0)
+goto finish;
+else
+goto fail;
+}
+
+if (PA_UNLIKELY(u-sink-thread_info.rewind_requested))
+pa_sink_process_rewind(u-sink, 0);
+
+if (u-connected
+pa_stream_get_state(u-stream) == PA_STREAM_READY
+PA_SINK_IS_LINKED(u-sink-thread_info.state)) {
+/* TODO: use IS_RUNNING and cork the stream when sink is not
running */
The stream should be corked only if the sink is suspended. The sink can
be idle or running, and in both cases the stream should stay uncorked.
+static void stream_state_cb(pa_stream *stream, void *userdata) {
+struct userdata *u = userdata;
+
+pa_assert(u);
+
+switch (pa_stream_get_state(stream)) {
+case PA_STREAM_FAILED:
+pa_log_error(Stream failed.);
+u-connected = false;
+u-thread_mainloop_api-quit(u-thread_mainloop_api,
TUNNEL_THREAD_FAILED_MAINLOOP);
+break;
+case PA_STREAM_TERMINATED:
+pa_log_debug(Stream terminated.);
+break;
+case PA_STREAM_READY:
+/* only call our requested_latency_cb when request_latency
changed between PA_CONNECTING - PA_STREAM_READY
+ * otherwhise we would deny servers response to bufferattr
(which defines latency) */
Comments should be wrapped at 80 characters.
+if (u-update_stream_bufferattr_after_connect)
+sink_update_requested_latency_cb(u-sink);
There should be an else section that updates the sink max_request to
match the tlength that the server assigned.
+static void sink_update_requested_latency_cb(pa_sink *s) {
+struct userdata *u;
+pa_operation *operation;
+size_t nbytes;
+pa_usec_t block_usec;
+pa_buffer_attr bufferattr;
+
+pa_sink_assert_ref(s);
+pa_assert_se(u = s-userdata);
+
+block_usec = pa_sink_get_requested_latency_within_thread(s);
+if (block_usec == (pa_usec_t) -1)
+block_usec = s-thread_info.max_latency;
+
+nbytes = pa_usec_to_bytes(block_usec, s-sample_spec);
+pa_sink_set_max_request_within_thread(s, nbytes);
+
+if (u-stream) {
+switch (pa_stream_get_state(u-stream)) {
+case PA_STREAM_READY:
One level of indentation is missing.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] source-output: Get the correct source for direct_on_input streams
On Wed, 2013-08-14 at 16:58 +0300, Tanu Kaskinen wrote:
If a capture stream captures from a single sink input (so the capture
stream is a so called direct on input stream), then it needs to
connect to the monitor source of the sink to which the sink input is
connected. Previously the correct source was not figured out
automatically, causing the capture stream creation to fail.
---
src/pulsecore/source-output.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
index 66a33bd..fafc226 100644
--- a/src/pulsecore/source-output.c
+++ b/src/pulsecore/source-output.c
@@ -255,9 +255,17 @@ int pa_source_output_new(
pa_return_val_if_fail(!data-driver || pa_utf8_valid(data-driver),
-PA_ERR_INVALID);
if (!data-source) {
-pa_source *source = pa_namereg_get(core, NULL, PA_NAMEREG_SOURCE);
-pa_return_val_if_fail(source, -PA_ERR_NOENTITY);
-pa_source_output_new_data_set_source(data, source, false);
+pa_source *source;
+
+if (data-direct_on_input) {
+source = data-direct_on_input-sink-monitor_source;
+pa_return_val_if_fail(source, -PA_ERR_INVALID);
+pa_source_output_new_data_set_source(data, source, false);
+} else {
+source = pa_namereg_get(core, NULL, PA_NAMEREG_SOURCE);
+pa_return_val_if_fail(source, -PA_ERR_NOENTITY);
+pa_source_output_new_data_set_source(data, source, false);
+}
}
/* Routing's done, we have a source. Now let's fix the format and set up
the
No feedback received, I have now applied this patch.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 60/60] bluetooth: Revive module-bluetooth-discover
On Mon, 2013-08-19 at 12:54 +0300, Tanu Kaskinen wrote:
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
+static bool exists(const char *filename) {
+const char *paths, *state = NULL;
+char *p, *pathname;
+bool result;
+
+if (!(paths = lt_dlgetsearchpath()))
undefined reference to `lt_dlgetsearchpath'
Does this compile on your machine?
Apparently this works on some machines and not on others. On my machine
this is required to make the linking work:
-module_bluetooth_discover_la_LIBADD = $(MODULE_LIBADD)
+module_bluetooth_discover_la_LIBADD = $(MODULE_LIBADD) $(LIBLTDL)
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Low latency audio in-out application
On Mon, 2013-08-26 at 13:31 +0200, Svein Seldal wrote: Hi I'm working on using a [dedicated] linux box for an audio filtering use. My intentions is to take stereo audio in, process it, and play the result on audio out (all on the same sound device). The catch is that this app requires as low latency as possible to be usable in the field (40ms) Since pulseaudio is conveniently available on desktops, I wrote a simple pulse app which opens two streams, one record and one playback and filter the audio between them. In the current design, I clock everything by using a record stream read CB. The read callback takes the available data, process it, and write it asynch to the playback stream without using playback write CB. This works, but with significant latency (200ms). However, when I start to adjust the latency settings on either streams, I get underrun errors. Apparently, even though the two streams originate from the same wordclock, they expect/deliver data at varying intervals. But having a floating buffer inbetween adds further delay. Does anyone have ideas of what other approaches would be best for this app? E.g. clock it from the playback write CB and asynchronously read data from record? IMHO it would be nice if playback and record streams could be syncronized, not just playback streams. It would make things very much simpler when record deliver x number of bytes, the playback wants the same x number of bytes. Perhaps PA offers this already? No, PA doesn't offer that. Reading and writing to the sound card is handled completely separately, from different threads, so there's no read/write synchronization in the server either. You said that you use a dedicated box for the processing, so using the default sound server doesn't sound like a hard requirement. In that case I recommend using JACK, it's designed for this kind of use cases. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Low latency audio in-out application
On Mon, 2013-08-26 at 15:04 +0200, Svein Seldal wrote: On 08/26/2013 02:36 PM, Tanu Kaskinen wrote: No, PA doesn't offer that. Reading and writing to the sound card is handled completely separately, from different threads, so there's no read/write synchronization in the server either. You said that you use a dedicated box for the processing, so using the default sound server doesn't sound like a hard requirement. In that case I recommend using JACK, it's designed for this kind of use cases. Thanks Too bad. Using PA is convenient, as it's already integrated into most distros. And I am apparently misinterpreting the PA claim Good low latency behaviour -- but I guess that this depending on the definition of low latency... Dedicated box means that this PC is a prototype for some real-world HW DSP device which the software will later be moved to. Working on a linux application clearly helps development, debugging and observability. Perhaps it's best to approach ALSA directly for the lowest latency. Have I understood it correctly that PA emulates ALSA, so I need to disable PA for that particular sound device before I can access it directly? Yes, it's a good idea to make sure that PulseAudio doesn't try to open the sound card when you are using it (the ALSA emulation doesn't have anything to do with this, though). The easiest way to do this is to switch the card profile to Off with pavucontrol (in the Configuration tab). OOI: What use has the synchronize stream argument in stream playback? You can start/stop multiple streams at exactly the same time. I'm not aware of any real-world uses for that feature, though... -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH v2 1/3] source: Fix monitor source rate changing
When a sink changes its sample rate, also the monitor source rate
needs to be changed. In order to determine whether a source supports
rate changing, the code checks if the update_rate() callback is set,
but monitor sources don't have that callback set, so the old code
always failed to change the monitor source rate.
This patch fixes the monitor source rate changing by handling monitor
sources as a special case in pa_source_update_rate(): if the source is
a monitor source, then the update_rate() callback is not required.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=66424
---
src/pulsecore/source.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index 5cb6b73..c692511 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -972,14 +972,12 @@ bool pa_source_update_rate(pa_source *s, uint32_t rate,
bool passthrough) {
uint32_t desired_rate = rate;
uint32_t default_rate = s-default_sample_rate;
uint32_t alternate_rate = s-alternate_sample_rate;
-uint32_t idx;
-pa_source_output *o;
bool use_alternate = false;
if (rate == s-sample_spec.rate)
return true;
-if (!s-update_rate)
+if (!s-update_rate !s-monitor_of)
return false;
if (PA_UNLIKELY(default_rate == alternate_rate !passthrough)) {
@@ -1027,14 +1025,24 @@ bool pa_source_update_rate(pa_source *s, uint32_t rate,
bool passthrough) {
pa_log_debug(Suspending source %s due to changing the sample rate.,
s-name);
pa_source_suspend(s, true, PA_SUSPEND_INTERNAL);
-if (s-update_rate(s, desired_rate) == true) {
-pa_log_info(Changed sampling rate successfully );
+if (s-update_rate)
+ret = s-update_rate(s, desired_rate);
+else {
+/* This is a monitor source. */
+s-sample_spec.rate = desired_rate;
+ret = true;
+}
+
+if (ret) {
+uint32_t idx;
+pa_source_output *o;
PA_IDXSET_FOREACH(o, s-outputs, idx) {
if (o-state == PA_SOURCE_OUTPUT_CORKED)
pa_source_output_update_rate(o);
}
-ret = true;
+
+pa_log_info(Changed sampling rate successfully);
}
pa_source_suspend(s, false, PA_SUSPEND_INTERNAL);
--
1.8.1.2
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH v2 3/3] sink, source: Fix error reporting style for rate updates
---
src/modules/alsa/alsa-sink.c | 8
src/modules/alsa/alsa-source.c | 8
src/pulsecore/sink-input.c | 4 ++--
src/pulsecore/sink.c | 24
src/pulsecore/sink.h | 4 ++--
src/pulsecore/source-output.c | 4 ++--
src/pulsecore/source.c | 26 +-
src/pulsecore/source.h | 4 ++--
8 files changed, 41 insertions(+), 41 deletions(-)
diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
index f910d19..6e60646 100644
--- a/src/modules/alsa/alsa-sink.c
+++ b/src/modules/alsa/alsa-sink.c
@@ -1593,7 +1593,7 @@ static bool sink_set_formats(pa_sink *s, pa_idxset
*formats) {
return true;
}
-static bool sink_update_rate_cb(pa_sink *s, uint32_t rate) {
+static int sink_update_rate_cb(pa_sink *s, uint32_t rate) {
struct userdata *u = s-userdata;
int i;
bool supported = false;
@@ -1609,16 +1609,16 @@ static bool sink_update_rate_cb(pa_sink *s, uint32_t
rate) {
if (!supported) {
pa_log_info(Sink does not support sample rate of %d Hz, rate);
-return false;
+return -1;
}
if (!PA_SINK_IS_OPENED(s-state)) {
pa_log_info(Updating rate for device %s, new rate is
%d,u-device_name, rate);
u-sink-sample_spec.rate = rate;
-return true;
+return 0;
}
-return false;
+return -1;
}
static int process_rewind(struct userdata *u) {
diff --git a/src/modules/alsa/alsa-source.c b/src/modules/alsa/alsa-source.c
index faf8965..78125b1 100644
--- a/src/modules/alsa/alsa-source.c
+++ b/src/modules/alsa/alsa-source.c
@@ -1401,7 +1401,7 @@ static void source_update_requested_latency_cb(pa_source
*s) {
update_sw_params(u);
}
-static bool source_update_rate_cb(pa_source *s, uint32_t rate) {
+static int source_update_rate_cb(pa_source *s, uint32_t rate) {
struct userdata *u = s-userdata;
int i;
bool supported = false;
@@ -1417,16 +1417,16 @@ static bool source_update_rate_cb(pa_source *s,
uint32_t rate) {
if (!supported) {
pa_log_info(Source does not support sample rate of %d Hz, rate);
-return false;
+return -1;
}
if (!PA_SOURCE_IS_OPENED(s-state)) {
pa_log_info(Updating rate for device %s, new rate is %d,
u-device_name, rate);
u-source-sample_spec.rate = rate;
-return true;
+return 0;
}
-return false;
+return -1;
}
static void thread_func(void *userdata) {
diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
index a275715..b5e8a0b 100644
--- a/src/pulsecore/sink-input.c
+++ b/src/pulsecore/sink-input.c
@@ -426,7 +426,7 @@ int pa_sink_input_new(
module-suspend-on-idle can resume a sink */
pa_log_info(Trying to change sample rate);
-if (pa_sink_update_rate(data-sink, data-sample_spec.rate,
pa_sink_input_new_data_is_passthrough(data)) == true)
+if (pa_sink_update_rate(data-sink, data-sample_spec.rate,
pa_sink_input_new_data_is_passthrough(data)) = 0)
pa_log_info(Rate changed to %u Hz, data-sink-sample_spec.rate);
}
@@ -1829,7 +1829,7 @@ int pa_sink_input_finish_move(pa_sink_input *i, pa_sink
*dest, bool save) {
SINK_INPUT_MOVE_FINISH hook */
pa_log_info(Trying to change sample rate);
-if (pa_sink_update_rate(dest, i-sample_spec.rate,
pa_sink_input_is_passthrough(i)) == true)
+if (pa_sink_update_rate(dest, i-sample_spec.rate,
pa_sink_input_is_passthrough(i)) = 0)
pa_log_info(Rate changed to %u Hz, dest-sample_spec.rate);
}
diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index bc8a3fd..de8c82e 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -1377,8 +1377,8 @@ void pa_sink_render_full(pa_sink *s, size_t length,
pa_memchunk *result) {
}
/* Called from main thread */
-bool pa_sink_update_rate(pa_sink *s, uint32_t rate, bool passthrough) {
-bool ret = false;
+int pa_sink_update_rate(pa_sink *s, uint32_t rate, bool passthrough) {
+int ret = -1;
uint32_t desired_rate = rate;
uint32_t default_rate = s-default_sample_rate;
uint32_t alternate_rate = s-alternate_sample_rate;
@@ -1387,32 +1387,32 @@ bool pa_sink_update_rate(pa_sink *s, uint32_t rate,
bool passthrough) {
bool use_alternate = false;
if (rate == s-sample_spec.rate)
-return true;
+return 0;
if (!s-update_rate)
-return false;
+return -1;
if (PA_UNLIKELY(default_rate == alternate_rate !passthrough)) {
pa_log_debug(Default and alternate sample rates are the same.);
-return false;
+return -1;
}
if (PA_SINK_IS_RUNNING(s-state)) {
pa_log_info(Cannot update rate, SINK_IS_RUNNING, will keep using %u
Hz,
s-sample_spec.rate);
-return false;
+return -1;
}
if (s-monitor_source) {
if
[pulseaudio-discuss] [PATCH v2 2/3] source: When updating a monitor source's rate, update the sink rate too
If the sink rate is not updated, then the monitor source will appear
to have a different rate than the sink, but in reality there's never
any resampling done when moving data from the sink to the monitor
source, so it's a lie that the monitor source has a different rate.
The result of lying is that clients that capture from the monitor
source will have streams that run too fast or slow.
---
src/pulsecore/source.c | 34 --
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index c692511..5e59a40 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -991,6 +991,13 @@ bool pa_source_update_rate(pa_source *s, uint32_t rate,
bool passthrough) {
return false;
}
+if (s-monitor_of) {
+if (PA_SINK_IS_RUNNING(s-monitor_of-state)) {
+pa_log_info(Cannot update rate, this is a monitor source and the
sink is running.);
+return false;
+}
+}
+
if (PA_UNLIKELY (desired_rate 8000 ||
desired_rate PA_RATE_MAX))
return false;
@@ -1029,8 +1036,31 @@ bool pa_source_update_rate(pa_source *s, uint32_t rate,
bool passthrough) {
ret = s-update_rate(s, desired_rate);
else {
/* This is a monitor source. */
-s-sample_spec.rate = desired_rate;
-ret = true;
+
+/* XXX: This code is written with non-passthrough streams in mind. I
+ * have no idea whether the behaviour with passthrough streams is
+ * sensible. */
+if (!passthrough) {
+uint32_t old_rate = s-sample_spec.rate;
+
+s-sample_spec.rate = desired_rate;
+ret = pa_sink_update_rate(s-monitor_of, desired_rate, false);
+
+if (!ret) {
+/* Changing the sink rate failed, roll back the old rate for
+ * the monitor source. Why did we set the source rate before
+ * calling pa_sink_update_rate(), you may ask. The reason is
+ * that pa_sink_update_rate() tries to update the monitor
+ * source rate, but we are already in the process of updating
+ * the monitor source rate, so there's a risk of entering an
+ * infinite loop. Setting the source rate before calling
+ * pa_sink_update_rate() makes the rate == s-sample_spec.rate
+ * check in the beginning of this function return early, so we
+ * avoid looping. */
+s-sample_spec.rate = old_rate;
+}
+} else
+ret = false;
}
if (ret) {
--
1.8.1.2
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH] resampler: Never return zero for max block size
With very low input sample rates the memory pool max block size may
not be big enough, in which case we should return the size of one
frame. Returning zero caused crashing.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=68616
---
src/pulsecore/resampler.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/src/pulsecore/resampler.c b/src/pulsecore/resampler.c
index 5599035..4480823 100644
--- a/src/pulsecore/resampler.c
+++ b/src/pulsecore/resampler.c
@@ -534,7 +534,23 @@ size_t pa_resampler_max_block_size(pa_resampler *r) {
if (r-remap_buf_contains_leftover_data)
frames -= r-remap_buf.length / (r-w_sz * r-o_ss.channels);
-return ((uint64_t) frames * r-i_ss.rate / max_ss.rate) * r-i_fz;
+block_size_max = ((uint64_t) frames * r-i_ss.rate / max_ss.rate) *
r-i_fz;
+
+if (block_size_max 0)
+return block_size_max;
+else
+/* A single input frame may result in so much output that it doesn't
+ * fit in one standard memblock (e.g. converting 1 Hz to 44100 Hz). In
+ * this case the max block size will be set to one frame, and some
+ * memory will be probably be allocated with malloc() instead of using
+ * the memory pool.
+ *
+ * XXX: Should we support this case at all? We could also refuse to
+ * create resamplers whose max block size would exceed the memory pool
+ * block size. In this case also updating the resampler rate should
+ * fail if the new rate would cause an excessive max block size (in
+ * which case the stream would probably have to be killed). */
+return r-i_fz;
}
void pa_resampler_reset(pa_resampler *r) {
--
1.8.1.2
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] suspend-on-idle: Allow disabling suspending for specific devices
On Fri, 2013-08-30 at 08:16 +0200, David Henningsson wrote: On 08/29/2013 04:36 PM, Tanu Kaskinen wrote: Sometimes it would be nice to disable module-suspend-on-idle for specific devices. For me the use case is to keep a HDMI sink running all the time to avoid loss of audio when starting to play a stream to the device (the HDMI receiver eats a bit from the beginning of the stream when the device is opened). This is arguably a hacky solution to the problem, but on the other hand, I think it's very sensible to interpret negative timeout in the module-suspend-on-idle.timeout property as disabling the suspending altogher. This is also how the exit-idle-time configuration option behaves (negative value disables automatic exiting). Didn't Arun have another solution for a similar problem? Like a sink ALWAYS_RUNNING flag or something. Yes, he did. That patch was not merged, and I don't know if Arun plans to still work on it. Regardless of Arun's solution, I think it makes sense to support negative value for the module-suspend-on-idle.timeout property to be consistent with other timeout parameters. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 31/60] bluetooth: Implement org.bluez.MediaEndpoint1.SelectConfiguration()
On Tue, 2013-09-03 at 21:18 -0300, João Paulo Rechi Vita wrote:
On Sun, Aug 18, 2013 at 6:37 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
static DBusMessage *endpoint_select_configuration(DBusConnection *conn,
DBusMessage *m, void *userdata) {
+pa_bluetooth_discovery *y = userdata;
+a2dp_sbc_t *cap, config;
+uint8_t *pconf = (uint8_t *) config;
+int i, size;
DBusMessage *r;
+DBusError err;
-pa_assert_se(r = dbus_message_new_error(m,
BLUEZ_MEDIA_ENDPOINT_INTERFACE .Error.NotImplemented,
-Method not implemented));
+static const struct {
+uint32_t rate;
+uint8_t cap;
+} freq_table[] = {
+{ 16000U, SBC_SAMPLING_FREQ_16000 },
+{ 32000U, SBC_SAMPLING_FREQ_32000 },
+{ 44100U, SBC_SAMPLING_FREQ_44100 },
+{ 48000U, SBC_SAMPLING_FREQ_48000 }
+};
+
+dbus_error_init(err);
+
+if (!dbus_message_get_args(m, err, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE,
cap, size, DBUS_TYPE_INVALID)) {
Passing cap may cause alignment issues when accessing cap later. A byte
array pointer should be passed instead, and the memory should be copied
to an a2dp_sbc_t variable.
In my understanding this is the same case of your previous comment in
enpoint_set_configuration(): a2dp_sbc_t is 1-byte aligned (packed and
has only uint8_t fields), so there shouldn't be any problem. Or am I
missing something?
No, you're not missing anything. My mistake.
+pa_log_error(Endpoint SelectConfiguration(): %s, err.message);
+dbus_error_free(err);
+goto fail;
+}
+
+if (size != sizeof(config)) {
+pa_log_error(Capabilities array has invalid size);
+goto fail;
+}
+pa_zero(config);
+
+/* Find the lowest freq that is at least as high as the requested
sampling rate */
+for (i = 0; (unsigned) i PA_ELEMENTSOF(freq_table); i++)
+if (freq_table[i].rate = y-core-default_sample_spec.rate
(cap-frequency freq_table[i].cap)) {
+config.frequency = freq_table[i].cap;
+break;
+}
+
+if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
+for (--i; i = 0; i--) {
+if (cap-frequency freq_table[i].cap) {
+config.frequency = freq_table[i].cap;
+break;
+}
+}
+
+if (i 0) {
+pa_log_error(Not suitable sample rate);
+goto fail;
+}
+}
+
+pa_assert((unsigned) i PA_ELEMENTSOF(freq_table));
+
+if (y-core-default_sample_spec.channels = 1) {
+if (cap-channel_mode SBC_CHANNEL_MODE_MONO)
+config.channel_mode = SBC_CHANNEL_MODE_MONO;
+}
+
+if (y-core-default_sample_spec.channels = 2) {
+if (cap-channel_mode SBC_CHANNEL_MODE_JOINT_STEREO)
+config.channel_mode = SBC_CHANNEL_MODE_JOINT_STEREO;
+else if (cap-channel_mode SBC_CHANNEL_MODE_STEREO)
+config.channel_mode = SBC_CHANNEL_MODE_STEREO;
+else if (cap-channel_mode SBC_CHANNEL_MODE_DUAL_CHANNEL)
+config.channel_mode = SBC_CHANNEL_MODE_DUAL_CHANNEL;
+else if (cap-channel_mode SBC_CHANNEL_MODE_MONO) {
+config.channel_mode = SBC_CHANNEL_MODE_MONO;
+} else {
+pa_log_error(No supported channel modes);
+goto fail;
+}
+}
You didn't address my earlier complaint: If
default_sample_spec.channels is 1, but cap doesn't contain
SBC_CHANNEL_MODE_MONO, then config.channel_mode is left uninitialized
(well, it's initialized to zero in the beginning). I believe this is not
what you intended.
Yes, I think it is better to return an error to the caller here.
There's no need to return an error, we should allow any of the
two-channel modes even if default_sample_spec.channels is 1.
I also said that if default_sample_spec is 2 and cap only contains
SBC_CHANNEL_MODE_MONO, then the negotiation fails. It seems that that
complaint was bogus, though, so you're right to ignore that bit.
Sorry, I didn't understand the second part of your comment. If
default_sample_spec.channels == 2 and cap-channel_mode has only
SBC_CHANNEL_MODE_MONO, then config.channel_mode will receive
SBC_CHANNEL_MODE_MONO.
Yes, this is what I meant by the complaint being bogus, so your code is
fine on this part.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 32/60] bluetooth: Implement org.bluez.MediaEndpoint1.ClearConfiguration()
On Wed, 2013-09-04 at 15:17 -0300, João Paulo Rechi Vita wrote:
On Sun, Aug 18, 2013 at 8:15 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/bluez5-util.c | 28 ++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 1194503..1d98174 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -697,11 +697,35 @@ fail:
}
static DBusMessage *endpoint_clear_configuration(DBusConnection *conn,
DBusMessage *m, void *userdata) {
+pa_bluetooth_discovery *y = userdata;
+pa_bluetooth_transport *t;
DBusMessage *r;
+DBusError err;
+const char *path;
-pa_assert_se(r = dbus_message_new_error(m,
BLUEZ_MEDIA_ENDPOINT_INTERFACE .Error.NotImplemented,
-Method not implemented));
+dbus_error_init(err);
+if (!dbus_message_get_args(m, err, DBUS_TYPE_OBJECT_PATH, path,
DBUS_TYPE_INVALID)) {
+pa_log_error(Endpoint ClearConfiguration(): %s, err.message);
+dbus_error_free(err);
+goto fail;
+}
+
+if ((t = pa_hashmap_get(y-transports, path))) {
+pa_log_debug(Clearing transport %s profile %s, t-path,
pa_bluetooth_profile_to_string(t-profile));
+transport_state_changed(t,
PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED);
+t-device-transports[t-profile] = NULL;
+/* TODO: check if there is no risk of calling
+ * transport_connection_changed_cb() when the transport is
already freed */
+pa_bluetooth_transport_free(t);
Similar to what I said about device_free(), the core code shouldn't call
pa_bluetooth_transport_free() here either. I think
pa_bluetooth_transport should have a configuration_cleared() callback,
and the transport backend can then call pa_bluetooth_transport_free() if
it wants.
All endpoint methods on the D-Bus interface are specific to the BlueZ
transport-backend, so it's not the core calling
pa_bluetooth_transport_free(). If we ever decide to separate the BlueZ
5 transport backend to a different file, the endpoint functions should
be moved altogether.
OK. What would you think about moving
t-device-transports[t-profile] = NULL;
to transport_state_changed()? Writing to pa_bluetooth_device variables
should ideally be done by the core, so if endpoint_clear_configuration()
is backend code, then this line is likely in the wrong place.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] card-restore: Watch for profiles added after card creation.
On Thu, 2013-07-25 at 16:39 +0200, poljar (Damir Jelić) wrote:
This patch adds the ability to restore profiles if they are added after
card creation.
Adding profiles after card creation mainly happens for bluetooth cards.
Buglink: https://bugs.freedesktop.org/show_bug.cgi?id=65349
---
src/modules/module-card-restore.c | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
Applied, thanks! I fixed a small issue pointed out below.
+static pa_hook_result_t card_profile_added_callback(pa_core *c,
pa_card_profile *profile, struct userdata *u) {
+struct entry *entry;
+
+pa_assert(profile);
+
+if (!(entry = entry_read(u, profile-card-name)))
+return PA_HOOK_OK;
+
+if (pa_safe_streq(entry-profile, profile-name)) {
+pa_card_set_profile(profile-card, profile-name, true);
+pa_log_info(Restored profile '%s' for card %s., profile-name,
profile-card-name);
pa_card_set_profile() can fail, in which case the log message shouldn't
be printed.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] tunnel-source-new: counterpart to module-tunnel-sink-new
On Thu, 2013-09-12 at 14:01 +0200, Alexander Couzens wrote:
The old tunnel module duplicates functionality that is in libpulse,
due to implementing the native protocol, and the protocol code in
the old tunnel module tends to get broken every now and then, because
people forget to update the tunnel module protocol implementation
when changing the native protocol. module-tunnel-source-new avoids this
problem by using libpulse to communicate with the remote server.
---
src/Makefile.am| 6 +
src/modules/module-tunnel-source-new.c | 552
+
2 files changed, 558 insertions(+)
create mode 100644 src/modules/module-tunnel-source-new.c
Thanks, patch applied with a couple of changes (see below).
+/* called from io context to read samples from the stream into our source */
+static void read_new_samples(struct userdata *u) {
+const void *p;
+size_t readable = 0;
+pa_memchunk memchunk;
+
+pa_assert(u);
+u-new_data = false;
+
+pa_memchunk_reset(memchunk);
+
+if (PA_UNLIKELY(!u-connected || pa_stream_get_state(u-stream) !=
PA_STREAM_READY))
+return;
+
+readable = pa_stream_readable_size(u-stream);
+while (PA_LIKELY(readable 0)) {
This PA_LIKELY doesn't do any good. The normal pattern is to evaluate
the condition twice: on the first pass the condition will evaluate to
true and on the second pass it will evaluate to false.
+size_t read = 0;
+if (PA_UNLIKELY(pa_stream_peek(u-stream, p, read) != 0)) {
+pa_log(pa_stream_peek() failed: %s,
pa_strerror(pa_context_errno(u-context)));
+u-thread_mainloop_api-quit(u-thread_mainloop_api,
TUNNEL_THREAD_FAILED_MAINLOOP);
+return;
+}
+
+if (PA_LIKELY(p)) {
+/* we have valid data */
+memchunk.memblock =
pa_memblock_new_fixed(u-module-core-mempool, (void *) p, read, true);
+memchunk.length = read;
+memchunk.index = 0;
+
+pa_source_post(u-source, memchunk);
+pa_memblock_unref_fixed(memchunk.memblock);
+pa_stream_drop(u-stream);
+
+readable -= read;
+} else /* if (!p read 0) */ {
+/* we have a hole. generate silence */
+pa_silence_memchunk_get(
+u-module-core-silence_cache,
+u-module-core-mempool,
+memchunk,
+u-source-sample_spec,
+read);
I think this is not safe to call from the IO thread. The silence cache
doesn't seem to be thread safe, so it's only useful in the main thread.
We can use the memblock in u-source-silence instead.
There's no guarantee that either the memchunk returned by
pa_silence_memchunk_get() or the memblock in u-source-silence is large
enough. If it's not large enough, I think we can just call
pa_source_post() in a loop using the same memblock multiple times.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] suspend-on-idle: Allow disabling suspending for specific devices
On Fri, 2013-09-13 at 13:20 +0530, Arun Raghavan wrote: On Fri, 2013-08-30 at 15:11 +0200, David Henningsson wrote: On 08/30/2013 09:28 AM, Tanu Kaskinen wrote: On Fri, 2013-08-30 at 08:16 +0200, David Henningsson wrote: On 08/29/2013 04:36 PM, Tanu Kaskinen wrote: Sometimes it would be nice to disable module-suspend-on-idle for specific devices. For me the use case is to keep a HDMI sink running all the time to avoid loss of audio when starting to play a stream to the device (the HDMI receiver eats a bit from the beginning of the stream when the device is opened). This is arguably a hacky solution to the problem, but on the other hand, I think it's very sensible to interpret negative timeout in the module-suspend-on-idle.timeout property as disabling the suspending altogher. This is also how the exit-idle-time configuration option behaves (negative value disables automatic exiting). Didn't Arun have another solution for a similar problem? Like a sink ALWAYS_RUNNING flag or something. Yes, he did. That patch was not merged, and I don't know if Arun plans to still work on it. Regardless of Arun's solution, I think it makes sense to support negative value for the module-suspend-on-idle.timeout property to be consistent with other timeout parameters. Hmm, it looks like there already is a module-suspend-on-idle.timeout property and you're just improving its functionality. Then I would typically prefer this over a new sink flag. Even better if the module-suspend-on-idle.timeout was documented though... Arun, would this solve your problem as well? Would it be possible for the two of you to synchronise your efforts? Tanu and I discussed this on IRC. I don't think we should use this property to do what I was trying to with ALWAYS_RUNNING - the module parameters is mechanism, and the always-running-ness is policy. Don't want to mix the two. That said, I've no objection to what the patch is trying to do, per se (i.e. extend/standardise the semantics of a device property that we already support). OK, patch pushed. For the record, I agree with Arun in that this property shouldn't be used in the hardware description files (UCM or alsa-mixer files). I didn't quite understand Arun's explanation about mechanism vs. policy. My reason for disliking the property is that it's specific to module-suspend-on-idle, while idle suspending not recommended is a hardware property that shouldn't be tied to any particular module. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCHv2 44/60] bluetooth: Get BlueZ 5 device object
On Fri, 2013-09-13 at 17:45 -0300, João Paulo Rechi Vita wrote:
On Sun, Aug 18, 2013 at 10:37 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Tue, 2013-08-13 at 01:54 -0300, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
Get the remote device information stored in pa_bluetooth_discovery. This
also creates the mandatory parameter 'path' for module-bluez5-device,
which is used to inform the object path of the remote device in BlueZ on
the module load.
---
src/modules/bluetooth/module-bluez5-device.c | 65
1 file changed, 65 insertions(+)
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index 890f7e4..c1c3477 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -25,6 +25,7 @@
#endif
#include pulsecore/module.h
+#include pulsecore/modargs.h
#include bluez5-util.h
@@ -34,10 +35,74 @@ PA_MODULE_AUTHOR(João Paulo Rechi Vita);
PA_MODULE_DESCRIPTION(BlueZ 5 Bluetooth audio sink and source);
PA_MODULE_VERSION(PACKAGE_VERSION);
PA_MODULE_LOAD_ONCE(false);
+PA_MODULE_USAGE(path=device object path);
+
+static const char* const valid_modargs[] = {
+path,
+NULL
+};
+
+struct userdata {
+pa_module *module;
+pa_core *core;
+pa_modargs *modargs;
The modargs field is not needed in userdata.
How am I supposed to free it in pa__done() then? I don't keep the
pointer to modargs anywhere else.
You only need the modargs in pa__init(), so you can free it in
pa__init().
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] An raop2 support
On Wed, 2013-09-18 at 23:09 -0500, Hajime Fujita wrote: Hi Tanu, Thank you so much for your advice. Tanu Kaskinen wrote: On Sun, 2013-09-15 at 00:24 -0500, Hajime Fujita wrote: For those who are interested in raop2 experiments, As Matthias pointed out [1], current volume calculation is incorrect. https://bugs.freedesktop.org/show_bug.cgi?id=42804#c43 I have uploaded a patch which calculates volume in more reasonable way. https://github.com/hfujita/pulseaudio-raop2/commit/69f3ce883dfb93eb7b24f98b9664e14071672475 First I have to confess that this is the first time for me to deal with the unit 'dB', so maybe I'm doing totally stupid. I'd be very happy if I could have any suggestions from who is more experienced in this field. I started looking at pa_raop_client_set_volume() in src/modules/raop/raop_client.c. It was using pa_sw_volume_to_dB to calculate dB from a linear scale. Correction: pa_volume_t doesn't use a linear scale, it uses a cubic scale (see pa_sw_volume_from_linear() and pa_sw_volume_to_linear()). I see. I should have noticed some comments about cubic scale. http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/pulse/volume.c#n251 Then I learned from code reading that pa_sw_volume_to_dB essentially calculates the following: 20 * log10((v/MAXVOLUME)^3) = 60 * log10(v/MAXVOLUME) This does not fit into the volume scale which RAOP is expecting, where -30.0 db is a kind of minimum [2]. [2]: http://git.zx2c4.com/Airtunes2/about/#id29 Also I looked at packet dumps from iPad and guessed how it calculated dB from a linear scale. It looked like iPad was using the following formula. db = 10 * log10(volume/MAXVOLUME/30) So for the time being I decided to follow this way. The way to convert pa_volume_t to dB is to use pa_sw_volume_to_dB(). Doing anything else means that the result is not dB, it's something different. The way to handle this is to ensure that the volume that is given to pa_raop_client_set_volume() is never outside the supported range. In module-raop-sink.c there's sink_set_volume_cb(), which calls pa_raop_client_set_volume(). In sink_set_volume_cb() there's this line: pa_cvolume_set(hw, s-sample_spec.channels, v); Before that line you should check what dB value v corresponds to, and if it's outside the supported range, then you should modify v to fall into the supported range. Any modification that you make to v will be compensated with software volume, so the modifications shouldn't affect the resulting volume at all, assuming that the RAOP device actually attenuates the signal by the dB amount that you send to it. According to your advice, I inserted an adjustment to v before pa_cvolume_set. Here's the patch. https://github.com/hfujita/pulseaudio-raop2/commit/c6337c0fef7710ee20b42ed3fb87ec12910ce35a Adjustment goes like this (pa_raop_client_adjust_volume()): adj(v) = v * (1 - minv/maxv) + minv; where pa_sw_volume_to_dB(minv) = -30.0, and pa_sw_volume_to_dB(maxv) = 0.0 The idea is that when the original v is 0, adj(0) = minv. And when the original v is maxv, adj(maxv) = maxv; Does this make sense? The idea was to simply adjust v to -30 dB if the original v is less than that. There's no need to do any complicated calculations. As you noted, this also affects the software volume, and in my environment, the actual volume (volume heard from the speaker) may be way too small. What do you mean by way too small? The maximum volume will be the same as before, but the minimum volume will be quieter than before. With the original code you complained that you need to keep the volume at rather low levels, so extending the scale towards low volumes should help with this. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Best Case Latency
On Wed, 2013-09-18 at 04:28 +1000, Patrick Shirkey wrote: Hi, Can someone shed some light on the best case and typical latency that Pulse provides for standard operations? What do you mean by standard operations? I'd estimate volume changes etc. normally (on an ALSA sink) have a few millisecond latency (best case 0.5 ms audio buffer + scheduling latency). Also how that is affected by using jack-sink assuming jack is set to the lowest possible latency that a device can handle? When jackd asks for N frames from PulseAudio, pulseaudio will render N frames. There's no extra buffering done in the Jack sink, so volume changes etc. will have latency that is roughly the same as the normal Jack client latency. If by standard operations you meant the stream latency that PulseAudio clients see (in which case standard operations is a strange way to say it, IMO), then that depends on what the clients request. If they let PulseAudio choose, the latency is typically 2 seconds. If they want minimal latency, with infinite CPU power I think the lower bound is maybe a couple of milliseconds, but since computers don't have infinite CPU power, the practical limit is some tens of milliseconds based on what I've heard (the exact limit naturally depends on how beefy the CPU is). -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] An raop2 support
On Thu, 2013-09-19 at 22:47 -0500, Hajime Fujita wrote: Hi Tanu, Tanu Kaskinen wrote: On Wed, 2013-09-18 at 23:09 -0500, Hajime Fujita wrote: Hi Tanu, Thank you so much for your advice. Tanu Kaskinen wrote: On Sun, 2013-09-15 at 00:24 -0500, Hajime Fujita wrote: For those who are interested in raop2 experiments, As Matthias pointed out [1], current volume calculation is incorrect. https://bugs.freedesktop.org/show_bug.cgi?id=42804#c43 I have uploaded a patch which calculates volume in more reasonable way. https://github.com/hfujita/pulseaudio-raop2/commit/69f3ce883dfb93eb7b24f98b9664e14071672475 First I have to confess that this is the first time for me to deal with the unit 'dB', so maybe I'm doing totally stupid. I'd be very happy if I could have any suggestions from who is more experienced in this field. I started looking at pa_raop_client_set_volume() in src/modules/raop/raop_client.c. It was using pa_sw_volume_to_dB to calculate dB from a linear scale. Correction: pa_volume_t doesn't use a linear scale, it uses a cubic scale (see pa_sw_volume_from_linear() and pa_sw_volume_to_linear()). I see. I should have noticed some comments about cubic scale. http://cgit.freedesktop.org/pulseaudio/pulseaudio/tree/src/pulse/volume.c#n251 Then I learned from code reading that pa_sw_volume_to_dB essentially calculates the following: 20 * log10((v/MAXVOLUME)^3) = 60 * log10(v/MAXVOLUME) This does not fit into the volume scale which RAOP is expecting, where -30.0 db is a kind of minimum [2]. [2]: http://git.zx2c4.com/Airtunes2/about/#id29 Also I looked at packet dumps from iPad and guessed how it calculated dB from a linear scale. It looked like iPad was using the following formula. db = 10 * log10(volume/MAXVOLUME/30) So for the time being I decided to follow this way. The way to convert pa_volume_t to dB is to use pa_sw_volume_to_dB(). Doing anything else means that the result is not dB, it's something different. The way to handle this is to ensure that the volume that is given to pa_raop_client_set_volume() is never outside the supported range. In module-raop-sink.c there's sink_set_volume_cb(), which calls pa_raop_client_set_volume(). In sink_set_volume_cb() there's this line: pa_cvolume_set(hw, s-sample_spec.channels, v); Before that line you should check what dB value v corresponds to, and if it's outside the supported range, then you should modify v to fall into the supported range. Any modification that you make to v will be compensated with software volume, so the modifications shouldn't affect the resulting volume at all, assuming that the RAOP device actually attenuates the signal by the dB amount that you send to it. According to your advice, I inserted an adjustment to v before pa_cvolume_set. Here's the patch. https://github.com/hfujita/pulseaudio-raop2/commit/c6337c0fef7710ee20b42ed3fb87ec12910ce35a Adjustment goes like this (pa_raop_client_adjust_volume()): adj(v) = v * (1 - minv/maxv) + minv; where pa_sw_volume_to_dB(minv) = -30.0, and pa_sw_volume_to_dB(maxv) = 0.0 The idea is that when the original v is 0, adj(0) = minv. And when the original v is maxv, adj(maxv) = maxv; Does this make sense? The idea was to simply adjust v to -30 dB if the original v is less than that. There's no need to do any complicated calculations. Do you mean something like v = max(minv, v); /* where minv corresponds to -30.0dB */ ? Yes. If I do that, resulted volume will be almost muted in the range of 0% = v = 31%, then suddenly goes up beyond 31%. (31% is where dB becomes -30.0) I was afraid that this was not an intuitive behavior to users. It sounds like the RAOP device doesn't apply the attenuation as it should. Could you verify this? If you don't apply any software volume (set s-soft_volume to PA_VOLUME_NORM), and you tell the RAOP device to use volume -30.0, do you get very quiet (but not silent) audio? And if you then tell the RAOP device to use volume -29.9, do you get much louder audio? This shouldn't happen. A change of 0.1 dB should have very small effect. As you noted, this also affects the software volume, and in my environment, the actual volume (volume heard from the speaker) may be way too small. What do you mean by way too small? The maximum volume will be the same as before, but the minimum volume will be quieter than before. With the original code you complained that you need to keep the volume at rather low levels, so extending the scale towards low volumes should help with this. Well, this could be just my personal feeling. Since it was a bit loud before (however it was almost the same volume level as iPad), this time I felt it was way too small. I wonder if it makes sense to 1) adjust v as adj(v) = v * (1 - minv/maxv) + minv; and 2) use the original v for pa_cvolume_set(), then 3) use adj(v
Re: [pulseaudio-discuss] [PATCH] sink: Increase max sink inputs per sink
On Fri, 2013-09-20 at 09:29 +0200, David Henningsson wrote: On 09/20/2013 05:44 AM, Arun Raghavan wrote: We're hitting the 32 sink-input limit quite often with increasing use of PA by web browsers, so let's increase this limit. Does this increase the risk of us being the target of attacks; i e, if a web browser deliberately starts 255 streams, would we risk getting killed by using too much rtprio cpu time? Even with the limit being at 32 it's possible to do a DoS attack by creating streams with many channels and a high sample rate. As far as I can see, raising the limit doesn't create any new threats. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/3] hashmap: Add a key+value iterator
On Fri, 2013-09-20 at 17:27 +0530, Arun Raghavan wrote: --- src/pulsecore/hashmap.h | 4 1 file changed, 4 insertions(+) diff --git a/src/pulsecore/hashmap.h b/src/pulsecore/hashmap.h index ae030ed..e42732a 100644 --- a/src/pulsecore/hashmap.h +++ b/src/pulsecore/hashmap.h @@ -85,6 +85,10 @@ void* pa_hashmap_last(pa_hashmap *h); #define PA_HASHMAP_FOREACH(e, h, state) \ for ((state) = NULL, (e) = pa_hashmap_iterate((h), (state), NULL); (e); (e) = pa_hashmap_iterate((h), (state), NULL)) +/* A macro to ease itration through all key, value pairs */ +#define PA_HASHMAP_FOREACH_KV(k, e, h, state) \ +for ((state) = NULL, (e) = pa_hashmap_iterate((h), (state), (const void **) (k)); (e); (e) = pa_hashmap_iterate((h), (state), (const void **) (k))) Are the (const void **) casts really needed? -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/3] hashmap: Add a key+value iterator
On Fri, 2013-09-20 at 17:55 +0530, Arun Raghavan wrote: On Fri, 2013-09-20 at 15:13 +0300, Tanu Kaskinen wrote: On Fri, 2013-09-20 at 17:27 +0530, Arun Raghavan wrote: --- src/pulsecore/hashmap.h | 4 1 file changed, 4 insertions(+) diff --git a/src/pulsecore/hashmap.h b/src/pulsecore/hashmap.h index ae030ed..e42732a 100644 --- a/src/pulsecore/hashmap.h +++ b/src/pulsecore/hashmap.h @@ -85,6 +85,10 @@ void* pa_hashmap_last(pa_hashmap *h); #define PA_HASHMAP_FOREACH(e, h, state) \ for ((state) = NULL, (e) = pa_hashmap_iterate((h), (state), NULL); (e); (e) = pa_hashmap_iterate((h), (state), NULL)) +/* A macro to ease itration through all key, value pairs */ +#define PA_HASHMAP_FOREACH_KV(k, e, h, state) \ +for ((state) = NULL, (e) = pa_hashmap_iterate((h), (state), (const void **) (k)); (e); (e) = pa_hashmap_iterate((h), (state), (const void **) (k))) Are the (const void **) casts really needed? Yes, the compiler complains about casts from (const type **) to (const void **). Ok. Silly compiler. I noticed that this casting is not done anywhere in the current code base, but on closer look it turned out that in every case the original type is const void ** anyway. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 2/3] device-port: Add mechanism to free implementation data
On Fri, 2013-09-20 at 17:56 +0530, Arun Raghavan wrote:
On Fri, 2013-09-20 at 15:21 +0300, Tanu Kaskinen wrote:
On Fri, 2013-09-20 at 17:27 +0530, Arun Raghavan wrote:
This will be needed if the implementation data stores pointers to
additional data that needs to be freed as well.
---
src/pulsecore/device-port.c | 3 +++
src/pulsecore/device-port.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/src/pulsecore/device-port.c b/src/pulsecore/device-port.c
index 0b65d5c..ac2c95e 100644
--- a/src/pulsecore/device-port.c
+++ b/src/pulsecore/device-port.c
@@ -94,6 +94,9 @@ static void device_port_free(pa_object *o) {
pa_assert(p);
pa_assert(pa_device_port_refcnt(p) == 0);
+if (p-impl_free)
+p-impl_free(p);
+
if (p-proplist)
pa_proplist_free(p-proplist);
diff --git a/src/pulsecore/device-port.h b/src/pulsecore/device-port.h
index b10d554..b5e80a5 100644
--- a/src/pulsecore/device-port.h
+++ b/src/pulsecore/device-port.h
@@ -54,6 +54,9 @@ struct pa_device_port {
pa_direction_t direction;
int64_t latency_offset;
+/* Free the extra implementation specific data. Called before other
members are freed. */
+void (*impl_free)(pa_device_port *port);
+
/* .. followed by some implementation specific data */
};
It's sad that this is needed, but ack. (I have probably said this
earlier: I'd like ports to not require refcounting, in which case they
would probably be always owned by cards.)
In which case the card would have to have a way to check what kind of
implementation-specific data is there and call the appropriate free
function. Doable, but not the prettiest, imo.
If you mean that pulsecore/card.c would know what kind of data the alsa
card is storing in the port, I don't see how that would be doable. But
that wasn't my idea anyway. If cards always owned the ports,
pa_device_port could have a userdata pointer set by the alsa card, and
the alsa card could free the extra port data at the same time when it
frees the pa_card object.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 08/41] bluetooth: Implement org.bluez.MediaEndpoint1.SetConfiguration()
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/bluez5-util.c | 176
+++-
src/modules/bluetooth/bluez5-util.h | 5 +
2 files changed, 179 insertions(+), 2 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 983f707..b0d3864 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -33,6 +33,8 @@
#include pulsecore/refcnt.h
#include pulsecore/shared.h
+#include a2dp-codecs.h
+
#include bluez5-util.h
#define BLUEZ_SERVICE org.bluez
@@ -406,12 +408,182 @@ fail:
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
}
+const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
+switch(profile) {
+case PA_BLUETOOTH_PROFILE_A2DP_SINK:
+return a2dp_sink;
+case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
+return a2dp_source;
+case PA_BLUETOOTH_PROFILE_OFF:
+return off;
+}
+
+return NULL;
+}
+
static DBusMessage *endpoint_set_configuration(DBusConnection *conn,
DBusMessage *m, void *userdata) {
+pa_bluetooth_discovery *y = userdata;
+pa_bluetooth_device *d;
+pa_bluetooth_transport *t;
+const char *sender, *path, *endpoint_path, *dev_path = NULL, *uuid =
NULL;
+const uint8_t *config = NULL;
+int size = 0;
+pa_bluetooth_profile_t p = PA_BLUETOOTH_PROFILE_OFF;
+DBusMessageIter args, props;
DBusMessage *r;
-pa_assert_se(r = dbus_message_new_error(m,
BLUEZ_MEDIA_ENDPOINT_INTERFACE .Error.NotImplemented,
-Method not implemented));
+if (!dbus_message_iter_init(m, args) ||
!pa_streq(dbus_message_get_signature(m), oa{sv})) {
+pa_log_error(Invalid signature for method SetConfiguration());
+goto fail2;
+}
+
+dbus_message_iter_get_basic(args, path);
+
+if (pa_hashmap_get(y-transports, path)) {
+pa_log_error(Endpoint SetConfiguration(): Transport %s is already
configured., path);
+goto fail2;
+}
+
+pa_assert_se(dbus_message_iter_next(args));
+
+dbus_message_iter_recurse(args, props);
+if (dbus_message_iter_get_arg_type(props) != DBUS_TYPE_DICT_ENTRY)
+goto fail;
+
+/* Read transport properties */
+while (dbus_message_iter_get_arg_type(props) == DBUS_TYPE_DICT_ENTRY) {
+const char *key;
+DBusMessageIter value, entry;
+int var;
+
+dbus_message_iter_recurse(props, entry);
+dbus_message_iter_get_basic(entry, key);
+
+dbus_message_iter_next(entry);
+dbus_message_iter_recurse(entry, value);
+
+var = dbus_message_iter_get_arg_type(value);
+
+if (pa_streq(key, UUID)) {
+if (var != DBUS_TYPE_STRING) {
+pa_log_error(Property %s of wrong type %c, key, (char)var);
+goto fail;
+}
+
+dbus_message_iter_get_basic(value, uuid);
+
+endpoint_path = dbus_message_get_path(m);
+if (pa_streq(endpoint_path, A2DP_SOURCE_ENDPOINT)) {
+if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SOURCE))
+p = PA_BLUETOOTH_PROFILE_A2DP_SINK;
+} else if (pa_streq(endpoint_path, A2DP_SINK_ENDPOINT)) {
+if (pa_streq(uuid, PA_BLUETOOTH_UUID_A2DP_SINK))
+p = PA_BLUETOOTH_PROFILE_A2DP_SOURCE;
+}
+
+if (p == PA_BLUETOOTH_PROFILE_OFF) {
+pa_log_error(UUID %s of transport %s incompatible with
endpoint %s, uuid, path, endpoint_path);
+goto fail;
+}
+} else if (pa_streq(key, Device)) {
+if (var != DBUS_TYPE_OBJECT_PATH) {
+pa_log_error(Property %s of wrong type %c, key, (char)var);
+goto fail;
+}
+
+dbus_message_iter_get_basic(value, dev_path);
+} else if (pa_streq(key, Configuration)) {
+DBusMessageIter array;
+a2dp_sbc_t *c;
+
+if (var != DBUS_TYPE_ARRAY) {
+pa_log_error(Property %s of wrong type %c, key, (char)var);
+goto fail;
+}
+
+dbus_message_iter_recurse(value, array);
+var = dbus_message_iter_get_arg_type(array);
+if (var != DBUS_TYPE_BYTE) {
+pa_log_error(%s is an array of wrong type %c, key,
(char)var);
+goto fail;
+}
+
+dbus_message_iter_get_fixed_array(array, config, size);
+if (size != sizeof(a2dp_sbc_t)) {
+pa_log_error(Properties array of invalid size);
Properties array is not very accurate choice of words. Configuration
array or Configuration blob would be better.
+
Re: [pulseaudio-discuss] [PATCH v4 09/41] bluetooth: Implement org.bluez.MediaEndpoint1.SelectConfiguration()
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
static DBusMessage *endpoint_select_configuration(DBusConnection *conn,
DBusMessage *m, void *userdata) {
+pa_bluetooth_discovery *y = userdata;
+a2dp_sbc_t *cap, config;
+uint8_t *pconf = (uint8_t *) config;
+int i, size;
DBusMessage *r;
+DBusError err;
-pa_assert_se(r = dbus_message_new_error(m,
BLUEZ_MEDIA_ENDPOINT_INTERFACE .Error.NotImplemented,
-Method not implemented));
+static const struct {
+uint32_t rate;
+uint8_t cap;
+} freq_table[] = {
+{ 16000U, SBC_SAMPLING_FREQ_16000 },
+{ 32000U, SBC_SAMPLING_FREQ_32000 },
+{ 44100U, SBC_SAMPLING_FREQ_44100 },
+{ 48000U, SBC_SAMPLING_FREQ_48000 }
+};
+
+dbus_error_init(err);
+
+if (!dbus_message_get_args(m, err, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE,
cap, size, DBUS_TYPE_INVALID)) {
+pa_log_error(Endpoint SelectConfiguration(): %s, err.message);
+dbus_error_free(err);
+goto fail;
+}
+
+if (size != sizeof(config)) {
+pa_log_error(Capabilities array has invalid size);
+goto fail;
+}
+
+pa_zero(config);
+
+/* Find the lowest freq that is at least as high as the requested
sampling rate */
+for (i = 0; (unsigned) i PA_ELEMENTSOF(freq_table); i++)
+if (freq_table[i].rate = y-core-default_sample_spec.rate
(cap-frequency freq_table[i].cap)) {
+config.frequency = freq_table[i].cap;
+break;
+}
+if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
+for (--i; i = 0; i--) {
+if (cap-frequency freq_table[i].cap) {
+config.frequency = freq_table[i].cap;
+break;
+}
+}
+
+if (i 0) {
+pa_log_error(Not suitable sample rate);
+goto fail;
+}
+}
+
+pa_assert((unsigned) i PA_ELEMENTSOF(freq_table));
+
+if (y-core-default_sample_spec.channels = 1)
+if (cap-channel_mode SBC_CHANNEL_MODE_MONO)
+config.channel_mode = SBC_CHANNEL_MODE_MONO;
You forgot to fix this code. The problem still exists that if
default_sample_spec.channels is 1 and the cap-channel_mode doesn't
contain SBC_CHANNEL_MODE_MONO, then config.channel_mode is left
uninitialized.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 13/41] bluetooth: Handle InterfacesAdded and InterfacesRemoved
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
This code is based on previous work by Mikel Astiz.
---
src/modules/bluetooth/bluez5-util.c | 55
+
1 file changed, 55 insertions(+)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index f5d4300..bf4a046 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -38,6 +38,7 @@
#include bluez5-util.h
#define BLUEZ_SERVICE org.bluez
+#define BLUEZ_DEVICE_INTERFACE BLUEZ_SERVICE .Device1
#define BLUEZ_MEDIA_ENDPOINT_INTERFACE BLUEZ_SERVICE .MediaEndpoint1
#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE .MediaTransport1
@@ -476,6 +477,53 @@ static DBusHandlerResult filter_cb(DBusConnection *bus,
DBusMessage *m, void *us
}
return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+} else if (dbus_message_is_signal(m,
org.freedesktop.DBus.ObjectManager, InterfacesAdded)) {
+DBusMessageIter arg_i;
+
+if (!y-objects_listed)
+return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; /* No reply received
yet from GetManagedObjects */
+
+if (!dbus_message_iter_init(m, arg_i) ||
!pa_streq(dbus_message_get_signature(m), oa{sa{sv}})) {
+pa_log_error(Invalid signature found in InterfacesAdded);
+goto fail;
+}
+
+/* TODO: parse interfaces and properties */
+
+return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+} else if (dbus_message_is_signal(m,
org.freedesktop.DBus.ObjectManager, InterfacesRemoved)) {
+const char *p;
+DBusMessageIter arg_i;
+DBusMessageIter element_i;
+
+if (!y-objects_listed)
+return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; /* No reply received
yet from GetManagedObjects */
+
+if (!dbus_message_iter_init(m, arg_i) ||
!pa_streq(dbus_message_get_signature(m), oas)) {
+pa_log_error(Invalid signature found in InterfacesRemoved);
+goto fail;
+}
+
+dbus_message_iter_get_basic(arg_i, p);
+
+pa_assert_se(dbus_message_iter_next(arg_i));
+pa_assert(dbus_message_iter_get_arg_type(arg_i) == DBUS_TYPE_ARRAY);
+
+dbus_message_iter_recurse(arg_i, element_i);
+
+while (dbus_message_iter_get_arg_type(element_i) ==
DBUS_TYPE_STRING) {
+const char *iface;
+
+dbus_message_iter_get_basic(element_i, iface);
+
+if (pa_streq(iface, BLUEZ_DEVICE_INTERFACE))
+device_remove(y, p);
+
+dbus_message_iter_next(element_i);
+}
This code handles only device removal, but I suppose adapter removal
should be handled too?
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 14/41] bluetooth: Parse BlueZ 5 D-Bus interfaces
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
Parse the arguments of the InterfacesAdded signal and the
GetManagedObjects() reply.
This code is based on previous work by Mikel Astiz.
---
src/modules/bluetooth/bluez5-util.c | 72
+++--
1 file changed, 70 insertions(+), 2 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index bf4a046..7f0a7ec 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -38,6 +38,7 @@
#include bluez5-util.h
#define BLUEZ_SERVICE org.bluez
+#define BLUEZ_ADAPTER_INTERFACE BLUEZ_SERVICE .Adapter1
#define BLUEZ_DEVICE_INTERFACE BLUEZ_SERVICE .Device1
#define BLUEZ_MEDIA_ENDPOINT_INTERFACE BLUEZ_SERVICE .MediaEndpoint1
#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE .MediaTransport1
@@ -379,6 +380,73 @@ static void adapter_remove_all(pa_bluetooth_discovery
*y) {
}
}
+static void parse_interfaces_and_properties(pa_bluetooth_discovery *y,
DBusMessageIter *dict_i) {
+DBusMessageIter element_i;
+const char *path;
+
+pa_assert(dbus_message_iter_get_arg_type(dict_i) ==
DBUS_TYPE_OBJECT_PATH);
+dbus_message_iter_get_basic(dict_i, path);
+
+pa_assert_se(dbus_message_iter_next(dict_i));
+pa_assert(dbus_message_iter_get_arg_type(dict_i) == DBUS_TYPE_ARRAY);
+
+dbus_message_iter_recurse(dict_i, element_i);
+
+while (dbus_message_iter_get_arg_type(element_i) ==
DBUS_TYPE_DICT_ENTRY) {
+DBusMessageIter iface_i;
+const char *interface;
+
+dbus_message_iter_recurse(element_i, iface_i);
+
+pa_assert(dbus_message_iter_get_arg_type(iface_i) ==
DBUS_TYPE_STRING);
+dbus_message_iter_get_basic(iface_i, interface);
+
+pa_assert_se(dbus_message_iter_next(iface_i));
+pa_assert(dbus_message_iter_get_arg_type(iface_i) ==
DBUS_TYPE_ARRAY);
+
+if (pa_streq(interface, BLUEZ_ADAPTER_INTERFACE)) {
+pa_bluetooth_adapter *a;
+
+if ((a = pa_hashmap_get(y-adapters, path))) {
+pa_log_error(Found duplicated D-Bus path for device %s,
path);
+return;
+} else
+a = adapter_create(y, path);
+
+pa_log_debug(Adapter %s found, path);
+
+/* TODO: parse adapter properties and register endpoints */
+
+} else if (pa_streq(interface, BLUEZ_DEVICE_INTERFACE)) {
+pa_bluetooth_device *d;
+
+if ((d = pa_hashmap_get(y-devices, path))) {
+if (d-device_info_valid == 1) {
+pa_log_error(Found duplicated D-Bus path for device
%s, path);
+return;
+}
+
+if (d-device_info_valid == -1) {
+pa_log_notice(Device %s was known before but had
invalid information, reseting, path);
+d-device_info_valid = 0;
Didn't we agree that the device shouldn't be reset? If the device is
reset, the device properties should be reset too, otherwise the old
property values can leak to the new initialization. But as discussed
last round, resetting the property values is error prone, so if the
device initialization fails once, then the device should stay
uninitialized forever. We don't need to resurrect failed devices.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 15/41] bluetooth: Parse BlueZ 5 adapter properties
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
static void parse_interfaces_and_properties(pa_bluetooth_discovery *y,
DBusMessageIter *dict_i) {
DBusMessageIter element_i;
const char *path;
@@ -415,7 +474,8 @@ static void
parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
pa_log_debug(Adapter %s found, path);
-/* TODO: parse adapter properties and register endpoints */
+parse_adapter_properties(a, iface_i, false);
If parsing fails, or if the Address property is missing, the adapter
should be marked as invalid.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 16/41] bluetooth: Register endpoints with BlueZ 5 adapter
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/bluez5-util.c | 82
-
1 file changed, 81 insertions(+), 1 deletion(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 4bc5967..03c73c9 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -40,9 +40,12 @@
#define BLUEZ_SERVICE org.bluez
#define BLUEZ_ADAPTER_INTERFACE BLUEZ_SERVICE .Adapter1
#define BLUEZ_DEVICE_INTERFACE BLUEZ_SERVICE .Device1
+#define BLUEZ_MEDIA_INTERFACE BLUEZ_SERVICE .Media1
#define BLUEZ_MEDIA_ENDPOINT_INTERFACE BLUEZ_SERVICE .MediaEndpoint1
#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE .MediaTransport1
+#define BLUEZ_ERROR_NOT_SUPPORTED org.bluez.Error.NotSupported
+
#define A2DP_SOURCE_ENDPOINT /MediaEndpoint/A2DPSource
#define A2DP_SINK_ENDPOINT /MediaEndpoint/A2DPSink
@@ -439,6 +442,81 @@ static int parse_adapter_properties(pa_bluetooth_adapter
*a, DBusMessageIter *i,
return 0;
}
+static void register_endpoint_reply(DBusPendingCall *pending, void
*userdata) {
+DBusMessage *r;
+pa_dbus_pending *p;
+pa_bluetooth_discovery *y;
+char *endpoint;
+
+pa_assert(pending);
+pa_assert_se(p = userdata);
+pa_assert_se(y = p-context_data);
+pa_assert_se(endpoint = p-call_data);
+pa_assert_se(r = dbus_pending_call_steal_reply(pending));
+
+if (dbus_message_is_error(r, DBUS_ERROR_SERVICE_UNKNOWN)) {
+pa_log_debug(Bluetooth daemon is apparently not available);
+device_remove_all(y);
Adapters should be removed too. I think there should be a function that
takes care of clearing everything, so that it's not necessary to
remember to remove both devices and adapters everywhere where we notice
that bluetoothd is non-functional.
+goto finish;
+}
+
+if (dbus_message_is_error(r, BLUEZ_ERROR_NOT_SUPPORTED)) {
+pa_log_info(Couldn't register endpoint %s because it is disabled in
BlueZ, endpoint);
+goto finish;
+}
+
+if (dbus_message_get_type(r) == DBUS_MESSAGE_TYPE_ERROR) {
+pa_log_error(org.bluez.Media.RegisterEndpoint() failed: %s: %s,
dbus_message_get_error_name(r),
+ pa_dbus_get_error_message(r));
The log message uses wrong interface name (Media instead of Media1).
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 15/41] bluetooth: Parse BlueZ 5 adapter properties
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
+static int parse_adapter_properties(pa_bluetooth_adapter *a, DBusMessageIter
*i, bool is_property_change) {
+DBusMessageIter element_i;
+
+pa_assert(a);
+
+dbus_message_iter_recurse(i, element_i);
+
+while (dbus_message_iter_get_arg_type(element_i) ==
DBUS_TYPE_DICT_ENTRY) {
+DBusMessageIter dict_i, variant_i;
+const char *key;
+
+dbus_message_iter_recurse(element_i, dict_i);
+
+key = check_variant_property(dict_i);
+if (key == NULL) {
+pa_log_error(Received invalid property for adapter %s,
a-path);
+return -1;
+}
+
+dbus_message_iter_recurse(dict_i, variant_i);
+
+if (dbus_message_iter_get_arg_type(variant_i) == DBUS_TYPE_STRING
pa_streq(key, Address)) {
+char *value;
+
+dbus_message_iter_get_basic(variant_i, value);
+a-address = pa_xstrdup(value);
+}
Can the Address property change? If not, it should be checked if
is_property_change is true, and if it is, then complain loudly. If it
can change, then the old address should be freed before assigning the
new address.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 14/41] bluetooth: Parse BlueZ 5 D-Bus interfaces
On Sat, 2013-09-21 at 14:02 +0300, Tanu Kaskinen wrote:
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
Parse the arguments of the InterfacesAdded signal and the
GetManagedObjects() reply.
This code is based on previous work by Mikel Astiz.
---
src/modules/bluetooth/bluez5-util.c | 72
+++--
1 file changed, 70 insertions(+), 2 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index bf4a046..7f0a7ec 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -38,6 +38,7 @@
#include bluez5-util.h
#define BLUEZ_SERVICE org.bluez
+#define BLUEZ_ADAPTER_INTERFACE BLUEZ_SERVICE .Adapter1
#define BLUEZ_DEVICE_INTERFACE BLUEZ_SERVICE .Device1
#define BLUEZ_MEDIA_ENDPOINT_INTERFACE BLUEZ_SERVICE .MediaEndpoint1
#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE .MediaTransport1
@@ -379,6 +380,73 @@ static void adapter_remove_all(pa_bluetooth_discovery
*y) {
}
}
+static void parse_interfaces_and_properties(pa_bluetooth_discovery *y,
DBusMessageIter *dict_i) {
+DBusMessageIter element_i;
+const char *path;
+
+pa_assert(dbus_message_iter_get_arg_type(dict_i) ==
DBUS_TYPE_OBJECT_PATH);
+dbus_message_iter_get_basic(dict_i, path);
+
+pa_assert_se(dbus_message_iter_next(dict_i));
+pa_assert(dbus_message_iter_get_arg_type(dict_i) == DBUS_TYPE_ARRAY);
+
+dbus_message_iter_recurse(dict_i, element_i);
+
+while (dbus_message_iter_get_arg_type(element_i) ==
DBUS_TYPE_DICT_ENTRY) {
+DBusMessageIter iface_i;
+const char *interface;
+
+dbus_message_iter_recurse(element_i, iface_i);
+
+pa_assert(dbus_message_iter_get_arg_type(iface_i) ==
DBUS_TYPE_STRING);
+dbus_message_iter_get_basic(iface_i, interface);
+
+pa_assert_se(dbus_message_iter_next(iface_i));
+pa_assert(dbus_message_iter_get_arg_type(iface_i) ==
DBUS_TYPE_ARRAY);
+
+if (pa_streq(interface, BLUEZ_ADAPTER_INTERFACE)) {
+pa_bluetooth_adapter *a;
+
+if ((a = pa_hashmap_get(y-adapters, path))) {
+pa_log_error(Found duplicated D-Bus path for device %s,
path);
+return;
+} else
+a = adapter_create(y, path);
+
+pa_log_debug(Adapter %s found, path);
+
+/* TODO: parse adapter properties and register endpoints */
+
+} else if (pa_streq(interface, BLUEZ_DEVICE_INTERFACE)) {
+pa_bluetooth_device *d;
+
+if ((d = pa_hashmap_get(y-devices, path))) {
+if (d-device_info_valid == 1) {
+pa_log_error(Found duplicated D-Bus path for device
%s, path);
+return;
+}
+
+if (d-device_info_valid == -1) {
+pa_log_notice(Device %s was known before but had
invalid information, reseting, path);
+d-device_info_valid = 0;
Didn't we agree that the device shouldn't be reset? If the device is
reset, the device properties should be reset too, otherwise the old
property values can leak to the new initialization. But as discussed
last round, resetting the property values is error prone, so if the
device initialization fails once, then the device should stay
uninitialized forever. We don't need to resurrect failed devices.
Never mind, I see you fixed this in a later patch (it would have been
good to fix it already in this patch, but no big deal).
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 18/41] bluetooth: Parse BlueZ 5 device properties
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
This code is based on previous work by Mikel Astiz.
---
src/modules/bluetooth/bluez5-util.c | 164
++--
src/modules/bluetooth/bluez5-util.h | 6 ++
2 files changed, 161 insertions(+), 9 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 7fc7d50..49d5c38 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -297,6 +297,7 @@ static pa_bluetooth_device*
device_create(pa_bluetooth_discovery *y, const char
d = pa_xnew0(pa_bluetooth_device, 1);
d-discovery = y;
d-path = pa_xstrdup(path);
+d-uuids = pa_hashmap_new(pa_idxset_string_hash_func,
pa_idxset_string_compare_func);
pa_hashmap_put(y-devices, d-path, d);
@@ -348,6 +349,9 @@ static void device_free(pa_bluetooth_device *d) {
pa_bluetooth_transport_free(t);
}
+if (d-uuids)
+pa_hashmap_free(d-uuids, NULL);
The hashmap is not necessarily empty, so this leaks the contained uuid
strings.
The hashmap interface changed recently, and now the correct way to deal
with this is to call pa_hashmap_new_full(), and provide a free callback
for the value (since the key is the same object as the value, don't
provide free callbacks for both the value and the key, otherwise there
will be double freeing).
+
d-discovery = NULL;
d-adapter = NULL;
pa_xfree(d-path);
@@ -408,6 +412,144 @@ static void adapter_remove_all(pa_bluetooth_discovery
*y) {
}
}
+static void parse_device_property(pa_bluetooth_device *d, DBusMessageIter
*i, bool is_property_change) {
+const char *key;
+DBusMessageIter variant_i;
+
+pa_assert(d);
+
+key = check_variant_property(i);
+if (key == NULL) {
+pa_log_error(Received invalid property for device %s, d-path);
+return;
+}
+
+dbus_message_iter_recurse(i, variant_i);
+
+switch (dbus_message_iter_get_arg_type(variant_i)) {
+
+case DBUS_TYPE_STRING: {
+const char *value;
+dbus_message_iter_get_basic(variant_i, value);
+
+if (pa_streq(key, Alias)) {
+pa_xfree(d-alias);
+d-alias = pa_xstrdup(value);
+pa_log_debug(%s: %s, key, value);
+} else if (pa_streq(key, Address)) {
+if (is_property_change) {
+pa_log_warn(Device property 'Address' expected to be
constant but changed for %s, ignoring, d-path);
+return;
+}
+
+if (d-address) {
+pa_log_warn(Device %s: Received a duplicate 'Address'
property, ignoring, d-path);
+return;
+}
+
+d-address = pa_xstrdup(value);
+pa_log_debug(%s: %s, key, value);
+}
+
+break;
+}
+
+case DBUS_TYPE_OBJECT_PATH: {
+const char *value;
+dbus_message_iter_get_basic(variant_i, value);
+
+if (pa_streq(key, Adapter)) {
+
+if (is_property_change) {
+pa_log_warn(Device property 'Adapter' expected to be
constant but changed for %s, ignoring, d-path);
+return;
+}
+
+if (d-adapter) {
+pa_log_warn(Device %s: Received a duplicate 'Adapter'
property, ignoring, d-path);
+return;
+}
+
+d-adapter = pa_hashmap_get(d-discovery-adapters, value);
+if (!d-adapter) {
+pa_log_warn(Device %s: 'Adapter' property references an
unknown adapter %s., d-path, value);
This is normal behaviour, no need to log anything, especially not at the
warning level.
+d-_adapter_path = pa_xstrdup(value);
+break;
Does it really make sense to break here? The only thing this skips is
the line that logs the property key and value, and I don't see any
reason to skip that.
+}
+
+pa_log_debug(%s: %s, key, value);
+}
+
+break;
+}
+
+case DBUS_TYPE_UINT32: {
+uint32_t value;
+dbus_message_iter_get_basic(variant_i, value);
+
+if (pa_streq(key, Class)) {
+d-class_of_device = value;
+pa_log_debug(%s: %d, key, value);
+}
+
+break;
+}
+
+case DBUS_TYPE_ARRAY: {
+DBusMessageIter ai;
+dbus_message_iter_recurse(variant_i, ai);
+
+if (dbus_message_iter_get_arg_type(ai) == DBUS_TYPE_STRING
pa_streq(key, UUIDs)) {
+/* bluetoothd never removes UUIDs from a device object so
there
+
Re: [pulseaudio-discuss] [PATCH v4 34/41] bluetooth: Process sink messages for BlueZ 5 cards
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/module-bluez5-device.c | 75
1 file changed, 75 insertions(+)
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index 4f2fbd8..31088b7 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -700,6 +700,80 @@ static int add_source(struct userdata *u) {
return 0;
}
+/* Run from IO thread */
+static int sink_process_msg(pa_msgobject *o, int code, void *data, int64_t
offset, pa_memchunk *chunk) {
+struct userdata *u = PA_SINK(o)-userdata;
+bool failed = false;
+int r;
+
+pa_assert(u-sink == PA_SINK(o));
+pa_assert(u-transport);
+
+switch (code) {
+
+case PA_SINK_MESSAGE_SET_STATE:
+
+switch ((pa_sink_state_t) PA_PTR_TO_UINT(data)) {
+
+case PA_SINK_SUSPENDED:
+/* Ignore if transition is
PA_SINK_INIT-PA_SINK_SUSPENDED */
+if (!PA_SINK_IS_OPENED(u-sink-thread_info.state))
+break;
+
+/* Stop the device if the source is suspended as well */
+if (!u-source || u-source-state ==
PA_SOURCE_SUSPENDED)
+/* We deliberately ignore whether stopping
+ * actually worked. Since the stream_fd is
+ * closed it doesn't really matter */
+transport_release(u);
+
+break;
+
+case PA_SINK_IDLE:
+case PA_SINK_RUNNING:
+if (u-sink-thread_info.state != PA_SINK_SUSPENDED)
+break;
+
+/* Resume the device if the source was suspended as well
*/
+if (!u-source ||
!PA_SOURCE_IS_OPENED(u-source-thread_info.state)) {
+if (transport_acquire(u, false) 0)
+failed = true;
+else
+setup_stream(u);
+}
+
+break;
+
+case PA_SINK_UNLINKED:
+case PA_SINK_INIT:
+case PA_SINK_INVALID_STATE:
+break;
+}
+
+break;
+
+case PA_SINK_MESSAGE_GET_LATENCY: {
+pa_usec_t wi, ri;
+
+if (u-read_smoother) {
+ri = pa_smoother_get(u-read_smoother, pa_rtclock_now());
+wi = pa_bytes_to_usec(u-write_index + u-write_block_size,
u-sample_spec);
+} else {
+ri = pa_rtclock_now() - u-started_at;
+wi = pa_bytes_to_usec(u-write_index, u-sample_spec);
+}
+
+*((pa_usec_t*) data) = FIXED_LATENCY_PLAYBACK_A2DP + wi - ri 0
? FIXED_LATENCY_PLAYBACK_A2DP + wi - ri : 0;
The compiler treats the result of FIXED_LATENCY_PLAYBACK_A2DP + wi -
ri as an unsigned integer, so it's never negative, and thus the
comparison to 0 doesn't work as intended.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 35/41] bluetooth: Process source messages for BlueZ 5 cards
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/module-bluez5-device.c | 77
1 file changed, 77 insertions(+)
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index 31088b7..d9e1fc6 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -660,6 +660,82 @@ static void setup_stream(struct userdata *u) {
u-read_smoother = pa_smoother_new(PA_USEC_PER_SEC,
2*PA_USEC_PER_SEC, true, true, 10, pa_rtclock_now(), true);
}
+/* Run from IO thread */
+static int source_process_msg(pa_msgobject *o, int code, void *data, int64_t
offset, pa_memchunk *chunk) {
+struct userdata *u = PA_SOURCE(o)-userdata;
+bool failed = false;
+int r;
+
+pa_assert(u-source == PA_SOURCE(o));
+pa_assert(u-transport);
+
+switch (code) {
+
+case PA_SOURCE_MESSAGE_SET_STATE:
+
+switch ((pa_source_state_t) PA_PTR_TO_UINT(data)) {
+
+case PA_SOURCE_SUSPENDED:
+/* Ignore if transition is
PA_SOURCE_INIT-PA_SOURCE_SUSPENDED */
+if (!PA_SOURCE_IS_OPENED(u-source-thread_info.state))
+break;
+
+/* Stop the device if the sink is suspended as well */
+if (!u-sink || u-sink-state == PA_SINK_SUSPENDED)
+transport_release(u);
+
+if (u-read_smoother)
+pa_smoother_pause(u-read_smoother,
pa_rtclock_now());
+
+break;
+
+case PA_SOURCE_IDLE:
+case PA_SOURCE_RUNNING:
+if (u-source-thread_info.state != PA_SOURCE_SUSPENDED)
+break;
+
+/* Resume the device if the sink was suspended as well */
+if (!u-sink ||
!PA_SINK_IS_OPENED(u-sink-thread_info.state)) {
+if (transport_acquire(u, false) 0)
+failed = true;
+else
+setup_stream(u);
+}
+
+/* We don't resume the smoother here. Instead we
+ * wait until the first packet arrives */
+
+break;
+
+case PA_SOURCE_UNLINKED:
+case PA_SOURCE_INIT:
+case PA_SOURCE_INVALID_STATE:
+break;
+}
+
+break;
+
+case PA_SOURCE_MESSAGE_GET_LATENCY: {
+pa_usec_t wi, ri;
+
+if (u-read_smoother) {
+wi = pa_smoother_get(u-read_smoother, pa_rtclock_now());
+ri = pa_bytes_to_usec(u-read_index, u-sample_spec);
+
+*((pa_usec_t*) data) = FIXED_LATENCY_PLAYBACK_A2DP + wi - ri
0 ? FIXED_LATENCY_PLAYBACK_A2DP + wi - ri : 0;
The same complaint as with the sink: the comparison to 0 doesn't work.
Also, the constant should be FIXED_LATENCY_RECORD_A2DP, not
FIXED_LATENCY_PLAYBACK_A2DP.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 36/41] bluetooth: Handle changes to BlueZ 5 transports state
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/module-bluez5-device.c | 78
1 file changed, 78 insertions(+)
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index d9e1fc6..2eaeb77 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -97,6 +97,7 @@ struct userdata {
pa_core *core;
pa_hook_slot *device_connection_changed_slot;
+pa_hook_slot *transport_state_changed_slot;
pa_bluetooth_discovery *discovery;
pa_bluetooth_device *device;
@@ -1670,6 +1671,62 @@ static int add_card(struct userdata *u) {
}
/* Run from main thread */
+static void handle_transport_state_change(struct userdata *u, struct
pa_bluetooth_transport *t) {
+bool acquire = false;
+bool release = false;
+pa_card_profile *cp;
+pa_device_port *port;
+
+pa_assert(u);
+pa_assert(t);
+
+/* Update profile availability */
+if (!(cp = pa_hashmap_get(u-card-profiles,
pa_bluetooth_profile_to_string(t-profile
+return;
+pa_card_profile_set_available(cp,
transport_state_to_availability(t-state));
+
+/* Update port availability */
+pa_assert_se(port = pa_hashmap_get(u-card-ports, u-output_port_name));
+pa_device_port_set_available(port, get_port_availability(u,
PA_DIRECTION_OUTPUT));
+pa_assert_se(port = pa_hashmap_get(u-card-ports, u-input_port_name));
+pa_device_port_set_available(port, get_port_availability(u,
PA_DIRECTION_INPUT));
+
+/* Acquire or release transport as needed */
+acquire = (t-state == PA_BLUETOOTH_TRANSPORT_STATE_PLAYING
u-profile == t-profile);
+release = (t-state != PA_BLUETOOTH_TRANSPORT_STATE_PLAYING
u-profile == t-profile);
+
+if (acquire transport_acquire(u, true) = 0) {
+if (u-source) {
+pa_log_debug(Resuming source %s because its transport state
changed to playing, u-source-name);
+pa_source_suspend(u-source, false,
PA_SUSPEND_IDLE|PA_SUSPEND_USER);
This is now the third time I ask you to add the following comment:
/* We remove the IDLE suspend cause, because otherwise module-loopback
doesn't uncork its streams. FIXME: Messing with the IDLE suspend cause
here is wrong, the correct way to handle this would probably be to
uncork the loopback streams not only when the other end is unsuspended,
but also when the other end's suspend cause changes to IDLE only
(currently there's no notification mechanism for suspend cause changes,
though). */
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 38/41] bluetooth: Fail to load driver is discovery module is not loaded
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote: From: João Paulo Rechi Vita jprv...@openbossa.org For quite some time now the device driver module doesn't work well without the discovery module, so for the BlueZ 5 support we'll prevent the device driver module to be loaded if the discovery module is not loaded. No reaction to my previous feedback: http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/17959/focus=18071 -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 39/41] module: Create pa_module_exists()
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
This new function checks if a certain module name is available in the
system.
---
src/pulsecore/module.c | 49 +
src/pulsecore/module.h | 2 ++
2 files changed, 51 insertions(+)
diff --git a/src/pulsecore/module.c b/src/pulsecore/module.c
index 16582b3..bc53f07 100644
--- a/src/pulsecore/module.c
+++ b/src/pulsecore/module.c
@@ -28,6 +28,7 @@
#include stdlib.h
#include string.h
#include errno.h
+#include ltdl.h
#include pulse/xmalloc.h
#include pulse/proplist.h
@@ -47,6 +48,54 @@
#define PA_SYMBOL_GET_N_USED pa__get_n_used
#define PA_SYMBOL_GET_DEPRECATE pa__get_deprecated
+bool pa_module_exists(const char *name) {
+const char *paths, *state = NULL;
+char *p, *pathname;
+bool result;
+
+pa_assert(name);
+
+if (name[0] == PA_PATH_SEP_CHAR) {
+result = access(name, F_OK) == 0 ? true : false;
+pa_log_debug(Checking for existence of '%s': %s, name, result ?
success : failure);
+if (result)
+return true;
+}
+
+if (!(paths = lt_dlgetsearchpath()))
+return false;
+
+/* strip .so from the end of name, if present */
+p = rindex(name, '.');
+if (p pa_streq(p, .so))
Use PA_SOEXT instead of hardcoding .so.
+p[0] = 0;
+
+while ((p = pa_split(paths, :, state))) {
+pathname = pa_sprintf_malloc(%s PA_PATH_SEP %s.so, p, name);
Use PA_SOEXT also here...
+result = access(pathname, F_OK) == 0 ? true : false;
+pa_log_debug(Checking for existence of '%s': %s, pathname, result
? success : failure);
+pa_xfree(pathname);
+pa_xfree(p);
+if (result)
+return true;
+}
+
+state = NULL;
+if (PA_UNLIKELY(pa_run_from_build_tree())) {
+while ((p = pa_split(paths, :, state))) {
+pathname = pa_sprintf_malloc(%s PA_PATH_SEP .libs
PA_PATH_SEP %s.so, p, name);
...and here.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 39/41] module: Create pa_module_exists()
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
This new function checks if a certain module name is available in the
system.
---
src/pulsecore/module.c | 49 +
src/pulsecore/module.h | 2 ++
2 files changed, 51 insertions(+)
diff --git a/src/pulsecore/module.c b/src/pulsecore/module.c
index 16582b3..bc53f07 100644
--- a/src/pulsecore/module.c
+++ b/src/pulsecore/module.c
@@ -28,6 +28,7 @@
#include stdlib.h
#include string.h
#include errno.h
+#include ltdl.h
#include pulse/xmalloc.h
#include pulse/proplist.h
@@ -47,6 +48,54 @@
#define PA_SYMBOL_GET_N_USED pa__get_n_used
#define PA_SYMBOL_GET_DEPRECATE pa__get_deprecated
+bool pa_module_exists(const char *name) {
+const char *paths, *state = NULL;
+char *p, *pathname;
+bool result;
+
+pa_assert(name);
+
+if (name[0] == PA_PATH_SEP_CHAR) {
+result = access(name, F_OK) == 0 ? true : false;
+pa_log_debug(Checking for existence of '%s': %s, name, result ?
success : failure);
+if (result)
+return true;
+}
+
+if (!(paths = lt_dlgetsearchpath()))
+return false;
+
+/* strip .so from the end of name, if present */
+p = rindex(name, '.');
+if (p pa_streq(p, .so))
+p[0] = 0;
This modifies the original string - you shouldn't do that.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Best Case Latency
On Thu, 2013-09-19 at 23:54 +1000, Patrick Shirkey wrote: On Thu, September 19, 2013 6:52 pm, Patrick Shirkey wrote: On Thu, September 19, 2013 6:42 pm, Tanu Kaskinen wrote: On Wed, 2013-09-18 at 04:28 +1000, Patrick Shirkey wrote: Hi, Can someone shed some light on the best case and typical latency that Pulse provides for standard operations? What do you mean by standard operations? I'd estimate volume changes etc. normally (on an ALSA sink) have a few millisecond latency (best case 0.5 ms audio buffer + scheduling latency). Just a normal round trip signal flow, for example : PA (in) - audacity (in) - audacity (out) - pa (out) Also how that is affected by using jack-sink assuming jack is set to the lowest possible latency that a device can handle? When jackd asks for N frames from PulseAudio, pulseaudio will render N frames. There's no extra buffering done in the Jack sink, so volume changes etc. will have latency that is roughly the same as the normal Jack client latency. Do you have an suggestions on how to get an accurate measurement? I'm using this method: jack_delay - pa_source - audacity - pa_sink - jack_delay jack is set to 64/48000/2 Using audacity with 0 internal latency and in pass through mode I see the following results. It starts out as 10.667ms and after a few seconds it climbs up to 70.667 ms. I see similar results with ecasound instead of audacity using the following chain: jack_delay - pa_source - ecasound - pa_sink - jack_delay ecasound -f:32,2,48000 -b:64 -i alsa -o alsa In that case it starts at 60.000 ms and climbs upto 572.000 ms after a few seconds of running the test. Full console logs here: http://boosthardware.com/pa-jack-latency.txt This appears to be caused by PA adding more buffer on a regular basis. Any thoughts on why this is happening and how to stop it? PulseAudio doesn't increase the buffering on the fly (well, some small adjustments are done with ALSA, but you're using JACK, so that's irrelevant). If the source produces audio faster than the sink consumes it, then I guess the buffer might accumulate in the PulseAudio client (audacity/ecasound). I think one reason for the source running faster could be that the sink side is experiencing underruns, which causes silence to be sent to jack_delay every now and then, and this silence is not compensated by dropping a matching amount of data from the PA client. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 09/41] bluetooth: Implement org.bluez.MediaEndpoint1.SelectConfiguration()
On Sat, 2013-09-21 at 13:30 -0500, João Paulo Rechi Vita wrote:
On Sat, Sep 21, 2013 at 5:16 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
static DBusMessage *endpoint_select_configuration(DBusConnection *conn,
DBusMessage *m, void *userdata) {
+pa_bluetooth_discovery *y = userdata;
+a2dp_sbc_t *cap, config;
+uint8_t *pconf = (uint8_t *) config;
+int i, size;
DBusMessage *r;
+DBusError err;
-pa_assert_se(r = dbus_message_new_error(m,
BLUEZ_MEDIA_ENDPOINT_INTERFACE .Error.NotImplemented,
-Method not implemented));
+static const struct {
+uint32_t rate;
+uint8_t cap;
+} freq_table[] = {
+{ 16000U, SBC_SAMPLING_FREQ_16000 },
+{ 32000U, SBC_SAMPLING_FREQ_32000 },
+{ 44100U, SBC_SAMPLING_FREQ_44100 },
+{ 48000U, SBC_SAMPLING_FREQ_48000 }
+};
+
+dbus_error_init(err);
+
+if (!dbus_message_get_args(m, err, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE,
cap, size, DBUS_TYPE_INVALID)) {
+pa_log_error(Endpoint SelectConfiguration(): %s, err.message);
+dbus_error_free(err);
+goto fail;
+}
+
+if (size != sizeof(config)) {
+pa_log_error(Capabilities array has invalid size);
+goto fail;
+}
+
+pa_zero(config);
+
+/* Find the lowest freq that is at least as high as the requested
sampling rate */
+for (i = 0; (unsigned) i PA_ELEMENTSOF(freq_table); i++)
+if (freq_table[i].rate = y-core-default_sample_spec.rate
(cap-frequency freq_table[i].cap)) {
+config.frequency = freq_table[i].cap;
+break;
+}
+if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
+for (--i; i = 0; i--) {
+if (cap-frequency freq_table[i].cap) {
+config.frequency = freq_table[i].cap;
+break;
+}
+}
+
+if (i 0) {
+pa_log_error(Not suitable sample rate);
+goto fail;
+}
+}
+
+pa_assert((unsigned) i PA_ELEMENTSOF(freq_table));
+
+if (y-core-default_sample_spec.channels = 1)
+if (cap-channel_mode SBC_CHANNEL_MODE_MONO)
+config.channel_mode = SBC_CHANNEL_MODE_MONO;
You forgot to fix this code. The problem still exists that if
default_sample_spec.channels is 1 and the cap-channel_mode doesn't
contain SBC_CHANNEL_MODE_MONO, then config.channel_mode is left
uninitialized.
No, I didn't, but I may have misunderstood you. Re-reading it new
trying to give another interpretation, it seems you suggest that we
don't differentiate between (y-core-default_sample_spec.channels =
1) and (y-core-default_sample_spec.channels = 2), but I don't think
that's what you meant. Can you please explain how we should
differentiate those two cases?
There are four modes we support: mono, and three stereo modes. We should
always allow any any of those modes. Depending on
default_sample_spec.channels, we should either prefer the mono mode or
the stereo modes, but if the device doesn't support our preferred mode,
we should fall back to a mode that the device supports. In your code
that fallback is implemented when default_sample_spec.channels = 2, but
not if default_sample_spec.channels == 1.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 16/41] bluetooth: Register endpoints with BlueZ 5 adapter
On Sat, 2013-09-21 at 14:17 -0500, João Paulo Rechi Vita wrote:
On Sat, Sep 21, 2013 at 7:44 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
From: João Paulo Rechi Vita jprv...@openbossa.org
---
src/modules/bluetooth/bluez5-util.c | 82
-
1 file changed, 81 insertions(+), 1 deletion(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 4bc5967..03c73c9 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -40,9 +40,12 @@
#define BLUEZ_SERVICE org.bluez
#define BLUEZ_ADAPTER_INTERFACE BLUEZ_SERVICE .Adapter1
#define BLUEZ_DEVICE_INTERFACE BLUEZ_SERVICE .Device1
+#define BLUEZ_MEDIA_INTERFACE BLUEZ_SERVICE .Media1
#define BLUEZ_MEDIA_ENDPOINT_INTERFACE BLUEZ_SERVICE .MediaEndpoint1
#define BLUEZ_MEDIA_TRANSPORT_INTERFACE BLUEZ_SERVICE .MediaTransport1
+#define BLUEZ_ERROR_NOT_SUPPORTED org.bluez.Error.NotSupported
+
#define A2DP_SOURCE_ENDPOINT /MediaEndpoint/A2DPSource
#define A2DP_SINK_ENDPOINT /MediaEndpoint/A2DPSink
@@ -439,6 +442,81 @@ static int
parse_adapter_properties(pa_bluetooth_adapter *a, DBusMessageIter *i,
return 0;
}
+static void register_endpoint_reply(DBusPendingCall *pending, void
*userdata) {
+DBusMessage *r;
+pa_dbus_pending *p;
+pa_bluetooth_discovery *y;
+char *endpoint;
+
+pa_assert(pending);
+pa_assert_se(p = userdata);
+pa_assert_se(y = p-context_data);
+pa_assert_se(endpoint = p-call_data);
+pa_assert_se(r = dbus_pending_call_steal_reply(pending));
+
+if (dbus_message_is_error(r, DBUS_ERROR_SERVICE_UNKNOWN)) {
+pa_log_debug(Bluetooth daemon is apparently not available);
+device_remove_all(y);
Adapters should be removed too. I think there should be a function that
takes care of clearing everything, so that it's not necessary to
remember to remove both devices and adapters everywhere where we notice
that bluetoothd is non-functional.
We use device_remove_all and adapter_remove_all independently in
pa_done, and I don't like the idea of having one function that simply
wrap to functions together.
Why? The log message says: Bluetooth daemon is apparently not
available. The thing to do in that situation is to clear all state, and
if clearing all state takes several steps (as it does) and if the same
clearing needs to be done in multiple places (as it does), then a helper
function seems like the obvious choice.
However, it looks to me that checking for DBUS_ERROR_SERVICE_UNKNOWN
here is pretty pointless. We shouldn't receive that error as long as
there's an owner for the org.bluez name, and when the owner of the name
goes away, we get a notification about that and do the cleanup at that
point.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 15/41] bluetooth: Parse BlueZ 5 adapter properties
On Sat, 2013-09-21 at 15:56 -0500, João Paulo Rechi Vita wrote:
On Sat, Sep 21, 2013 at 7:51 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
+static int parse_adapter_properties(pa_bluetooth_adapter *a,
DBusMessageIter *i, bool is_property_change) {
+DBusMessageIter element_i;
+
+pa_assert(a);
+
+dbus_message_iter_recurse(i, element_i);
+
+while (dbus_message_iter_get_arg_type(element_i) ==
DBUS_TYPE_DICT_ENTRY) {
+DBusMessageIter dict_i, variant_i;
+const char *key;
+
+dbus_message_iter_recurse(element_i, dict_i);
+
+key = check_variant_property(dict_i);
+if (key == NULL) {
+pa_log_error(Received invalid property for adapter %s,
a-path);
+return -1;
+}
+
+dbus_message_iter_recurse(dict_i, variant_i);
+
+if (dbus_message_iter_get_arg_type(variant_i) ==
DBUS_TYPE_STRING pa_streq(key, Address)) {
+char *value;
+
+dbus_message_iter_get_basic(variant_i, value);
+a-address = pa_xstrdup(value);
+}
Can the Address property change? If not, it should be checked if
is_property_change is true, and if it is, then complain loudly. If it
can change, then the old address should be freed before assigning the
new address.
The address property should not change, I'm adding those checks.
There should also be check that a-address is not already set (which it
can be, if the received properties contain duplicate Address entries).
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Best Case Latency
On Sun, 2013-09-22 at 02:30 +1000, Patrick Shirkey wrote: On Sun, September 22, 2013 1:55 am, Tanu Kaskinen wrote: PulseAudio doesn't increase the buffering on the fly (well, some small adjustments are done with ALSA, but you're using JACK, so that's irrelevant). If the source produces audio faster than the sink consumes it, then I guess the buffer might accumulate in the PulseAudio client (audacity/ecasound). I think one reason for the source running faster could be that the sink side is experiencing underruns, which causes silence to be sent to jack_delay every now and then, and this silence is not compensated by dropping a matching amount of data from the PA client. That seems like a reasonable explanation. The device I am using is an onboard hda_intel and I am seeing a lot of xruns in the jack console messages. From the perspective of an ignorant user this appears to be a sort of bug in that the audio stream is being delivered to the speakers but not to jack_delay. Although ears can be deceiving I don't hear any obvious dropouts. Can you think of a way to mitigate this issue when using both jack and pa at the same time? I am seeing this issue with both 64 frames/period and 1024 . I usually run with 1024 on JACK and that stable for me albeit I don't ever try to record to jack apps with the onboard mic. However, I use skype via PA regularly and it usually provides semi decent quality for phone conversations (without jack running). I tried using the skype call testing service with jack+pa. I can hear my voice and there are no obvious artefacts. The end goal here is to find a way to accurately measure the round trip latency when both JACK and PA are in use and figure out a robust method to achieve the lowest latency possible while PA is connected to JACK . Ideally I would like to measure the latency between each node in the graph. Does the PA API allow for obtaining latency measurements between nodes in the graph? Could I spin up a tool to gather that data for testing/QA purposes? By combining that info with jack_delay and a couple of other system metrics I might be able to gain some useful insight into tweaking the system to provide better performance other than installing a realtime kernel and setting the priorities with rtirq. That's assuming that there is anything that can be done to either PA or JACK to make it perform better. If you want to get the minimum latency of this path: jack_delay - pulseaudio - application - pulseaudio - jack_delay then you should use an application that reports underruns and lets you control the latency that the application requests from pulseaudio. Then you can tune the application latency to find the point where you start to get underruns. You should also know the maximum internal buffer size of the application. You probably need to write that application yourself. pa_stream_get_latency() gives you an estimate of the latency between the hardware and your application. pa_stream_get_timing_info() gives some more details about how that latency is split between different parts in pulseaudio. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/4] alsa: Add extcon (Android switch) jack detection
On Fri, 2013-09-20 at 09:41 -0500, João Paulo Rechi Vita wrote: On Thu, Sep 19, 2013 at 4:53 PM, David Henningsson david.hennings...@canonical.com wrote: On 09/19/2013 01:17 PM, Damir Jelić wrote: Sorry if I'm a little bit annoying here but I'd like to keep our from now on with as little style inconsistency as possible. I haven't done any proper review/testing on this, just a quick coding style check. Sorry if I'm even more annoying here ;-) but perhaps you haven't heard my personal view on this matter. I totally disagree. I would instead like to keep our coding style rules to a bare minimum to keep the code decently readable. Anything else is just an additional road block towards what's important: spending as much time as possible fixing bugs and implementing new features, rather than spending time complying to coding style rules. So; for the opening bracket on newline - I think our current coding style is wrong and should be changed for functions, and I always forget that we have this odd style here. You're okay to comment on that, because it is in our coding style rules, but I would appreciate more, as you say, any proper review/testing, rather than a simple coding style check. You focus on what looks good on the surface, but what really matters is real code quality - i e, whether the code is going to solve our users' problems without causing annoying bugs, or not. If you're really lazy you can use my sed script to fix this issues (well the opening bracket on newline issues at least).[2] For the stuff that's *not* in the coding style wiki page, I'm even lazier. If you prefer a different style than I happen to write, feel free to run your scripts every six months or so and submit a patch for it, and I won't complain if somebody else reviews and commit it. (Well, unless it severely decreases readability somehow.) The problem with commits that only fix coding style is that it screws up history, making things like 'git blame' much harder, so I think we should avoid that as much as possible. The only way to avoid it is trying to have clear coding style rules and trying to adhere to them as much as possible. OTOH, I agree with David that there are some more important stuff for time to be spent on than spending time dealing with coding style. The only solution I know that would help is to have some coding style checker that would help us on finding problems with the coding style. This tool should be available in the repository, so anyone sending patches can check style before submitting patches, but this should also be checked by the maintainer before committing the code upstream. If any coding style problem is found at this later point, it's ok IMO that the maintainer fixes the coding style problems by himself (amending to the original commit) so we don't add an additional peer-review roundtrip. What do you guys think? A style checker script would be great. On another topic: I want a pony ;) It seems that at least a mass conversion the current code base to the new style is getting quite a lot of opposition, so that's probably not going to happen. Then there's the question that should we keep complaining about style violations with the curly braces. Should contributors be allowed to put the curly braces where they want, and inconsistencies would be fixed later if the lines in question are modified as part of some other patch? The curly brace issue is such that inconsistency wouldn't bother me much. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Best Case Latency
On Sun, 2013-09-22 at 20:03 +1000, Patrick Shirkey wrote: On Sun, September 22, 2013 4:37 pm, Tanu Kaskinen wrote: If you want to get the minimum latency of this path: jack_delay - pulseaudio - application - pulseaudio - jack_delay then you should use an application that reports underruns and lets you control the latency that the application requests from pulseaudio. Then you can tune the application latency to find the point where you start to get underruns. You should also know the maximum internal buffer size of the application. You probably need to write that application yourself. I am using ecasound which reports underruns and allows me to set the internal buffer. I have it set to 64 frames/period for this test. It does report occasional underruns but over a 6 hour period I had about 20. Based on that info it leads me to PA sink as the bottle neck. Do you have any suggestions about how I can rule that out? The audio is accumulating in some buffer, and the JACK sink in PulseAudio doesn't have any buffer, so it's not the sink. It could be some other buffer in PulseAudio, or it could be a buffer in ecasound. Saying that ecasound is configured with 64 frames/period isn't terribly informative, because I don't know how ecasound handles the transfer from the input to the output. Does it push the data immediately to the output when it gets something from the input, or are both directions callback based? If the former, then the data can accumulate in the stream buffer in PA, and if the latter, then there has to be an intermediate buffer between the input and the output in ecasound, and the data can accumulate there. pa_stream_get_latency() gives you an estimate of the latency between the hardware and your application. pa_stream_get_timing_info() gives some more details about how that latency is split between different parts in pulseaudio. Can you or anyone else recommend an example app that I can use as a reference for this code? No. And anyway, if you want to really measure the latency (detect when a signal pulse arrives at a measurement point), these functions are useless (except for checking that is PulseAudio reporting the latency correctly or not, which would actually be quite interesting information). I am thinking of building an app that will let me gather latency data from both PA and JACK. I would like it to measure the timing of a signal pulse as it is transferred between each node in the graph. It would communicate with both JACK and PA at the same time. Do you see any insurmountable blockers to achieving that goal from a technical perspective in regards to PA? So you would generate a pulse in a jack client, and detect it in a PA client, and then detect it again in a jack client? No, I don't see any obstacles to doing that. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] alsa: Turn one assertion into two
On Mon, 2013-09-23 at 09:55 +0200, David Henningsson wrote:
According to coding style, one should have one assertion per line
and not combine assertions.
Signed-off-by: David Henningsson david.hennings...@canonical.com
---
src/modules/alsa/module-alsa-card.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/modules/alsa/module-alsa-card.c
b/src/modules/alsa/module-alsa-card.c
index 96e88e5..be982ed 100644
--- a/src/modules/alsa/module-alsa-card.c
+++ b/src/modules/alsa/module-alsa-card.c
@@ -378,8 +378,8 @@ static int report_jack_state(snd_hctl_elem_t *elem,
unsigned int mask) {
pa_assert(port);
}
else {
-pa_assert(jack-path jack-path-port);
-port = jack-path-port;
+pa_assert(jack-path);
+pa_assert_se(port = jack-path-port);
}
report_port_state(port, u);
}
Ack.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Suggested coding style change for functions
On Mon, 2013-09-23 at 10:21 +0200, David Henningsson wrote:
On 09/20/2013 11:23 AM, Peter Meerwald wrote:
I think both coding styles are sane and I suggest to NOT convert to a new
style
the 'other projects' is Gnome?
Gnome, Linux Kernel, FluidSynth (as mentioned previously).
there is not the ONE style and it really depends on your personal
situation which styles you work more often with -- what's good for one
contributer might not be for another
Out of curiousity, do you know any bigger open source projects that do
the no newline style? I don't think I've ever seen one except for
PulseAudio.
Quoting the CODING_STYLE file of systemd:
- Try to use this:
void foo() {
}
instead of this:
void foo()
{
}
But it's OK if you don't.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v4 09/41] bluetooth: Implement org.bluez.MediaEndpoint1.SelectConfiguration()
On Mon, 2013-09-23 at 08:39 -0500, João Paulo Rechi Vita wrote:
On Sun, Sep 22, 2013 at 12:19 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Sat, 2013-09-21 at 13:30 -0500, João Paulo Rechi Vita wrote:
On Sat, Sep 21, 2013 at 5:16 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
static DBusMessage *endpoint_select_configuration(DBusConnection
*conn, DBusMessage *m, void *userdata) {
+pa_bluetooth_discovery *y = userdata;
+a2dp_sbc_t *cap, config;
+uint8_t *pconf = (uint8_t *) config;
+int i, size;
DBusMessage *r;
+DBusError err;
-pa_assert_se(r = dbus_message_new_error(m,
BLUEZ_MEDIA_ENDPOINT_INTERFACE .Error.NotImplemented,
-Method not implemented));
+static const struct {
+uint32_t rate;
+uint8_t cap;
+} freq_table[] = {
+{ 16000U, SBC_SAMPLING_FREQ_16000 },
+{ 32000U, SBC_SAMPLING_FREQ_32000 },
+{ 44100U, SBC_SAMPLING_FREQ_44100 },
+{ 48000U, SBC_SAMPLING_FREQ_48000 }
+};
+
+dbus_error_init(err);
+
+if (!dbus_message_get_args(m, err, DBUS_TYPE_ARRAY,
DBUS_TYPE_BYTE, cap, size, DBUS_TYPE_INVALID)) {
+pa_log_error(Endpoint SelectConfiguration(): %s,
err.message);
+dbus_error_free(err);
+goto fail;
+}
+
+if (size != sizeof(config)) {
+pa_log_error(Capabilities array has invalid size);
+goto fail;
+}
+
+pa_zero(config);
+
+/* Find the lowest freq that is at least as high as the requested
sampling rate */
+for (i = 0; (unsigned) i PA_ELEMENTSOF(freq_table); i++)
+if (freq_table[i].rate = y-core-default_sample_spec.rate
(cap-frequency freq_table[i].cap)) {
+config.frequency = freq_table[i].cap;
+break;
+}
+if ((unsigned) i == PA_ELEMENTSOF(freq_table)) {
+for (--i; i = 0; i--) {
+if (cap-frequency freq_table[i].cap) {
+config.frequency = freq_table[i].cap;
+break;
+}
+}
+
+if (i 0) {
+pa_log_error(Not suitable sample rate);
+goto fail;
+}
+}
+
+pa_assert((unsigned) i PA_ELEMENTSOF(freq_table));
+
+if (y-core-default_sample_spec.channels = 1)
+if (cap-channel_mode SBC_CHANNEL_MODE_MONO)
+config.channel_mode = SBC_CHANNEL_MODE_MONO;
You forgot to fix this code. The problem still exists that if
default_sample_spec.channels is 1 and the cap-channel_mode doesn't
contain SBC_CHANNEL_MODE_MONO, then config.channel_mode is left
uninitialized.
No, I didn't, but I may have misunderstood you. Re-reading it new
trying to give another interpretation, it seems you suggest that we
don't differentiate between (y-core-default_sample_spec.channels =
1) and (y-core-default_sample_spec.channels = 2), but I don't think
that's what you meant. Can you please explain how we should
differentiate those two cases?
There are four modes we support: mono, and three stereo modes. We should
always allow any any of those modes. Depending on
default_sample_spec.channels, we should either prefer the mono mode or
the stereo modes, but if the device doesn't support our preferred mode,
we should fall back to a mode that the device supports. In your code
that fallback is implemented when default_sample_spec.channels = 2, but
not if default_sample_spec.channels == 1.
Ok, and what preference order we should use between the stereo modes
for default_sample_spec.channels == 1? (I'm assuming mono is the 1st
preference in this case).
Yes, mono is the first preferred mode. I don't have any idea about the
ordering of the different stereo modes, except that the ordering should
be the same for both default_sample_spec.channels == 1 and
default_sample_spec.channels == 2, unless you know some reason to use
different ordering.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Best Case Latency
On Wed, 2013-09-25 at 23:35 +1000, Patrick Shirkey wrote: On Tue, September 24, 2013 10:33 pm, Patrick Shirkey wrote: On Mon, September 23, 2013 6:11 pm, Tanu Kaskinen wrote: The audio is accumulating in some buffer, and the JACK sink in PulseAudio doesn't have any buffer, so it's not the sink. It could be some other buffer in PulseAudio, or it could be a buffer in ecasound. Saying that ecasound is configured with 64 frames/period isn't terribly informative, because I don't know how ecasound handles the transfer from the input to the output. Does it push the data immediately to the output when it gets something from the input, or are both directions callback based? If the former, then the data can accumulate in the stream buffer in PA, and if the latter, then there has to be an intermediate buffer between the input and the output in ecasound, and the data can accumulate there. I've asked for some more details on the ecasound list. If the PA stream buffer is kicking in, Is there any way I can verify if it is happening? is there anything I can do to minimise that affect? I had verification from the ecasound developer Kai Vehmanen that ecasound does not have an internal buffer that could be affecting the latency in this way. It just buffers 64 frames and if it can't keep up it will drop data and report an underun. In my tests there were just a few underruns over several hours so it appears safe to rule out ecasound. Does anyone have any suggestions for testing the PA Stream Buffer to check if it is a bottleneck in my test environment? Sorry, forgot to reply earlier. Yes, you can check the stream buffer with pactl list sink-inputs. The Buffer Latency field shows the amount of audio in the stream buffer (not yet written to the sink). The Sink Latency field shows the latency of the sink. The sum of these two values is the full server-side latency for the stream. In addition to the server-side latency, there's the transport latency between the client and the server (very low for local streams). -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Best Case Latency
On Fri, 2013-09-27 at 19:26 +1000, Patrick Shirkey wrote: On Thu, September 26, 2013 2:27 am, Tanu Kaskinen wrote: Sorry, forgot to reply earlier. Yes, you can check the stream buffer with pactl list sink-inputs. The Buffer Latency field shows the amount of audio in the stream buffer (not yet written to the sink). The Sink Latency field shows the latency of the sink. The sum of these two values is the full server-side latency for the stream. In addition to the server-side latency, there's the transport latency between the client and the server (very low for local streams). Thanks for that info. Polling the output I see the results fluctuate over time in much the same way that the results from jack_iodelay fluctuate. I haven't tried to correlate them yet but here's a snapshot to give you a better idea of what is happening on this machine. The latency stays around 10 ms. You complained earlier that the latency would grow to several hundreds of milliseconds, but this shows no such behaviour, so the problem is somewhere else than in the playback stream inside pulseaudio. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v5 39/39] bluetooth: Revive module-bluetooth-discover
On Tue, 2013-09-24 at 16:16 -0700, Fox, Kevin M wrote:
Is there a reason that the struct is:
+struct userdata {
+pa_module *bluez5_module;
+pa_module *bluez4_module;
+};
and not:
+struct userdata {
+pa_module *bluez_module;
+};
The code might be cleaner looking with the latter struct.
Yes, there is a reason for the code being how it is. We want
module-bluetooth-discover to load two modules, if support for both
bluez4 and bluez5 is compiled in, but in your suggestion only one module
can be loaded.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Best Case Latency
On Fri, 2013-09-27 at 23:31 +1000, Patrick Shirkey wrote: On Fri, September 27, 2013 10:52 pm, Tanu Kaskinen wrote: On Fri, 2013-09-27 at 19:26 +1000, Patrick Shirkey wrote: On Thu, September 26, 2013 2:27 am, Tanu Kaskinen wrote: Sorry, forgot to reply earlier. Yes, you can check the stream buffer with pactl list sink-inputs. The Buffer Latency field shows the amount of audio in the stream buffer (not yet written to the sink). The Sink Latency field shows the latency of the sink. The sum of these two values is the full server-side latency for the stream. In addition to the server-side latency, there's the transport latency between the client and the server (very low for local streams). Thanks for that info. Polling the output I see the results fluctuate over time in much the same way that the results from jack_iodelay fluctuate. I haven't tried to correlate them yet but here's a snapshot to give you a better idea of what is happening on this machine. The latency stays around 10 ms. You complained earlier that the latency would grow to several hundreds of milliseconds, but this shows no such behaviour, so the problem is somewhere else than in the playback stream inside pulseaudio. It may seem like I am complaining but I am not trying to come across that way. I guess that was a bad choice of words from my part. You were reporting undesirable behaviour, which I referred to as complaining. I used it as a neutral word, not as in whining or anything like that. I am trying to get hard data on the real world latency performance using the combination of JACK + PA to allow a bunch of people including some of your colleagues at Intel to attempt to make a professional business decision about the best way to integrate them (or not) for mobile platforms. IMO it is best to run them with the method that I am testing and fix any issues that might not have been ironed out yet but others are suggesting they should be used interchangeably. That opens up a can of works that may be a lot more effort to implement both in terms of resources and politically for various reasons so I am reluctant to follow that path unless all options with combining JACK + PA have been categorically shown to be infeasible or impractical (or both). Sorry, I couldn't really follow what you're saying, but I won't go into detail, since I believe this is not relevant for solving the latency mystery. At the moment I'm trying to pin down the cause of the consistent and reproducible but fluctuating latency results that I see with this machine. I am doing this in order to get the hard data I need to present the JACK + PA solution as the better method. One of the unclear things is what other method you are comparing the JACK + PA solution. So far I cannot point the finger at any specific process or bottleneck. Given that I am apparently the only person in the world who has the time and/or motivation to run these tests we are working with the small dataset that I can provide. All your feedback is crucial and very useful as I do not have the complete overview of PA codebase like yourself or others on this list. To recap, I have tested with two jack graphs. 1: jack_delay - pa - ecasound - pa - jack_delay The latency starts low and then consistently climbs (see below). 2: jack_delay - pa - ecasound - pa - system_out - system_in - jack_delay The latency jumps around from anything between 4ms to 1300ms. It might also go lower but I didn't see that yet. - Here's an interesting snapshot for consideration captured just now while running graph 1. The latency captured while polling with pactl list sink-inputs is the same as the previous snapshot (approx 10ms). ex. Buffer Latency: 8000 usec Sink Latency: 3166 usec However, jack_delay reports the following data. Where you see ?? that indicates a dropout in the audio stream so jack_delay was not able to make a real measurement so guessed. What do you mean by dropout? An xrun reported by jackd? Note that there are several different kinds of underruns and overruns that can occur. One kind is the case where jackd is late to read/write to the sound card (alsa). Another kind is when pulseaudio is late to read/write to jackd (I'm not sure if this will always translate also to alsa level under/overruns too). Third kind is when ecasound is late to read/write to pulseaudio (this is what I'll refer to as stream under/overruns, and these are invisible to the jack domain). Do you get logs about each of these? I don't remember if the alsa compatibility layer forwards the stream under/overrun notifications to the alsa application - IIRC there were problems when those were reported and there were different problems when those were not reported, and I don't remember what the current code does. I don't know if distinguishing between the different kinds of under/overruns is really important at this point
Re: [pulseaudio-discuss] [PATCH v4 15/41] bluetooth: Parse BlueZ 5 adapter properties
On Thu, 2013-09-26 at 14:11 -0300, João Paulo Rechi Vita wrote:
On Wed, Sep 25, 2013 at 7:33 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Tue, 2013-09-24 at 19:19 -0300, João Paulo Rechi Vita wrote:
On Sun, Sep 22, 2013 at 2:38 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Sat, 2013-09-21 at 16:12 -0500, João Paulo Rechi Vita wrote:
On Sat, Sep 21, 2013 at 6:12 AM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Wed, 2013-09-18 at 16:17 -0500, jprv...@gmail.com wrote:
static void parse_interfaces_and_properties(pa_bluetooth_discovery
*y, DBusMessageIter *dict_i) {
DBusMessageIter element_i;
const char *path;
@@ -415,7 +474,8 @@ static void
parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
pa_log_debug(Adapter %s found, path);
-/* TODO: parse adapter properties and register
endpoints */
+parse_adapter_properties(a, iface_i, false);
If parsing fails, or if the Address property is missing, the adapter
should be marked as invalid.
We don't need to add a adapter_info_valid field, just not registering
as an endpoint with that adapter should be enough.
I don't think that's enough. Devices can point to adapters, so devices
that point to invalid adapters should be marked as invalid too. I think
an info_valid field also for adapters is the way to go.
There is no problem if devices point to invalid adapters, since we
have not registered as endpoints we'll never receive transports for
these devices, so nothing will happen.
pa_bluetooth_discovery_get_device_by_address() will crash if a device
points to an adapter with NULL address.
Actually, marking a device invalid if the adapter is invalid isn't
sufficient to fix pa_bluetooth_discovery_get_device_by_address(). The
function should check d-device_info_valid before trying to access
d-adapter.
Ok, that's a problem in pa_bluetooth_discovery_get_device_by_address()
then. Fixing it leaves no other problems related to the adapter being
invalid, right?
Seems so, with the current code. However, logically it doesn't make
sense to treat devices as valid if their adapter is invalid. Future code
can easily have bugs due to this, if adapters ever become more important
(right now the adapter usage is very limited: we track the adapters only
to be able to look up devices based on the local/remote address pair).
I think it would be good to have the adapter_info_valid field, and to
treat devices with an invalid address as invalid themselves, but if you
don't want to do that, feel free to leave the code as it is.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v5 09/39] bluetooth: Create a function to remove only one adapter object
On Tue, 2013-09-24 at 19:45 -0300, jprv...@gmail.com wrote:
@@ -371,11 +398,8 @@ static void adapter_remove_all(pa_bluetooth_discovery
*y) {
/* When this function is called all devices have already been freed */
-while ((a = pa_hashmap_steal_first(y-adapters))) {
-pa_xfree(a-path);
-pa_xfree(a-address);
-pa_xfree(a);
-}
+while ((a = pa_hashmap_steal_first(y-adapters)))
+adapter_free(a);
pa_hashmap_remove_all() can be used here (the hashmap initialization
needs to be changed so that adapter_free is passed to
pa_hashmap_new_full()). Actually, the whole adapter_remove_all()
function is now redundant, because it's effectively just an alias for
pa_hashmap_remove_all().
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v5 12/39] bluetooth: Parse BlueZ 5 adapter properties
On Tue, 2013-09-24 at 19:45 -0300, jprv...@gmail.com wrote:
+static void parse_adapter_properties(pa_bluetooth_adapter *a,
DBusMessageIter *i, bool is_property_change) {
+DBusMessageIter element_i;
+
+pa_assert(a);
+
+dbus_message_iter_recurse(i, element_i);
+
+while (dbus_message_iter_get_arg_type(element_i) ==
DBUS_TYPE_DICT_ENTRY) {
+DBusMessageIter dict_i, variant_i;
+const char *key;
+
+dbus_message_iter_recurse(element_i, dict_i);
+
+key = check_variant_property(dict_i);
+if (key == NULL) {
+pa_log_error(Received invalid property for adapter %s,
a-path);
+return;
+}
+
+dbus_message_iter_recurse(dict_i, variant_i);
+
+if (dbus_message_iter_get_arg_type(variant_i) == DBUS_TYPE_STRING
pa_streq(key, Address)) {
+char *value;
The type should be const char *.
+
+if (is_property_change) {
+pa_log_warn(Adapter property 'Address' expected to be
constant but changed for %s, ignoring, a-path);
+return;
+}
+
+if (a-address) {
+pa_log_warn(Adapter %s received a duplicate 'Address'
property, ignoring, a-path);
+return;
+}
+
+dbus_message_iter_get_basic(variant_i, value);
+a-address = pa_xstrdup(value);
+}
+
+dbus_message_iter_next(element_i);
+}
+}
+
static void parse_interfaces_and_properties(pa_bluetooth_discovery *y,
DBusMessageIter *dict_i) {
DBusMessageIter element_i;
const char *path;
@@ -439,7 +506,11 @@ static void
parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
pa_log_debug(Adapter %s found, path);
-/* TODO: parse adapter properties and register endpoints */
+parse_adapter_properties(a, iface_i, false);
+if (!a-address)
+return;
(Note to self: there is an ongoing discussion about whether there should
be an adapter_info_valid field. Depending on the conclusion of the
discussion, this code may change in v6.)
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v5 16/39] bluetooth: Protect from a misbehaving bluetoothd
On Tue, 2013-09-24 at 19:45 -0300, jprv...@gmail.com wrote:
@@ -745,6 +748,15 @@ static void
parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
dbus_message_iter_next(element_i);
}
+PA_HASHMAP_FOREACH(d, y-devices, state)
+if (!d-adapter d-adapter_path) {
+d-adapter = pa_hashmap_get(d-discovery-adapters,
d-adapter_path);
+if (!d-adapter) {
+pa_log_error(Device %s is child of nonexistent adapter %s,
d-path, d-adapter_path);
+d-device_info_valid = -1;
+}
+}
As discussed, the DEVICE_CONNECTION_CHANGED hook should be fired after
parsing the device properties (I'm mentioning this just to make sure
that I remember to check this in v6).
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v5 36/39] bluetooth: Fail to load driver if discovery module is not loaded
On Tue, 2013-09-24 at 19:45 -0300, jprv...@gmail.com wrote:
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index 36d7174..013cce5 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -39,6 +39,7 @@
#include pulsecore/modargs.h
#include pulsecore/poll.h
#include pulsecore/rtpoll.h
+#include pulsecore/shared.h
#include pulsecore/socket-util.h
#include pulsecore/thread.h
#include pulsecore/thread-mq.h
@@ -1792,8 +1793,12 @@ int pa__init(pa_module* m) {
goto fail;
}
-if (!(u-discovery = pa_bluetooth_discovery_get(m-core)))
+if ((u-discovery = pa_shared_get(u-core, bluetooth-discovery)))
+pa_bluetooth_discovery_ref(u-discovery);
+else {
+pa_log_error(module-bluez5-discover doesn't seem to be loaded,
refusing to load module-bluez5-device);
goto fail;
+}
(Note to self: there is ongoing discussion about this:
http://thread.gmane.org/gmane.comp.audio.pulseaudio.general/17959/focus=18491)
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH 0/5] Bluetooth polishing
Here are some small fixes to issues that I found while reviewing the big Bluetooth patch set from João. Tanu Kaskinen (5): bluetooth: Remove adapter_remove_all() bluetooth: Fix variable constness bluetooth: Fix notifying about new devices bluetooth: Remove device_remove_all() bluetooth: Set device_info_valid to -1 when the device's adapter disappears src/modules/bluetooth/bluez5-util.c | 79 +++-- 1 file changed, 41 insertions(+), 38 deletions(-) -- 1.8.1.2 ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH 2/5] bluetooth: Fix variable constness
The string points to memory inside a DBusMessage, so we don't own the string.
---
src/modules/bluetooth/bluez5-util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 2be15d8..eaff4b1 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -651,7 +651,7 @@ static void parse_adapter_properties(pa_bluetooth_adapter
*a, DBusMessageIter *i
dbus_message_iter_recurse(dict_i, variant_i);
if (dbus_message_iter_get_arg_type(variant_i) == DBUS_TYPE_STRING
pa_streq(key, Address)) {
-char *value;
+const char *value;
if (is_property_change) {
pa_log_warn(Adapter property 'Address' expected to be
constant but changed for %s, ignoring, a-path);
--
1.8.1.2
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH 4/5] bluetooth: Remove device_remove_all()
The function did two things: set device_info_valid to -1 and called
device_free() for each device in the hashmap. Setting
device_info_valid to -1 was unnecessary. The main purpose of that was
to fire DEVICE_CONNECTION_CHANGED as a side effect, but that hook is
fired anyway in device_free(), as a side effect of removing all
transports. Calling device_free() can be delegated to pa_hashmap, when
freeing or emptying it.
---
src/modules/bluetooth/bluez5-util.c | 20
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index c7e934e..591ea50 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -454,17 +454,6 @@ static void set_device_info_valid(pa_bluetooth_device
*device, int valid) {
pa_hook_fire(device-discovery-hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED],
device);
}
-static void device_remove_all(pa_bluetooth_discovery *y) {
-pa_bluetooth_device *d;
-
-pa_assert(y);
-
-while ((d = pa_hashmap_steal_first(y-devices))) {
-set_device_info_valid(d, -1);
-device_free(d);
- }
-}
-
static pa_bluetooth_adapter* adapter_create(pa_bluetooth_discovery *y, const
char *path) {
pa_bluetooth_adapter *a;
@@ -928,7 +917,7 @@ static DBusHandlerResult filter_cb(DBusConnection *bus,
DBusMessage *m, void *us
if (pa_streq(name, BLUEZ_SERVICE)) {
if (old_owner *old_owner) {
pa_log_debug(Bluetooth daemon disappeared);
-device_remove_all(y);
+pa_hashmap_remove_all(y-devices);
pa_hashmap_remove_all(y-adapters);
y-objects_listed = false;
}
@@ -1533,7 +1522,8 @@ pa_bluetooth_discovery*
pa_bluetooth_discovery_get(pa_core *c) {
y-core = c;
y-adapters = pa_hashmap_new_full(pa_idxset_string_hash_func,
pa_idxset_string_compare_func, NULL,
(pa_free_cb_t) adapter_free);
-y-devices = pa_hashmap_new(pa_idxset_string_hash_func,
pa_idxset_string_compare_func);
+y-devices = pa_hashmap_new_full(pa_idxset_string_hash_func,
pa_idxset_string_compare_func, NULL,
+ (pa_free_cb_t) device_free);
y-transports = pa_hashmap_new(pa_idxset_string_hash_func,
pa_idxset_string_compare_func);
PA_LLIST_HEAD_INIT(pa_dbus_pending, y-pending);
@@ -1608,10 +1598,8 @@ void pa_bluetooth_discovery_unref(pa_bluetooth_discovery
*y) {
pa_dbus_free_pending_list(y-pending);
-if (y-devices) {
-device_remove_all(y);
+if (y-devices)
pa_hashmap_free(y-devices);
-}
if (y-adapters)
pa_hashmap_free(y-adapters);
--
1.8.1.2
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH 3/5] bluetooth: Fix notifying about new devices
Normally devices are created and their properties are parsed before
creating any transports for the device, and the
DEVICE_CONNECTION_CHANGED hook is fired when the first transport is
created. It's possible, however, that a transport is created before
the device that it belongs to, and in this case
DEVICE_CONNECTION_CHANGED should be fired when the properties have
been successfully parsed for the device. The old code didn't fire the
hook at this situation, and this patch fixes that.
---
src/modules/bluetooth/bluez5-util.c | 43 +
1 file changed, 34 insertions(+), 9 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index eaff4b1..c7e934e 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -438,14 +438,29 @@ static void device_remove(pa_bluetooth_discovery *y,
const char *path) {
}
}
+static void set_device_info_valid(pa_bluetooth_device *device, int valid) {
+bool old_any_connected;
+
+pa_assert(device);
+pa_assert(valid == -1 || valid == 0 || valid == 1);
+
+if (valid == device-device_info_valid)
+return;
+
+old_any_connected = pa_bluetooth_device_any_transport_connected(device);
+device-device_info_valid = valid;
+
+if (pa_bluetooth_device_any_transport_connected(device) !=
old_any_connected)
+
pa_hook_fire(device-discovery-hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED],
device);
+}
+
static void device_remove_all(pa_bluetooth_discovery *y) {
pa_bluetooth_device *d;
pa_assert(y);
while ((d = pa_hashmap_steal_first(y-devices))) {
-d-device_info_valid = -1;
-pa_hook_fire(y-hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED],
d);
+set_device_info_valid(d, -1);
device_free(d);
}
}
@@ -607,7 +622,6 @@ static void parse_device_property(pa_bluetooth_device *d,
DBusMessageIter *i, bo
static int parse_device_properties(pa_bluetooth_device *d, DBusMessageIter *i,
bool is_property_change) {
DBusMessageIter element_i;
-int ret = 0;
dbus_message_iter_recurse(i, element_i);
@@ -621,12 +635,17 @@ static int parse_device_properties(pa_bluetooth_device
*d, DBusMessageIter *i, b
if (!d-address || !d-adapter_path || !d-alias) {
pa_log_error(Non-optional information missing for device %s,
d-path);
-d-device_info_valid = -1;
+set_device_info_valid(d, -1);
return -1;
}
-d-device_info_valid = 1;
-return ret;
+if (!is_property_change d-adapter)
+set_device_info_valid(d, 1);
+
+/* If d-adapter is NULL, device_info_valid will be left as 0, and updated
+ * after all interfaces have been parsed. */
+
+return 0;
}
static void parse_adapter_properties(pa_bluetooth_adapter *a, DBusMessageIter
*i, bool is_property_change) {
@@ -804,14 +823,20 @@ static void
parse_interfaces_and_properties(pa_bluetooth_discovery *y, DBusMessa
dbus_message_iter_next(element_i);
}
-PA_HASHMAP_FOREACH(d, y-devices, state)
+PA_HASHMAP_FOREACH(d, y-devices, state) {
+if (d-device_info_valid != 0)
+continue;
+
if (!d-adapter d-adapter_path) {
d-adapter = pa_hashmap_get(d-discovery-adapters,
d-adapter_path);
+
if (!d-adapter) {
pa_log_error(Device %s is child of nonexistent adapter %s,
d-path, d-adapter_path);
-d-device_info_valid = -1;
-}
+set_device_info_valid(d, -1);
+} else
+set_device_info_valid(d, 1);
}
+}
return;
}
--
1.8.1.2
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH 5/5] bluetooth: Set device_info_valid to -1 when the device's adapter disappears
When parsing device properties, missing adapter will result in
device_info_valid being set to -1. It is then logical that if the
adapter goes missing at a later point, device_info_valid gets set to
-1.
---
src/modules/bluetooth/bluez5-util.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 591ea50..3408b2c 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -477,8 +477,10 @@ static void adapter_free(pa_bluetooth_adapter *a) {
pa_assert(a-discovery);
PA_HASHMAP_FOREACH(d, a-discovery-devices, state)
-if (d-adapter == a)
+if (d-adapter == a) {
+set_device_info_valid(d, -1);
d-adapter = NULL;
+}
pa_xfree(a-path);
pa_xfree(a-address);
--
1.8.1.2
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH 1/5] bluetooth: Remove adapter_remove_all()
The function was redundant, because all it did was call adapter_free()
for each adapter in the hashmap, and that can be delegated to
pa_hashmap when freeing or emptying it.
---
src/modules/bluetooth/bluez5-util.c | 20
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index 225e5c8..2be15d8 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -492,17 +492,6 @@ static void adapter_remove(pa_bluetooth_discovery *y,
const char *path) {
}
}
-static void adapter_remove_all(pa_bluetooth_discovery *y) {
-pa_bluetooth_adapter *a;
-
-pa_assert(y);
-
-/* When this function is called all devices have already been freed */
-
-while ((a = pa_hashmap_steal_first(y-adapters)))
-adapter_free(a);
-}
-
static void parse_device_property(pa_bluetooth_device *d, DBusMessageIter *i,
bool is_property_change) {
const char *key;
DBusMessageIter variant_i;
@@ -915,7 +904,7 @@ static DBusHandlerResult filter_cb(DBusConnection *bus,
DBusMessage *m, void *us
if (old_owner *old_owner) {
pa_log_debug(Bluetooth daemon disappeared);
device_remove_all(y);
-adapter_remove_all(y);
+pa_hashmap_remove_all(y-adapters);
y-objects_listed = false;
}
@@ -1517,7 +1506,8 @@ pa_bluetooth_discovery*
pa_bluetooth_discovery_get(pa_core *c) {
y = pa_xnew0(pa_bluetooth_discovery, 1);
PA_REFCNT_INIT(y);
y-core = c;
-y-adapters = pa_hashmap_new(pa_idxset_string_hash_func,
pa_idxset_string_compare_func);
+y-adapters = pa_hashmap_new_full(pa_idxset_string_hash_func,
pa_idxset_string_compare_func, NULL,
+ (pa_free_cb_t) adapter_free);
y-devices = pa_hashmap_new(pa_idxset_string_hash_func,
pa_idxset_string_compare_func);
y-transports = pa_hashmap_new(pa_idxset_string_hash_func,
pa_idxset_string_compare_func);
PA_LLIST_HEAD_INIT(pa_dbus_pending, y-pending);
@@ -1598,10 +1588,8 @@ void pa_bluetooth_discovery_unref(pa_bluetooth_discovery
*y) {
pa_hashmap_free(y-devices);
}
-if (y-adapters) {
-adapter_remove_all(y);
+if (y-adapters)
pa_hashmap_free(y-adapters);
-}
if (y-transports) {
pa_assert(pa_hashmap_isempty(y-transports));
--
1.8.1.2
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/3] hashmap: Add a key+value iterator
On Fri, 2013-09-20 at 17:27 +0530, Arun Raghavan wrote: --- src/pulsecore/hashmap.h | 4 1 file changed, 4 insertions(+) diff --git a/src/pulsecore/hashmap.h b/src/pulsecore/hashmap.h index ae030ed..e42732a 100644 --- a/src/pulsecore/hashmap.h +++ b/src/pulsecore/hashmap.h @@ -85,6 +85,10 @@ void* pa_hashmap_last(pa_hashmap *h); #define PA_HASHMAP_FOREACH(e, h, state) \ for ((state) = NULL, (e) = pa_hashmap_iterate((h), (state), NULL); (e); (e) = pa_hashmap_iterate((h), (state), NULL)) +/* A macro to ease itration through all key, value pairs */ +#define PA_HASHMAP_FOREACH_KV(k, e, h, state) \ +for ((state) = NULL, (e) = pa_hashmap_iterate((h), (state), (const void **) (k)); (e); (e) = pa_hashmap_iterate((h), (state), (const void **) (k))) + /* A macro to ease iteration through all entries, backwards */ #define PA_HASHMAP_FOREACH_BACKWARDS(e, h, state) \ for ((state) = NULL, (e) = pa_hashmap_iterate_backwards((h), (state), NULL); (e); (e) = pa_hashmap_iterate_backwards((h), (state), NULL)) I applied this patch (I need the macro myself). -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] default/system.pa: Do not load module-dbus-protocol
On Fri, 2013-09-27 at 10:32 +0200, David Henningsson wrote: The author of this module, Tanu Kaskinen, has said that this module is not suitable for general use. Also, it is still causing crashes on card removal (see bug 69871). Qpaeq, and possibly other tools, use this module - but they can load the module manually if they still wish to use it. Signed-off-by: David Henningsson david.hennings...@canonical.com --- src/daemon/default.pa.in |7 --- src/daemon/system.pa.in |5 - 2 files changed, 12 deletions(-) diff --git a/src/daemon/default.pa.in b/src/daemon/default.pa.in index f50d929..4e77ae9 100755 --- a/src/daemon/default.pa.in +++ b/src/daemon/default.pa.in @@ -171,13 +171,6 @@ load-module module-filter-heuristics load-module module-filter-apply ])dnl -ifelse(@HAVE_DBUS@, 1, [dnl -### Load DBus protocol -.ifexists module-dbus-protocol@PA_SOEXT@ -load-module module-dbus-protocol -.endif -])dnl - ifelse(@HAVE_X11@, 1, [dnl # X11 modules should not be started from default.pa so that one daemon # can be shared by multiple sessions. diff --git a/src/daemon/system.pa.in b/src/daemon/system.pa.in index e881a12..6da880e 100755 --- a/src/daemon/system.pa.in +++ b/src/daemon/system.pa.in @@ -52,11 +52,6 @@ load-module module-device-restore ### that look up the default sink/source get the right value load-module module-default-device-restore -.ifexists module-dbus-protocol@PA_SOEXT@ -### If you want to allow TCP connections, set access to remote or local,remote. -load-module module-dbus-protocol access=local -.endif - ### Automatically move streams to the default sink if the sink they are ### connected to dies, similar for sources load-module module-rescue-streams Looks good to me. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Using nodes as the primary routing mechanism
On Tue, 2013-10-01 at 13:12 +0200, David Henningsson wrote: On 09/23/2013 11:50 AM, Tanu Kaskinen wrote: Hi all, With nodes, there will be two routing layers: the higher-level node layer and the lower-level layer with sink inputs, sinks etc. The user will be able to still use the old move sink input interface, but what to do if the user tries to move a sink input that has a node to a sink that doesn't have a node? From the node layer point of view the sink input is not routed anywhere. Normally, if a sink input node is not routed anywhere, the routing system will connect the sink input to a private null sink (the null sink is dynamically created for the sink input, is not used for anything else, and doesn't have a node). However, in this case the sink input should not be connected to the null sink, but to the sink that the user specified. Respecting the user choice would require some extra complexity in the routing system. My proposal is to avoid this complexity by not allowing the user to route sink inputs with a node to sinks without a node, and also by completely preventing the user from controlling the routing of sink inputs that don't have a node. This would mean that we can map all valid sink input move requests to node operations, because both the sink input and the target sink would always have a node. A consequence of this proposal would be that I should implement a node for every sink and sink input in the current code base, including monitors and loopbacks etc., because otherwise there would be regressions for users. I was originally hoping that I could postpone that work for some later time (or in the best case, avoid that work completely). For example, I was planning not to implement nodes for the streams of module-loopback, but if I don't do that, then there will be a regression: it was previously possible for users to move the sink input and source output of module-loopback, but without nodes for the sink input and source output that wouldn't be possible. This wouldn't mean that every loopback would have to have nodes for the streams: loopbacks created by the routing system itself don't need nodes, but module-loopback instances loaded by users do need nodes. In conclusion: I'm proposing that nodes will be the primary routing mechanism, and the old non-node-aware routing interfaces will be implemented on top of the node interfaces, not side-by-side. Operations where the translation can't be done will simply fail. Regressions can be avoided by creating nodes for all entities that were user-routable before. Sorry for the late answer. The plan you're now suggesting is more in line with what I thought originally, and also, I think it is a better architecture. This also turns the node routing layer into a more complete graph as nodes can be both input and output nodes. If a sink is a node, then it would both be able to take audio from its sink inputs, as well as give audio to its monitors, i e, both an input and output node. Is that a correct understanding? I did not plan to allow dual-direction nodes. If you want to represent sinks as dual-direction nodes in a UI, I'm fine with adding information to the nodes that allows the UI to do the correlation. I'm less enthusiastic about doing the merging in the core. That said, I don't have better arguments for this than that allowing dual-direction nodes may cause complications, and I don't have good examples of such complications at this point, so I may change my opinion. I have bad example, though: what's the type and owner of the node, if it covers both a sink and a monitor source? This is a bad example, because the current system with a single type and owner is broken anyway. A headset output on a typical phone can belong to two domains: the pulse domain and a dsp domain. In the pulse domain the headset output is owned by a port, but in the dsp domain there's no sink (the audio comes directly from the cellular modem without pulseaudio ever seeing the audio samples), and therefore no port either, so the node type and owner is domain-dependent, that is, whoever wants to know the type or owner must ask that through an interface to which the caller provides the domain. It's not much of a leap then to make the type and owner also direction-dependent: just add a direction parameter to the pa_node_get_owner() function. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Using nodes as the primary routing mechanism
On Tue, 2013-10-01 at 15:09 +0200, David Henningsson wrote: On 10/01/2013 02:51 PM, Tanu Kaskinen wrote: I did not plan to allow dual-direction nodes. If you want to represent sinks as dual-direction nodes in a UI, I'm fine with adding information to the nodes that allows the UI to do the correlation. I'm less enthusiastic about doing the merging in the core. That said, I don't have better arguments for this than that allowing dual-direction nodes may cause complications, and I don't have good examples of such complications at this point, so I may change my opinion. So a filter sink, which both consumes and produces audio data, would then have two nodes instead, one for input and one for output? Could you elaborate on this, i e, how these two nodes (belonging to the same filter sink) are connected to each other? The filter sink creates a sink and a sink input as before, and requests that a node is created for both. There's no need to explicitly connect those two nodes - they will be connected by the fact that the filter sink forwards the audio from the sink to its sink input, as it has always done. To allow a nice graph in a UI, the sink input node can indicate with an extra pointer that it's really part of the same node as the sink node and vice versa. I do see the point of dual-direction nodes, and I'm starting to think the idea should be tried out before committing to the solution of adding the extra pointers between e.g. filter sinks and their sink inputs. I added Janos to CC, in case he has some opinions about this. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 4/5] bluetooth: Remove device_remove_all()
On Tue, 2013-10-01 at 10:43 -0300, João Paulo Rechi Vita wrote: Ack. Same question about testing applies here, and I should have made it more generic, actually: Do you have audio devices to be able to test these changes? Devices aren't a problem (I have two A2DP/HFP headsets and one HFP-only headset), but I haven't yet installed BlueZ 5. So, these patches have not been tested. What would you recommend as an easy-to-use GUI to replace the Gnome Bluetooth applet, which will inevitably break if I replace BlueZ 4 with BlueZ 5? (I don't have the newest Gnome version yet, and I'm not going to install Gnome from source.) -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 3/5] bluetooth: Fix notifying about new devices
On Tue, 2013-10-01 at 10:34 -0300, João Paulo Rechi Vita wrote:
On Sun, Sep 29, 2013 at 12:49 PM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
Normally devices are created and their properties are parsed before
creating any transports for the device, and the
DEVICE_CONNECTION_CHANGED hook is fired when the first transport is
created. It's possible, however, that a transport is created before
the device that it belongs to, and in this case
DEVICE_CONNECTION_CHANGED should be fired when the properties have
been successfully parsed for the device. The old code didn't fire the
hook at this situation, and this patch fixes that.
---
src/modules/bluetooth/bluez5-util.c | 43
+
1 file changed, 34 insertions(+), 9 deletions(-)
diff --git a/src/modules/bluetooth/bluez5-util.c
b/src/modules/bluetooth/bluez5-util.c
index eaff4b1..c7e934e 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -438,14 +438,29 @@ static void device_remove(pa_bluetooth_discovery *y,
const char *path) {
}
}
+static void set_device_info_valid(pa_bluetooth_device *device, int valid) {
+bool old_any_connected;
+
+pa_assert(device);
+pa_assert(valid == -1 || valid == 0 || valid == 1);
+
+if (valid == device-device_info_valid)
+return;
+
+old_any_connected =
pa_bluetooth_device_any_transport_connected(device);
+device-device_info_valid = valid;
+
+if (pa_bluetooth_device_any_transport_connected(device) !=
old_any_connected)
+
pa_hook_fire(device-discovery-hooks[PA_BLUETOOTH_HOOK_DEVICE_CONNECTION_CHANGED],
device);
+}
+
I like the idea of having a helper function here. But wouldn't be
better to factor the occurences of device-device_info_valid = X
into set_device_info_valid() in one commit and the
DEVICE_CONNECTION_CHANGED hook fire, together with the respective
checks, in a separate commit? I think they are two different
modifications, so the would belong to separate commits, making the
history cleaner and easier to read. If for some reason you don't want
to separate them it's ok for me (just please explain me why you think
so).
Good point, I'll update the patches.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Best Case Latency
On Thu, 2013-10-03 at 02:33 +1000, Patrick Shirkey wrote: On Thu, October 3, 2013 12:14 am, Tanu Kaskinen wrote: On Tue, 2013-10-01 at 00:38 +1000, Patrick Shirkey wrote: On Sat, September 28, 2013 3:12 am, Patrick Shirkey wrote: On Sat, September 28, 2013 12:53 am, Tanu Kaskinen wrote: I don't remember if the alsa compatibility layer forwards the stream under/overrun notifications to the alsa application - IIRC there were problems when those were reported and there were different problems when those were not reported, and I don't remember what the current code does. This seems like it would be a good place to verify. Looking at the pulse alsa plugin source, it seems that stream overflow events (the recording direction) are not reported. Underflows (the playback direction) are reported by default (configurable in asoundrc). Overflows are pretty unlikely, since the maximum stream buffer length appears to be configured to 4 MB. I see the following sprinkled in /var/log/messages Sep 30 12:19:02 xxx pulseaudio[28845]: [pulseaudio] ratelimit.c: 1997 events suppressed Sep 30 12:19:02 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally snip Sep 30 21:40:04 xxx pulseaudio[28845]: [jack-source] ratelimit.c: 41 events suppressed Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [jack-source] asyncq.c: q overrun, queuing locally snip Oct 1 09:53:30 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] ratelimit.c: 2291 events suppressed Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:42 xxx pulseaudio[28845]: [pulseaudio] ratelimit.c: 2321 events suppressed That doesn't look healthy. The message is printed when pa_asyncmsgq_post() is called and the message queue is full. The message queue can store 256 messages before this starts to happen, so some queue consumer is having serious trouble keeping up with the producer. It would be nice to know which pa_asyncmsgq_post() call this is (you could set a breakpoint on the line that prints q overrun, and then get a backtrace). Also does anyone know why the PA Stream Buffer is hovering around 10ms? That number doesn't appear to match the 64 frames/period that JACK is set to so it would be useful to verify that it is calculated correctly and/or if it can be lowered. You can see in the pulseaudio log, when a stream is created, what memblockq parameters the client requests and what are the final parameters. According to pcm_pulse.c, the requested stream buffer length (tlength in the memblockq parameters) should be io-buffer_size * pcm-frame_size; I don't know how io-buffer_size is calculated, but I'd expect that to be based on the period number and size provided by you (number of periods * period size). 2 (periods) * 64 (frames per period) * 4 (bytes per frame) / 48 (frames/millisecond) equals 10.67 milliseconds, so it looks like the reported latency of 10 milliseconds for playback is what you'd expect. Thanks
Re: [pulseaudio-discuss] Best Case Latency
On Thu, 2013-10-03 at 18:20 +1000, Patrick Shirkey wrote: On Thu, October 3, 2013 5:14 pm, Tanu Kaskinen wrote: On Thu, 2013-10-03 at 02:33 +1000, Patrick Shirkey wrote: I see the following sprinkled in /var/log/messages Sep 30 12:19:02 xxx pulseaudio[28845]: [pulseaudio] ratelimit.c: 1997 events suppressed Sep 30 12:19:02 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally snip Sep 30 21:40:04 xxx pulseaudio[28845]: [jack-source] ratelimit.c: 41 events suppressed Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Sep 30 21:40:04 xxx pulseaudio[28845]: [jack-source] asyncq.c: q overrun, queuing locally snip Oct 1 09:53:30 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] ratelimit.c: 2291 events suppressed Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:20 xxx pulseaudio[28845]: [pulseaudio] asyncq.c: q overrun, queuing locally Oct 1 10:01:42 xxx pulseaudio[28845]: [pulseaudio] ratelimit.c: 2321 events suppressed That doesn't look healthy. The message is printed when pa_asyncmsgq_post() is called and the message queue is full. The message queue can store 256 messages before this starts to happen, so some queue consumer is having serious trouble keeping up with the producer. It would be nice to know which pa_asyncmsgq_post() call this is (you could set a breakpoint on the line that prints q overrun, and then get a backtrace). Sorry, if this is dense but how do I set a breakpoint on this line in PA while it is running? Have you used gdb before? You can either start pulseaudio in gdb, or connect gdb to a running pulseaudio instance. The latter can be done with command gdb pulseaudio $PID_OF_RUNNING_PULSEAUDIO. See man gdb. Either approach requires debug symbols to be available. If building from source is not a problem, then I recommend building and installing[1] the latest pulseaudio from git, which is the distro independent way of getting the debug symbols. Once you're in the gdb prompt, run this command (the line number refers to the current code in the master branch, adjust the line number as necessary if you use some other version): break async.c:211 If you connected to a running pulseaudio, continue the execution with this command: continue If you started pulseaudio in gdb, start the execution with this command: run [1] http://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/PulseAudioFromGit/ -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH] simple: Improve pa_simple_read() documentation
There was a question in IRC about whether pa_simple_read() blocks or not. It's already documented on the simple API overview page, but it's good to say it also in the function reference. As a bonus, I added some words about the error handling as well. --- src/pulse/simple.h | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/pulse/simple.h b/src/pulse/simple.h index 0fab8ee..4a8162b 100644 --- a/src/pulse/simple.h +++ b/src/pulse/simple.h @@ -137,7 +137,10 @@ int pa_simple_write(pa_simple *s, const void *data, size_t bytes, int *error); /** Wait until all data already written is played by the daemon. */ int pa_simple_drain(pa_simple *s, int *error); -/** Read some data from the server. */ +/** Read some data from the server. This function blocks until \a bytes amount + * of data has been received from the server, or until an error occurs. + * Returns a negative value on failure. If \a error is non-NULL, the error code + * is stored in it. */ int pa_simple_read(pa_simple *s, void *data, size_t bytes, int *error); /** Return the playback latency. */ -- 1.8.4.rc3 ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] connect pulse to jack - headless
On Fri, 2013-10-04 at 19:15 +1000, Patrick Shirkey wrote: Hi, Can anyone point me to instructions for connecting pulse to jack in headless mode? I can run jack like this: eval dbus-launch --auto-syntax export DBUS_SESSION_BUS_ADDRESS='unix:abstract=/tmp/dbus-UBmwIyooQF,guid=86f4d04e50ca1406a4b7f215524db419' DBUS_SESSION_BUS_PID=6293; jackd -d alsa -r48000 -p64 -n2 But how do I get PA to run and connect to the same dbus session? pulseaudio -D runs but does not connect to jack Having DBUS_SESSION_BUS_ADDRESS set to the same value in pulseaudio's environment should work. I don't know if DBUS_SESSION_BUS_PID is important at all, but it shouldn't hurt to set that variable too. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH v2] simple: Improve pa_simple_read() documentation
From: Tanu Kaskinen ta...@iki.fi There was a question in IRC about whether pa_simple_read() blocks or not. It's already documented on the simple API overview page, but it's good to say it also in the function reference. As a bonus, I added some additional details to the documentation too. --- src/pulse/simple.h | 13 +++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/pulse/simple.h b/src/pulse/simple.h index 0fab8ee..2224766 100644 --- a/src/pulse/simple.h +++ b/src/pulse/simple.h @@ -137,8 +137,17 @@ int pa_simple_write(pa_simple *s, const void *data, size_t bytes, int *error); /** Wait until all data already written is played by the daemon. */ int pa_simple_drain(pa_simple *s, int *error); -/** Read some data from the server. */ -int pa_simple_read(pa_simple *s, void *data, size_t bytes, int *error); +/** Read some data from the server. This function blocks until \a bytes amount + * of data has been received from the server, or until an error occurs. + * Returns a negative value on failure. */ +int pa_simple_read( +pa_simple *s, /** The connection object. */ +void *data, /** A pointer to a buffer. */ +size_t bytes, /** The number of bytes to read. */ +int *error +/** A pointer where the error code is stored when the function returns + * a negative value. It is OK to pass NULL here. */ +); /** Return the playback latency. */ pa_usec_t pa_simple_get_latency(pa_simple *s, int *error); -- 1.8.1.2 ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 2/2] tunnel-sink-new: use *_new-style for thread_mq init
On Mon, 2013-09-16 at 13:06 +0200, Alexander Couzens wrote:
@@ -528,11 +529,14 @@ void pa__done(pa_module *m) {
pa_sink_unlink(u-sink);
if (u-thread) {
-pa_asyncmsgq_send(u-thread_mq.inq, NULL, PA_MESSAGE_SHUTDOWN, NULL,
0, NULL);
+pa_asyncmsgq_send(u-thread_mq-inq, NULL, PA_MESSAGE_SHUTDOWN,
NULL, 0, NULL);
pa_thread_free(u-thread);
}
-pa_thread_mq_done(u-thread_mq);
+if (u-thread_mq) {
+pa_thread_mq_done(u-thread_mq);
+pa_xfree(u-thread);
This should be u-thread_mq, not u-thread.
Otherwise looks good, I pushed now this and the other (1/2) patch.
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] Best Case Latency
On Fri, 2013-10-04 at 21:49 +1000, Patrick Shirkey wrote: On Thu, October 3, 2013 11:41 pm, Thomas Martitz wrote: Am 03.10.2013 10:20, schrieb Patrick Shirkey: On Thu, October 3, 2013 5:14 pm, Tanu Kaskinen wrote: That doesn't look healthy. The message is printed when pa_asyncmsgq_post() is called and the message queue is full. The message queue can store 256 messages before this starts to happen, so some queue consumer is having serious trouble keeping up with the producer. It would be nice to know which pa_asyncmsgq_post() call this is (you could set a breakpoint on the line that prints q overrun, and then get a backtrace). Sorry, if this is dense but how do I set a breakpoint on this line in PA while it is running? $ gdb --pid=$(pidof pulseaudio) If you're on debian you need to install debug symbols ($ sudo apt-get install pulseaudio-dbg). For other distros I can't help. Compiling from source in debug mode of course also works. Thanks. FYI, installing pulseaudio-dbg hosed my NVIDIA video driver which was a bit confusing. Looking at /var/log/messages I spotted this when starting PA: Oct 4 17:30:49 xxx pulseaudio[6571]: [pulseaudio] sink.c: Default and alternate sample rates are the same. Oct 4 17:30:49 xxx pulseaudio[6571]: [pulseaudio] module-jack-sink.c: JACK error Cannot use real-time scheduling (RR/5)(1: Operation not permitted) Oct 4 17:30:49 xxx pulseaudio[6571]: [pulseaudio] module-jack-sink.c: JACK error JackClient::AcquireSelfRealTime error Oct 4 17:30:49 xxx pulseaudio[6571]: [pulseaudio] source.c: Default and alternate sample rates are the same. Oct 4 17:30:49 xxx pulseaudio[6571]: [pulseaudio] module-jack-source.c: JACK error Cannot use real-time scheduling (RR/5)(1: Operation not permitted) Oct 4 17:30:49 xxx pulseaudio[6571]: [pulseaudio] module-jack-source.c: JACK error JackClient::AcquireSelfRealTime error - Not sure why it is being printed twice. Once for the sink and once for the source. I have enabled realtime in /etc/pulse/daemon.conf and jack is running in realtime mode so not sure why PA is unhappy. It could be a bug that is already fixed though. It's not really PA that is unhappy. The JACK error foo messages come from libjack. If you are able to run jackd with RT scheduling, under the same user that you're running pulseaudio, I don't know what could be the problem. Pulseaudio limits the realtime priority that it can use (configured with the rlimit-rtprio option in daemon.conf), but if I understand the log message correctly, libjack tries to use priority 5, which shouldn't be a problem, because the default priority that pulseaudio limits itself is 9. - I'm running pulseaudio 4.0 in debian pulseaudio-dbg:amd64/unstable 4.0-6+b1 - Here's the backtrace based on the line number that Tanu provided. Not sure if I nailed it though. (gdb) break asyncq.c:211 Breakpoint 1 at 0x7f4d4572ff90: file pulsecore/asyncq.c, line 211. (gdb) continue Continuing. [Switching to Thread 0x7f4d39b2b700 (LWP 6686)] Breakpoint 1, 0x7f4d4572ff90 in pa_asyncq_post () from /usr/lib/libpulsecore-4.0.so (gdb) bt #0 0x7f4d4572ff90 in pa_asyncq_post () from /usr/lib/libpulsecore-4.0.so #1 0x7f4d4572ee7b in pa_asyncmsgq_post () at pulsecore/asyncmsgq.c:137 #2 0x7f4d3bf374f3 in source_output_push_cb () at pulsecore/protocol-native.c:1832 #3 0x7f4d4576d3b3 in pa_source_output_push () at pulsecore/source-output.c:822 #4 0x7f4d45773e5b in pa_source_post () at pulsecore/source.c:935 #5 0x7f4d39baf820 in source_process_msg () at modules/jack/module-jack-source.c:115 #6 0x7f4d4574d084 in asyncmsgq_read_work () from /usr/lib/libpulsecore-4.0.so #7 0x7f4d4574c607 in pa_rtpoll_run () from /usr/lib/libpulsecore-4.0.so #8 0x7f4d39baf953 in thread_func () at modules/jack/module-jack-source.c:201 #9 0x7f4d454f5d08 in ?? () from /usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-4.0.so #10 0x7f4d449fae0e in start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0 #11 0x7f4d4402593d in clone () from /lib/x86_64-linux-gnu/libc.so.6 Ok, so this happens when the native protocol tries to send a block of audio from the jack source thread to the main thread. My guess is that the main thread is not getting enough CPU time to deal with all the incoming audio (it's not RT scheduled). This could very well cause growing latency. If the jack source pushes audio blocks to the message queue faster than the main thread reads those blocks, then that message queue becomes a significant (and constantly growing?) audio buffer. What to do? I don't know. Currently there's no upper limit for how long the message queue can grow (so this is effectively also a memory leak problem), and no way to query the queue length. Even if there was a maximum length for the queue, what should be done if that maximum length is exceeded? Perhaps the native protocol should
Re: [pulseaudio-discuss] [PATCH v5 39/39] bluetooth: Revive module-bluetooth-discover
On Tue, 2013-10-08 at 11:19 -0300, João Paulo Rechi Vita wrote:
On Sun, Sep 29, 2013 at 1:39 PM, Tanu Kaskinen
tanu.kaski...@linux.intel.com wrote:
On Tue, 2013-09-24 at 19:45 -0300, jprv...@gmail.com wrote:
+void pa__done(pa_module* m) {
+struct userdata *u;
+
+pa_assert(m);
+
+if (!(u = m-userdata))
+return;
+
+if (u-bluez5_module)
+pa_module_unload(m-core, u-bluez5_module, true);
+
+if (u-bluez4_module)
+pa_module_unload(m-core, u-bluez4_module, true);
This crashes when shutting down the daemon, because when the daemon
unloads all modules, module-bluez*-discover gets unloaded before
module-bluetooth-discover, so the y-bluez5_module and u-bluez4_module
pointers become stale. I see two ways of fixing this: add a hook that is
fired when modules are unloaded and use that hook in
module-bluetooth-discover to drop the reference to the unloaded module,
or unload module-bluetooth-discover immediately after loading
module-bluez5-discover and module-bluez4-discover. The second solution
is of course much simpler, but I proposed that already earlier, and you
didn't like that.
It doesn't crash (and that's what I'm experiencing here) because
pa_module_unload() will look for that module pointer in its internal
hash of modules before trying to unload it. I agree we are left with a
stale pointer, but as long as we don't dereference it, we should be
fine.
pa_module_unload() dereferences the pointer already before
if (!(m = pa_idxset_remove_by_data(c-modules, m, NULL)))
return;
which I think you are referring to as the safeguard. The line that
crashes is this:
if (m-core-disallow_module_loading !force)
It doesn't crash always, because m-core is a random address that may or
may not be possible to dereference (it was 0 when I debugged this).
When this line doesn't crash, I get an assertion about pa_core.shared
not being empty when pa_core is freed. I hope that's another symptom of
this same bug (I looked at the code and the management of pa_core.shared
seemed correct).
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] sndfile-util: fix format for 24bit depth wav files
On Mon, 2013-10-07 at 09:43 -0700, Kiran Krishnappa wrote:
PA_SAMPLE_24NE generated in pa_sndfile_read_sample_spec is not
handled in pa_sndfile_readf_function. paplay used to get aborted
for 24bit depth wav files
How does paplay now behave? I suppose it still isn't able to play 24bit
files, so does it exit cleanly?
---
src/pulsecore/sndfile-util.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/src/pulsecore/sndfile-util.c b/src/pulsecore/sndfile-util.c
index 0820ee4..46689af 100644
--- a/src/pulsecore/sndfile-util.c
+++ b/src/pulsecore/sndfile-util.c
@@ -386,6 +386,9 @@ pa_sndfile_readf_t pa_sndfile_readf_function(const
pa_sample_spec *ss) {
case PA_SAMPLE_ALAW:
return NULL;
+case PA_SAMPLE_S24NE:
+return NULL;
I'd prefer the case to be grouped together with ULAW and ALAW, since in
all of the three cases the end result is the same: return NULL.
Also, this same problem is in pa_sndfile_writef_function(). In the bug
report there is a parecord command that triggers a crash in
pa_sndfile_writef_function(). If you could fix also
pa_sndfile_writef_function() and test how parecord behaves after that,
your patch will go in sooner (if you don't do this, I'll do the changes
and testing myself when I have time).
--
Tanu
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] sndfile-util: fix format for 24bit depth wav files
On Thu, 2013-10-10 at 20:01 -0700, Kiran Krishnappa wrote: Hi, I tested both paplay and parecord for 24bit files. Looks to be working fine now. How can it be working fine, if the read/write functions don't exist for 24-bit files? I looked at the code, and it seems that if the read/write functions haven't been set, pacat treats the files as raw PCM data. This is not good, because the user requested wav files, not raw files. If wav files are treated as raw files, then the wav header will be played to the speakers, and when recording, the wav header will be missing from the output file. If getting the read/write function fails, pacat should print an error message and exit. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH 0/2] Conditional defining of _FORTIFY_SOURCE
I switched to Fedora, and the system headers complained to me about defining _FORTIFY_SOURCE when compiling without optimizations. I don't know how other Fedora users have managed to live with this all these years... Tanu Kaskinen (2): build-sys: Don't define _FORTIFY_SOURCE when building with -O0 build-sys: Print CPPFLAGS in configure configure.ac | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) -- 1.8.3.1 ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
[pulseaudio-discuss] [PATCH 1/2] build-sys: Don't define _FORTIFY_SOURCE when building with -O0
--- configure.ac | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 42cb8b8..e2465e4 100644 --- a/configure.ac +++ b/configure.ac @@ -165,12 +165,17 @@ esac Compiler flags AX_APPEND_COMPILE_FLAGS( -[-Wall -W -Wextra -pipe -Wno-long-long -Wno-overlength-strings -Wunsafe-loop-optimizations -Wundef -Wformat=2 -Wlogical-op -Wsign-compare -Wformat-security -Wmissing-include-dirs -Wformat-nonliteral -Wold-style-definition -Wpointer-arith -Winit-self -Wdeclaration-after-statement -Wfloat-equal -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wmissing-declarations -Wmissing-noreturn -Wshadow -Wendif-labels -Wcast-align -Wstrict-aliasing -Wwrite-strings -Wno-unused-parameter -ffast-math -Wp,-D_FORTIFY_SOURCE=2 -fno-common -fdiagnostics-show-option], +[-Wall -W -Wextra -pipe -Wno-long-long -Wno-overlength-strings -Wunsafe-loop-optimizations -Wundef -Wformat=2 -Wlogical-op -Wsign-compare -Wformat-security -Wmissing-include-dirs -Wformat-nonliteral -Wold-style-definition -Wpointer-arith -Winit-self -Wdeclaration-after-statement -Wfloat-equal -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wmissing-declarations -Wmissing-noreturn -Wshadow -Wendif-labels -Wcast-align -Wstrict-aliasing -Wwrite-strings -Wno-unused-parameter -ffast-math -fno-common -fdiagnostics-show-option], [], [-pedantic -Werror]) # Only enable fastpath asserts when doing a debug build, e.g. from bootstrap.sh. AS_CASE([ $CFLAGS ], [* -O0 *], [], [AX_APPEND_FLAG([-DFASTPATH], [CPPFLAGS])]) +# Only set _FORTIFY_SOURCE when optimizations are enabled. If optimizations +# are disabled, _FORTIFY_SOURCE doesn't do anything, and causes tons of +# warnings during compiling on some distributions (at least Fedora). +AS_CASE([ $CFLAGS ], [* -O0 *], [], [AX_APPEND_FLAG([-D_FORTIFY_SOURCE=2], [CPPFLAGS])]) + Linker flags -- 1.8.3.1 ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] sndfile-util: fix format for 24bit depth wav files
[Added pulseaudio-discuss back to CC.] On Sat, 2013-10-12 at 09:31 -0700, Kiran Krishnappa wrote: Ok. I got it. I didn't think much about wav file header. Just played 24 bit file and also recorded 24 bit file, there were no aborts and when I played 24bit file, it couldn't make out difference. I will understand wav format and sndfile API's and try to give proper patch. Actually, it looks like I was wrong. I assumed that falling back to sf_read_raw() and sf_write_raw() would mean that the wav header wouldn't be handled correctly, but after reading the API documentation for those functions, I believe that was an invalid assumption. So, your patch should be fine. I'll apply it. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
