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. This can easily be reproduced by choosing a starved STDIN backend: pacat /dev/random pa_stream_write() failed: EINVAL echo 1234 | pacat pa_stream_write() failed: EINVAL or by playing an incomplete WAV file in raw, non-paplay, mode. So guard against such incomplete kernel reads by writing only in frame-aligned sizes, while caching any trailing partial frame for subsequent writes. Other operation modes are not affected. Non-raw paplay playback is handled by libsndfile, ensuring complete reads, and recording mode just writes to the STDOUT fd without any special needs. CommitReference: 22827a5e1e62 CommitReference: 150ace90f380 BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=98475 BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=77595 Suggested-by: David Henningsson <diwic at ubuntu.com> Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com> --- src/utils/pacat.c | 98 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 53 insertions(+), 45 deletions(-) # # To test this patch, you can compare existing pacat with the # patched one in the following scenarios: # # First scenario: # # $ sudo apt-get install haveged # $ sudo systemctl start haveged # $ ./src/pacat /dev/random # $ pacat /dev/random # pa_stream_write() fail # # Second scenario: # # $ wget http://ccrma.stanford.edu/~jos/wav/violin2.wav # $ ./src/pacat violin2.wav # $ pacat violin2.wav # pa_stream_write() fail # # Third scenario: # # $ echo -n 123 | ./src/pacat # $ echo -n 123 | pacat # pa_stream_write() fail # # Fourth scenario # # # <-- just testing latency --> # $ ./src/pacat -r --latency-msec=4 | ./src/pacat --latency-msec=4 # # # v3 log: # # Apply Tanu's remarks; don't pa_stream_write() with zero len # diff --git a/src/utils/pacat.c b/src/utils/pacat.c index 68362ec..4e1bbfc 100644 --- a/src/utils/pacat.c +++ b/src/utils/pacat.c @@ -56,6 +56,23 @@ static pa_context *context = NULL; static pa_stream *stream = NULL; static pa_mainloop_api *mainloop_api = NULL; +/* Playback Mode (raw): + * + * We can only write audio to the PA stream in multiples of the stream's + * sample-spec frame size. Meanwhile, the STDIN read(2) system call can return + * a length much smaller than the frame-aligned size requested - leading to + * invalid writes. This can be reproduced by choosing a starved STDIN backend + * (e.g. "pacat /dev/random", "echo 1234 | pacat"), or an incomplete WAV file + * in raw non-paplay mode. + * + * Solve this by writing only frame-aligned sizes, while caching the resulting + * trailing partial frames here. This partial frame is then directly written + * in the next stream write iteration. Rinse and repeat. + */ +static void *partialframe_buf = NULL; +static size_t partialframe_len = 0; + +/* Recording Mode buffers */ static void *buffer = NULL; static size_t buffer_length = 0, buffer_index = 0; @@ -153,34 +170,6 @@ static void start_drain(void) { quit(0); } -/* Write some data to the stream */ -static void do_stream_write(size_t length) { - size_t l; - pa_assert(length); - - if (!buffer || !buffer_length) - return; - - l = length; - if (l > buffer_length) - l = buffer_length; - - if (pa_stream_write(stream, (uint8_t*) buffer + buffer_index, l, NULL, 0, PA_SEEK_RELATIVE) < 0) { - pa_log(_("pa_stream_write() failed: %s"), pa_strerror(pa_context_errno(context))); - quit(1); - return; - } - - buffer_length -= l; - buffer_index += l; - - if (!buffer_length) { - pa_xfree(buffer); - buffer = NULL; - buffer_index = buffer_length = 0; - } -} - /* This is called whenever new data may be written to the stream */ static void stream_write_callback(pa_stream *s, size_t length, void *userdata) { pa_assert(s); @@ -192,11 +181,6 @@ static void stream_write_callback(pa_stream *s, size_t length, void *userdata) { if (stdio_event) mainloop_api->io_enable(stdio_event, PA_IO_EVENT_INPUT); - if (!buffer) - return; - - do_stream_write(length); - } else { sf_count_t bytes; void *data; @@ -540,25 +524,34 @@ fail: /* New data on STDIN **/ static void stdin_callback(pa_mainloop_api*a, pa_io_event *e, int fd, pa_io_event_flags_t f, void *userdata) { - size_t l, w = 0; - ssize_t r; - bool stream_not_ready; + uint8_t *buf = NULL; + size_t writable, towrite, r; pa_assert(a == mainloop_api); pa_assert(e); pa_assert(stdio_event == e); - stream_not_ready = !stream || pa_stream_get_state(stream) != PA_STREAM_READY || - !(l = w = pa_stream_writable_size(stream)); + /* Stream not ready? */ + if (!stream || pa_stream_get_state(stream) != PA_STREAM_READY || + !(writable = pa_stream_writable_size(stream))) { - if (buffer || stream_not_ready) { mainloop_api->io_enable(stdio_event, PA_IO_EVENT_NULL); return; } - buffer = pa_xmalloc(l); + if (pa_stream_begin_write(stream, (void **)&buf, &writable) < 0) { + pa_log(_("pa_stream_begin_write() failed: %s"), pa_strerror(pa_context_errno(context))); + quit(1); + return; + } - if ((r = pa_read(fd, buffer, l, userdata)) <= 0) { + /* Partial frame cached from a previous write iteration? */ + if (partialframe_len) { + pa_assert(partialframe_len < pa_frame_size(&sample_spec)); + memcpy(buf, partialframe_buf, partialframe_len); + } + + if ((r = pa_read(fd, buf + partialframe_len, writable - partialframe_len, userdata)) <= 0) { if (r == 0) { if (verbose) pa_log(_("Got EOF.")); @@ -574,12 +567,23 @@ static void stdin_callback(pa_mainloop_api*a, pa_io_event *e, int fd, pa_io_even stdio_event = NULL; return; } + r += partialframe_len; + + /* Cache any trailing partial frames for the next write */ + towrite = pa_frame_align(r, &sample_spec); + partialframe_len = r - towrite; - buffer_length = (uint32_t) r; - buffer_index = 0; + if (partialframe_len) + memcpy(partialframe_buf, buf + towrite, partialframe_len); - if (w) - do_stream_write(w); + if (towrite) { + if (pa_stream_write(stream, buf, towrite, NULL, 0, PA_SEEK_RELATIVE) < 0) { + pa_log(_("pa_stream_write() failed: %s"), pa_strerror(pa_context_errno(context))); + quit(1); + return; + } + } else + pa_stream_cancel_write(stream); } /* Some data may be written to STDOUT */ @@ -1148,6 +1152,9 @@ int main(int argc, char *argv[]) { } } + if (raw && mode == PLAYBACK) + partialframe_buf = pa_xmalloc(pa_frame_size(&sample_spec)); + /* Set up a new main loop */ if (!(m = pa_mainloop_new())) { pa_log(_("pa_mainloop_new() failed.")); @@ -1229,6 +1236,7 @@ quit: pa_xfree(silence_buffer); pa_xfree(buffer); + pa_xfree(partialframe_buf); pa_xfree(server); pa_xfree(device); -- Darwish http://darwish.chasingpointers.com
