Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5859003:
Add ALSA support to volume keys (Closed)
|
Created:
10 years ago by davejcool Modified:
9 years, 7 months ago CC:
chromium-reviews, davemoore+watch_chromium.org Visibility:
Public. |
DescriptionAdd ALSA support to volume keys
If PulseAudio is running, everything should behave as before, otherwise use ALSA API for adjusting volume. The previous PulseAudioMixer was split into AudioMixerBase and audioMixerPusle, then AudioMixerAlsa was added.
BUG=chromium-os:10470
TEST=Volume keys should work even if pulseaudio disabled
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71115
Patch Set 1 : Adding ALSA volume support #
Total comments: 7
Patch Set 2 : cleanups #
Total comments: 62
Patch Set 3 : Updated from comments #Patch Set 4 : more cleanup #
Total comments: 5
Patch Set 5 : fix nits #
Total comments: 3
Patch Set 6 : cleaning #
Total comments: 1
Patch Set 7 : holding lock during operations #
Total comments: 2
Patch Set 8 : alphabetize headers #Patch Set 9 : base/thread.h now in base/threading/thread.h #Patch Set 10 : using snd_mixer_selem_id_malloc #
Total comments: 32
Patch Set 11 : Responded to comments #
Total comments: 1
Patch Set 12 : fix comment #
Messages
Total messages: 18 (0 generated)
Here's a medium-ish sized code review...
I didn't look at the ALSA code with the hope that someone else on the list is more qualified. http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.cc (right): http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:137: ++using_mixer_; this seems fragile; please spell out the transitions between the different values instead http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:175: LOG(ERROR) << "Lost connection to Mixer"; nit: make all of the "mixer"s in these comments lowercase http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.h (right): http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.h:75: int using_mixer_; use an enum instead, e.g. MixerType with MIXER_TYPE_PULSE_AUDIO and MIXER_TYPE_ALSA values http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.h (right): http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:28: bool Init(InitDoneCallback* callback); chrome style is to put "virtual" in front of all implementations of virtual methods
Integrated initial suggestions. http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.cc (right): http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:137: ++using_mixer_; On 2010/12/20 22:01:12, Daniel Erat wrote: > this seems fragile; please spell out the transitions between the different > values instead Now logic is in UseNextMixer() http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.h (right): http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.h:75: int using_mixer_; On 2010/12/20 22:01:12, Daniel Erat wrote: > use an enum instead, e.g. MixerType with MIXER_TYPE_PULSE_AUDIO and > MIXER_TYPE_ALSA values Funny, I went back and forth on that one... but it will make things clearer, even if it does create more lines than just doing "++using_mixer". http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.h (right): http://codereview.chromium.org/5859003/diff/12001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:28: bool Init(InitDoneCallback* callback); On 2010/12/20 22:01:12, Daniel Erat wrote: > chrome style is to put "virtual" in front of all implementations of virtual > methods Thanks, done.
http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.cc (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:116: return true; Prefer early-return only on error cases, and return true at the end. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:134: mixer_->GetVolumeLimits(&min_volume_db_, &max_volume_db_); undefined mXX_volume_db_ on error? http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:138: max_volume_db_ = kMaxVolumeDb; Suggest moving this into a utility function. Something like ClipVolume(&, &). http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:25: : min_volume_(-90), Define a const for this. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:153: (alsa_mixer_ == NULL)) can be in one line. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:209: } return false. Same below. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:294: alsa_long_t db_lo_int, db_hi_int; Better initialize them just in case those snd_xxx api failed. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:300: check vol_hi != vol_lo http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:325: int * switched) const { space after * http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:326: snd_mixer_selem_get_playback_switch(elem, (snd_mixer_selem_channel_id_t)0, use static_cast to be consistent. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:335: VLOG(1) << "Set playback switch" << snd_mixer_selem_get_name(elem) space here after ' switch'?
http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.cc (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:12: #include "chrome/browser/chromeos/audio_mixer_alsa.h" nit: alphabetize include order http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:13: // asynchronously on the worker thread. I think it'd be good to expand this to explain the relationship between master and PCM looking at the code it looks like "volume = master + PCM" but I'm not completely sure http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:20: const char* kMasterVolume = {"Master"}; nits: - no need to have this indented - also do you need the { } ? typically do without http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:25: : min_volume_(-90), magic number alert! do we have a constant for this somewhere? http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:28: mixer_state_lock_(), nit: no need to list variables that have default constructors http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:33: thread_(NULL) { nit: no need to list variables that have default constructors (scoped_ptr will initialize to NULL) http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:49: AutoLock lock(mixer_state_lock_); nit: over indented do we need to lock + update state if the object is going away after this function exits? http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:60: thread_->message_loop()->PostTask(FROM_HERE, we only seem to use this thread for doing initialization... you don't have to do the change now but perhaps there's an existing thread we can use to do initialization instead of spinning one up and then not using it http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:61: NewRunnableMethod(this, &AudioMixerAlsa::DoInit, callback)); nit: indent by extra 2 spaces http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:118: return (switched) ? false : true; instead of having callees do the int<->bool conversion I'd suggest doing it inside Set/GetElementSwitched() to keep callees looking cleaner http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:124: int new_value = mute ? 0 : 1; ditto http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:152: if ((mixer_state_ == READY) && nit: condition can fit on one line http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:189: AutoLock lock(mixer_state_lock_); sanity check: did you intend to lock for this entire function? http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:193: // ALSA hardware devices start at 0 nit: end comment w/ period http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:195: if ((snd_card_next(&device_id_) < 0) || (device_id_ < 0)) { so this selects the first ALSA device found on the machine? http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:201: snd_mixer_t* handle = NULL; you may want to consider defining a scoped_ptr_malloc that calls snd_mixer_close() when setting the ptr goes out of scope http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:204: snprintf(card, sizeof(card), "hw:%i", device_id_); nit: use base/stringprintf.h, std::string and c_str() instead http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:225: if (err < 0) { nit: for consistency might as well put snd_mixer_load(handle) inside the condition http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:265: snd_mixer_elem_t* AudioMixerAlsa::FindElementWithName(snd_mixer_t* handle, nit: these two arguments should be dropped to next line + indented by 4 spaces http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:282: snd_mixer_selem_get_playback_dB(elem, (snd_mixer_selem_channel_id_t)0, no c-style casts http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:324: bool AudioMixerAlsa::GetElementSwitched(snd_mixer_elem_t* elem, this function always return true... maybe it should return the switch value itself? http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:331: bool AudioMixerAlsa::SetElementSwitched(snd_mixer_elem_t* elem, this function always return true.. perhaps it should be void http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:341: AutoLock lock(mixer_state_lock_); nit: over indented http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.h (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:9: #include <alsa/asoundlib.h> avoid system includes in header files at all costs and instead forward declare all necessary ALSA types http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:16: nit: no blank line http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:23: nit: no blank line http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:25: ~AudioMixerAlsa(); add virtual http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:66: bool GetElementSwitched(snd_mixer_elem_t* elem, maybe these should be renamed IsElementMuted / SetElementMuted ? Switched seems like a strange choice of wording (even though it's what ALSA uses) http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_base.h (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_base.h:14: const double kSilenceDb = -200.0; this needs a comment + move the actual assignment to the .cc http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_base.h:16: class AudioMixerBase { this looks more like an interface rather than a base class I'd rename this simply AudioMixer http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_base.h:23: IN_ERROR nit: add trailing , http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_base.h:26: AudioMixerBase() {} avoid inlining virtual methods (i.e., put these in a .cc) http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_base.h:40: virtual void GetVolumeLimits(double* vol_min, double* vol_max) = 0; needs a comment is it blocking? etc.. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_base.h:42: // Non-blocking call. do all these blocking comments apply to both pulse and alsa? or is it a contract of the interface that this not be a blocking call? http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_base.h:57: protected: believe this should be private (no need to have subclasses have access to copy constructor, etc...) http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_base.h:58: nit: no blank line http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_pulse.cc (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_pulse.cc:76: NewRunnableMethod(this, &AudioMixerPulse::DoInit, callback)); nit: indent by two extra spaces http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_pulse.h (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_pulse.h:14: nit: no blank line http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_pulse.h:29: ~AudioMixerPulse(); add virtual
Thanks for all the great feedback! It's looking cleaner now. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.cc (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:116: return true; On 2010/12/22 22:21:26, zhurunz wrote: > Prefer early-return only on error cases, and return true at the end. Fixed. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:134: mixer_->GetVolumeLimits(&min_volume_db_, &max_volume_db_); On 2010/12/22 22:21:26, zhurunz wrote: > undefined mXX_volume_db_ on error? Values are unchanged on error, but now GetVolumeLimits is bool to detect error case. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:20: const char* kMasterVolume = {"Master"}; On 2010/12/22 22:31:58, scherkus wrote: > nits: > - no need to have this indented > - also do you need the { } ? typically do without :-) Microsoft + 1990's "C" = {"Funny Braces"} http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:25: : min_volume_(-90), On 2010/12/22 22:31:58, scherkus wrote: > magic number alert! do we have a constant for this somewhere? We do now! http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:60: thread_->message_loop()->PostTask(FROM_HERE, I just wanted the init to happen away from everything else (not doing it on the UI or IO threads)... but if putting it on say the IO thread is OK, that would simplify things. Or how about killing the thread once initialization completes? http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:118: return (switched) ? false : true; Yup, removing the 'switched' language from Get/SetElement simplified this up as well. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:189: AutoLock lock(mixer_state_lock_); On 2010/12/22 22:31:58, scherkus wrote: > sanity check: did you intend to lock for this entire function? In this case I did intend this. The initialization is much more straightforward than with Pulse, not to mention a lot faster (I timed it at 0.002 ms!) http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:195: if ((snd_card_next(&device_id_) < 0) || (device_id_ < 0)) { On 2010/12/22 22:31:58, scherkus wrote: > so this selects the first ALSA device found on the machine? Oops... this was left over from earlier testing. We're just using the default sound device now. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:201: snd_mixer_t* handle = NULL; On 2010/12/22 22:31:58, scherkus wrote: > you may want to consider defining a scoped_ptr_malloc that calls > snd_mixer_close() when setting the ptr goes out of scope Thanks, I'll have learn more about that one. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:331: bool AudioMixerAlsa::SetElementSwitched(snd_mixer_elem_t* elem, On 2010/12/22 22:31:58, scherkus wrote: > this function always return true.. perhaps it should be void Ahh, of course! In a previous incarnation the function could fail if the element wasn't found, but now it's passed in. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.h (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:9: #include <alsa/asoundlib.h> On 2010/12/22 22:31:58, scherkus wrote: > avoid system includes in header files at all costs and instead forward declare > all necessary ALSA types OK, I considered that too until I saw several header files elsewhere including the same asoundlib.h. http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_base.h (right): http://codereview.chromium.org/5859003/diff/19001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_base.h:14: const double kSilenceDb = -200.0; On 2010/12/22 22:31:58, scherkus wrote: > this needs a comment + move the actual assignment to the .cc It seems a const can't be defined elsewhere except at the declaration. It looks like making it a static member of the class and defining it there is an accepted practice.
If this looks good now, I'd like to check this in soon. Thanks!
Just a few nits; I didn't look at the ALSA or PulseAudio code in detail. http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.cc (right): http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:162: if (TryToConnect(true)) { I think that Chrome style is to omit curly braces for single-line statements, like: if (TryToConnect(true)) break; (This matches what you have elsewhere in the file, too.) http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.h (right): http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.h:82: MixerType using_mixer_; Could you rename this member to something like mixer_type_? using_mixer_ makes me think it's a bool describing whether the mixer is being used or not. http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer.cc (right): http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.cc:9: AudioMixer::AudioMixer() { Any reason that these can't just go in the header?
http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.h (right): http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.h:82: MixerType using_mixer_; On 2011/01/05 01:06:53, Daniel Erat wrote: > Could you rename this member to something like mixer_type_? using_mixer_ makes > me think it's a bool describing whether the mixer is being used or not. Good point. Done. http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer.cc (right): http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.cc:9: AudioMixer::AudioMixer() { On 2011/01/05 01:06:53, Daniel Erat wrote: > Any reason that these can't just go in the header? I moved them into a .cc after Andrew's comment to: "avoid inlining virtual methods (i.e., put these in a .cc)". The destructor is virtual, so perhaps empty virtual functions would be an exception?
On Tue, Jan 4, 2011 at 5:42 PM, <davej@chromium.org> wrote: > > http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... > File chrome/browser/chromeos/audio_handler.h (right): > > http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... > chrome/browser/chromeos/audio_handler.h:82: MixerType using_mixer_; > On 2011/01/05 01:06:53, Daniel Erat wrote: >> >> Could you rename this member to something like mixer_type_? > > using_mixer_ makes >> >> me think it's a bool describing whether the mixer is being used or > > not. > > Good point. Done. > > http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... > File chrome/browser/chromeos/audio_mixer.cc (right): > > http://codereview.chromium.org/5859003/diff/32002/chrome/browser/chromeos/aud... > chrome/browser/chromeos/audio_mixer.cc:9: AudioMixer::AudioMixer() { > On 2011/01/05 01:06:53, Daniel Erat wrote: >> >> Any reason that these can't just go in the header? > > I moved them into a .cc after Andrew's comment to: "avoid inlining > virtual methods (i.e., put these in a .cc)". > > The destructor is virtual, so perhaps empty virtual functions would be > an exception? I think that his advice was referring to http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in.... , which also says "You CAN inline virtual destructors in pure abstract base classes." > http://codereview.chromium.org/5859003/ >
http://codereview.chromium.org/5859003/diff/39001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/5859003/diff/39001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:76: if (mixer_state_ != READY) Do we need a lock here? Or just call MixerReady() http://codereview.chromium.org/5859003/diff/39001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:79: double vol_total = 0; "0.0" is better http://codereview.chromium.org/5859003/diff/39001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:274: alsa_long_t long_vol; = 0; In case the API failed.
To ensure proper shutdown, I changed to holding the lock for the duration of the call instead of just for the check if ready for the Get/Set calls. http://codereview.chromium.org/5859003/diff/44001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/5859003/diff/44001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:72: if (mixer_state_ != READY) Changing to locking for the duration of the call. The previous calls to MixerReady() did not keep the lock held after the check, which could have caused issues if shutting down before the function exited. These have been replaced with an AutoLock for the duration of the function
Keeping lock held during operations so FreeAlsaMixer will not close mixer if another operation is still being done. http://codereview.chromium.org/5859003/diff/24002/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/5859003/diff/24002/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:117: if (mute) { Calling DoGetvolumeDb instead of the public GetVolumeDb since acquiring a lock cannot be nested. http://codereview.chromium.org/5859003/diff/24002/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:240: Putting DoSet/DoGet here so it can be called when the lock is already being held (e.g. from SetMute).
Headers changed in project (base/thread.h ==> base/threading/thread.h)
I think this is ready to check in now. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:276: Using the ALSA macro snd_mixer_selem_id_alloca(&sid) causes the warning, "the address of 'sid' will always evaluate as 'true'. Somewhere in the macro there must be a test on &sid for non-NULL which causes this warning. So I switched to using the corresponding _id_malloc and _id_free calls instead and that resolved the warning. Now the try bots are happy.
few more comments but looking good sorry for taking a while to review! http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.cc (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:142: } I'd early return here + get rid of the else clause http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:157: min_volume_db_(-90), do we have a constant for this? http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:160: if (TryToConnect(true)) I made a similar comment in another file, but this loop doesn't really do what we think it's doing here... since we're doing async connect what this loop *actually* does it try to connect and only switch to the next mixer if the first mixer fails to create a thread (not actually initialize!) we're likely to always break here and then wait for OnMixerInitialized() to execute I'm wondering if this should simply be changed to "TryToConnect(true)" with a comment describing what's happening http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:195: connected_ = TryToConnect(false); so to confirm (as a sanity check) we'll do a synchronous audio connection on the UI thread if needed? do we know how often this can get triggered? http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.h (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.h:50: MIXER_TYPE_NONE nit: add trailing , http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.h:81: // Which mixer is being used, PulseAudio or ALSA nit: period for comments http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer.h (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:24: AudioMixer() {} don't inline ctors / dtors (might have to write a .cc unfortunately!) http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:30: virtual bool Init(InitDoneCallback* callback) = 0; I'm inclined to say either use the callback to report status or use a return value but avoid both as its confusing in the "false" case (what happens to my callback? does it get executed with false as well? does it ever get executed? cleanup etc...?) currently both the implementations would return false only if creating a thread failed, in which case we have bigger issues to worry about! also you can achieve the same effect of reporting thread-creation-failure by executing the callback on the callee's thread with a value of false http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:33: virtual bool InitSync() = 0; we can consider deferring to a later change but I'm wondering if it makes sense to push the async+sync initialization down to each AudioMixer implementation when it's only AudioHandler that really need to have that flexibility what do you think about standardizing on an asynchronous Initialization() for AudioMixers and then having AudioHandler use a WaitableEvent to implement its synchronous version? Just a thought (and probably suitable after this CL lands) http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:55: virtual State CheckState() const = 0; nit: I think this used to be called CheckState() because it would attempt a re-connection... given that it simply is returning the state we can call this GetState() what do you think? (my memory might be a big foggy as well here so correct me if wrong!) http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:58: static const double kSilenceDb = -200.0; constants at top of class declarations + move the assignment to .cc http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:55: return false; what happens to the callback? http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.h (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:26: // Implementation of AudioMixer, see AudioMixerBase for explanations. nit: remove ", see AudioMixerBase for explanations." I also don't think AudioMixerBase exists anymore! http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:49: double DoGetVolumeDb() const; nit: a style we've used in media code is to append _Locked() as the method name to offer a hint that it assumes lock is held http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:52: struct _snd_mixer_elem* FindElementWithName(struct _snd_mixer* handle, question: do we actually have to use "struct _snd_mixer_elem" or can't we just use "_snd_mixer_elem" ? I thought C++ let you do that but I'm not 100% sure http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:53: const char* element_name) const; since params don't find drop both params to next line + indent by 4 http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_pulse.cc (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_pulse.cc:69: return false; what happens to the callback? http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_pulse.h (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_pulse.h:30: // Implementation of AudioMixer, see AudioMixerBase for explanations. ditto on AudioMixerBase comment
Made some changes after Andrew's cavalcade of comments. Thanks, Andrew! http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.cc (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:142: } On 2011/01/11 00:24:12, scherkus wrote: > I'd early return here + get rid of the else clause Done. Much cleaner. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:157: min_volume_db_(-90), On 2011/01/11 00:24:12, scherkus wrote: > do we have a constant for this? Yes, it is fine to use our existing constants. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:160: if (TryToConnect(true)) On 2011/01/11 00:24:12, scherkus wrote: > I made a similar comment in another file, but this loop doesn't really do what > we think it's doing here... > > since we're doing async connect what this loop *actually* does it try to connect > and only switch to the next mixer if the first mixer fails to create a thread > (not actually initialize!) > > we're likely to always break here and then wait for OnMixerInitialized() to > execute > > I'm wondering if this should simply be changed to "TryToConnect(true)" with a > comment describing what's happening After simplifying Init() so you must listen to the callback for success/failure, this goofy case did indeed go away. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:195: connected_ = TryToConnect(false); On 2011/01/11 00:24:12, scherkus wrote: > so to confirm (as a sanity check) we'll do a synchronous audio connection on the > UI thread if needed? > > do we know how often this can get triggered? I've had it happen once after hours of usage, where the system had to restart pulseaudio and this case would have reconnected. It is on the UI thread, but a rare case. I could change this to init asynchronously, but it adds a bit more complexity for a very uncommon case. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer.h (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:24: AudioMixer() {} On 2011/01/11 00:24:12, scherkus wrote: > don't inline ctors / dtors (might have to write a .cc unfortunately!) The style guide says it is fine to inline virtual destructors in pure abstract base classes. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:30: virtual bool Init(InitDoneCallback* callback) = 0; On 2011/01/11 00:24:12, scherkus wrote: > I'm inclined to say either use the callback to report status or use a return > value but avoid both as its confusing in the "false" case (what happens to my > callback? does it get executed with false as well? does it ever get executed? > cleanup etc...?) > > currently both the implementations would return false only if creating a thread > failed, in which case we have bigger issues to worry about! > > also you can achieve the same effect of reporting thread-creation-failure by > executing the callback on the callee's thread with a value of false Thanks, done. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:33: virtual bool InitSync() = 0; On 2011/01/11 00:24:12, scherkus wrote: > we can consider deferring to a later change but I'm wondering if it makes sense > to push the async+sync initialization down to each AudioMixer implementation > when it's only AudioHandler that really need to have that flexibility > > what do you think about standardizing on an asynchronous Initialization() for > AudioMixers and then having AudioHandler use a WaitableEvent to implement its > synchronous version? > > Just a thought (and probably suitable after this CL lands) I like the idea- it would simplify the AudioMixer interface just that much more. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:55: virtual State CheckState() const = 0; On 2011/01/11 00:24:12, scherkus wrote: > nit: I think this used to be called CheckState() because it would attempt a > re-connection... given that it simply is returning the state we can call this > GetState() > > what do you think? (my memory might be a big foggy as well here so correct me if > wrong!) Yes, GetState() Now makes more sense. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer.h:58: static const double kSilenceDb = -200.0; On 2011/01/11 00:24:12, scherkus wrote: > constants at top of class declarations + move the assignment to .cc Moved to the top where it belongs. I've seen constants like this defined in similar header files, so I'd like to keep it here... (plus, otherwise a .cc would be required for just this single constant?) http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.cc:55: return false; On 2011/01/11 00:24:12, scherkus wrote: > what happens to the callback? Deleting callback if not used now. http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_alsa.h (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_alsa.h:52: struct _snd_mixer_elem* FindElementWithName(struct _snd_mixer* handle, On 2011/01/11 00:24:12, scherkus wrote: > question: do we actually have to use "struct _snd_mixer_elem" or can't we just > use "_snd_mixer_elem" ? > > I thought C++ let you do that but I'm not 100% sure Eventually it'll stick that 'struct' is mostly superfluous now days :-) http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_mixer_pulse.cc (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_mixer_pulse.cc:69: return false; On 2011/01/11 00:24:12, scherkus wrote: > what happens to the callback? Ahhh! This callback wasn't being deleted in earlier versions either.
LGTM! looks really awesome! http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... File chrome/browser/chromeos/audio_handler.cc (right): http://codereview.chromium.org/5859003/diff/63001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:195: connected_ = TryToConnect(false); On 2011/01/11 02:52:54, davejcool wrote: > On 2011/01/11 00:24:12, scherkus wrote: > > so to confirm (as a sanity check) we'll do a synchronous audio connection on > the > > UI thread if needed? > > > > do we know how often this can get triggered? > > I've had it happen once after hours of usage, where the system had to restart > pulseaudio and this case would have reconnected. It is on the UI thread, but a > rare case. > > I could change this to init asynchronously, but it adds a bit more complexity > for a very uncommon case. Cool -- thanks for explainin! http://codereview.chromium.org/5859003/diff/79001/chrome/browser/chromeos/aud... chrome/browser/chromeos/audio_handler.cc:161: break; nit: add () to function names and get rid of blank lines since comment goes w/ code |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
