[pulseaudio-discuss] [PATCH 52/56] bluetooth: Create I/O thread function for BlueZ 5 cards

[Tanu Kaskinen]

On Thu, 2013-07-25 at 12:29 +0300, Tanu Kaskinen wrote:
> > +/* Run from IO thread */
> > +static int a2dp_process_push(struct userdata *u) {
> > +    int ret = 0;
> > +    pa_memchunk memchunk;
> > +
> > +    pa_assert(u);
> > +    pa_assert(u->profile == PROFILE_A2DP_SOURCE);
> > +    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 (;;) {
> > +        bool found_tstamp = false;
> > +        pa_usec_t tstamp;
> > +        struct sbc_info *sbc_info;
> > +        struct rtp_header *header;
> > +        struct rtp_payload *payload;
> > +        const void *p;
> > +        void *d;
> > +        ssize_t l;
> > +        size_t to_write, to_decode;
> > +
> > +        a2dp_prepare_buffer(u);
> > +
> > +        sbc_info = &u->sbc_info;
> > +        header = sbc_info->buffer;
> > +        payload = (struct rtp_payload*) ((uint8_t*) sbc_info->buffer + sizeof(*header));
> > +
> > +        l = pa_read(u->stream_fd, sbc_info->buffer, sbc_info->buffer_size, &u->stream_write_type);
> 
> The code assumes later that what we read here is exactly one complete
> rtp packet, nothing more, nothing less. I don't understand how such
> assumption can be done. I think we should parse the rtp header to find
> the packet boundary, and buffer any incomplete packets for later
> reading. Or perhaps recvmsg() should be used (I guess that's needed also
> for reading the timestamp, which is currently a TODO item).

Correction: I don't think we need recvmsg() for reading the timestamp,
because I don't know what the socket timestamp would be useful for. The
RTP timestamp is what matters.

> Phew, this one took long.

I forgot to mention: I would like these two comments to be added on top
of a2dp_process_render() and a2dp_process_push(), respectively:

/* This function renders write_block_size amount of data, encodes it and writes
 * it to stream_fd. The return value can be either 1, 0 or -1. 1 means
 * a successful write. 0 means EAGAIN, so nothing was written, but the caller
 * should retry after polling. -1 means error, and the caller is expected to
 * release the transport and reset write_memchunk and write_index.
 *
 * If write_block_size changes when write_memchunk.memblock is non-NULL (i.e.
 * after EAGAIN has occurred), write_memchunk must be reset, because this
 * function assumes that the memchunk length is exactly write_block_size!
 * FIXME: Resetting write_memchunk causes audio loss, so some other way to
 * handle this would be preferable. */

/* This function reads as much data from stream_fd as possible
 * (sbc_info.buffer_size at most, though). The read data is assumed to be
 * exactly one complete RTP packet (FIXME: invalid assumption). The read RTP
 * packet is decoded and posted to the source. The return value can be either
 * the number of bytes that were read from stream_fd, 0 or -1. 0 means EAGAIN,
 * so nothing was read, but the caller should retry after polling. -1 means
 * error, and the caller is expected to release the transport and reset
 * read_index and read_smoother. */

-- 
Tanu