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.
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.
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.
> > +
> > + /* 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.
> 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.
Unless fragmentation is not used... which is allowed for bluetooth SBC.
> 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.
Uff... this looks like a bigger problem that I though...
> 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.
Ok.
> > 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.
Ok, I can do it.
--
Pali Rohár
pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
