[Bug 641072] [baseaudiosink] Improve latency-time handling and optionally aggregate buffers until latency-time is reached

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=641072
  GStreamer | gst-plugins-base | 0.10.x

--- Comment #9 from David Henningsson <david.henningsson at canonical.com> 2011-08-18 13:30:11 UTC ---
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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.