Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1186293003:
Implement HasInputDevices in CrasAudioManager (Closed)
|
Created:
5 years, 6 months ago by cychiang Modified:
5 years, 5 months ago CC:
chromium-reviews, sadrul, feature-media-reviews_chromium.org, oshima+watch_chromium.org, kalyank, henrika (OOO until Aug 14) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement HasInputDevices in CrasAudioManager
Currently in CrasAudioManager, HasInputDevices always returns True.
We need to let HasInputDevices reflect the truth for device without
internal microphone like ChromeBox.
Let CrasAudioHandler updates the flag in CrasAudioManager whenever
CrasAudioHandler gets the audio node info from Cras.
Add a property is_for_simple_usage to AudioDeviceType to indicate
that a device is for simple usage, not for special usage like
loopback, always on keyword mic, or keyboard mic.
This property can also replace the logic in audio_detailed_view.cc
to filter out the devices that we do not want to display in UI.
BUG=490851
TEST=Check audio devices in UI does not contain loopback devices.
TEST=Check virtual keyboard does not show microphone for chromebox
(Not tested yet).
R=dalecurtis@chromium.org, derat@chromium.org, dgreid@chromium.org, jennyz@chromium.org
Committed: https://crrev.com/3c3d910e31448a88e014b31334a57242ba5e8960
Cr-Commit-Position: refs/heads/master@{#337791}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address the comments and add unittest #
Total comments: 13
Patch Set 3 : Move audio_manager to be a regular member of CrasAudioManager. Fix InitializeForTesting #
Total comments: 14
Patch Set 4 : Fix unittest. Pass AudioManagerWrapperImpl in production code. Address other nits. #Patch Set 5 : Fix usage of CrasAudioHandler::Initialize in extensions/shell/browser/shell_browser_main_parts.cc #Patch Set 6 : Add AudioManagerWrapperForTesting and fix CrasAudioHandler::InitializeForTesting for unittests #Patch Set 7 : Rebase #Patch Set 8 : Add SetHasInputDevice in FakeAudioManager #
Total comments: 1
Patch Set 9 : Get audio devices from chromeos::CrasAudioHandler in media::AudioManagerCras #Patch Set 10 : Add a comment in cras_audio_handler.h for GetAudioDevices #
Messages
Total messages: 52 (18 generated)
henrika@chromium.org changed reviewers: - henrika@chromium.org
lgtm % comment fix. https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device.h File chromeos/audio/audio_device.h (right): https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... chromeos/audio/audio_device.h:45: bool is_for_simple_usage; Needs a comment explaining what this means.
https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device.cc File chromeos/audio/audio_device.cc (right): https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... chromeos/audio/audio_device.cc:43: return (type == AUDIO_TYPE_HEADPHONE || type == AUDIO_TYPE_INTERNAL_MIC || nit: How about put each type is a separate line, so it looks neater and easier to list all the types? https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... chromeos/audio/audio_device.cc:116: type(AUDIO_TYPE_OTHER), This seems conflicting with IsForSimpleUsage(), the default type AUDIO_TYPE_OTHER is not listed as a simple usage type, while is_for_simple_usage is default to true here? https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device.h File chromeos/audio/audio_device.h (right): https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... chromeos/audio/audio_device.h:45: bool is_for_simple_usage; On 2015/06/16 16:40:51, DaleCurtis wrote: > Needs a comment explaining what this means. Yes, please add a comment to explain its meaning.
is there any way to add a unit test for this change, or is it already covered by existing tests? https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device.cc File chromeos/audio/audio_device.cc (right): https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... chromeos/audio/audio_device.cc:126: is_for_simple_usage = IsForSimpleUsage(type); this member is just derived from another member, type. it'd be better to make it be a simple inlined public is_for_simple_usage() method. that eliminates the risk of the two getting out of sync. https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager.h File media/audio/audio_manager.h (right): https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager.h... media/audio/audio_manager.h:216: // Only supported on ChromeOS. nit: s/ChromeOS/Chrome OS/ (the product name has always had a space in it) https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager_b... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager_b... media/audio/audio_manager_base.h:115: void SetHasInputDevices(bool has_input_devices) override; nit: if these methods all implement the AudioManager interface, the usual approach is to put them all together without any blank lines between them and preface the block with a comment like: // AudioManager:
Also added unittests. Thanks a lot! https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device.cc File chromeos/audio/audio_device.cc (right): https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... chromeos/audio/audio_device.cc:43: return (type == AUDIO_TYPE_HEADPHONE || type == AUDIO_TYPE_INTERNAL_MIC || On 2015/06/16 21:51:35, jennyz wrote: > nit: How about put each type is a separate line, so it looks neater and easier > to list all the types? Done. https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... chromeos/audio/audio_device.cc:116: type(AUDIO_TYPE_OTHER), On 2015/06/16 21:51:35, jennyz wrote: > This seems conflicting with IsForSimpleUsage(), the default type > AUDIO_TYPE_OTHER is not listed as a simple usage type, while is_for_simple_usage > is default to true here? Done. https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... chromeos/audio/audio_device.cc:126: is_for_simple_usage = IsForSimpleUsage(type); On 2015/06/18 14:53:20, Daniel Erat wrote: > this member is just derived from another member, type. it'd be better to make it > be a simple inlined public is_for_simple_usage() method. that eliminates the > risk of the two getting out of sync. Done. https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device.h File chromeos/audio/audio_device.h (right): https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... chromeos/audio/audio_device.h:45: bool is_for_simple_usage; On 2015/06/16 21:51:35, jennyz wrote: > On 2015/06/16 16:40:51, DaleCurtis wrote: > > Needs a comment explaining what this means. > > Yes, please add a comment to explain its meaning. Done. https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager.h File media/audio/audio_manager.h (right): https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager.h... media/audio/audio_manager.h:216: // Only supported on ChromeOS. On 2015/06/18 14:53:20, Daniel Erat wrote: > nit: s/ChromeOS/Chrome OS/ (the product name has always had a space in it) Done. https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager_b... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager_b... media/audio/audio_manager_base.h:115: void SetHasInputDevices(bool has_input_devices) override; On 2015/06/18 14:53:20, Daniel Erat wrote: > nit: if these methods all implement the AudioManager interface, the usual > approach is to put them all together without any blank lines between them and > preface the block with a comment like: > > // AudioManager: Done.
A new class AudioManagerWrapper is added to CrasAudioHandler to provide the interface to call SetHasInputDevice on AudioManager. In cras_audio_handler_unittest AudioManagerWrapper can be mocked by either StrictMock or NiceMock. Without this layer of abstraction, the unittest will be broken because on workstation we can not create an AudioManagerCras object. On 2015/06/23 06:05:40, cychiang wrote: > Also added unittests. Thanks a lot! > > https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device.cc > File chromeos/audio/audio_device.cc (right): > > https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... > chromeos/audio/audio_device.cc:43: return (type == AUDIO_TYPE_HEADPHONE || type > == AUDIO_TYPE_INTERNAL_MIC || > On 2015/06/16 21:51:35, jennyz wrote: > > nit: How about put each type is a separate line, so it looks neater and easier > > to list all the types? > > Done. > > https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... > chromeos/audio/audio_device.cc:116: type(AUDIO_TYPE_OTHER), > On 2015/06/16 21:51:35, jennyz wrote: > > This seems conflicting with IsForSimpleUsage(), the default type > > AUDIO_TYPE_OTHER is not listed as a simple usage type, while > is_for_simple_usage > > is default to true here? > > Done. > > https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... > chromeos/audio/audio_device.cc:126: is_for_simple_usage = > IsForSimpleUsage(type); > On 2015/06/18 14:53:20, Daniel Erat wrote: > > this member is just derived from another member, type. it'd be better to make > it > > be a simple inlined public is_for_simple_usage() method. that eliminates the > > risk of the two getting out of sync. > > Done. > > https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device.h > File chromeos/audio/audio_device.h (right): > > https://codereview.chromium.org/1186293003/diff/1/chromeos/audio/audio_device... > chromeos/audio/audio_device.h:45: bool is_for_simple_usage; > On 2015/06/16 21:51:35, jennyz wrote: > > On 2015/06/16 16:40:51, DaleCurtis wrote: > > > Needs a comment explaining what this means. > > > > Yes, please add a comment to explain its meaning. > > Done. > > https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager.h > File media/audio/audio_manager.h (right): > > https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager.h... > media/audio/audio_manager.h:216: // Only supported on ChromeOS. > On 2015/06/18 14:53:20, Daniel Erat wrote: > > nit: s/ChromeOS/Chrome OS/ (the product name has always had a space in it) > > Done. > > https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager_b... > File media/audio/audio_manager_base.h (right): > > https://codereview.chromium.org/1186293003/diff/1/media/audio/audio_manager_b... > media/audio/audio_manager_base.h:115: void SetHasInputDevices(bool > has_input_devices) override; > On 2015/06/18 14:53:20, Daniel Erat wrote: > > nit: if these methods all implement the AudioManager interface, the usual > > approach is to put them all together without any blank lines between them and > > preface the block with a comment like: > > > > // AudioManager: > > Done.
thanks for the tests! https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/audio_de... File chromeos/audio/audio_device.h (right): https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/audio_de... chromeos/audio/audio_device.h:48: inline bool IsForSimpleUsage() { you don't need 'inline' here, and you should use lowercase_with_underscore style (https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...): bool is_for_simple_usage() { return ... } https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:36: static CrasAudioHandler::AudioManagerWrapper* audio_manager = NULL; does this really need to be a global, or could it just be a regular member of CrasAudioHandler? https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:105: // Set audio manager wrapper for test. Must be called before Initialize delete this comment; it's already present in the header https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:942: void CrasAudioHandler::SetAudioManagerHasInputDevices(bool has_input_devices) { this method is just one line; it seems simpler to just inline the call to audio_manager->SetHasInputDevices() in UpdateAudioManagerHasInputDevices() https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:25: nit: delete extra blank line here https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:88: CrasAudioHandler::AudioManagerWrapper* test_wrapper); pass scoped_ptr<CrasAudioHandler::AudioManagerWrapper> so the ownership transfer is obvious https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:2609: AudioNodeList audio_nodes = {kHeadphone, i don't think that this initialization style is allowed in chromium: see "(Uniform) Initialization Syntax" at http://chromium-cpp.appspot.com/
Thanks for the pointer to the guides! I moved audio_manager to be a regular member in CrasAudioHandler, and make it a scoped_ptr. Initialize, InitializeForTesting, and CrasAudioHandler constructor needs to be changed as well. cras_audio_handler_unittest is changed to meet the new usage. Also, fix the missing SetHasInputDevice implementation in mock_audio_manager which is used in other unittests. Thanks a lot! https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/audio_de... File chromeos/audio/audio_device.h (right): https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/audio_de... chromeos/audio/audio_device.h:48: inline bool IsForSimpleUsage() { On 2015/06/23 14:15:49, Daniel Erat wrote: > you don't need 'inline' here, and you should use lowercase_with_underscore style > (https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...): > > bool is_for_simple_usage() { > return ... > } Done. https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:36: static CrasAudioHandler::AudioManagerWrapper* audio_manager = NULL; On 2015/06/23 14:15:49, Daniel Erat wrote: > does this really need to be a global, or could it just be a regular member of > CrasAudioHandler? Done. Now this manager needs to be passed through CrasAudioHandler::Initialize and through the constructor of CrasAudioHandler. https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:105: // Set audio manager wrapper for test. Must be called before Initialize On 2015/06/23 14:15:49, Daniel Erat wrote: > delete this comment; it's already present in the header Done. https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:942: void CrasAudioHandler::SetAudioManagerHasInputDevices(bool has_input_devices) { On 2015/06/23 14:15:49, Daniel Erat wrote: > this method is just one line; it seems simpler to just inline the call to > audio_manager->SetHasInputDevices() in UpdateAudioManagerHasInputDevices() Done. https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:25: On 2015/06/23 14:15:49, Daniel Erat wrote: > nit: delete extra blank line here Done. https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/1186293003/diff/20001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:2609: AudioNodeList audio_nodes = {kHeadphone, On 2015/06/23 14:15:49, Daniel Erat wrote: > i don't think that this initialization style is allowed in chromium: see > "(Uniform) Initialization Syntax" at http://chromium-cpp.appspot.com/ Done.
i'm out for the rest of the week so i'll be slow in responding, but this generally looks fine to me after a few more comments are addressed. if other reviewers are happy, feel free to check it in without an explicit el gee tee em from me; otherwise i can probably take another look either tonight or tomorrow morning. https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:475: if (!audio_pref_handler.get()) this isn't part of your change, but it seems strange -- the c'tor skips initializing the object if a null arguments is passed? https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:89: scoped_ptr<AudioManagerWrapper> audio_manager = NULL); default arguments aren't allowed in cases like this: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu... how about making the audio_manager argument mandatory and just passing in a concrete implementation of it in the production code? https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:213: explicit CrasAudioHandler( nit: you don't need 'explicit' here now that this has two arguments https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:349: // audio_manager_ will point to an AudioManagerWrapperImpl object or nit: "will hold" would probably be more accurate than "will point to" now that this is a scoped_ptr https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:20: using testing::InSequence; nit: move this above NiceMock to keep the list alphabetized https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:360: mock_audio_manager_.reset(new StrictMock<MockAudioManagerWrapper>()); it might be better to make mock_audio_manager_ be a bare pointer rather than a scoped_ptr. SetUpCrasAudioHandler() passes ownership of it, so you're currently ending up with a null member that you can't use. https://codereview.chromium.org/1186293003/diff/40001/media/audio/mock_audio_... File media/audio/mock_audio_manager.cc (right): https://codereview.chromium.org/1186293003/diff/40001/media/audio/mock_audio_... media/audio/mock_audio_manager.cc:109: void MockAudioManager::SetHasInputDevices(bool has_input_devices) {}; nit: remove trailing semicolon
Thanks a lot! https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:475: if (!audio_pref_handler.get()) On 2015/06/24 13:37:45, Daniel Erat (OOO until 6-29) wrote: > this isn't part of your change, but it seems strange -- the c'tor skips > initializing the object if a null arguments is passed? This was added long time ago in https://chromiumcodereview.appspot.com/14678004/#ps29001, Perhaps Jenny or James knows better if this is still needed. https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:89: scoped_ptr<AudioManagerWrapper> audio_manager = NULL); On 2015/06/24 13:37:45, Daniel Erat (OOO until 6-29) wrote: > default arguments aren't allowed in cases like this: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu... > > how about making the audio_manager argument mandatory and just passing in a > concrete implementation of it in the production code? Done. Change production code at chrome/browser/chromeos/chrome_browser_main_chromeos.cc. Also, move AudioManagerWrapperImpl to public of CrasAudioHandler. https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:213: explicit CrasAudioHandler( On 2015/06/24 13:37:45, Daniel Erat (OOO until 6-29) wrote: > nit: you don't need 'explicit' here now that this has two arguments Done. https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.h:349: // audio_manager_ will point to an AudioManagerWrapperImpl object or On 2015/06/24 13:37:45, Daniel Erat (OOO until 6-29) wrote: > nit: "will hold" would probably be more accurate than "will point to" now that > this is a scoped_ptr Done. https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:20: using testing::InSequence; On 2015/06/24 13:37:45, Daniel Erat (OOO until 6-29) wrote: > nit: move this above NiceMock to keep the list alphabetized InSequence is not used after using mock_audio_manager_bare_pointer https://codereview.chromium.org/1186293003/diff/40001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:360: mock_audio_manager_.reset(new StrictMock<MockAudioManagerWrapper>()); On 2015/06/24 13:37:45, Daniel Erat (OOO until 6-29) wrote: > it might be better to make mock_audio_manager_ be a bare pointer rather than a > scoped_ptr. SetUpCrasAudioHandler() passes ownership of it, so you're currently > ending up with a null member that you can't use. Done. I see! make_scoped_ptr is very useful. This also fix the problem that InSequence does not fully test the scenario because ideally SetHasInputDevice(true) can only be called after mic node is added to CrasAudioHandler. https://codereview.chromium.org/1186293003/diff/40001/media/audio/mock_audio_... File media/audio/mock_audio_manager.cc (right): https://codereview.chromium.org/1186293003/diff/40001/media/audio/mock_audio_... media/audio/mock_audio_manager.cc:109: void MockAudioManager::SetHasInputDevices(bool has_input_devices) {}; On 2015/06/24 13:37:45, Daniel Erat (OOO until 6-29) wrote: > nit: remove trailing semicolon Done.
thanks! lgtm
The CQ bit was checked by cychiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1186293003/#ps60001 (title: "Fix unittest. Pass AudioManagerWrapperImpl in production code. Address other nits.")
The CQ bit was unchecked by cychiang@chromium.org
The CQ bit was checked by cychiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1186293003/#ps60001 (title: "Fix unittest. Pass AudioManagerWrapperImpl in production code. Address other nits.")
The CQ bit was unchecked by cychiang@chromium.org
The CQ bit was checked by cychiang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1186293003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by cychiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1186293003/#ps100001 (title: "Add AudioManagerWrapperForTesting and fix CrasAudioHandler::InitializeForTesting for unittests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1186293003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by cychiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1186293003/#ps80002 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1186293003/80002
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
It seems there are two issues in this CL. 1. either compile error or cycle in dependency graph: http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... linux_chromium_chromeos_compile_dbg_ng build failed at ../../chromeos/audio/cras_audio_handler.cc:100: error: undefined reference to 'media::AudioManager::Get()' I suspect this is because I did not add '../media/media.gyp:media' to the dependencies of target "chromeos". I can not do this because there will be a cycle in dependency graph detected in gclient runhooks: gyp: Cycles in dependency graph detected: Cycle: /usr/local/google/work/chromium/src/ui/ozone/ozone.gyp:ozone_platform_drm#target -> /usr/local/google/work/chromium/src/ui/base/ui_base.gyp:ui_base#target -> /usr/local/google/work/chromium/src/chromeos/chromeos.gyp:chromeos#target -> /usr/local/google/work/chromium/src/media/media.gyp:media#target -> /usr/local/google/work/chromium/src/ui/ozone/ozone.gyp:ozone#target -> /usr/local/google/work/chromium/src/ui/ozone/ozone.gyp:ozone_platform_drm#target Currently I only add '../media/media.gyp:media' to the dependencies of target "chromeos_unittests". When chromeos_unittests is built, there is no such problem. This is shown in other builds that run tests. 2. The second problem is in browser_tests, interactive_ui_tests, chromevox_tests, and extension_browsertests. http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... They all failed at audio_manager_base.cc(414)] Check failed: false. I think this is because these tests use ChromeBrowserMainPartsChromeos as production code, and therefore it creates a AudioManagerWrapperImpl which will really calls AudioManager::Get()->SetHasInputDevice. However, since the test is actually run on linux, not chrome os, the audio manager it use will not be AudioManagerCras. So, the test reaches SetHasInputDevice in AudioManagerCras, which has an NOTREACHED() check. For issue 2, i think we can add a dummy implementation in SetHasInputDevice in AudioManagerBase. However, for issue 1, I am still not sure how this cycle dependency problem can be solved. Maybe cras_audio_handler, as a lower level module, should not refer to media, which is a higher level module ? Maybe we should check again if we can make AudioManagerCras an observer of CrasAudioHandler? Any suggestion would be appreciated. Thanks!
On 2015/07/01 06:35:54, cychiang wrote: > It seems there are two issues in this CL. > 1. either compile error or cycle in dependency graph: > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > linux_chromium_chromeos_compile_dbg_ng build failed at > ../../chromeos/audio/cras_audio_handler.cc:100: error: undefined reference to > 'media::AudioManager::Get()' > I suspect this is because I did not add '../media/media.gyp:media' to the > dependencies of target "chromeos". > I can not do this because there will be a cycle in dependency graph detected in > gclient runhooks: > > gyp: Cycles in dependency graph detected: > Cycle: > /usr/local/google/work/chromium/src/ui/ozone/ozone.gyp:ozone_platform_drm#target > -> /usr/local/google/work/chromium/src/ui/base/ui_base.gyp:ui_base#target -> > /usr/local/google/work/chromium/src/chromeos/chromeos.gyp:chromeos#target -> > /usr/local/google/work/chromium/src/media/media.gyp:media#target -> > /usr/local/google/work/chromium/src/ui/ozone/ozone.gyp:ozone#target -> > /usr/local/google/work/chromium/src/ui/ozone/ozone.gyp:ozone_platform_drm#target > > Currently I only add '../media/media.gyp:media' to the dependencies of target > "chromeos_unittests". When chromeos_unittests is built, there is no such > problem. This is shown in other builds that run tests. > > 2. The second problem is in browser_tests, interactive_ui_tests, > chromevox_tests, and extension_browsertests. > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > They all failed at audio_manager_base.cc(414)] Check failed: false. > I think this is because these tests use ChromeBrowserMainPartsChromeos as > production code, and therefore it creates a AudioManagerWrapperImpl which will > really calls AudioManager::Get()->SetHasInputDevice. > However, since the test is actually run on linux, not chrome os, the audio > manager it use will not be AudioManagerCras. So, the test reaches > SetHasInputDevice in AudioManagerCras, which has an NOTREACHED() check. Sorry! A typo It reaches SetHasInputDevices in AudioManagerBase, which has an NOTREACHED() check. > > For issue 2, i think we can add a dummy implementation in SetHasInputDevice in > AudioManagerBase. However, for issue 1, I am still not sure how this cycle > dependency problem can be solved. Maybe cras_audio_handler, as a lower level > module, should not refer to media, which is a higher level module ? Maybe we > should check again if we can make AudioManagerCras an observer of > CrasAudioHandler? > Any suggestion would be appreciated. > Thanks!
[cc-ing stevenjb@ for thoughts] maybe chromeos/audio/ should live somewhere else. media/audio/ already has a bunch of platform-specific subdirectories like linux, max, openbsd, and win. with the disclaimer that i don't know anything about media/, would somewhere like media/audio/chromeos/ be a more-appropriate location for this code? would that fix the dependency issue? there's already media/audio/cras/, so if you moved this code you'd probably also want to move the cras code to somewhere like media/audio/chromeos/cras/.
On 2015/07/01 14:04:51, Daniel Erat wrote: > [cc-ing stevenjb@ for thoughts] > > maybe chromeos/audio/ should live somewhere else. media/audio/ already has a > bunch of platform-specific subdirectories like linux, max, openbsd, and win. > with the disclaimer that i don't know anything about media/, would somewhere > like media/audio/chromeos/ be a more-appropriate location for this code? would > that fix the dependency issue? there's already media/audio/cras/, so if you > moved this code you'd probably also want to move the cras code to somewhere like > media/audio/chromeos/cras/. I would /strongly/ prefer we avoid adding a src/media dependency to src/chromeos. src/media depends on (parts of) src/ui which is a pretty large dependency. (See the comment in src/DEPS - adding src/media/audio to DEPS brings in everything in src/media/DEPS). In theory we would like to be able to use src/chromeos in smaller applications. In the short term we need to avoid the circular dependency you noted. Without looking too closely at the code, there are two general approaches here: 1) As Dan suggested, move code that depends on src/media into src/media. If that is straightforward, that seems ideal. Adding a src/chromeos dependency to src/media (on chromeos) should be fine, that is one of the reasons to keep the src/chromeos dependencies to a minimum. 2) Create a Delegate for CrasAudioHandler and implement the Delegate in src/ash (or wherever). If (1) is straightforward (I agree with Dan that if there is already a media/audio/linux and a media/audio/cras/ then media/audio/chromeos sounds like the right place for this), I would go with that. If that turns out to be a major re-factoring, the Delegate approach might be easier for now.
On 2015/07/01 15:43:37, stevenjb wrote: > On 2015/07/01 14:04:51, Daniel Erat wrote: > > [cc-ing stevenjb@ for thoughts] > > > > maybe chromeos/audio/ should live somewhere else. media/audio/ already has a > > bunch of platform-specific subdirectories like linux, max, openbsd, and win. > > with the disclaimer that i don't know anything about media/, would somewhere > > like media/audio/chromeos/ be a more-appropriate location for this code? would > > that fix the dependency issue? there's already media/audio/cras/, so if you > > moved this code you'd probably also want to move the cras code to somewhere > like > > media/audio/chromeos/cras/. > > I would /strongly/ prefer we avoid adding a src/media dependency to > src/chromeos. src/media depends on (parts of) src/ui which is a pretty large > dependency. (See the comment in src/DEPS - adding src/media/audio to DEPS brings > in everything in src/media/DEPS). In theory we would like to be able to use > src/chromeos in smaller applications. In the short term we need to avoid the > circular dependency you noted. > > Without looking too closely at the code, there are two general approaches here: > 1) As Dan suggested, move code that depends on src/media into src/media. If that > is straightforward, that seems ideal. Adding a src/chromeos dependency to > src/media (on chromeos) should be fine, that is one of the reasons to keep the > src/chromeos dependencies to a minimum. > 2) Create a Delegate for CrasAudioHandler and implement the Delegate in src/ash > (or wherever). > > If (1) is straightforward (I agree with Dan that if there is already a > media/audio/linux and a media/audio/cras/ then media/audio/chromeos sounds like > the right place for this), I would go with that. If that turns out to be a major > re-factoring, the Delegate approach might be easier for now. Thanks Daniel and Steven. Moving chromeos/audio/ would be too complicated for me. I will try to go with delegate method first. The AudioManagerWrapper in the current CL is like a delegate, which only provides the interface to call SetHasInputDevice. If I move AudioManagerWrapperImpl to media/ , this should solve the dependency problem. Thanks a lot!
If possible moving to media/audio/cras is fine with me. I thought some parts of the handler needed dbus though which would mean they can't be moved since media/ doesn't know about the UI thread or anything like that.
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/1186293003/diff/130001/chromeos/audio/cras_au... File chromeos/audio/cras_audio_handler.h (right): https://codereview.chromium.org/1186293003/diff/130001/chromeos/audio/cras_au... chromeos/audio/cras_audio_handler.h:26: class AudioManager; Is this needed? I don't see any references to it below.
On 2015/07/01 17:59:22, DaleCurtis wrote: > If possible moving to media/audio/cras is fine with me. I thought some parts of > the handler needed dbus though which would mean they can't be moved since media/ > doesn't know about the UI thread or anything like that. The DBus handlers use global instances, e.g. chromeos::DBusThreadManager::Get()->GetCrasAudioClient(). Moving the code out of src/chromeos won't change any of the thread dependencies (i.e. any calls onto CrasAudioClient should still be made from the primary/UI thread).
On 2015/07/01 21:18:10, stevenjb wrote: > On 2015/07/01 17:59:22, DaleCurtis wrote: > > If possible moving to media/audio/cras is fine with me. I thought some parts > of > > the handler needed dbus though which would mean they can't be moved since > media/ > > doesn't know about the UI thread or anything like that. > > The DBus handlers use global instances, e.g. > chromeos::DBusThreadManager::Get()->GetCrasAudioClient(). Moving the code out of > src/chromeos won't change any of the thread dependencies (i.e. any calls onto > CrasAudioClient should still be made from the primary/UI thread). If we can call methods of CrasAudioHandler in AudioManager, I feel that making AudioManagerCras extends CrasAudioHandler::AudioObserver seems more right. AudioManagerCras can get new audio devices using chromeos::CrasAudioHandler::Get()->GetAudioDevices(&devices), and check if there is any input device of simple usage. Also, I can try to call methods of CrasAudioHandler that is related to dbus( e.g. SetOutputMuteInternal) in AudioManager, and see if that works (although not needed). Thanks a lot!
On 2015/07/02 05:11:46, cychiang wrote: > On 2015/07/01 21:18:10, stevenjb wrote: > > On 2015/07/01 17:59:22, DaleCurtis wrote: > > > If possible moving to media/audio/cras is fine with me. I thought some parts > > of > > > the handler needed dbus though which would mean they can't be moved since > > media/ > > > doesn't know about the UI thread or anything like that. > > > > The DBus handlers use global instances, e.g. > > chromeos::DBusThreadManager::Get()->GetCrasAudioClient(). Moving the code out > of > > src/chromeos won't change any of the thread dependencies (i.e. any calls onto > > CrasAudioClient should still be made from the primary/UI thread). > > If we can call methods of CrasAudioHandler in AudioManager, I feel that making > AudioManagerCras extends CrasAudioHandler::AudioObserver seems more right. > AudioManagerCras can get new audio devices using > chromeos::CrasAudioHandler::Get()->GetAudioDevices(&devices), and check if there > is any input device of simple usage. > Also, I can try to call methods of CrasAudioHandler that is related to dbus( > e.g. SetOutputMuteInternal) in AudioManager, and see if that works (although not > needed). > Thanks a lot! After checking the code again, I guess I can try to put the logic to search through audio devices in AudioManagerCras::HasAudioInputDevices, just like how AudioManagerPulse and AudioManagerAlsa handle this. Then I don't need to make AudioManagerCras an observer, or make a delegate in CrasAudioHandler. This would be much simpler. Thanks a lot!
Now I move the logic to media::AudioManagerCras. However, Dale was right about calling methods of CrasAudioHandler in AudioManagerCras. It crashed right away if I call methods related to Dbus, e.g. chromeos::CrasAudioHandler::Get()->SetOutputMute(true). But if I just call chromeos::CrasAudioHandler::Get()->GetAudioDevices(&devices), there is no problem. I think the code is much simpler now. However, we need some mechanism to avoid calling methods that lead to crash. I will investigate the coredump to see why it crashes. Thanks a lot!!
On 2015/07/02 10:59:32, cychiang wrote: > Now I move the logic to media::AudioManagerCras. > However, Dale was right about calling methods of CrasAudioHandler in > AudioManagerCras. > It crashed right away if I call methods related to Dbus, e.g. > chromeos::CrasAudioHandler::Get()->SetOutputMute(true). > But if I just call > chromeos::CrasAudioHandler::Get()->GetAudioDevices(&devices), there is no > problem. > > I think the code is much simpler now. However, we need some mechanism to avoid > calling methods that lead to crash. > I will investigate the coredump to see why it crashes. > Thanks a lot!! If it's crashing right away you may be running into a CHECK. Try running a Debug build if you aren't already. If you are making DBus calls from a non UI task runner (thread), then you may need to Post a task to run them on the main/UI task runner (which you may need to pass to the audio handler when it is initialized). I'm not sure why moving the code would suddenly trigger that however.
On 2015/07/02 16:08:52, stevenjb wrote: > On 2015/07/02 10:59:32, cychiang wrote: > > Now I move the logic to media::AudioManagerCras. > > However, Dale was right about calling methods of CrasAudioHandler in > > AudioManagerCras. > > It crashed right away if I call methods related to Dbus, e.g. > > chromeos::CrasAudioHandler::Get()->SetOutputMute(true). > > But if I just call > > chromeos::CrasAudioHandler::Get()->GetAudioDevices(&devices), there is no > > problem. > > > > I think the code is much simpler now. However, we need some mechanism to avoid > > calling methods that lead to crash. > > I will investigate the coredump to see why it crashes. > > Thanks a lot!! > > If it's crashing right away you may be running into a CHECK. Try running a Debug > build if you aren't already. > > If you are making DBus calls from a non UI task runner (thread), then you may > need to Post a task to run them on the main/UI task runner (which you may need > to pass to the audio handler when it is initialized). I'm not sure why moving > the code would suddenly trigger that however. Thanks a lot for the tips. I put CrasAudioHandler::Get()->SetOutputMute(true) in AudioManagerCras::HasAudioInputDevices() just for testing. I ran a debug build and found that when I click the voice recognition icon, it crashed at bus_->AssertOnOriginThread(); https://code.google.com/p/chromium/codesearch#chromium/src/dbus/object_proxy.... This is because the call is from I/O thread, not UI thread. From this assert it is clear enough that methods related to dbus can only be called from UI thread. If that is needed, it should be done by posting a task you just mentioned. For methods like GetAudioDevices, it does not use dbus, so it can be called from I/O thread. For now I am going to put a comment in the cras_audio_handler.h to show that GetAudioDevices can be called from I/O thread. Thanks a lot!
The CQ bit was checked by cychiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1186293003/#ps170001 (title: "Add a comment in cras_audio_handler.h for GetAudioDevices")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1186293003/170001
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3c3d910e31448a88e014b31334a57242ba5e8960 Cr-Commit-Position: refs/heads/master@{#337791}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:170001) has been created in https://codereview.chromium.org/1220873010/ by mnissler@chromium.org. The reason for reverting is: Looks like this broke the build: http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=ChromiumO.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
