Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11344005:
Handle audio device changes on Mac (Closed)
|
Created:
8 years, 1 month ago by sail Modified:
8 years, 1 month ago Reviewers:
Scott Hess - ex-Googler, Chris Rogers, no longer working on chromium, DaleCurtis, henrika (OOO until Aug 14) CC:
chromium-reviews, feature-media-reviews_chromium.org, Chris Rogers Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandle audio device changes on Mac
This is the Mac version of this Windows CL: https://codereview.chromium.org/11233023/
With this CL we now observer the default audio output device setting. When the setting changes we fire the AudioManagerMac::NotifyAllOutputDeviceChangeListeners notificaiton. This causes the output stream to be recreated and target the new default device.
BUG=156788
TEST=
Web Audio: Play audio from http://chromium.googlecode.com/svn/trunk/samples/audio/shiny-drum-machine.html
Go to system preferences, change the default audio output device.
Verify that audio is now playing for the new device.
Flash Audio: Play a youtube video. Do the same as above.
Patch Set 1 #Patch Set 2 : bind to right thread #
Total comments: 14
Patch Set 3 : address review comments #Patch Set 4 : add thread check #
Total comments: 15
Patch Set 5 : address review comments #Patch Set 6 : rebase #Patch Set 7 : locking #
Total comments: 9
Patch Set 8 : address review comments #
Total comments: 18
Patch Set 9 : fix crash #
Total comments: 1
Patch Set 10 : address review comments #
Total comments: 6
Patch Set 11 : cleanup #Patch Set 12 : " #
Messages
Total messages: 28 (0 generated)
Nice work! I didn't know anyone was working on this!! In my local testing my mac handled device changes without any code changes, so I didn't think this was necessary. Semi related, if you change the channel layout on the system which is failing the device change tests for you, does it also fail during the AUAudioOutputStream::Open() call? I dropped channel mixing for M24 because crogers and I both found OSX was doing the "right" thing behind the scenes. +crogers to confirm some more of the mac specific bits. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:12: : listener_cb_(listener_cb) { 4 space indent. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:14: kAudioHardwarePropertyDefaultOutputDevice, Can you add a listener for preferred sample rate as well? Otherwise AudioOutputResampler will probably still break. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:18: AudioObjectAddPropertyListener( Do you need to be error checking these things? http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:33: AudioObjectRemovePropertyListener( Ditto? http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.h (right): http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:21: explicit AudioDeviceListenerMac(const base::Closure& listener_cb); Include comment from the Windows version that |listener_cb| is a permanent callback. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:27: static OSStatus OnDefaultDeviceChanged( Just make this a static method within the class since it gets the pointer through |content|, then you can drop the #include <CoreAudio> too. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:239: output_device_listener_.reset(new AudioDeviceListenerMac(BindToLoop( You need to do this in the InitializeOnAudioThread() method, you can't pass "this" out to another thread during construction; it'll lead to races. Which means you'll need to follow the same pattern as AUdioManagerWin to keep thread checker happy.
Thanks for the quick review! I've addressed all the review comments. I also found another issue with my code. I assumed that the system callback was always on the same thread that added the listener. It turns out that's not true. By default the callback is on the UI thread but this can change if some other part of the code changes the value of kAudioHardwarePropertyRunLoop. I've updated AudioDeviceListenerMac to handle this situation. It now keeps a reference to the thread that it was created on. When the system callback is invoked we first switch to that thread. The only fallout from this was that AudioDeviceListenerMac now has to be a ref counted object. I couldn't use a weak ptr because those aren't thread safe. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:12: : listener_cb_(listener_cb) { On 2012/10/29 01:58:45, DaleCurtis wrote: > 4 space indent. Done. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:14: kAudioHardwarePropertyDefaultOutputDevice, On 2012/10/29 01:58:45, DaleCurtis wrote: > Can you add a listener for preferred sample rate as well? Otherwise > AudioOutputResampler will probably still break. Added kAudioDevicePropertyNominalSampleRate. Do you know how I can test this? I'm not sure if observing kAudioDevicePropertyNominalSampleRate on kAudioObjectSystemObject will work or not. Thanks. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:18: AudioObjectAddPropertyListener( On 2012/10/29 01:58:45, DaleCurtis wrote: > Do you need to be error checking these things? Done. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:33: AudioObjectRemovePropertyListener( On 2012/10/29 01:58:45, DaleCurtis wrote: > Ditto? Done. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.h (right): http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:21: explicit AudioDeviceListenerMac(const base::Closure& listener_cb); On 2012/10/29 01:58:45, DaleCurtis wrote: > Include comment from the Windows version that |listener_cb| is a permanent > callback. Done. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:27: static OSStatus OnDefaultDeviceChanged( On 2012/10/29 01:58:45, DaleCurtis wrote: > Just make this a static method within the class since it gets the pointer > through |content|, then you can drop the #include <CoreAudio> too. Done. http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_manag... File media/audio/mac/audio_manager_mac.cc (right): http://codereview.chromium.org/11344005/diff/3001/media/audio/mac/audio_manag... media/audio/mac/audio_manager_mac.cc:239: output_device_listener_.reset(new AudioDeviceListenerMac(BindToLoop( On 2012/10/29 01:58:45, DaleCurtis wrote: > You need to do this in the InitializeOnAudioThread() method, you can't pass > "this" out to another thread during construction; it'll lead to races. > > Which means you'll need to follow the same pattern as AUdioManagerWin to keep > thread checker happy. Done.
haven't yet had a chance to look at the code, but it *is* necessary to handle device changes. Currently we crash in some situations, such as switching from built-in to a FireWire device. There can also be sample-rate changes in these types of situations. The situation is a bit muddled at this point, and subject to change as we consolidate the various OSX audio back-ends.
On 2012/10/29 04:55:03, Chris Rogers wrote: > haven't yet had a chance to look at the code, but it *is* necessary to handle > device changes. Currently we crash in some situations, such as switching from > built-in to a FireWire device. There can also be sample-rate changes in these > types of situations. The situation is a bit muddled at this point, and subject > to change as we consolidate the various OSX audio back-ends. You should revert the message loop changes. Your first patch set was correct. The callback is bound to the correct thread by BindToLoop during construction. You don't need to worry about that so long as you use Initialize OnAudio Thread. We can discuss tomorrow.
Thanks for helping out Sailesh! I must also ask the obvious question. What happens for the described scenarios without this patch? I was also under the assumption that at least a default-device switch already worked on Mac. I will be on vacation for the rest of the week and will give OK not to block anything. I am sure that Dale and Chris can take the rest. Use Dale for any threading parts which remains and Chris for Mac specific parts. LGTM. http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:24: OSStatus OnDefaultDeviceChangedCallback( static http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:31: listener->message_loop()->PostTask( As mentioned in the header file. I find it more clear to do all this in the ctor. http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:53: &OnDefaultDeviceChangedCallback, Could you add some reference or comments here regarding what we can detect other than a change in default device? Compare with the Windows version. Also, it is not clear to me where you ensure that detection is for output only. http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.h (right): http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:24: // |listener_cb| must either outlive AudioDeviceListenerMac or the callback Would it be possible to make the style more similar to the Windows version? E.g., set the callback in the ctor etc. It would perhaps also enable us to use one common (platform independent) unit test. WDYT? http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac_unittest.cc (right): http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac_unittest.cc:14: class AudioDeviceListenerMacTest : public testing::Test { As mentioned before. Would it be beneficial to refactor to ensure that we have one common unit test for all platforms. Have not done any detailed thinking but it would be nice. http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac_unittest.cc:37: // Simulate a device change events and ensure we get the right callbacks. event, callback instead of events, callbacks
I just took a quick look at the code, it seems to me that this CL just installs a listener on the default device, and do nothing upon the notification. As Dale pointed out, Mac is capable of handling the device change automatically in most of the cases. To handle the specific case Chris talked about, we might need to restart the audio stream to avoid the crash. Will there be a follow-up to do this? Could you please add a test case in the description to explain in which scenario this CL is necessary? Another concern about this audio_device_listener_mac.cc is that, it looks like the same code as the audio part in content/browser/device_monitor_mac.cc, we probably should use the code there, and register AudioManagerMac to SystemMonitor to get the notification instead. (One benefit of doing this is that SystemMonitor will post the notification on the same thread as the callback registration thread)
https://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devi... File media/audio/mac/audio_device_listener_mac.h (right): https://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devi... media/audio/mac/audio_device_listener_mac.h:24: // |listener_cb| must either outlive AudioDeviceListenerMac or the callback I've checked Dale's test again and I don't think it is worth the work to make this stuff platform independent. Feel free to ignore my comments on this area.
On 2012/10/29 09:04:39, xians1 wrote: > I just took a quick look at the code, it seems to me that this CL just installs > a listener on the default device, and do nothing upon the notification. > > As Dale pointed out, Mac is capable of handling the device change automatically > in most of the cases. To handle the specific case Chris talked about, we might > need to restart the audio stream to avoid the crash. Will there be a follow-up > to do this? Could you please add a test case in the description to explain in > which scenario this CL is necessary? > Henrik pointed out that NotifyAllOutputDeviceChangeListeners will restart the audio stream, so please ignore the relevant question. But hope you agree that it is good to add a specific test case to verify the necessarity of this CL.
https://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devi... File media/audio/mac/audio_device_listener_mac.cc (right): https://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devi... media/audio/mac/audio_device_listener_mac.cc:24: OSStatus OnDefaultDeviceChangedCallback( On 2012/10/29 08:44:07, henrika wrote: > static Functions in anonymous namespace don't need to be static. https://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devi... File media/audio/mac/audio_device_listener_mac_unittest.cc (right): https://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devi... media/audio/mac/audio_device_listener_mac_unittest.cc:37: // Simulate a device change events and ensure we get the right callbacks. On 2012/10/29 08:44:07, henrika wrote: > event, callback instead of events, callbacks Done.
Addressed all review comments. Also, AudioDeviceListenerMac is no longer ref counted. After rebasing to tip of tree there's a crash in the audio code. I'm investigating that separately.
https://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devi... File media/audio/mac/audio_device_listener_mac.cc (right): https://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devi... media/audio/mac/audio_device_listener_mac.cc:58: base::AutoLock auto_lock(lock_); AFAICT, you have this lock to prevent races WRT OnDefaultDeviceChanged() messaging the callback? I'm worried that holding this lock while the callback is being called could lead to deadlock. But I'm not thinking my way out of that box, so maybe I'll ignore it. That said, there is a definite problem, here, as if OnDefaultDeviceChangedCallback() is called after this has gotten the lock, it could end up messaging a destructed |this|.
http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:53: &OnDefaultDeviceChangedCallback, On 2012/10/29 08:44:07, henrika wrote: > Could you add some reference or comments here regarding what we can detect other > than a change in default device? Compare with the Windows version. Also, it is > not clear to me where you ensure that detection is for output only. +1. I suspect you need to use ScopeOutput instead of ScopeGlobal. http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac_unittest.cc (right): http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac_unittest.cc:14: class AudioDeviceListenerMacTest : public testing::Test { On 2012/10/29 08:44:07, henrika wrote: > As mentioned before. Would it be beneficial to refactor to ensure that we have > one common unit test for all platforms. Have not done any detailed thinking but > it would be nice. Since there's only one test, the class is unnecessary. You could roll all this into one TEST() if you want. http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:16: AudioObjectPropertySelector g_observed_properties[] = { static const. Then the name should be styled kObservedProperties[] or kPropertiesToObserve[] http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:58: base::AutoLock auto_lock(lock_); Does this cause races during destruction? I assume not, but watch TSAN. Also, is it enough to hold the lock indefinitely during destruction? What happens in OnDefaultDeviceChanged() if the lock is destructed while another thread is waiting? At best it seems racy, at worst it would be a use after free. If you put an AutoUnlock inside the loop below after calling AudioObjectRemovePropertyListener that will guarantee that there are no outstanding callbacks. Since each unlock will allow one listener to complete and after each RemovePropertyListenerCall we know (I hope!) that callback can't be called again. http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.h (right): http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:30: friend class base::RefCountedThreadSafe<AudioDeviceListenerMac>; Remove.
http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:53: &OnDefaultDeviceChangedCallback, On 2012/10/29 21:11:32, DaleCurtis wrote: > On 2012/10/29 08:44:07, henrika wrote: > > Could you add some reference or comments here regarding what we can detect > other > > than a change in default device? Compare with the Windows version. Also, it is > > not clear to me where you ensure that detection is for output only. > > +1. I suspect you need to use ScopeOutput instead of ScopeGlobal. ScopeGlobal is correct here. Since we're observing the kAudioHardwarePropertyDefaultOutputDevice property we only get notified when the output device changes. I verified on my machine that no notification is sent when the default input device is changed. http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac_unittest.cc (right): http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac_unittest.cc:14: class AudioDeviceListenerMacTest : public testing::Test { On 2012/10/29 21:11:32, DaleCurtis wrote: > On 2012/10/29 08:44:07, henrika wrote: > > As mentioned before. Would it be beneficial to refactor to ensure that we have > > one common unit test for all platforms. Have not done any detailed thinking > but > > it would be nice. > > Since there's only one test, the class is unnecessary. You could roll all this > into one TEST() if you want. Done. http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:16: AudioObjectPropertySelector g_observed_properties[] = { On 2012/10/29 21:11:32, DaleCurtis wrote: > static const. Then the name should be styled kObservedProperties[] or > kPropertiesToObserve[] Changed to const. This doesn't need to be static since it's in an anonymous name space. http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:58: base::AutoLock auto_lock(lock_); On 2012/10/29 21:11:32, DaleCurtis wrote: > Does this cause races during destruction? I assume not, but watch TSAN. > > Also, is it enough to hold the lock indefinitely during destruction? What > happens in OnDefaultDeviceChanged() if the lock is destructed while another > thread is waiting? At best it seems racy, at worst it would be a use after > free. > > If you put an AutoUnlock inside the loop below after calling > AudioObjectRemovePropertyListener that will guarantee that there are no > outstanding callbacks. Since each unlock will allow one listener to complete > and after each RemovePropertyListenerCall we know (I hope!) that callback can't > be called again. Done. http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.h (right): http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:30: friend class base::RefCountedThreadSafe<AudioDeviceListenerMac>; On 2012/10/29 21:11:32, DaleCurtis wrote: > Remove. Done.
http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:53: &OnDefaultDeviceChangedCallback, On 2012/10/29 21:38:28, sail wrote: > On 2012/10/29 21:11:32, DaleCurtis wrote: > > On 2012/10/29 08:44:07, henrika wrote: > > > Could you add some reference or comments here regarding what we can detect > > other > > > than a change in default device? Compare with the Windows version. Also, it > is > > > not clear to me where you ensure that detection is for output only. > > > > +1. I suspect you need to use ScopeOutput instead of ScopeGlobal. > > ScopeGlobal is correct here. Since we're observing the > kAudioHardwarePropertyDefaultOutputDevice property we only get notified when the > output device changes. I verified on my machine that no notification is sent > when the default input device is changed. Is kAudioDevicePropertyNominalSampleRate only for the output device as well? http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:58: base::AutoLock auto_lock(lock_); On 2012/10/29 21:38:28, sail wrote: > On 2012/10/29 21:11:32, DaleCurtis wrote: > > Does this cause races during destruction? I assume not, but watch TSAN. > > > > Also, is it enough to hold the lock indefinitely during destruction? What > > happens in OnDefaultDeviceChanged() if the lock is destructed while another > > thread is waiting? At best it seems racy, at worst it would be a use after > > free. > > > > If you put an AutoUnlock inside the loop below after calling > > AudioObjectRemovePropertyListener that will guarantee that there are no > > outstanding callbacks. Since each unlock will allow one listener to complete > > and after each RemovePropertyListenerCall we know (I hope!) that callback > can't > > be called again. > > Done. You should double check my logic here to ensure this actually lets other threads through and doesn't just immediately reacquire the lock. I can't remember if contention is FIFO based or not with pthreads, Windows explicitly provides no guarantee of ordering. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:9: #include "base/bind.h" Not needed? http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:10: #include "base/location.h" Not needed? http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:17: kAudioHardwarePropertyDefaultOutputDevice, I think this should be a 4-space indent. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:52: OSSTATUS_DCHECK(result == noErr, result); If this actually fails in release mode and we try to remove the listener later, what happens? Should we have a |initialized_| flag or something similar ? http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.h (right): http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:21: // thus the callee must be thread safe. |listener| is a permanent callback |listener| -> |listener_cb| http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:22: // and must outlive AudioDeviceListenerWin. s/Win/Mac/ http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:27: void OnDefaultDeviceChanged(); private + FRIEND_TEST_ALL_PREFIXES() ? http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:32: base::Lock lock_; Needs DISALLOW_COPY_AND_ASSIGN() http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac_unittest.cc (right): http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac_unittest.cc:5: #include <string> Not needed?
http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/5005/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:58: base::AutoLock auto_lock(lock_); On 2012/10/29 21:57:14, DaleCurtis wrote: > You should double check my logic here to ensure this actually lets other threads > through and doesn't just immediately reacquire the lock. I can't remember if > contention is FIFO based or not with pthreads, Windows explicitly provides no > guarantee of ordering. I'm finding myself nervous, here. I'm wondering if a less brittle option is available. If this were a chrome-style callback, a weak pointer factory would be the way to go. But that's not going to work. One option might be to have a single global callback, with a vector of interested objects protected by a lock. The constructor would add itself to the list (under a lock) and register the callback if it's the first object. The destructor would remove itself from the list (under a lock) and terminate the callback if empty. Since construction and destruction are bound to a single thread, I don't think you need to debounce that, unless listeners can individually be constructed on different threads.
Addressed review comments please take another look. Note, I had to change AUAudioOutputStream to use the new AudioComponentInstance API. This fixed a bug where changing devices would cause a crash. http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/2002/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:53: &OnDefaultDeviceChangedCallback, On 2012/10/29 21:57:14, DaleCurtis wrote: > On 2012/10/29 21:38:28, sail wrote: > > On 2012/10/29 21:11:32, DaleCurtis wrote: > > > On 2012/10/29 08:44:07, henrika wrote: > > > > Could you add some reference or comments here regarding what we can detect > > > other > > > > than a change in default device? Compare with the Windows version. Also, > it > > is > > > > not clear to me where you ensure that detection is for output only. > > > > > > +1. I suspect you need to use ScopeOutput instead of ScopeGlobal. > > > > ScopeGlobal is correct here. Since we're observing the > > kAudioHardwarePropertyDefaultOutputDevice property we only get notified when > the > > output device changes. I verified on my machine that no notification is sent > > when the default input device is changed. > > Is kAudioDevicePropertyNominalSampleRate only for the output device as well? I don't know what kAudioDevicePropertyNominalSampleRate. As per my previous question I'm not sure what this is for and how to test it. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:9: #include "base/bind.h" On 2012/10/29 21:57:14, DaleCurtis wrote: > Not needed? Done. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:10: #include "base/location.h" On 2012/10/29 21:57:14, DaleCurtis wrote: > Not needed? Done. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:17: kAudioHardwarePropertyDefaultOutputDevice, On 2012/10/29 21:57:14, DaleCurtis wrote: > I think this should be a 4-space indent. I haven't seen that before. Everywhere else we do 2 spaces: http://code.google.com/searchframe#OAMlx_jo-ck/src/base/string_util_unittest.... http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:52: OSSTATUS_DCHECK(result == noErr, result); On 2012/10/29 21:57:14, DaleCurtis wrote: > If this actually fails in release mode and we try to remove the listener later, > what happens? Should we have a |initialized_| flag or something similar ? The remove function is a no-op if nothing was previously registered. I commented out the add listener call and verified that nothing fails. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.h (right): http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:21: // thus the callee must be thread safe. |listener| is a permanent callback On 2012/10/29 21:57:14, DaleCurtis wrote: > |listener| -> |listener_cb| Done. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:22: // and must outlive AudioDeviceListenerWin. On 2012/10/29 21:57:14, DaleCurtis wrote: > s/Win/Mac/ Done. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:27: void OnDefaultDeviceChanged(); On 2012/10/29 21:57:14, DaleCurtis wrote: > private + FRIEND_TEST_ALL_PREFIXES() ? Done. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:32: base::Lock lock_; On 2012/10/29 21:57:14, DaleCurtis wrote: > Needs DISALLOW_COPY_AND_ASSIGN() Done. http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac_unittest.cc (right): http://codereview.chromium.org/11344005/diff/7008/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac_unittest.cc:5: #include <string> On 2012/10/29 21:57:14, DaleCurtis wrote: > Not needed? Done.
http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:21: std::vector<media::AudioDeviceListenerMac*>* g_listeners = NULL; This is going to cause Valgrind/DrMemory to go nuts and is pretty complicated. If we're going to leak objects, we might as well make AudioDeviceListenerMac a singleton pattern or faux-singleton (like AudioMessageFilter) and initialize it only once. That said, I'd like to find a way that results in us not having to leak objects, but I'm running short on ideas. :/
http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:21: std::vector<media::AudioDeviceListenerMac*>* g_listeners = NULL; On 2012/10/29 23:14:16, DaleCurtis wrote: > This is going to cause Valgrind/DrMemory to go nuts and is pretty complicated. > If we're going to leak objects, we might as well make AudioDeviceListenerMac a > singleton pattern or faux-singleton (like AudioMessageFilter) and initialize it > only once. > > That said, I'd like to find a way that results in us not having to leak objects, > but I'm running short on ideas. :/ On that Mac we explicitly avoid using singletons because it slows down shutdown. Leaking is always preferred.
On 2012/10/29 23:22:18, sail wrote: > http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... > File media/audio/mac/audio_device_listener_mac.cc (right): > > http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... > media/audio/mac/audio_device_listener_mac.cc:21: > std::vector<media::AudioDeviceListenerMac*>* g_listeners = NULL; > On 2012/10/29 23:14:16, DaleCurtis wrote: > > This is going to cause Valgrind/DrMemory to go nuts and is pretty complicated. > > > If we're going to leak objects, we might as well make AudioDeviceListenerMac a > > singleton pattern or faux-singleton (like AudioMessageFilter) and initialize > it > > only once. > > > > That said, I'd like to find a way that results in us not having to leak > objects, > > but I'm running short on ideas. :/ > > On that Mac we explicitly avoid using singletons because it slows down shutdown. > Leaking is always preferred. Actually for tests we sometimes add a "CleanUpForTests" method that makes valgrind happy. I'll add one now.
http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.cc (right): http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:38: g_listeners->begin(); it != g_listeners->end(); ++it) { I think 'g' needs to align with 's'. http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:46: if (!g_listener_lock) { Does this have reasonable guarantees around thread-safety? I didn't see any thread checks in the direct caller, but maybe someone up-stack has it under control? http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:49: g_listeners = new std::vector<media::AudioDeviceListenerMac*>; I think you need a barrier here, before registering for the callback. Otherwise, the callback could run on some other CPU before g_listener_lock has been stored. I think it would suffice to acquire the lock. http://codereview.chromium.org/11344005/diff/8007/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.cc:75: std::find(g_listeners->begin(), g_listeners->end(), listener)); Would there be any value to un-registering the callback at this point? I have no idea, I'm just wondering if it would cause the OS frameworks to keep extra cruft around. I'd guess that if we never actually remove all of the listeners it wouldn't matter.
I agree with Dale that this is getting a bit too complicated. My feeling is that we are actually interested in getting the notifications up until the process dies, so lets just make this a singleton. We can create a way to "neuter" this object so that it stops doing anything with further notifications once it's been "neutered". If AudioDeviceListenerMac never gets destructed, then all our problems go away. http://codereview.chromium.org/11344005/diff/3006/media/audio/mac/audio_devic... File media/audio/mac/audio_device_listener_mac.h (right): http://codereview.chromium.org/11344005/diff/3006/media/audio/mac/audio_devic... media/audio/mac/audio_device_listener_mac.h:22: // and must outlive AudioDeviceListenerWin. Win?
and by "neutering" I mean clearing/resetting |listener_cb_| inside a lock
Moved to a singleton class.
LGTM, the singleton side-steps the ordering issues. Does the media/audio/mac/audio_low_latency_output_mac.cc change belong?
On 2012/10/29 23:58:36, shess wrote: > LGTM, the singleton side-steps the ordering issues. > > Does the media/audio/mac/audio_low_latency_output_mac.cc change belong? Taking over from sail, will clean up and reupload under my name.
On 2012/10/30 00:08:30, DaleCurtis wrote: > On 2012/10/29 23:58:36, shess wrote: > > LGTM, the singleton side-steps the ordering issues. > > > > Does the media/audio/mac/audio_low_latency_output_mac.cc change belong? > > Taking over from sail, will clean up and reupload under my name. New CL here: http://codereview.chromium.org/11338024/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
