[PATCH] Allow pulseaudio writes in larger chunks

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Allow pulseaudio writes in larger chunks

David Henningsson
The previous patch (posted several months ago) for improving pulsesink
writes was rejected, due to causing underruns when playing wma files.
I've been able to reproduce that error here, and have come up with a new
version of the patch which does not underruns in that case.

In short, the behavioural difference is
  - without the patch, waiting and write size were both never above segsize
  - with the old patch, both waiting and write size could grow up to
bufsize.
  - with the new patch, waiting is done up to segsize. However when
waiting is done, it writes as much as it can.

Also, the previous patch was blamed to "ignore the latency-time" of the
sink. This statement is false, at least for the new version of the
patch. The latency is controlled by setting tlength and minreq
parameters of pa_buffer_attr, which in turn controls the behaviour of
pa_stream_writable_size.

Let me go through the patch quickly for you:

 > diff --git a/ext/pulse/pulsesink.c b/ext/pulse/pulsesink.c
 > index c9f0b58..b90ebe3 100644
 > --- a/ext/pulse/pulsesink.c
 > +++ b/ext/pulse/pulsesink.c
 > @@ -1430,9 +1430,7 @@ gst_pulseringbuffer_commit (GstRingBuffer *
buf, guint64 * sample,
 >
 >      towrite = out_samples * bps;
 >
 > -    /* Only ever write segsize bytes at once. This will
 > -     * also limit the PA shm buffer to segsize
 > -     */
 > +    /* Wait for at least segsize bytes to become available */
 >      if (towrite > buf->spec.segsize)
 >        towrite = buf->spec.segsize;

Just change the comment to adhere to the new behaviour.

 > @@ -1481,7 +1479,7 @@ gst_pulseringbuffer_commit (GstRingBuffer *
buf, guint64 * sample,
 >             goto uncork_failed;
 >         }
 >
 > -        /* we can't write a single byte, wait a bit */
 > +        /* we can't write segsize bytes, wait a bit */
 >          GST_LOG_OBJECT (psink, "waiting for free space");
 >          pa_threaded_mainloop_wait (mainloop);

Comment clarification.

 > @@ -1489,14 +1487,10 @@ gst_pulseringbuffer_commit (GstRingBuffer *
buf, guint64 * sample,
 >            goto was_paused;
 >        }
 >
 > -      /* make sure we only buffer up latency-time samples */
 > -      if (pbuf->m_writable > buf->spec.segsize) {
 > -        /* limit buffering to latency-time value */
 > -        pbuf->m_writable = buf->spec.segsize;
 > -
 > -        GST_LOG_OBJECT (psink, "Limiting buffering to %" >
G_GSIZE_FORMAT,
 > -            pbuf->m_writable);
 > -      }

So this is what's essentially wrong - there is no extra "buffering"
being done here (despite the comment), as the samples are already in a
buffer when they're coming into this function. This limit is what
worsens performance, so remove it.

 > +      /* Recalculate what we can write in the next chunk */
 > +      towrite = out_samples * bps;
 > +      if (pbuf->m_writable > towrite)
 > +          pbuf->m_writable = towrite;
 >
 >        GST_LOG_OBJECT (psink, "requesting %" G_GSIZE_FORMAT " bytes of "
 >            "shared memory", pbuf->m_writable);

Instead limit what we write to what's left to write in this round.

 > @@ -1510,14 +1504,9 @@ gst_pulseringbuffer_commit (GstRingBuffer *
buf, guint64 * sample,
 >        GST_LOG_OBJECT (psink, "got %" G_GSIZE_FORMAT " bytes of
shared memory",
 >            pbuf->m_writable);
 >
 > -      /* Just to make sure that we didn't get more than requested */
 > -      if (pbuf->m_writable > buf->spec.segsize) {
 > -        /* limit buffering to latency-time value */
 > -        pbuf->m_writable = buf->spec.segsize;
 > -      }
 >      }

The same write limit stuff is being done twice, remove it here as well.

 >
 > -    if (pbuf->m_writable < towrite)
 > +    if (towrite > pbuf->m_writable)
 >        towrite = pbuf->m_writable;
 >      avail = towrite / bps;

Just more readable/common semantic IMO.

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

_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

0001-pulse-Allow-writes-in-bigger-chunks.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Allow pulseaudio writes in larger chunks

Sebastian Dröge-7
On Thu, 2011-08-18 at 14:07 +0200, David Henningsson wrote:

> The previous patch (posted several months ago) for improving pulsesink
> writes was rejected, due to causing underruns when playing wma files.
> I've been able to reproduce that error here, and have come up with a new
> version of the patch which does not underruns in that case.
>
> In short, the behavioural difference is
>   - without the patch, waiting and write size were both never above segsize
>   - with the old patch, both waiting and write size could grow up to
> bufsize.
>   - with the new patch, waiting is done up to segsize. However when
> waiting is done, it writes as much as it can.
>
> Also, the previous patch was blamed to "ignore the latency-time" of the
> sink. This statement is false, at least for the new version of the
> patch. The latency is controlled by setting tlength and minreq
> parameters of pa_buffer_attr, which in turn controls the behaviour of
> pa_stream_writable_size.
Hi,

thanks for still working on this but could you put the patch in
Bugzilla, including your explanation of the various parts of the patch?

_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel

signature.asc (205 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Allow pulseaudio writes in larger chunks

David Henningsson
On 08/18/2011 02:46 PM, Sebastian Dröge wrote:

> On Thu, 2011-08-18 at 14:07 +0200, David Henningsson wrote:
>> The previous patch (posted several months ago) for improving pulsesink
>> writes was rejected, due to causing underruns when playing wma files.
>> I've been able to reproduce that error here, and have come up with a new
>> version of the patch which does not underruns in that case.
>>
>> In short, the behavioural difference is
>>    - without the patch, waiting and write size were both never above segsize
>>    - with the old patch, both waiting and write size could grow up to
>> bufsize.
>>    - with the new patch, waiting is done up to segsize. However when
>> waiting is done, it writes as much as it can.
>>
>> Also, the previous patch was blamed to "ignore the latency-time" of the
>> sink. This statement is false, at least for the new version of the
>> patch. The latency is controlled by setting tlength and minreq
>> parameters of pa_buffer_attr, which in turn controls the behaviour of
>> pa_stream_writable_size.
>
> Hi,
>
> thanks for still working on this but could you put the patch in
> Bugzilla, including your explanation of the various parts of the patch?

Okay, now added to https://bugzilla.gnome.org/show_bug.cgi?id=641072

I hope this is enough to get "the ball rolling" - i e reviewing by
relevant people, and finally committing it (hopefully).

--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
_______________________________________________
gstreamer-devel mailing list
[hidden email]
http://lists.freedesktop.org/mailman/listinfo/gstreamer-devel