[pulseaudio-discuss] [PATCH v2 1/4] pulse: Fix hole handling in pa_stream_peek().

[David Henningsson]

On 11/07/2012 03:52 PM, Tanu Kaskinen wrote:
> Previously, if there was a hole in a recording stream,
> pa_stream_peek() would crash. Holes could be handled silently inside
> pa_stream_peek() by generating silence (wouldn't work for compressed
> streams, though) or by skipping any holes. However, I think it's
> better to let the caller decide how the holes should be handled, so
> in case of holes, pa_stream_peek() will return NULL data pointer and
> the length of the hole in the nbytes argument.
>
> This change is technically an interface break, because previously the
> documentation didn't mention the possibility of holes that need
> special handling. However, since holes caused crashing anyway in the
> past, it's not a regression if applications keep misbehaving due to
> not handing holes properly.
>
> Some words about when holes can appear in recording streams: I think
> it would be reasonable behavior if overruns due to the application
> reading data too slowly would cause holes. Currently that's not the
> case - overruns will just cause audio to be skipped. But the point is
> that this might change some day. I'm not sure how holes can occur
> with the current code, but as the linked bug shows, they can happen.
> It's most likely due to recording from a monitor source where the
> thing being monitored has holes in its playback stream.
>
> BugLink: http://bugs.launchpad.net/bugs/1058200

This one reviewed and applied.

> ---
>   src/pulse/stream.c |   20 ++++++++++++++++----
>   src/pulse/stream.h |   20 +++++++++++++++-----
>   2 files changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/src/pulse/stream.c b/src/pulse/stream.c
> index 2b6d306..f692d37 100644
> --- a/src/pulse/stream.c
> +++ b/src/pulse/stream.c
> @@ -1593,9 +1593,17 @@ int pa_stream_peek(pa_stream *s, const void **data, size_t *length) {
>       if (!s->peek_memchunk.memblock) {
>
>           if (pa_memblockq_peek(s->record_memblockq, &s->peek_memchunk) < 0) {
> +            /* record_memblockq is empty. */
>               *data = NULL;
>               *length = 0;
>               return 0;
> +
> +        } else if (!s->peek_memchunk.memblock) {
> +            /* record_memblockq isn't empty, but it doesn't have any data at
> +             * the current read index. */
> +            *data = NULL;
> +            *length = s->peek_memchunk.length;
> +            return 0;
>           }
>
>           s->peek_data = pa_memblock_acquire(s->peek_memchunk.memblock);
> @@ -1614,7 +1622,7 @@ int pa_stream_drop(pa_stream *s) {
>       PA_CHECK_VALIDITY(s->context, !pa_detect_fork(), PA_ERR_FORKED);
>       PA_CHECK_VALIDITY(s->context, s->state == PA_STREAM_READY, PA_ERR_BADSTATE);
>       PA_CHECK_VALIDITY(s->context, s->direction == PA_STREAM_RECORD, PA_ERR_BADSTATE);
> -    PA_CHECK_VALIDITY(s->context, s->peek_memchunk.memblock, PA_ERR_BADSTATE);
> +    PA_CHECK_VALIDITY(s->context, s->peek_memchunk.length > 0, PA_ERR_BADSTATE);
>
>       pa_memblockq_drop(s->record_memblockq, s->peek_memchunk.length);
>
> @@ -1622,9 +1630,13 @@ int pa_stream_drop(pa_stream *s) {
>       if (s->timing_info_valid && !s->timing_info.read_index_corrupt)
>           s->timing_info.read_index += (int64_t) s->peek_memchunk.length;
>
> -    pa_assert(s->peek_data);
> -    pa_memblock_release(s->peek_memchunk.memblock);
> -    pa_memblock_unref(s->peek_memchunk.memblock);
> +    if (s->peek_memchunk.memblock) {
> +        pa_assert(s->peek_data);
> +        s->peek_data = NULL;
> +        pa_memblock_release(s->peek_memchunk.memblock);
> +        pa_memblock_unref(s->peek_memchunk.memblock);
> +    }
> +
>       pa_memchunk_reset(&s->peek_memchunk);
>
>       return 0;
> diff --git a/src/pulse/stream.h b/src/pulse/stream.h
> index b4464fa..a6785ec 100644
> --- a/src/pulse/stream.h
> +++ b/src/pulse/stream.h
> @@ -534,11 +534,21 @@ int pa_stream_write(
>           pa_seek_mode_t seek      /**< Seek mode, must be PA_SEEK_RELATIVE for upload streams */);
>
>   /** Read the next fragment from the buffer (for recording streams).
> - * \a data will point to the actual data and \a nbytes will contain the size
> - * of the data in bytes (which can be less or more than a complete
> - * fragment). Use pa_stream_drop() to actually remove the data from
> - * the buffer. If no data is available this will return a NULL
> - * pointer. */
> + * If there is data at the current read index, \a data will point to
> + * the actual data and \a nbytes will contain the size of the data in
> + * bytes (which can be less or more than a complete fragment).
> + *
> + * If there is no data at the current read index, it means that either
> + * the buffer is empty or it contains a hole (that is, the write index
> + * is ahead of the read index but there's no data where the read index
> + * points at). If the buffer is empty, \a data will be NULL and
> + * \a nbytes will be 0. If there is a hole, \a data will be NULL and
> + * \a nbytes will contain the length of the hole.
> + *
> + * Use pa_stream_drop() to actually remove the data from the buffer
> + * and move the read index forward. pa_stream_drop() should not be
> + * called if the buffer is empty, but it should be called if there is
> + * a hole. */
>   int pa_stream_peek(
>           pa_stream *p                 /**< The stream to use */,
>           const void **data            /**< Pointer to pointer that will point to data */,
>



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic