Adds volume level measurements to the AudioInputController for low-latency clients

Adds volume level measurements to the AudioInputController for low-latency clients

BUG=
R=xians@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272961

[tommi (sloooow) - chröme, 2014-05-17 13:44:59]

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:33: // semantics.  This value was
arbitrarily chosen, but seems to work well.
maybe you can elaborate on how you tested this... seems to work well for...?
(would also be good to mention the platform since this might work fine on
desktop but not as well on mobile?)

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:34: const int
kPowerMeasurementTimeConstantMillis = 10;
nit: Milliseconds or Ms

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:37: const int
kPowerMonitorLogIntervalMilliSeconds = 500;
nit: Milliseconds

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:190: audio_bus_ =
AudioBus::Create(params);
how cpu intensive is it to add this?  This seems like a lot when all we want to
do is to detect signal absense - right?

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:408: audio_bus_->FromInterleaved(
safe to access these variables on a different thread without a lock? Maybe add
comments for how this is guaranteed to be safe.

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:417: static const float
kSilenceThresholdDBFS = -72.24719896f;
how does this translate to a data packet of all zeros vs a data packet that has
one non-zero frame with a very low value?
For how manu packets is the value calculated?

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:429: return;
nit: remove?

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.h:316: scoped_ptr<AudioPowerMonitor>
audio_level_;
Is computing the average energy levels vs the per packet energy levels the right
approach?
Will this for example report 0 (or the dbfs equivalent of no signal), for some
non-0 frames in a data packet?

I think this is probably the right way to measure audio levels, but I'm
wondering if we get false positives with this approach when we want to detect a
hardware muted mic or no signal at all.

[henrika (OOO until Aug 14), 2014-05-19 11:58:51]

Thanks Tommi.

Mainly comments so far. I have done a redesign where I move code into the power
monitor to ensure that I can reuse the new stuff inside both AIC and the native
parts.

I'll upload it and try to incorporate your review comments in that new change
instead.

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:33: // semantics.  This value was
arbitrarily chosen, but seems to work well.
It is a time constant taken from the initial implementation of the
AudioPowerMonitor and it gives a nice balance between how fast changes are
detected and how much the output fluctuates. There is no difference in this
behavior between desktop and mobile. It is the same value as is used for the
power measurements on the render side.

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:34: const int
kPowerMeasurementTimeConstantMillis = 10;
I had done modifications before this review was finalized. This is changed on
the new version.

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:37: const int
kPowerMonitorLogIntervalMilliSeconds = 500;
On 2014/05/17 13:45:00, tommi wrote:
> nit: Milliseconds

Done.

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:190: audio_bus_ =
AudioBus::Create(params);
The plan was to perform measurements at a very low rate, like once every other
second or so. Can't see that is could be an issue. On the output side, the same
is done for each audio frame (not on mobile).

Yes, one could come up with another type of detector which looks for signal
absence but given that we might pick up background noise etc. it feels more
efficient to use this type of level meter IMO. Then we can also detect states
like: "Look, there is a clear input signal (non-zero) but it does not reach the
other side, or the renderer does not get it".

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:408: audio_bus_->FromInterleaved(
I have rewritten this part in the latest version. Let me check again and come
back.

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:417: static const float
kSilenceThresholdDBFS = -72.24719896f;
Assume that you by packet mean all audio frames contained on one Ondata
callback.

We will not detect one single audio frame (4 bytes) being zero since the power
meter uses an exponential MA filter
(http://en.wikipedia.org/wiki/Exponential_smoothing). You need a longer sequence
of zeros to reset the filter. Takes a few milliseconds. If the signal drops from
max to zero, it should take about 10ms for the measurement to reflect it. But
again, we must see a sequence (full packet) of low values.

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:429: return;
has been modified.

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/287873004/diff/50001/media/audio/audio_input_...
media/audio/audio_input_controller.h:316: scoped_ptr<AudioPowerMonitor>
audio_level_;
I selected the existing AudioPowerMonitor since it is used in Chrome already and
seems to be well optimized for most platforms.

It takes some time for the monitor to generate the min value but we are only
talking about milliseconds and I can modify the time constant to use a smoothing
factor of almost 1 which is the same as not taking old samples into account.
E.g. y(n)=alpha*x(n) + (1-alpha)*y(n-1). Today alpha is about 0.02 (I think).

In any case, the tests I've done shows that the output results in "0" very
quickly when I mute a mic.

[henrika (OOO until Aug 14), 2014-05-19 15:02:02]

The idea is to start with logging in the native parts for Linux and Mac but the
CL only contains support for Linux as is. Give me one more round.

[no longer working on chromium, 2014-05-19 15:15:18]

On 2014/05/19 15:02:02, henrika wrote:
> The idea is to start with logging in the native parts for Linux and Mac but
the
> CL only contains support for Linux as is. Give me one more round.


why Linux? I would think Mac and/or ChromeOS instead, AFAIK we don't have mic
problem with pulse.

[no longer working on chromium, 2014-05-19 15:15:48]

I will take another look after you have a newer version.

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_input_...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:286: if (audio_level_)
skip if

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:401: if (!audio_level_)
can audio_level be NULL?

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_power_...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_power_...
media/audio/audio_power_monitor.cc:29: 1.0f - expf(-1.0f / (params.sample_rate()
* kTimeConstantInSec))),
indentation.

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_power_...
media/audio/audio_power_monitor.cc:78: void AudioPowerMonitor::Scan(const void*
data, uint32 size) {
Ideally we should not change this audio_power_monitor.cc, but change our input
code to work with AudioBus instead.

You'd better check with Dale if you want audio_power_minitor.cc to deal with
void*

[henrika (OOO until Aug 14), 2014-05-19 15:21:59]

I added Linux simply because it is a platform most of us use on a daily basis. I
can remove it if you like.

[henrika (OOO until Aug 14), 2014-05-19 15:25:46]

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_input_...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:286: if (audio_level_)
It can be NULL.

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:401: if (!audio_level_)
Yes, using new ctor. I can modify and create when needed instead.

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_power_...
File media/audio/audio_power_monitor.cc (right):

https://codereview.chromium.org/287873004/diff/70001/media/audio/audio_power_...
media/audio/audio_power_monitor.cc:78: void AudioPowerMonitor::Scan(const void*
data, uint32 size) {
It is a rather big change to do that for all platforms and I did not see it as
goal of this CL. I could add a TODO to remove this implementation once that has
been done. Sound OK?

[henrika (OOO until Aug 14), 2014-05-20 13:18:02]

xians@: done some improvements. Note that I still don't add any logs to the
native logs. Will do that as a final step when we have decided that the solution
is OK and what logging frequency to use.

[henrika (OOO until Aug 14), 2014-05-23 08:11:42]

Friendly ping xians@.

[no longer working on chromium, 2014-05-23 08:29:55]

Sorry for the delay.

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

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:57: static void AddPowerLevelToLog(float
level_dbfs) {
shouldn't you hook it up to the native log?

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:321: audio_level_->Reset();
I realized I was confused with Reset() and reset().

I think we should put a webrtc log here so that we know the stream is closed.

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:442: if (!audio_level_)
why this needs to be under the lock?

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:448: kPowerMonitorLogIntervalMilliseconds)
{
indentation

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:450: audio_bus_->FromInterleaved(
please add a todo here to remind switching to AudioBus for input and remove the
FromInterleaved here.

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:459: audio_level_->Reset();
I am not familiar with AudioPowerMonitor, do you know how this Reset affect the
smoothing factor ? for example, if the frame size in audio_bus is 3ms,
kPowerMeasurementTimeConstantMilliseconds is 10, but you Reset the audio level
after scanning one frame, does this still work?

[henrika (OOO until Aug 14), 2014-05-23 08:42:45]

Only comments for now. Will try on Mac next and come back with more feedback.
Thanks.

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

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:57: static void AddPowerLevelToLog(float
level_dbfs) {
Yes, just wanted OK about general stuff first. Will add that next round.

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:321: audio_level_->Reset();
Good point. I can do that.

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:442: if (!audio_level_)
The callback is called on a native audio thread and not the AM thread on which
the AudioPowerMonitor was created. OK?

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:448: kPowerMonitorLogIntervalMilliseconds)
{
On 2014/05/23 08:29:56, xians1 wrote:
> indentation

Done.

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:450: audio_bus_->FromInterleaved(
Will do.

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:459: audio_level_->Reset();
Yes it works. The smoothing factor controls how fast the filter will detect that
the signal changes but given that we only filter once every other second it
makes no sense to keep the old value in memory. 

Regarding 3ms, let me run some more tests on Mac and check. Let me think some
more on this.

[henrika (OOO until Aug 14), 2014-05-26 11:02:26]

xians@: PTAL

Example of output where I mute the mic twice:

[23327:23346:0526/124701:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-43.06 dBFS
[23327:23346:0526/124707:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-inf dBFS <=> no audio input!
[23327:23346:0526/124713:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-inf dBFS <=> no audio input!
[23327:23346:0526/124719:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-inf dBFS <=> no audio input!
[23327:23346:0526/124725:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-43.66 dBFS
[23327:23346:0526/124731:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-20.88 dBFS
[23327:23346:0526/124737:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-20.17 dBFS
[23327:23346:0526/124743:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-52.11 dBFS
[23327:23346:0526/124749:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-inf dBFS <=> no audio input!
[23327:23346:0526/124755:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-inf dBFS <=> no audio input!
[23327:23346:0526/124801:VERBOSE1:audio_input_controller.cc(450)] AIC::OnData:
average audio level=-53.69 dBFS
[23327:23345:0526/124806:VERBOSE1:audio_input_controller.cc(306)] AIC::DoClose:
closing input stream

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

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:57: static void AddPowerLevelToLog(float
level_dbfs) {
I removed this part and log in dBFS instead. Easier to compare with log in WAC
(in render process) then.

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:321: audio_level_->Reset();
On 2014/05/23 08:42:45, henrika wrote:
> Good point. I can do that.

Done.

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:448: kPowerMonitorLogIntervalMilliseconds)
{
Actually; this is what I get when using git cl format.

[henrika (OOO until Aug 14), 2014-05-26 11:11:01]

I have also verified that the new log turns up in chrome://webrtc-logs/ during
after making a Hangout.

[no longer working on chromium, 2014-05-26 14:20:15]

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

https://codereview.chromium.org/287873004/diff/100001/media/audio/audio_input...
media/audio/audio_input_controller.cc:442: if (!audio_level_)
On 2014/05/23 08:42:45, henrika wrote:
> The callback is called on a native audio thread and not the AM thread on which
> the AudioPowerMonitor was created. OK?

not sure I follow here. In DoCreate(), the creation of audio_level_ is not under
the lock, so this lock protects nothing here.

Also, audio_level_ is created before the stream is create, and OnData() is
triggered when the stream is active. And audio_level_ is deleted before AIC goes
away. theoretically audio_level_ should be always valid here.

Or do I miss something?

https://codereview.chromium.org/287873004/diff/120001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/287873004/diff/120001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:223: return;
In the AIC, you log when the stream is closed, so if you return here, it will
fail your intention.

Probably you do something like this:
std::string log_string = entry ?
base::StringPrintf("[stream_id=%d] ", entry->stream_id) :
"No corresponding stream"
log_string += message;
MediaStreamManager::SendMessageToNativeLog(log_string);

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

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
media/audio/audio_input_controller.cc:304: std::string log_string("AIC::DoClose:
closing input stream");
nit, const

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
media/audio/audio_input_controller.cc:476: std::pair<float, bool> result =
audio_level_->ReadCurrentPowerAndClip();
this introduces a race here. audio_level_ is changed/accessed by both the
real-time audio thread and task_runner_ thread.

Instead, you should do all the calculation in the real-time audio thread and
just and post the values to task_runner_ thread.

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
media/audio/audio_input_controller.h:323: #if defined(AUDIO_POWER_MONITORING)
I don't think you save much by excluding these members from those platfroms, I
suggest removing the AUDIO_POWER_MONITORING here and move the definition of
AUDIO_POWER_MONITORING to the implementation instead

[henrika (OOO until Aug 14), 2014-05-26 15:01:17]

xians@: one more time, please ;-)

https://codereview.chromium.org/287873004/diff/120001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/287873004/diff/120001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_renderer_host.cc:223: return;
As discussed offline; will remove close logging in this CL.

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

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
media/audio/audio_input_controller.cc:304: std::string log_string("AIC::DoClose:
closing input stream");
Removed.

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
media/audio/audio_input_controller.cc:476: std::pair<float, bool> result =
audio_level_->ReadCurrentPowerAndClip();
Will fix.

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
media/audio/audio_input_controller.h:323: #if defined(AUDIO_POWER_MONITORING)
I copied how it was done on the output side (in the AOC) to make it symmetric.
OK?

[henrika (OOO until Aug 14), 2014-05-27 07:46:35]

xians@: PTAL

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
media/audio/audio_input_controller.h:323: #if defined(AUDIO_POWER_MONITORING)
In addition, we should really do our best to simplify as much as possible for
mobile. Every byte counts.

[no longer working on chromium, 2014-05-27 09:06:09]

On 2014/05/27 07:46:35, henrika wrote:
> xians@: PTAL
> 
>
https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
> File media/audio/audio_input_controller.h (right):
> 
>
https://codereview.chromium.org/287873004/diff/120001/media/audio/audio_input...
> media/audio/audio_input_controller.h:323: #if defined(AUDIO_POWER_MONITORING)
> In addition, we should really do our best to simplify as much as possible for
> mobile. Every byte counts.

lgtm after addressing the offline comments.

[henrika (OOO until Aug 14), 2014-05-27 09:16:15]

The CQ bit was checked by henrika@chromium.org

[commit-bot: I haz the power, 2014-05-27 09:16:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/henrika@chromium.org/287873004/240001

[commit-bot: I haz the power, 2014-05-27 12:35:19]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[henrika (OOO until Aug 14), 2014-05-27 13:25:15]

Committed patchset #12 manually as r272961 (presubmit successful).