Comment on attachment 179744 [details] [review]
Patch against git head

Thanks for the patch!

commit 1e2c1467ae042a3c6bb1a6bc0c07aeff13ec5edb
Author: David Henningsson <david.henningsson@canonical.com>
Date:   Mon Jan 31 05:58:36 2011 +0100

    Pulsesink: Allow chunks up to bufsize instead of segsize
    
    By allowing larger chunks to be sent, PulseAudio will have a
    lower CPU usage. This is especially important on low-end machines,
    where PulseAudio can crash if packets are coming in at a higher
    rate than PulseAudio can process them.
    
    Signed-off-by: David Henningsson <david.henningsson@canonical.com>

Doesn't this change make pulsesink ignore the "latency-time" property of baseaudiosink? Just choosing a higher value for latency-time would fix this bug too

(In reply to comment #3)
> Doesn't this change make pulsesink ignore the "latency-time" property of
> baseaudiosink? Just choosing a higher value for latency-time would fix this bug
> too

Yes, you are absolutely right. That was not visible from the patch (lack of context), but
  bufsize = buf->spec.segsize * buf->spec.segtotal;

What we should do instead is to configure the buffer- and latency-time from the high-level bins (and obviously revert the patch):
- playbin2 can set long buffer-time (1 sec) and use low number of segments (2-4).
- rtpbin would consigure a low-latency time and use 2 segments (buffer-time=2*latency-time)

I mark this as a blocker in oder to get consensus before next release.

Hrmm. I don't know GStreamer internals that well, but still, we never send more than has been created by the caller to gst_pulseringbuffer_commit. And if that is more than "latency-time", the caller has obviously already ignored latency-time. 

There is no gain in not sending what we already have created, right? So we shouldn't care about latency-time here, but earlier in the chain. 

Btw, I do agree that 10 ms is terribly low for a music player and that 1 sec or so would be more appropriate.

Collecting multiple buffers and only sending them to PA all together decreases the number of context switches

I'm going to revert the patch for this release. What should be done is what Stefan said, set the buffer/latency times on the elements. And maybe baseaudiosink/audiosink could have another property to always collect latency-time samples before pushing them to the sink and aggregate buffers if upstream provided smaller buffers.

I'll reassign this bug to -base and let's continue discussion then.

FYI, this bug is related to the changes I suggested in #641544.
I think we all agree that the default buffer sizes aren't great for music playback and could be increased.
However as Arun pointed out, increasing buffer sizes up to the default pulseaudio 2s has pretty nasty side effects on existing applications. Totem takes forever to pause and Rhythmbox ends-up crapping out... Instead of forcing a large buffer in playbin2 we may want the application to use a profile to indirectly set theses values.

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.

Created attachment 194130 [details] [review]
Patch v2 - allow bigger writes

commit e70020b45630d99132a99892ecc6e1dcd1ce6f8a
Author: David Henningsson <david.henningsson@canonical.com>
Date:   Thu Aug 18 13:37:39 2011 +0200

    pulsesink: Allow writes in bigger chunks
    
    There's no use in splitting the incoming data down to the segsize
    limit - by writing as much as possible in one chunk, we increase
    performance and avoid PulseAudio unnecessary rewinds.
    
    Signed-off-by: David Henningsson <david.henningsson@canonical.com>

I think there's still something to do with this bug in baseaudiosink, right?

I think this is really two separate bugs in one:

1) The pulsesink writing is inoptimal in that it splits packets when it's not needed, which is now fixed in my patch

2) The default buffer sizes for music playback should be larger, for better performance in general.

(In reply to comment #13)
> I think this is really two separate bugs in one:
> 
> 1) The pulsesink writing is inoptimal in that it splits packets when it's not
> needed, which is now fixed in my patch
> 
> 2) The default buffer sizes for music playback should be larger, for better
> performance in general.

*bashes at bugzilla for making some strange unintended hyperlink*

Is this still an issue with current git ? If so maybe Arun could have a look ?

This can be closed now. David's aggregation patch has been merged a while back, and the larger latency handling is discussed under bug #641544.