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

Sun Jun 16 08:13:11 UTC 2019

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.

-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk