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

[Pali Rohár]

On Sunday 10 November 2019 11:03:25 Georg Chini wrote:
> 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.

Yes.

> Also, if SO_TIMESTAMP is not
> supported, we can't do anything about it and we get a warning
> when the socket is created.

That is true, it is only warning.

But I think it has a value for debugging purposes. It could be possible
that today or future there code can work quite differently and if user
provide logs with above warning it could be easier to debug problems...

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

-- 
Pali Rohár
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/pulseaudio-discuss/attachments/20191110/c9eb11f7/attachment.sig>