On Sun, 10 Nov 2019, at 2:24 PM, Pali Rohár wrote:
> 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.
As a note, I've seen weird RTP timestamps from some devices (iPhones, iirc), so these timestamps are generally not reliable in my experience. Maybe we find a use for them at some point.
Cheers,
Arun
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
