Hi George,
On Mon, Apr 9, 2018 at 7:16 PM, Georg Chini <georg at chini.tk> wrote:
> Currently the PA bluetooth code calls source_push() for each HSP source packet.
> The default HSP MTU is 48 bytes, this means that source_push() is called every
> 3ms, which leads to increased CPU load especially on embedded systems.
>
> This patch adds a hsp_source_buffer_msec argument to module-bluez5-discover and
> module-bluez5-device. The argument gives the number of milliseconds of audio which
> are buffered, before source_push() is called. The value can range from 0 to 100ms,
> and is rounded down to the next multiple of the MTU size. Therefore a value of less
> than 2*MTU time corresponds to the original behavior.
Well SCO is, as the name suggests, synchronous or to be more precise
it isochronous so I wonder if this has been tested? It seems to me
this will start to behave like A2DP which buffer frames for a while
before sending, though A2DP is not really for voice where latency may
cause problems like lip sync issues or talking over someone else on a
call because the audio has a few second delay. I get that using 3ms
when the default-fragment-size-msec is bigger than that may not make
any difference so we should probably use that instead.
> ---
> src/modules/bluetooth/module-bluetooth-discover.c | 1 +
> src/modules/bluetooth/module-bluez5-device.c | 68 +++++++++++++++++------
> src/modules/bluetooth/module-bluez5-discover.c | 14 ++++-
> 3 files changed, 64 insertions(+), 19 deletions(-)
I do have patches changing the bluez5 modules to bluetooth, I wonder
what happen to that.
> diff --git a/src/modules/bluetooth/module-bluetooth-discover.c b/src/modules/bluetooth/module-bluetooth-discover.c
> index 63195d3e..14c0a38f 100644
> --- a/src/modules/bluetooth/module-bluetooth-discover.c
> +++ b/src/modules/bluetooth/module-bluetooth-discover.c
> @@ -32,6 +32,7 @@ PA_MODULE_LOAD_ONCE(true);
> PA_MODULE_USAGE(
> "headset=ofono|native|auto (bluez 5 only)"
> "autodetect_mtu=<boolean> (bluez 5 only)"
> + "hsp_source_buffer_msec=<0 - 100> (bluez5 only)"
> );
>
> struct userdata {
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index b81c233c..d40bbb0c 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -54,7 +54,8 @@ 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>"
> - "autodetect_mtu=<boolean>");
> + "autodetect_mtu=<boolean>"
> + "hsp_source_buffer_msec=<0 - 100>");
>
> #define FIXED_LATENCY_PLAYBACK_A2DP (25 * PA_USEC_PER_MSEC)
> #define FIXED_LATENCY_PLAYBACK_SCO (25 * PA_USEC_PER_MSEC)
> @@ -68,6 +69,7 @@ PA_MODULE_USAGE("path=<device object path>"
> static const char* const valid_modargs[] = {
> "path",
> "autodetect_mtu",
> + "hsp_source_buffer_msec",
> NULL
> };
>
> @@ -123,6 +125,9 @@ struct userdata {
> pa_card *card;
> pa_sink *sink;
> pa_source *source;
> + uint32_t source_buffer_usec;
> + uint32_t source_buffered_blocks;
> + pa_memchunk source_buffer;
> pa_bluetooth_profile_t profile;
> char *output_port_name;
> char *input_port_name;
> @@ -314,10 +319,10 @@ static int sco_process_render(struct userdata *u) {
> /* Run from IO thread */
> static int sco_process_push(struct userdata *u) {
> ssize_t l;
> - pa_memchunk memchunk;
> struct cmsghdr *cm;
> struct msghdr m;
> bool found_tstamp = false;
> + uint32_t max_blocks;
> pa_usec_t tstamp = 0;
>
> pa_assert(u);
> @@ -326,11 +331,17 @@ static int sco_process_push(struct userdata *u) {
> pa_assert(u->source);
> pa_assert(u->read_smoother);
>
> - memchunk.memblock = pa_memblock_new(u->core->mempool, u->read_block_size);
> - memchunk.index = memchunk.length = 0;
> + max_blocks = u->source_buffer_usec / pa_bytes_to_usec(u->read_block_size, &u->source->sample_spec);
> + if (max_blocks == 0)
> + max_blocks = 1;
> +
> + if (!u->source_buffer.memblock) {
> + u->source_buffer.memblock = pa_memblock_new(u->core->mempool, max_blocks * u->read_block_size);
> + u->source_buffer.index = u->source_buffer.length = 0;
> + }
>
> for (;;) {
> - void *p;
> + uint8_t *p;
> uint8_t aux[1024];
> struct iovec iov;
>
> @@ -343,11 +354,11 @@ static int sco_process_push(struct userdata *u) {
> m.msg_control = aux;
> m.msg_controllen = sizeof(aux);
>
> - p = pa_memblock_acquire(memchunk.memblock);
> - iov.iov_base = p;
> - iov.iov_len = pa_memblock_get_length(memchunk.memblock);
> + p = pa_memblock_acquire(u->source_buffer.memblock);
> + iov.iov_base = p + u->source_buffer.index;
> + iov.iov_len = u->read_block_size;
> l = recvmsg(u->stream_fd, &m, 0);
> - pa_memblock_release(memchunk.memblock);
> + pa_memblock_release(u->source_buffer.memblock);
>
> if (l > 0)
> break;
> @@ -356,8 +367,6 @@ static int sco_process_push(struct userdata *u) {
> /* Retry right away if we got interrupted */
> continue;
>
> - pa_memblock_unref(memchunk.memblock);
> -
> if (l < 0 && errno == EAGAIN)
> /* Hmm, apparently the socket was not readable, give up for now. */
> return 0;
> @@ -366,7 +375,7 @@ static int sco_process_push(struct userdata *u) {
> return -1;
> }
>
> - pa_assert((size_t) l <= pa_memblock_get_length(memchunk.memblock));
> + pa_assert((size_t) l <= u->read_block_size);
>
> /* In some rare occasions, we might receive packets of a very strange
> * size. This could potentially be possible if the SCO packet was
> @@ -376,11 +385,10 @@ static int sco_process_push(struct userdata *u) {
> * packet */
> if (!pa_frame_aligned(l, &u->sample_spec)) {
> pa_log_warn("SCO packet received of unaligned size: %zu", l);
> - pa_memblock_unref(memchunk.memblock);
> return -1;
> }
>
> - memchunk.length = (size_t) l;
> + u->source_buffer.index += (size_t) l;
> u->read_index += (uint64_t) l;
>
> for (cm = CMSG_FIRSTHDR(&m); cm; cm = CMSG_NXTHDR(&m, cm))
> @@ -397,11 +405,19 @@ static int sco_process_push(struct userdata *u) {
> tstamp = pa_rtclock_now();
> }
>
> - pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->sample_spec));
> - pa_smoother_resume(u->read_smoother, tstamp, true);
> + u->source_buffered_blocks++;
>
> - pa_source_post(u->source, &memchunk);
> - pa_memblock_unref(memchunk.memblock);
> + if (u->source_buffered_blocks >= max_blocks) {
> + pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->sample_spec));
> + pa_smoother_resume(u->read_smoother, tstamp, true);
> +
> + u->source_buffer.length = u->source_buffer.index;
> + u->source_buffer.index = 0;
> + pa_source_post(u->source, &u->source_buffer);
> + pa_memblock_unref(u->source_buffer.memblock);
> + pa_memchunk_reset(&u->source_buffer);
> + u->source_buffered_blocks = 0;
> + }
>
> return l;
> }
> @@ -784,6 +800,12 @@ static void teardown_stream(struct userdata *u) {
> pa_memchunk_reset(&u->write_memchunk);
> }
>
> + if (u->source_buffer.memblock) {
> + pa_memblock_unref(u->source_buffer.memblock);
> + pa_memchunk_reset(&u->source_buffer);
> + }
> + u->source_buffered_blocks = 0;
> +
> pa_log_debug("Audio stream torn down");
> u->stream_setup_done = false;
> }
> @@ -947,6 +969,7 @@ static int source_process_msg(pa_msgobject *o, int code, void *data, int64_t off
> ri = pa_bytes_to_usec(u->read_index, &u->sample_spec);
>
> *((int64_t*) data) = u->source->thread_info.fixed_latency + wi - ri;
> + *((int64_t*) data) += u->source_buffered_blocks * pa_bytes_to_usec(u->read_block_size, &u->source->sample_spec);
> } else
> *((int64_t*) data) = 0;
>
> @@ -2362,6 +2385,7 @@ int pa__init(pa_module* m) {
> const char *path;
> pa_modargs *ma;
> bool autodetect_mtu;
> + uint32_t source_buffer_msec;
>
> pa_assert(m);
>
> @@ -2397,7 +2421,15 @@ int pa__init(pa_module* m) {
> goto fail_free_modargs;
> }
>
> + source_buffer_msec = 0;
> + if (pa_modargs_get_value_u32(ma, "hsp_source_buffer_msec", &source_buffer_msec) < 0 || source_buffer_msec > 100) {
> + pa_log("Invalid value for hsp_source_buffer_msec parameter, value must be <= 100");
> + goto fail;
> + }
> +
> u->device->autodetect_mtu = autodetect_mtu;
> + u->source_buffer_usec = source_buffer_msec * PA_USEC_PER_MSEC;
> + u->source_buffered_blocks = 0;
>
> pa_modargs_free(ma);
>
> diff --git a/src/modules/bluetooth/module-bluez5-discover.c b/src/modules/bluetooth/module-bluez5-discover.c
> index 44578214..8108627d 100644
> --- a/src/modules/bluetooth/module-bluez5-discover.c
> +++ b/src/modules/bluetooth/module-bluez5-discover.c
> @@ -36,11 +36,14 @@ PA_MODULE_VERSION(PACKAGE_VERSION);
> PA_MODULE_LOAD_ONCE(true);
> PA_MODULE_USAGE(
> "headset=ofono|native|auto"
> + "autodetect_mtu=<boolean>"
> + "hsp_source_buffer_msec=<0 - 100>"
> );
>
> static const char* const valid_modargs[] = {
> "headset",
> "autodetect_mtu",
> + "hsp_source_buffer_msec",
> NULL
> };
>
> @@ -51,6 +54,7 @@ struct userdata {
> pa_hook_slot *device_connection_changed_slot;
> pa_bluetooth_discovery *discovery;
> bool autodetect_mtu;
> + uint32_t source_buffer_msec;
> };
>
> static pa_hook_result_t device_connection_changed_cb(pa_bluetooth_discovery *y, const pa_bluetooth_device *d, struct userdata *u) {
> @@ -71,7 +75,7 @@ static pa_hook_result_t device_connection_changed_cb(pa_bluetooth_discovery *y,
> if (!module_loaded && pa_bluetooth_device_any_transport_connected(d)) {
> /* a new device has been connected */
> pa_module *m;
> - char *args = pa_sprintf_malloc("path=%s autodetect_mtu=%i", d->path, (int)u->autodetect_mtu);
> + char *args = pa_sprintf_malloc("path=%s autodetect_mtu=%i hsp_source_buffer_msec=%u", d->path, (int)u->autodetect_mtu, u->source_buffer_msec);
>
> pa_log_debug("Loading module-bluez5-device %s", args);
> pa_module_load(&m, u->module->core, "module-bluez5-device", args);
> @@ -102,6 +106,7 @@ int pa__init(pa_module *m) {
> const char *headset_str;
> int headset_backend;
> bool autodetect_mtu;
> + uint32_t source_buffer_msec;
>
> pa_assert(m);
>
> @@ -128,10 +133,17 @@ int pa__init(pa_module *m) {
> goto fail;
> }
>
> + source_buffer_msec = 0;
> + if (pa_modargs_get_value_u32(ma, "hsp_source_buffer_msec", &source_buffer_msec) < 0 || source_buffer_msec > 100) {
> + pa_log("Invalid value for hsp_source_buffer_msec parameter, value must be <= 100");
> + goto fail;
> + }
> +
> m->userdata = u = pa_xnew0(struct userdata, 1);
> u->module = m;
> u->core = m->core;
> u->autodetect_mtu = autodetect_mtu;
> + u->source_buffer_msec = source_buffer_msec;
> u->loaded_device_paths = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
>
> if (!(u->discovery = pa_bluetooth_discovery_get(u->core, headset_backend)))
> --
> 2.14.1
>
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
--
Luiz Augusto von Dentz
