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

Sun Jun 16 16:43:09 UTC 2019

On Sunday 16 June 2019 11:13:11 Tanu Kaskinen wrote:
> 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?

Which argument for those function is for input buffer size and which
argument for output buffer size?

Currently get_read_buffer_size fill output buffer size into third
argument and input buffer size to fourth argument.

And on the other hand, get_write_buffer_size fill output buffer size
into fourth argument and input buffer size to third argument.

So it is inconsistency in API, which cannot be fixed for obvious reason.
So I decided to add name "input" / "output" to variable names, so
readers would know it.

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

Great, it should fix problem how how variable names should be called :-)

> > > > +
> > > > +    /* 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.

Probably later after we would have working encoding support for other
A2DP codecs.

This SBC fragmentation would need more logic and state machine in PA SBC
code to implement is properly...

-- 
Pali Rohár
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20190616/7af68b75/attachment.sig>