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