Re: [pulseaudio-discuss] pulseaudio master is frozen

[Pali Rohár]

On Tuesday 02 July 2019 08:56:08 Pali Rohár wrote:
> On Tuesday 02 July 2019 08:56:29 Tanu Kaskinen wrote:
> > On Mon, 2019-07-01 at 11:29 +0530, Arun Raghavan wrote:
> > > Hi folks,
> > > As we were discussing, 13.0 is a bit overdue, so we're freezing
> > > master now. Release tracker issue is at:
> > > 
> > >   https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/174
> > > 
> > > Just as a refresher, this means we will not be landing any new
> > > features to master without an explicit exception, and only critical
> > > bug fixes should land outside of those changes.
> > > 
> > > We used to keep a temporary 'next' branch for the times where the
> > > freeze cycle went longer and we didn't want to hold reviewed patches
> > > back. This may not be necessary any more with the Gitlab merge
> > > request system
> > > 
> > > We have discussed an exception for Pali's initial A2DP fixes,
> > > Russell's info script, and the fixes for the pending bug on the
> > > tracker relating to detecting when we are running in a VM.
> > 
> > I had a look at where we are with Pali's patches, and it seems that the
> > patches that we really want are reviewed, and there were some
> > improvements requested. Therefore this part is currently blocked on
> > waiting for new patch versions.
> > 
> > Pali, do you have fixes for the first three (or even just the first
> > two) patches? These:
> >  * bluetooth: Fix A2DP codec API to provide information about data
> > buffer
> >  * bluetooth: Fix usage of RTP structures in SBC codec
> >  * bluetooth: Implement reading SO_TIMESTAMP for A2DP source
> 
> Yes, I have fixes for all these patches. Just I need to squash commits,
> properly test them and prepare via format-patch. I guess that tomorrow
> evening I should have time for it.

Sorry for delay. Now I sent new version of patches to ML.

-- 
Pali Rohár
pali.ro...@gmail.com


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

Re: [pulseaudio-discuss] [PATCH v11 01/11] bluetooth: Fix A2DP codec API to provide information about data buffer

[Pali Rohár]

On Tuesday 18 June 2019 10:07:15 Tanu Kaskinen wrote:
> On Mon, 2019-06-17 at 12:05 +0200, Pali Rohár wrote:
> > On Saturday 15 June 2019 11:50:10 Tanu Kaskinen wrote:
> > > Although I made that mistake, I think I'm right in saying that our
> > > reading logic is broken at least with SBC. The sender can change the
> > > frame size without warning, so we shouldn't base our read (encoded)
> > > buffer size on that. If our buffer size is less than MTU (which it
> > > currently can be), the frame size may change in such a way that future
> > > packets are larger than our allocated read buffer. That will lead to
> > > reading partial packets.
> > > 
> > > This is what I think would be correct:
> > > 
> > > 1) Use the read MTU as the encoded data buffer size.
> > > 
> > > 2) After calling pa_read(), inspect the RTP header to find out the RTP
> > > packet payload size. If it's larger than what can fit our read buffer,
> > > that's an error, because the packets shouldn't exceed the MTU.
> > > 
> > > 3) Decode only the payload part, not the whole buffer.
> > > 
> > > 4) It's unfortunately possible (or so I think until proven otherwise)
> > > that there were two RTP packets queued in the socket, and the first one
> > > didn't fill the MTU completely, so we have the beginning of the second
> > > packet in our read buffer. If this is the case, we have to save the
> > > leftover part somewhere. That somewhere can be the beginning of the
> > > read buffer. The next time we read from the socket, we read using an
> > > offset so that the new data goes after the earlier leftover data.
> > > 
> > > 5) When the streaming stops, the leftover offset needs to be reset, so
> > > that it doesn't cause trouble when restarting streaming later.
> > 
> > I'm not sure if all this can happen. A2DP socket created by bluez, which
> > is passed to pulseaudio via dbus, is of type SOCK_SEQPACKET.
> > 
> > man 2 socket describes it as:
> > 
> >   SOCK_SEQPACKET  Provides a sequenced, reliable, two-way connection-
> >   based data transmission path for datagrams of fixed
> >   maximum length; a consumer is required to read an
> >   entire packet with each input system call.
> > 
> >   SOCK_SEQPACKET sockets employ the same system calls as
> >   SOCK_STREAM sockets.  The only difference is that read(2) calls will
> >   return only the amount of data requested, and any data remaining in
> >   the arriving packet will be discarded.  Also all message boundaries
> >   in incoming datagrams are preserved.
> 
> Ok, that's great! There won't be any leftover data that we need to be
> concerned about. And if we call msgrcv() with 100 byte buffer and the
> frame is 70 bytes, we will get only 70 bytes. We just need to make sure
> that the read buffer is always big enough (equal to MTU should
> suffice).

Fixed in v12 that buffer has correct size.

-- 
Pali Rohár
pali.ro...@gmail.com


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

[pulseaudio-discuss] [PATCH v12 05/13] bluetooth: Print SO_TIMESTAMP warning for SCO source only once

[Pali Rohár]

---
 src/modules/bluetooth/module-bluez5-device.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/modules/bluetooth/module-bluez5-device.c 
