Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8677012:
Refactor the AudioInputDeviceManager by removing the device thread. (Closed)
|
Created:
9 years, 1 month ago by no longer working on chromium Modified:
9 years ago Reviewers:
wjia(left Chromium), tommi (sloooow) - chröme, henrika (OOO until Aug 14), mflodman_chromium_OOO CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, acolwell+watch_chromium.org, annacc+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr., vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionRefactor the AudioInputDeviceManager by removing the device thread.
With this patch, all the work will be done in the IO thread instead, this is needed by WASAPI.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111641
Patch Set 1 #
Total comments: 20
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 18
Patch Set 4 : fixed the comments #
Total comments: 6
Patch Set 5 : update #
Messages
Total messages: 20 (0 generated)
Hi, Please review. This patch refactors the AudioInputDeviceManager, and I also changed its unittests correspondingly. I may add some test cases to WebRTCAudioDeviceTest after sorting out some questions. Thanks, BR, -SX
overall looks very good. I've got just one question below. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:67: devices)); won't |devices| go out of scope and thus be garbage when the task runs? I guess I must be missing something... will there be a copy made? http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:121: int session_id, AudioInputDeviceManagerEventHandler* event_handler) { move int session_id to the previous line
http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:67: devices)); On 2011/11/23 20:16:31, tommi wrote: > won't |devices| go out of scope and thus be garbage when the task runs? I guess > I must be missing something... will there be a copy made? I did not dig into the details on how the bind works regarding to this question, but I found quite a lot of sample code doing the similar things. So please allow me to assume it is a copy made. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:121: int session_id, AudioInputDeviceManagerEventHandler* event_handler) { On 2011/11/23 20:16:31, tommi wrote: > move int session_id to the previous line "AudioInputDeviceManagerEventHandler* event_handler" is too long to be aligned to "int session_id" if I move it to the previous line.
Looks really good. Some comments. Also, please add more descriptions for all tests. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:67: devices)); Can probably be done without a copy. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:89: &AudioInputDeviceManager::OpenedOnIOThread, Odd indentation. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:115: &AudioInputDeviceManager::ClosedOnIOThread, Fix indentation. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:118: // Wait for the callback. As I've understand it all comments but the first in a scope should have an empty line above. Goes for the complete test. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:136: // Test the devices can be opened and closed. Tests that... http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:212: // This should still pass. Comment does not add much value to the test description. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:387: // Try start a invalid session device. an http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:422: // Try start a session twice. Try to start.. And please add some more about the test.
Please take second round. Magnus, it will be great if you can take a quick look at it to see if there is any substantial mistake. I hope to land it ASAP. BR; -SX http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:67: devices)); On 2011/11/24 15:22:26, henrika1 wrote: > Can probably be done without a copy. Use a pointer as Tommi suggests. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:89: &AudioInputDeviceManager::OpenedOnIOThread, On 2011/11/24 15:22:26, henrika1 wrote: > Odd indentation. Done. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager.cc:115: &AudioInputDeviceManager::ClosedOnIOThread, On 2011/11/24 15:22:26, henrika1 wrote: > Fix indentation. Done. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:118: // Wait for the callback. On 2011/11/24 15:22:26, henrika1 wrote: > As I've understand it all comments but the first in a scope should have an empty > line above. Goes for the complete test. Done. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:136: // Test the devices can be opened and closed. On 2011/11/24 15:22:26, henrika1 wrote: > Tests that... Done. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:212: // This should still pass. On 2011/11/24 15:22:26, henrika1 wrote: > Comment does not add much value to the test description. Done. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:387: // Try start a invalid session device. On 2011/11/24 15:22:26, henrika1 wrote: > an Done. http://codereview.chromium.org/8677012/diff/1/content/browser/renderer_host/m... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:422: // Try start a session twice. On 2011/11/24 15:22:26, henrika1 wrote: > Try to start.. > And please add some more about the test. Done.
Some minor comments. Almost there. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:48: // Get the device list from system. what system? And better to move these comment to header file. Think that is how it should be. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:98: DCHECK(devices_.find(session_id) != devices_.end()); Explain. Add comment please. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:102: // Checks if the device has been stopped, if not, sends stop signal. send http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:144: // Post a callback through the AudioInputRendererHost to notify the renderer "to notify renderer device has started", please rewrite since the sentence is not complete. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:159: // Ensure |devices| gets deleted on exit. that http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:253: // Tests the Start and Close function after opening the devices. functions http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:268: // Loops through the devices, and Open/start/stop/close each device. Fix language and use Open()/Start() etc. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:368: // Start/stop/close the default device twice. Start()/Stop() etc.
lgtm with Henrik's comments addressed. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:392: // Starts an invalid session device, it should fail. "it should fail" suggests that the test should fail, yet the test does not have the FAILS_ prefix. Suggest that you just remove the "it should fail" bit or change to "Try to start an invalid session device, which should not be possible"
Fixed the comments. I am going to commit it since Niklas reports a bug in content_unittests, and this CL will resolve the problem. Magnus, welcomed to review the code even after I land it, I will be glad to refactor it more if you find out any problem. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:48: // Get the device list from system. On 2011/11/25 09:48:25, henrika1 wrote: > what system? And better to move these comment to header file. Think that is how > it should be. Those APIs are defined in MediaStreamProvider, and there have been comments there. I remove this line, hope it is fine. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:98: DCHECK(devices_.find(session_id) != devices_.end()); On 2011/11/25 09:48:25, henrika1 wrote: > Explain. Add comment please. Done. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:102: // Checks if the device has been stopped, if not, sends stop signal. On 2011/11/25 09:48:25, henrika1 wrote: > send Done. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:144: // Post a callback through the AudioInputRendererHost to notify the renderer On 2011/11/25 09:48:25, henrika1 wrote: > "to notify renderer device has started", please rewrite since the sentence is > not complete. Done. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager.cc:159: // Ensure |devices| gets deleted on exit. On 2011/11/25 09:48:25, henrika1 wrote: > that Done. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:253: // Tests the Start and Close function after opening the devices. On 2011/11/25 09:48:25, henrika1 wrote: > functions Done. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:268: // Loops through the devices, and Open/start/stop/close each device. On 2011/11/25 09:48:25, henrika1 wrote: > Fix language and use Open()/Start() etc. Done. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:368: // Start/stop/close the default device twice. On 2011/11/25 09:48:25, henrika1 wrote: > Start()/Stop() etc. Done. http://codereview.chromium.org/8677012/diff/4005/content/browser/renderer_hos... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:392: // Starts an invalid session device, it should fail. On 2011/11/25 12:48:42, tommi wrote: > "it should fail" suggests that the test should fail, yet the test does not have > the FAILS_ prefix. Suggest that you just remove the "it should fail" bit or > change to "Try to start an invalid session device, which should not be possible" Done.
+ Wei Wei, I am refactoring this AudioInputDeviceManager to remove the device thread and do things on IO thread. This is needed by WASAPI since the enumeration APIs are only allowed to be called on IO thread. Tommi and Henrik have reviewed the CL, but I need yout lgtm to land it, please take a quick look at it. Thanks, BR, -SX
http://codereview.chromium.org/8677012/diff/14002/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): http://codereview.chromium.org/8677012/diff/14002/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager.cc:49: AudioManager::GetAudioManager()->GetAudioInputDeviceNames(&device_names); How much time does this operation typically take? Is it appropriate for IO thread if it's long? http://codereview.chromium.org/8677012/diff/14002/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager.cc:56: it->unique_id, false)); nit: indent. one more space. http://codereview.chromium.org/8677012/diff/14002/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager.cc:79: There was an error check (if device is valid) in previous function OpenOnDeviceThread. Is it needed here?
http://codereview.chromium.org/8677012/diff/14002/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): http://codereview.chromium.org/8677012/diff/14002/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager.cc:49: AudioManager::GetAudioManager()->GetAudioInputDeviceNames(&device_names); On 2011/11/25 16:37:20, wjia wrote: > How much time does this operation typically take? Is it appropriate for IO > thread if it's long? I think it should be fine. My tests shows that it usually takes 30ms, but for the first time running the enumeration, it may take around 100ms. http://codereview.chromium.org/8677012/diff/14002/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager.cc:56: it->unique_id, false)); On 2011/11/25 16:37:20, wjia wrote: > nit: indent. one more space. Done. http://codereview.chromium.org/8677012/diff/14002/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager.cc:79: On 2011/11/25 16:37:20, wjia wrote: > There was an error check (if device is valid) in previous function > OpenOnDeviceThread. Is it needed here? No, it is not needed any more. Since checking the device is valid or not takes resource from IO thread, so we decide to skip it. And if the device is invalid, it will fail when creating the audio stream.
lgtm. Please do keep an eye on the time spent on GetAudioInputDeviceNames().
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8677012/12008
Try job failure for 8677012-12008 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8677012/12008
Try job failure for 8677012-12008 (retry) on win_rel for steps "nacl_integration, interactive_ui_tests". It's a second try, previously, steps "unit_tests, browser_tests, ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8677012/12008
Try job failure for 8677012-12008 (retry) (retry) on win_rel for step "interactive_ui_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8677012/12008
Change committed as 111641 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
