Calculate the signal level on the media stream local audio track

Calculate the signal level on the media stream local audio track. And the signal level will be pass to libjingle after the new libjingle interface is implemented by chrome.


BUG=264611
TEST=content_unittests, nothing breaks.
R=tommi@chromium.org

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

[no longer working on chromium, 2014-02-26 15:12:07]

Hi Tommi,

Please review this CL.

FYI, the MediaStreamAudioLevelCalculator has to keep using the int16* instead of
AudioBus* since we will keep using int16* until we can completely get rid of the
flag.

SX

[tommi (sloooow) - chröme, 2014-02-27 08:39:43]

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.cc (right):

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.cc:14: static const
int kPermutation[33] =
Add documentation. Also document why the size is 33.

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.cc:16: static const
int kUpdateFrequency = 10;
documentation.  Also, since this is only used in one function, we could define
the constant there.  Same for kPermutation.

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.cc:17: static const
int k16BitsMaxValue = 32767;
do we need this constant?

Could we instead just use std::numeric_limits<int16>::max() below where
k16BitsMaxValue is used?

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.cc:44:
DCHECK(CalledOnValidThread());
nice

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.h:25: int
Calculate(const int16* audio_data, int number_of_channels,
would it make sense to add another Calculate method that accepts an AudioBus
instead of these parameters and have a TODO to remove this one?  That way, we
can use that method straight away whenever we have an AudioBus instance (e.g.
while behind the flag) and we don't have to go and update that code once we get
rid of the int16 assumption.

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

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:51: void
WebRtcLocalAudioTrackAdapter::SetSignalLevel(int signal_level) {
document on which thread this is called?

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

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:72: int
signal_level_;
As is, this variable is never used.  Can we skip adding it in this cl?

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.h:122:
scoped_ptr<MediaStreamAudioLevelCalculator> level_calculator_;
what about just making this a non pointer?
MediaStreamAudioLevelCalculator level_calculator_;

[no longer working on chromium, 2014-02-28 08:16:25]

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.cc (right):

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.cc:14: static const
int kPermutation[33] =
On 2014/02/27 08:39:43, tommi wrote:
> Add documentation. Also document why the size is 33.

Done.

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.cc:16: static const
int kUpdateFrequency = 10;
On 2014/02/27 08:39:43, tommi wrote:
> documentation.  Also, since this is only used in one function, we could define
> the constant there.  Same for kPermutation.

Done.

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.cc:17: static const
int k16BitsMaxValue = 32767;
On 2014/02/27 08:39:43, tommi wrote:
> do we need this constant?
> 
> Could we instead just use std::numeric_limits<int16>::max() below where
> k16BitsMaxValue is used?

I used it because that is how webrtc code was written, but taking another look
at the problem, if the data type is int16, probably the maximum value will never
be bigger than std::numeric_limits<int16>::max().
So I added a DCHECK here, and will run some tests to verify it.

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.h:25: int
Calculate(const int16* audio_data, int number_of_channels,
On 2014/02/27 08:39:43, tommi wrote:
> would it make sense to add another Calculate method that accepts an AudioBus
> instead of these parameters and have a TODO to remove this one?  That way, we
> can use that method straight away whenever we have an AudioBus instance (e.g.
> while behind the flag) and we don't have to go and update that code once we
get
> rid of the int16 assumption.

We may have to keep using const int16* until we can completely remove the flag.
That is because adding a callback using AudioBus* will touch all the code from
source, track, then down to the sinks. We (WebRtc) can live with it, but the
sink clients probably don't want a un-used overload method.

My plan was to switch const int16* to AudioBus* when we are removing the flag,
and do the change in all the layers at the same time. But I am fine with adding
a Calculate(AudioBus*) here if you strongly want it.

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

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.cc:51: void
WebRtcLocalAudioTrackAdapter::SetSignalLevel(int signal_level) {
On 2014/02/27 08:39:43, tommi wrote:
> document on which thread this is called?

Done with adding a comment in the header file.

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

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/w...
content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h:72: int
signal_level_;
On 2014/02/27 08:39:43, tommi wrote:
> As is, this variable is never used.  Can we skip adding it in this cl?

Done.

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_track.h (right):

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_track.h:122:
scoped_ptr<MediaStreamAudioLevelCalculator> level_calculator_;
On 2014/02/27 08:39:43, tommi wrote:
> what about just making this a non pointer?
> MediaStreamAudioLevelCalculator level_calculator_;

No, we can't. In WebRtcAudioCapturer, we will re-create a new source if the
format changes, that means we will have new audio thread. And
MediaStreamAudioLevelCalculator is a non-thread safe object, so we need to
create a new object each time the format changes.

[no longer working on chromium, 2014-03-01 11:34:28]

PTAL, thanks.

SX

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.cc (right):

https://codereview.chromium.org/178223013/diff/70001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.cc:17: static const
int k16BitsMaxValue = 32767;
On 2014/02/27 08:39:43, tommi wrote:
> do we need this constant?
> 
> Could we instead just use std::numeric_limits<int16>::max() below where
> k16BitsMaxValue is used?

AudioBus::ToInterleave guarantees the value in the range of [-32768, 32767], so
the absolute amplitude should not be bigger than 32768, I added a check instead.

https://codereview.chromium.org/178223013/diff/90001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/178223013/diff/90001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.h:17: class
MediaStreamAudioLevelCalculator : public base::NonThreadSafe {
I can't use the NonThreadSafe here since the object will be deleted in the new
capture thread. So I simply add a thread check to make sure Calculate() is
always called on the same thread.

[tommi (sloooow) - chröme, 2014-03-01 11:49:33]

https://codereview.chromium.org/178223013/diff/90001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/178223013/diff/90001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.h:17: class
MediaStreamAudioLevelCalculator : public base::NonThreadSafe {
On 2014/03/01 11:34:28, xians1 wrote:
> I can't use the NonThreadSafe here since the object will be deleted in the new
> capture thread. So I simply add a thread check to make sure Calculate() is
> always called on the same thread.

Can you document that then instead?

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_audio_level_calculator.cc (right):

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.cc:23: DCHECK(max <=
std::abd(std::numeric_limits<int16>::min()));
a few questions:

* does this compile? (abd?)
* The comment says "smaller than 32768".  The DCHECK allows _equal_ to 32768.
* 32768 is greater than int16 max, so we would be overflowing int16 if that
happened. (it's actually 0x8000 which when stored in an int16 is int16::min()!)
* Why not <= max()?

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.cc:57: // Divide the
max amplitude (32768) by 1000 to get in the range of [0,32]
it can't be 32768.  that's higher than int16 can carry.

[no longer working on chromium, 2014-03-01 12:37:00]

https://codereview.chromium.org/178223013/diff/90001/content/renderer/media/m...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/178223013/diff/90001/content/renderer/media/m...
content/renderer/media/media_stream_audio_level_calculator.h:17: class
MediaStreamAudioLevelCalculator : public base::NonThreadSafe {
On 2014/03/01 11:49:33, tommi wrote:
> On 2014/03/01 11:34:28, xians1 wrote:
> > I can't use the NonThreadSafe here since the object will be deleted in the
new
> > capture thread. So I simply add a thread check to make sure Calculate() is
> > always called on the same thread.
> 
> Can you document that then instead?

Done with adding comment above the thread_check_ member

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_audio_level_calculator.cc (right):

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.cc:23: DCHECK(max <=
std::abd(std::numeric_limits<int16>::min()));
On 2014/03/01 11:49:33, tommi wrote:
> a few questions:
> 
> * does this compile? (abd?)

No,  it should be abs.

> * The comment says "smaller than 32768".  The DCHECK allows _equal_ to 32768.

It should be not bigger than 32768.

> * 32768 is greater than int16 max, so we would be overflowing int16 if that
> happened. (it's actually 0x8000 which when stored in an int16 is
int16::min()!)
> * Why not <= max()?

We are calculating the absolute value of the amplitude, since the minimum value
of int16 is -32768, so the maximum of the absolute value is 32768.

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.cc:57: // Divide the
max amplitude (32768) by 1000 to get in the range of [0,32]
On 2014/03/01 11:49:33, tommi wrote:
> it can't be 32768.  that's higher than int16 can carry.


The same explaination, absolute value of <int16>::min() is 32768

[tommi (sloooow) - chröme, 2014-03-02 10:19:53]

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_audio_level_calculator.cc (right):

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.cc:23: DCHECK(max <=
std::abd(std::numeric_limits<int16>::min()));
On 2014/03/01 12:37:00, xians1 wrote:
> On 2014/03/01 11:49:33, tommi wrote:
> > a few questions:
> > 
> > * does this compile? (abd?)
> 
> No,  it should be abs.
> 
> > * The comment says "smaller than 32768".  The DCHECK allows _equal_ to
32768.
> 
> It should be not bigger than 32768.
> 
> > * 32768 is greater than int16 max, so we would be overflowing int16 if that
> > happened. (it's actually 0x8000 which when stored in an int16 is
> int16::min()!)
> > * Why not <= max()?
> 
> We are calculating the absolute value of the amplitude, since the minimum
value
> of int16 is -32768, so the maximum of the absolute value is 32768.

OK, now I get it.  Can you add documentation to MaxAmplitude can return a value
greater than std::numeric_limits<int16>::max()?  My concern was that if we would
at some point generalize this code to support other bits-per-sample situations,
then we can't return the same type as the samples themselves are stored in. 
I.e.

int16 max = MaxAmplitude(my_int16_buffer, length);

can actually end up with a negative number due to overflowing int16.

https://codereview.chromium.org/178223013/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/178223013/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.h:31: // audio
thread, this is because the WebRtcAudioCapturer will creates a new
s/creates/create

https://codereview.chromium.org/178223013/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.h:32: // source with
a new audio thread when the format of audio changes.
I think this comment assumes that the reader knows everything about how this
code hangs together.  It's not clear how WebRtcAudioCapturer and this class are
related but it looks like this class is now depending on implementation details
of WebRtcAudioCapturer.  Can this object sometimes be deleted on the thread on
which it is used and sometimes not?

Thanks for adding the documentation but I guess I'm actually more confused :)

Assuming I'm still generally parsing the comment correctly though; if the object
is created and used on the same thread, why do we need to pass ownership over to
a different thread just to delete it?

[no longer working on chromium, 2014-03-03 13:07:12]

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_audio_level_calculator.cc (right):

https://codereview.chromium.org/178223013/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.cc:23: DCHECK(max <=
std::abd(std::numeric_limits<int16>::min()));
On 2014/03/02 10:19:53, tommi wrote:
> On 2014/03/01 12:37:00, xians1 wrote:
> > On 2014/03/01 11:49:33, tommi wrote:
> > > a few questions:
> > > 
> > > * does this compile? (abd?)
> > 
> > No,  it should be abs.
> > 
> > > * The comment says "smaller than 32768".  The DCHECK allows _equal_ to
> 32768.
> > 
> > It should be not bigger than 32768.
> > 
> > > * 32768 is greater than int16 max, so we would be overflowing int16 if
that
> > > happened. (it's actually 0x8000 which when stored in an int16 is
> > int16::min()!)
> > > * Why not <= max()?
> > 
> > We are calculating the absolute value of the amplitude, since the minimum
> value
> > of int16 is -32768, so the maximum of the absolute value is 32768.
> 
> OK, now I get it.  Can you add documentation to MaxAmplitude can return a
value
> greater than std::numeric_limits<int16>::max()?  My concern was that if we
would
> at some point generalize this code to support other bits-per-sample
situations,
> then we can't return the same type as the samples themselves are stored in. 
> I.e.
> 
> int16 max = MaxAmplitude(my_int16_buffer, length);
> 
> can actually end up with a negative number due to overflowing int16.

Done with adding a comment.

https://codereview.chromium.org/178223013/diff/120001/content/renderer/media/...
File content/renderer/media/media_stream_audio_level_calculator.h (right):

https://codereview.chromium.org/178223013/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.h:31: // audio
thread, this is because the WebRtcAudioCapturer will creates a new
On 2014/03/02 10:19:53, tommi wrote:
> s/creates/create

The comment has been modified.

https://codereview.chromium.org/178223013/diff/120001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.h:32: // source with
a new audio thread when the format of audio changes.
On 2014/03/02 10:19:53, tommi wrote:
> I think this comment assumes that the reader knows everything about how this
> code hangs together.  It's not clear how WebRtcAudioCapturer and this class
are
> related but it looks like this class is now depending on implementation
details
> of WebRtcAudioCapturer.  Can this object sometimes be deleted on the thread on
> which it is used and sometimes not?

No, either the main render thread or the new audio thread.

> 
> Thanks for adding the documentation but I guess I'm actually more confused :)
> 
> Assuming I'm still generally parsing the comment correctly though; if the
object
> is created and used on the same thread, why do we need to pass ownership over
to
> a different thread just to delete it?

We don't really pass the ownership to another thread.
The track uses a scoped_ptr for the MediaStreamAudioLevelCalculator, and create
the instance in OnSetFormat() which happens on the audio thread. So if
OnSetFormat() only happen once, the object gets deleted when the track goes
away, which happens on the main render thread.
If OnSetFormat() is called more than once, the old instance will go out of scope
and new instance is created, and this happens on a new audio thread.

I changed the comment and hide the implementation details, PTAL.

[tommi (sloooow) - chröme, 2014-03-04 11:04:52]

lgtm

[no longer working on chromium, 2014-03-04 11:22:30]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-03-04 11:23:13]

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

[no longer working on chromium, 2014-03-04 14:39:26]

Committed patchset #8 manually as r254759 (presubmit successful).

[ajm, 2014-03-04 22:20:04]

Drive-by.

I know you're cloning the behavior of voice engine here, but this has always
been a very ad-hoc calculation. Much better would be to return some kind of more
standardized energy value, and let the app massage that into a level meter as it
desires.

Are we constraining ourselves with this, or are we free to change the
calculation later?

https://codereview.chromium.org/178223013/diff/160001/content/renderer/media/...
File content/renderer/media/media_stream_audio_level_calculator.cc (right):

https://codereview.chromium.org/178223013/diff/160001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.cc:45: // Permutation
of bars that reprents the amplitude level of the audio signal.
nit: represents

https://codereview.chromium.org/178223013/diff/160001/content/renderer/media/...
content/renderer/media/media_stream_audio_level_calculator.cc:64: // its in the
range 0-250 (instead of 0-1000)
nit: "i.e." "it's"

[DaleCurtis, 2014-03-04 22:23:45]

Drive by: Could you instead use media/audio/audio_power_monitor.h?

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

On 2014/03/04 22:20:04, ajm wrote:
> Drive-by.
> 
> I know you're cloning the behavior of voice engine here, but this has always
> been a very ad-hoc calculation. Much better would be to return some kind of
more
> standardized energy value, and let the app massage that into a level meter as
it
> desires.
> 
> Are we constraining ourselves with this, or are we free to change the
> calculation later?

The reason for keeping compatibility with voice engine is because these values
are fed back to PC for getStats.
Once we start making these properties available directly from audio track
objects in javascript, I think we should change the algorithm.  As is, we don't
want to change the behavior or the peerconnection stats reporting.

> 
>
https://codereview.chromium.org/178223013/diff/160001/content/renderer/media/...
> File content/renderer/media/media_stream_audio_level_calculator.cc (right):
> 
>
https://codereview.chromium.org/178223013/diff/160001/content/renderer/media/...
> content/renderer/media/media_stream_audio_level_calculator.cc:45: //
Permutation
> of bars that reprents the amplitude level of the audio signal.
> nit: represents
> 
>
https://codereview.chromium.org/178223013/diff/160001/content/renderer/media/...
> content/renderer/media/media_stream_audio_level_calculator.cc:64: // its in
the
> range 0-250 (instead of 0-1000)
> nit: "i.e." "it's"

[ajm, 2014-03-04 23:41:06]

On 2014/03/04 22:33:43, tommi wrote:
> On 2014/03/04 22:20:04, ajm wrote:
> > Drive-by.
> > 
> > I know you're cloning the behavior of voice engine here, but this has always
> > been a very ad-hoc calculation. Much better would be to return some kind of
> more
> > standardized energy value, and let the app massage that into a level meter
as
> it
> > desires.
> > 
> > Are we constraining ourselves with this, or are we free to change the
> > calculation later?
> 
> The reason for keeping compatibility with voice engine is because these values
> are fed back to PC for getStats.
> Once we start making these properties available directly from audio track
> objects in javascript, I think we should change the algorithm.  As is, we
don't
> want to change the behavior or the peerconnection stats reporting.

Got it, thanks.