Implements the GetSignalLevel and GetStats interface for the local audio track.

Implements the GetSignalLevel and GetStats interface for the local audio track.
From this patch, libjingle is able to get the signal level and stats from the local audio track in chrome.

Note, that this patch works with and without kEnableAudioTrackProcessing. When kEnableAudioTrackProcessing is set, libjingle gets values from the chromium track, otherwise it will continue to use the values got from webrtc.

After https://codereview.chromium.org/178223013/ is landed, this CL will rebase and implement the GetSignalLevel() correctly.

BUG=334273
TEST=content_unittests, nothing breaks;

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

[no longer working on chromium, 2014-03-03 11:51:09]

Hi, please review this CL.

Andrew, please take a look at
media_stream_audio_processor_options.cc::GetAecStats(), but you are welcomed to
review whatever you want.
Tommi, the overall design.

SX

[tommi (sloooow) - chröme, 2014-03-03 15:25:18]

lgtm % nits

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_options.cc (right):

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:185:
stats->echo_return_loss = -100;
Where is it documented that these are the lowest possible values?
Would it make sense to have constants defined somewhere for them?

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:93:
base::AutoLock auto_lock(lock_);
GetAudioProcessor should really return a scoped_refptr<
webrtc::AudioProcessorInterface> since there exist multi threaded
implementations.  As is, this is racy.  I thought we discussed why before but it
seems to have dropped off the table so we should fix it.

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:110: // if the |processor|
does not have the audio processing, which can happen if
does not have audio processing

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:111: // the
kEnableAudioTrackProcessing is not set or all the constraints in
s/the kEnableAudioTrackProcessing/kEnableAudioTrackProcessing

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:112: // the |processor| are
turned off. In such case, we pass a NULL to the
In such a case, we pass NULL to the

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:113: // adapter to indicate
that no stats can be got from the processor.
can be gotten

[ajm, 2014-03-04 02:23:11]

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_options.cc (right):

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:181: void
GetAecStats(AudioProcessing* audio_processing,
How often is this method polled? Calling GetMetrics() averages over the window
since the last call to it and resets the metrics.

You don't want to poll this often and report at a much lower frequency. Ideally
the polling and reporting would have the same frequency. (Though unfortunately
that's not the way it works in libjingle at the moment. It's polled at a 1
second interval and reported at a 10 second. Would be great to improve :)

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:185:
stats->echo_return_loss = -100;
On 2014/03/03 15:25:18, tommi wrote:
> Where is it documented that these are the lowest possible values?

I had a look and unfortunately it's not documented anywhere, but I can confirm
this is indeed the case.

> Would it make sense to have constants defined somewhere for them?

[no longer working on chromium, 2014-03-04 15:48:59]

Hi Tommi and Andrew, I have addressed your comments, PTAL.

SX

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_processor_options.cc (right):

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:181: void
GetAecStats(AudioProcessing* audio_processing,
On 2014/03/04 02:23:12, ajm wrote:
> How often is this method polled? Calling GetMetrics() averages over the window
> since the last call to it and resets the metrics.
> 
> You don't want to poll this often and report at a much lower frequency.
Ideally
> the polling and reporting would have the same frequency. (Though unfortunately
> that's not the way it works in libjingle at the moment. It's polled at a 1
> second interval and reported at a 10 second. Would be great to improve :)

Definitely.

The method is triggered by StatsCollector::ExtractVoiceInfo, so it is the same
frequency as StatsCollector::GetStats().

I measured the frequency using ML, it looks like libjingle is polling every
200ms instead of every 1 second. I am not sure if something has been changed or
it is the way for a long time.
There might be ways to optimize this GetStats() call in libjingle, but there is
nothing we can do here in chrome.

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/m...
content/renderer/media/media_stream_audio_processor_options.cc:185:
stats->echo_return_loss = -100;
On 2014/03/04 02:23:12, ajm wrote:
> On 2014/03/03 15:25:18, tommi wrote:
> > Where is it documented that these are the lowest possible values?
> 
> I had a look and unfortunately it's not documented anywhere, but I can confirm
> this is indeed the case.
> 
> > Would it make sense to have constants defined somewhere for them?
> 

Thanks.

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc (right):

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:93:
base::AutoLock auto_lock(lock_);
On 2014/03/03 15:25:18, tommi wrote:
> GetAudioProcessor should really return a scoped_refptr<
> webrtc::AudioProcessorInterface> since there exist multi threaded
> implementations.  As is, this is racy.  I thought we discussed why before but
it
> seems to have dropped off the table so we should fix it.

Done.

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:110: // if the |processor|
does not have the audio processing, which can happen if
On 2014/03/03 15:25:18, tommi wrote:
> does not have audio processing

Done.

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:111: // the
kEnableAudioTrackProcessing is not set or all the constraints in
On 2014/03/03 15:25:18, tommi wrote:
> s/the kEnableAudioTrackProcessing/kEnableAudioTrackProcessing

Done.

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:112: // the |processor| are
turned off. In such case, we pass a NULL to the
On 2014/03/03 15:25:18, tommi wrote:
> In such a case, we pass NULL to the

Done.

https://codereview.chromium.org/185413009/diff/20001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:113: // adapter to indicate
that no stats can be got from the processor.
On 2014/03/03 15:25:18, tommi wrote:
> can be gotten

Done.

[ajm, 2014-03-04 17:26:15]

lgtm on GetAecStats given that you can't do anything about the polling frequency
here.

[tommi (sloooow) - chröme, 2014-03-04 17:33:53]

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h (right):

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:59:
talk_base::scoped_refptr<webrtc::AudioProcessorInterface> processor);
do we need to use talk_base::scoped_refptr here?
I'd prefer to use chrome types wherever possible and reduce usage of third_party
types to only where we absolutely need it.

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:89:
talk_base::scoped_refptr<webrtc::AudioProcessorInterface> audio_processor_;
scoped_refptr here as well

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:126:
adapter_->SetAudioProcessor(NULL);
nit: for fewer call sites and less code:
adapter_->SetAudioProcessor(
    processor->has_audio_processing() ? processor.get() : NULL);

also, if you change SetAudioProcessor to use chrome's scoped_refptr, you won't
need the call to .get().

[no longer working on chromium, 2014-03-04 18:54:06]

Tommi, ptal.

SX

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h (right):

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:59:
talk_base::scoped_refptr<webrtc::AudioProcessorInterface> processor);
On 2014/03/04 17:33:54, tommi wrote:
> do we need to use talk_base::scoped_refptr here?
> I'd prefer to use chrome types wherever possible and reduce usage of
third_party
> types to only where we absolutely need it.

Done.

Actually it is not a problem since adapter is designed to work with the
libjingle interface.

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:89:
talk_base::scoped_refptr<webrtc::AudioProcessorInterface> audio_processor_;
On 2014/03/04 17:33:54, tommi wrote:
> scoped_refptr here as well

Done.

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/185413009/diff/40001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.cc:126:
adapter_->SetAudioProcessor(NULL);
On 2014/03/04 17:33:54, tommi wrote:
> nit: for fewer call sites and less code:
> adapter_->SetAudioProcessor(
>     processor->has_audio_processing() ? processor.get() : NULL);
> 
> also, if you change SetAudioProcessor to use chrome's scoped_refptr, you won't
> need the call to .get().

Done.

[tommi (sloooow) - chröme, 2014-03-04 19:26:13]

lgtm

[no longer working on chromium, 2014-03-06 10:09:28]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-06 10:09:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/185413009/70001

[commit-bot: I haz the power, 2014-03-06 14:39:10]

Change committed as 255351