[pulseaudio-discuss] [RFCv0 03/21] bluetooth: Add basic support for HEADSET profiles

[Tanu Kaskinen]

On Tue, 2014-02-04 at 19:03 -0300, jprvita at gmail.com wrote:
> From: João Paulo Rechi Vita <jprvita at openbossa.org>
> 
> This commit adds basic support for devices implementing HSP Headset
> Unit, HSP Audio Gateway, HFP Handsfree Unit, HFP Audio Gateway to the
> BlueZ 5 bluetooth audio devices driver module (module-bluez5-device).
> ---
>  src/modules/bluetooth/bluez5-util.c          |   4 +
>  src/modules/bluetooth/bluez5-util.h          |   6 +
>  src/modules/bluetooth/module-bluez5-device.c | 439 ++++++++++++++++++++-------
>  3 files changed, 341 insertions(+), 108 deletions(-)

Finally I got around to reviewing these... The first two patches looked
good, so I applied them. I have some change suggestions for this patch,
but there were no big issues.

>  /* Run from IO thread */
> +static int sco_process_render(struct userdata *u) {
> +    int ret = 0;
> +
> +    pa_assert(u);
> +    pa_assert(u->profile == PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT || u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY);
> +    pa_assert(u->sink);
> +
> +    /* First, render some data */
> +    if (!u->write_memchunk.memblock)
> +        pa_sink_render_full(u->sink, u->write_block_size, &u->write_memchunk);

You don't need to use u->write_memchunk in this function, because on
success you will always reset the memchunk, and on failure the whole
sink will be torn down. u->write_memchunk doesn't carry any meaningful
state information. Using a local variable makes the code easier to
understand.

> +
> +    pa_assert(u->write_memchunk.length == u->write_block_size);
> +
> +    for (;;) {
> +        ssize_t l;
> +        const void *p;
> +
> +        /* Now write that data to the socket. The socket is of type
> +         * SEQPACKET, and we generated the data of the MTU size, so this
> +         * should just work. */
> +
> +        p = (const uint8_t *) pa_memblock_acquire_chunk(&u->write_memchunk);
> +        l = pa_write(u->stream_fd, p, u->write_memchunk.length, &u->stream_write_type);
> +        pa_memblock_release(u->write_memchunk.memblock);
> +
> +        pa_assert(l != 0);
> +
> +        if (l < 0) {
> +
> +            if (errno == EINTR)
> +                /* Retry right away if we got interrupted */
> +                continue;
> +
> +            else if (errno == EAGAIN)
> +                /* Hmm, apparently the socket was not writable, give up for now */
> +                break;
> +
> +            pa_log_error("Failed to write data to SCO socket: %s", pa_cstrerror(errno));
> +            ret = -1;
> +            break;
> +        }

I'd prefer to end the for loop already here. If we get past this point,
it will be the last iteration of the loop anyway.

> +
> +        pa_assert((size_t) l <= u->write_memchunk.length);
> +
> +        if ((size_t) l != u->write_memchunk.length) {
> +            pa_log_error("Wrote memory block to socket only partially! %llu written, wanted to write %llu.",
> +                        (unsigned long long) l,
> +                        (unsigned long long) u->write_memchunk.length);
> +            ret = -1;
> +            break;
> +        }
> +
> +        u->write_index += (uint64_t) u->write_memchunk.length;
> +        pa_memblock_unref(u->write_memchunk.memblock);
> +        pa_memchunk_reset(&u->write_memchunk);
> +
> +        ret = 1;
> +        break;
> +    }
> +
> +    return ret;
> +}
> +
> +/* Run from IO thread */
> +static int sco_process_push(struct userdata *u) {
> +    int ret = 0;
> +    pa_memchunk memchunk;
> +
> +    pa_assert(u);
> +    pa_assert(u->profile == PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT || u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY);
> +    pa_assert(u->source);
> +    pa_assert(u->read_smoother);
> +
> +    memchunk.memblock = pa_memblock_new(u->core->mempool, u->read_block_size);
> +    memchunk.index = memchunk.length = 0;
> +
> +    for (;;) {
> +        ssize_t l;
> +        void *p;
> +        struct msghdr m;
> +        struct cmsghdr *cm;
> +        uint8_t aux[1024];
> +        struct iovec iov;
> +        bool found_tstamp = false;
> +        pa_usec_t tstamp;
> +
> +        memset(&m, 0, sizeof(m));
> +        memset(&aux, 0, sizeof(aux));
> +        memset(&iov, 0, sizeof(iov));

You could use pa_zero() here.

> +
> +        m.msg_iov = &iov;
> +        m.msg_iovlen = 1;
> +        m.msg_control = aux;
> +        m.msg_controllen = sizeof(aux);
> +
> +        p = pa_memblock_acquire(memchunk.memblock);
> +        iov.iov_base = p;
> +        iov.iov_len = pa_memblock_get_length(memchunk.memblock);
> +        l = recvmsg(u->stream_fd, &m, 0);
> +        pa_memblock_release(memchunk.memblock);
> +
> +        if (l <= 0) {
> +
> +            if (l < 0 && errno == EINTR)
> +                /* Retry right away if we got interrupted */
> +                continue;
> +
> +            else if (l < 0 && errno == EAGAIN)
> +                /* Hmm, apparently the socket was not readable, give up for now. */
> +                break;
> +
> +            pa_log_error("Failed to read data from SCO socket: %s", l < 0 ? pa_cstrerror(errno) : "EOF");
> +            ret = -1;
> +            break;
> +        }

The for loop could end here.

-- 
Tanu