[v11,02/11] bluetooth: Fix usage of RTP structures in SBC codec

Submitted by Pali Rohár on June 2, 2019, 3:25 p.m.

Details

Message ID 20190602152520.2935-3-pali.rohar@gmail.com
State New
Headers show
Series "New API for Bluetooth A2DP codecs" ( rev: 83 82 81 80 79 78 77 76 75 74 73 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Pali Rohár June 2, 2019, 3:25 p.m.
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(-)

Patch hide | download patch | download mbox

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");
 
     p = input_buffer + sizeof(*header) + sizeof(*payload);
     to_decode = input_size - sizeof(*header) - sizeof(*payload);
diff --git a/src/modules/bluetooth/rtp.h b/src/modules/bluetooth/rtp.h
index 20694c1e1..813d9e390 100644
--- a/src/modules/bluetooth/rtp.h
+++ b/src/modules/bluetooth/rtp.h
@@ -3,6 +3,7 @@ 
  *  BlueZ - Bluetooth protocol stack for Linux
  *
  *  Copyright (C) 2004-2010  Marcel Holtmann <marcel@holtmann.org>
+ *  Copyright (C) 2019       Pali Rohár <pali.rohar@gmail.com>
  *
  *
  *  This library is free software; you can redistribute it and/or
@@ -19,16 +20,20 @@ 
  *  License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#if __BYTE_ORDER == __LITTLE_ENDIAN
+#include <endian.h>
+#include <stdint.h>
+
+#if defined(__BYTE_ORDER) && defined(__LITTLE_ENDIAN) && \
+	__BYTE_ORDER == __LITTLE_ENDIAN
 
 struct rtp_header {
-	unsigned cc:4;
-	unsigned x:1;
-	unsigned p:1;
-	unsigned v:2;
+	uint8_t cc:4;
+	uint8_t x:1;
+	uint8_t p:1;
+	uint8_t v:2;
 
-	unsigned pt:7;
-	unsigned m:1;
+	uint8_t pt:7;
+	uint8_t m:1;
 
 	uint16_t sequence_number;
 	uint32_t timestamp;
@@ -36,24 +41,25 @@  struct rtp_header {
 	uint32_t csrc[0];
 } __attribute__ ((packed));
 
-struct rtp_payload {
-	unsigned frame_count:4;
-	unsigned rfa0:1;
-	unsigned is_last_fragment:1;
-	unsigned is_first_fragment:1;
-	unsigned is_fragmented:1;
+struct rtp_sbc_payload {
+	uint8_t frame_count:4;
+	uint8_t rfa0:1;
+	uint8_t is_last_fragment:1;
+	uint8_t is_first_fragment:1;
+	uint8_t is_fragmented:1;
 } __attribute__ ((packed));
 
-#elif __BYTE_ORDER == __BIG_ENDIAN
+#elif defined(__BYTE_ORDER) && defined(__BIG_ENDIAN) && \
+	__BYTE_ORDER == __BIG_ENDIAN
 
 struct rtp_header {
-	unsigned v:2;
-	unsigned p:1;
-	unsigned x:1;
-	unsigned cc:4;
+	uint8_t v:2;
+	uint8_t p:1;
+	uint8_t x:1;
+	uint8_t cc:4;
 
-	unsigned m:1;
-	unsigned pt:7;
+	uint8_t m:1;
+	uint8_t pt:7;
 
 	uint16_t sequence_number;
 	uint32_t timestamp;
@@ -61,12 +67,12 @@  struct rtp_header {
 	uint32_t csrc[0];
 } __attribute__ ((packed));
 
-struct rtp_payload {
-	unsigned is_fragmented:1;
-	unsigned is_first_fragment:1;
-	unsigned is_last_fragment:1;
-	unsigned rfa0:1;
-	unsigned frame_count:4;
+struct rtp_sbc_payload {
+	uint8_t is_fragmented:1;
+	uint8_t is_first_fragment:1;
+	uint8_t is_last_fragment:1;
+	uint8_t rfa0:1;
+	uint8_t frame_count:4;
 } __attribute__ ((packed));
 
 #else

Comments

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.