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

[Tanu Kaskinen]

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
---
 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 */,
-- 
1.7.10.4