Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12328097:
Always fully fill PulseAudio's requested buffer. Allow larger initial requests. (Closed)
|
Created:
7 years, 10 months ago by DaleCurtis Modified:
7 years, 7 months ago Reviewers:
no longer working on chromium CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAlways fully fill PulseAudio's requested buffer. Allow larger initial requests.
Instead of forcing PulseAudio to always call us with a buffer size matching
our requested size, let it automatically choose its initial buffers and only
ask for our requested size in steady state.
Doing this requires using a WaitTillDataReady() for those times when we
need to fill a larger buffer than we expect.
BUG=32757
NOTRY=true
TEST=no more PulseAudio glitching when using native sample rate.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185593
Patch Set 1 #
Total comments: 2
Patch Set 2 : Comments. #
Total comments: 4
Patch Set 3 : Fixes. #
Total comments: 3
Messages
Total messages: 15 (0 generated)
It's possible this will lead to some CHECK() failures from pa_stream_begin_write(), but I'd rather see if we can get away with the simple path before fixing multiple buffer support.
I'm not sure I'm really qualified to review pulse stuff -- hint: that means you xians ;) -- but OOC is this fixing a bug? The CL description doesn't mention why/what having larger initial requests is accomplishing. https://codereview.chromium.org/12328097/diff/1/media/audio/pulse/pulse_outpu... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/12328097/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.cc:156: // Tell pulse audio we only want callbacks of a certain size. is this comment still true?
Sorry this is the outcome of the discussion on https://codereview.chromium.org/12328084/
https://codereview.chromium.org/12374005/ to fix the scherkus-stamp issue :) https://codereview.chromium.org/12328097/diff/1/media/audio/pulse/pulse_outpu... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/12328097/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.cc:156: // Tell pulse audio we only want callbacks of a certain size. On 2013/02/28 01:20:13, scherkus wrote: > is this comment still true? Updated.
-scherkus now that xians can approve! Feel free to CQ it STO time if you think it lgty.
Hey Dale, just some questions. https://codereview.chromium.org/12328097/diff/6001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/12328097/diff/6001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:164: pa_buffer_attributes.fragsize = static_cast<uint32_t>(-1); by looking at http://freedesktop.org/software/pulseaudio/doxygen/structpa__buffer__attr.html it seems fragsize, prebuf and tlength can affect the latency of Pulse when PA_STREAM_ADJUST_LATENCY is set. If I understand correctly, setting them to -1 means we let Pulse buffer up enough data and compromise its latency for good audio. Do you know if flash and audio tag are always trying to open the streams using the default buffer size (512)? https://codereview.chromium.org/12328097/diff/6001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:248: CHECK_EQ(bytes_to_fill, static_cast<size_t>(params_.GetBytesPerBuffer())); I agree these checks might be dangerous if we support different buffer sizes. Could you elaborate why we want to check it instead of handling it?
https://codereview.chromium.org/12328097/diff/6001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/12328097/diff/6001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:164: pa_buffer_attributes.fragsize = static_cast<uint32_t>(-1); On 2013/02/28 08:56:09, xians1 wrote: > by looking at > http://freedesktop.org/software/pulseaudio/doxygen/structpa__buffer__attr.html > it seems fragsize, prebuf and tlength can affect the latency of Pulse when > PA_STREAM_ADJUST_LATENCY is set. If I understand correctly, setting them to -1 > means we let Pulse buffer up enough data and compromise its latency for good > audio. > > Do you know if flash and audio tag are always trying to open the streams using > the default buffer size (512)? Hmm I retested this with multiple streams and Pulse went berserk (super fast callbacks a la http://crbug.com/116435). After some trial and error this seems to work reliably without a latency increase (as measured by audio_latency_perf). I needed to adjust some of the flags to make this work though. Without PA_STREAM_INTERPOLATE_TIMING, the latency value was on the order of seconds (!) when using a tlength value this small. Please patch this in locally with your other Pulse patches and see how it sounds. https://codereview.chromium.org/12328097/diff/6001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:248: CHECK_EQ(bytes_to_fill, static_cast<size_t>(params_.GetBytesPerBuffer())); On 2013/02/28 08:56:09, xians1 wrote: > I agree these checks might be dangerous if we support different buffer sizes. > Could you elaborate why we want to check it instead of handling it? Mostly because I don't think we'll need to since we're requesting a buffer equal in size to our minreq. As such I'd rather not over complicate the code at this time. We'll notice immediately in crash reports if we need to handle this or not. https://codereview.chromium.org/12328097/diff/4003/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/12328097/diff/4003/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:316: // Set |source_callback_| to NULL so all FulfillWriteRequest() calls which may Also discovered some shutdown hangs here due to WaitTilDataReady() hanging for 1.5seconds multiple times.
I tried this CL, together with my CL https://codereview.chromium.org/12316131/(which provides the native sample rate), it sounds quite good for webrtc. But it is a bit choppy for audio tag in http://hpr.dogphilosophy.net/test/. I think it might be a mistake on my CL since it is trying to use 512 samples as buffer size for Pulse to open the audio tag, while for the ALSA, it is using 2048. Could you please do a quick test with http://hpr.dogphilosophy.net/test/?
Make sure you restart pulse before testing. After fiddling with settings for a while I've noticed it can get in a bad state.
(also you need to patch AudioManagerLinux and AudioRendererMixerManager to not pass through the input sample rate).
(everything sounds good to me :)
great. thanks for the verification. lgtm. https://codereview.chromium.org/12328097/diff/4003/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/12328097/diff/4003/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:316: // Set |source_callback_| to NULL so all FulfillWriteRequest() calls which may On 2013/03/01 00:08:17, DaleCurtis wrote: > Also discovered some shutdown hangs here due to WaitTilDataReady() hanging for > 1.5seconds multiple times. makes sense.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/12328097/4003
Message was sent while issue was closed.
Change committed as 185593
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/12328097/diff/4003/media/audio/pulse/p... File media/audio/pulse/pulse_output.cc (right): https://chromiumcodereview.appspot.com/12328097/diff/4003/media/audio/pulse/p... media/audio/pulse/pulse_output.cc:176: PA_STREAM_FIX_RATE | PA_STREAM_INTERPOLATE_TIMING | All, I realize I'm a bit late to the party here (as of this writing, this code has been moved to pulse_util.cc) but PA_STREAM_FIX_RATE is improper here. This forces the stream's sample rate to match the sink's sample rate, which isn't necessarily the same as info->sample_spec.rate. When this happens, PulseAudio begins playing back the audio at an improper sample rate, leading to e.g. https://code.google.com/p/webrtc/issues/detail?id=1587 Thanks, Sam |
