[v11,01/11] bluetooth: Fix A2DP codec API to provide information about data buffer

Submitted by Pali Rohár on June 2, 2019, 3:25 p.m.

Details

Message ID 20190602152520.2935-2-pali.rohar@gmail.com
State Superseded
Headers show
Series "New API for Bluetooth A2DP codecs" ( rev: 82 81 80 79 78 77 76 75 74 73 72 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Pali Rohár June 2, 2019, 3:25 p.m.
Each codec has different compression ratio and own method how to calculate
buffer size of encoded or decoded samples. So change A2DP codec API to
provide this information for module-bluez5-device module and fix
a2dp_prepare_encoder_buffer() and a2dp_prepare_decoder_buffer() functions.

API functions get_read_buffer_size() and get_write_buffer_size() now set
both decoded and encoded buffer sizes. Function reduce_encoder_bitrate()
was changed to not return new buffer size (it was not obvious if buffer
size was for encoded or decoded samples), but caller rather should call
get_write_buffer_size() to get new sizes.
---
 src/modules/bluetooth/a2dp-codec-api.h       | 17 ++++++------
 src/modules/bluetooth/a2dp-codec-sbc.c       | 25 ++++++++++-------
 src/modules/bluetooth/module-bluez5-device.c | 41 ++++++++++++++++++----------
 3 files changed, 50 insertions(+), 33 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..517dc76f1 100644
--- a/src/modules/bluetooth/a2dp-codec-api.h
+++ b/src/modules/bluetooth/a2dp-codec-api.h
@@ -72,15 +72,14 @@  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 */
-    size_t (*get_read_block_size)(void *codec_info, size_t read_link_mtu);
-    /* Get write block size for codec */
-    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
-     * if not changed, called when socket is not accepting encoded data fast
-     * enough */
-    size_t (*reduce_encoder_bitrate)(void *codec_info, size_t write_link_mtu);
+    /* Get buffer sizes for read operations */
+    void (*get_read_buffer_size)(void *codec_info, size_t read_link_mtu, size_t *output_buffer_size, size_t *encoded_buffer_size);
+    /* Get buffer sizes for write operations */
+    void (*get_write_buffer_size)(void *codec_info, size_t write_link_mtu, size_t *input_buffer_size, size_t *encoded_buffer_size);
+
+    /* Reduce encoder bitrate for codec, returns non-zero on failure,
+     * called when socket is not accepting encoded data fast enough */
+    int (*reduce_encoder_bitrate)(void *codec_info);
 
     /* Encode input_buffer of input_size to output_buffer of output_size,
      * returns size of filled ouput_buffer and set processed to size of
diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
index cdc20d7f0..f339b570d 100644
--- a/src/modules/bluetooth/a2dp-codec-sbc.c
+++ b/src/modules/bluetooth/a2dp-codec-sbc.c
@@ -423,7 +423,10 @@  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 buffer size
+     * in get_buffer_size() function. Buffer size 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);
@@ -475,20 +478,22 @@  static void reset(void *codec_info) {
     sbc_info->seq_num = 0;
 }
 
-static size_t get_block_size(void *codec_info, size_t link_mtu) {
+static void get_buffer_size(void *codec_info, size_t link_mtu, size_t *decoded_buffer_size, size_t *encoded_buffer_size) {
     struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
+    size_t rtp_size = sizeof(struct rtp_header) + sizeof(struct rtp_payload);
+    size_t num_of_frames = (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;
+    *decoded_buffer_size = num_of_frames * sbc_info->codesize;
+    *encoded_buffer_size = num_of_frames * sbc_info->frame_length + rtp_size;
 }
 
-static size_t reduce_encoder_bitrate(void *codec_info, size_t write_link_mtu) {
+static int reduce_encoder_bitrate(void *codec_info) {
     struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
     uint8_t bitpool;
 
     /* Check if bitpool is already at its limit */
     if (sbc_info->sbc.bitpool <= SBC_BITPOOL_DEC_LIMIT)
-        return 0;
+        return -1;
 
     bitpool = sbc_info->sbc.bitpool - SBC_BITPOOL_DEC_STEP;
 
@@ -496,10 +501,10 @@  static size_t reduce_encoder_bitrate(void *codec_info, size_t write_link_mtu) {
         bitpool = SBC_BITPOOL_DEC_LIMIT;
 
     if (sbc_info->sbc.bitpool == bitpool)
-        return 0;
+        return -1;
 
     set_bitpool(sbc_info, bitpool);
-    return get_block_size(codec_info, write_link_mtu);
+    return 0;
 }
 
 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) {
@@ -639,8 +644,8 @@  const pa_a2dp_codec pa_a2dp_codec_sbc = {
     .init = init,
     .deinit = deinit,
     .reset = reset,
-    .get_read_block_size = get_block_size,
-    .get_write_block_size = get_block_size,
+    .get_read_buffer_size = get_buffer_size,
+    .get_write_buffer_size = get_buffer_size,
     .reduce_encoder_bitrate = reduce_encoder_bitrate,
     .encode_buffer = encode_buffer,
     .decode_buffer = decode_buffer,
diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
index 56c96054d..c0b293d94 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -125,7 +125,9 @@  struct userdata {
     size_t read_link_mtu;
     size_t write_link_mtu;
     size_t read_block_size;
+    size_t read_encoded_block_size;
     size_t write_block_size;
+    size_t write_encoded_block_size;
     uint64_t read_index;
     uint64_t write_index;
     pa_usec_t started_at;
@@ -412,10 +414,10 @@  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)
+    if (u->encoder_buffer_size >= u->write_encoded_block_size)
         return;
 
-    u->encoder_buffer_size = 2 * u->write_link_mtu;
+    u->encoder_buffer_size = u->write_encoded_block_size;
     pa_xfree(u->encoder_buffer);
     u->encoder_buffer = pa_xmalloc(u->encoder_buffer_size);
 }
@@ -424,10 +426,10 @@  static void a2dp_prepare_encoder_buffer(struct userdata *u) {
 static void a2dp_prepare_decoder_buffer(struct userdata *u) {
     pa_assert(u);
 
-    if (u->decoder_buffer_size >= u->read_link_mtu)
+    if (u->decoder_buffer_size >= u->read_encoded_block_size)
         return;
 
-    u->decoder_buffer_size = 2 * u->read_link_mtu;
+    u->decoder_buffer_size = u->read_encoded_block_size;
     pa_xfree(u->decoder_buffer);
     u->decoder_buffer = pa_xmalloc(u->decoder_buffer_size);
 }
@@ -436,6 +438,9 @@  static void a2dp_prepare_decoder_buffer(struct userdata *u) {
 static int a2dp_write_buffer(struct userdata *u, size_t nbytes) {
     int ret = 0;
 
+    if (!nbytes)
+        return 0;
+
     for (;;) {
         ssize_t l;
 
@@ -504,14 +509,14 @@  static int a2dp_process_render(struct userdata *u) {
     /* Try to create a packet of the full MTU */
     ptr = (const uint8_t *) pa_memblock_acquire_chunk(&u->write_memchunk);
 
-    length = u->a2dp_codec->encode_buffer(u->encoder_info, u->write_index / pa_frame_size(&u->encoder_sample_spec), ptr, u->write_memchunk.length, u->encoder_buffer, u->encoder_buffer_size, &processed);
+    length = u->a2dp_codec->encode_buffer(u->encoder_info, u->write_index / pa_frame_size(&u->encoder_sample_spec), ptr, u->write_memchunk.length, u->encoder_buffer, u->write_encoded_block_size, &processed);
 
     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);
 }
@@ -539,7 +544,7 @@  static int a2dp_process_push(struct userdata *u) {
 
         a2dp_prepare_decoder_buffer(u);
 
-        l = pa_read(u->stream_fd, u->decoder_buffer, u->decoder_buffer_size, &u->stream_write_type);
+        l = pa_read(u->stream_fd, u->decoder_buffer, u->read_encoded_block_size, &u->stream_write_type);
 
         if (l <= 0) {
 
@@ -568,6 +573,14 @@  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 (processed != (size_t) l) {
+            pa_log_error("Decoding error");
+            pa_memblock_release(memchunk.memblock);
+            ret = -1;
+            break;
+        }
+
         if (memchunk.length == 0) {
             pa_memblock_release(memchunk.memblock);
             ret = 0;
@@ -732,9 +745,9 @@  static void transport_config_mtu(struct userdata *u) {
     } else {
         pa_assert(u->a2dp_codec);
         if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
-            u->write_block_size = u->a2dp_codec->get_write_block_size(u->encoder_info, u->write_link_mtu);
+            u->a2dp_codec->get_write_buffer_size(u->encoder_info, u->write_link_mtu, &u->write_block_size, &u->write_encoded_block_size);
         } else {
-            u->read_block_size = u->a2dp_codec->get_read_block_size(u->decoder_info, u->read_link_mtu);
+            u->a2dp_codec->get_read_buffer_size(u->decoder_info, u->read_link_mtu, &u->read_block_size, &u->read_encoded_block_size);
         }
     }
 
@@ -1438,9 +1451,9 @@  static void thread_func(void *userdata) {
                             }
 
                             if (u->write_index > 0 && u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
-                                size_t new_write_block_size = u->a2dp_codec->reduce_encoder_bitrate(u->encoder_info, u->write_link_mtu);
-                                if (new_write_block_size) {
-                                    u->write_block_size = new_write_block_size;
+                                int failed = u->a2dp_codec->reduce_encoder_bitrate(u->encoder_info);
+                                if (!failed) {
+                                    u->a2dp_codec->get_write_buffer_size(u->encoder_info, u->write_link_mtu, &u->write_block_size, &u->write_encoded_block_size);
                                     handle_sink_block_size_change(u);
                                 }
                             }

Comments

On Sun, 2019-06-02 at 17:25 +0200, Pali Rohár wrote:
> Each codec has different compression ratio and own method how to calculate
> buffer size of encoded or decoded samples. So change A2DP codec API to
> provide this information for module-bluez5-device module and fix
> a2dp_prepare_encoder_buffer() and a2dp_prepare_decoder_buffer() functions.
> 
> API functions get_read_buffer_size() and get_write_buffer_size() now set
> both decoded and encoded buffer sizes. Function reduce_encoder_bitrate()
> was changed to not return new buffer size (it was not obvious if buffer
> size was for encoded or decoded samples), but caller rather should call
> get_write_buffer_size() to get new sizes.
> ---
>  src/modules/bluetooth/a2dp-codec-api.h       | 17 ++++++------
>  src/modules/bluetooth/a2dp-codec-sbc.c       | 25 ++++++++++-------
>  src/modules/bluetooth/module-bluez5-device.c | 41 ++++++++++++++++++----------
>  3 files changed, 50 insertions(+), 33 deletions(-)
> 
> diff --git a/src/modules/bluetooth/a2dp-codec-api.h b/src/modules/bluetooth/a2dp-codec-api.h
> index 55bb9ff70..517dc76f1 100644
> --- a/src/modules/bluetooth/a2dp-codec-api.h
> +++ b/src/modules/bluetooth/a2dp-codec-api.h
> @@ -72,15 +72,14 @@ 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 */
> -    size_t (*get_read_block_size)(void *codec_info, size_t read_link_mtu);
> -    /* Get write block size for codec */
> -    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
> -     * if not changed, called when socket is not accepting encoded data fast
> -     * enough */
> -    size_t (*reduce_encoder_bitrate)(void *codec_info, size_t write_link_mtu);
> +    /* Get buffer sizes for read operations */
> +    void (*get_read_buffer_size)(void *codec_info, size_t read_link_mtu, size_t *output_buffer_size, size_t *encoded_buffer_size);
> +    /* Get buffer sizes for write operations */
> +    void (*get_write_buffer_size)(void *codec_info, size_t write_link_mtu, size_t *input_buffer_size, size_t *encoded_buffer_size);

Since these return two sizes, I think "size" in the callback name
should be changed to "sizes".

In my opinion decoded_buffer_size would be a better name than
output/input_buffer_size.

> +
> +    /* Reduce encoder bitrate for codec, returns non-zero on failure,
> +     * called when socket is not accepting encoded data fast enough */
> +    int (*reduce_encoder_bitrate)(void *codec_info);
>  
>      /* Encode input_buffer of input_size to output_buffer of output_size,
>       * returns size of filled ouput_buffer and set processed to size of
> diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
> index cdc20d7f0..f339b570d 100644
> --- a/src/modules/bluetooth/a2dp-codec-sbc.c
> +++ b/src/modules/bluetooth/a2dp-codec-sbc.c
> @@ -423,7 +423,10 @@ 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 buffer size
> +     * in get_buffer_size() function. Buffer size 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;

Please specify that you're talking about the decoded buffer size. I
(for some reason) assumed that you meant the encoded buffer size, and
started writing a complaint about "buffer size is inversely
proportional to frame length" being wrong...

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.

>  
>      set_params(sbc_info);
> @@ -475,20 +478,22 @@ static void reset(void *codec_info) {
>      sbc_info->seq_num = 0;
>  }
>  
> -static size_t get_block_size(void *codec_info, size_t link_mtu) {
> +static void get_buffer_size(void *codec_info, size_t link_mtu, size_t *decoded_buffer_size, size_t *encoded_buffer_size) {

If you agree to do the "size" -> "sizes" renaming, this function name
needs to be changed too.

>      struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
> +    size_t rtp_size = sizeof(struct rtp_header) + sizeof(struct rtp_payload);
> +    size_t num_of_frames = (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;
> +    *decoded_buffer_size = num_of_frames * sbc_info->codesize;
> +    *encoded_buffer_size = num_of_frames * sbc_info->frame_length + rtp_size;
>  }
>  
> -static size_t reduce_encoder_bitrate(void *codec_info, size_t write_link_mtu) {
> +static int reduce_encoder_bitrate(void *codec_info) {
>      struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
>      uint8_t bitpool;
>  
>      /* Check if bitpool is already at its limit */
>      if (sbc_info->sbc.bitpool <= SBC_BITPOOL_DEC_LIMIT)
> -        return 0;
> +        return -1;
>  
>      bitpool = sbc_info->sbc.bitpool - SBC_BITPOOL_DEC_STEP;
>  
> @@ -496,10 +501,10 @@ static size_t reduce_encoder_bitrate(void *codec_info, size_t write_link_mtu) {
>          bitpool = SBC_BITPOOL_DEC_LIMIT;
>  
>      if (sbc_info->sbc.bitpool == bitpool)
> -        return 0;
> +        return -1;
>  
>      set_bitpool(sbc_info, bitpool);
> -    return get_block_size(codec_info, write_link_mtu);
> +    return 0;
>  }
>  
>  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) {
> @@ -639,8 +644,8 @@ const pa_a2dp_codec pa_a2dp_codec_sbc = {
>      .init = init,
>      .deinit = deinit,
>      .reset = reset,
> -    .get_read_block_size = get_block_size,
> -    .get_write_block_size = get_block_size,
> +    .get_read_buffer_size = get_buffer_size,
> +    .get_write_buffer_size = get_buffer_size,
>      .reduce_encoder_bitrate = reduce_encoder_bitrate,
>      .encode_buffer = encode_buffer,
>      .decode_buffer = decode_buffer,
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 56c96054d..c0b293d94 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -125,7 +125,9 @@ struct userdata {
>      size_t read_link_mtu;
>      size_t write_link_mtu;
>      size_t read_block_size;
> +    size_t read_encoded_block_size;
>      size_t write_block_size;
> +    size_t write_encoded_block_size;

I think renaming read/write_block_size to read/write_decoded_block_size
would improve clarity.

On Sat, 2019-06-15 at 11:05 +0200, Pali Rohár wrote:
> On Saturday 15 June 2019 11:50:10 Tanu Kaskinen wrote:
> > On Sun, 2019-06-02 at 17:25 +0200, Pali Rohár wrote:
> > > Each codec has different compression ratio and own method how to calculate
> > > buffer size of encoded or decoded samples. So change A2DP codec API to
> > > provide this information for module-bluez5-device module and fix
> > > a2dp_prepare_encoder_buffer() and a2dp_prepare_decoder_buffer() functions.
> > > 
> > > API functions get_read_buffer_size() and get_write_buffer_size() now set
> > > both decoded and encoded buffer sizes. Function reduce_encoder_bitrate()
> > > was changed to not return new buffer size (it was not obvious if buffer
> > > size was for encoded or decoded samples), but caller rather should call
> > > get_write_buffer_size() to get new sizes.
> > > ---
> > >  src/modules/bluetooth/a2dp-codec-api.h       | 17 ++++++------
> > >  src/modules/bluetooth/a2dp-codec-sbc.c       | 25 ++++++++++-------
> > >  src/modules/bluetooth/module-bluez5-device.c | 41 ++++++++++++++++++----------
> > >  3 files changed, 50 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/src/modules/bluetooth/a2dp-codec-api.h b/src/modules/bluetooth/a2dp-codec-api.h
> > > index 55bb9ff70..517dc76f1 100644
> > > --- a/src/modules/bluetooth/a2dp-codec-api.h
> > > +++ b/src/modules/bluetooth/a2dp-codec-api.h
> > > @@ -72,15 +72,14 @@ 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 */
> > > -    size_t (*get_read_block_size)(void *codec_info, size_t read_link_mtu);
> > > -    /* Get write block size for codec */
> > > -    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
> > > -     * if not changed, called when socket is not accepting encoded data fast
> > > -     * enough */
> > > -    size_t (*reduce_encoder_bitrate)(void *codec_info, size_t write_link_mtu);
> > > +    /* Get buffer sizes for read operations */
> > > +    void (*get_read_buffer_size)(void *codec_info, size_t read_link_mtu, size_t *output_buffer_size, size_t *encoded_buffer_size);
> > > +    /* Get buffer sizes for write operations */
> > > +    void (*get_write_buffer_size)(void *codec_info, size_t write_link_mtu, size_t *input_buffer_size, size_t *encoded_buffer_size);
> > 
> > Since these return two sizes, I think "size" in the callback name
> > should be changed to "sizes".
> 
> Ok, fine!
> 
> > In my opinion decoded_buffer_size would be a better name than
> > output/input_buffer_size.
> 
> I was thinking about it for a longer time. Main problem is that reader
> should know which size is for input buffer size and which for output
> buffer size. More times I had a mistake that I switched input and
> output, because from name "encoded" and "decoded" it is not know which
> one is input and which output.

I would have imagined that the "get_read_buffer_size" callback name is
enough to make it clear which is which, but if you have experience of
getting them mixed up anyway, then it no doubt is a real problem.

> When encoding, input is decoded buffer; when decoding, input is encoded
> buffer.
> 
> And you can see that it is not possible to have consistency in functions
> get_read_buffer_sizes and get_write_buffer_sizes. Either third argument
> would be input buffer size; or it would be decoded buffer size.

I don't follow the argument in this paragraph. What consistency problem
is there?

> So I decided to have both information in function parameters, to know
> which one is input/output and also which one is encoded/decoded.
> 
> 
> Another suggestion how to solve this problem to know which buffer is
> input and which output:
> 
> Use names: decoded_output_buffer_size, encoded_input_buffer_size
> 
> It is just longer name, but contains both information.

I think these longer names are good too.

> > > +
> > > +    /* Reduce encoder bitrate for codec, returns non-zero on failure,
> > > +     * called when socket is not accepting encoded data fast enough */
> > > +    int (*reduce_encoder_bitrate)(void *codec_info);
> > >  
> > >      /* Encode input_buffer of input_size to output_buffer of output_size,
> > >       * returns size of filled ouput_buffer and set processed to size of
> > > diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
> > > index cdc20d7f0..f339b570d 100644
> > > --- a/src/modules/bluetooth/a2dp-codec-sbc.c
> > > +++ b/src/modules/bluetooth/a2dp-codec-sbc.c
> > > @@ -423,7 +423,10 @@ 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 buffer size
> > > +     * in get_buffer_size() function. Buffer size 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;
> > 
> > Please specify that you're talking about the decoded buffer size. I
> > (for some reason) assumed that you meant the encoded buffer size, and
> > started writing a complaint about "buffer size is inversely
> > proportional to frame length" being wrong...
> 
> Ok, I will add this information. I understand that it is not so easy and
> obvious.
> 
> > Although I made that mistake, I think I'm right in saying that our
> > reading logic is broken at least with SBC.
> 
> Yes, in SBC we moreover do not support fragmented RTP packets. In next
> patch I added warning when such packet is received so user could see in
> log that pulseaudio has troubles...
> 
> Proper way is to rewrite SBC decoder logic to support all these kind of
> features, like fragmented RTP packets, dynamic frame size change, etc...
> 
> But it does not belong to this patch.

Ok, fair enough. I hope you plan to write the patch that fixes these
problems.

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.
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).