GNOME Bugzilla – Bug 617339
pulsesink doesn't make use of pa_stream_begin_write
Last modified: 2010-06-09 14:34:56 UTC
Created attachment 160029 [details] [review] patch against git master The current version of pulsesink creates a lot of overhead. For example, when decoding mp3, it'll send 4608 bytes at once, even when the latency was specified as 'don't care' with a large buffer-time/tlength value. The proposed solution (patch attached) relies on a shm buffer provided by pulse audio. PCM samples are copied into this buffer, and it's flushed when there's no space left. This saves a lots of useless overhead and helps reduce power consumption. On a Menlow platform this help go from 12% C0 to 9%. We use this patch for all power/performance measurements at Intel, and we might as well contribute it to the community. This solution works only with PulseAudio >0.9.16 (about a year old). The patch has not been tested for the case where the 'toy resampler' is used, mainly since I don't know how to test it. An additional improvement could be to flush the buffer when the latency-time value was reached. Flushing the buffer when it is full could be an issue for low-latency apps. Feedback welcome.
Review of attachment 160029 [details] [review]: The patch looks quite good to me. I wonder if thinks could be refactored somewhat to reduce the copy and paste between the the two branches of the #ifdef HAVE_PULSE_0_9_16 in gst_pulseringbuffer_commit(). Maybe some static inline functions. Should a similar change also be made for the source? ::: ext/pulse/pulsesink.c @@ +1257,3 @@ + GST_LOG_OBJECT (psink, + "need to write %d samples at offset %" G_GINT64_FORMAT, *toprocess, + offset); the log statement could stay out of the ifdef (its the same in #else)
One thing I noticed is, that you should a) reset m_data, m_writable and m_towrite when canceling a write and b) should check if m_data is non-NULL before flushing the buffer. I think, before this can get in we should make sure, that never more than latency_time samples are pending.
Agree that the code is a bit thick with the ifdef. I wanted functionality first... Yes, clamping the writable size to latency_time would make sense. This was one of the options I indicated. I will do some additional tests, edits and resubmit the patch based on these comments.
Created attachment 160390 [details] [review] 2nd patch- limits internal buffering to be applied on top of first patch. changed from PA_SEEK_ABSOLUTE to PA_SEEK_RELATIVE to simplify offset management
Created attachment 160391 [details] [review] 3rd patch- test for NULL buffer to be applied on top of 2 first patches
Created attachment 160392 [details] [review] 4th patch - reset variables forgot to add this one
(In reply to comment #4) > Created an attachment (id=160390) [details] [review] > 2nd patch- limits internal buffering > > to be applied on top of first patch. > changed from PA_SEEK_ABSOLUTE to PA_SEEK_RELATIVE to simplify offset management What's the advantage of doing this? The non shm variant still needs to do the offset management anyway And you're only flushing the buffer for latency-time the next try, wouldn't it be better to flush it immediately if latency-time is reached?
I also think we had problems with PA_SEEK_RELATIVE in the past: commit abee4f1d644dd79fe8a74884a513872ec17b5047 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Tue Aug 25 16:53:29 2009 +0200 pulsesink: don't use relative seeks Don't use relative seeks, it's too hard to track where we are after a flush etc. fixes #593015 I'd also prefer to have a cumulative patch for the topic.
Good points. Honestly I used SEEK_RELATIVE because I didn't understand why this is was an ABSOLUTE_SEEK in the first place, and it made my life simpler. What's unclear to me is whether the offset can have discontinuities between two calls to ringbuffer-commit, and how I could test this. If jumps in the offset parameters are possible then I probably need to do some additional work.
It can. You can test with a live pipeline or you can go PLAYING -> PAUSED -> PLAYING and the offset should be unaligned.
Created attachment 162508 [details] [review] combined patch, limits buffering to latency-time and uses PA_SEEK_ABSOLUTE mode here's a combined patch aginst git master that hopefully addresses all concerns on buffering limitation, use of SEEK_ABSOLUTE and initializations.
commit 66a76d1f65141ec1180809d2fc258b1e4ecb9768 Author: Pierre-Louis Bossart <pierre-louis.bossart@intel.com> Date: Tue Jun 1 18:54:41 2010 -0500 pulsesink: optimize communication with PulseAudio using pa_stream_begin_writ
This introduces a regression when playing back asf files. See #620540