Bug 103103

If the write index is behind the read index by 30 seconds, writing 30 seconds of audio should succeed. The stream buffer size limit should only kick in after the write index has passed the read index. I read the code, but I didn't find any bug. can_push() in memblockq.c seems to handle the "write index behind read index" case correctly.

The behaviour that you describe sounds like pulsesink is writing the 30 second block at the current read index, not 30 seconds behind the read index. Can you do further debugging to verify what is really happening?

Created attachment 135110 [details]
Figure with write and read indexes with overflow marker

Created attachment 135111 [details]
Figure with write and read indexes with overflow marker (zooming area near 33s)

Created attachment 135112 [details]
An application, which should reproduce a problem

Scenario for the problem based on attached images:
a. when after 3 seconds of playback, the application stops provide data, writable_size increased due to request send by PA server,
b. when after 30 seconds, when the application wants to start provide data the pa_stream_writable_size() returns 2639040 bytes,
c. the application writes 2639040 bytes in less than 5 ms without pa_stream_write() error,
d. after 3 ms from last write, "overflow" notification callback is called for almost all earlier invocation of pa_stream_write (on my machine data for only 1.5 s is preserved, it is also effect that some of data are written to late),
e. due to "overflow" most of data are dropped, but due to already increased write_index, PA server will play silence and the application will be blocked due to no space left, till read index does not catch write index.

IMHO pa_stream_writable_size() does not return valid information in this case and is a source of later problems.

Configuration of buffer: pa_buffer_attr bufattr = {.maxlength = 122880, .tlength = 38400, .prebuf = 0, .minreq = 1920, .fragsize = 0}, mirrors configuration of GStreamer's pulseaudio element.

It seems that change presented below correct the problem. Verification and better understanding is needed.

diff --git a/src/pulsecore/memblockq.c b/src/pulsecore/memblockq.c
index b132dd3..1db4ec2 100644
--- a/src/pulsecore/memblockq.c
+++ b/src/pulsecore/memblockq.c
@@ -251,10 +251,13 @@ static void write_index_changed(pa_memblockq *bq, int64_t old_write_index, bool
 
     delta = bq->write_index - old_write_index;
 
-    if (account)
+    if (account) {
         bq->requested -= delta;
-    else
+    } else {
         bq->missing -= delta;
+       if (bq->missing < 0)
+          bq->missing = 0;
+    }

The problem is composition of overflow (due to huge value returned by pa_stream_writable_size()) and next underflow. The previous comment treated underflow. The overflow can be addressed by change:

diff --git a/src/pulse/stream.c b/src/pulse/stream.c
index ee95757..87c5f9d 100644
--- a/src/pulse/stream.c
+++ b/src/pulse/stream.c
@@ -842,6 +842,9 @@ void pa_command_request(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tag
 #ifdef STREAM_DEBUG
     pa_log_debug("got request for %lli, now at %lli", (long long) bytes, (long long) s->requested_bytes);
 #endif
+    if (s->buffer_attr.tlength != -1)
+        if (s->requested_bytes > s->buffer_attr.tlength)
+            s->requested_bytes = s->buffer_attr.tlength;
 
     if (s->requested_bytes > 0 && s->write_callback)
         s->write_callback(s, (size_t) s->requested_bytes, s->write_userdata);

Thanks for the investigation, especially for the test program! I'll look into this. I can't say right away whether your changes are good or not, I don't know this stuff that well. To me it seems that there should be no overflow in the first place.

Overflow will be a result of pa_stream_write() calls to "catch" value returned by pa_stream_writable_size(), after not providing data for 30 s. From documentation of pa_stream_writeable_size(): "It is possible to write more than this amount, up to the stream's buffer_attr.maxlength bytes. This is usually not desirable, though, as it would increase stream latency to be higher than requested (buffer_attr.tlength)" it seems that pa_stream_writable_size() should always be less than buffer_attr.maxlength (for != -1 case).

Currently attached test program does not correctly calculate offset after long gap (the program starts to write in offset, which was already played). It is enough to see problem with overflow (please see attached png image), but has to be fixed to show that my "fixes" can fix something. I will try to provide fixed program in near future.

The documentation of pa_stream_writable_size() is probably inaccurate. The only limit that I'm aware of is that the write index can't go further than buffer_attr.maxlength ahead of the read index (or it can, but silence will be inserted instead of real audio data). If the write index is behind the read index, then it should be possible to write more than buffer_attr.maxlength.

Created attachment 135231 [details]
Second version of application, which should reproduce a problem

Please find second version of application, which should reproduce a problem. The application should also show that after apply two changes from previous comments, the problem should be resolved.

From my point of view, the problem is resolved if I apply proposed changes in comments above. Also limitation of writable size to stream's tlength can be loosen up to maxlength (if silence was played, buffer should be empty).

If you haven't objections, I would like to send a patch or patches (if modification of stream.c and memblockq.c should be in separate patches).

I played with the test program a bit, and I noticed that after the pause the offset is increased. Based on the earlier discussion, I was expecting that the program would continue writing where it left off. If I remove the offset incrementing, then there are no overflows, even though the program writes more than maxlength in one go.

Skipping ahead is a reasonable thing that an application might do after such pause, but then it must not write the full amount that pa_stream_writable_size() reported. However, for some reason when I try to implement this by decreasing length by delta, overflows keep happening. I'll keep investigating.

Hmm, now I don't get the overflows. I don't think I changed anything. Maybe I forgot to compile my changed version in the previous test?

At the moment it looks like there's no bug in PulseAudio.

(In reply to Tanu Kaskinen from comment #14)
> Hmm, now I don't get the overflows. I don't think I changed anything.

Sorry, I actually did change something - I disabled the jumping forwards code again, and somehow forgot that I did that. The unexpected overflows are back.

I found out why the overruns happen even in the "fixed" version. Adjusting the write amount in write_samples() to compensate for the jump doesn't help, because pa_stream_write() subtracts only the write length from the internal requested_bytes variable. The jump is not taken into account, so pa_stream_writable_size() will keep returning non-zero values even though in reality the buffer has been filled completely.

pa_stream_write() actually has a comment about this:

    /* This is obviously wrong since we ignore the seeking index . But
     * that's OK, the server side applies the same error */
    s->requested_bytes -= (seek == PA_SEEK_RELATIVE ? offset : 0) + (int64_t) length;

I don't know why Lennart didn't bother fixing this properly in https://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=373b5efe51238b0ad34cb9a9d8fc61b973afdad8

So in conclusion: the requested_bytes tracking needs to be fixed at the client side, and judging from the comment, apparently there's a similar problem at the server side.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/pulseaudio/pulseaudio/issues/51.