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
