Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11098031:
Get PulseAudio implementation working. (Closed)
|
Created:
8 years, 2 months ago by DaleCurtis Modified:
8 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, Chris Rogers Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGet PulseAudio implementation working.
- Switches from iterating the PA mainloop to a threaded mainloop
which PulseAudio runs on its own.
- Fixes several bugs and cleans up the code.
Performance is much better than ALSA. I'm able to run audio
output at 440 frames smoothly with Pulse whereas ALSA I
couldn't get it to run below 1k w/o glitching.
Sadly while the jitter is much better too, it's still not
as smooth as the 8k buffer we were using previously:
http://commondatastorage.googleapis.com/dalecurtis-shared/alsa-vs-pulse.png
Which means I still need to look into writing some clock
smoothing in order for us to turn on renderer side mixing.
Unit tests to come later, mostly just getting it working now.
BUG=32757
TEST=media_unittests / manual playback tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=168116
Patch Set 1 #
Total comments: 8
Patch Set 2 : Cork. BeginWrite. Cleanup. #
Total comments: 30
Patch Set 3 : Comments. #
Total comments: 22
Patch Set 4 : Fixes. #
Total comments: 6
Patch Set 5 : Comments. #
Messages
Total messages: 28 (0 generated)
Oddly, getting it working required deleting more code than adding it :o
few initial comments cool stuff! please use BUG=32757 https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.cc:181: while (context_state_ != PA_CONTEXT_READY) { instead of the volatile funny business and state callbacks, it looks like the PA docs recommend using the PA locking + signalling APIs to convert async operations into sync operations: http://freedesktop.org/software/pulseaudio/doxygen/threaded_mainloop.html#cb_sec Can something like the following work? pa_threaded_mainloop_lock(pa_mainloop_); while (pa_context_get_state(pa_context_) != PA_CONTEXT_READY) { // blah blah blah pa_threaded_mainloop_wait(pa_mainloop_); } pa_threaded_mainloop_unlock(pa_mainloop_); https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.cc:243: while (stream_state_ != PA_STREAM_READY) { ditto https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.cc:263: if (playback_handle_) { can we make stronger guarantees over which objects should be alive? for example, it seems the lifetime of pa_mainloop_ > pa_context_ > playback_handle_, which means we should be able to use early returns + DCHECKs https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_output.h File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.h:5: // Creates an audio output stream based on the PulseAudio asynchronous API. does this comment need to be updated?
https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.cc:181: while (context_state_ != PA_CONTEXT_READY) { On 2012/10/10 17:56:29, scherkus wrote: > instead of the volatile funny business and state callbacks, it looks like the PA > docs recommend using the PA locking + signalling APIs to convert async > operations into sync operations: > http://freedesktop.org/software/pulseaudio/doxygen/threaded_mainloop.html#cb_sec > > Can something like the following work? > > pa_threaded_mainloop_lock(pa_mainloop_); > while (pa_context_get_state(pa_context_) != PA_CONTEXT_READY) { > // blah blah blah > pa_threaded_mainloop_wait(pa_mainloop_); > } > pa_threaded_mainloop_unlock(pa_mainloop_); Nice, didn't see that. Will convert. Even more code to delete :) https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.cc:263: if (playback_handle_) { On 2012/10/10 17:56:29, scherkus wrote: > can we make stronger guarantees over which objects should be alive? > > for example, it seems the lifetime of pa_mainloop_ > pa_context_ > > playback_handle_, which means we should be able to use early returns + DCHECKs I can add a if (!pa_mainloop) return early. Is that what you're thinking? playback_handle_ may or may not have been created. I'm also going to change the CHECK()'s in Open to if (!) return false, so pa_context_ may or may not be there either. https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_output.h File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.h:5: // Creates an audio output stream based on the PulseAudio asynchronous API. On 2012/10/10 17:56:29, scherkus wrote: > does this comment need to be updated? It's still accurate, but I'll add some details on the pa_threaded_mainloop stuff. https://codereview.chromium.org/11098031/diff/1/media/audio/pulse/pulse_outpu... media/audio/pulse/pulse_output.h:102: scoped_array<uint8> interleaved_audio_data_; Looks like I can remove this too by using pa_stream_begin_write() to get pulseaudio to hand me a buffer!
Great work. Dale, I had a CL which did the same thing, but never got my reviewer to take a look at it. You can use it as reference for this CL. http://codereview.chromium.org/8496007/ As I remember, what is lacking there is a buffer underflow callback to handle the underrun.
I have been very busy on working on a M23 beta crash today, and will take a look at the CL tomorrow.
On 2012/10/10 21:05:49, xians1 wrote: > I have been very busy on working on a M23 beta crash today, and will take a look > at the CL tomorrow. Nice! I didn't realize there was a CL out there already. Looks like we took similar paths.
On 2012/10/10 22:38:55, DaleCurtis wrote: > On 2012/10/10 21:05:49, xians1 wrote: > > I have been very busy on working on a M23 beta crash today, and will take a > look > > at the CL tomorrow. > > Nice! I didn't realize there was a CL out there already. Looks like we took > similar paths. New CL is up using scherkus's suggestions as well as ideas from the docs and xians' CL. Unfortunately I'm away from my linux box today so I can only confirm that it compiles. I'll test and fix tomorrow as necessary.
On 2012/10/11 00:25:17, DaleCurtis wrote: > On 2012/10/10 22:38:55, DaleCurtis wrote: > > On 2012/10/10 21:05:49, xians1 wrote: > > > I have been very busy on working on a M23 beta crash today, and will take a > > look > > > at the CL tomorrow. > > > > Nice! I didn't realize there was a CL out there already. Looks like we took > > similar paths. > > New CL is up using scherkus's suggestions as well as ideas from the docs and > xians' CL. Unfortunately I'm away from my linux box today so I can only confirm > that it compiles. I'll test and fix tomorrow as necessary. Dale, I will wait for your next set of code before doing a formal review.
https://codereview.chromium.org/11098031/diff/12001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/11098031/diff/12001/media/audio/audio_io.h#ne... media/audio/audio_io.h:94: // must always be followed by a call to Close() even if Open() fails. this makes me sad considering we return true/false :| https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:127: #define PULSE_CHECK(expression, message) \ this isn't really a CHECK... a more appropriate name would be RETURN_ON_FAILURE() similar to our code under content/common/gpu/media/ https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:128: if (expression) { \ you should check for the negated expression reading the CHECKs below seem strange as we're checking for the error value as opposed to the success value (i.e., CHECK that it's null?!) https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:128: if (expression) { \ this should be wrapped in a do { ... } while (0) see http://stackoverflow.com/questions/1067226/c-multi-line-macro-do-while0-vs-sc... https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:130: pa_threaded_mainloop_unlock(pa_mainloop_); \ how about declaring AutoPulseAudioLock helper at the top of this file? https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:131: DLOG(ERROR) << message << pa_context_errno(pa_context_); \ this forces log messages to know that the context error number gets tacked on the end how about a separate DLOG() for pa_context_errno() when pa_context_ != NULL or applying some spacing/formatting? https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:162: pa_threaded_mainloop_wait(pa_mainloop_); should we wait before checking the state? i.e., will we hang if state reaches READY before we get to this line as no more events happen? https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:176: // TODO(dalecurtis): Is this section correct? can you elaborate as to what might be incorrect? https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:203: static_cast<pa_stream_flags_t>( nit: construct flags variable outside of this function call w/ doc/summary/link as to what they do https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:212: pa_threaded_mainloop_wait(pa_mainloop_); ditto https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:272: &negative) != 0 || negative) can you split this into two ifs? it's a bit subtle with the negative out-param https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:299: // Let pa_stream_begin_write auto detect the buffer size, it should choose the add () https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:311: pa_stream_write(playback_handle_, audio_buffer, bytes_filled, NULL, 0LL, check return value? https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:352: void PulseAudioOutputStream::WaitForPulseOperation(pa_operation* op) { this function assumes that the caller has locked the mainloop do you need to acquire the lock for running the operations (i.e., cork/uncork) or for checking the state + waiting? if it's the latter, PA docs state that their locks are recursive so you _could_ add lock/unlock inside this function + remove the restriction on the callee https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.h:23: #include <pulse/pulseaudio.h> now that you got rid of the pa_stream_state_t stuff, can you move this to the .cc and fwd decl the PA types? it's always nice to limit the amount of system headers in .h files
Comments addressed and tested. Removing the stream flags made the accuracy of the latency information much better, enough that we may not need clock smoothing anymore. I'll need to regenerate the graph later. http://codereview.chromium.org/11098031/diff/12001/media/audio/audio_io.h File media/audio/audio_io.h (right): http://codereview.chromium.org/11098031/diff/12001/media/audio/audio_io.h#new... media/audio/audio_io.h:94: // must always be followed by a call to Close() even if Open() fails. On 2012/10/11 20:19:31, scherkus wrote: > this makes me sad considering we return true/false :| Yeah, it's not ideal. At least the comment codifies the behavior all of our existing streams rely on. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:127: #define PULSE_CHECK(expression, message) \ On 2012/10/11 20:19:31, scherkus wrote: > this isn't really a CHECK... a more appropriate name would be > RETURN_ON_FAILURE() similar to our code under content/common/gpu/media/ PULSE_CHECK sounds cooler though! :) Sadly, done. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:128: if (expression) { \ On 2012/10/11 20:19:31, scherkus wrote: > this should be wrapped in a do { ... } while (0) > > see > http://stackoverflow.com/questions/1067226/c-multi-line-macro-do-while0-vs-sc... That's not necessary here since the code's already in an if() {} block. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:128: if (expression) { \ On 2012/10/11 20:19:31, scherkus wrote: > this should be wrapped in a do { ... } while (0) > > see > http://stackoverflow.com/questions/1067226/c-multi-line-macro-do-while0-vs-sc... Done. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:130: pa_threaded_mainloop_unlock(pa_mainloop_); \ On 2012/10/11 20:19:31, scherkus wrote: > how about declaring AutoPulseAudioLock helper at the top of this file? Done. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:131: DLOG(ERROR) << message << pa_context_errno(pa_context_); \ On 2012/10/11 20:19:31, scherkus wrote: > this forces log messages to know that the context error number gets tacked on > the end > > how about a separate DLOG() for pa_context_errno() when pa_context_ != NULL or > applying some spacing/formatting? Done. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:162: pa_threaded_mainloop_wait(pa_mainloop_); On 2012/10/11 20:19:31, scherkus wrote: > should we wait before checking the state? > > i.e., will we hang if state reaches READY before we get to this line as no more > events happen? No we shouldn't, this will hang. Fixed. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:176: // TODO(dalecurtis): Is this section correct? On 2012/10/11 20:19:31, scherkus wrote: > can you elaborate as to what might be incorrect? No, I just haven't reviewed it thoroughly. Removed the TODO() for now. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:203: static_cast<pa_stream_flags_t>( On 2012/10/11 20:19:31, scherkus wrote: > nit: construct flags variable outside of this function call w/ doc/summary/link > as to what they do Removed them all except PA_STREAM_START_CORKED as the others were the cause of a poor audio clock. May not need that smoothing function after all! PA_STREAM_START_CORKED is self explanatory and explained elsewhere so I've left it inline. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:212: pa_threaded_mainloop_wait(pa_mainloop_); On 2012/10/11 20:19:31, scherkus wrote: > ditto Done. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:272: &negative) != 0 || negative) On 2012/10/11 20:19:31, scherkus wrote: > can you split this into two ifs? > > it's a bit subtle with the negative out-param Done. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:299: // Let pa_stream_begin_write auto detect the buffer size, it should choose the On 2012/10/11 20:19:31, scherkus wrote: > add () Done. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:311: pa_stream_write(playback_handle_, audio_buffer, bytes_filled, NULL, 0LL, On 2012/10/11 20:19:31, scherkus wrote: > check return value? Done. Add a lot of error handling elsewhere too. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:352: void PulseAudioOutputStream::WaitForPulseOperation(pa_operation* op) { On 2012/10/11 20:19:31, scherkus wrote: > this function assumes that the caller has locked the mainloop > > do you need to acquire the lock for running the operations (i.e., cork/uncork) > or for checking the state + waiting? > > if it's the latter, PA docs state that their locks are recursive so you _could_ > add lock/unlock inside this function + remove the restriction on the callee I'm pretty sure those calls need the lock too since we want to make sure there aren't any outstanding callbacks when we set |source_callback_| to NULL, etc. http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.h (right): http://codereview.chromium.org/11098031/diff/12001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.h:23: #include <pulse/pulseaudio.h> On 2012/10/11 20:19:31, scherkus wrote: > now that you got rid of the pa_stream_state_t stuff, can you move this to the > .cc and fwd decl the PA types? > > it's always nice to limit the amount of system headers in .h files Done.
It looks really nice, great job. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (left): http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:237: PA_STREAM_ADJUST_LATENCY | From internet: The server tries to assure that at least tlength bytes are always available in the per-stream server-side playback buffer. It is recommended to set this to (uint32_t) -1, which will initialize this to a value that is deemed sensible by the server. However, this value will default to something like 2s, i.e. for applications that have specific latency requirements this value should be set to the maximum latency that the application can deal with. When PA_STREAM_ADJUST_LATENCY is not set this value will influence only the per-stream playback buffer size. When PA_STREAM_ADJUST_LATENCY is set the overall latency of the sink plus the playback buffer size is configured to this value. Set PA_STREAM_ADJUST_LATENCY if you are interested in adjusting the overall latency. Don't set it if you are interested in configuring the server-side per-stream playback buffer size. Though I don't think we will have 2s latency, but I am curious on what the real latency will be without using PA_STREAM_ADJUST_LATENCY, if it is too high, we might need to get it back, and use pa_stream_set_underflow_callback to setup a underrun callback to adjust the buffer attributes. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:16: class AutoPulseLock { How about change the name to ScopedPulseLock? http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:32: static pa_sample_format_t BitsToPASampleFormat(int bits_per_sample) { what about extracting these helper class/functions to pulse_util.cc? Then we can keep the pulse_ouput.cc clean while sharing the code for the input that I might add. It is fine not to do if you don't have time. Thanks. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:248: &pa_sample_specifications, map); indentation. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:254: // TODO(dalecurtis): Why is this happening? Upstream bug? I was hit by this issue too, do you know if it comes with pa_stream_connect_playback or pa_stream_set_write_callback? http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:298: if (pa_stream_) { Do we need to call pa_stream_set_state_callback to NULL here? http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:379: CHECK_EQ(bytes_available, requested_bytes); I am not sure but afraid that we might be hit by this CHECK, especially for apps like html5 using high buffer size, can this pa_stream_begin_write returns a value smaller than requested_bytes? http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:380: CHECK_EQ(requested_bytes, static_cast<size_t>(params_.GetBytesPerBuffer())); if pa can't keep up with the pace of the callbacks, will it adjust it internal buffer size and affect this requested_bytes?
OOC aside from my/xians' comments is there anything else this is waiting on? http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:185: if (!(expression)) { \ "if() {}" is a compound statement and doesn't require a semicolon at the end using "do {} while(0)" expands into a regular statement that enforces ending the statement with a semicolon http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:337: if (pa_stream_get_latency(pa_stream_, &pa_latency_micros, &negative) != 0 || I meant splitting as in if (pa_stream_get_latency(...) != 0) return 0; if (negative) return 0; http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:423: pa_stream_, &StreamSuccessCallback, this)); indent
Depends on what we want. It's ready to go in as is w/ the current --use-pulseaudio flag, but we'll need a pulse input side before we can turn this on for default. During the bimonthly audio meeting, xians said he might have time to look into the input side for doing synchronous I/O. Until then, I don't have bandwidth to start the input side at the moment. Maybe for M25.
On 2012/10/17 22:53:29, DaleCurtis wrote: > Depends on what we want. It's ready to go in as is w/ the current > --use-pulseaudio flag, but we'll need a pulse input side before we can turn this > on for default. > > During the bimonthly audio meeting, xians said he might have time to look into > the input side for doing synchronous I/O. > > Until then, I don't have bandwidth to start the input side at the moment. Maybe > for M25. For the sake of avoiding bitrot... any reason not to at least commit this CL after addressing comments?
On 2012/10/18 01:32:16, scherkus wrote: > On 2012/10/17 22:53:29, DaleCurtis wrote: > > Depends on what we want. It's ready to go in as is w/ the current > > --use-pulseaudio flag, but we'll need a pulse input side before we can turn > this > > on for default. > > > > During the bimonthly audio meeting, xians said he might have time to look into > > the input side for doing synchronous I/O. > > > > Until then, I don't have bandwidth to start the input side at the moment. > Maybe > > for M25. > > For the sake of avoiding bitrot... any reason not to at least commit this CL > after addressing comments? Yeah that's my plan. Once I get my M24 CLS straightened out I'll get this landed :)
On 2012/10/18 01:34:02, DaleCurtis wrote: > On 2012/10/18 01:32:16, scherkus wrote: > > On 2012/10/17 22:53:29, DaleCurtis wrote: > > > Depends on what we want. It's ready to go in as is w/ the current > > > --use-pulseaudio flag, but we'll need a pulse input side before we can turn > > this > > > on for default. > > > > > > During the bimonthly audio meeting, xians said he might have time to look > into > > > the input side for doing synchronous I/O. > > > > > > Until then, I don't have bandwidth to start the input side at the moment. > > Maybe > > > for M25. > > > > For the sake of avoiding bitrot... any reason not to at least commit this CL > > after addressing comments? > > Yeah that's my plan. Once I get my M24 CLS straightened out I'll get this > landed :) Awesome!
On 2012/10/17 22:53:29, DaleCurtis wrote: > Depends on what we want. It's ready to go in as is w/ the current > --use-pulseaudio flag, but we'll need a pulse input side before we can turn this > on for default. > > During the bimonthly audio meeting, xians said he might have time to look into > the input side for doing synchronous I/O. > > Until then, I don't have bandwidth to start the input side at the moment. Maybe > for M25. Great, all sound good to me. It does make sense to wait for the input before turning this on by default. I have a CL which works for the audio, but since the input side needs device enumeration, APIs to get native sample rate, channel layouts, ..etc, I can't upload it for review yet. I need to work on some high priority work in media stream and UI in the coming few weeks, my plan is to try to allocate 1 hour per day on this work flow for the input side, then synchronous IO.
Hmm, came back to this today and there's lot of clicking :-/ Latency information is also not being returned without the stream flags from patch set 2. Will have to do some further investigation. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:16: class AutoPulseLock { On 2012/10/14 19:03:41, xians1 wrote: > How about change the name to ScopedPulseLock? I like AutoPulseLock for consistently with the existing AutoLock and AutoUnlock classes. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:32: static pa_sample_format_t BitsToPASampleFormat(int bits_per_sample) { On 2012/10/14 19:03:41, xians1 wrote: > what about extracting these helper class/functions to pulse_util.cc? Then we can > keep the pulse_ouput.cc clean while sharing the code for the input that I might > add. > > It is fine not to do if you don't have time. > > Thanks. Lets keep them here for now, when we have an input side we can extract them to a utility class. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:185: if (!(expression)) { \ On 2012/10/17 22:44:53, scherkus wrote: > "if() {}" is a compound statement and doesn't require a semicolon at the end > > using "do {} while(0)" expands into a regular statement that enforces ending the > statement with a semicolon Sure, but that's pretty redundant here. Done in any case. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:248: &pa_sample_specifications, map); On 2012/10/14 19:03:41, xians1 wrote: > indentation. Done. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:254: // TODO(dalecurtis): Why is this happening? Upstream bug? On 2012/10/14 19:03:41, xians1 wrote: > I was hit by this issue too, do you know if it comes with > pa_stream_connect_playback or pa_stream_set_write_callback? pa_stream_connect_playback(). http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:298: if (pa_stream_) { On 2012/10/14 19:03:41, xians1 wrote: > Do we need to call pa_stream_set_state_callback to NULL here? Done. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:337: if (pa_stream_get_latency(pa_stream_, &pa_latency_micros, &negative) != 0 || On 2012/10/17 22:44:53, scherkus wrote: > I meant splitting as in > > if (pa_stream_get_latency(...) != 0) > return 0; > if (negative) > return 0; Done.
Turns out the pops and clicks are from underflow combined with hitting PulseAudio's resampler with a new test clip I tried which was not 44.1kHz... using the native sample rate resolves any glitches. Given this Pulse audio may only be useful for a "low latency" style output driver, which isn't the end of the world since we're going away for high latency devices. Unfortunately, as far as I can find, there's no easy way to get the native sample rate! To do so you have to create a threaded mainloop and context, then issue a pa_context_get_server_info() call, then perform the shutdown dance like Reset() xians, am I missing something? This seems completely insane to have to construct a mainloop and context just to retrieve the server information.
On Wed, Nov 7, 2012 at 3:41 AM, <dalecurtis@chromium.org> wrote: > Turns out the pops and clicks are from underflow combined with hitting > PulseAudio's resampler with a new test clip I tried which was not > 44.1kHz... > using the native sample rate resolves any glitches. Given this Pulse > audio may > only be useful for a "low latency" style output driver, which isn't the > end of > the world since we're going away for high latency devices. > > Unfortunately, as far as I can find, there's no easy way to get the native > sample rate! To do so you have to create a threaded mainloop and context, > then > issue a pa_context_get_server_info() call, then perform the shutdown dance > like > Reset() > > xians, am I missing something? This seems completely insane to have to > construct > a mainloop and context just to retrieve the server information. > Hi Dale, You are right, we need a threaded mainloop and context to do the queries. And we have to do it if we want to do things like querying the native sample rate, device enumeration, ..etc My CL for the input is doing this, it created a thread mainloop and context when AudioManagerLinux is created and will be shared by all the recording streams, it is also used to query the native sample rate and device lists. check it out in https://codereview.chromium.org/10952024 (the last time I worked on it was 2 weeks ago, and I still need to finish the volume API, native sample rate, dynamic library linking before sending it out for formal review) I am not sure if we need to do it for output, one benefit is to share one thread for all the output streams, but the drop back is that the audio might not be as stable as one threaded mainloop for one audio output stream. BR, SX > > http://codereview.chromium.**org/11098031/<http://codereview.chromium.org/110... >
Thanks Xians. I think that sounds like a reasonable approach. I think now is a good time to collapse the AudioUtil::* methods into the AudioManager a la http://crbug.com/137326
Whoops, didn't finish my thought. We should do that so we don't have to make PulseUtil a singleton or what not accessible by AudioUtil.
Guys this should be ready to land now. It's glitchy if opened at the non-hw sample rate, but we'll hook it up to the low latency path in a future patch set. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:91: Turns out this code was wrong after all... http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:379: CHECK_EQ(bytes_available, requested_bytes); On 2012/10/14 19:03:41, xians1 wrote: > I am not sure but afraid that we might be hit by this CHECK, especially for apps > like html5 using high buffer size, can this pa_stream_begin_write returns a > value smaller than requested_bytes? Done. Now breaks the buffer into whatever chunks pa_stream_begin_write returns. http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:380: CHECK_EQ(requested_bytes, static_cast<size_t>(params_.GetBytesPerBuffer())); On 2012/10/14 19:03:41, xians1 wrote: > if pa can't keep up with the pace of the callbacks, will it adjust it internal > buffer size and affect this requested_bytes? Not so long as we force the buffer size in the pa_sample_spec used during stream creation. Chrome isn't compatible with the Pulse audio model of an adjusted buffer size depending on system load. I don't think it's worth rearchitecting to support this on Linux either. Opening at the native sample rate seems to work well even with a 440 frame buffer size, otherwise even an 8k buffer size isn't enough to prevent pops and clicks :-/ http://codereview.chromium.org/11098031/diff/17001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:423: pa_stream_, &StreamSuccessCallback, this)); On 2012/10/17 22:44:53, scherkus wrote: > indent Done. http://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): http://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:262: PA_STREAM_ADJUST_LATENCY | PA_STREAM_AUTO_TIMING_UPDATE | I think the source of jitter was the PA_STREAM_INTERPOLATE_TIMING flag. With just these flags we get latency information and playback seems smooth even with a 480 frame buffer (so long as we're opening at my hw sample rate which is 48kHz).
http://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): http://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:262: PA_STREAM_ADJUST_LATENCY | PA_STREAM_AUTO_TIMING_UPDATE | On 2012/11/09 01:10:52, DaleCurtis wrote: > I think the source of jitter was the PA_STREAM_INTERPOLATE_TIMING flag. With > just these flags we get latency information and playback seems smooth even with > a 480 frame buffer (so long as we're opening at my hw sample rate which is > 48kHz). Also, I'm thinking of adding PA_STREAM_NOT_MONOTONIC here too since the docs seem to indicate it should allow more accurate latency measurements. We're not using pa_stream_get_time() so we don't care if that goes backwards.
On 2012/11/09 01:10:52, DaleCurtis wrote: > Guys this should be ready to land now. It's glitchy if opened at the non-hw > sample rate, but we'll hook it up to the low latency path in a future patch set. > sgtm. lgtm if you want to land it now.
lgtm w/ nits lets iterate on this when you/we/someone has the time :) http://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.cc (right): http://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.cc:446: while (pa_operation_get_state(op) == PA_OPERATION_RUNNING) nit: can you {} around while? it's a bit subtle w/o them http://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_ou... File media/audio/pulse/pulse_output.h (right): http://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_ou... media/audio/pulse/pulse_output.h:57: // Used simply to trigger pa_threaded_mainloop_signal() and avoid dead locks. Wording suggestions: s/Used simple to trigger/Triggers/ s/and/to/ s/dead locks/deadlocks/
Thanks guys! CQ'ing. https://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:446: while (pa_operation_get_state(op) == PA_OPERATION_RUNNING) On 2012/11/14 22:31:31, scherkus wrote: > nit: can you {} around while? it's a bit subtle w/o them Done. https://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.h (right): https://codereview.chromium.org/11098031/diff/35001/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.h:57: // Used simply to trigger pa_threaded_mainloop_signal() and avoid dead locks. On 2012/11/14 22:31:31, scherkus wrote: > Wording suggestions: > > s/Used simple to trigger/Triggers/ > s/and/to/ > s/dead locks/deadlocks/ Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11098031/38003 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
