Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11233023:
Handle audio device changes on Windows. (Closed)
|
Created:
8 years, 2 months ago by DaleCurtis Modified:
8 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org, Chris Rogers Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandle audio device changes on Windows.
Uses the new AudioDeviceListener framework to notify of device
changes. Handles only default device changes at the moment,
e.g., not manually changing the sample rate, etc on a current
default device.
This all works well enough that I can connect / disconnect remote
desktop sessions with and without audio and everything continues
to play seamlessly and in sync!
BUG=153056
TEST=Unplug... Plug... Unplug! Plug!
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=164236
Patch Set 1 : Wow! It actually works! #
Total comments: 49
Patch Set 2 : Comments! #
Total comments: 30
Patch Set 3 : Comments! Unit tests! #
Total comments: 16
Patch Set 4 : Callbacks! #
Total comments: 8
Patch Set 5 : DestructOnAudioThread(). BindToLoop. #
Total comments: 2
Patch Set 6 : No ctor thread check. #
Total comments: 1
Messages
Total messages: 31 (0 generated)
Boom!
Nice Dale! Added some early comment on the nasty MMDevice-specific parts. https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (right): https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:11: #include "base/stringprintf.h" Not used. https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:15: #include "media/audio/win/avrt_wrapper_win.h" Don't need this one. https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:21: static ScopedComPtr<IMMDeviceEnumerator> CreateDeviceEnumerator() { TODO(henrika) https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:40: bool avrt_init = avrt::Initialize(); Don't need this for the notification parts. Only required in combination with MMCSS to boost up thread prios. https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:40: bool avrt_init = avrt::Initialize(); Don't need the avrt-part. Only required in combination with MMCSS to boost thread prios. https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:53: ScopedComPtr<IMMDevice> endpoint_device; The last part of this method is for logging only; correct? https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:71: DLOG(ERROR) << "Default Device: " << default_device_id_; Why ERROR? https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:91: // TODO(dalecurtis): Why do we have this code under NOTREACHED()? Henrik? The IMMNotification implementation feels like a hack if you ask me and I don't remember now why I added NOTREACHED. They can most likely be removed. Let me check some more. https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:127: // TODO(dalecurtis): This is slightly different for exclusive mode right? We Not related to exclusive mode. Notifications should similar for this method. Might be differences in OnProperty. Not sure. https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:139: // Only fire a state change event if the device has actually changed. Let me do some more tests on Monday and get back with feedback. Note that there are three types of default devices on Windows. Default, Default communication and Default multimedia. Not my idea... https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:150: listener_->OnDeviceChange(); Should we comment on the fact that is call will come from an internal MMDevice thread dedicated for notifications. https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.h (right): https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:20: class MEDIA_EXPORT AudioDeviceListenerWin : public IMMNotificationClient { Can you add a TODO on me here so I can refactor some parts based on the upcoming media::win::CoreAudioUtil methods.
Also, please note that you are now running two different notifiers in MMDevice. Are you sure that it is the new one that does the trick here? In any case; we should not have two.
Pretty sure it was this one since I had log statements in AudioOutputController during testing. Good point though, I'll clean up the client in output stream.
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:91: // TODO(dalecurtis): Why do we have this code under NOTREACHED()? Henrik? The interface requires that we add implementations for AddRef/Release and QueryInterface but the doc states that we should not use them(see http://msdn.microsoft.com/en-us/library/windows/desktop/dd371403(v=VS.85).aspx). I guess we can remove NOTREACHED. I just added it as an extra sanity.
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:139: // Only fire a state change event if the device has actually changed. I think we must take this off line. It is actually not trivial to make a perfect detection here and the sequence depends on how your system is configured. Case 1) Two devices A and B where A is default and B is default comm. Now, set B to Default instead. Case 2) Two devices A and B where A is Default and B is disabled. Now enable B => another sequence. In addition, all sequences will also call as eMultimedia is modified.
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:102: // TODO(dalecurtis): We need to handle changes for the current default device Hhmm, what if we have three devices, A, B and C and the user changes props for C but A is default. Then we should not do anything.... Hence, the notifier must know that the change is for the default device and ignore change if not.
cool stuff! https://codereview.chromium.org/11233023/diff/5001/media/audio/audio_manager_... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/11233023/diff/5001/media/audio/audio_manager_... media/audio/audio_manager_base.cc:217: output_params.format() != AudioParameters::AUDIO_FAKE) { sanity q: do we want to explicitly handle the AUDIO_FAKE case? For example, today it would get used w/ the audio mixer if it was enabled. https://codereview.chromium.org/11233023/diff/5001/media/audio/audio_manager_... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/11233023/diff/5001/media/audio/audio_manager_... media/audio/audio_manager_base.h:126: typedef std::map< std::pair<AudioParameters, AudioParameters>, nit: we usually don't have the space after the opening < (but as required by compilers have a space before the trailing >) https://codereview.chromium.org/11233023/diff/5001/media/audio/audio_paramete... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/11233023/diff/5001/media/audio/audio_paramete... media/audio/audio_parameters.h:83: inline bool operator<(const AudioParameters& a, can we de-inline? https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (right): https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:104: // a single change like sample rate. this seems like a worthy follow-up optimization... file bug? https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.h (right): https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:25: STDMETHOD_(ULONG, AddRef)(); emailed chromium-dev about this style https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:40: }; DISALLOW? https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_mana... File media/audio/win/audio_manager_win.h (right): https://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_mana... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { considering this is win-specific code talking to win-specific code, you can ditch this implementation, the scary Init() override, and pass a AMWin* pointer to ADLWin and have it call a non-virtual function
http://codereview.chromium.org/11233023/diff/5001/media/audio/audio_manager_b... File media/audio/audio_manager_base.cc (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/audio_manager_b... media/audio/audio_manager_base.cc:217: output_params.format() != AudioParameters::AUDIO_FAKE) { On 2012/10/22 23:51:05, scherkus wrote: > sanity q: do we want to explicitly handle the AUDIO_FAKE case? > > For example, today it would get used w/ the audio mixer if it was enabled. That'd be okay and probably even preferable since we'd then only spin up a single fake audio output stream instead of a bunch of them. For AudioOutputResampler though it doesn't make sense since it'd just be additional overhead. http://codereview.chromium.org/11233023/diff/5001/media/audio/audio_manager_b... File media/audio/audio_manager_base.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/audio_manager_b... media/audio/audio_manager_base.h:126: typedef std::map< std::pair<AudioParameters, AudioParameters>, On 2012/10/22 23:51:05, scherkus wrote: > nit: we usually don't have the space after the opening < (but as required by > compilers have a space before the trailing >) Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/audio_parameters.h File media/audio/audio_parameters.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/audio_parameter... media/audio/audio_parameters.h:83: inline bool operator<(const AudioParameters& a, On 2012/10/22 23:51:05, scherkus wrote: > can we de-inline? No that'll lead to multiple definitions and it needs to be declared in the header file as far as I can tell; this is what is done elsewhere in base/ classes for operator overloading. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:11: #include "base/stringprintf.h" On 2012/10/21 09:00:20, henrika wrote: > Not used. Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:15: #include "media/audio/win/avrt_wrapper_win.h" On 2012/10/21 09:00:20, henrika wrote: > Don't need this one. Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:21: static ScopedComPtr<IMMDeviceEnumerator> CreateDeviceEnumerator() { On 2012/10/21 09:00:20, henrika wrote: > TODO(henrika) Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:40: bool avrt_init = avrt::Initialize(); On 2012/10/21 09:00:20, henrika wrote: > Don't need the avrt-part. Only required in combination with MMCSS to boost > thread prios. Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:53: ScopedComPtr<IMMDevice> endpoint_device; On 2012/10/21 09:00:20, henrika wrote: > The last part of this method is for logging only; correct? No we need the default_device_id to filter OnDefaultDeviceChanged notifications. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:71: DLOG(ERROR) << "Default Device: " << default_device_id_; On 2012/10/21 09:00:20, henrika wrote: > Why ERROR? Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:91: // TODO(dalecurtis): Why do we have this code under NOTREACHED()? Henrik? On 2012/10/22 10:40:24, henrika wrote: > The interface requires that we add implementations for AddRef/Release and > QueryInterface but the doc states that we should not use them(see > http://msdn.microsoft.com/en-us/library/windows/desktop/dd371403%28v=VS.85%29...). > I guess we can remove NOTREACHED. I just added it as an extra sanity. Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:102: // TODO(dalecurtis): We need to handle changes for the current default device On 2012/10/22 13:44:31, henrika wrote: > Hhmm, what if we have three devices, A, B and C and the user changes props for C > but A is default. Then we should not do anything.... > Hence, the notifier must know that the change is for the default device and > ignore change if not. We can use the default_device_id_ saved in construction to filter this. For now lets just focus on the more common device change problem. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:104: // a single change like sample rate. On 2012/10/22 23:51:05, scherkus wrote: > this seems like a worthy follow-up optimization... file bug? Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:127: // TODO(dalecurtis): This is slightly different for exclusive mode right? We On 2012/10/21 09:00:20, henrika wrote: > Not related to exclusive mode. Notifications should similar for this method. > Might be differences in OnProperty. Not sure. Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:139: // Only fire a state change event if the device has actually changed. On 2012/10/22 13:39:01, henrika wrote: > I think we must take this off line. It is actually not trivial to make a perfect > detection here and the sequence depends on how your system is configured. Case > 1) Two devices A and B where A is default and B is default comm. Now, set B to > Default instead. > Case 2) Two devices A and B where A is Default and B is disabled. Now enable B > => another sequence. > In addition, all sequences will also call as eMultimedia is modified. This will still work in that case, it's just that the audio will continue to output to the old device, right? If so, I think that's fine for now. I'm really only worried about the when USB audio devices are added / removed or hardware mute is applied. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:150: listener_->OnDeviceChange(); On 2012/10/21 09:00:20, henrika wrote: > Should we comment on the fact that is call will come from an internal MMDevice > thread dedicated for notifications. Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.h:20: class MEDIA_EXPORT AudioDeviceListenerWin : public IMMNotificationClient { On 2012/10/21 09:00:20, henrika wrote: > Can you add a TODO on me here so I can refactor some parts based on the upcoming > media::win::CoreAudioUtil methods. Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.h:25: STDMETHOD_(ULONG, AddRef)(); On 2012/10/22 23:51:05, scherkus wrote: > emailed chromium-dev about this style __stdcall seems to be winning. changed. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.h:40: }; On 2012/10/22 23:51:05, scherkus wrote: > DISALLOW? Done. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { On 2012/10/22 23:51:05, scherkus wrote: > considering this is win-specific code talking to win-specific code, you can > ditch this implementation, the scary Init() override, and pass a AMWin* pointer > to ADLWin and have it call a non-virtual function We'll still need the Init() override since we can't call any of the audio device listener functions until the COM thread has been initialized via audio_thread_.
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { On 2012/10/23 01:49:42, DaleCurtis wrote: > On 2012/10/22 23:51:05, scherkus wrote: > > considering this is win-specific code talking to win-specific code, you can > > ditch this implementation, the scary Init() override, and pass a AMWin* > pointer > > to ADLWin and have it call a non-virtual function > > We'll still need the Init() override since we can't call any of the audio device > listener functions until the COM thread has been initialized via audio_thread_. Huh? ADLWin doesn't run on audio_thread_ and AFAIK COM initialization on one thread doesn't (shouldn't?) carry over to other threads. I suspect ADLWin is piggy backing on the COM initialization of whatever thread called AudioManager::Init() i.e., main browser thread as content::BrowserMainLoop::MainMessageLoopStart() calls AudioManager::Create(), which calls AudioManager::Init(). http://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_c... media/audio/audio_output_controller.cc:373: // implementations and this can be dropped at that time. ...not as long as we're shipping on XP perhaps a better way to think of this is that the AudioManager should know if it's running on an OS where low latency is available or not (i.e., XP, Linux) and could make the policy decision as to whether streams need to be recreated there http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:21: // and forwarding to AudioManagerBase so it can notify downstream clients. s/AudioManagerWin/? http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:31: ULONG __stdcall AddRef(); nit: add "IMMNotificationClient implementation." comment do these also need to be virtual and OVERRIDE?
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { On 2012/10/23 02:23:11, scherkus wrote: > On 2012/10/23 01:49:42, DaleCurtis wrote: > > On 2012/10/22 23:51:05, scherkus wrote: > > > considering this is win-specific code talking to win-specific code, you can > > > ditch this implementation, the scary Init() override, and pass a AMWin* > > pointer > > > to ADLWin and have it call a non-virtual function > > > > We'll still need the Init() override since we can't call any of the audio > device > > listener functions until the COM thread has been initialized via > audio_thread_. > > Huh? ADLWin doesn't run on audio_thread_ and AFAIK COM initialization on one > thread doesn't (shouldn't?) carry over to other threads. > > I suspect ADLWin is piggy backing on the COM initialization of whatever thread > called AudioManager::Init() > > i.e., main browser thread as content::BrowserMainLoop::MainMessageLoopStart() > calls AudioManager::Create(), which calls AudioManager::Init(). Doh, you're right. How about collapsing the existing Init() into the constructor and adding an AudioManagerBase::InitializeOnAudioThread() method which can be overridden/implemented by platform specific audio managers. The constructor will then post a task upon thread setup completion. http://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_c... media/audio/audio_output_controller.cc:373: // implementations and this can be dropped at that time. On 2012/10/23 02:23:11, scherkus wrote: > ...not as long as we're shipping on XP > > perhaps a better way to think of this is that the AudioManager should know if > it's running on an OS where low latency is available or not (i.e., XP, Linux) > and could make the policy decision as to whether streams need to be recreated > there I was considering WaveOut on XP ~= low latency. That's not a bad idea though, we can use the command line flags (disable audio renderer mixer / resampler) to determine if >Vista+Mac need stream recreation or not. We'll end up recreating some streams which may not need it, but it would be unexpected behavior to a user. OnDeviceChange() is kind of a strange name then as the caller is expected to take a specific action if it is received. http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:31: ULONG __stdcall AddRef(); On 2012/10/23 02:23:11, scherkus wrote: > nit: add "IMMNotificationClient implementation." comment > > do these also need to be virtual and OVERRIDE? I dunno, this compiles and runs fine though. Henrik?
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.h:25: STDMETHOD_(ULONG, AddRef)(); Thanks for the effort Andrew. http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { Sounds fine by me. It works today, hence COM init is OK but it would be easier to follow if we did the new COM parts on the AM thread which we know is COM MTA. http://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_c... media/audio/audio_output_controller.cc:373: // implementations and this can be dropped at that time. Can you make a few tests on a Win7 machine where you fake that you are on XP (IsWasapi... returns false) to verify that it works? http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:19: // TODO(henrika): Move to CoreAudioUtil class. Nit, is is actually a bunch of static media::win::foo() methods. http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:50: hr = device_enumerator_->GetDefaultAudioEndpoint( Should we comment on the fact that you are hard coding for default rendering here? I mean, you store default_device and it is not clear what flow direction it is. My point is that we will use the same base for capture as well. http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:55: DLOG(ERROR) << "GetDefaultAudioEndpoint() failed. No devices?"; Easier to debug if you add << std::hex << hr. http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:62: DLOG(ERROR) << "GetId() failed"; dito http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:67: DVLOG(1) << "Default Device: " << default_device_id_; render http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:94: // TODO(dalecurtis): We need to handle changes for the current default device I have written code that would work here but it is tricky and it is not worth adding it now. Ping me when you need help. http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:112: // TODO(dalecurtis): Are there any events we need to handle here? Not at this stage. These events are very early and we can't use them as indicator to restart. Useful for debugging. http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:118: // Only listen for output device changes right now... TODO? http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:130: if (new_device_id.compare(default_device_id_) == 0) I also prefer using STL here. Another way could be to work with WCHAR and use wcscmp(). I vote for some extra WideTo.. and STL. http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:135: // TODO(dalecurtis): This still seems to fire an extra event on my machine for Still true?
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.h:25: STDMETHOD_(ULONG, AddRef)(); But as Tommi states, I am also in favor of keeping the COM style. "+1 For using the macros. That's the standard and FWIW the #2 approach also means using Windows specific macros (DWORD, REFIID, et al). COM is riddled with these macros and types that are specific to COM. IID, HRESULT, S_OK, CLASSID, FAILED(), SUCCEEDED(), the list goes on and on. All of these are very familiar and immediately recognizable by anyone with COM experience. The same goes for STDMETMOD (header file declaration with __stdcall and the virtual keyword), STDMETHODIMPL (use in the source file, doesn't have the virtual keyword), macros. There isn't a lot of COM code in Chromium, so I don't see the harm in keeping this specific code in line with what developers that write and maintain it are used to. It is what it is :-)" http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:31: ULONG __stdcall AddRef(); I vote for COM style and use what's in there. Taken care of in the COM macros. No need to rewrite if you ask me.
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { On 2012/10/23 07:38:06, henrika wrote: > Sounds fine by me. It works today, hence COM init is OK but it would be easier > to follow if we did the new COM parts on the AM thread which we know is COM MTA. SG. http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:31: ULONG __stdcall AddRef(); On 2012/10/23 08:21:43, henrika wrote: > I vote for COM style and use what's in there. Taken care of in the COM macros. > No need to rewrite if you ask me. Yeah it looks like the tide has turned in favour of the COM methods. I don't really mind either way.
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { On 2012/10/23 07:38:06, henrika wrote: > Sounds fine by me. It works today, hence COM init is OK but it would be easier > to follow if we did the new COM parts on the AM thread which we know is COM MTA. Split off here: https://codereview.chromium.org/11226057/ http://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_c... File media/audio/audio_output_controller.cc (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_c... media/audio/audio_output_controller.cc:373: // implementations and this can be dropped at that time. On 2012/10/23 07:38:06, henrika wrote: > Can you make a few tests on a Win7 machine where you fake that you are on XP > (IsWasapi... returns false) to verify that it works? What are you looking to test? I was planning to have a full unit test for AudioDeviceListenerWin which manually fired the callbacks on Windows. http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:130: if (new_device_id.compare(default_device_id_) == 0) On 2012/10/23 07:38:06, henrika wrote: > I also prefer using STL here. Another way could be to work with WCHAR and use > wcscmp(). I vote for some extra WideTo.. and STL. I'm a complete windows string noob, so if you've got a better way please let me know :) http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:31: ULONG __stdcall AddRef(); On 2012/10/23 17:48:19, scherkus wrote: > On 2012/10/23 08:21:43, henrika wrote: > > I vote for COM style and use what's in there. Taken care of in the COM macros. > > No need to rewrite if you ask me. > > Yeah it looks like the tide has turned in favour of the COM methods. I don't > really mind either way. Will change back in the next revision.
http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/5001/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { On 2012/10/23 20:09:07, DaleCurtis wrote: > On 2012/10/23 07:38:06, henrika wrote: > > Sounds fine by me. It works today, hence COM init is OK but it would be easier > > to follow if we did the new COM parts on the AM thread which we know is COM > MTA. > > Split off here: https://codereview.chromium.org/11226057/ Haha, that is totally the wrong link, I meant: https://codereview.chromium.org/11175066/
OK I reviewed that other CL. Please re-P+M this CL when you've updated + rebased.
All good. Now with a unit test! https://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11233023/diff/14004/media/audio/audio_output_... media/audio/audio_output_controller.cc:373: // implementations and this can be dropped at that time. On 2012/10/23 20:09:08, DaleCurtis wrote: > On 2012/10/23 07:38:06, henrika wrote: > > Can you make a few tests on a Win7 machine where you fake that you are on XP > > (IsWasapi... returns false) to verify that it works? > > What are you looking to test? I was planning to have a full unit test for > AudioDeviceListenerWin which manually fired the callbacks on Windows. I decided to do this check in the platform specific audio manager, if the device changes on a platform which supports low latency it will fire the request. This means non-low-latency streams will be recreated, but I don't think it'll be unusual from the user's point of view since they just changed audio devices. https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... File media/audio/win/audio_device_listener_win.cc (right): https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... media/audio/win/audio_device_listener_win.cc:50: hr = device_enumerator_->GetDefaultAudioEndpoint( On 2012/10/23 07:38:06, henrika wrote: > Should we comment on the fact that you are hard coding for default rendering > here? I mean, you store default_device and it is not clear what flow direction > it is. My point is that we will use the same base for capture as well. I've renamed the variable to default_render_device_id_. I've added some clarifying comments to class description and a TODO for capture side. https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... media/audio/win/audio_device_listener_win.cc:55: DLOG(ERROR) << "GetDefaultAudioEndpoint() failed. No devices?"; On 2012/10/23 07:38:06, henrika wrote: > Easier to debug if you add << std::hex << hr. Done. https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... media/audio/win/audio_device_listener_win.cc:62: DLOG(ERROR) << "GetId() failed"; On 2012/10/23 07:38:06, henrika wrote: > dito Done. https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... media/audio/win/audio_device_listener_win.cc:67: DVLOG(1) << "Default Device: " << default_device_id_; On 2012/10/23 07:38:06, henrika wrote: > render Done. https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... media/audio/win/audio_device_listener_win.cc:112: // TODO(dalecurtis): Are there any events we need to handle here? On 2012/10/23 07:38:06, henrika wrote: > Not at this stage. These events are very early and we can't use them as > indicator to restart. Useful for debugging. Done. https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... media/audio/win/audio_device_listener_win.cc:118: // Only listen for output device changes right now... On 2012/10/23 07:38:06, henrika wrote: > TODO? Added to class docs. https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... media/audio/win/audio_device_listener_win.cc:135: // TODO(dalecurtis): This still seems to fire an extra event on my machine for On 2012/10/23 07:38:06, henrika wrote: > Still true? Yes. https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... File media/audio/win/audio_device_listener_win.h (right): https://codereview.chromium.org/11233023/diff/14004/media/audio/win/audio_dev... media/audio/win/audio_device_listener_win.h:21: // and forwarding to AudioManagerBase so it can notify downstream clients. On 2012/10/23 02:23:11, scherkus wrote: > s/AudioManagerWin/? Done.
https://codereview.chromium.org/11233023/diff/7029/media/audio/audio_paramete... File media/audio/audio_parameters.h (right): https://codereview.chromium.org/11233023/diff/7029/media/audio/audio_paramete... media/audio/audio_parameters.h:83: inline bool operator<(const AudioParameters& a, this fits on a single line https://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (right): https://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:35: if (base::win::GetVersion() < base::win::VERSION_VISTA) FYI this is equivalent to calling !IsWASAPISupported() https://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:85: if (iid == IID_IUnknown || iid == __uuidof(IMMNotificationClient)) { I ain't a COM pro... but does this say if it's IUnknown we cast to IMMNotificationClient? Does that work? https://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:86: *object = static_cast < IMMNotificationClient*>(this); remove extra spaces https://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_mana... File media/audio/win/audio_manager_win.cc (right): https://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_mana... media/audio/win/audio_manager_win.cc:358: if (!media::IsWASAPISupported()) This is impossible because your check for the equivalent statement when creating ADLWin during creation. I'd like to isolate WASAPI/Vista-checking code to the managers if at all possible. How about: * ADLWin documents + CHECK()s we're on the right OS * AMWin doesn't create an ADLWin if we're not on the right OS * This method DCHECKs https://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_mana... File media/audio/win/audio_manager_win.h (right): https://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_mana... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { I still don't see a reason to use multiple inheritance here. Why not pass a AudioManagerWin* pointer into ADLWin?
http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { On 2012/10/24 04:27:09, scherkus wrote: > I still don't see a reason to use multiple inheritance here. > > Why not pass a AudioManagerWin* pointer into ADLWin? Hmm, I kept it this way because the unit test was clear and simple. One alternative would be to create a real AudioManager() instance and register our mock OnDeviceChange listener through the public AudioManager methods. The test would then need to friend AudioManagerWin so it can fake callbacks. WDYT?
http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { On 2012/10/24 04:52:47, DaleCurtis wrote: > On 2012/10/24 04:27:09, scherkus wrote: > > I still don't see a reason to use multiple inheritance here. > > > > Why not pass a AudioManagerWin* pointer into ADLWin? > > Hmm, I kept it this way because the unit test was clear and simple. One > alternative would be to create a real AudioManager() instance and register our > mock OnDeviceChange listener through the public AudioManager methods. The test > would then need to friend AudioManagerWin so it can fake callbacks. WDYT? Those darn unit tests! OK what about using a callback instead of multiple inheritance? That's still a win IMO.
http://codereview.chromium.org/11233023/diff/7029/media/audio/audio_parameters.h File media/audio/audio_parameters.h (right): http://codereview.chromium.org/11233023/diff/7029/media/audio/audio_parameter... media/audio/audio_parameters.h:83: inline bool operator<(const AudioParameters& a, On 2012/10/24 04:27:09, scherkus wrote: > this fits on a single line Done. http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devic... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:35: if (base::win::GetVersion() < base::win::VERSION_VISTA) On 2012/10/24 04:27:09, scherkus wrote: > FYI this is equivalent to calling !IsWASAPISupported() Hah! Really?! I thought we were actually querying to see if WASAPI was supported as I thought it could be disabled under certain circumstances... http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:85: if (iid == IID_IUnknown || iid == __uuidof(IMMNotificationClient)) { On 2012/10/24 04:27:09, scherkus wrote: > I ain't a COM pro... but does this say if it's IUnknown we cast to > IMMNotificationClient? Does that work? This looks correct (minus a fix I just added) from the literature I've found: http://blogs.msdn.com/b/oldnewthing/archive/2004/03/26/96777.aspx http://msdn.microsoft.com/en-us/library/windows/desktop/ms686590(v=vs.85).aspx Essentially if we receive an IUnknown query we have to return our current pointer for identity checks or something. http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:86: *object = static_cast < IMMNotificationClient*>(this); On 2012/10/24 04:27:09, scherkus wrote: > remove extra spaces Done. http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_manag... media/audio/win/audio_manager_win.cc:358: if (!media::IsWASAPISupported()) On 2012/10/24 04:27:09, scherkus wrote: > This is impossible because your check for the equivalent statement when creating > ADLWin during creation. > > I'd like to isolate WASAPI/Vista-checking code to the managers if at all > possible. > > How about: > * ADLWin documents + CHECK()s we're on the right OS > * AMWin doesn't create an ADLWin if we're not on the right OS > * This method DCHECKs > Done. http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_manag... File media/audio/win/audio_manager_win.h (right): http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_manag... media/audio/win/audio_manager_win.h:24: NON_EXPORTED_BASE(public AudioManager::AudioDeviceListener) { On 2012/10/24 16:52:11, scherkus wrote: > On 2012/10/24 04:52:47, DaleCurtis wrote: > > On 2012/10/24 04:27:09, scherkus wrote: > > > I still don't see a reason to use multiple inheritance here. > > > > > > Why not pass a AudioManagerWin* pointer into ADLWin? > > > > Hmm, I kept it this way because the unit test was clear and simple. One > > alternative would be to create a real AudioManager() instance and register our > > mock OnDeviceChange listener through the public AudioManager methods. The > test > > would then need to friend AudioManagerWin so it can fake callbacks. WDYT? > > Those darn unit tests! > > OK what about using a callback instead of multiple inheritance? That's still a > win IMO. Done.
http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devic... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:35: if (base::win::GetVersion() < base::win::VERSION_VISTA) I have not been able to come up with a better method than this. It is simple and it works well. http://codereview.chromium.org/11233023/diff/7029/media/audio/win/audio_devic... media/audio/win/audio_device_listener_win.cc:85: if (iid == IID_IUnknown || iid == __uuidof(IMMNotificationClient)) { Working with COM usually result in that you must turn to MSDN examples for guidance. Here is an example which is inline with our current implementation: http://msdn.microsoft.com/en-us/library/windows/desktop/dd370810(v=vs.85).aspx
lgtm w/ nits nice stuff! http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:30: // thus the callee must be thread safe. mention this is a permanent callback http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:48: base::Closure listener_; nit: listener_cb_
LGTM w/nits. Assuming green lights of course.
http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:162: // AudioDeviceListenerWin; currently just disabled since extraction is tricky. tricky -> extremely advanced ;-) http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_mana... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_mana... media/audio/win/audio_manager_win.cc:13: #include "base/bind.h" Fix order.
http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.h (right): http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:30: // thus the callee must be thread safe. On 2012/10/24 21:00:59, scherkus wrote: > mention this is a permanent callback Done. http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.h:48: base::Closure listener_; On 2012/10/24 21:00:59, scherkus wrote: > nit: listener_cb_ Done. http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:162: // AudioDeviceListenerWin; currently just disabled since extraction is tricky. On 2012/10/25 18:12:56, henrika wrote: > tricky -> extremely advanced ;-) Done. http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_mana... File media/audio/win/audio_manager_win.cc (right): http://codereview.chromium.org/11233023/diff/37016/media/audio/win/audio_mana... media/audio/win/audio_manager_win.cc:13: #include "base/bind.h" On 2012/10/25 18:12:56, henrika wrote: > Fix order. Done. Removed a bunch of unused imports too.
lgtm w/ nit http://codereview.chromium.org/11233023/diff/42001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/42001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:34: DCHECK(thread_checker_.CalledOnValidThread()); remove -- this is always true
http://codereview.chromium.org/11233023/diff/42001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (right): http://codereview.chromium.org/11233023/diff/42001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:34: DCHECK(thread_checker_.CalledOnValidThread()); On 2012/10/25 20:39:32, scherkus wrote: > remove -- this is always true Whoops, I misread the docs, for some reason I thought the first call set the thread id, but that's just silly. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11233023/37019
https://codereview.chromium.org/11233023/diff/37019/media/audio/win/audio_dev... File media/audio/win/audio_device_listener_win_unittest.cc (right): https://codereview.chromium.org/11233023/diff/37019/media/audio/win/audio_dev... media/audio/win/audio_device_listener_win_unittest.cc:72: // The second device event should be ignored since the device id has not Have you really set the expectations to verify that this is the case here? (.Times(0)?) Also, do you not need VerifyAndClear again? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
