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
