Hi George,

On Mon, Apr 9, 2018 at 7:16 PM, Georg Chini <ge...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss



-- 
Luiz Augusto von Dentz
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

Reply via email to