Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11529012:
Use the device listener in Media on windows (Closed)
|
Created:
8 years ago by no longer working on chromium Modified:
8 years ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis patch will use the device listener in media to monitor audio capture device changed.
The reasons for doing this include:
# when using the device notification from content/browser, the notification comes before the device is fully initialized, so the change won't show up in the following device enumeration.
# I saw a serious crash by plugging in or unplugging an audio device, I believe it is because some racing between two notifications, if this is the case, this patch should fix it.
BUG=160872, 165147
TEST=manual test by plugging and unplugging device, look at the device selection menu in content setting media.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173350
Patch Set 1 #
Total comments: 4
Patch Set 2 : used media::CoreAudioUtil::IsSupported() #
Total comments: 14
Patch Set 3 : addressed Tommi's comments #Patch Set 4 : addressed Tommi's offline comment to use only the statechange callback. #
Total comments: 1
Messages
Total messages: 27 (0 generated)
Dale, could you please review this? BR, SX
Hmm, the device listener changes seem reasonable. I'm not familiar with the system monitor though. You'll need another reviewer for that part. Doesn't this only fix the mentioned issues for Vista+? What about XP users? https://codereview.chromium.org/11529012/diff/1/content/browser/system_messag... File content/browser/system_message_window_win.cc (right): https://codereview.chromium.org/11529012/diff/1/content/browser/system_messag... content/browser/system_message_window_win.cc:57: if (IsCoreAudioSupported() && This should use Henrik's CoreAudioUtil class.
Avi, could you please review the content/browser code? Right now we have two audio device monitors for windows vista+, this patch disable the monitor in content/browser. Thanks, BR, SX https://codereview.chromium.org/11529012/diff/1/content/browser/system_messag... File content/browser/system_message_window_win.cc (right): https://codereview.chromium.org/11529012/diff/1/content/browser/system_messag... content/browser/system_message_window_win.cc:57: if (IsCoreAudioSupported() && On 2012/12/11 19:27:59, DaleCurtis wrote: > This should use Henrik's CoreAudioUtil class. I tried include the media/audio/win/core_audio_util_win.h, but it introduced a compiling warning something about "no object", not sure if it is because the content/browser has a stricter compiling flags. I will double check this with Henrik and Tommi tomorrow.
I am not a windows guy. I am willing to stamp when you have an OK from an expert. Ping me when you are ready there.
Tommi, could you please review the content/browser windows code? Thanks, SX https://codereview.chromium.org/11529012/diff/1/content/browser/system_messag... File content/browser/system_message_window_win.cc (right): https://codereview.chromium.org/11529012/diff/1/content/browser/system_messag... content/browser/system_message_window_win.cc:57: if (IsCoreAudioSupported() && On 2012/12/11 19:27:59, DaleCurtis wrote: > This should use Henrik's CoreAudioUtil class. including the CoreAudioUtil.h gives me this compiling error: [1/15] CXX obj\content\browser\content.system_message_window_win.obj FAILED: ninja -t msvc -r . -o obj\content\browser\content.system_message_window_win.obj -e environment.x86 -- cl.exe /no logo /showIncludes /FC @obj\content\browser\content.system_message_window_win.obj.rsp /c ..\..\content\browser\system_m essage_window_win.cc /Foobj\content\browser\content.system_message_window_win.obj /Fdcontent.pdb c:\users\xians\dev\chrome1\src\content\browser\system_message_window_win.cc(31) : error C2039: 'GetVersion' : is not a m ember of 'base::win' c:\users\xians\dev\chrome1\src\content\browser\system_message_window_win.cc(31) : error C2039: 'VERSION_VISTA' : is not a member of 'base::win' c:\users\xians\dev\chrome1\src\content\browser\system_message_window_win.cc(31) : error C2065: 'VERSION_VISTA' : undecla red identifier ninja: build stopped: subcommand failed. Tommi, do you know why? Does it worth the effort to fix it?
https://codereview.chromium.org/11529012/diff/1/content/browser/system_messag... File content/browser/system_message_window_win.cc (right): https://codereview.chromium.org/11529012/diff/1/content/browser/system_messag... content/browser/system_message_window_win.cc:57: if (IsCoreAudioSupported() && On 2012/12/12 10:17:16, xians1 wrote: > On 2012/12/11 19:27:59, DaleCurtis wrote: > > This should use Henrik's CoreAudioUtil class. > > including the CoreAudioUtil.h gives me this compiling error: > [1/15] CXX obj\content\browser\content.system_message_window_win.obj > FAILED: ninja -t msvc -r . -o > obj\content\browser\content.system_message_window_win.obj -e environment.x86 -- > cl.exe /no > logo /showIncludes /FC > @obj\content\browser\content.system_message_window_win.obj.rsp /c > ..\..\content\browser\system_m > essage_window_win.cc > /Foobj\content\browser\content.system_message_window_win.obj /Fdcontent.pdb > c:\users\xians\dev\chrome1\src\content\browser\system_message_window_win.cc(31) > : error C2039: 'GetVersion' : is not a m > ember of 'base::win' > c:\users\xians\dev\chrome1\src\content\browser\system_message_window_win.cc(31) > : error C2039: 'VERSION_VISTA' : is not > a member of 'base::win' > c:\users\xians\dev\chrome1\src\content\browser\system_message_window_win.cc(31) > : error C2065: 'VERSION_VISTA' : undecla > red identifier > ninja: build stopped: subcommand failed. > > Tommi, do you know why? Does it worth the effort to fix it? This doesn't look like related to CoreAudioUtil.h but rather that you removed the #include for base/win/windows_version.h and did not remove your IsCoreAudioSupported method.
Tommi and Henrik helped me fix the compiling error, now it is using CoreAudioUtil::IsSupported(). Please take another look. Thanks, SX
https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... File content/browser/system_message_window_win.cc (right): https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... content/browser/system_message_window_win.cc:17: no need for these whitespace changes. https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... content/browser/system_message_window_win.cc:50: // From Vista, we use a device listener in AudioDeviceListenerWin. The phrasing can be improved here and I think it's confusing to talk about Vista and then check for CoreAudio. Maybe change the comment to something like: // If CoreAudio is supported, AudioDeviceListenerWin will // take care of monitoring audio devices. https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... content/browser/system_message_window_win.cc:51: if (media::CoreAudioUtil::IsSupported() && nit: Just call this method once outside the loop and use a temp here. https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... content/browser/system_message_window_win.cc:53: continue; use {} when an if clause spans more than 2 lines. https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (left): https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:109: // Only listen for output device changes right now... why remove this comment? https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (right): https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:106: if (!pending_capture_device_changed_) This doesn't seem right. With this change we ignore all non-default audio devices. Is that what you intend to do? https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:120: pending_capture_device_changed_ = true; Imho this variable should be called |default_capture_device_changed_| since it is only raised for the default capture device. Also, what does the 'pending' part of the name mean?
Tommi, please take a second look. https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... File content/browser/system_message_window_win.cc (right): https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... content/browser/system_message_window_win.cc:17: On 2012/12/13 13:16:58, tommi wrote: > no need for these whitespace changes. Done. https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... content/browser/system_message_window_win.cc:50: // From Vista, we use a device listener in AudioDeviceListenerWin. On 2012/12/13 13:16:58, tommi wrote: > The phrasing can be improved here and I think it's confusing to talk about Vista > and then check for CoreAudio. > Maybe change the comment to something like: > > // If CoreAudio is supported, AudioDeviceListenerWin will > // take care of monitoring audio devices. Done. https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... content/browser/system_message_window_win.cc:51: if (media::CoreAudioUtil::IsSupported() && On 2012/12/13 13:16:58, tommi wrote: > nit: Just call this method once outside the loop and use a temp here. Done. https://codereview.chromium.org/11529012/diff/7001/content/browser/system_mes... content/browser/system_message_window_win.cc:53: continue; On 2012/12/13 13:16:58, tommi wrote: > use {} when an if clause spans more than 2 lines. Done. https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (left): https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:109: // Only listen for output device changes right now... On 2012/12/13 13:16:58, tommi wrote: > why remove this comment? because we are also listening to capture device changed now. https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... File media/audio/win/audio_device_listener_win.cc (right): https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:106: if (!pending_capture_device_changed_) On 2012/12/13 13:16:58, tommi wrote: > This doesn't seem right. With this change we ignore all non-default audio > devices. Is that what you intend to do? This flag is to prevent sending ProcessDevicesChanged for each DeviceStateChanged event. I tested it on win7, every time a device is changed, it will trigger the OnDefaultDeviceChanged and multiple OnDeviceStateChanged callbacks. So we need this. https://codereview.chromium.org/11529012/diff/7001/media/audio/win/audio_devi... media/audio/win/audio_device_listener_win.cc:120: pending_capture_device_changed_ = true; On 2012/12/13 13:16:58, tommi wrote: > Imho this variable should be called |default_capture_device_changed_| since it > is only raised for the default capture device. > > Also, what does the 'pending' part of the name mean? I changed the name to capture_device_changed_, since it might not be a default device, but somehow it will trigger the OnDefaultDeviceChanged callback. I used the word "pending" to make it clear that we are pending the notification until getting a statechanged callback.
Hi Dale and Avi, I might have to sign off early tomorrow, can I have your owner stamps for media and content/browser before Tommi's final review? I would like to make it for M25. Thanks, SX
media/ lgtm.
On 2012/12/13 18:38:15, DaleCurtis wrote: > media/ lgtm. When you have their OK, you have my lgtm to land.
Tommi, please take another look.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/11529012/19002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/11529012/19002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/11529012/19002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/11529012/19002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/11529012/19002
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/11529012/19002
Message was sent while issue was closed.
Change committed as 173350
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/11529012/diff/19002/media/audio/win/au... File media/audio/win/audio_device_listener_win.cc (right): https://chromiumcodereview.appspot.com/11529012/diff/19002/media/audio/win/au... media/audio/win/audio_device_listener_win.cc:110: monitor->ProcessDevicesChanged(base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE); How do you know that we are dealing with a capture device here? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
