Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
582733002:
Adds a platform independent detecion of microphone hardware mute (Closed)
|
Created:
6 years, 3 months ago by henrika (OOO until Aug 14) Modified:
6 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdds a platform independent detection of microphone hardware mute.
This change ensures that a WebRTC client can detect (and log) if a user has enabled the microphone mute. No platform-dependent code is involved.
BUG=NONE
TEST=Manual tests of WebRTC client on Linux, Mac and Windows.
Committed: https://crrev.com/c24f2b417072000af2c8f0676a7cc0194bd8ff7a
Cr-Commit-Position: refs/heads/master@{#296164}
Patch Set 1 #Patch Set 2 : Adds a platform independent detecion of microphone hardware mute #
Total comments: 6
Patch Set 3 : tommi@ #
Total comments: 2
Patch Set 4 : nit #
Total comments: 5
Patch Set 5 : Now builds on Android #Messages
Total messages: 19 (4 generated)
henrika@chromium.org changed reviewers: + tommi@chromium.org, xians@chromium.org
Will add XML part when/if OK from other reviewers.
https://codereview.chromium.org/582733002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/582733002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:26: MICROPHONE_MUTE_MAX = MICROPHONE_IS_NOT_MUTED hmm... I think this should be MICROPHONE_IS_MUTED as is. (max is actually 0) However, do you mind switching this so that "IS_MUTED" comes first? basically: IS_MUTED = 0, IS_NOT_MUTED = 1, MUTE_MAX = IS_NOT_MUTED https://codereview.chromium.org/582733002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:597: // button in audio settings for the selected microphone. The idea here is to do you know if this is the equivalent of a hardware mute or if this is implemented in software (possibly in kernel mode)? The reason I'm wondering about this is because of microphones that actually have a physical on/off button which the user can play around with which does not affect the software state. I think that the physical button scenario is what hw mute detection was meant to detect (i.e. not related to audio settings). Because of this, I think that the practical difference between software mute and hardware mute could be that the former will deliver all zeros when muted, but the hw solution might actually deliver non-zero samples when muted. https://codereview.chromium.org/582733002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:604: "AIC::OnData: microphone is most likely hardware muted!"); nit: "microphone muted" is probably enough
https://codereview.chromium.org/582733002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/582733002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:26: MICROPHONE_MUTE_MAX = MICROPHONE_IS_NOT_MUTED On 2014/09/19 13:09:32, tommi wrote: > hmm... I think this should be MICROPHONE_IS_MUTED as is. (max is actually 0) > > However, do you mind switching this so that "IS_MUTED" comes first? > basically: > > IS_MUTED = 0, > IS_NOT_MUTED = 1, > MUTE_MAX = IS_NOT_MUTED Done. https://codereview.chromium.org/582733002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:597: // button in audio settings for the selected microphone. The idea here is to I tried one USB headset with mute button and when activating, the mute check box was not marked. Audio was still recorded at a reasonable level but it was silent in loopback. I was not able to restore audio again so something is broken with this headset. Will try on other platforms next. https://codereview.chromium.org/582733002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:604: "AIC::OnData: microphone is most likely hardware muted!"); On 2014/09/19 13:09:32, tommi wrote: > nit: "microphone muted" is probably enough > Done.
https://codereview.chromium.org/582733002/diff/40001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/582733002/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:604: "AIC::OnData: microphone is muted!"); no need for StringPrintf
Tommi, if I may suggest; would it be OK to land as is to start getting some data? I don't think that we will ever be able to come up with a perfect solution for all headsets and combinations of how a hardware-mute button on the headset works. Also, we will most likely not be able to find any OS specific APIs for the hardware button mute either. IMO, the existing patch will work well for one essential "mute case" and it might work for some hardware-mute cases as well. https://codereview.chromium.org/582733002/diff/40001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/582733002/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:604: "AIC::OnData: microphone is muted!"); LOL, thx.
https://codereview.chromium.org/582733002/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/582733002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:26: MICROPHONE_MUTE_MAX = MICROPHONE_IS_NOT_MUTED If you change MICROPHONE_MUTE_MAX = MICROPHONE_IS_NOT_MUTED + 1, then you can just use MICROPHONE_MUTE_MAX in LogMicrophoneMuteResult https://codereview.chromium.org/582733002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:30: UMA_HISTOGRAM_ENUMERATION("Media.MicrophoneMuted", what is the follow-up after we get the muted or unmuted uma stats?
https://codereview.chromium.org/582733002/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/582733002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:26: MICROPHONE_MUTE_MAX = MICROPHONE_IS_NOT_MUTED Found this style in other places in Chrome. Feels more intuitive to specify max as the actual max and add 1 below. Let me know if you feel strongly about it. https://codereview.chromium.org/582733002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:30: UMA_HISTOGRAM_ENUMERATION("Media.MicrophoneMuted", The idea is that we shall learn how many users actually do mute their microphone this way. It adds one more piece to the big puzzle.
lgtm
Thanks Tommi! Shijing, do I hear an OK from you as well?
lgtm https://codereview.chromium.org/582733002/diff/60001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/582733002/diff/60001/media/audio/audio_input_... media/audio/audio_input_controller.cc:26: MICROPHONE_MUTE_MAX = MICROPHONE_IS_NOT_MUTED On 2014/09/22 11:24:13, henrika wrote: > Found this style in other places in Chrome. Feels more intuitive to specify max > as the actual max and add 1 below. Let me know if you feel strongly about it. I think the most common style is enum MicrophoneMuteResult { MICROPHONE_IS_MUTED = 0, MICROPHONE_IS_NOT_MUTED = 1, MICROPHONE_MUTE_MAX = 2 } But this is just nit, feel free to keep what it is.
The CQ bit was checked by henrika@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582733002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by henrika@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/582733002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 7e742bb995729257ccd1c6e543b7a60606592b94
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c24f2b417072000af2c8f0676a7cc0194bd8ff7a Cr-Commit-Position: refs/heads/master@{#296164} |
