|
|
DescriptionHandle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices.
BUG=698327
TEST=See the feature spec document in the bug.
TBR=dalecurtis@chromium.org
Review-Url: https://codereview.chromium.org/2721733003
Cr-Commit-Position: refs/heads/master@{#454933}
Committed: https://chromium.googlesource.com/chromium/src/+/fbaa3e6092a9d8d08a91cd71fdd104dca8df2cfc
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : Fix build issue. #Patch Set 4 : Fix test issue. #
Total comments: 30
Patch Set 5 : Address the code review comments. #Patch Set 6 : Nit: log only the invalid camera facing mode value. #
Messages
Total messages: 28 (12 generated)
Description was changed from ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=672537 ========== to ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=672537 ==========
jennyz@chromium.org changed reviewers: + hychao@chromium.org, shenghao@chromium.org
dalecurtis@, please review base/media. shenghao@ and hychao@, please review everything.
Description was changed from ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=672537 ========== to ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=672537,679061 ==========
On 2017/03/01 00:39:16, jennyz wrote: Please add "TEST=" in the CL description to explain how you tested this CL.
https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:141: NOTREACHED(); I am not 100% sure but I think it's possible to reach here. External camera should have facing=MEDIA_VIDEO_FACING_NONE. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:174: // Switch to front mic properly. Why is it "front mic"? Do you mean "correct mic"? https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:175: DeviceActivateType activate_by = s/activate_by/activated_by/ https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:177: SwitchToDevice(*GetDeviceByType(AUDIO_TYPE_FRONT_MIC), true, activate_by); Why do we also switch to AUDIO_TYPE_FRONT_MIC? Is it possible to switch to AUDIO_TYPE_BACK_MIC? https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1234: if (IsFrontOrRearMic(top_device) && HasDualInternalMic() && IsCameraOn()) { I think you can write: if (IsFrontOrRearMic(top_device)) { ActivateInternalMicForActiveCamera(); return; } https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1504: // reaa microphone for rear camera. s/reaa/rear/ https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1507: if (IsCameraOn()) { Why do we need to check IsCameraOn() twice here and line 1491? https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1522: break; You don't need "break" here because of the return statement. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1524: NOTREACHED(); Same as line 141 https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1530: for (AudioDeviceMap::const_iterator it = audio_devices_.begin(); For all the "AudioDeviceMap::const_iterator", can we use "const auto" to replace them? https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1562: bool CrasAudioHandler::HasExternalDevice(bool is_input) const { It seems that this method is only called in one place, and |is_input| is true. Can we remove the parameter? https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:223: class FakeVideoCaptureMagner { s/FakeVideoCaptureMagner/FakeVideoCaptureManager/ https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:3792: TEST_P(CrasAudioHandlerTest, SwitchFrontRearCamera) { I think it's beneficial to add a test for the case that both front and rear camera are active.
https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:177: SwitchToDevice(*GetDeviceByType(AUDIO_TYPE_FRONT_MIC), true, activate_by); On 2017/03/01 11:25:55, shenghao wrote: > Why do we also switch to AUDIO_TYPE_FRONT_MIC? Is it possible to switch to > AUDIO_TYPE_BACK_MIC? typo: also => always
Description was changed from ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=672537,679061 ========== to ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=679061 TEST=See the feature spec document in the bug. ==========
https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:141: NOTREACHED(); On 2017/03/01 11:25:55, shenghao wrote: > I am not 100% sure but I think it's possible to reach here. External camera > should have facing=MEDIA_VIDEO_FACING_NONE. I need to know in which scenario it will pass MEDIA_VIDEO_FACING_NONE in under dual internal mic mode, and how we are going to handle it? For now, I would just log a warning and do nothing just in case it happens. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:174: // Switch to front mic properly. On 2017/03/01 11:25:55, shenghao wrote: > Why is it "front mic"? Do you mean "correct mic"? In R58, chrome code will only handle the dual camera/dual mic case with only one camera active at a time, which is the typical chrome book and camera app case. For the developers, they can use audio extension api to set both mic to be active if they need to do so for their app. For our use case I mentioned above, the camera stop, typically, user close the camera app, we would switch back to internal mic and the front mic is the default mic to set active. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:175: DeviceActivateType activate_by = On 2017/03/01 11:25:55, shenghao wrote: > s/activate_by/activated_by/ Done. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:177: SwitchToDevice(*GetDeviceByType(AUDIO_TYPE_FRONT_MIC), true, activate_by); On 2017/03/01 11:25:55, shenghao wrote: > Why do we also switch to AUDIO_TYPE_FRONT_MIC? Is it possible to switch to > AUDIO_TYPE_BACK_MIC? See comments above. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:177: SwitchToDevice(*GetDeviceByType(AUDIO_TYPE_FRONT_MIC), true, activate_by); On 2017/03/01 11:27:54, shenghao wrote: > On 2017/03/01 11:25:55, shenghao wrote: > > Why do we also switch to AUDIO_TYPE_FRONT_MIC? Is it possible to switch to > > AUDIO_TYPE_BACK_MIC? > > typo: also => always Acknowledged. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1234: if (IsFrontOrRearMic(top_device) && HasDualInternalMic() && IsCameraOn()) { On 2017/03/01 11:25:55, shenghao wrote: > I think you can write: > > if (IsFrontOrRearMic(top_device)) { > ActivateInternalMicForActiveCamera(); > return; > } The above code is not equivalent to the original lines. But I did simplify it by calling ActivateInternalMicForActiveCamera() inside the if block. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1504: // reaa microphone for rear camera. On 2017/03/01 11:25:55, shenghao wrote: > s/reaa/rear/ Done. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1507: if (IsCameraOn()) { On 2017/03/01 11:25:55, shenghao wrote: > Why do we need to check IsCameraOn() twice here and line 1491? Removed IsCameraOn() from 1491. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1522: break; On 2017/03/01 11:25:55, shenghao wrote: > You don't need "break" here because of the return statement. Done. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1524: NOTREACHED(); On 2017/03/01 11:25:55, shenghao wrote: > Same as line 141 This code path is not reachable, the code path to call this function would check and filter out the case that facing is not the first two cases. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1530: for (AudioDeviceMap::const_iterator it = audio_devices_.begin(); On 2017/03/01 11:25:55, shenghao wrote: > For all the "AudioDeviceMap::const_iterator", can we use "const auto" to replace > them? Done. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:1562: bool CrasAudioHandler::HasExternalDevice(bool is_input) const { On 2017/03/01 11:25:55, shenghao wrote: > It seems that this method is only called in one place, and |is_input| is true. > Can we remove the parameter? I would rather keep it just in case it could be used by output device in the future. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler_unittest.cc (right): https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:223: class FakeVideoCaptureMagner { On 2017/03/01 11:25:55, shenghao wrote: > s/FakeVideoCaptureMagner/FakeVideoCaptureManager/ Done. https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler_unittest.cc:3792: TEST_P(CrasAudioHandlerTest, SwitchFrontRearCamera) { On 2017/03/01 11:25:55, shenghao wrote: > I think it's beneficial to add a test for the case that both front and rear > camera are active. Added such a case at the end.
On 2017/03/02 17:31:41, jennyz wrote: > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > File chromeos/audio/cras_audio_handler.cc (right): > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:141: NOTREACHED(); > On 2017/03/01 11:25:55, shenghao wrote: > > I am not 100% sure but I think it's possible to reach here. External camera > > should have facing=MEDIA_VIDEO_FACING_NONE. > > I need to know in which scenario it will pass MEDIA_VIDEO_FACING_NONE in under > dual internal mic mode, and how we are going to handle it? For now, I would just > log a warning and do nothing just in case it happens. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:174: // Switch to front mic properly. > On 2017/03/01 11:25:55, shenghao wrote: > > Why is it "front mic"? Do you mean "correct mic"? > > In R58, chrome code will only handle the dual camera/dual mic case with only one > camera active at a time, which is the typical chrome book and camera app case. > For the developers, they can use audio extension api to set both mic to be > active if they need to do so for their app. > > For our use case I mentioned above, the camera stop, typically, user close the > camera app, we would switch back to internal mic and the front mic is the > default mic to set active. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:175: DeviceActivateType activate_by = > On 2017/03/01 11:25:55, shenghao wrote: > > s/activate_by/activated_by/ > > Done. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:177: > SwitchToDevice(*GetDeviceByType(AUDIO_TYPE_FRONT_MIC), true, activate_by); > On 2017/03/01 11:25:55, shenghao wrote: > > Why do we also switch to AUDIO_TYPE_FRONT_MIC? Is it possible to switch to > > AUDIO_TYPE_BACK_MIC? > > See comments above. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:177: > SwitchToDevice(*GetDeviceByType(AUDIO_TYPE_FRONT_MIC), true, activate_by); > On 2017/03/01 11:27:54, shenghao wrote: > > On 2017/03/01 11:25:55, shenghao wrote: > > > Why do we also switch to AUDIO_TYPE_FRONT_MIC? Is it possible to switch to > > > AUDIO_TYPE_BACK_MIC? > > > > typo: also => always > > Acknowledged. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:1234: if (IsFrontOrRearMic(top_device) && > HasDualInternalMic() && IsCameraOn()) { > On 2017/03/01 11:25:55, shenghao wrote: > > I think you can write: > > > > if (IsFrontOrRearMic(top_device)) { > > ActivateInternalMicForActiveCamera(); > > return; > > } > > The above code is not equivalent to the original lines. But I did simplify it by > calling ActivateInternalMicForActiveCamera() inside the if block. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:1504: // reaa microphone for rear camera. > On 2017/03/01 11:25:55, shenghao wrote: > > s/reaa/rear/ > > Done. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:1507: if (IsCameraOn()) { > On 2017/03/01 11:25:55, shenghao wrote: > > Why do we need to check IsCameraOn() twice here and line 1491? > > Removed IsCameraOn() from 1491. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:1522: break; > On 2017/03/01 11:25:55, shenghao wrote: > > You don't need "break" here because of the return statement. > > Done. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:1524: NOTREACHED(); > On 2017/03/01 11:25:55, shenghao wrote: > > Same as line 141 > > This code path is not reachable, the code path to call this function would check > and filter out the case that facing is not the first two cases. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:1530: for (AudioDeviceMap::const_iterator > it = audio_devices_.begin(); > On 2017/03/01 11:25:55, shenghao wrote: > > For all the "AudioDeviceMap::const_iterator", can we use "const auto" to > replace > > them? > > Done. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler.cc:1562: bool > CrasAudioHandler::HasExternalDevice(bool is_input) const { > On 2017/03/01 11:25:55, shenghao wrote: > > It seems that this method is only called in one place, and |is_input| is true. > > Can we remove the parameter? > > I would rather keep it just in case it could be used by output device in the > future. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > File chromeos/audio/cras_audio_handler_unittest.cc (right): > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler_unittest.cc:223: class FakeVideoCaptureMagner > { > On 2017/03/01 11:25:55, shenghao wrote: > > s/FakeVideoCaptureMagner/FakeVideoCaptureManager/ > > Done. > > https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... > chromeos/audio/cras_audio_handler_unittest.cc:3792: TEST_P(CrasAudioHandlerTest, > SwitchFrontRearCamera) { > On 2017/03/01 11:25:55, shenghao wrote: > > I think it's beneficial to add a test for the case that both front and rear > > camera are active. > > Added such a case at the end. lgtm with nit
https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:141: NOTREACHED(); On 2017/03/02 17:31:40, jennyz wrote: > On 2017/03/01 11:25:55, shenghao wrote: > > I am not 100% sure but I think it's possible to reach here. External camera > > should have facing=MEDIA_VIDEO_FACING_NONE. > > I need to know in which scenario it will pass MEDIA_VIDEO_FACING_NONE in under > dual internal mic mode, and how we are going to handle it? For now, I would just > log a warning and do nothing just in case it happens. Since external camera has MEDIA_VIDEO_FACING_NONE, I don't think we need to log warning for it. Only log warning for NUM_MEDIA_VIDEO_FACING_MODES case.
https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... File chromeos/audio/cras_audio_handler.cc (right): https://codereview.chromium.org/2721733003/diff/60001/chromeos/audio/cras_aud... chromeos/audio/cras_audio_handler.cc:141: NOTREACHED(); On 2017/03/03 03:45:21, shenghao wrote: > On 2017/03/02 17:31:40, jennyz wrote: > > On 2017/03/01 11:25:55, shenghao wrote: > > > I am not 100% sure but I think it's possible to reach here. External camera > > > should have facing=MEDIA_VIDEO_FACING_NONE. > > > > I need to know in which scenario it will pass MEDIA_VIDEO_FACING_NONE in under > > dual internal mic mode, and how we are going to handle it? For now, I would > just > > log a warning and do nothing just in case it happens. > > Since external camera has MEDIA_VIDEO_FACING_NONE, I don't think we need to log > warning for it. Only log warning for NUM_MEDIA_VIDEO_FACING_MODES case. Done.
The CQ bit was checked by jennyz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shenghao@chromium.org Link to the patchset: https://codereview.chromium.org/2721733003/#ps100001 (title: "Nit: log only the invalid camera facing mode value.")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=679061 TEST=See the feature spec document in the bug. ========== to ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=698327 TEST=See the feature spec document in the bug. ==========
hychao@, would you please take a look at this cl? shenghao already LGTMed it. But I guess he has not become a chromium reviewer yet. Please help review the cl. Thanks!
lgtm
Description was changed from ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=698327 TEST=See the feature spec document in the bug. ========== to ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=698327 TEST=See the feature spec document in the bug. TBR=dalecurtis@chromium.org ==========
The CQ bit was checked by jennyz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488824699434040, "parent_rev": "13934c3f62b6e223d085e10a3e2727c6701333e1", "commit_rev": "fbaa3e6092a9d8d08a91cd71fdd104dca8df2cfc"}
Message was sent while issue was closed.
Description was changed from ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=698327 TEST=See the feature spec document in the bug. TBR=dalecurtis@chromium.org ========== to ========== Handle the dual microphones and dual cameras cases. Activate the proper microphone when user activates the front or rear camera on basking devices. BUG=698327 TEST=See the feature spec document in the bug. TBR=dalecurtis@chromium.org Review-Url: https://codereview.chromium.org/2721733003 Cr-Commit-Position: refs/heads/master@{#454933} Committed: https://chromium.googlesource.com/chromium/src/+/fbaa3e6092a9d8d08a91cd71fdd1... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/fbaa3e6092a9d8d08a91cd71fdd1... |