Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10952024:
Adding pulseaudio input support to chrome (Closed)
|
Created:
8 years, 3 months ago by no longer working on chromium Modified:
7 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding pulseaudio input support to chrome.
This input implementation also support device enumeration and volume APIs.
BUG=172259, 169075, 175393, 164361
TEST=content_unittests and media_unittests, manual test with https://webrtc-demos.appspot.com/html/pc1.html
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184122
Patch Set 1 #Patch Set 2 : sound #Patch Set 3 : second #Patch Set 4 : #Patch Set 5 : rebased. #Patch Set 6 : rebased and ready for review. #
Total comments: 30
Patch Set 7 : moved the pulse code to AudioManagerPulse, and addressed Dale's comments. #
Total comments: 10
Patch Set 8 : switched to dynamic linking and addressed Andrew's comments. #
Total comments: 16
Patch Set 9 : addressed Dale's comments. #Patch Set 10 : used the stubs script to do the dynamic linking #
Total comments: 62
Patch Set 11 : addressed Dale's comments #
Total comments: 2
Patch Set 12 : rebased and addressed Dale's final comments. #
Messages
Total messages: 28 (0 generated)
Hi guys, could you please review this CL?
Dale, I remember you mentioned that we would need a dynamic linking to the pulse
library, so I suppose that I need to hide all the pulseaudio code behind the
USE_PULSEAUDIO macro, please correct me here if I don't need to do so.
And media.gyp, we have comment like this:
# Override to dynamically link the PulseAudio library.
'use_pulseaudio%': 0,
Do you know if the dynamic linking has been done?
Few high level questions to help me review ... 1) Are you aiming for a particular deadline / is this a "must have" or a "nice to have"? 2) I'd really like for us to split AudioManagerLinux into separate objects (http://crbug.com/148661) -- perhaps it's worth doing that w/ Pulse before this CL. Any thoughts? 3) Do we really need to use dynamic linking against pulse? Has the state of the Linux world advanced enough to do so?
Hi Andrew, please see answer in line. On 2013/01/26 01:26:28, scherkus wrote: > Few high level questions to help me review ... > 1) Are you aiming for a particular deadline / is this a "must have" or a "nice > to have"? I am aiming to put PulseAudio as the default input and output implementation on Linux running Pulse, for those machines without Pulse, it should automatically fall back to ALSA. It is almost a "must" given the amount of audio issues we have in Linux, and also it is the key to move hangout to webrtc since we need to make the audio quality in par with the plugin. > 2) I'd really like for us to split AudioManagerLinux into separate objects > (http://crbug.com/148661) -- perhaps it's worth doing that w/ Pulse before this > CL. Any thoughts? I think we can separate CRAS from others easily by using the USE_CRAS macro, but we need to detect PulseAudio or Alsa in runtime, so we can't split it further for Pulse and Alsa. Let me know if you want me to do the splition of CRAS and others. > 3) Do we really need to use dynamic linking against pulse? Has the state of > the Linux world advanced enough to do so? According to my understanding, we need to link libpulse to be able to use pulseaudio, today we are using static linking in media.gyp using a flag use_pulseaudio to control it. In case chrome is building with pulse support but the OS does not have libpulse, chrome will fail on launching due to it could not link the library. Please correct me if I am wrong here.
On 2013/01/28 14:33:30, xians1 wrote: > Hi Andrew, please see answer in line. > > On 2013/01/26 01:26:28, scherkus wrote: > > Few high level questions to help me review ... > > 1) Are you aiming for a particular deadline / is this a "must have" or a > "nice > > to have"? > > I am aiming to put PulseAudio as the default input and output implementation on > Linux running Pulse, for those machines without Pulse, it should automatically > fall back to ALSA. > > It is almost a "must" given the amount of audio issues we have in Linux, and > also it is the key to move hangout to webrtc since we need to make the audio > quality in par with the plugin. > > > 2) I'd really like for us to split AudioManagerLinux into separate objects > > (http://crbug.com/148661) -- perhaps it's worth doing that w/ Pulse before > this > > CL. Any thoughts? > > I think we can separate CRAS from others easily by using the USE_CRAS macro, but > we need to detect PulseAudio or Alsa in runtime, so we can't split it further > for Pulse and Alsa. If we need to do run-time checks (depends on answer for 3), AFAIK it can be done inside CreateAudioManager(): https://code.google.com/searchframe#OAMlx_jo-ck/src/media/audio/linux/audio_m... ... and return either AudioManagerAlsa or AudioManagerPulse. > Let me know if you want me to do the splition of CRAS and others. If you have the time to do so that'd be great. I think it'd be good to start off answering (3) first. > > 3) Do we really need to use dynamic linking against pulse? Has the state of > > the Linux world advanced enough to do so? > > According to my understanding, we need to link libpulse to be able to use > pulseaudio, today we are using static linking in media.gyp using a flag > use_pulseaudio to control it. In case chrome is building with pulse support but > the OS does not have libpulse, chrome will fail on launching due to it could not > link the library. > Please correct me if I am wrong here. I honestly don't know but it'd be prudent to double-check before adding run-time support. Any input here dalecurtis@?
I'm pretty sure xians is right and we'll get runtime errors for missing symbols on systems that don't have Pulse installed. If we don't want to deprecate ALSA we'll need to come up with a dynamic loading solution. I have no idea what percentage of people still use pure ALSA, it may be that we can simply flip the current situation and put alsa behind a flag for distros which don't have pulse. +phadan.jr who might have a better idea of pulse vs alsa usage.
Haven't reviewed pulse_util.cc yet. https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... File media/audio/linux/audio_manager_linux.cc (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.cc:66: #if defined(USE_PULSEAUDIO) Construct in #if block inside constructor? https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.cc:131: if (pulse_->has_pulse()) { Can this be done a la UseCras; i.e. no need for #ifs? https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.cc:323: return new PulseAudioOutputStream(params, this); Indent is wrong. https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... File media/audio/linux/audio_manager_linux.h (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.h:13: #include "media/audio/pulse/pulse_util.h" Needs guards? https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:131: pa_operation* operation = pa_stream_cork(handle_, 0, NULL, NULL); Do you need to wait for this? I do in PulseOutput. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:147: Need to flush? https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:190: void PulseAudioInputStream::SetVolume(double volume) { Slightly off topic, but does WebRTC or anything actually use all the Get/SetVolume stuff? I'm thinking of removing it throughout the code base. HTML5 volume will be handled by renderer side mixing. At least on the output side that would get rid of a bunch of useless AdjustVolume calls. I see something about the AGC using volume below, but I'm unfamiliar with how this all fits together. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:210: pa_cvolume_set(&pa_volume, channels_, volume); Weird that you need the channels_ here. Do you know why? https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:280: int PulseAudioInputStream::GetHardwareLatencyInBytes() { Is this something that should be in PulseUtil? https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:305: pa_stream_peek(handle_, &data, &length); Does this length correlate well with the requested length? I.e. is it frequently not what we requested? I'm worried our read callbacks will have infrequent (read: high latency) timing. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:318: while (buffer_->forward_bytes() >= packet_size) { Following up on the last comment, for this to work in a reliable manner, the pulse callback must happen more frequently than we expect it to. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.cc (left): https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:103: void PulseAudioOutputStream::ContextNotifyCallback(pa_context* c, Should we try to collapse these into pulse util? https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:352: void PulseAudioOutputStream::WaitForPulseOperation(pa_operation* op) { Move to PulseUtil? https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_u... File media/audio/pulse/pulse_util.h (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.h:16: class PulseInputObject { Why have this here vs in the pulse input stream? https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.h:42: static void SamplerateInfoCallback(pa_context* context, Is this going to pipe into AudioUtil:: somehow? (Although ideally I want to nuke that class and move everything to AudioManager).
On 2013/01/29 19:08:50, DaleCurtis wrote: > I'm pretty sure xians is right and we'll get runtime errors for missing symbols > on systems that don't have Pulse installed. If we don't want to deprecate ALSA > we'll need to come up with a dynamic loading solution. > > I have no idea what percentage of people still use pure ALSA, it may be that we > can simply flip the current situation and put alsa behind a flag for distros > which don't have pulse. +phadan.jr who might have a better idea of pulse vs alsa > usage. According to http://www.freedesktop.org/wiki/Software/PulseAudio/About , PA is enabled by default on Ubuntu and Fedora, but not Debian and openSUSE, two other distros we claim to support: http://support.google.com/chrome/bin/answer.py?hl=en&answer=95411 There is also a pretty vocal minority of users who hate PA. IMHO focus on making Google Chrome run on officially supported distros. Having PA support a build-time flag for distros is useful, and I'm fine with the defaults flipped (i.e. enable PA by default and have a switch to use pure ALSA). Thank you for asking here. Please let me know if I can help more.
Hi Andrew, Do you know doing a dynamic linking to a library is quick or not? I am worrying that doing run-time checks on CreateAudioManager might increase the startup time if we are using dynamic linking for the pulse library. If it is a concern, preferably it should be done at the first time audio stream is created. > If we need to do run-time checks (depends on answer for 3), AFAIK it > can be done > inside CreateAudioManager(): Dale, Thanks for the confirmation and review, I think we should use dynamic linking to handle the auto fallback to ALSA, then we can make PA as default. We can do this on a separate CL.
On 2013/01/30 13:55:10, xians1 wrote: > Hi Andrew, > > Do you know doing a dynamic linking to a library is quick or not? > > I am worrying that doing run-time checks on CreateAudioManager might increase > the startup time if we are using dynamic linking for the pulse library. If it is > a concern, preferably it should be done at the first time audio stream is > created. It should be a one-time cost when starting the first audio stream, which should be acceptable. I imagine pulse/alsa libraries gain some library load time benefit being commonly used by other applications. That being said .. you can always measure warm + cold load times locally ;) > > If we need to do run-time checks (depends on answer for 3), AFAIK it > can be > done > > inside CreateAudioManager(): > > Dale, > Thanks for the confirmation and review, I think we should use dynamic linking to > handle the auto fallback to ALSA, then we can make PA as default. We can do this > on a separate CL. SGTM
Dale and Scherkus, sorry for the silence when I was busy with UI. I moved the pulse code from the AudioManagerLinux to AudioManagerPulse, and also addressed Dale's comments. Could you please take another look? BR, SX https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... File media/audio/linux/audio_manager_linux.cc (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.cc:66: #if defined(USE_PULSEAUDIO) On 2013/01/30 02:54:30, DaleCurtis wrote: > Construct in #if block inside constructor? This code is removed. https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.cc:131: if (pulse_->has_pulse()) { On 2013/01/30 02:54:30, DaleCurtis wrote: > Can this be done a la UseCras; i.e. no need for #ifs? Removed. https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.cc:323: return new PulseAudioOutputStream(params, this); On 2013/01/30 02:54:30, DaleCurtis wrote: > Indent is wrong. Removed. https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... File media/audio/linux/audio_manager_linux.h (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.h:13: #include "media/audio/pulse/pulse_util.h" On 2013/01/30 02:54:30, DaleCurtis wrote: > Needs guards? Done. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:131: pa_operation* operation = pa_stream_cork(handle_, 0, NULL, NULL); On 2013/01/30 02:54:30, DaleCurtis wrote: > Do you need to wait for this? I do in PulseOutput. I think both work, but make more sense to wait here. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:147: On 2013/01/30 02:54:30, DaleCurtis wrote: > Need to flush? Done. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:190: void PulseAudioInputStream::SetVolume(double volume) { On 2013/01/30 02:54:30, DaleCurtis wrote: > Slightly off topic, but does WebRTC or anything actually use all the > Get/SetVolume stuff? I'm thinking of removing it throughout the code base. HTML5 > volume will be handled by renderer side mixing. At least on the output side that > would get rid of a bunch of useless AdjustVolume calls. > > I see something about the AGC using volume below, but I'm unfamiliar with how > this all fits together. Yes, WebRtc analog AGC needs these analog volume control. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:210: pa_cvolume_set(&pa_volume, channels_, volume); On 2013/01/30 02:54:30, DaleCurtis wrote: > Weird that you need the channels_ here. Do you know why? The API description looks like this: Set the volume of the specified number of channels to the volume v. So I assume we need to know the number of channels of the device before setting the volume. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:280: int PulseAudioInputStream::GetHardwareLatencyInBytes() { On 2013/01/30 02:54:30, DaleCurtis wrote: > Is this something that should be in PulseUtil? Done. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:305: pa_stream_peek(handle_, &data, &length); On 2013/01/30 02:54:30, DaleCurtis wrote: > Does this length correlate well with the requested length? I.e. is it frequently > not what we requested? I'm worried our read callbacks will have infrequent > (read: high latency) timing. most of the cases it is the same as the requested length. but if overrun happens, this might change, but I am not sure. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:318: while (buffer_->forward_bytes() >= packet_size) { On 2013/01/30 02:54:30, DaleCurtis wrote: > Following up on the last comment, for this to work in a reliable manner, the > pulse callback must happen more frequently than we expect it to. Right, if this can happen, we might need something like WaitTillDataReady on the input side to handle the asynchronous share memory. I put a sleep for 2ms for now, and will revisit it if we see the need for WaitTillDataReady. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.cc (left): https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:103: void PulseAudioOutputStream::ContextNotifyCallback(pa_context* c, On 2013/01/30 02:54:30, DaleCurtis wrote: > Should we try to collapse these into pulse util? Not sure how we can, since it accesses the stream->source_callback_ inside this function, and we have different callback names for input and output. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:352: void PulseAudioOutputStream::WaitForPulseOperation(pa_operation* op) { On 2013/01/30 02:54:30, DaleCurtis wrote: > Move to PulseUtil? Done. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_u... File media/audio/pulse/pulse_util.h (right): https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.h:16: class PulseInputObject { On 2013/01/30 02:54:30, DaleCurtis wrote: > Why have this here vs in the pulse input stream? It is moved to AudioManagerPulse. https://codereview.chromium.org/10952024/diff/17011/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.h:42: static void SamplerateInfoCallback(pa_context* context, On 2013/01/30 02:54:30, DaleCurtis wrote: > Is this going to pipe into AudioUtil:: somehow? (Although ideally I want to nuke > that class and move everything to AudioManager). Done.
https://codereview.chromium.org/10952024/diff/31001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): https://codereview.chromium.org/10952024/diff/31001/media/audio/audio_util.cc... media/audio/audio_util.cc:158: // TODO(crogers) : return correct value in rare non-48KHz cases. did you mean to move this comment? https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:4: // remove line https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:9: #include <pulse/pulseaudio.h> it appears that all pa_xxx types can be forward declared if that's the case, can you forward declare them and move this to the .cc ? https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:56: AudioManagerBase* audio_manager_; make this AudioManagerPulse -- I hope AudioManagerBase can go away some day :) https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:85: #endif // MEDIA_AUDIO_PULSE_PULSE_OUTPUT_H_ fix
Hello, I just added the dynamic linking to the pulse audio library. It will try to load the pulse library when creating the AudioManagerPulse, if it fails, saying the library does not exist, it will automatically fallback to AudioManagerLinux, which uses ALSA. So I would say that Pulse is almost ready for production, and the audio quality is better compared with ALSA. Please take another look, I want to land it ASAP. BR, SX https://codereview.chromium.org/10952024/diff/31001/media/audio/audio_util.cc File media/audio/audio_util.cc (right): https://codereview.chromium.org/10952024/diff/31001/media/audio/audio_util.cc... media/audio/audio_util.cc:158: // TODO(crogers) : return correct value in rare non-48KHz cases. On 2013/02/14 00:51:35, scherkus wrote: > did you mean to move this comment? Yes, but not by this CL, it will be done in a follow-up. https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:4: // On 2013/02/14 00:51:35, scherkus wrote: > remove line Done. https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:9: #include <pulse/pulseaudio.h> On 2013/02/14 00:51:35, scherkus wrote: > it appears that all pa_xxx types can be forward declared > > if that's the case, can you forward declare them and move this to the .cc ? Done. https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:56: AudioManagerBase* audio_manager_; On 2013/02/14 00:51:35, scherkus wrote: > make this AudioManagerPulse -- I hope AudioManagerBase can go away some day :) Done. https://codereview.chromium.org/10952024/diff/31001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:85: #endif // MEDIA_AUDIO_PULSE_PULSE_OUTPUT_H_ On 2013/02/14 00:51:35, scherkus wrote: > fix Done.
I definitely recommend you look at generate_stubs, the pulse_wrapper makes me sad :( https://codereview.chromium.org/10952024/diff/35003/media/audio/linux/audio_m... File media/audio/linux/audio_manager_linux.cc (right): https://codereview.chromium.org/10952024/diff/35003/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.cc:334: #if defined(USE_PULSEAUDIO) Can we just remove this #define now? https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/audio_m... File media/audio/pulse/audio_manager_pulse.cc (right): https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:168: input_params.sample_rate(), 16, buffer_size); We can land it this way, but I'm pretty sure we'll need to use the native sample rate if we want good pulse performance with tiny buffers. https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:171: int AudioManagerPulse::GetNativeSampleRate() { We'll want to plumb this into AudioUtil for before we make this the default, or go ahead and move all audio_util calls onto the AudioManager. (I'm working on it!) https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:107: pa_mainloop_ = wrapper_->pa_threaded_mainloop_new_(); Should we be creating a new mainloop for every stream? https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/pulse_w... File media/audio/pulse/pulse_wrapper.cc (right): https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/pulse_w... media/audio/pulse/pulse_wrapper.cc:69: base::NativeLibrary pulse_lib = base::LoadNativeLibrary( Hmm, this is way different than what I was thinking you'd have to do. Why can't you do something like we do in media_posix? https://code.google.com/p/chromium/codesearch#chromium/src/media/base/media_p... You essentially just create a list of these functions in a .sigs file and use tools/generate_stubs to generate a static library we can link against. See http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=ch... http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=ff... It should be simpler than maintaining this file by hand and then having to also deal with wrapper_-> stuff. https://codereview.chromium.org/10952024/diff/35003/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/10952024/diff/35003/media/media.gyp#newcode611 media/media.gyp:611: ['use_pulseaudio == 1', { Can we just remove this now? https://codereview.chromium.org/10952024/diff/35003/media/media.gyp#newcode615 media/media.gyp:615: # 'link_settings': { Delete? https://codereview.chromium.org/10952024/diff/35003/media/media.gyp#newcode657 media/media.gyp:657: 'audio/pulse/audio_manager_pulse.cc', Why all this? We don't exclude audio_manager_linux?
Hi Dale, I think I have already addressed all your comments. I was not aware of the script to do the dynamic linking. But it will be very painful to re-do it again :), hope to keep this way unless we have very strong preference on using the script. Please take another look. SX https://codereview.chromium.org/10952024/diff/35003/media/audio/linux/audio_m... File media/audio/linux/audio_manager_linux.cc (right): https://codereview.chromium.org/10952024/diff/35003/media/audio/linux/audio_m... media/audio/linux/audio_manager_linux.cc:334: #if defined(USE_PULSEAUDIO) On 2013/02/15 00:28:51, DaleCurtis wrote: > Can we just remove this #define now? My plan is to remove the switch in another CL, after testing the audio performance for the major products. Quite likely we need to tweak some params like the default buffer size. https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/audio_m... File media/audio/pulse/audio_manager_pulse.cc (right): https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:168: input_params.sample_rate(), 16, buffer_size); On 2013/02/15 00:28:51, DaleCurtis wrote: > We can land it this way, but I'm pretty sure we'll need to use the native sample > rate if we want good pulse performance with tiny buffers. Yes, we definitely need to use the native sample rate. https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:171: int AudioManagerPulse::GetNativeSampleRate() { On 2013/02/15 00:28:51, DaleCurtis wrote: > We'll want to plumb this into AudioUtil for before we make this the default, or > go ahead and move all audio_util calls onto the AudioManager. (I'm working on > it!) Great, FYI, we need to access some member variable for this AudioManagerPulse::GetNativeSampleRate, like input_context_, we might need to create a virtual interface in the AudioManager, instead of using static functions. https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/pulse_o... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/pulse_o... media/audio/pulse/pulse_output.cc:107: pa_mainloop_ = wrapper_->pa_threaded_mainloop_new_(); On 2013/02/15 00:28:51, DaleCurtis wrote: > Should we be creating a new mainloop for every stream? I do it this way to share the threads for all the recording streams. I believe/or hope it is fine here, but will keep an eye here, in case there is some bottleneck on this design, we might need to create one thread for each stream. https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/pulse_w... File media/audio/pulse/pulse_wrapper.cc (right): https://codereview.chromium.org/10952024/diff/35003/media/audio/pulse/pulse_w... media/audio/pulse/pulse_wrapper.cc:69: base::NativeLibrary pulse_lib = base::LoadNativeLibrary( On 2013/02/15 00:28:51, DaleCurtis wrote: > Hmm, this is way different than what I was thinking you'd have to do. Why can't > you do something like we do in media_posix? > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/media_p... > > You essentially just create a list of these functions in a .sigs file and use > tools/generate_stubs to generate a static library we can link against. See > > http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=ch... > http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=ff... > > It should be simpler than maintaining this file by hand and then having to also > deal with wrapper_-> stuff. Simply because I don't way we have a script to do the work. But some question about using the generate_stubs, what will happen if some of the symbols could not be loaded properly? We will get error and be able to fall back to ALSA? My way is a bit painful on writing the code, but it allows us to know what has failed and fall back. https://codereview.chromium.org/10952024/diff/35003/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/10952024/diff/35003/media/media.gyp#newcode611 media/media.gyp:611: ['use_pulseaudio == 1', { On 2013/02/15 00:28:51, DaleCurtis wrote: > Can we just remove this now? I prefer to do it on another trivial CL. After testing with webrtc, webaudio, pepper, and audio tags. https://codereview.chromium.org/10952024/diff/35003/media/media.gyp#newcode615 media/media.gyp:615: # 'link_settings': { On 2013/02/15 00:28:51, DaleCurtis wrote: > Delete? Done. https://codereview.chromium.org/10952024/diff/35003/media/media.gyp#newcode657 media/media.gyp:657: 'audio/pulse/audio_manager_pulse.cc', On 2013/02/15 00:28:51, DaleCurtis wrote: > Why all this? We don't exclude audio_manager_linux? audio_manager_linux is automatically excluded because of being in linux folder, but we need to exclude pulse explicitly.
If the generate stubs approach can work I would strongly prefer to go that path as the current approach is a maintenance burden.
All right, I will what I can do here. Is there any document/wiki about the script? or anyone knows how it works? sx On Feb 15, 2013 7:06 PM, <scherkus@chromium.org> wrote: > If the generate stubs approach can work I would strongly prefer to go that > path > as the current approach is a maintenance burden. > > https://codereview.chromium.**org/10952024/<https://codereview.chromium.org/1... >
Just what I wrote in my comment and what's in tools/generate_stubs.py. It shouldn't be difficult outside of a little gyp magic.
Dale, could you please briefly tell me how the dynamic link is done when using the script? like when it is loaded? How does it affect the startup time? and what happens if some of the symbols are missing? On Feb 16, 2013 12:35 AM, <dalecurtis@chromium.org> wrote: > Just what I wrote in my comment and what's in tools/generate_stubs.py. It > shouldn't be difficult outside of a little gyp magic. > > https://codereview.chromium.**org/10952024/<https://codereview.chromium.org/1... >
Library load would be done via a InitializeStubs() call like we have in
media_posix. This will attempt to dlsym every symbol listed in the .sigs file at
generation time. When the stubs gyp target runs you'll get a static library
that contains code like the following:
extern void avcodec_flush_buffers(AVCodecContext *avctx) __attribute__((weak));
void avcodec_flush_buffers(AVCodecContext *avctx) {
avcodec_flush_buffers_ptr(avctx);
}
See <out path>/obj/third_party/ffmpeg/ffmpeg.gen/ffmpeg_stubs.cc in your local
system for complete details.
If any symbol fails to load the initialize call will return false. In
media_posix this is tracked by "g_media_library_is_initialized".
(You would call InitializeStubs() before choosing AudioManagerPulse() or AudioManagerLinux())
Hi Dale and Andrew, I have already changed to code to use the stub script for the dynamic linking, please take another look and I hope to land it ASAP. Thanks, SX
.sigs is much nicer! https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... File media/audio/pulse/audio_manager_pulse.cc (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:26: namespace { No need for namespace {}. Media style is static const. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:33: Extra line. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:55: Terminate(); How about a more descriptive name than Terminate() since we've already got Shutdown() ? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:70: bool AudioManagerPulse::CanShowAudioInputSettings() { Delete. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:88: void AudioManagerPulse::ShowAudioInputSettings() { Same as AudioManagerLinux, just make it a static there and forward the call here. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:123: if (!device_names->empty()) { Should this be "if (device_name->empty())" ? Otherwise a comment on what you're doing here would be nice. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:279: return; indent is off. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:285: info->name)); Indent is off. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... File media/audio/pulse/audio_manager_pulse.h (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.h:27: virtual bool CanShowAudioInputSettings() OVERRIDE; You'll need to rebase, this was deleted a few revisions ago. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.h:57: static void SamplerateInfoCallback(pa_context* context, s/Samplerate/SampleRate/ Is there a reason you didn't put all this in PulseUtil? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse.sigs File media/audio/pulse/pulse.sigs (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse.s... media/audio/pulse/pulse.sigs:1: # Copyright 2013 The Chromium Authors. All rights reserved. Looks much nicer!! https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:85: int err = pa_stream_connect_record( There's no supporting documentation on the return value here. Are you just guessing? Is there a doc elsewhere? http://freedesktop.org/software/pulseaudio/doxygen/stream_8h.html#abfd34293aa... https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:128: Clear buffer_ ? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:133: pa_operation* operation = pa_stream_cork(handle_, 0, NULL, NULL); Can this hang or fail w/o a success callback? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:145: Side note: I really dislike that callback_ isn't set to NULL here, even though all the other input streams do this so they can issue OnClose(). Can we do something about this? It feels like a use-after-free just waiting to happen. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:191: size_t index = pa_stream_get_device_index(handle_); Should this just be done during open and the values cached? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:222: WaitForOperationCompletion(pa_mainloop_, operation); Is it worth doing this or should you just cache the volume_ value during the initial set? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:244: if (error) { The docs indicate that value is "eol", without elaboration :( http://freedesktop.org/software/pulseaudio/doxygen/introspect_8h.html#a64473f... https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:249: if (stream->channels_ != info->channel_map.channels) Why would these values differ? This seems sketchy. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:294: while (true) { while (pa_stream_readable_size(handle_) > 0) instead? or while (pa_stream_peek() && data && length) { Append() } ? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:322: base::PlatformThread::Sleep( The general delay for this is 5ms. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:64: // entered an unrecoverable error state, or the Stio() has executed. Stio? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.cc:96: ch = static_cast<Channels>(ch + 1)) { Indent. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... File media/audio/pulse/pulse_util.h (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.h:15: WDYT about putting these in a pulse sub-namespace under media? I.e. media::pulse::() or a class similar to henrika's CoreAudioUtil? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.h:17: class AutoPulseLock { Just inline the implementation like base/lock.h https://codereview.chromium.org/10952024/diff/62001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/10952024/diff/62001/media/media.gyp#newcode617 media/media.gyp:617: 'generate_stubs_script': '../tools/generate_stubs/generate_stubs.py', For the variables which are only used once, just put them directly in the strings below. https://codereview.chromium.org/10952024/diff/62001/media/media.gyp#newcode631 media/media.gyp:631: '..', Is .. necessary for this? https://codereview.chromium.org/10952024/diff/62001/media/media.gyp#newcode633 media/media.gyp:633: 'dependencies': [ base is already a dependency elsewhere. https://codereview.chromium.org/10952024/diff/62001/media/media.gyp#newcode637 media/media.gyp:637: 'direct_dependent_settings': { These shouldn't be necessary here since you're in target.
https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... File media/audio/pulse/audio_manager_pulse.cc (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:26: namespace { On 2013/02/20 00:17:38, DaleCurtis wrote: > No need for namespace {}. Media style is static const. Done. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:33: On 2013/02/20 00:17:38, DaleCurtis wrote: > Extra line. Done. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:55: Terminate(); On 2013/02/20 00:17:38, DaleCurtis wrote: > How about a more descriptive name than Terminate() since we've already got > Shutdown() ? How about DestroyPulse()? https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:70: bool AudioManagerPulse::CanShowAudioInputSettings() { On 2013/02/20 00:17:38, DaleCurtis wrote: > Delete. Done. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:88: void AudioManagerPulse::ShowAudioInputSettings() { On 2013/02/20 00:17:38, DaleCurtis wrote: > Same as AudioManagerLinux, just make it a static there and forward the call > here. Done. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:123: if (!device_names->empty()) { On 2013/02/20 00:17:38, DaleCurtis wrote: > Should this be "if (device_name->empty())" ? Otherwise a comment on what you're > doing here would be nice. No, we append the default device on top of the list if the list is not empty. In case there is no device on the machine, we should not add the default device. Done with adding a comment. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:279: return; On 2013/02/20 00:17:38, DaleCurtis wrote: > indent is off. Done. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:285: info->name)); On 2013/02/20 00:17:38, DaleCurtis wrote: > Indent is off. Done. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... File media/audio/pulse/audio_manager_pulse.h (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.h:27: virtual bool CanShowAudioInputSettings() OVERRIDE; On 2013/02/20 00:17:38, DaleCurtis wrote: > You'll need to rebase, this was deleted a few revisions ago. Yes, I realized when Dylan pointed it out in another thread. Done with removing this function. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.h:57: static void SamplerateInfoCallback(pa_context* context, On 2013/02/20 00:17:38, DaleCurtis wrote: > s/Samplerate/SampleRate/ > Done. > Is there a reason you didn't put all this in PulseUtil? Two reasons: we should only query the native sample rate in AudioManagerPulse; This callback needs to cast the user_data to a specific object, and access the members in the object, so it needs to be a static member function. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:85: int err = pa_stream_connect_record( On 2013/02/20 00:17:38, DaleCurtis wrote: > There's no supporting documentation on the return value here. Are you just > guessing? Is there a doc elsewhere? > > http://freedesktop.org/software/pulseaudio/doxygen/stream_8h.html#abfd34293aa... I did not find the document to explain the returned value. It is the same as pa_stream_connect_playback here, and from the sample code, a negative value will be returned if error, otherwise 0. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:128: On 2013/02/20 00:17:38, DaleCurtis wrote: > Clear buffer_ ? True, thanks. Done. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:133: pa_operation* operation = pa_stream_cork(handle_, 0, NULL, NULL); On 2013/02/20 00:17:38, DaleCurtis wrote: > Can this hang or fail w/o a success callback? I am not sure if I understand the question. Are you asking if pa_stream_cork will hang or fail? I am honestly not sure, but I never see one. If this can hang, preferably we use pa_operation_unref() for this operation, then we don't block the audio thread. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:145: On 2013/02/20 00:17:38, DaleCurtis wrote: > Side note: I really dislike that callback_ isn't set to NULL here, even though > all the other input streams do this so they can issue OnClose(). Can we do > something about this? It feels like a use-after-free just waiting to happen. I remember it was only the speech recognition using the OnClose() callback, but I am not sure here. Looking at AudioInputController::OnClose(AudioInputStream* stream), it seems to be just a dummy function. If so, I think we can remove this OnClose(). But we should do it on a separate CL. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:191: size_t index = pa_stream_get_device_index(handle_); On 2013/02/20 00:17:38, DaleCurtis wrote: > Should this just be done during open and the values cached? Not really, the device can be changed on the fly. For example, user uses the default device to open the stream, then he change the default device to another device via the OS sound setting, in this case the cached value is not valid anymore. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:222: WaitForOperationCompletion(pa_mainloop_, operation); On 2013/02/20 00:17:38, DaleCurtis wrote: > Is it worth doing this or should you just cache the volume_ value during the > initial set? The volume can be changed by different ways. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:244: if (error) { On 2013/02/20 00:17:38, DaleCurtis wrote: > The docs indicate that value is "eol", without elaboration :( > > http://freedesktop.org/software/pulseaudio/doxygen/introspect_8h.html#a64473f... pulse has a very poor documentation. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:249: if (stream->channels_ != info->channel_map.channels) On 2013/02/20 00:17:38, DaleCurtis wrote: > Why would these values differ? This seems sketchy. Same reason as the comment in line 191, the device can be changed on the fly. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:294: while (true) { On 2013/02/20 00:17:38, DaleCurtis wrote: > while (pa_stream_readable_size(handle_) > 0) instead? > > or while (pa_stream_peek() && data && length) { > Append() > } > > ? I changed it to use a do { } while (a_stream_readable_size(handle_) > 0) https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:322: base::PlatformThread::Sleep( On 2013/02/20 00:17:38, DaleCurtis wrote: > The general delay for this is 5ms. Done. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.h (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.h:64: // entered an unrecoverable error state, or the Stio() has executed. On 2013/02/20 00:17:38, DaleCurtis wrote: > Stio? Oh, the comment is not valid any more, removed. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... File media/audio/pulse/pulse_util.cc (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.cc:96: ch = static_cast<Channels>(ch + 1)) { On 2013/02/20 00:17:38, DaleCurtis wrote: > Indent. Done. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... File media/audio/pulse/pulse_util.h (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.h:15: On 2013/02/20 00:17:38, DaleCurtis wrote: > WDYT about putting these in a pulse sub-namespace under media? I.e. > media::pulse::() or a class similar to henrika's CoreAudioUtil? Done with adding the media::pulse namespace https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_u... media/audio/pulse/pulse_util.h:17: class AutoPulseLock { On 2013/02/20 00:17:38, DaleCurtis wrote: > Just inline the implementation like base/lock.h Done. https://codereview.chromium.org/10952024/diff/62001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/10952024/diff/62001/media/media.gyp#newcode617 media/media.gyp:617: 'generate_stubs_script': '../tools/generate_stubs/generate_stubs.py', On 2013/02/20 00:17:38, DaleCurtis wrote: > For the variables which are only used once, just put them directly in the > strings below. Only sig_files and outfile_type are used once, I hope it is fine to put them as variables to help readability. https://codereview.chromium.org/10952024/diff/62001/media/media.gyp#newcode631 media/media.gyp:631: '..', On 2013/02/20 00:17:38, DaleCurtis wrote: > Is .. necessary for this? We need to include the output_root since the header pulse_stubs.h is locating there. But not .. https://codereview.chromium.org/10952024/diff/62001/media/media.gyp#newcode633 media/media.gyp:633: 'dependencies': [ On 2013/02/20 00:17:38, DaleCurtis wrote: > base is already a dependency elsewhere. Removed. https://codereview.chromium.org/10952024/diff/62001/media/media.gyp#newcode637 media/media.gyp:637: 'direct_dependent_settings': { On 2013/02/20 00:17:38, DaleCurtis wrote: > These shouldn't be necessary here since you're in target. Removed
ping
lgtm https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:133: pa_operation* operation = pa_stream_cork(handle_, 0, NULL, NULL); On 2013/02/20 14:43:38, xians1 wrote: > On 2013/02/20 00:17:38, DaleCurtis wrote: > > Can this hang or fail w/o a success callback? > > I am not sure if I understand the question. > Are you asking if pa_stream_cork will hang or fail? > I am honestly not sure, but I never see one. If this can hang, preferably we use > pa_operation_unref() for this operation, then we don't block the audio thread. When I was writing PulseOutput I ran into situations where certain methods would hang if they didn't have a callback which signaled the mainloop. If this works at all it should be fine. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:145: On 2013/02/20 14:43:38, xians1 wrote: > On 2013/02/20 00:17:38, DaleCurtis wrote: > > Side note: I really dislike that callback_ isn't set to NULL here, even though > > all the other input streams do this so they can issue OnClose(). Can we do > > something about this? It feels like a use-after-free just waiting to happen. > > I remember it was only the speech recognition using the OnClose() callback, but > I am not sure here. > > Looking at AudioInputController::OnClose(AudioInputStream* stream), it seems to > be just a dummy function. > If so, I think we can remove this OnClose(). > > But we should do it on a separate CL. Awesome, yes please! https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:191: size_t index = pa_stream_get_device_index(handle_); On 2013/02/20 14:43:38, xians1 wrote: > On 2013/02/20 00:17:38, DaleCurtis wrote: > > Should this just be done during open and the values cached? > > Not really, the device can be changed on the fly. For example, user uses the > default device to open the stream, then he change the default device to another > device via the OS sound setting, in this case the cached value is not valid > anymore. Hmmm, we'll need an AudioDeviceListenerPulse or something for output then, because if the device changes and the sample rate is different we'll get glitching. https://codereview.chromium.org/10952024/diff/69001/media/audio/pulse/audio_m... File media/audio/pulse/audio_manager_pulse.cc (right): https://codereview.chromium.org/10952024/diff/69001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:77: pulse::AutoPulseLock auto_lock(input_mainloop_); Could add pulse::AutoPulseLock and pulse::WaitFor... to the "using" clause above to avoid this prefixing.
https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... File media/audio/pulse/pulse_input.cc (right): https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:133: pa_operation* operation = pa_stream_cork(handle_, 0, NULL, NULL); On 2013/02/21 21:54:21, DaleCurtis wrote: > On 2013/02/20 14:43:38, xians1 wrote: > > On 2013/02/20 00:17:38, DaleCurtis wrote: > > > Can this hang or fail w/o a success callback? > > > > I am not sure if I understand the question. > > Are you asking if pa_stream_cork will hang or fail? > > I am honestly not sure, but I never see one. If this can hang, preferably we > use > > pa_operation_unref() for this operation, then we don't block the audio thread. > > When I was writing PulseOutput I ran into situations where certain methods would > hang if they didn't have a callback which signaled the mainloop. If this works > at all it should be fine. I will keep an eye on this. https://codereview.chromium.org/10952024/diff/62001/media/audio/pulse/pulse_i... media/audio/pulse/pulse_input.cc:191: size_t index = pa_stream_get_device_index(handle_); On 2013/02/21 21:54:21, DaleCurtis wrote: > On 2013/02/20 14:43:38, xians1 wrote: > > On 2013/02/20 00:17:38, DaleCurtis wrote: > > > Should this just be done during open and the values cached? > > > > Not really, the device can be changed on the fly. For example, user uses the > > default device to open the stream, then he change the default device to > another > > device via the OS sound setting, in this case the cached value is not valid > > anymore. > > Hmmm, we'll need an AudioDeviceListenerPulse or something for output then, > because if the device changes and the sample rate is different we'll get > glitching. Please point it out if I miss something. We don't need device listener for Pulse if we are using the native pulse sample rate. The native pulse sample rate is the sample rate that pulse server is running on, which is configurable in pulse.conf. No matter what device is being used, pulse is running on its sample rate and won't change. https://codereview.chromium.org/10952024/diff/69001/media/audio/pulse/audio_m... File media/audio/pulse/audio_manager_pulse.cc (right): https://codereview.chromium.org/10952024/diff/69001/media/audio/pulse/audio_m... media/audio/pulse/audio_manager_pulse.cc:77: pulse::AutoPulseLock auto_lock(input_mainloop_); On 2013/02/21 21:54:22, DaleCurtis wrote: > Could add pulse::AutoPulseLock and pulse::WaitFor... to the "using" clause above > to avoid this prefixing. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10952024/80001
Message was sent while issue was closed.
Change committed as 184122 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
