[pulseaudio-discuss] [PATCH v11 02/11] bluetooth: Fix usage of RTP structures in SBC codec

[Tanu Kaskinen]

On Sun, 2019-06-02 at 17:25 +0200, Pali Rohár wrote:
> Rename struct rtp_payload to rtp_sbc_payload as it is specific for SBC
> codec. Add proper checks for endianity in rtp.h header and use uint8_t type
> where appropriated. And because rtp_sbc_payload structure is not parsed by
> decoder there is no support for fragmented SBC frames. Add warning for it.
> ---
>  src/modules/bluetooth/a2dp-codec-sbc.c | 16 ++++++----
>  src/modules/bluetooth/rtp.h            | 58 +++++++++++++++++++---------------
>  2 files changed, 42 insertions(+), 32 deletions(-)
> 
> diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
> index f339b570d..6ab0b46cd 100644
> --- a/src/modules/bluetooth/a2dp-codec-sbc.c
> +++ b/src/modules/bluetooth/a2dp-codec-sbc.c
> @@ -480,7 +480,7 @@ static void reset(void *codec_info) {
>  
>  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 rtp_size = sizeof(struct rtp_header) + sizeof(struct rtp_sbc_payload);
>      size_t num_of_frames = (link_mtu - rtp_size) / sbc_info->frame_length;
>  
>      *decoded_buffer_size = num_of_frames * sbc_info->codesize;
> @@ -510,14 +510,14 @@ static int reduce_encoder_bitrate(void *codec_info) {
>  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) {
>      struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
>      struct rtp_header *header;
> -    struct rtp_payload *payload;
> +    struct rtp_sbc_payload *payload;
>      uint8_t *d;
>      const uint8_t *p;
>      size_t to_write, to_encode;
>      unsigned frame_count;
>  
>      header = (struct rtp_header*) output_buffer;
> -    payload = (struct rtp_payload*) (output_buffer + sizeof(*header));
> +    payload = (struct rtp_sbc_payload*) (output_buffer + sizeof(*header));
>  
>      frame_count = 0;
>  
> @@ -562,7 +562,7 @@ static size_t encode_buffer(void *codec_info, uint32_t timestamp, const uint8_t
>      } PA_ONCE_END;
>  
>      /* write it to the fifo */
> -    memset(output_buffer, 0, sizeof(*header) + sizeof(*payload));
> +    pa_memzero(output_buffer, sizeof(*header) + sizeof(*payload));
>      header->v = 2;
>  
>      /* A2DP spec: "A payload type in the RTP dynamic range shall be chosen".
> @@ -583,13 +583,17 @@ static size_t decode_buffer(void *codec_info, const uint8_t *input_buffer, size_
>      struct sbc_info *sbc_info = (struct sbc_info *) codec_info;
>  
>      struct rtp_header *header;
> -    struct rtp_payload *payload;
> +    struct rtp_sbc_payload *payload;
>      const uint8_t *p;
>      uint8_t *d;
>      size_t to_write, to_decode;
>  
>      header = (struct rtp_header *) input_buffer;
> -    payload = (struct rtp_payload*) (input_buffer + sizeof(*header));
> +    payload = (struct rtp_sbc_payload*) (input_buffer + sizeof(*header));
> +
> +    /* TODO: Add support for decoding fragmented SBC frames */
> +    if (payload->is_fragmented)
> +        pa_log_warn("SBC frame is fragmented, decoding may fail");

If we don't currently support fragmented frames, I think it would be
better to just flat out fail here. I imagine we'll hit a fatal error
soon anyway, but if by some miracle PulseAudio managed to decode the
stream anyway (maybe the frames aren't fragmented after all), the
syslog would potentially get flooded with these warnings.

-- 
Tanu

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