b/src/modules/bluetooth/module-bluez5-device.c
index efb092089..a97c6c703 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -395,7 +395,9 @@ static int sco_process_push(struct userdata *u) {
 }
 
 if (!found_tstamp) {
-pa_log_warn("Couldn't find SO_TIMESTAMP data in auxiliary recvmsg() 
data!");
+PA_ONCE_BEGIN {
+pa_log_warn("Couldn't find SO_TIMESTAMP data in auxiliary 
recvmsg() data!");
+} PA_ONCE_END;
 tstamp = pa_rtclock_now();
 }
 
-- 
2.11.0

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

[pulseaudio-discuss] [PATCH v12 02/13] bluetooth: Change A2DP codec API of reset() method to indicate failure

[Pali Rohár]

SBC codec reset() method may fail, so propagate this failure to caller.
---
 src/modules/bluetooth/a2dp-codec-api.h   |  4 ++--
 src/modules/bluetooth/a2dp-codec-sbc.c   |  5 +++--
 src/modules/bluetooth/module-bluez5-device.c | 18 --
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/src/modules/bluetooth/a2dp-codec-api.h 
b/src/modules/bluetooth/a2dp-codec-api.h
index 881cc659b..bc4844596 100644
--- a/src/modules/bluetooth/a2dp-codec-api.h
+++ b/src/modules/bluetooth/a2dp-codec-api.h
@@ -69,8 +69,8 @@ typedef struct pa_a2dp_codec {
 void *(*init)(bool for_encoding, bool for_backchannel, const uint8_t 
*config_buffer, uint8_t config_size, pa_sample_spec *sample_spec);
 /* Deinitialize and release codec info data in codec_info */
 void (*deinit)(void *codec_info);
-/* Reset internal state of codec info data in codec_info */
-void (*reset)(void *codec_info);
+/* Reset internal state of codec info data in codec_info, returns non-zero 
on failure */
+int (*reset)(void *codec_info);
 
 /* Get read block size for codec, it is minimal size of buffer
  * needed to decode read_link_mtu bytes of encoded data */
diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c 
b/src/modules/bluetooth/a2dp-codec-sbc.c
index e2db91b63..f57c7b01a 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -466,20 +466,21 @@ static void set_bitpool(struct sbc_info *sbc_info, 
uint8_t bitpool) {
 pa_log_debug("Bitpool has changed to %u", sbc_info->sbc.bitpool);
 }
 
-static void reset(void *codec_info) {
+static int reset(void *codec_info) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
 int ret;
 
 ret = sbc_reinit(_info->sbc, 0);
 if (ret != 0) {
 pa_log_error("SBC reinitialization failed: %d", ret);
-return;
+return -1;
 }
 
 /* sbc_reinit() sets also default parameters, so reset them back */
 set_params(sbc_info);
 
 sbc_info->seq_num = 0;
+return 0;
 }
 
 static size_t get_block_size(void *codec_info, size_t link_mtu) {
diff --git a/src/modules/bluetooth/module-bluez5-device.c 
b/src/modules/bluetooth/module-bluez5-device.c
index 140ddb8fb..0b63cd44a 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -762,22 +762,24 @@ static void transport_config_mtu(struct userdata *u) {
 }
 
 /* Run from I/O thread */
-static void setup_stream(struct userdata *u) {
+static bool setup_stream(struct userdata *u) {
 struct pollfd *pollfd;
 int one;
 
 /* return if stream is already set up */
 if (u->stream_setup_done)
-return;
+return true;
 
 pa_log_info("Transport %s resuming", u->transport->path);
 
 if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
 pa_assert(u->a2dp_codec);
-u->a2dp_codec->reset(u->encoder_info);
+if (u->a2dp_codec->reset(u->encoder_info) != 0)
+return false;
 } else if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE) {
 pa_assert(u->a2dp_codec);
-u->a2dp_codec->reset(u->decoder_info);
+if (u->a2dp_codec->reset(u->decoder_info) != 0)
+return false;
 }
 
 transport_config_mtu(u);
@@ -802,6 +804,8 @@ static void setup_stream(struct userdata *u) {
 
 if (u->source)
 u->read_smoother = pa_smoother_new(PA_USEC_PER_SEC, 2*PA_USEC_PER_SEC, 
true, true, 10, pa_rtclock_now(), true);
+
+return true;
 }
 
 /* Called from I/O thread, returns true if the transport was acquired or
@@ -813,8 +817,10 @@ static bool setup_transport_and_stream(struct userdata *u) 
{
 if (transport_error < 0) {
 if (transport_error != -EAGAIN)
 return false;
-} else
-setup_stream(u);
+} else {
+if (!setup_stream(u))
+return false;
+}
 return true;
 }
 
-- 
2.11.0

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

[pulseaudio-discuss] [PATCH v12 08/13] bluetooth: Add A2DP aptX and aptX HD codecs support

[Pali Rohár]

This patch provides support for aptX and aptX HD codecs in bluetooth A2DP
profile. It uses open source LGPLv2.1+ licensed libopenaptx library which
can be found at https://github.com/pali/libopenaptx.

aptX for s24 stereo samples provides fixed 6:1 compression ratio and
bitrate 352.8 kbit/s, aptX HD provides fixed 4:1 compression ratio and
bitrate 529.2 kbit/s.

According to soundexpert research, aptX codec used in bluetooth A2DP is no
better than SBC High Quality settings. And you cannot hear difference
between aptX and SBC High Quality, aptX is just a copper-less overpriced
audio cable.

aptX HD is high-bitrate version of aptX. It has clearly noticeable increase
in sound quality (not dramatic though taking into account the increase in
bitrate).

http://soundexpert.org/news/-/blogs/audio-quality-of-bluetooth-aptx
---
 configure.ac|  36 +++
 src/Makefile.am |   6 +
 src/modules/bluetooth/a2dp-codec-aptx.c | 448 
 src/modules/bluetooth/a2dp-codec-util.c |   8 +
 4 files changed, 498 insertions(+)
 create mode 100644 src/modules/bluetooth/a2dp-codec-aptx.c

diff --git a/configure.ac b/configure.ac
index 79b82845b..60a7866b3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1118,6 +1118,40 @@ AC_SUBST(HAVE_BLUEZ_5_NATIVE_HEADSET)
 AM_CONDITIONAL([HAVE_BLUEZ_5_NATIVE_HEADSET], [test 
"x$HAVE_BLUEZ_5_NATIVE_HEADSET" = x1])
 AS_IF([test "x$HAVE_BLUEZ_5_NATIVE_HEADSET" = "x1"], 
AC_DEFINE([HAVE_BLUEZ_5_NATIVE_HEADSET], 1, [Bluez 5 native headset backend 
enabled]))
 
+ Bluetooth A2DP aptX codec (optional) ###
+
+AC_ARG_ENABLE([aptx],
+AS_HELP_STRING([--disable-aptx],[Disable optional bluetooth A2DP aptX and 
aptX HD codecs support (via libopenaptx)]))
+AC_ARG_VAR([OPENAPTX_CPPFLAGS], [C preprocessor flags for openaptx])
+AC_ARG_VAR([OPENAPTX_LDFLAGS], [linker flags for openaptx])
+
+CPPFLAGS_SAVE="$CPPFLAGS"
+LDFLAGS_SAVE="$LDFLAGS"
+LIBS_SAVE="$LIBS"
+
+CPPFLAGS="$CPPFLAGS $OPENAPTX_CPPFLAGS"
+LDFLAGS="$LDFLAGS $OPENAPTX_LDFLAGS"
+
+AS_IF([test "x$HAVE_BLUEZ_5" = "x1" && test "x$enable_aptx" != "xno"],
+[AC_CHECK_HEADER([openaptx.h],
+[AC_SEARCH_LIBS([aptx_init], [openaptx],
+[HAVE_OPENAPTX=1; AS_IF([test "x$ac_cv_search_aptx_init" != "xnone 
required"], [OPENAPTX_LDFLAGS="$OPENAPTX_LDFLAGS $ac_cv_search_aptx_init"])],
+[HAVE_OPENAPTX=0])],
+[HAVE_OPENAPTX=0])])
+
+CPPFLAGS="$CPPFLAGS_SAVE"
+LDFLAGS="$LDFLAGS_SAVE"
+LIBS="$LIBS_SAVE"
+
+AS_IF([test "x$HAVE_BLUEZ_5" = "x1" && test "x$enable_aptx" = "xyes" && test 
"x$HAVE_OPENAPTX" = "x0"],
+[AC_MSG_ERROR([*** libopenaptx from https://github.com/pali/libopenaptx 
not found])])
+
+AC_SUBST(OPENAPTX_CPPFLAGS)
+AC_SUBST(OPENAPTX_LDFLAGS)
+AC_SUBST(HAVE_OPENAPTX)
+AM_CONDITIONAL([HAVE_OPENAPTX], [test "x$HAVE_OPENAPTX" = "x1"])
+AS_IF([test "x$HAVE_OPENAPTX" = "x1"], AC_DEFINE([HAVE_OPENAPTX], 1, [Have 
openaptx codec library]))
+
  UDEV support (optional) 
 
 AC_ARG_ENABLE([udev],
@@ -1603,6 +1637,7 @@ AS_IF([test "x$HAVE_SYSTEMD_JOURNAL" = "x1"], 
ENABLE_SYSTEMD_JOURNAL=yes, ENABLE
 AS_IF([test "x$HAVE_BLUEZ_5" = "x1"], ENABLE_BLUEZ_5=yes, ENABLE_BLUEZ_5=no)
 AS_IF([test "x$HAVE_BLUEZ_5_OFONO_HEADSET" = "x1"], 
ENABLE_BLUEZ_5_OFONO_HEADSET=yes, ENABLE_BLUEZ_5_OFONO_HEADSET=no)
 AS_IF([test "x$HAVE_BLUEZ_5_NATIVE_HEADSET" = "x1"], 
ENABLE_BLUEZ_5_NATIVE_HEADSET=yes, ENABLE_BLUEZ_5_NATIVE_HEADSET=no)
+AS_IF([test "x$HAVE_OPENAPTX" = "x1"], ENABLE_APTX=yes, ENABLE_APTX=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 
(DEPRECATED)", ENABLE_LIBSAMPLERATE=no)
@@ -1661,6 +1696,7 @@ echo "
   Enable BlueZ 5:  ${ENABLE_BLUEZ_5}
 Enable ofono headsets: ${ENABLE_BLUEZ_5_OFONO_HEADSET}
 Enable native headsets:${ENABLE_BLUEZ_5_NATIVE_HEADSET}
+Enable aptX+aptXHD codecs: ${ENABLE_APTX}
 Enable udev:   ${ENABLE_UDEV}
   Enable HAL->udev compat: ${ENABLE_HAL_COMPAT}
 Enable systemd
diff --git a/src/Makefile.am b/src/Makefile.am
index 438bf57fe..268df2a80 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2161,6 +2161,12 @@ libbluez5_util_la_SOURCES += 
modules/bluetooth/a2dp-codec-sbc.c
 libbluez5_util_la_LIBADD += $(SBC_LIBS)
 libbluez5_util_la_CFLAGS += $(SBC_CFLAGS)
 
+if HAVE_OPENAPTX
+libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-aptx.c
+libbluez5_util_la_CPPFLAGS += $(OPENAPTX_CPPFLAGS)
+libbluez5_util_la_LDFLAGS += $(OPENAPTX_LDFLAGS)
+endif
+
 module_bluez5_discover_la_SOURCES = modules/bluetooth/module-bluez5-discover.c
 module_bluez5_discover_la_LDFLAGS = $(MODULE_LDFLAGS)
 module_bluez5_discover_la_LIBADD = $(MODULE_LIBADD) $(DBUS_LIBS) 
libbluez5-util.la
diff --git a/src/modules/bluetooth/a2dp-codec-aptx.c 

[pulseaudio-discuss] [PATCH v12 07/13] bluetooth: Set initial A2DP profile which bluez already activated

[Pali Rohár]

Bluez and remote device decide which A2DP codec would use. Use this
selected A2DP codec as initial profile in pulseaudio.

In most cases it is either last used codec or codec with higher priority by
defined by remote device.

To detect which A2DP profile was activated by bluez, look at bluez
transport state.
---
 src/modules/bluetooth/module-bluez5-device.c | 66 +++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/src/modules/bluetooth/module-bluez5-device.c 
b/src/modules/bluetooth/module-bluez5-device.c
index 0bfe33d3a..433ac89ef 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -1964,6 +1964,70 @@ static int uuid_to_profile(const char *uuid, 
pa_bluetooth_profile_t *_r) {
 return 0;
 }
 
+static void choose_initial_profile(struct userdata *u) {
+struct pa_bluetooth_transport *transport;
+pa_card_profile *iter_profile;
+pa_card_profile *profile;
+void *state;
+
+pa_log_debug("Looking for A2DP profile which has active bluez transport 
for card %s", u->card->name);
+
+profile = NULL;
+
+/* First try to find the best A2DP profile with transport in playing state 
*/
+PA_HASHMAP_FOREACH(iter_profile, u->card->profiles, state) {
+transport = u->device->transports[*(pa_bluetooth_profile_t 
*)PA_CARD_PROFILE_DATA(iter_profile)];
+
+/* Ignore profiles without active bluez transport in playing state */
+if (!transport || transport->state != 
PA_BLUETOOTH_TRANSPORT_STATE_PLAYING)
+continue;
+
+/* Ignore non-A2DP profiles */
+if (!pa_startswith(iter_profile->name, "a2dp_"))
+continue;
+
+pa_log_debug("%s has active bluez transport in playing state", 
iter_profile->name);
+
+if (!profile || iter_profile->priority > profile->priority)
+profile = iter_profile;
+}
+
+/* Second if there is no profile A2DP profile with transport in playing 
state, look at active transports */
+if (!profile) {
+PA_HASHMAP_FOREACH(iter_profile, u->card->profiles, state) {
+transport = u->device->transports[*(pa_bluetooth_profile_t 
*)PA_CARD_PROFILE_DATA(iter_profile)];
+
+/* Ignore profiles without active bluez transport */
+if (!transport || transport->state == 
PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED)
+continue;
+
+/* Ignore non-A2DP profiles */
+if (!pa_startswith(iter_profile->name, "a2dp_"))
+continue;
+
+pa_log_debug("%s has active bluez transport", iter_profile->name);
+
+if (!profile || iter_profile->priority > profile->priority)
+profile = iter_profile;
+}
+}
+
+/* When there is no active A2DP bluez transport, fallback to core 
pulseaudio function for choosing initial profile */
+if (!profile) {
+pa_log_debug("No A2DP profile with bluez active transport was found 
for card %s", u->card->name);
+pa_card_choose_initial_profile(u->card);
+return;
+}
+
+/* Do same job as pa_card_choose_initial_profile() */
+pa_log_info("Setting initial A2DP profile '%s' for card %s", 
profile->name, u->card->name);
+u->card->active_profile = profile;
+u->card->save_profile = false;
+
+/* Let policy modules override the default. */
+
pa_hook_fire(>card->core->hooks[PA_CORE_HOOK_CARD_CHOOSE_INITIAL_PROFILE], 
u->card);
+}
+
 /* Run from main thread */
 static int add_card(struct userdata *u) {
 const pa_bluetooth_device *d;
@@ -2034,7 +2098,7 @@ static int add_card(struct userdata *u) {
 
 u->card->userdata = u;
 u->card->set_profile = set_profile_cb;
-pa_card_choose_initial_profile(u->card);
+choose_initial_profile(u);
 pa_card_put(u->card);
 
 p = PA_CARD_PROFILE_DATA(u->card->active_profile);
-- 
2.11.0

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

[pulseaudio-discuss] [PATCH v12 03/13] bluetooth: Fix usage of RTP structures in SBC codec

[Pali Rohár]

Rename struct rtp_payload to rtp_sbc_payload as it is specific for SBC
codec payload.

Add proper checks for endianity in rtp.h header and use uint8_t type
where appropriated.

Field frame_count is only 4 bit number, so add checks to prevent overflow.

And because is_fragmented field is not parsed by decoder there is no
support for decoding fragmented SBC frames. So throw an error in this case.
---
 src/modules/bluetooth/a2dp-codec-sbc.c | 35 ++--
 src/modules/bluetooth/rtp.h| 58 +++---
 2 files changed, 58 insertions(+), 35 deletions(-)

diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c 
b/src/modules/bluetooth/a2dp-codec-sbc.c
index f57c7b01a..89c647fbe 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -485,9 +485,13 @@ static int reset(void *codec_info) {
 
 static size_t get_block_size(void *codec_info, size_t link_mtu) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
-size_t rtp_size = sizeof(struct rtp_header) + sizeof(struct rtp_payload);
+size_t rtp_size = sizeof(struct rtp_header) + sizeof(struct 
rtp_sbc_payload);
 size_t frame_count = (link_mtu - rtp_size) / sbc_info->frame_length;
 
+/* frame_count is only 4 bit number */
+if (frame_count > 15)
+frame_count = 15;
+
 return frame_count * sbc_info->codesize;
 }
 
@@ -514,14 +518,14 @@ static size_t reduce_encoder_bitrate(void *codec_info, 
size_t write_link_mtu) {
 static size_t encode_buffer(void *codec_info, uint32_t timestamp, const 
uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t 
output_size, size_t *processed) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
 struct rtp_header *header;
-struct rtp_payload *payload;
+struct rtp_sbc_payload *payload;
 uint8_t *d;
 const uint8_t *p;
 size_t to_write, to_encode;
-unsigned frame_count;
+uint8_t frame_count;
 
 header = (struct rtp_header*) output_buffer;
-payload = (struct rtp_payload*) (output_buffer + sizeof(*header));
+payload = (struct rtp_sbc_payload*) (output_buffer + sizeof(*header));
 
 frame_count = 0;
 
@@ -531,7 +535,8 @@ static size_t encode_buffer(void *codec_info, uint32_t 
timestamp, const uint8_t
 d = output_buffer + sizeof(*header) + sizeof(*payload);
 to_write = output_size - sizeof(*header) - sizeof(*payload);
 
-while (PA_LIKELY(to_encode > 0 && to_write > 0)) {
+/* frame_count is only 4 bit number */
+while (PA_LIKELY(to_encode > 0 && to_write > 0 && frame_count < 15)) {
 ssize_t written;
 ssize_t encoded;
 
@@ -575,7 +580,7 @@ static size_t encode_buffer(void *codec_info, uint32_t 
timestamp, const uint8_t
 }
 
 /* write it to the fifo */
-memset(output_buffer, 0, sizeof(*header) + sizeof(*payload));
+pa_memzero(output_buffer, sizeof(*header) + sizeof(*payload));
 header->v = 2;
 
 /* A2DP spec: "A payload type in the RTP dynamic range shall be chosen".
@@ -596,13 +601,23 @@ static size_t decode_buffer(void *codec_info, const 
uint8_t *input_buffer, size_
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
 
 struct rtp_header *header;
-struct rtp_payload *payload;
+struct rtp_sbc_payload *payload;
 const uint8_t *p;
 uint8_t *d;
 size_t to_write, to_decode;
+uint8_t frame_count;
 
 header = (struct rtp_header *) input_buffer;
-payload = (struct rtp_payload*) (input_buffer + sizeof(*header));
+payload = (struct rtp_sbc_payload*) (input_buffer + sizeof(*header));
+
+frame_count = payload->frame_count;
+
+/* TODO: Add support for decoding fragmented SBC frames */
+if (payload->is_fragmented) {
+pa_log_error("Unsupported fragmented SBC frame");
+*processed = 0;
+return 0;
+}
 
 p = input_buffer + sizeof(*header) + sizeof(*payload);
 to_decode = input_size - sizeof(*header) - sizeof(*payload);
@@ -610,7 +625,7 @@ static size_t decode_buffer(void *codec_info, const uint8_t 
*input_buffer, size_
 d = output_buffer;
 to_write = output_size;
 
-while (PA_LIKELY(to_decode > 0 && to_write > 0)) {
+while (PA_LIKELY(to_decode > 0 && to_write > 0 && frame_count > 0)) {
 size_t written;
 ssize_t decoded;
 
@@ -638,6 +653,8 @@ static size_t decode_buffer(void *codec_info, const uint8_t 
*input_buffer, size_
 
 d += written;
 to_write -= written;
+
+frame_count--;
 }
 
 *processed = p - input_buffer;
diff --git a/src/modules/bluetooth/rtp.h b/src/modules/bluetooth/rtp.h
index 20694c1e1..813d9e390 100644
--- a/src/modules/bluetooth/rtp.h
+++ b/src/modules/bluetooth/rtp.h
@@ -3,6 +3,7 @@
  *  BlueZ - Bluetooth protocol stack for Linux
  *
  *  Copyright (C) 2004-2010  Marcel Holtmann 
+ *  Copyright (C) 2019   Pali Rohár 
  *
  *
  *  This library is free software; you can redistribute it and/or
@@ -19,16 +20,20 @@
  *  

[pulseaudio-discuss] [PATCH v12 01/13] bluetooth: Fix usage of MTU, buffer sizes and return values of encode/decode methods

[Pali Rohár]

Add explanation why minimal bitpool value is used in SBC codec as initial
bitpool value for A2DP source.

Set buffer size for reading/writing from/to A2DP socket to exact link MTU
value. This would ensure that A2DP codec does not produce larger packet as
maximal possible size which can be sent.

Because A2DP socket is of SOCK_SEQPACKET type, it is guaranteed that
we do not read two packets via one read/recvmsg call.

Properly check for all return values of encode/encode methods of A2DP codec
functions. They may fail at different levels. Also encode or decode API
method may return zero length buffer (e.g. because of algorithmic delay of
codec), so do not fail in this case.
---
 src/modules/bluetooth/a2dp-codec-api.h   |  6 ++-
 src/modules/bluetooth/a2dp-codec-sbc.c   | 30 +++
 src/modules/bluetooth/module-bluez5-device.c | 55 +---
 3 files changed, 60 insertions(+), 31 deletions(-)

diff --git a/src/modules/bluetooth/a2dp-codec-api.h 
b/src/modules/bluetooth/a2dp-codec-api.h
index 55bb9ff70..881cc659b 100644
--- a/src/modules/bluetooth/a2dp-codec-api.h
+++ b/src/modules/bluetooth/a2dp-codec-api.h
@@ -72,9 +72,11 @@ typedef struct pa_a2dp_codec {
 /* Reset internal state of codec info data in codec_info */
 void (*reset)(void *codec_info);
 
-/* Get read block size for codec */
+/* Get read block size for codec, it is minimal size of buffer
+ * needed to decode read_link_mtu bytes of encoded data */
 size_t (*get_read_block_size)(void *codec_info, size_t read_link_mtu);
-/* Get write block size for codec */
+/* Get write block size for codec, it is maximal size of buffer
+ * which can produce at most write_link_mtu bytes of encoded data */
 size_t (*get_write_block_size)(void *codec_info, size_t write_link_mtu);
 
 /* Reduce encoder bitrate for codec, returns new write block size or zero
diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c 
b/src/modules/bluetooth/a2dp-codec-sbc.c
index e4c1dff01..e2db91b63 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -426,7 +426,11 @@ static void *init(bool for_encoding, bool for_backchannel, 
const uint8_t *config
 sbc_info->min_bitpool = config->min_bitpool;
 sbc_info->max_bitpool = config->max_bitpool;
 
-/* Set minimum bitpool for source to get the maximum possible block_size */
+/* Set minimum bitpool for source to get the maximum possible block_size
+ * in get_block_size() function. This block_size is length of buffer used
+ * for decoded audio data and so is inversely proportional to frame length
+ * which depends on bitpool value. Bitpool is controlled by other side from
+ * range [min_bitpool, max_bitpool]. */
 sbc_info->initial_bitpool = for_encoding ? sbc_info->max_bitpool : 
sbc_info->min_bitpool;
 
 set_params(sbc_info);
@@ -480,9 +484,10 @@ static void reset(void *codec_info) {
 
 static size_t get_block_size(void *codec_info, size_t link_mtu) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
+size_t rtp_size = sizeof(struct rtp_header) + sizeof(struct rtp_payload);
+size_t frame_count = (link_mtu - rtp_size) / sbc_info->frame_length;
 
-return (link_mtu - sizeof(struct rtp_header) - sizeof(struct rtp_payload))
-   / sbc_info->frame_length * sbc_info->codesize;
+return frame_count * sbc_info->codesize;
 }
 
 static size_t reduce_encoder_bitrate(void *codec_info, size_t write_link_mtu) {
@@ -536,8 +541,12 @@ static size_t encode_buffer(void *codec_info, uint32_t 
timestamp, const uint8_t
 
 if (PA_UNLIKELY(encoded <= 0)) {
 pa_log_error("SBC encoding error (%li)", (long) encoded);
-*processed = p - input_buffer;
-return 0;
+break;
+}
+
+if (PA_UNLIKELY(written < 0)) {
+pa_log_error("SBC encoding error (%li)", (long) written);
+break;
 }
 
 pa_assert_fp((size_t) encoded <= to_encode);
@@ -559,6 +568,11 @@ static size_t encode_buffer(void *codec_info, uint32_t 
timestamp, const uint8_t
 pa_log_debug("Using SBC codec implementation: %s", 
pa_strnull(sbc_get_implementation_info(_info->sbc)));
 } PA_ONCE_END;
 
+if (PA_UNLIKELY(frame_count == 0)) {
+*processed = 0;
+return 0;
+}
+
 /* write it to the fifo */
 memset(output_buffer, 0, sizeof(*header) + sizeof(*payload));
 header->v = 2;
@@ -595,7 +609,7 @@ static size_t decode_buffer(void *codec_info, const uint8_t 
*input_buffer, size_
 d = output_buffer;
 to_write = output_size;
 
-while (PA_LIKELY(to_decode > 0)) {
+while (PA_LIKELY(to_decode > 0 && to_write > 0)) {
 size_t written;
 ssize_t decoded;
 
@@ -606,8 +620,7 @@ static size_t decode_buffer(void *codec_info, const uint8_t 
*input_buffer, size_
 
 if (PA_UNLIKELY(decoded <= 0)) {
 pa_log_error("SBC decoding error (%li)", (long) 

[pulseaudio-discuss] [PATCH v12 11/13] bluetooth: policy: Reflect a2dp profile names

[Pali Rohár]

In next patches, codec name is appended end the end of a2dp profile names.
---
 src/modules/bluetooth/module-bluetooth-policy.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/modules/bluetooth/module-bluetooth-policy.c 
b/src/modules/bluetooth/module-bluetooth-policy.c
index 0a6d59d28..04313aa84 100644
--- a/src/modules/bluetooth/module-bluetooth-policy.c
+++ b/src/modules/bluetooth/module-bluetooth-policy.c
@@ -85,7 +85,7 @@ static pa_hook_result_t source_put_hook_callback(pa_core *c, 
pa_source *source,
 if (!s)
 return PA_HOOK_OK;
 
-if (u->enable_a2dp_source && pa_streq(s, "a2dp_source"))
+if (u->enable_a2dp_source && pa_startswith(s, "a2dp_source"))
 role = "music";
 else if (u->enable_ag && pa_streq(s, "headset_audio_gateway"))
 role = "phone";
@@ -154,7 +154,7 @@ static void card_set_profile(struct userdata *u, pa_card 
*card, bool revert_to_a
 
 /* Check for correct profile based on revert_to_a2dp */
 if (revert_to_a2dp) {
-if (!pa_streq(profile->name, "a2dp_sink"))
+if (!pa_startswith(profile->name, "a2dp_sink"))
 continue;
 } else {
 if (!pa_streq(profile->name, "headset_head_unit"))
@@ -196,11 +196,11 @@ static void switch_profile(pa_card *card, bool 
revert_to_a2dp, void *userdata) {
 return;
 
 /* Skip card if already has active a2dp profile */
-if (pa_streq(card->active_profile->name, "a2dp_sink"))
+if (pa_startswith(card->active_profile->name, "a2dp_sink"))
 return;
 } else {
 /* Skip card if does not have active a2dp profile */
-if (!pa_streq(card->active_profile->name, "a2dp_sink"))
+if (!pa_startswith(card->active_profile->name, "a2dp_sink"))
 return;
 
 /* Skip card if already has active hsp profile */
@@ -307,7 +307,7 @@ static pa_hook_result_t 
card_init_profile_hook_callback(pa_core *c, pa_card *car
 
 /* Ignore card if has already set other initial profile than a2dp */
 if (card->active_profile &&
-!pa_streq(card->active_profile->name, "a2dp_sink"))
+!pa_startswith(card->active_profile->name, "a2dp_sink"))
 return PA_HOOK_OK;
 
 /* Set initial profile to hsp */
@@ -359,7 +359,7 @@ static pa_hook_result_t 
profile_available_hook_callback(pa_core *c, pa_card_prof
 return PA_HOOK_OK;
 
 /* Do not automatically switch profiles for headsets, just in case */
-if (pa_streq(profile->name, "a2dp_sink") || pa_streq(profile->name, 
"headset_head_unit"))
+if (pa_startswith(profile->name, "a2dp_sink") || pa_streq(profile->name, 
"headset_head_unit"))
 return PA_HOOK_OK;
 
 is_active_profile = card->active_profile == profile;
-- 
2.11.0

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

[pulseaudio-discuss] [PATCH v12 04/13] bluetooth: Implement reading SO_TIMESTAMP for A2DP source

[Pali Rohár]

---
 src/modules/bluetooth/module-bluez5-device.c | 33 ++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/src/modules/bluetooth/module-bluez5-device.c 
b/src/modules/bluetooth/module-bluez5-device.c
index 0b63cd44a..efb092089 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -544,13 +544,29 @@ static int a2dp_process_push(struct userdata *u) {
 a2dp_prepare_decoder_buffer(u);
 
 for (;;) {
+uint8_t aux[1024];
+struct iovec iov;
+struct cmsghdr *cm;
+struct msghdr m;
 bool found_tstamp = false;
 pa_usec_t tstamp;
 uint8_t *ptr;
 ssize_t l;
 size_t processed;
 
-l = pa_read(u->stream_fd, u->decoder_buffer, u->decoder_buffer_size, 
>stream_write_type);
+pa_zero(m);
+pa_zero(aux);
+pa_zero(iov);
+
+m.msg_iov = 
+m.msg_iovlen = 1;
+m.msg_control = aux;
+m.msg_controllen = sizeof(aux);
+
+iov.iov_base = u->decoder_buffer;
+iov.iov_len = u->decoder_buffer_size;
+
+l = recvmsg(u->stream_fd, , 0);
 
 if (l <= 0) {
 
@@ -570,8 +586,21 @@ static int a2dp_process_push(struct userdata *u) {
 pa_assert((size_t) l <= u->decoder_buffer_size);
 
 /* TODO: get timestamp from rtp */
+
+for (cm = CMSG_FIRSTHDR(); cm; cm = CMSG_NXTHDR(, cm)) {
+if (cm->cmsg_level == SOL_SOCKET && cm->cmsg_type == SO_TIMESTAMP) 
{
+struct timeval *tv = (struct timeval*) CMSG_DATA(cm);
+pa_rtclock_from_wallclock(tv);
+tstamp = pa_timeval_load(tv);
+found_tstamp = true;
+break;
+}
+}
+
 if (!found_tstamp) {
-/* pa_log_warn("Couldn't find SO_TIMESTAMP data in auxiliary 
recvmsg() data!"); */
+PA_ONCE_BEGIN {
+pa_log_warn("Couldn't find SO_TIMESTAMP data in auxiliary 
recvmsg() data!");
+} PA_ONCE_END;
 tstamp = pa_rtclock_now();
 }
 
-- 
2.11.0

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

[pulseaudio-discuss] [PATCH v12 09/13] bluetooth: Add A2DP FastStream codec support

[Pali Rohár]

This patch provides support for FastStream codec in bluetooth A2DP profile.
FastStream codec is bi-directional, which means that support both music
playback and microphone voice at the same time.

FastStream codec is just SBC codec with fixed parameters. For playback are
used following parameters: 48.0kHz or 44.1kHz, Blocks 16, Sub-bands 8,
Joint Stereo, Loudness, Bitpool = 29 (data rate = 212kbps, packet size =
(71+1)*3 <= DM5 = 220, with 3 SBC frames). SBC frame size is 71 bytes, but
FastStream is zero-padded to the even size (72). For microphone are used
following SBC parameters: 16kHz, Mono, Blocks 16, Sub-bands 8, Loudness,
Bitpool = 32 (data rate = 72kbps, packet size = 72*3 <= DM5 = 220, with
3 SBC frames).

So FastStream codec is equivalent to SBC in Low Quality settings. But the
main benefit of FastStream codec is support for microphone voice channel
for audio calls. Compared to bluetooth HSP profile (with CVSD codec), it
provides better audio quality for both playback and recording.
---
 src/Makefile.am   |   2 +
 src/modules/bluetooth/a2dp-codec-faststream.c | 454 ++
 src/modules/bluetooth/a2dp-codec-util.c   |   2 +
 src/modules/bluetooth/meson.build |   1 +
 4 files changed, 459 insertions(+)
 create mode 100644 src/modules/bluetooth/a2dp-codec-faststream.c

diff --git a/src/Makefile.am b/src/Makefile.am
index 268df2a80..6a8331451 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -2161,6 +2161,8 @@ libbluez5_util_la_SOURCES += 
modules/bluetooth/a2dp-codec-sbc.c
 libbluez5_util_la_LIBADD += $(SBC_LIBS)
 libbluez5_util_la_CFLAGS += $(SBC_CFLAGS)
 
+libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-faststream.c
+
 if HAVE_OPENAPTX
 libbluez5_util_la_SOURCES += modules/bluetooth/a2dp-codec-aptx.c
 libbluez5_util_la_CPPFLAGS += $(OPENAPTX_CPPFLAGS)
diff --git a/src/modules/bluetooth/a2dp-codec-faststream.c 
b/src/modules/bluetooth/a2dp-codec-faststream.c
new file mode 100644
index 0..0ce28c322
--- /dev/null
+++ b/src/modules/bluetooth/a2dp-codec-faststream.c
@@ -0,0 +1,454 @@
+/***
+  This file is part of PulseAudio.
+
+  Copyright 2018-2019 Pali Rohár 
+
+  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, see .
+***/
+
+#ifdef HAVE_CONFIG_H
+#include 
+#endif
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include "a2dp-codecs.h"
+#include "a2dp-codec-api.h"
+
+struct faststream_info {
+sbc_t sbc;   /* Codec data */
+size_t codesize, frame_length;   /* SBC Codesize, frame_length. We 
simply cache those values here */
+bool is_backchannel;
+uint8_t frequency;
+};
+
+static bool can_accept_capabilities(const uint8_t *capabilities_buffer, 
uint8_t capabilities_size, bool for_encoding) {
+const a2dp_faststream_t *capabilities = (const a2dp_faststream_t *) 
capabilities_buffer;
+
+if (A2DP_GET_VENDOR_ID(capabilities->info) != FASTSTREAM_VENDOR_ID || 
A2DP_GET_CODEC_ID(capabilities->info) != FASTSTREAM_CODEC_ID)
+return false;
+
+if (!(capabilities->direction & FASTSTREAM_DIRECTION_SINK) || 
!(capabilities->direction & FASTSTREAM_DIRECTION_SOURCE))
+return false;
+
+if (!(capabilities->sink_frequency & (FASTSTREAM_SINK_SAMPLING_FREQ_44100 
| FASTSTREAM_SINK_SAMPLING_FREQ_48000)))
+return false;
+
+if (!(capabilities->source_frequency & 
FASTSTREAM_SOURCE_SAMPLING_FREQ_16000))
+return false;
+
+return true;
+}
+
+static const char *choose_remote_endpoint(const pa_hashmap 
*capabilities_hashmap, const pa_sample_spec *default_sample_spec, bool 
for_encoding) {
+const pa_a2dp_codec_capabilities *a2dp_capabilities;
+const char *key;
+void *state;
+
+/* There is no preference, just choose random valid entry */
+PA_HASHMAP_FOREACH_KV(key, a2dp_capabilities, capabilities_hashmap, state) 
{
+if (can_accept_capabilities(a2dp_capabilities->buffer, 
a2dp_capabilities->size, for_encoding))
+return key;
+}
+
+return NULL;
+}
+
+static uint8_t fill_capabilities(uint8_t 
capabilities_buffer[MAX_A2DP_CAPS_SIZE]) {
+a2dp_faststream_t *capabilities = (a2dp_faststream_t *) 
capabilities_buffer;
+
+pa_zero(*capabilities);
+
+capabilities->info = A2DP_SET_VENDOR_ID_CODEC_ID(FASTSTREAM_VENDOR_ID, 
FASTSTREAM_CODEC_ID);
+capabilities->direction = 

[pulseaudio-discuss] [PATCH v12 12/13] bluetooth: Implement A2DP codec switching and backchannel support

[Pali Rohár]

Some A2DP codecs (like FastStream or aptX Low Latency) are bi-directional
and can be used for both music playback and audio call. This patch
implements usage of backchannel if A2DP codec provided by pulseaudio API
supports it.

A2DP codec switching needs new version of bluez as older version does not
provide needed DBus API.

Pulseaudio use for each A2DP codec separate pulseaudio profile, therefore
codec switching is implemented via changing pulseaudio profile and
currently used A2DP codec is visible in pulseaudio profile.

Getting list of supported codecs by remote device is supported only by new
version of bluez daemon.

If old version is detected then profile is created for every codec
supported by pulseaudio (therefore also codecs unavailable by remote
endpoint). Old version of bluez daemon does not support profile switching,
so unavailable profile codecs are harmless.
---
 src/modules/bluetooth/bluez5-util.c| 476 +++--
 src/modules/bluetooth/bluez5-util.h|  39 +-
 src/modules/bluetooth/module-bluez5-device.c   | 417 +++---
 src/modules/bluetooth/module-bluez5-discover.c |   3 +-
 4 files changed, 770 insertions(+), 165 deletions(-)

diff --git a/src/modules/bluetooth/bluez5-util.c 
b/src/modules/bluetooth/bluez5-util.c
index 3c5a000c0..4e4ab0cb8 100644
--- a/src/modules/bluetooth/bluez5-util.c
+++ b/src/modules/bluetooth/bluez5-util.c
@@ -171,11 +171,13 @@ static const char 
*transport_state_to_string(pa_bluetooth_transport_state_t stat
 }
 
 static bool device_supports_profile(pa_bluetooth_device *device, 
pa_bluetooth_profile_t profile) {
+const pa_a2dp_codec_capabilities *a2dp_codec_capabilities;
+const pa_a2dp_codec *a2dp_codec;
+bool is_a2dp_sink;
+pa_hashmap *endpoints;
+void *state;
+
 switch (profile) {
-case PA_BLUETOOTH_PROFILE_A2DP_SINK:
-return !!pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_A2DP_SINK);
-case PA_BLUETOOTH_PROFILE_A2DP_SOURCE:
-return !!pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_A2DP_SOURCE);
 case PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT:
 return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HSP_HS)
 || !!pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_HSP_HS_ALT)
@@ -184,10 +186,33 @@ static bool device_supports_profile(pa_bluetooth_device 
*device, pa_bluetooth_pr
 return !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HSP_AG)
 || !!pa_hashmap_get(device->uuids, PA_BLUETOOTH_UUID_HFP_AG);
 case PA_BLUETOOTH_PROFILE_OFF:
-pa_assert_not_reached();
+return true;
+default:
+break;
+}
+
+a2dp_codec = pa_bluetooth_profile_to_a2dp_codec(profile);
+is_a2dp_sink = pa_bluetooth_profile_is_a2dp_sink(profile);
+
+if (is_a2dp_sink && !pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_A2DP_SINK))
+return false;
+else if (!is_a2dp_sink && !pa_hashmap_get(device->uuids, 
PA_BLUETOOTH_UUID_A2DP_SOURCE))
+return false;
+
+if (is_a2dp_sink)
+endpoints = pa_hashmap_get(device->a2dp_sink_endpoints, 
_codec->id);
+else
+endpoints = pa_hashmap_get(device->a2dp_source_endpoints, 
_codec->id);
+
+if (!endpoints)
+return false;
+
+PA_HASHMAP_FOREACH(a2dp_codec_capabilities, endpoints, state) {
+if 
(a2dp_codec->can_accept_capabilities(a2dp_codec_capabilities->buffer, 
a2dp_codec_capabilities->size, is_a2dp_sink))
+return true;
 }
 
-pa_assert_not_reached();
+return false;
 }
 
 static bool device_is_profile_connected(pa_bluetooth_device *device, 
pa_bluetooth_profile_t profile) {
@@ -199,9 +224,11 @@ static bool 
device_is_profile_connected(pa_bluetooth_device *device, pa_bluetoot
 
 static unsigned device_count_disconnected_profiles(pa_bluetooth_device 
*device) {
 pa_bluetooth_profile_t profile;
+unsigned bluetooth_profile_count;
 unsigned count = 0;
 
-for (profile = 0; profile < PA_BLUETOOTH_PROFILE_COUNT; profile++) {
+bluetooth_profile_count = pa_bluetooth_profile_count();
+for (profile = 0; profile < bluetooth_profile_count; profile++) {
 if (!device_supports_profile(device, profile))
 continue;
 
@@ -224,6 +251,7 @@ static void wait_for_profiles_cb(pa_mainloop_api *api, 
pa_time_event* event, con
 pa_bluetooth_device *device = userdata;
 pa_strbuf *buf;
 pa_bluetooth_profile_t profile;
+unsigned bluetooth_profile_count;
 bool first = true;
 char *profiles_str;
 
@@ -231,7 +259,8 @@ static void wait_for_profiles_cb(pa_mainloop_api *api, 
pa_time_event* event, con
 
 buf = pa_strbuf_new();
 
-for (profile = 0; profile < PA_BLUETOOTH_PROFILE_COUNT; profile++) {
+bluetooth_profile_count = pa_bluetooth_profile_count();
+for (profile = 0; profile < bluetooth_profile_count; profile++) {
 if (device_is_profile_connected(device, profile))
 

[pulseaudio-discuss] [PATCH v12 13/13] bluetooth: policy: Treat bi-directional A2DP profiles as suitable for VOIP

[Pali Rohár]

Previously module-bluetooth-policy was switching from A2DP to HSP profile
when VOIP application started recording of source. Now it switch to profile
with the highest priority which has both sink and source. In most cases it
is HSP profile, but it can be also bi-directional A2DP profile (e.g.
FastStream codec) as it has better audio quality.
---
 src/modules/bluetooth/module-bluetooth-policy.c | 123 
 1 file changed, 62 insertions(+), 61 deletions(-)

diff --git a/src/modules/bluetooth/module-bluetooth-policy.c 
b/src/modules/bluetooth/module-bluetooth-policy.c
index 04313aa84..9652a91fe 100644
--- a/src/modules/bluetooth/module-bluetooth-policy.c
+++ b/src/modules/bluetooth/module-bluetooth-policy.c
@@ -59,7 +59,12 @@ struct userdata {
 pa_hook_slot *card_init_profile_slot;
 pa_hook_slot *card_unlink_slot;
 pa_hook_slot *profile_available_changed_slot;
-pa_hashmap *will_need_revert_card_map;
+pa_hashmap *profile_switch_map;
+};
+
+struct profile_switch {
+const char *from_profile;
+const char *to_profile;
 };
 
 /* When a source is created, loopback it to default sink */
@@ -142,43 +147,57 @@ static pa_hook_result_t sink_put_hook_callback(pa_core 
*c, pa_sink *sink, void *
 return PA_HOOK_OK;
 }
 
-static void card_set_profile(struct userdata *u, pa_card *card, bool 
revert_to_a2dp)
-{
+static void card_set_profile(struct userdata *u, pa_card *card, const char 
*revert_to_profile_name) {
+pa_card_profile *iter_profile;
 pa_card_profile *profile;
+struct profile_switch *ps;
+char *old_profile_name;
 void *state;
 
-/* Find available profile and activate it */
-PA_HASHMAP_FOREACH(profile, card->profiles, state) {
-if (profile->available == PA_AVAILABLE_NO)
-continue;
-
-/* Check for correct profile based on revert_to_a2dp */
-if (revert_to_a2dp) {
-if (!pa_startswith(profile->name, "a2dp_sink"))
+if (revert_to_profile_name) {
+profile = pa_hashmap_get(card->profiles, revert_to_profile_name);
+} else {
+/* Find highest priority profile with both sink and source */
+profile = NULL;
+PA_HASHMAP_FOREACH(iter_profile, card->profiles, state) {
+if (iter_profile->available == PA_AVAILABLE_NO)
 continue;
-} else {
-if (!pa_streq(profile->name, "headset_head_unit"))
+if (iter_profile->n_sources == 0 || iter_profile->n_sinks == 0)
 continue;
+if (!profile || profile->priority < iter_profile->priority)
+profile = iter_profile;
 }
+}
 
-pa_log_debug("Setting card '%s' to profile '%s'", card->name, 
profile->name);
+if (!profile) {
+pa_log_warn("Could not find any suitable profile for card '%s'", 
card->name);
+return;
+}
 
-if (pa_card_set_profile(card, profile, false) != 0) {
-pa_log_warn("Could not set profile '%s'", profile->name);
-continue;
-}
+old_profile_name = card->active_profile->name;
+
+pa_log_debug("Setting card '%s' from profile '%s' to profile '%s'", 
card->name, old_profile_name, profile->name);
 
-/* When we are not in revert_to_a2dp phase flag this card for 
will_need_revert */
-if (!revert_to_a2dp)
-pa_hashmap_put(u->will_need_revert_card_map, card, 
PA_INT_TO_PTR(1));
+if (pa_card_set_profile(card, profile, false) != 0) {
+pa_log_warn("Could not set profile '%s'", profile->name);
+return;
+}
 
-break;
+/* When not reverting, store data for future reverting */
+if (!revert_to_profile_name) {
+ps = pa_xnew0(struct profile_switch, 1);
+ps->from_profile = old_profile_name;
+ps->to_profile = profile->name;
+pa_hashmap_put(u->profile_switch_map, card, ps);
 }
 }
 
 /* Switch profile for one card */
-static void switch_profile(pa_card *card, bool revert_to_a2dp, void *userdata) 
{
+static void switch_profile(pa_card *card, bool revert, void *userdata) {
 struct userdata *u = userdata;
+struct profile_switch *ps;
+const char *from_profile;
+const char *to_profile;
 const char *s;
 
 /* Only consider bluetooth cards */
@@ -186,29 +205,25 @@ static void switch_profile(pa_card *card, bool 
revert_to_a2dp, void *userdata) {
 if (!s || !pa_streq(s, "bluetooth"))
 return;
 
-if (revert_to_a2dp) {
-/* In revert_to_a2dp phase only consider cards with will_need_revert 
flag and remove it */
-if (!pa_hashmap_remove(u->will_need_revert_card_map, card))
+if (revert) {
+/* In revert phase only consider cards which switched profile */
+if (!(ps = pa_hashmap_remove(u->profile_switch_map, card)))
 return;
 
-/* Skip card if does not have active hsp profile */
-if (!pa_streq(card->active_profile->name, "headset_head_unit"))
-return;
+

[pulseaudio-discuss] [PATCH v12 00/13] Bluetooth A2DP codecs

[Pali Rohár]

Changes in v12:
* Renamed SBC UHQ to SBC XQ to match naming convention from
  
http://soundexpert.org/articles/-/blogs/audio-quality-of-sbc-xq-bluetooth-audio-codec
* Throw error when PA receive fragmented SBC frame
* Log "Couldn't find SO_TIMESTAMP" warning message only once
* Update comment for SBC bitpool selection
* Add more checks for return values from libsbc
* Propagate error value from sbc_reinit() to module-bluez5-device.c
* Add checks for SBC frame count
* Completely rework/fix MTU, buffer sizes and return values of encode/decode 
methods

Pali Rohár (13):
  bluetooth: Fix usage of MTU, buffer sizes and return values of
encode/decode methods
  bluetooth: Change A2DP codec API of reset() method to indicate failure
  bluetooth: Fix usage of RTP structures in SBC codec
  bluetooth: Implement reading SO_TIMESTAMP for A2DP source
  bluetooth: Print SO_TIMESTAMP warning for SCO source only once
  bluetooth: Parse remote timestamp from A2DP RTP packets when available
  bluetooth: Set initial A2DP profile which bluez already activated
  bluetooth: Add A2DP aptX and aptX HD codecs support
  bluetooth: Add A2DP FastStream codec support
  bluetooth: Add more variants of SBC codec
  bluetooth: policy: Reflect a2dp profile names
  bluetooth: Implement A2DP codec switching and backchannel support
  bluetooth: policy: Treat bi-directional A2DP profiles as suitable for
VOIP

 configure.ac|  36 ++
 src/Makefile.am |   8 +
 src/modules/bluetooth/a2dp-codec-api.h  |  14 +-
 src/modules/bluetooth/a2dp-codec-aptx.c | 448 +
 src/modules/bluetooth/a2dp-codec-faststream.c   | 454 +
 src/modules/bluetooth/a2dp-codec-sbc.c  | 806 +++-
 src/modules/bluetooth/a2dp-codec-util.c |  26 +-
 src/modules/bluetooth/bluez5-util.c | 476 +-
 src/modules/bluetooth/bluez5-util.h |  39 +-
 src/modules/bluetooth/meson.build   |   1 +
 src/modules/bluetooth/module-bluetooth-policy.c | 127 ++--
 src/modules/bluetooth/module-bluez5-device.c| 587 -
 src/modules/bluetooth/module-bluez5-discover.c  |   3 +-
 src/modules/bluetooth/rtp.h |  58 +-
 14 files changed, 2644 insertions(+), 439 deletions(-)
 create mode 100644 src/modules/bluetooth/a2dp-codec-aptx.c
 create mode 100644 src/modules/bluetooth/a2dp-codec-faststream.c

-- 
2.11.0

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

[pulseaudio-discuss] [PATCH v12 06/13] bluetooth: Parse remote timestamp from A2DP RTP packets when available

[Pali Rohár]

Some A2DP codecs (e.g. SBC or aptX-HD) use RTP packets. For sources use
timestamps from RTP packets to calculate read index and therefore remote
timestamp for synchronization.
---
 src/modules/bluetooth/a2dp-codec-api.h   |  4 ++--
 src/modules/bluetooth/a2dp-codec-sbc.c   |  3 ++-
 src/modules/bluetooth/module-bluez5-device.c | 14 +++---
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/modules/bluetooth/a2dp-codec-api.h 
b/src/modules/bluetooth/a2dp-codec-api.h
index bc4844596..d39f8aafa 100644
--- a/src/modules/bluetooth/a2dp-codec-api.h
+++ b/src/modules/bluetooth/a2dp-codec-api.h
@@ -90,8 +90,8 @@ typedef struct pa_a2dp_codec {
 size_t (*encode_buffer)(void *codec_info, uint32_t timestamp, const 
uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t 
output_size, size_t *processed);
 /* Decode input_buffer of input_size to output_buffer of output_size,
  * returns size of filled ouput_buffer and set processed to size of
- * processed input_buffer */
-size_t (*decode_buffer)(void *codec_info, const uint8_t *input_buffer, 
size_t input_size, uint8_t *output_buffer, size_t output_size, size_t 
*processed);
+ * processed input_buffer and set timestamp */
+size_t (*decode_buffer)(void *codec_info, uint32_t *timestamp, const 
uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t 
output_size, size_t *processed);
 } pa_a2dp_codec;
 
 #endif
diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c 
b/src/modules/bluetooth/a2dp-codec-sbc.c
index 89c647fbe..733c1a9ab 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -597,7 +597,7 @@ static size_t encode_buffer(void *codec_info, uint32_t 
timestamp, const uint8_t
 return d - output_buffer;
 }
 
-static size_t decode_buffer(void *codec_info, const uint8_t *input_buffer, 
size_t input_size, uint8_t *output_buffer, size_t output_size, size_t 
*processed) {
+static size_t decode_buffer(void *codec_info, uint32_t *timestamp, const 
uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t 
output_size, size_t *processed) {
 struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
 
 struct rtp_header *header;
@@ -657,6 +657,7 @@ static size_t decode_buffer(void *codec_info, const uint8_t 
*input_buffer, size_
 frame_count--;
 }
 
+*timestamp = ntohl(header->timestamp);
 *processed = p - input_buffer;
 return d - output_buffer;
 }
diff --git a/src/modules/bluetooth/module-bluez5-device.c 
b/src/modules/bluetooth/module-bluez5-device.c
index a97c6c703..0bfe33d3a 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -552,6 +552,7 @@ static int a2dp_process_push(struct userdata *u) {
 struct msghdr m;
 bool found_tstamp = false;
 pa_usec_t tstamp;
+uint32_t timestamp;
 uint8_t *ptr;
 ssize_t l;
 size_t processed;
@@ -587,8 +588,6 @@ static int a2dp_process_push(struct userdata *u) {
 
 pa_assert((size_t) l <= u->decoder_buffer_size);
 
-/* TODO: get timestamp from rtp */
-
 for (cm = CMSG_FIRSTHDR(); cm; cm = CMSG_NXTHDR(, cm)) {
 if (cm->cmsg_level == SOL_SOCKET && cm->cmsg_type == SO_TIMESTAMP) 
{
 struct timeval *tv = (struct timeval*) CMSG_DATA(cm);
@@ -609,7 +608,8 @@ static int a2dp_process_push(struct userdata *u) {
 ptr = pa_memblock_acquire(memchunk.memblock);
 memchunk.length = pa_memblock_get_length(memchunk.memblock);
 
-memchunk.length = u->a2dp_codec->decode_buffer(u->decoder_info, 
u->decoder_buffer, l, ptr, memchunk.length, );
+timestamp = 0; /* Decoder does not have to fill RTP timestamp */
+memchunk.length = u->a2dp_codec->decode_buffer(u->decoder_info, 
, u->decoder_buffer, l, ptr, memchunk.length, );
 
 pa_memblock_release(memchunk.memblock);
 
@@ -619,6 +619,14 @@ static int a2dp_process_push(struct userdata *u) {
 break;
 }
 
+/* Some codecs may provide RTP timestamp, so use it to update 
read_index for calculation of remote tstamp */
+if (timestamp) {
+/* RTP timestamp is only 32bit and may overflow, avoid it by 
calculating high 32bits from the last read_index */
+size_t frame_size = pa_frame_size(>decoder_sample_spec);
+uint64_t timestamp_hi = (u->read_index / frame_size) & 
0xULL;
+u->read_index = (timestamp_hi | timestamp) * frame_size;
+}
+
 u->read_index += (uint64_t) memchunk.length;
 pa_smoother_put(u->read_smoother, tstamp, 
pa_bytes_to_usec(u->read_index, >decoder_sample_spec));
 pa_smoother_resume(u->read_smoother, tstamp, true);
-- 
2.11.0

___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org

Re: [pulseaudio-discuss] [PATCH v2 4/8] core: move sink-inputs conditionally when update default_sink

[Georg Chini]

On 05.07.19 10:57, Georg Chini wrote:

On 05.07.19 09:41, Tanu Kaskinen wrote:

On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:

On 02.07.19 08:43, Tanu Kaskinen wrote:

On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:

On 01.07.19 07:08, Tanu Kaskinen wrote:

On Sun, 2019-06-30 at 13:52 +0200, Georg Chini wrote:

On 17.01.19 07:53, Hui Wang wrote:
When the default sink changes, the streams from the old default 
sink

should be moved to the new default sink, unless the preferred_sink
string is set to the old default sink and the active port of 
the old

default sink is not unavailable


  static pa_hook_result_t 
sink_put_hook_callback(pa_core *c, pa_sink *sink, void* 
userdata) {

-    pa_sink_input *i;
-    uint32_t idx;
-    pa_sink *old_default_sink;
 const char *s;
 struct userdata *u = userdata;
 @@ -88,7 +85,7 @@ static pa_hook_result_t 
sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
  /* No default sink, nothing to move away, just 
set the new default */

 if (!c->default_sink) {
-    pa_core_set_configured_default_sink(c, sink->name);
+    pa_core_set_configured_default_sink(c, sink->name, 
false);

 return PA_HOOK_OK;
 }
 @@ -103,28 +100,8 @@ static pa_hook_result_t 
sink_put_hook_callback(pa_core *c, pa_sink *sink, void*

 return PA_HOOK_OK;
 }
 -    old_default_sink = c->default_sink;
-
 /* Actually do the switch to the new sink */
-    pa_core_set_configured_default_sink(c, sink->name);
-
-    /* Now move all old inputs over */
-    if (pa_idxset_size(old_default_sink->inputs) <= 0) {
-    pa_log_debug("No sink inputs to move away.");
-    return PA_HOOK_OK;
-    }
-
-    PA_IDXSET_FOREACH(i, old_default_sink->inputs, idx) {
-    if (pa_safe_streq(i->sink->name, i->preferred_sink) || 
!PA_SINK_INPUT_IS_LINKED(i->state))

-    continue;
-
-    if (pa_sink_input_move_to(i, sink, false) < 0)
-    pa_log_info("Failed to move sink input %u \"%s\" 
to %s.", i->index,
- pa_strnull(pa_proplist_gets(i->proplist, 
PA_PROP_APPLICATION_NAME)), sink->name);

-    else
-    pa_log_info("Successfully moved sink input %u 
\"%s\" to %s.", i->index,
- pa_strnull(pa_proplist_gets(i->proplist, 
PA_PROP_APPLICATION_NAME)), sink->name);

-    }
+    pa_core_set_configured_default_sink(c, sink->name, false);

I wonder if we could use something like
pa_core_set_temporary_default_sink() here
and have a variable core->temporary_default_sink that has even 
higher

priority
than core->configured_default_sink in the default sink selection
process. The temporary
default sink could be reset when the sink vanishes again or 
replaced

when another new
sink turns up. What module-switch-on-connect does is to temporarily
override the default
sink that the user configured. If we save this in another 
variable we

would not overwrite
the user configured default sink by a sink that is not expected 
to be

present the next
time PA is started. But that would be just nice to have.

I'm against adding that kind of extra complexity without a very good
justification. module-switch-on-connect should become nearly 
obsolete

anyway after this patch set has been merged.

We already talked about this some time ago and you agreed
that this would be useful. Maybe you remember that discussion.

I remember that we discussed my initial attempt at implementing what
this patch set does. I probably should go back to that discussion to
find out why I agreed that we should add a separate "temporary default
sink" variable.

You did not exactly agree to adding a temporary_default_sink
variable. You agreed however (after some discussion) that
what module-switch-on-connect does is a temporary override
of the configured default sink and that there should be a way
to restore the original default sink. (The user may have set
another than the highest priority sink as default, see also
below).

It seems quite simple to add such a variable and replace it
when a new sink turns up or set it to NULL if the current
temporary default sink is removed. This would then make
it really easy to implement the module-switch-on-connect
functionality in the core at some later point.

I'm concerned about the complexity of answering the question "how is
the default sink determined". The fact that "default sink" and
"configured default sink" are different things is already bothersome.


Why do you find this bothersome? I think it is pretty obvious
that the two are not necessarily identical.


Your point about restoring the old configured default sink is
definitely valid, although that problem seems to be not specific to
module-switch-on-connect: if there are 3 sinks, the user can't specify
which of them is the second most preferred sink in case the configured
default sink goes away. Having a priority list of sinks based on manual
configuration history might be a better way to solve this (although 

Re: [pulseaudio-discuss] [PATCH v2 4/8] core: move sink-inputs conditionally when update default_sink

[Georg Chini]

On 05.07.19 09:56, Tanu Kaskinen wrote:

On Tue, 2019-07-02 at 10:25 +0200, Georg Chini wrote:

On 02.07.19 06:49, Tanu Kaskinen wrote:

On Mon, 2019-07-01 at 08:48 +0200, Georg Chini wrote:

On 01.07.19 08:03, Georg Chini wrote:

On 01.07.19 07:08, Tanu Kaskinen wrote:

On Sun, 2019-06-30 at 13:52 +0200, Georg Chini wrote:

On 17.01.19 07:53, Hui Wang wrote:

When the default sink changes, the streams from the old default sink
should be moved to the new default sink, unless the preferred_sink
string is set to the old default sink and the active port of the old
default sink is not unavailable
 +
+void pa_sink_move_streams_to_default_sink(pa_core *core, pa_sink
*old_sink, bool from_user) {
+pa_sink_input *i;
+uint32_t idx;
+bool old_sink_is_unavailable;

Does this not give a warning about using uninitialized variables?

+
+pa_assert(core);
+pa_assert(old_sink);
+
+if (old_sink == core->default_sink)
+return;
+
+if (old_sink->active_port && old_sink->active_port->available
== PA_AVAILABLE_NO)
+old_sink_is_unavailable = true;

This is not sufficient to determine if the old sink is still available.
There are sinks
that do not have ports (null-sink, virtual sinks). I think you will
need
another boolean
argument to the function which indicates availability of the old sink.

The definition of an unavailable sink is that its active port is
unavailable. I don't know what kind of sinks you're thinking about
here, maybe virtual sinks that are on top of an unavailable hardware
sink? There are many places where we check the availability of a sink
this way, and I don't think it makes sense to be different here.

I don't understand. Virtual sinks don't have ports. So checking only
sinks that have an active port excludes all sinks that don't have
ports like the null-sink and virtual sinks. Following your definition
above, those sinks are never available.

You have it reversed: following my definition above, those sinks are
always available.

Right, sorry. (I seem to be reading things wrong quite often
the last few weeks ... I'll do my best to be more thorough in
the future.)

Checking the code, only alsa, bluetooth and raop sinks define ports and
the "many places" you are referring to are compare_sinks() and
compare_sources() in core.c and a check in module-switch-on-connect,
which is used to determine the availability of the default sink.

At least module-stream-restore checks device availability too (although
probably not anymore after this patch set, because module-stream-
restore won't do the routing directly anymore, it will just restore the
preferred_sink variable, which can be done regardless of the sink
availability).

Extending the hardware sink availability to filter sinks probably makes
sense, but I think that's a topic for a separate patch (or patches).

You suggested an extra flag for pa_sink_move_streams_to_default_sink(),
which seems unnecessary to me. The function can in any case figure out
the availability by itself (possibly using a helper function
pa_sink_is_available() that can handle filter sinks too).


I think some additional check is still needed at least for
s->unlink_requested
because when a sink is going to be unlinked and the function is called from
pa_sink_unlink() in patch 7, there may be nothing else that indicates
that the
sink is going to be removed. When I proposed an additional argument, I did
not see that pa_sink_unlink() already sets a flag that can be used.

So I would suggest that the helper function checks on
- link state

I don't expect this to be necessary, although it can't hurt either.


- port availability
- s->unlink_requested

If this is indeed used from pa_sink_unlink() to rescue streams, then
checking this seems appropriate (and can't hurt anyway).


- suspend state (any reason apart from SUSPEND_IDLE)

This doesn't seem appropriate, unless we start avoiding suspended sinks
in the same way we avoid unavailable sinks. That would mean that we
would never use a suspended sink as the default sink. That would
probably make sense actually, so I'm just pointing out that we need to
be consistent: if we avoid suspended sinks in one situation, we should
avoid them in all situations.


Yes, we should do that. Therefore my suggestion is to add a helper
function to this patch set. For consistency we could exclude the
suspend state for the moment, though I would recommend to include
it. Using the helper function outside this patch set in all places where
we check availability should then be the task of another patch series.
By suspended I guess you mean suspended apart from idle, correct?




Using the helper function in other places (and extending it if I forgot
something
in the list above) could then be done in a separate patch. Are you OK
with that?

Yes, I think a helper function can be added, and if it's added, it's
added in a separate patch.


I don't really understand what your suggestion means for the patch set
we are discussing. 

Re: [pulseaudio-discuss] [PATCH v2 4/8] core: move sink-inputs conditionally when update default_sink

[Georg Chini]

On 05.07.19 09:41, Tanu Kaskinen wrote:

On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:

On 02.07.19 08:43, Tanu Kaskinen wrote:

On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:

On 01.07.19 07:08, Tanu Kaskinen wrote:

On Sun, 2019-06-30 at 13:52 +0200, Georg Chini wrote:

On 17.01.19 07:53, Hui Wang wrote:

When the default sink changes, the streams from the old default sink
should be moved to the new default sink, unless the preferred_sink
string is set to the old default sink and the active port of the old
default sink is not unavailable


 
 static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void* userdata) {

-pa_sink_input *i;
-uint32_t idx;
-pa_sink *old_default_sink;
 const char *s;
 struct userdata *u = userdata;
 
@@ -88,7 +85,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
 
 /* No default sink, nothing to move away, just set the new default */

 if (!c->default_sink) {
-pa_core_set_configured_default_sink(c, sink->name);
+pa_core_set_configured_default_sink(c, sink->name, false);
 return PA_HOOK_OK;
 }
 
@@ -103,28 +100,8 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*

 return PA_HOOK_OK;
 }
 
-old_default_sink = c->default_sink;

-
 /* Actually do the switch to the new sink */
-pa_core_set_configured_default_sink(c, sink->name);
-
-/* Now move all old inputs over */
-if (pa_idxset_size(old_default_sink->inputs) <= 0) {
-pa_log_debug("No sink inputs to move away.");
-return PA_HOOK_OK;
-}
-
-PA_IDXSET_FOREACH(i, old_default_sink->inputs, idx) {
-if (pa_safe_streq(i->sink->name, i->preferred_sink) || 
!PA_SINK_INPUT_IS_LINKED(i->state))
-continue;
-
-if (pa_sink_input_move_to(i, sink, false) < 0)
-pa_log_info("Failed to move sink input %u \"%s\" to %s.", i->index,
-pa_strnull(pa_proplist_gets(i->proplist, 
PA_PROP_APPLICATION_NAME)), sink->name);
-else
-pa_log_info("Successfully moved sink input %u \"%s\" to %s.", 
i->index,
-pa_strnull(pa_proplist_gets(i->proplist, 
PA_PROP_APPLICATION_NAME)), sink->name);
-}
+pa_core_set_configured_default_sink(c, sink->name, false);

I wonder if we could use something like
pa_core_set_temporary_default_sink() here
and have a variable core->temporary_default_sink that has even higher
priority
than core->configured_default_sink in the default sink selection
process. The temporary
default sink could be reset when the sink vanishes again or replaced
when another new
sink turns up. What module-switch-on-connect does is to temporarily
override the default
sink that the user configured. If we save this in another variable we
would not overwrite
the user configured default sink by a sink that is not expected to be
present the next
time PA is started. But that would be just nice to have.

I'm against adding that kind of extra complexity without a very good
justification. module-switch-on-connect should become nearly obsolete
anyway after this patch set has been merged.

We already talked about this some time ago and you agreed
that this would be useful. Maybe you remember that discussion.

I remember that we discussed my initial attempt at implementing what
this patch set does. I probably should go back to that discussion to
find out why I agreed that we should add a separate "temporary default
sink" variable.

You did not exactly agree to adding a temporary_default_sink
variable. You agreed however (after some discussion) that
what module-switch-on-connect does is a temporary override
of the configured default sink and that there should be a way
to restore the original default sink. (The user may have set
another than the highest priority sink as default, see also
below).

It seems quite simple to add such a variable and replace it
when a new sink turns up or set it to NULL if the current
temporary default sink is removed. This would then make
it really easy to implement the module-switch-on-connect
functionality in the core at some later point.

I'm concerned about the complexity of answering the question "how is
the default sink determined". The fact that "default sink" and
"configured default sink" are different things is already bothersome.


Why do you find this bothersome? I think it is pretty obvious
that the two are not necessarily identical.


Your point about restoring the old configured default sink is
definitely valid, although that problem seems to be not specific to
module-switch-on-connect: if there are 3 sinks, the user can't specify
which of them is the second most preferred sink in case the configured
default sink goes away. Having a priority list of sinks based on manual
configuration history might be a better way to solve this (although I'm
afraid 

Re: [pulseaudio-discuss] [PATCH v2 4/8] core: move sink-inputs conditionally when update default_sink

[Tanu Kaskinen]

On Tue, 2019-07-02 at 10:25 +0200, Georg Chini wrote:
> On 02.07.19 06:49, Tanu Kaskinen wrote:
> > On Mon, 2019-07-01 at 08:48 +0200, Georg Chini wrote:
> > > On 01.07.19 08:03, Georg Chini wrote:
> > > > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > > > On Sun, 2019-06-30 at 13:52 +0200, Georg Chini wrote:
> > > > > > On 17.01.19 07:53, Hui Wang wrote:
> > > > > > > When the default sink changes, the streams from the old default 
> > > > > > > sink
> > > > > > > should be moved to the new default sink, unless the preferred_sink
> > > > > > > string is set to the old default sink and the active port of the 
> > > > > > > old
> > > > > > > default sink is not unavailable
> > > > > > > +
> > > > > > > +void pa_sink_move_streams_to_default_sink(pa_core *core, pa_sink
> > > > > > > *old_sink, bool from_user) {
> > > > > > > +pa_sink_input *i;
> > > > > > > +uint32_t idx;
> > > > > > > +bool old_sink_is_unavailable;
> > > > > > Does this not give a warning about using uninitialized variables?
> > > > > > > +
> > > > > > > +pa_assert(core);
> > > > > > > +pa_assert(old_sink);
> > > > > > > +
> > > > > > > +if (old_sink == core->default_sink)
> > > > > > > +return;
> > > > > > > +
> > > > > > > +if (old_sink->active_port && old_sink->active_port->available
> > > > > > > == PA_AVAILABLE_NO)
> > > > > > > +old_sink_is_unavailable = true;
> > > > > > This is not sufficient to determine if the old sink is still 
> > > > > > available.
> > > > > > There are sinks
> > > > > > that do not have ports (null-sink, virtual sinks). I think you will
> > > > > > need
> > > > > > another boolean
> > > > > > argument to the function which indicates availability of the old 
> > > > > > sink.
> > > > > The definition of an unavailable sink is that its active port is
> > > > > unavailable. I don't know what kind of sinks you're thinking about
> > > > > here, maybe virtual sinks that are on top of an unavailable hardware
> > > > > sink? There are many places where we check the availability of a sink
> > > > > this way, and I don't think it makes sense to be different here.
> > > > I don't understand. Virtual sinks don't have ports. So checking only
> > > > sinks that have an active port excludes all sinks that don't have
> > > > ports like the null-sink and virtual sinks. Following your definition
> > > > above, those sinks are never available.
> > You have it reversed: following my definition above, those sinks are
> > always available.
> Right, sorry. (I seem to be reading things wrong quite often
> the last few weeks ... I'll do my best to be more thorough in
> the future.)
> > > Checking the code, only alsa, bluetooth and raop sinks define ports and
> > > the "many places" you are referring to are compare_sinks() and
> > > compare_sources() in core.c and a check in module-switch-on-connect,
> > > which is used to determine the availability of the default sink.
> > At least module-stream-restore checks device availability too (although
> > probably not anymore after this patch set, because module-stream-
> > restore won't do the routing directly anymore, it will just restore the
> > preferred_sink variable, which can be done regardless of the sink
> > availability).
> > 
> > Extending the hardware sink availability to filter sinks probably makes
> > sense, but I think that's a topic for a separate patch (or patches).
> > 
> > You suggested an extra flag for pa_sink_move_streams_to_default_sink(),
> > which seems unnecessary to me. The function can in any case figure out
> > the availability by itself (possibly using a helper function
> > pa_sink_is_available() that can handle filter sinks too).
> > 
> I think some additional check is still needed at least for 
> s->unlink_requested
> because when a sink is going to be unlinked and the function is called from
> pa_sink_unlink() in patch 7, there may be nothing else that indicates 
> that the
> sink is going to be removed. When I proposed an additional argument, I did
> not see that pa_sink_unlink() already sets a flag that can be used.
> 
> So I would suggest that the helper function checks on
> - link state

I don't expect this to be necessary, although it can't hurt either.

> - port availability
> - s->unlink_requested

If this is indeed used from pa_sink_unlink() to rescue streams, then
checking this seems appropriate (and can't hurt anyway).

> - suspend state (any reason apart from SUSPEND_IDLE)

This doesn't seem appropriate, unless we start avoiding suspended sinks
in the same way we avoid unavailable sinks. That would mean that we
would never use a suspended sink as the default sink. That would
probably make sense actually, so I'm just pointing out that we need to
be consistent: if we avoid suspended sinks in one situation, we should
avoid them in all situations.

> Using the helper function in other places (and extending it if I forgot 
> something
> in the list above) could then be done in a separate patch. Are you OK 
> 

Re: [pulseaudio-discuss] [PATCH v2 4/8] core: move sink-inputs conditionally when update default_sink

[Tanu Kaskinen]

On Tue, 2019-07-02 at 09:08 +0200, Georg Chini wrote:
> On 02.07.19 08:43, Tanu Kaskinen wrote:
> > On Mon, 2019-07-01 at 08:03 +0200, Georg Chini wrote:
> > > On 01.07.19 07:08, Tanu Kaskinen wrote:
> > > > On Sun, 2019-06-30 at 13:52 +0200, Georg Chini wrote:
> > > > > On 17.01.19 07:53, Hui Wang wrote:
> > > > > > When the default sink changes, the streams from the old default sink
> > > > > > should be moved to the new default sink, unless the preferred_sink
> > > > > > string is set to the old default sink and the active port of the old
> > > > > > default sink is not unavailable
> > > > > > 
> > > > > > Signed-off-by: Hui Wang 
> > > > > > ---
> > > > > > src/modules/dbus/iface-core.c   |  2 +-
> > > > > > src/modules/module-default-device-restore.c |  2 +-
> > > > > > src/modules/module-switch-on-connect.c  | 27 
> > > > > > ++--
> > > > > > src/pulsecore/cli-command.c |  2 +-
> > > > > > src/pulsecore/core.c| 10 --
> > > > > > src/pulsecore/core.h|  4 +--
> > > > > > src/pulsecore/device-port.c |  2 +-
> > > > > > src/pulsecore/protocol-native.c |  2 +-
> > > > > > src/pulsecore/sink.c| 35 
> > > > > > +++--
> > > > > > src/pulsecore/sink.h|  6 
> > > > > > 10 files changed, 54 insertions(+), 38 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/modules/dbus/iface-core.c 
> > > > > > b/src/modules/dbus/iface-core.c
> > > > > > index 5229c0467..9763480d2 100644
> > > > > > --- a/src/modules/dbus/iface-core.c
> > > > > > +++ b/src/modules/dbus/iface-core.c
> > > > > > @@ -721,7 +721,7 @@ static void 
> > > > > > handle_set_fallback_sink(DBusConnection *conn, DBusMessage *msg, DBu
> > > > > > return;
> > > > > > }
> > > > > > 
> > > > > > -pa_core_set_configured_default_sink(c->core, 
> > > > > > pa_dbusiface_device_get_sink(fallback_sink)->name);
> > > > > > +pa_core_set_configured_default_sink(c->core, 
> > > > > > pa_dbusiface_device_get_sink(fallback_sink)->name, true);
> > > > > > 
> > > > > > pa_dbus_send_empty_reply(conn, msg);
> > > > > > }
> > > > > > diff --git a/src/modules/module-default-device-restore.c 
> > > > > > b/src/modules/module-default-device-restore.c
> > > > > > index c4dbad99f..33e74c071 100644
> > > > > > --- a/src/modules/module-default-device-restore.c
> > > > > > +++ b/src/modules/module-default-device-restore.c
> > > > > > @@ -69,7 +69,7 @@ static void load(struct userdata *u) {
> > > > > > pa_log_warn("Invalid sink name: %s", ln);
> > > > > > else {
> > > > > > pa_log_info("Restoring default sink '%s'.", ln);
> > > > > > -pa_core_set_configured_default_sink(u->core, ln);
> > > > > > +pa_core_set_configured_default_sink(u->core, ln, 
> > > > > > false);
> > > > > > }
> > > > > > 
> > > > > > } else if (errno != ENOENT)
> > > > > > diff --git a/src/modules/module-switch-on-connect.c 
> > > > > > b/src/modules/module-switch-on-connect.c
> > > > > > index f0cb29a7c..3ceac8b4f 100644
> > > > > > --- a/src/modules/module-switch-on-connect.c
> > > > > > +++ b/src/modules/module-switch-on-connect.c
> > > > > > @@ -54,9 +54,6 @@ struct userdata {
> > > > > > };
> > > > > > 
> > > > > > static pa_hook_result_t sink_put_hook_callback(pa_core *c, 
> > > > > > pa_sink *sink, void* userdata) {
> > > > > > -pa_sink_input *i;
> > > > > > -uint32_t idx;
> > > > > > -pa_sink *old_default_sink;
> > > > > > const char *s;
> > > > > > struct userdata *u = userdata;
> > > > > > 
> > > > > > @@ -88,7 +85,7 @@ static pa_hook_result_t 
> > > > > > sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
> > > > > > 
> > > > > > /* No default sink, nothing to move away, just set the new 
> > > > > > default */
> > > > > > if (!c->default_sink) {
> > > > > > -pa_core_set_configured_default_sink(c, sink->name);
> > > > > > +pa_core_set_configured_default_sink(c, sink->name, false);
> > > > > > return PA_HOOK_OK;
> > > > > > }
> > > > > > 
> > > > > > @@ -103,28 +100,8 @@ static pa_hook_result_t 
> > > > > > sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
> > > > > > return PA_HOOK_OK;
> > > > > > }
> > > > > > 
> > > > > > -old_default_sink = c->default_sink;
> > > > > > -
> > > > > > /* Actually do the switch to the new sink */
> > > > > > -pa_core_set_configured_default_sink(c, sink->name);
> > > > > > -
> > > > > > -/* Now move all old inputs over */
> > > > > > -if (pa_idxset_size(old_default_sink->inputs) <= 0) {
> > > > > > -pa_log_debug("No sink inputs to move away.");
> > > > > > -return PA_HOOK_OK;
> > > > > > -}
> > > > > > -
> > >