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

Submitted by Pali Rohár on July 5, 2019, 1:02 p.m.

Details

Message ID 20190705130226.5496-2-pali.rohar@gmail.com
State Superseded
Headers show
Series "New API for Bluetooth A2DP codecs" ( rev: 95 94 93 92 91 90 89 88 87 86 85 84 83 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Pali Rohár July 5, 2019, 1:02 p.m.
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(-)

Patch hide | download patch | download mbox

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(&sbc_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) decoded);
-            *processed = p - input_buffer;
-            return 0;
+            break;
         }
 
         /* Reset frame length, it can be changed due to bitpool change */
@@ -616,6 +629,7 @@  static size_t decode_buffer(void *codec_info, const uint8_t *input_buffer, size_
         pa_assert_fp((size_t) decoded <= to_decode);
         pa_assert_fp((size_t) decoded == sbc_info->frame_length);
 
+        pa_assert_fp((size_t) written <= to_write);
         pa_assert_fp((size_t) written == sbc_info->codesize);
 
         p += decoded;
diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
index 56c96054d..140ddb8fb 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -412,30 +412,41 @@  static int sco_process_push(struct userdata *u) {
 static void a2dp_prepare_encoder_buffer(struct userdata *u) {
     pa_assert(u);
 
-    if (u->encoder_buffer_size >= u->write_link_mtu)
-        return;
+    if (u->encoder_buffer_size < u->write_link_mtu) {
+        pa_xfree(u->encoder_buffer);
+        u->encoder_buffer = pa_xmalloc(u->write_link_mtu);
+    }
 
-    u->encoder_buffer_size = 2 * u->write_link_mtu;
-    pa_xfree(u->encoder_buffer);
-    u->encoder_buffer = pa_xmalloc(u->encoder_buffer_size);
+    /* Encoder buffer cannot be larger then link MTU, otherwise
+     * encode method would produce larger packets then link MTU */
+    u->encoder_buffer_size = u->write_link_mtu;
 }
 
 /* Run from IO thread */
 static void a2dp_prepare_decoder_buffer(struct userdata *u) {
     pa_assert(u);
 
-    if (u->decoder_buffer_size >= u->read_link_mtu)
-        return;
+    if (u->decoder_buffer_size < u->read_link_mtu) {
+        pa_xfree(u->decoder_buffer);
+        u->decoder_buffer = pa_xmalloc(u->read_link_mtu);
+    }
 
-    u->decoder_buffer_size = 2 * u->read_link_mtu;
-    pa_xfree(u->decoder_buffer);
-    u->decoder_buffer = pa_xmalloc(u->decoder_buffer_size);
+    /* Decoder buffer cannot be larger then link MTU, otherwise
+     * decode method would produce larger output then read_block_size */
+    u->decoder_buffer_size = u->read_link_mtu;
 }
 
 /* Run from IO thread */
 static int a2dp_write_buffer(struct userdata *u, size_t nbytes) {
     int ret = 0;
 
+    if (PA_UNLIKELY(!nbytes)) {
+        u->write_index += (uint64_t) u->write_memchunk.length;
+        pa_memblock_unref(u->write_memchunk.memblock);
+        pa_memchunk_reset(&u->write_memchunk);
+        return 0;
+    }
+
     for (;;) {
         ssize_t l;
 
@@ -508,10 +519,10 @@  static int a2dp_process_render(struct userdata *u) {
 
     pa_memblock_release(u->write_memchunk.memblock);
 
-    if (length == 0)
+    if (processed != u->write_memchunk.length) {
+        pa_log_error("Encoding error");
         return -1;
-
-    pa_assert(processed == u->write_memchunk.length);
+    }
 
     return a2dp_write_buffer(u, length);
 }
@@ -530,6 +541,8 @@  static int a2dp_process_push(struct userdata *u) {
     memchunk.memblock = pa_memblock_new(u->core->mempool, u->read_block_size);
     memchunk.index = memchunk.length = 0;
 
+    a2dp_prepare_decoder_buffer(u);
+
     for (;;) {
         bool found_tstamp = false;
         pa_usec_t tstamp;
@@ -537,8 +550,6 @@  static int a2dp_process_push(struct userdata *u) {
         ssize_t l;
         size_t processed;
 
-        a2dp_prepare_decoder_buffer(u);
-
         l = pa_read(u->stream_fd, u->decoder_buffer, u->decoder_buffer_size, &u->stream_write_type);
 
         if (l <= 0) {
@@ -568,9 +579,12 @@  static int a2dp_process_push(struct userdata *u) {
         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, &processed);
-        if (memchunk.length == 0) {
-            pa_memblock_release(memchunk.memblock);
-            ret = 0;
+
+        pa_memblock_release(memchunk.memblock);
+
+        if (processed != (size_t) l) {
+            pa_log_error("Decoding error");
+            ret = -1;
             break;
         }
 
@@ -578,9 +592,8 @@  static int a2dp_process_push(struct userdata *u) {
         pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->decoder_sample_spec));
         pa_smoother_resume(u->read_smoother, tstamp, true);
 
-        pa_memblock_release(memchunk.memblock);
-
-        pa_source_post(u->source, &memchunk);
+        if (PA_LIKELY(memchunk.length))
+            pa_source_post(u->source, &memchunk);
 
         ret = l;
         break;

Comments

On Fri, 2019-07-05 at 15:02 +0200, Pali Rohár wrote:
>  /* Run from IO thread */
>  static int a2dp_write_buffer(struct userdata *u, size_t nbytes) {
>      int ret = 0;
>  
> +    if (PA_UNLIKELY(!nbytes)) {
> +        u->write_index += (uint64_t) u->write_memchunk.length;
> +        pa_memblock_unref(u->write_memchunk.memblock);
> +        pa_memchunk_reset(&u->write_memchunk);
> +        return 0;
> +    }

Is this to handle a situation where the encoder accepts input but
doesn't produce any output (the algorithmic delay thing you mentioned
in the commit message)? This can't happen with SBC, right? I think
there should be a comment explaining when nbytes can be zero.

> @@ -578,9 +592,8 @@ static int a2dp_process_push(struct userdata *u) {
>          pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->decoder_sample_spec));
>          pa_smoother_resume(u->read_smoother, tstamp, true);
>  
> -        pa_memblock_release(memchunk.memblock);
> -
> -        pa_source_post(u->source, &memchunk);
> +        if (PA_LIKELY(memchunk.length))
> +            pa_source_post(u->source, &memchunk);

I'm not sure about this. It sounds like it should be impossible that an
encoded frame results in no decoded data, so maybe an assertion would
make more sense? On the other hand, a simple check like this can't do
much harm, and if some codec indeed can return zero data, just skipping
pa_source_post() in that case seems correct.