Add support for audio input mute detection on all platforms

Add support for audio input mute detection on all platforms.

TBR=rkc@chromium.org
BUG=422275
TEST=Manual tests using DVLOGs to confirm valid microphone mute states.

Committed: https://crrev.com/c569d98674f56fccf39c53f7ec7a63127755390f
Cr-Commit-Position: refs/heads/master@{#299891}

[henrika (OOO until Aug 14), 2014-10-10 13:46:51]

henrika@chromium.org changed reviewers:
+ tommi@chromium.org

[henrika (OOO until Aug 14), 2014-10-10 13:47:15]

Added new API and implementation for Windows.

[henrika (OOO until Aug 14), 2014-10-10 13:49:43]

FYI, will add Mac and Linux next.

[tommi (sloooow) - chröme, 2014-10-10 15:54:36]

can you ping me when the cl is ready to be submitted?

https://codereview.chromium.org/645923002/diff/1/media/audio/audio_io.h
File media/audio/audio_io.h (right):

https://codereview.chromium.org/645923002/diff/1/media/audio/audio_io.h#newco...
media/audio/audio_io.h:169: virtual bool IsMuted() { return false; };
will this be made pure virtual in the next patch set (per offline discussion)?

https://codereview.chromium.org/645923002/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/645923002/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:279: return (is_muted == TRUE) ?
true : false;
checking for TRUE always makes me nervous on windows. Who knows what the
implementation might do :)  S_FALSE is 1, TRUE is 1... are you relying on a
bitwise operator!() (i.e. complement) - if so then !FALSE is not equal to
TRUE... VARIANT_TRUE is furthermore defined as -1, which actually is a
complement of 0 (although VARIANT_BOOL is 16 bit... sigh)...  Anyway, it's a
mess. (also see
http://blogs.msdn.com/b/oldnewthing/archive/2004/12/22/329884.aspx)

So, for my piece of mind, please just do:

return is_muted != FALSE;

[henrika (OOO until Aug 14), 2014-10-13 12:27:17]

Comments only. Will add support for Linux before uploading next.

https://codereview.chromium.org/645923002/diff/1/media/audio/audio_io.h
File media/audio/audio_io.h (right):

https://codereview.chromium.org/645923002/diff/1/media/audio/audio_io.h#newco...
media/audio/audio_io.h:169: virtual bool IsMuted() { return false; };
Yes it will.

https://codereview.chromium.org/645923002/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/645923002/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:279: return (is_muted == TRUE) ?
true : false;
Thx. Actually checked the exact same link before making my first patch. Using
your style now.

[henrika (OOO until Aug 14), 2014-10-14 11:13:00]

henrika@chromium.org changed reviewers:
+ xians@chromium.org

[henrika (OOO until Aug 14), 2014-10-14 11:13:25]

+xians for Linux PulseAudio implementation.

[henrika (OOO until Aug 14), 2014-10-14 11:21:10]

henrika@chromium.org changed reviewers:
+ dgreid@chromium.org

[henrika (OOO until Aug 14), 2014-10-14 11:22:02]

+dgreid for trivial cras_input implementation

[no longer working on chromium, 2014-10-14 11:45:42]

lgtm iif you address the comment.

https://codereview.chromium.org/645923002/diff/160001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_input.cc (right):

https://codereview.chromium.org/645923002/diff/160001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:205: AutoPulseLock auto_lock(pa_mainloop_);
please extract these lines of code into a helper method which takes a callback
as input, so GetVolume and IsMuted() will call the same method.

[dgreid, 2014-10-14 12:17:26]

cras_input lgtm.

ChromeOS doesn't mute the input.

[henrika (OOO until Aug 14), 2014-10-14 14:21:13]

https://codereview.chromium.org/645923002/diff/160001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_input.cc (right):

https://codereview.chromium.org/645923002/diff/160001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:205: AutoPulseLock auto_lock(pa_mainloop_);
Done.

[no longer working on chromium, 2014-10-14 14:23:29]

lgtm

https://codereview.chromium.org/645923002/diff/180001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_input.h (right):

https://codereview.chromium.org/645923002/diff/180001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.h:49: int error, void* user_data);
nit, indentation.

[henrika (OOO until Aug 14), 2014-10-14 14:25:54]

Thanks. Will do Mac next.

[henrika (OOO until Aug 14), 2014-10-15 11:52:09]

Tommi, please check. Now added Mac support as well. Works for stereo mic.

Tommi: not sure if you want me to add usage of the new API in this CL as well.
That is: should I remove the existing "mute check" and replace with new? Or
should this be a isolated "API fix only"? I vote for adding more to make merging
less difficult (if needed).

[henrika (OOO until Aug 14), 2014-10-15 13:04:30]

Tommi, as discussed off-line: also added usage of the new API in
media/audio/audio_input_controller.cc. Goal is to (1) use correct IsMuted
implementation on all platforms, and (2) avoid logging audio levels if we know
that the mic is muted.

[tommi (sloooow) - chröme, 2014-10-15 17:00:48]

looks pretty good :) got some nits and double checks

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:586: std::string log_string;
nit: move this down to where you need it (where print the audio levels)

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:591: if (microphone_is_muted) {
nit: you don't really need this variable.

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:593: log_string = base::StringPrintf(
no need for StringPrintf

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:596: // Return early if microphone is
muted. No idea adding logs and UMA stats
nit: "No idea" -> "No need to" (etc)

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:599: } else {
nit: no need for else since there's an early return above

https://codereview.chromium.org/645923002/diff/320001/media/audio/mac/audio_l...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/645923002/diff/320001/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_input_mac.cc:457: NOTREACHED() << "Device ID
is unknown";
should this be a DCHECK then?

DCHECK_NE(input_device_id_, kAudioObjectUnknown)

https://codereview.chromium.org/645923002/diff/320001/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_input_mac.cc:462: kAudioDevicePropertyMute,
nit: indent by two spaces and put the closing brace on a line of its own (see
elsewhere in this file)

https://codereview.chromium.org/645923002/diff/320001/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_input_mac.cc:467: NOTREACHED() << "Device does
not support checking master mute state";
since NOTREACHED is essentially the same as DCHECK, I'm wondering if this should
perhaps rather be a DLOG(ERROR)?

https://codereview.chromium.org/645923002/diff/320001/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_input_mac.cc:476: return
static_cast<bool>(muted);
nit: return muted != 0u;

actually, you could also just do:

DLOG_IF(WARNING, result != noErr) << "Failed to get mute state";
return result == noErr && muted != 0;

https://codereview.chromium.org/645923002/diff/320001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_input.cc (right):

https://codereview.chromium.org/645923002/diff/320001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:236: void
PulseAudioInputStream::MuteCallback(pa_context* context,
on which thread does this callback run?  Is it safe to touch muted_ here?
If we can add a thread check, that would be good.

https://codereview.chromium.org/645923002/diff/320001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:244:
pa_threaded_mainloop_signal(stream->pa_mainloop_, 0);
Can you document why we need to call this here and why we only call it if
there's an error?

I'm a bit confused why StreamNotifyCallback always calls it and VolumeCallback
doesn't.

https://codereview.chromium.org/645923002/diff/320001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:248: stream->muted_ =
static_cast<bool>(info->mute);
stream->muted_ = info->mute != 0;

[henrika (OOO until Aug 14), 2014-10-16 09:58:00]

Thanks Tommi.

PTAL

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:586: std::string log_string;
On 2014/10/15 17:00:48, tommi wrote:
> nit: move this down to where you need it (where print the audio levels)

Done.

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:591: if (microphone_is_muted) {
On 2014/10/15 17:00:48, tommi wrote:
> nit: you don't really need this variable.

Done.

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:593: log_string = base::StringPrintf(
On 2014/10/15 17:00:48, tommi wrote:
> no need for StringPrintf

Done.

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:596: // Return early if microphone is
muted. No idea adding logs and UMA stats
On 2014/10/15 17:00:48, tommi wrote:
> nit: "No idea" -> "No need to" (etc)

Done.

https://codereview.chromium.org/645923002/diff/320001/media/audio/audio_input...
media/audio/audio_input_controller.cc:599: } else {
On 2014/10/15 17:00:48, tommi wrote:
> nit: no need for else since there's an early return above

Done.

https://codereview.chromium.org/645923002/diff/320001/media/audio/mac/audio_l...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/645923002/diff/320001/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_input_mac.cc:462: kAudioDevicePropertyMute,
On 2014/10/15 17:00:48, tommi wrote:
> nit: indent by two spaces and put the closing brace on a line of its own (see
> elsewhere in this file)

Done.

https://codereview.chromium.org/645923002/diff/320001/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_input_mac.cc:467: NOTREACHED() << "Device does
not support checking master mute state";
On 2014/10/15 17:00:48, tommi wrote:
> since NOTREACHED is essentially the same as DCHECK, I'm wondering if this
should
> perhaps rather be a DLOG(ERROR)?

Done.

https://codereview.chromium.org/645923002/diff/320001/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_input_mac.cc:476: return
static_cast<bool>(muted);
On 2014/10/15 17:00:48, tommi wrote:
> nit: return muted != 0u;
> 
> actually, you could also just do:
> 
> DLOG_IF(WARNING, result != noErr) << "Failed to get mute state";
> return result == noErr && muted != 0;

Done.

https://codereview.chromium.org/645923002/diff/320001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_input.cc (right):

https://codereview.chromium.org/645923002/diff/320001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:236: void
PulseAudioInputStream::MuteCallback(pa_context* context,
Based on off-line discussion. Will add comment.

https://codereview.chromium.org/645923002/diff/320001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:236: void
PulseAudioInputStream::MuteCallback(pa_context* context,
On 2014/10/15 17:00:48, tommi wrote:
> on which thread does this callback run?  Is it safe to touch muted_ here?
> If we can add a thread check, that would be good.

Done.

https://codereview.chromium.org/645923002/diff/320001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:244:
pa_threaded_mainloop_signal(stream->pa_mainloop_, 0);
Discussed off-line.

https://codereview.chromium.org/645923002/diff/320001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:248: stream->muted_ =
static_cast<bool>(info->mute);
On 2014/10/15 17:00:48, tommi wrote:
> stream->muted_ = info->mute != 0;

Done.

[tommi (sloooow) - chröme, 2014-10-16 12:36:54]

lgtm

https://codereview.chromium.org/645923002/diff/340001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_input.cc (right):

https://codereview.chromium.org/645923002/diff/340001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:240: 
nit: remove empty line

[henrika (OOO until Aug 14), 2014-10-16 13:04:19]

Done.

https://codereview.chromium.org/645923002/diff/340001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_input.cc (right):

https://codereview.chromium.org/645923002/diff/340001/media/audio/pulse/pulse...
media/audio/pulse/pulse_input.cc:240: 
On 2014/10/16 12:36:53, tommi wrote:
> nit: remove empty line

Done.

[henrika (OOO until Aug 14), 2014-10-16 13:04:30]

The CQ bit was checked by henrika@chromium.org

[commit-bot: I haz the power, 2014-10-16 13:05:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645923002/360001

[commit-bot: I haz the power, 2014-10-16 13:14:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-16 13:14:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[henrika (OOO until Aug 14), 2014-10-16 13:46:39]

Adding rkc@ as TBR due to:

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    components/copresence/mediums/audio/audio_recorder_unittest.cc

This CL only does a trivial fix in audio_recorder_unittest.cc.

Hope TBR is OK.

[henrika (OOO until Aug 14), 2014-10-16 13:46:55]

The CQ bit was checked by henrika@chromium.org

[commit-bot: I haz the power, 2014-10-16 13:48:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645923002/360001

[commit-bot: I haz the power, 2014-10-16 14:44:25]

Committed patchset #14 (id:360001)

[commit-bot: I haz the power, 2014-10-16 14:45:13]

Patchset 14 (id:??) landed as
https://crrev.com/c569d98674f56fccf39c53f7ec7a63127755390f
Cr-Commit-Position: refs/heads/master@{#299891}