[pulseaudio-discuss] [PATCH v13 01/10] bluetooth: Implement reading SO_TIMESTAMP for A2DP source

[Georg Chini]

On 10.11.19 09:44, Pali Rohár wrote:
> On Saturday 09 November 2019 12:34:02 Georg Chini wrote:
>> On 06.10.19 19:58, Pali Rohár wrote:
>>> ---
>>>    src/modules/bluetooth/module-bluez5-device.c | 33 ++++++++++++++++++++++++++--
>>>    1 file changed, 31 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
>>> index cff1cd6f2..9a81f4c69 100644
>>> --- a/src/modules/bluetooth/module-bluez5-device.c
>>> +++ b/src/modules/bluetooth/module-bluez5-device.c
>>> @@ -548,13 +548,29 @@ static int a2dp_process_push(struct userdata *u) {
>>>        a2dp_prepare_decoder_buffer(u);
>>>        for (;;) {
>>> +        uint8_t aux[1024];
>>> +        struct iovec iov;
>>> +        struct cmsghdr *cm;
>>> +        struct msghdr m;
>>>            bool found_tstamp = false;
>>>            pa_usec_t tstamp;
>>>            uint8_t *ptr;
>>>            ssize_t l;
>>>            size_t processed;
>>> -        l = pa_read(u->stream_fd, u->decoder_buffer, u->decoder_buffer_size, &u->stream_write_type);
>>> +        pa_zero(m);
>>> +        pa_zero(aux);
>>> +        pa_zero(iov);
>>> +
>>> +        m.msg_iov = &iov;
>>> +        m.msg_iovlen = 1;
>>> +        m.msg_control = aux;
>>> +        m.msg_controllen = sizeof(aux);
>>> +
>>> +        iov.iov_base = u->decoder_buffer;
>>> +        iov.iov_len = u->decoder_buffer_size;
>>> +
>>> +        l = recvmsg(u->stream_fd, &m, 0);
>>>            if (l <= 0) {
>>> @@ -574,8 +590,21 @@ static int a2dp_process_push(struct userdata *u) {
>>>            pa_assert((size_t) l <= u->decoder_buffer_size);
>>>            /* TODO: get timestamp from rtp */
>> You should remove the TODO from the comment.
> Why? This comment has nothing to do with patch in this email.
>
> This patch does *not* implement reading timestamp from RTP packet. It
> reads SO_TIMESTAMP from socket added by kernel.

I thought this referred to reading the SO_TIMESTAMP, sorry.

>
>>> +
>>> +        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);
>>> +                pa_rtclock_from_wallclock(tv);
>>> +                tstamp = pa_timeval_load(tv);
>>> +                found_tstamp = true;
>>> +                break;
>>> +            }
>>> +        }
>>> +
>>>            if (!found_tstamp) {
>>> -            /* pa_log_warn("Couldn't find SO_TIMESTAMP data in auxiliary recvmsg() data!"); */
>>> +            PA_ONCE_BEGIN {
>>> +                pa_log_warn("Couldn't find SO_TIMESTAMP data in auxiliary recvmsg() data!");
>>> +            } PA_ONCE_END;
>>>                tstamp = pa_rtclock_now();
>>>            }
>> Otherwise looks good, though I wonder if the warning is really necessary.
> It was there before, so I have not deleted it. It is also there for SCO
> socket.

I know, but I am actually for dropping the warning in both cases.
Given the precision of the smoother, it should not matter much
if the current time or the  time when the packet was received
are used in the calculations. Also, if SO_TIMESTAMP is not
supported, we can't do anything about it and we get a warning
when the socket is created.

>
>> It should run fine even when system time is used.
> Maybe kernel can for some reason do not add it? I do not know.
>