[pulseaudio-discuss] [PATCH v2] pacat: Write to stream in frame-sized chunks

[Tanu Kaskinen]

On Sat, 2016-12-17 at 01:43 +0200, Ahmed S. Darwish wrote:
> # Note: Documentation of `pa_stream_begin_write(stream, &buf, &nbytes)'
> # suggests to pass nbytes = -1. Doing so though lends to a huge returned
> # nbytes values. Calling a pa_stream_write() with such values chokes the
> # stream: making it go silent and not produce any further write-callback
> # events.
> #
> # Is this a problem in usage pattern, or a bug?

Sounds like a bug. Trying to write more than buffer_attr.maxlength
bytes at once would be expected to cause some kind of problems, but I
don't think that's the case here. The returned nbytes value is 64k,
isn't it? pacat uses the default maxlength, and the default maxlength
is 4M, so nbytes is nowhere near that limit. As long as maxlength isn't
exceeded, any "extra" audio should just get buffered in the stream
buffer at the server side.

>  /* 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, 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,18 @@ static void stdin_callback(pa_mainloop_api*a, pa_io_event *e, int fd, pa_io_even
>          stdio_event = NULL;
>          return;
>      }
> +    r += partialframe_len;
>  
> -    buffer_length = (uint32_t) r;
> -    buffer_index = 0;
> +    /* Cache any trailing partial frames for the next write */
> +    partialframe_len = r - pa_frame_align(r, &sample_spec);
> +    if (partialframe_len)
> +        memcpy(partialframe_buf, buf + r - partialframe_len, partialframe_len);
>  
> -    if (w)
> -        do_stream_write(w);
> +    if (pa_stream_write(stream, buf, r - partialframe_len, NULL, 0, PA_SEEK_RELATIVE) < 0) {
> +        pa_log(_("pa_stream_write() failed: %s"), pa_strerror(pa_context_errno(context)));
> +        quit(1);
> +        return;
> +    }

If we read less than a full frame, we can end up calling
pa_stream_write() with zero length. That's quite possibly ok, but it
makes me a bit nervous. I'd prefer cancelling the write if r -
partialframe_len equals zero.

-- 
Tanu

https://www.patreon.com/tanuk