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 |
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. 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 |
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 |
Free forum by Nabble | Edit this page |