Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763383002:
Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one. (Closed)
|
Created:
3 years, 9 months ago by o1ka Modified:
3 years, 8 months ago CC:
audio-mojo-cl_google.com, chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, Max Morin, miu+watch_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitching AudioInputDeviceManager from using AudioManager interface to AudioSystem one.
1) AudioSystem::GetAssociatedOutputDeviceID() is added + unit tests for it;
2) DeviceOpener helper for AudioInputDeviceManager is introduced, which operates on audio thread and navigates through a sequence of calls to AudioSystem, to receive the reply asynchronously and make the next request until the whole device opening sequence is completed.
3) I chose to make DeviceOpener to work on audio thread to reduce the number of thread hops. But it comes with a cost of accessing AudioSystem on audio thread. To make that work correctly during shutdown, weak pointer access to AudioSystem is added + deleter which destroys it on audio thread.
Alternatively, we can make all the requests to AudioSystem from IO thread - then we don't need to care about AudioSystem lifetime, but will be hopping between audio thread and IO several times.
4) AudioInputDeviceManager unit tests are updated to take threading into account.
5) Now we have multiple post-reply events in the device opening sequence instead of one synchronous call. "Media.AudioInputDeviceManager.OpenOnDeviceThreadTime" metric already exists to measure the time of device opening. I reused it, so we'll be able to measure the effect of the change.
6) In the future this sequence of posts and replies will be a series of Mojo IPC exchanges. So we may reconsider the interface later to minimize IPC hops if the metric is seriously affected after switching to Mojo.
BUG=672468
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
TBR=alokp@chromium.org
Review-Url: https://codereview.chromium.org/2763383002
Cr-Commit-Position: refs/heads/master@{#463640}
Committed: https://chromium.googlesource.com/chromium/src/+/3dfb4cfaa34efcf58fbf79206197e94402266f18
Patch Set 1 #
Total comments: 4
Patch Set 2 : (fails on Mac in content_unittests) #
Total comments: 4
Patch Set 3 : WeakPtr removed; combined GetInputDeviceInfo operation introduced #
Total comments: 20
Patch Set 4 : fix for AudioManager destruction in tests (will be overriden by alokp@ CL) #
Total comments: 1
Patch Set 5 : review comments addressed #
Total comments: 15
Patch Set 6 : Comments updated #Patch Set 7 : rebase #Dependent Patchsets: Messages
Total messages: 55 (23 generated)
Description was changed from ========== Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one. BUG=672468 ========== to ========== Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one. 1) AudioSystem::GetAssociatedOutputDeviceID() is added + unit tests for it; 2) DeviceOpener helper for AudioInputDeviceManager is introduced, which operates on audio thread and navigates through a sequence of calls to AudioSystem, to receive the reply asynchronously and make the next request until the whole device opening sequence is completed. 3) I chose to make DeviceOpener to work on audio thread to reduce the number of thread hops. But it comes with a cost of accessing AudioSystem on audio thread. To make that work correctly during shutdown, weak pointer access to AudioSystem is added + deleter which destroys it on audio thread. Alternatively, we can make all the requests to AudioSystem from IO thread - then we don't need to care about AudioSystem lifetime, but will be hopping between audio thread and IO several times. 4) AudioInputDeviceManager unit tests are updated to take threading into account. 5) Now we have multiple post-reply events in the device opening sequence instead of one synchronous call. "Media.AudioInputDeviceManager.OpenOnDeviceThreadTime" metric already exists to measure the time of device opening. I reused it, so we'll be able to measure the effect of the change. 6) In the future this sequence of posts and replies will be a series of Mojo IPC exchanges. So we may reconsider the interface later to minimize IPC hops if the metric is seriously affected after switching to Mojo. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
olka@chromium.org changed reviewers: + dalecurtis@chromium.org, guidou@chromium.org
guidou@, dalecurtis@ - PTAL https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager.cc:209: UMA_HISTOGRAM_TIMES("Media.AudioInputDeviceManager.OpenOnDeviceThreadTime", TODO: Make sure UMA meaning is unchanged. Add UMA reviewer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
I think it makes sense to supply this information when enumerating, rather than having the browser do lots of IPC calls. This is just my intuitive feeling though, haven't looked closely. :) https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager.cc:43: // Wwill reply with |on_opened_cb| on the thread Open() is called on. Typo Wwill
On 2017/03/22 17:46:25, Max Morin wrote: > I think it makes sense to supply this information when enumerating, rather than > having the browser do lots of IPC calls. This is just my intuitive feeling > though, haven't looked closely. :) > Yes, this is another option with pros and cons. I'm open for discussion :) > https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/media/audio_input_device_manager.cc (right): > > https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/media/audio_input_device_manager.cc:43: // Wwill > reply with |on_opened_cb| on the thread Open() is called on. > Typo Wwill
I see some tests are failing with shutdown issues, but still let's discuss the approach. Do we want to pack the logic into one call, or to have (relatively) primitive methods to build with? And if latter, which approach to threading is preferable?
Please redesign to avoid externally vended WeakPtrs. They should really only be used for internal task cancellation, not for vending weak references to other objects. It's preferred that you instead document and ensure the lifetime of the class relative to all owners. https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_device_manager.h (right): https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_device_manager.h:43: explicit AudioInputDeviceManager(media::AudioSystem* audio_system); Document lifetime. https://codereview.chromium.org/2763383002/diff/1/media/audio/audio_system.h File media/audio/audio_system.h (right): https://codereview.chromium.org/2763383002/diff/1/media/audio/audio_system.h#... media/audio/audio_system.h:79: // To be used on audio thread only. Sorry, this is bad practice, can you find another way to achieve what you're trying to do here? We try to avoid vending WeakPtrs to outside classes. Instead lifetime needs to be managed such that this class outlives everything that should all it.
On 2017/03/22 19:08:34, DaleCurtis wrote: > Please redesign to avoid externally vended WeakPtrs. They should really only be > used for internal task cancellation, not for vending weak references to other > objects. It's preferred that you instead document and ensure the lifetime of the > class relative to all owners. > > https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/media/audio_input_device_manager.h (right): > > https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/media/audio_input_device_manager.h:43: explicit > AudioInputDeviceManager(media::AudioSystem* audio_system); > Document lifetime. > > https://codereview.chromium.org/2763383002/diff/1/media/audio/audio_system.h > File media/audio/audio_system.h (right): > > https://codereview.chromium.org/2763383002/diff/1/media/audio/audio_system.h#... > media/audio/audio_system.h:79: // To be used on audio thread only. > Sorry, this is bad practice, can you find another way to achieve what you're > trying to do here? We try to avoid vending WeakPtrs to outside classes. Instead > lifetime needs to be managed such that this class outlives everything that > should all it. I agree that this WeakPtr thing is pretty ugly, and I'm thinking how to change the design. However, I've got a question: It's considered safe to post AudioManager* as Unretained(), because it's destroyed on audio thread during browser main loop destruction. But what if a task posted to audio thread before AM destructor posts AM operation to audio thread? Then we have AM operation scheduled after AM destruction. Basically, AM* should not be posted as Unretained from audio thread. Is this restriction expressed in the code somehow (I haven't found anything)? It looks like AM should be destroyed after audio message loop destruction instead of being posted for destruction there?
On 2017/03/23 at 14:58:55, olka wrote: > On 2017/03/22 19:08:34, DaleCurtis wrote: > > Please redesign to avoid externally vended WeakPtrs. They should really only be > > used for internal task cancellation, not for vending weak references to other > > objects. It's preferred that you instead document and ensure the lifetime of the > > class relative to all owners. > > > > https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... > > File content/browser/renderer_host/media/audio_input_device_manager.h (right): > > > > https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... > > content/browser/renderer_host/media/audio_input_device_manager.h:43: explicit > > AudioInputDeviceManager(media::AudioSystem* audio_system); > > Document lifetime. > > > > https://codereview.chromium.org/2763383002/diff/1/media/audio/audio_system.h > > File media/audio/audio_system.h (right): > > > > https://codereview.chromium.org/2763383002/diff/1/media/audio/audio_system.h#... > > media/audio/audio_system.h:79: // To be used on audio thread only. > > Sorry, this is bad practice, can you find another way to achieve what you're > > trying to do here? We try to avoid vending WeakPtrs to outside classes. Instead > > lifetime needs to be managed such that this class outlives everything that > > should all it. > > I agree that this WeakPtr thing is pretty ugly, and I'm thinking how to change the design. > > However, I've got a question: > > It's considered safe to post AudioManager* as Unretained(), because it's destroyed on audio thread during browser main loop destruction. > But what if a task posted to audio thread before AM destructor posts AM operation to audio thread? Then we have AM operation scheduled after AM destruction. > Basically, AM* should not be posted as Unretained from audio thread. > Is this restriction expressed in the code somehow (I haven't found anything)? > > It looks like AM should be destroyed after audio message loop destruction instead of being posted for destruction there? Not a problem since AM::Shutdown() joins and stops the audio thread on non-OSX platforms. And on OSX the main thread is the audio thread, so no further tasks are running at shutdonw.
On 2017/03/23 17:35:20, DaleCurtis wrote: > On 2017/03/23 at 14:58:55, olka wrote: > > On 2017/03/22 19:08:34, DaleCurtis wrote: > > > Please redesign to avoid externally vended WeakPtrs. They should really only > be > > > used for internal task cancellation, not for vending weak references to > other > > > objects. It's preferred that you instead document and ensure the lifetime of > the > > > class relative to all owners. > > > > > > > https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... > > > File content/browser/renderer_host/media/audio_input_device_manager.h > (right): > > > > > > > https://codereview.chromium.org/2763383002/diff/1/content/browser/renderer_ho... > > > content/browser/renderer_host/media/audio_input_device_manager.h:43: > explicit > > > AudioInputDeviceManager(media::AudioSystem* audio_system); > > > Document lifetime. > > > > > > https://codereview.chromium.org/2763383002/diff/1/media/audio/audio_system.h > > > File media/audio/audio_system.h (right): > > > > > > > https://codereview.chromium.org/2763383002/diff/1/media/audio/audio_system.h#... > > > media/audio/audio_system.h:79: // To be used on audio thread only. > > > Sorry, this is bad practice, can you find another way to achieve what you're > > > trying to do here? We try to avoid vending WeakPtrs to outside classes. > Instead > > > lifetime needs to be managed such that this class outlives everything that > > > should all it. > > > > I agree that this WeakPtr thing is pretty ugly, and I'm thinking how to change > the design. > > > > However, I've got a question: > > > > It's considered safe to post AudioManager* as Unretained(), because it's > destroyed on audio thread during browser main loop destruction. > > But what if a task posted to audio thread before AM destructor posts AM > operation to audio thread? Then we have AM operation scheduled after AM > destruction. > > Basically, AM* should not be posted as Unretained from audio thread. > > Is this restriction expressed in the code somehow (I haven't found anything)? > > > > It looks like AM should be destroyed after audio message loop destruction > instead of being posted for destruction there? > > Not a problem since AM::Shutdown() joins and stops the audio thread on non-OSX > platforms. And on OSX the main thread is the audio thread, so no further tasks > are running at shutdonw. Could you elaborate/point me to the code? I don't see anything like that in AudioManager/AudioManagerBase/AudioManagerThread/BrowserMainLoop. What I'm missing? Thanks!
dalecurtis@chromium.org changed reviewers: + alokp@chromium.org
You're missing that I forgot alokp@ deleted all that code :) Sorry for the confusion. I agree that what you're saying might be an issue, +alokp to remember why it's not. The original change is here: https://bugs.chromium.org/p/chromium/issues/detail?id=594234
On 2017/03/23 19:00:38, DaleCurtis wrote: > You're missing that I forgot alokp@ deleted all that code :) Sorry for the > confusion. I agree that what you're saying might be an issue, +alokp to remember > why it's not. The original change is here: > https://bugs.chromium.org/p/chromium/issues/detail?id=594234 Now that I think about it, the case olka@ described above is technically possible. We do set AudioManager::g_last_created to NULL before posting DeleteSoon, so that AudioManager::Get() returns NULL. But any client could use a cached AM pointer to post a task. So I guess we do need to use WeakPtr to post AM tasks.
On 2017/03/23 at 21:04:40, alokp wrote: > On 2017/03/23 19:00:38, DaleCurtis wrote: > > You're missing that I forgot alokp@ deleted all that code :) Sorry for the > > confusion. I agree that what you're saying might be an issue, +alokp to remember > > why it's not. The original change is here: > > https://bugs.chromium.org/p/chromium/issues/detail?id=594234 > > Now that I think about it, the case olka@ described above is technically possible. We do set AudioManager::g_last_created to NULL before posting DeleteSoon, so that AudioManager::Get() returns NULL. But any client could use a cached AM pointer to post a task. So I guess we do need to use WeakPtr to post AM tasks. We don't want to start vending external WeakPtrs for AudioManager, so I think we'll instead need to stop the thread like we used to. Probably this means we now need a public AM::Shutdown() method that BML can post to the audio task runner and then call Thread::Stop(), after that completes AM can be destructed on the BML thread; this would be a noop on OSX.
On 2017/03/23 23:03:41, DaleCurtis wrote: > On 2017/03/23 at 21:04:40, alokp wrote: > > On 2017/03/23 19:00:38, DaleCurtis wrote: > > > You're missing that I forgot alokp@ deleted all that code :) Sorry for the > > > confusion. I agree that what you're saying might be an issue, +alokp to > remember > > > why it's not. The original change is here: > > > https://bugs.chromium.org/p/chromium/issues/detail?id=594234 > > > > Now that I think about it, the case olka@ described above is technically > possible. We do set AudioManager::g_last_created to NULL before posting > DeleteSoon, so that AudioManager::Get() returns NULL. But any client could use a > cached AM pointer to post a task. So I guess we do need to use WeakPtr to post > AM tasks. > > We don't want to start vending external WeakPtrs for AudioManager, so I think > we'll instead need to stop the thread like we used to. Probably this means we > now need a public AM::Shutdown() method that BML can post to the audio task > runner and then call Thread::Stop(), after that completes AM can be destructed > on the BML thread; this would be a noop on OSX. Makes sense, although I need to think about how to handle Audio thread managed by content embedders. I will try to send a patch next week.
On 2017/03/24 04:40:54, alokp wrote: > On 2017/03/23 23:03:41, DaleCurtis wrote: > > On 2017/03/23 at 21:04:40, alokp wrote: > > > On 2017/03/23 19:00:38, DaleCurtis wrote: > > > > You're missing that I forgot alokp@ deleted all that code :) Sorry for the > > > > confusion. I agree that what you're saying might be an issue, +alokp to > > remember > > > > why it's not. The original change is here: > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=594234 > > > > > > Now that I think about it, the case olka@ described above is technically > > possible. We do set AudioManager::g_last_created to NULL before posting > > DeleteSoon, so that AudioManager::Get() returns NULL. But any client could use > a > > cached AM pointer to post a task. So I guess we do need to use WeakPtr to post > > AM tasks. > > > > We don't want to start vending external WeakPtrs for AudioManager, so I think > > we'll instead need to stop the thread like we used to. Probably this means we > > now need a public AM::Shutdown() method that BML can post to the audio task > > runner and then call Thread::Stop(), after that completes AM can be destructed > > on the BML thread; this would be a noop on OSX. > > Makes sense, although I need to think about how to handle Audio thread managed > by content embedders. I will try to send a patch next week. Thank you alokp@!
On 2017/03/24 13:59:22, o1ka wrote: > On 2017/03/24 04:40:54, alokp wrote: > > On 2017/03/23 23:03:41, DaleCurtis wrote: > > > On 2017/03/23 at 21:04:40, alokp wrote: > > > > On 2017/03/23 19:00:38, DaleCurtis wrote: > > > > > You're missing that I forgot alokp@ deleted all that code :) Sorry for > the > > > > > confusion. I agree that what you're saying might be an issue, +alokp to > > > remember > > > > > why it's not. The original change is here: > > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=594234 > > > > > > > > Now that I think about it, the case olka@ described above is technically > > > possible. We do set AudioManager::g_last_created to NULL before posting > > > DeleteSoon, so that AudioManager::Get() returns NULL. But any client could > use > > a > > > cached AM pointer to post a task. So I guess we do need to use WeakPtr to > post > > > AM tasks. > > > > > > We don't want to start vending external WeakPtrs for AudioManager, so I > think > > > we'll instead need to stop the thread like we used to. Probably this means > we > > > now need a public AM::Shutdown() method that BML can post to the audio task > > > runner and then call Thread::Stop(), after that completes AM can be > destructed > > > on the BML thread; this would be a noop on OSX. > > > > Makes sense, although I need to think about how to handle Audio thread managed > > by content embedders. I will try to send a patch next week. > > Thank you alokp@! I filed https://bugs.chromium.org/p/chromium/issues/detail?id=705455
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, see my comments https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:96: device.id, base::Bind(&AudioInputDeviceManager::OpenedOnIOThread, this, Since AudioSystem is accessed by AudioInputDeviceManager IO thread only, which is stopped before the mail loop destruction, we may want to bind Unretained(this) here. guidou@ WDYT? https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:167: UMA_HISTOGRAM_TIMES("Media.AudioInputDeviceManager.OpenOnDeviceThreadTime", Now we record it on IO thread, so IO->Device thread->IO hopping is included into the time, but it should be insignificant. We want to keep the same metric to be able to compare with historical data. https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_system.h File media/audio/audio_system.h (right): https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system.h:38: // AudioSystem is destroyed on. See also this comment https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?type.... Now AudioSystem is deleted on UI thread before Audiomanager/AudioThread, which means there potentially can be AudioSystem operation scheduled on the audio thread after AudioSystem is deleted. Audio thread needs to be stopped before AudioSystem is deleted. One option is to make AudioSystem to own AudioManager - which I don't really like, because we want to expose AM stream creation functionality through a separate interface. Another option is to have an "audio owner" which would own AudioThread, AudioManager and AudioSystem and would do a correct shutdown (something like shuts down AM, deleted AS, and then deletes AM). It depends on how the new shut down logic for AudioManager looks like. alokp@ have you figured it out? In any case, there are no AudioSystem operations posted to the audio thread as of now, so there is no use after free (which does not make it a good design though). https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system.h:74: OnDeviceIdCallback on_device_id_cb) = 0; This method will be used by WebRTC private API as well. https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system.h:81: OnInputDeviceInfoCallback on_input_device_info_cb) = 0; I do not really like it because it's basically a composite method consisting of GetInputStreamParameters(), GetAssociatedOutputDeviceID() and GetOutputStreamParameters(). On the other hand, with Mojo we'll end up with 3 IPC calls to get this info, which is not nice as well.
still need to fix shutdown in content unittests on Mac - PTAL anyways
https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:96: device.id, base::Bind(&AudioInputDeviceManager::OpenedOnIOThread, this, On 2017/03/28 at 16:40:16, o1ka wrote: > Since AudioSystem is accessed by AudioInputDeviceManager IO thread only, which > is stopped before the mail loop destruction, we may want to bind > Unretained(this) here. > > guidou@ WDYT? Whatever you decide, note the same situation exists above. https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:61: class MAYBE_AudioInputDeviceManagerTest : public testing::Test { Hmm, does MAYBE actually work as a classname? I thought DISABLED_/MAYBE_ only work as the test name not the test class name. https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_system.h File media/audio/audio_system.h (right): https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system.h:38: // AudioSystem is destroyed on. On 2017/03/28 at 16:40:16, o1ka wrote: > See also this comment https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?type.... > > Now AudioSystem is deleted on UI thread before Audiomanager/AudioThread, which means there potentially can be AudioSystem operation scheduled on the audio thread after AudioSystem is deleted. > > Audio thread needs to be stopped before AudioSystem is deleted. > One option is to make AudioSystem to own AudioManager - which I don't really like, because we want to expose AM stream creation functionality through a separate interface. > > Another option is to have an "audio owner" which would own AudioThread, AudioManager and AudioSystem and would do a correct shutdown (something like shuts down AM, deleted AS, and then deletes AM). > > It depends on how the new shut down logic for AudioManager looks like. > alokp@ have you figured it out? > > In any case, there are no AudioSystem operations posted to the audio thread as of now, so there is no use after free (which does not make it a good design though). This comment will need resolution and removal before submission, such a restriction is too onerous for callers to actually verify exhaustively. We don't want internal details of the AM implementation to leak here. https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system.h:81: OnInputDeviceInfoCallback on_input_device_info_cb) = 0; On 2017/03/28 at 16:40:16, o1ka wrote: > I do not really like it because it's basically a composite method consisting of > GetInputStreamParameters(), GetAssociatedOutputDeviceID() and > GetOutputStreamParameters(). > On the other hand, with Mojo we'll end up with 3 IPC calls to get this info, which is not nice as well. I guess it depends on how callers end up using this normally. Do they end up issuing all 3 callbacks to get the information they need in regular course? Are any clients only using one of these? https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system_impl.cc:180: GetTaskRunner()->PostTask( Hmm, seems like either the PostTask or BelongsToCurrentThread() check are unnecessary? Who do you expect to call this and from what thread? Generally you'd either not PostTask and use BindToCurrentLoop(), or always PostTask and always BindToCurrentLoop. https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... File media/audio/audio_system_impl.h (right): https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system_impl.h:46: AudioManager* GetAudioManager() const override; Hmm, why does this method exist? We probably shouldn't have multiple avenues for retrieving the AudioManager
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please see my questions and answers. https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:96: device.id, base::Bind(&AudioInputDeviceManager::OpenedOnIOThread, this, On 2017/03/28 18:22:11, DaleCurtis wrote: > On 2017/03/28 at 16:40:16, o1ka wrote: > > Since AudioSystem is accessed by AudioInputDeviceManager IO thread only, which > > is stopped before the mail loop destruction, we may want to bind > > Unretained(this) here. > > > > guidou@ WDYT? > > Whatever you decide, note the same situation exists above. Right - the question is for both. https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:61: class MAYBE_AudioInputDeviceManagerTest : public testing::Test { On 2017/03/28 18:22:11, DaleCurtis wrote: > Hmm, does MAYBE actually work as a classname? I thought DISABLED_/MAYBE_ only > work as the test name not the test class name. Looks like it does: https://github.com/google/googletest/blob/master/googletest/docs/V1_7_Advance...: "If you need to disable all tests in a test case, you can either add DISABLED_ to the front of the name of each test, or alternatively add it to the front of the test case name" https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_system.h File media/audio/audio_system.h (right): https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system.h:38: // AudioSystem is destroyed on. On 2017/03/28 18:22:11, DaleCurtis wrote: > On 2017/03/28 at 16:40:16, o1ka wrote: > > See also this comment > https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?type.... > > > > Now AudioSystem is deleted on UI thread before Audiomanager/AudioThread, which > means there potentially can be AudioSystem operation scheduled on the audio > thread after AudioSystem is deleted. > > > > Audio thread needs to be stopped before AudioSystem is deleted. > > One option is to make AudioSystem to own AudioManager - which I don't really > like, because we want to expose AM stream creation functionality through a > separate interface. > > > > Another option is to have an "audio owner" which would own AudioThread, > AudioManager and AudioSystem and would do a correct shutdown (something like > shuts down AM, deleted AS, and then deletes AM). > > > > It depends on how the new shut down logic for AudioManager looks like. > > alokp@ have you figured it out? > > > > In any case, there are no AudioSystem operations posted to the audio thread as > of now, so there is no use after free (which does not make it a good design > though). > > This comment will need resolution and removal before submission, such a > restriction is too onerous for callers to actually verify exhaustively. We don't > want internal details of the AM implementation to leak here. Agree. This is a problem shared between AM and AS. https://codereview.chromium.org/2784433002/ needs to address it. Should we wait for it to land? https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system.h:81: OnInputDeviceInfoCallback on_input_device_info_cb) = 0; On 2017/03/28 18:22:11, DaleCurtis wrote: > On 2017/03/28 at 16:40:16, o1ka wrote: > > I do not really like it because it's basically a composite method consisting > of > > GetInputStreamParameters(), GetAssociatedOutputDeviceID() and > > GetOutputStreamParameters(). > > On the other hand, with Mojo we'll end up with 3 IPC calls to get this info, > which is not nice as well. > > I guess it depends on how callers end up using this normally. Do they end up > issuing all 3 callbacks to get the information they need in regular course? Are > any clients only using one of these? Each of the operations has multiple users. AudioInputDeviceManager is the only one who runs a sequence of calls. It always gets associated output device id; input parameters - if there is no "use fake device" flag, and output parameters if there is an associated output device and no "use fake" flag. All this is done for each device in the enumeration. In the future there may be other situations when we want to execute a sequence of operations in the audio process, but for now it's only this case. So to me having this one combined call is an acceptable compromise. (Actually, I would prefer to refactor the whole audio device enumeration at some point) https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system_impl.cc:180: GetTaskRunner()->PostTask( On 2017/03/28 18:22:11, DaleCurtis wrote: > Hmm, seems like either the PostTask or BelongsToCurrentThread() check are > unnecessary? Who do you expect to call this and from what thread? Generally > you'd either not PostTask and use BindToCurrentLoop(), or always PostTask and > always BindToCurrentLoop. We may be either already on the audio thread (for example, on Mac), or on another thread (for example, IO). If we are of the audio thread, we still cannot reply synchronously, because the client expect to receive the reply in asynchronous manner. But we don't need to bind to the current loop in this case. I added a comment. https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... File media/audio/audio_system_impl.h (right): https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system_impl.h:46: AudioManager* GetAudioManager() const override; On 2017/03/28 18:22:11, DaleCurtis wrote: > Hmm, why does this method exist? We probably shouldn't have multiple avenues for > retrieving the AudioManager Will remove after landing https://codereview.chromium.org/2785463005/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2763383002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:80: audio_system_->GetAssociatedOutputDeviceID( If the device is fake, does GetAssociatedOutputDeviceID do anything useful? If not, why not just call/post OpenedOnIOThread? https://codereview.chromium.org/2763383002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:200: remove vertical whitespace https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:96: device.id, base::Bind(&AudioInputDeviceManager::OpenedOnIOThread, this, On 2017/03/28 16:40:16, o1ka wrote: > Since AudioSystem is accessed by AudioInputDeviceManager IO thread only, which > is stopped before the mail loop destruction, we may want to bind > Unretained(this) here. > > guidou@ WDYT? The AIDM is destroyed on the IO thread during IO Message loop destruction https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi... Using Unretained here should be OK.
https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:96: device.id, base::Bind(&AudioInputDeviceManager::OpenedOnIOThread, this, On 2017/04/03 13:04:39, Guido Urdaneta wrote: > On 2017/03/28 16:40:16, o1ka wrote: > > Since AudioSystem is accessed by AudioInputDeviceManager IO thread only, which > > is stopped before the mail loop destruction, we may want to bind > > Unretained(this) here. > > > > guidou@ WDYT? > > The AIDM is destroyed on the IO thread during IO Message loop destruction > https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/medi... > Using Unretained here should be OK. I mean, would be destroyed there provided no more reference counting is used.
Addressed the comments; the CL is pending AudioManager/AudioSystem lifetime fixes. https://codereview.chromium.org/2763383002/diff/20001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:80: audio_system_->GetAssociatedOutputDeviceID( On 2017/04/03 13:04:38, Guido Urdaneta wrote: > If the device is fake, does GetAssociatedOutputDeviceID do anything useful? If > not, why not just call/post OpenedOnIOThread? It gets device association from the AudioManager (which maybe a mock one for tests, returning some expected association). I don't really want to change it in this CL, since it can break some unrelated tests. This "for testing" logic of AudioInputDeviceManager can be cleaned in a separate CL if you wish. https://codereview.chromium.org/2763383002/diff/20001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:200: On 2017/04/03 13:04:38, Guido Urdaneta wrote: > remove vertical whitespace Done. https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... File media/audio/audio_system_impl.h (right): https://codereview.chromium.org/2763383002/diff/40001/media/audio/audio_syste... media/audio/audio_system_impl.h:46: AudioManager* GetAudioManager() const override; On 2017/03/30 15:11:53, o1ka wrote: > On 2017/03/28 18:22:11, DaleCurtis wrote: > > Hmm, why does this method exist? We probably shouldn't have multiple avenues > for > > retrieving the AudioManager > > Will remove after landing https://codereview.chromium.org/2785463005/ Done. https://codereview.chromium.org/2763383002/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): https://codereview.chromium.org/2763383002/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:305: TEST_F(MAYBE_AudioInputDeviceManagerTest, NoCrashOnShutdown) { Does not make sense now when there are no weak pointers and base::Unretained is used. Now it depends on the correct shutdown sequence of the browser main loop.
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dear reviewers, I have a question on this CL: AudioManager/AudioSystem lifetime issue is not specific to this CL and the CL does not add anything new to it, except the comments explaining the problem. Can we proceed with reviewing and landing it, and continue to work on the lifetime fix in parallel?
https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:76: // AudioInputDeviceManager destruction on the main thread. Update the comment: AIDM is not destroyed on the main thread. The intent is to destroy it on the IO thread, during destruction of the IO-thread's message loop. It is still safe to use base::Unretained. Also note that since the object is ref counted, destruction could be somewhere else (but hopefully it is not). https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:79: audio_system_->GetAssociatedOutputDeviceID( Why get associated output device ID of a fake device? What will it return? https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_manag... File media/audio/audio_manager.cc (right): https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_manag... media/audio/audio_manager.cc:257: // If we are on Mac, tasks after this point are not executed, hence this is The code is no longer Mac specific. Update the comment. https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_syste... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_syste... media/audio/audio_system_impl.cc:186: base::Unretained(audio_manager_), input_device_id, When using Unretained, always explain why it is safe to do so.
https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:176: info.device.input.sample_rate = input_params.sample_rate(); We have the copy constructor enabled for AudioParameters, can you use that here? https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_manag... File media/audio/audio_manager.cc (right): https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_manag... media/audio/audio_manager.cc:257: // If we are on Mac, tasks after this point are not executed, hence this is Comment needs to change. Did you mean to leave this in? https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_system.h File media/audio/audio_system.h (right): https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_syste... media/audio/audio_system.h:36: // Must not be called on audio system thread if it differs from the one I'd prefer not to land this without resolving this first unless you expect this warning to have no effect on any current or near term callers (~few weeks). Otherwise we end up in a situation where AM destruction complexity is leaking into new areas.
Updated comments, replied to the questions. https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:76: // AudioInputDeviceManager destruction on the main thread. On 2017/04/06 14:26:41, Guido Urdaneta wrote: > Update the comment: AIDM is not destroyed on the main thread. > The intent is to destroy it on the IO thread, during destruction of the > IO-thread's message loop. It is still safe to use base::Unretained. > > Also note that since the object is ref counted, destruction could be somewhere > else (but hopefully it is not). Done. https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:79: audio_system_->GetAssociatedOutputDeviceID( On 2017/04/06 14:26:41, Guido Urdaneta wrote: > Why get associated output device ID of a fake device? What will it return? This is how the code works now (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...). AudioSystem goes to AudioManager to ask for an associated output device ID. kUseFakeDeviceForMediaStream is "under tests" situation. Some tests provide fake/mock audio managers. So it all depends on how the test is set up and what mocks do. I'm intentionally not changing this behavior, because I don't want to break tests unrelated to this CL. We can explore improvements of this for-test logic separately. https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:176: info.device.input.sample_rate = input_params.sample_rate(); On 2017/04/06 18:37:17, DaleCurtis wrote: > We have the copy constructor enabled for AudioParameters, can you use that here? These are not AudioParameters :( (https://cs.chromium.org/chromium/src/content/public/common/media_stream_reque...) https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_manag... File media/audio/audio_manager.cc (right): https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_manag... media/audio/audio_manager.cc:257: // If we are on Mac, tasks after this point are not executed, hence this is On 2017/04/06 18:37:17, DaleCurtis wrote: > Comment needs to change. Did you mean to leave this in? Done. https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_manag... media/audio/audio_manager.cc:257: // If we are on Mac, tasks after this point are not executed, hence this is On 2017/04/06 14:26:41, Guido Urdaneta wrote: > The code is no longer Mac specific. Update the comment. Done. https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_system.h File media/audio/audio_system.h (right): https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_syste... media/audio/audio_system.h:36: // Must not be called on audio system thread if it differs from the one On 2017/04/06 18:37:17, DaleCurtis wrote: > I'd prefer not to land this without resolving this first unless you expect this > warning to have no effect on any current or near term callers (~few weeks). > Otherwise we end up in a situation where AM destruction complexity is leaking > into new areas. It's just a comment describing the current situation which have existed before the CL. I also added a link to the bug. No new callers are expected. It's a problem similar to AudioManager::Get(), which still exists and which we had number of bugs for. The issue should be covered with AM and AS lifetime / audio thread stop fix. The only effect would be on this CL if we decide to use 3 separate AudioSystem calls instead of all-in-one AudioSystem::GetInputDeviceInfo(). If we choose to do so, we need to post AudioSystem operations on the audio thread and the CL cannot be landed until lifetime issues are fixed. But it does not affect the CL as it is now (with GetInputDeviceInfo()). https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_syste... File media/audio/audio_system_impl.cc (right): https://codereview.chromium.org/2763383002/diff/80001/media/audio/audio_syste... media/audio/audio_system_impl.cc:186: base::Unretained(audio_manager_), input_device_id, On 2017/04/06 14:26:41, Guido Urdaneta wrote: > When using Unretained, always explain why it is safe to do so. see ll.14-16: there is a file-wide comment there (otherwise I need to add it in 10+ places here).
Okay, this lgtm if you don't see anyone else introducing issues around this before it's resolved.
content/browser/renderer_host/media lgtm https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_device_manager.cc (right): https://codereview.chromium.org/2763383002/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_device_manager.cc:79: audio_system_->GetAssociatedOutputDeviceID( On 2017/04/07 12:22:17, o1ka wrote: > On 2017/04/06 14:26:41, Guido Urdaneta wrote: > > Why get associated output device ID of a fake device? What will it return? > > This is how the code works now > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...). > > > AudioSystem goes to AudioManager to ask for an associated output device ID. > kUseFakeDeviceForMediaStream is "under tests" situation. Some tests provide > fake/mock audio managers. So it all depends on how the test is set up and what > mocks do. > > I'm intentionally not changing this behavior, because I don't want to break > tests unrelated to this CL. > We can explore improvements of this for-test logic separately. Acknowledged.
olka@chromium.org changed reviewers: + kinuko@chromium.org
Thanks dalecurtis@ and guidou@. kinuko@chromium.org: Please RS content/browser/speech/speech_recognizer_impl_unittest.cc and content/browser/browser_main_loop.h.
lgtm
On 2017/04/11 14:04:42, kinuko wrote: > lgtm Thanks kinuko@
The CQ bit was checked by olka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one. 1) AudioSystem::GetAssociatedOutputDeviceID() is added + unit tests for it; 2) DeviceOpener helper for AudioInputDeviceManager is introduced, which operates on audio thread and navigates through a sequence of calls to AudioSystem, to receive the reply asynchronously and make the next request until the whole device opening sequence is completed. 3) I chose to make DeviceOpener to work on audio thread to reduce the number of thread hops. But it comes with a cost of accessing AudioSystem on audio thread. To make that work correctly during shutdown, weak pointer access to AudioSystem is added + deleter which destroys it on audio thread. Alternatively, we can make all the requests to AudioSystem from IO thread - then we don't need to care about AudioSystem lifetime, but will be hopping between audio thread and IO several times. 4) AudioInputDeviceManager unit tests are updated to take threading into account. 5) Now we have multiple post-reply events in the device opening sequence instead of one synchronous call. "Media.AudioInputDeviceManager.OpenOnDeviceThreadTime" metric already exists to measure the time of device opening. I reused it, so we'll be able to measure the effect of the change. 6) In the future this sequence of posts and replies will be a series of Mojo IPC exchanges. So we may reconsider the interface later to minimize IPC hops if the metric is seriously affected after switching to Mojo. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one. 1) AudioSystem::GetAssociatedOutputDeviceID() is added + unit tests for it; 2) DeviceOpener helper for AudioInputDeviceManager is introduced, which operates on audio thread and navigates through a sequence of calls to AudioSystem, to receive the reply asynchronously and make the next request until the whole device opening sequence is completed. 3) I chose to make DeviceOpener to work on audio thread to reduce the number of thread hops. But it comes with a cost of accessing AudioSystem on audio thread. To make that work correctly during shutdown, weak pointer access to AudioSystem is added + deleter which destroys it on audio thread. Alternatively, we can make all the requests to AudioSystem from IO thread - then we don't need to care about AudioSystem lifetime, but will be hopping between audio thread and IO several times. 4) AudioInputDeviceManager unit tests are updated to take threading into account. 5) Now we have multiple post-reply events in the device opening sequence instead of one synchronous call. "Media.AudioInputDeviceManager.OpenOnDeviceThreadTime" metric already exists to measure the time of device opening. I reused it, so we'll be able to measure the effect of the change. 6) In the future this sequence of posts and replies will be a series of Mojo IPC exchanges. So we may reconsider the interface later to minimize IPC hops if the metric is seriously affected after switching to Mojo. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=alokp@chromium.org ==========
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491920126400460,
"parent_rev": "869da13e8612fdd406e41bd6b2de3a73d371e369", "commit_rev":
"3dfb4cfaa34efcf58fbf79206197e94402266f18"}
Message was sent while issue was closed.
Description was changed from ========== Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one. 1) AudioSystem::GetAssociatedOutputDeviceID() is added + unit tests for it; 2) DeviceOpener helper for AudioInputDeviceManager is introduced, which operates on audio thread and navigates through a sequence of calls to AudioSystem, to receive the reply asynchronously and make the next request until the whole device opening sequence is completed. 3) I chose to make DeviceOpener to work on audio thread to reduce the number of thread hops. But it comes with a cost of accessing AudioSystem on audio thread. To make that work correctly during shutdown, weak pointer access to AudioSystem is added + deleter which destroys it on audio thread. Alternatively, we can make all the requests to AudioSystem from IO thread - then we don't need to care about AudioSystem lifetime, but will be hopping between audio thread and IO several times. 4) AudioInputDeviceManager unit tests are updated to take threading into account. 5) Now we have multiple post-reply events in the device opening sequence instead of one synchronous call. "Media.AudioInputDeviceManager.OpenOnDeviceThreadTime" metric already exists to measure the time of device opening. I reused it, so we'll be able to measure the effect of the change. 6) In the future this sequence of posts and replies will be a series of Mojo IPC exchanges. So we may reconsider the interface later to minimize IPC hops if the metric is seriously affected after switching to Mojo. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=alokp@chromium.org ========== to ========== Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one. 1) AudioSystem::GetAssociatedOutputDeviceID() is added + unit tests for it; 2) DeviceOpener helper for AudioInputDeviceManager is introduced, which operates on audio thread and navigates through a sequence of calls to AudioSystem, to receive the reply asynchronously and make the next request until the whole device opening sequence is completed. 3) I chose to make DeviceOpener to work on audio thread to reduce the number of thread hops. But it comes with a cost of accessing AudioSystem on audio thread. To make that work correctly during shutdown, weak pointer access to AudioSystem is added + deleter which destroys it on audio thread. Alternatively, we can make all the requests to AudioSystem from IO thread - then we don't need to care about AudioSystem lifetime, but will be hopping between audio thread and IO several times. 4) AudioInputDeviceManager unit tests are updated to take threading into account. 5) Now we have multiple post-reply events in the device opening sequence instead of one synchronous call. "Media.AudioInputDeviceManager.OpenOnDeviceThreadTime" metric already exists to measure the time of device opening. I reused it, so we'll be able to measure the effect of the change. 6) In the future this sequence of posts and replies will be a series of Mojo IPC exchanges. So we may reconsider the interface later to minimize IPC hops if the metric is seriously affected after switching to Mojo. BUG=672468 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=alokp@chromium.org Review-Url: https://codereview.chromium.org/2763383002 Cr-Commit-Position: refs/heads/master@{#463640} Committed: https://chromium.googlesource.com/chromium/src/+/3dfb4cfaa34efcf58fbf79206197... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/3dfb4cfaa34efcf58fbf79206197... |
