[pulseaudio-commits] src/pulse

[David Henningsson]

 src/pulse/stream.c |   20 ++++++++++++++++----
 src/pulse/stream.h |   20 +++++++++++++++-----
 2 files changed, 31 insertions(+), 9 deletions(-)

New commits:
commit dfd44036b54d65314664622ff93dfd18eee03c7b
Author: Tanu Kaskinen <tanuk at iki.fi>
Date:   Wed Nov 7 16:52:37 2012 +0200

    pulse: Fix hole handling in pa_stream_peek().
    
    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

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