Hi! On Mon, Nov 28, 2016 at 09:04:01AM +0100, David Henningsson wrote: > > On 2016-11-28 05:01, Ahmed S. Darwish wrote: > > Current pacat code reads whatever available from STDIN and writes > > it directly to the playback stream. A minimal buffer is created > > for each read operation; no further reads are then allowed unless > > earlier read buffer has been fully consumed by a stream write. > > > > While quite simple, this model breaks upon the new requirements of > > writing only frame-aligned data to the stream (commits #1 and #2). > > The kernel read syscall can return a length much smaller than the > > frame-aligned size requested, leading to invalid unaligned writes. > > Sorry, but I don't understand why you need a ringbuffer to resolve this? > Hmm.. - Ringbuffer length is frame-size aligned - Consumption from that buffer only occurs in frame-sized units - Partial frames are completed by subsequent STDIN reads Thus all stream writes have a length that is frame-size aligned, which is the problem that needs to be fixed. It's just classical buffering between a producer (STDIN callback) and a consumer (stream write callback). > And now that I think of it, I'm not sure that the pa_xmalloc (used to > allocated the ringbuffer's memory) it guaranteed to be aligned either, it > just happens to be that way in practice. > What was meant is the _size_ of each pa_stream_write() to be aligned, rather than the alignment of the write buffer address. How important is buffer address alignment I honesly don't know. The original code also buffered audio through pa_xmalloc though. > Wouldn't it be better if we had something like: > By better, you mean simpler? > 1) Call pa_stream_begin_write to get a buffer. > 2) If we have half a frame from previous iteration at 4), put that half > frame first in the buffer. > 3) Call read / pa_read to read data from a file into the rest of the > buffer. > 4) If the last frame read is not complete, store that half frame in a local > variable. > 5) Call pa_stream_write with the number of complete frames. > 6) Repeat from 1). > Hmm, didn't know about pa_stream_begin_write() before; it seems to offer zero-copy benefits, especially in the SHM case, too. The above implies that "stream_write_callback()" will not do any actual writes though. That's because nothing will be buffered by that point (no actual buffers), and we cannot do any read() or we will risk blocking, thus possible underruns. Correct? IMHO the logic complexity seems equivalent to the ringbuffer, especially that steps "2)" and "4)" are implicitly done by the ring anyway. I'll give it a try though; maybe it'll grow on me afterwards :D Thanks for the review! -- Darwish http://darwish.chasingpointers.com
