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.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
