On Saturday 09 November 2019 13:08:34 Georg Chini wrote:
> On 06.10.19 19:58, Pali Rohár wrote:
> > Some A2DP codecs (e.g. SBC or aptX-HD) use RTP packets. For sources use
> > timestamps from RTP packets to calculate read index and therefore remote
> > timestamp for synchronization.
> > ---
> > src/modules/bluetooth/a2dp-codec-api.h | 4 ++--
> > src/modules/bluetooth/a2dp-codec-sbc.c | 3 ++-
> > src/modules/bluetooth/module-bluez5-device.c | 14 +++++++++++---
> > 3 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/modules/bluetooth/a2dp-codec-api.h b/src/modules/bluetooth/a2dp-codec-api.h
> > index a3123f4ca..1fd8e81d0 100644
> > --- a/src/modules/bluetooth/a2dp-codec-api.h
> > +++ b/src/modules/bluetooth/a2dp-codec-api.h
> > @@ -91,8 +91,8 @@ typedef struct pa_a2dp_codec {
> > 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);
> > /* Decode input_buffer of input_size to output_buffer of output_size,
> > * returns size of filled ouput_buffer and set processed to size of
> > - * processed input_buffer */
> > - size_t (*decode_buffer)(void *codec_info, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed);
> > + * processed input_buffer and set timestamp */
> > + size_t (*decode_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);
> > } pa_a2dp_codec;
> > #endif
> > diff --git a/src/modules/bluetooth/a2dp-codec-sbc.c b/src/modules/bluetooth/a2dp-codec-sbc.c
> > index 89c647fbe..733c1a9ab 100644
> > --- a/src/modules/bluetooth/a2dp-codec-sbc.c
> > +++ b/src/modules/bluetooth/a2dp-codec-sbc.c
> > @@ -597,7 +597,7 @@ static size_t encode_buffer(void *codec_info, uint32_t timestamp, const uint8_t
> > return d - output_buffer;
> > }
> > -static size_t decode_buffer(void *codec_info, const uint8_t *input_buffer, size_t input_size, uint8_t *output_buffer, size_t output_size, size_t *processed) {
> > +static size_t decode_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;
> > @@ -657,6 +657,7 @@ static size_t decode_buffer(void *codec_info, const uint8_t *input_buffer, size_
> > frame_count--;
> > }
> > + *timestamp = ntohl(header->timestamp);
> > *processed = p - input_buffer;
> > return d - output_buffer;
> > }
> > diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> > index 9da5d1ac3..fb77afaad 100644
> > --- a/src/modules/bluetooth/module-bluez5-device.c
> > +++ b/src/modules/bluetooth/module-bluez5-device.c
> > @@ -556,6 +556,7 @@ static int a2dp_process_push(struct userdata *u) {
> > struct msghdr m;
> > bool found_tstamp = false;
> > pa_usec_t tstamp;
> > + uint32_t timestamp;
> > uint8_t *ptr;
> > ssize_t l;
> > size_t processed;
> > @@ -591,8 +592,6 @@ static int a2dp_process_push(struct userdata *u) {
> > pa_assert((size_t) l <= u->decoder_buffer_size);
> > - /* TODO: get timestamp from rtp */
> > -
> > for (cm = CMSG_FIRSTHDR(&m); cm; cm = CMSG_NXTHDR(&m, cm)) {
> > if (cm->cmsg_level == SOL_SOCKET && cm->cmsg_type == SO_TIMESTAMP) {
> > struct timeval *tv = (struct timeval*) CMSG_DATA(cm);
> > @@ -613,7 +612,8 @@ static int a2dp_process_push(struct userdata *u) {
> > ptr = pa_memblock_acquire(memchunk.memblock);
> > memchunk.length = pa_memblock_get_length(memchunk.memblock);
> > - memchunk.length = u->a2dp_codec->decode_buffer(u->decoder_info, u->decoder_buffer, l, ptr, memchunk.length, &processed);
> > + timestamp = 0; /* Decoder does not have to fill RTP timestamp */
> > + memchunk.length = u->a2dp_codec->decode_buffer(u->decoder_info, ×tamp, u->decoder_buffer, l, ptr, memchunk.length, &processed);
> > pa_memblock_release(memchunk.memblock);
> > @@ -623,6 +623,14 @@ static int a2dp_process_push(struct userdata *u) {
> > break;
> > }
> > + /* Some codecs may provide RTP timestamp, so use it to update read_index for calculation of remote tstamp */
> > + if (timestamp) {
> > + /* RTP timestamp is only 32bit and may overflow, avoid it by calculating high 32bits from the last read_index */
> > + size_t frame_size = pa_frame_size(&u->decoder_sample_spec);
> > + uint64_t timestamp_hi = (u->read_index / frame_size) & 0xFFFFFFFF00000000ULL;
> > + u->read_index = (timestamp_hi | timestamp) * frame_size;
> > + }
> > +
> > u->read_index += (uint64_t) memchunk.length;
> > pa_smoother_put(u->read_smoother, tstamp, pa_bytes_to_usec(u->read_index, &u->decoder_sample_spec));
> > pa_smoother_resume(u->read_smoother, tstamp, true);
>
> This patch has three issues:
>
> 1) According to the specification, the RTP timestamp header may have an
> arbitrary offset.
>
> 2) I do not see how timestamp_hi could be anything but 0. Even if
> u->read_index is above
> the 32 bit border, the division by the frame size will always make it
> smaller because you
> start with a 32 bit number multiplied by frame size. So the logic for
> calculating the index
> is wrong.
>
> 3) As far as I understand, BT packets may be re-transmitted and therefore
> delivered out
> of order. Decrementing the read index while incrementing the system time
> will confuse the
> smoother, so if the index of the packet received is smaller than the current
> index, it should
> not be used. Due to the wrap-around of the RTP timestamp, this may be
> difficult to detect.
>
> Given the problems arising from the use of the RTP timestamp I wonder if we
> should
> use it at all. Or maybe we should use it to discard out-of-order packets,
> though as said
> above it is difficult to detect.
Thank you for comments. Seems that it is really not so obvious how to
correctly process RTP packets in bluetooth A2DP transfer. So I would
rater drop this patch for now and wait until we decide and come up with
the way how to properly handle RTP packets via bluetooth.
--
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
