Pull the Voice Activity Detector out from the AGC

Pull the Voice Activity Detector out from the AGC

This change generates bit-exact values when running through audioproc_f than before.

R=andrew@webrtc.org, bloch@google.com

Committed: https://chromium.googlesource.com/external/webrtc/+/34be126c1b3ee60ecdb86b1de41a0648347450b2

[aluebs-webrtc, 2015-06-12 02:09:51]

aluebs@webrtc.org changed reviewers:
+ andrew@webrtc.org, bloch@google.com

[aluebs-webrtc, 2015-06-12 02:09:53]

I know this CL looks mastodontic, but it is actually not THAT humongous. To
motivate you, here are some reasons why it is not THAT bad:
* All the moved files are duplicated, once Deleted and once Added (but mostly
the diffs are ok).
* The moved files required a lot of simple changes like includes, ifdef guards,
naming, etc.
* In the actual files that were modified and added, there is a lot of copied and
pasted code.
* You have to trust me that the sha1 files are right.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/noise_gmm_tables.h (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/noise_gmm_tables.h:1: /*
I didn't add this file. Just moved it. But the diff didn't realize :)

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/vad_audio_proc_internal.h (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/vad_audio_proc_internal.h:1: /*
I didn't modify this file. Just moved it. But the diffmaster didn't realized.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_gmm_tables.h (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_gmm_tables.h:1: /*
I didn't modify this file, just moved it. But the diff didn't realize.

[bloch, 2015-06-12 23:34:37]

I left a few comments on implementation details/choices. As such, you won't want
to make those changes in this refactoring, in order to preserve the consistency
of output you achieved with this CL.

I'll keep my questions/comments on record, and we can discuss them next round.
:)

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/vad_audio_proc_internal.h (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/vad_audio_proc_internal.h:1: /*
On 2015/06/12 02:09:52, aluebs-webrtc wrote:
> I didn't modify this file. Just moved it. But the diffmaster didn't realized.

Done.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector.cc (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:19: const int
kNumChannels = 1;
Just wondering, is there a VAD implementation that processes a multi-channel
input?

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector.h (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:36: // every time
there is no new data and then return a few chunk's data at once.
I'm thinking about what we discussed; I think I (personally) prefer this method.
It's closer to honestly reporting chunk-by-chunk data. However, will the
returned vector have all identical values? If so, it might make sense to just
return one. If not, it'll likely be too late to update the probability of a past
chunk--assuming you're using software that works in real-time.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:50: double
last_voice_probability_;
I know you're just refactoring old code, but is there a reason we're returning
doubles instead of floats? There's no way we have enough significant digits from
the calculation to need double the precision of just a floating-point number,
right?

[Andrew MacDonald, 2015-06-15 04:32:26]

https://codereview.webrtc.org/1181933002/diff/1/resources/audio_processing/va...
File resources/audio_processing/vad/rms.dat.sha1 (right):

https://codereview.webrtc.org/1181933002/diff/1/resources/audio_processing/va...
resources/audio_processing/vad/rms.dat.sha1:1:
eef89eb0ba1a2a9bc4722a3bfc56e1ffe2e6d753
Do you need to delete the corresponding files under agc?

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/agc/agc.cc (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/agc/agc.cc:54: std::vector<double> p =
vad_.chunkwise_voice_probabilities();
Could use const auto for these. Perhaps probabilities instead of just p?

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/agc/agc.cc:55: for (size_t i = 0; i <
rms.size(); ++i) {
DCHECK_EQ(rms.size(), p.size()) ?

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/gmm.cc (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/gmm.cc:25: double* out) {
In general, please avoid making unneeded changes when moving files around like
this.

But since you've done this, please run "git cl format --full" on all of these
files in a later CL.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector.cc (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:39: if (length !=
kLength10Ms) {
This assumes that chunks of 10 ms are passed in. I think you should continue to
take the sample rate as input.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:44: assert(length
== kLength10Ms);
DCHECK_EQ

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:50:
chunkwise_voice_probabilities_.clear();
minor: It might be overall more clear if you resize both vectors to num_frames
here.
chunkwise_voice_probabilities_.resize(features_.num_frames);
chunkwise_rms_.num_frames(features_.num_frames);
std::copy(features_.rms, features_.rms + chunkwise_rms_.size(),
chunkwise_rms_.begin());
...
std::fill(chunkwise_voice_probabilities_.begin(),
chunkwise_voice_probabilities_.end(), kLowProbability);
...

wdyt?

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:60:
kNeutralProbability);
Move the call to standalone_vad_->AddAudio to here? Looks like it's not needed
otherwise.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:62:
kMaxNumFrames),
I'm worried about this call. You're only guaranteed that
chunkwise_voice_probabilities_.size() == features_.num_frames, which could be
less than kMaxNumFrames, right?

Is kMaxNumFrames needed at all? Perhaps just use
chunkwise_voice_probabilities_.size() here?

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:69:
chunkwise_voice_probabilities_[features_.num_frames - 1];
chunkwise_voice_probabilities_.last()

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector.h (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:1: /*
After all reviewers have LGTM'd, please run git cl format on your totally new
files.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:33: void
ProcessCaptureAudio(const int16_t* audio, int length);
Perhaps ProcessChunk instead? It doesn't have to be captured audio.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:35: // Because
ISAC has a different chunk length, it returns a zero size vector
This is an implementation detail; you could put it in the implementation of
ProcessCaptureAudio. Here, I think you should discuss what this method returns
exactly (and for RMS below).

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:38: return
chunkwise_voice_probabilities_;
Since this is called on every chunk, you don't want to return by value here.
Return a "const std::vector<double>&" instead.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:40:
std::vector<double> chunkwise_rms() const { return chunkwise_rms_; }
As above, return a const reference.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:44: float
last_voice_probability() const { return last_voice_probability_; }
Should definitely convert things to float later given this inconsistency :)

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:50: double
last_voice_probability_;
On 2015/06/12 at 23:34:38, bloch wrote:
> I know you're just refactoring old code, but is there a reason we're returning
doubles instead of floats? There's no way we have enough significant digits from
the calculation to need double the precision of just a floating-point number,
right?

Agreed, I'm sure these could be floats. However it involves touching the moved
code as well. Alex, do you want to do that in a follow-up?

[aluebs-webrtc, 2015-06-16 01:17:53]

https://codereview.webrtc.org/1181933002/diff/1/resources/audio_processing/va...
File resources/audio_processing/vad/rms.dat.sha1 (right):

https://codereview.webrtc.org/1181933002/diff/1/resources/audio_processing/va...
resources/audio_processing/vad/rms.dat.sha1:1:
eef89eb0ba1a2a9bc4722a3bfc56e1ffe2e6d753
On 2015/06/15 04:32:25, andrew wrote:
> Do you need to delete the corresponding files under agc?

These are new files used only in the new voice_activity_detector_unittest.cc.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/agc/agc.cc (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/agc/agc.cc:54: std::vector<double> p =
vad_.chunkwise_voice_probabilities();
On 2015/06/15 04:32:25, andrew wrote:
> Could use const auto for these. Perhaps probabilities instead of just p?

I find the explicit types add readability here, but I totally agree that
"probabilities" is a better naming.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/agc/agc.cc:55: for (size_t i = 0; i <
rms.size(); ++i) {
On 2015/06/15 04:32:25, andrew wrote:
> DCHECK_EQ(rms.size(), p.size()) ?

Done.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/gmm.cc (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/gmm.cc:25: double* out) {
On 2015/06/15 04:32:25, andrew wrote:
> In general, please avoid making unneeded changes when moving files around like
> this.
> 
> But since you've done this, please run "git cl format --full" on all of these
> files in a later CL.

I only ran "git cl format" because the presubmit was complaining about these
files. I guess I could have skipped the presubmit somehow.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/vad_audio_proc_internal.h (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/vad_audio_proc_internal.h:1: /*
On 2015/06/12 23:34:38, bloch wrote:
> On 2015/06/12 02:09:52, aluebs-webrtc wrote:
> > I didn't modify this file. Just moved it. But the diffmaster didn't
realized.
> 
> Done.

:)

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector.cc (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:19: const int
kNumChannels = 1;
On 2015/06/12 23:34:38, bloch wrote:
> Just wondering, is there a VAD implementation that processes a multi-channel
> input?

Not right now, and I don't think we will need it, so I want to keep this as
simple as possible. Also, if multi-channel is needed, you can always have a
vector of VADs.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:39: if (length !=
kLength10Ms) {
On 2015/06/15 04:32:25, andrew wrote:
> This assumes that chunks of 10 ms are passed in. I think you should continue
to
> take the sample rate as input.

Agreed.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:44: assert(length
== kLength10Ms);
On 2015/06/15 04:32:25, andrew wrote:
> DCHECK_EQ

Done.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:50:
chunkwise_voice_probabilities_.clear();
On 2015/06/15 04:32:25, andrew wrote:
> minor: It might be overall more clear if you resize both vectors to num_frames
> here.
> chunkwise_voice_probabilities_.resize(features_.num_frames);
> chunkwise_rms_.num_frames(features_.num_frames);
> std::copy(features_.rms, features_.rms + chunkwise_rms_.size(),
> chunkwise_rms_.begin());
> ...
> std::fill(chunkwise_voice_probabilities_.begin(),
> chunkwise_voice_probabilities_.end(), kLowProbability);
> ...
> 
> wdyt?

But the point in clearing here is that it returns a zero-size vector when there
is no information. WDYT?

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:60:
kNeutralProbability);
On 2015/06/15 04:32:25, andrew wrote:
> Move the call to standalone_vad_->AddAudio to here? Looks like it's not needed
> otherwise.

But here it will not be run on each chunk, which is the idea. Internally it
buffers the audio every "AddAudio" call and runs the VAD on the buffered audio
on each "GetActivity" call.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:62:
kMaxNumFrames),
On 2015/06/15 04:32:25, andrew wrote:
> I'm worried about this call. You're only guaranteed that
> chunkwise_voice_probabilities_.size() == features_.num_frames, which could be
> less than kMaxNumFrames, right?
> 
> Is kMaxNumFrames needed at all? Perhaps just use
> chunkwise_voice_probabilities_.size() here?

That is an excellent point! I think in practice they are always equal, but it es
better to be sure.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:69:
chunkwise_voice_probabilities_[features_.num_frames - 1];
On 2015/06/15 04:32:25, andrew wrote:
> chunkwise_voice_probabilities_.last()

Done.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector.h (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:1: /*
On 2015/06/15 04:32:25, andrew wrote:
> After all reviewers have LGTM'd, please run git cl format on your totally new
> files.

Acknowledged.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:33: void
ProcessCaptureAudio(const int16_t* audio, int length);
On 2015/06/15 04:32:25, andrew wrote:
> Perhaps ProcessChunk instead? It doesn't have to be captured audio.

Good point! Done.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:35: // Because
ISAC has a different chunk length, it returns a zero size vector
On 2015/06/15 04:32:25, andrew wrote:
> This is an implementation detail; you could put it in the implementation of
> ProcessCaptureAudio. Here, I think you should discuss what this method returns
> exactly (and for RMS below).

Done.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:36: // every time
there is no new data and then return a few chunk's data at once.
On 2015/06/12 23:34:38, bloch wrote:
> I'm thinking about what we discussed; I think I (personally) prefer this
method.
> It's closer to honestly reporting chunk-by-chunk data. However, will the
> returned vector have all identical values? If so, it might make sense to just
> return one. If not, it'll likely be too late to update the probability of a
past
> chunk--assuming you're using software that works in real-time.

It will not have identical values. As you point out, in real time, you can't
update the probability of a past chunk. But it is used by the AGC for updating a
histograms with all the values.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:38: return
chunkwise_voice_probabilities_;
On 2015/06/15 04:32:26, andrew wrote:
> Since this is called on every chunk, you don't want to return by value here.
> Return a "const std::vector<double>&" instead.

Done.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:40:
std::vector<double> chunkwise_rms() const { return chunkwise_rms_; }
On 2015/06/15 04:32:25, andrew wrote:
> As above, return a const reference.

Done.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:44: float
last_voice_probability() const { return last_voice_probability_; }
On 2015/06/15 04:32:25, andrew wrote:
> Should definitely convert things to float later given this inconsistency :)

Fixed this inconsistency in this CL, since it makes no sense, but will convert
everything to float on another CL.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:50: double
last_voice_probability_;
On 2015/06/15 04:32:26, andrew wrote:
> On 2015/06/12 at 23:34:38, bloch wrote:
> > I know you're just refactoring old code, but is there a reason we're
returning
> doubles instead of floats? There's no way we have enough significant digits
from
> the calculation to need double the precision of just a floating-point number,
> right?
> 
> Agreed, I'm sure these could be floats. However it involves touching the moved
> code as well. Alex, do you want to do that in a follow-up?

Yes, I can do this in a follow up CL. But I changed to float here, to avoid a
cast.

[Andrew MacDonald, 2015-06-16 22:18:30]

andrew@webrtc.org changed reviewers:
+ turaj@webrtc.org

[Andrew MacDonald, 2015-06-16 22:18:32]

Turaj, can you have a quick look to verify the changes in agc.cc and
voice_activity_detector.cc?

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector.cc (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:50:
chunkwise_voice_probabilities_.clear();
On 2015/06/16 01:17:52, aluebs-webrtc wrote:
> On 2015/06/15 04:32:25, andrew wrote:
> > minor: It might be overall more clear if you resize both vectors to
num_frames
> > here.
> > chunkwise_voice_probabilities_.resize(features_.num_frames);
> > chunkwise_rms_.num_frames(features_.num_frames);
> > std::copy(features_.rms, features_.rms + chunkwise_rms_.size(),
> > chunkwise_rms_.begin());
> > ...
> > std::fill(chunkwise_voice_probabilities_.begin(),
> > chunkwise_voice_probabilities_.end(), kLowProbability);
> > ...
> > 
> > wdyt?
> 
> But the point in clearing here is that it returns a zero-size vector when
there
> is no information. WDYT?

It will work, assuming features_.num_frames == 0 when there are no features. The
vectors will be resized to zero.

Still this is more a preference; whatever you want to do.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:60:
kNeutralProbability);
On 2015/06/16 01:17:52, aluebs-webrtc wrote:
> On 2015/06/15 04:32:25, andrew wrote:
> > Move the call to standalone_vad_->AddAudio to here? Looks like it's not
needed
> > otherwise.
> 
> But here it will not be run on each chunk, which is the idea. Internally it
> buffers the audio every "AddAudio" call and runs the VAD on the buffered audio
> on each "GetActivity" call.

Ah OK. Can you put a comment to that effect on the call above?

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:62:
kMaxNumFrames),
On 2015/06/16 01:17:52, aluebs-webrtc wrote:
> On 2015/06/15 04:32:25, andrew wrote:
> > I'm worried about this call. You're only guaranteed that
> > chunkwise_voice_probabilities_.size() == features_.num_frames, which could
be
> > less than kMaxNumFrames, right?
> > 
> > Is kMaxNumFrames needed at all? Perhaps just use
> > chunkwise_voice_probabilities_.size() here?
> 
> That is an excellent point! I think in practice they are always equal, but it
es
> better to be sure.

Where is kMaxNumFrames defined? Can you remove it?

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/agc/agc.cc (right):

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc/agc.cc:55: std::vector<double> probabilities
= vad_.chunkwise_voice_probabilities();
You're still allocating two vectors here and copy assigning the return values to
them. You want:

const std::vector<double>& rms = ...

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector.cc (right):

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:36: // Else it
clears them.
Else -> Otherwise

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector.h (right):

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:33: void
ProcessChunk(const int16_t* audio, int length, int sample_rate_hz);
Should length be a size_t?

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:41: // Returns a
vector of RMS values for each chunk. It has the same length as
Vertical space above this line.

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:35:
TEST(VoiceActivityDetectorTest, DetectsVoiceActivityCorrectly) {
Is there some way we can test this without relying on golden output? Like feed
in your speech file and verify it returns an average probability higher than
some threshold. Feed in (generated) noise and it should be below some threshold?
Mix of the two and it should be higher than some lower threshold?

[aluebs-webrtc, 2015-06-17 01:42:23]

Patchset #3 (id:40001) has been deleted

[aluebs-webrtc, 2015-06-17 01:44:31]

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector.cc (right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:50:
chunkwise_voice_probabilities_.clear();
On 2015/06/16 22:18:31, andrew wrote:
> On 2015/06/16 01:17:52, aluebs-webrtc wrote:
> > On 2015/06/15 04:32:25, andrew wrote:
> > > minor: It might be overall more clear if you resize both vectors to
> num_frames
> > > here.
> > > chunkwise_voice_probabilities_.resize(features_.num_frames);
> > > chunkwise_rms_.num_frames(features_.num_frames);
> > > std::copy(features_.rms, features_.rms + chunkwise_rms_.size(),
> > > chunkwise_rms_.begin());
> > > ...
> > > std::fill(chunkwise_voice_probabilities_.begin(),
> > > chunkwise_voice_probabilities_.end(), kLowProbability);
> > > ...
> > > 
> > > wdyt?
> > 
> > But the point in clearing here is that it returns a zero-size vector when
> there
> > is no information. WDYT?
> 
> It will work, assuming features_.num_frames == 0 when there are no features.
The
> vectors will be resized to zero.
> 
> Still this is more a preference; whatever you want to do.

Good point. I agree it is clearer, so I changed it to your suggestion.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:60:
kNeutralProbability);
On 2015/06/16 22:18:31, andrew wrote:
> On 2015/06/16 01:17:52, aluebs-webrtc wrote:
> > On 2015/06/15 04:32:25, andrew wrote:
> > > Move the call to standalone_vad_->AddAudio to here? Looks like it's not
> needed
> > > otherwise.
> > 
> > But here it will not be run on each chunk, which is the idea. Internally it
> > buffers the audio every "AddAudio" call and runs the VAD on the buffered
audio
> > on each "GetActivity" call.
> 
> Ah OK. Can you put a comment to that effect on the call above?

Done.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:62:
kMaxNumFrames),
On 2015/06/16 22:18:31, andrew wrote:
> On 2015/06/16 01:17:52, aluebs-webrtc wrote:
> > On 2015/06/15 04:32:25, andrew wrote:
> > > I'm worried about this call. You're only guaranteed that
> > > chunkwise_voice_probabilities_.size() == features_.num_frames, which could
> be
> > > less than kMaxNumFrames, right?
> > > 
> > > Is kMaxNumFrames needed at all? Perhaps just use
> > > chunkwise_voice_probabilities_.size() here?
> > 
> > That is an excellent point! I think in practice they are always equal, but
it
> es
> > better to be sure.
> 
> Where is kMaxNumFrames defined? Can you remove it?

It is defined in vad/common.h and used to define the sizes of the arrays in
AudioFeatures and in:
* activity_metric.cc
* vad_audio_proc.cc
* standalone_vad_unittest.cc
* vad_audio_proc_unittest.cc
So I guess I can't remove it for now.

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/agc/agc.cc (right):

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc/agc.cc:55: std::vector<double> probabilities
= vad_.chunkwise_voice_probabilities();
On 2015/06/16 22:18:31, andrew wrote:
> You're still allocating two vectors here and copy assigning the return values
to
> them. You want:
> 
> const std::vector<double>& rms = ...

You are right. Done.

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector.cc (right):

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:36: // Else it
clears them.
On 2015/06/16 22:18:32, andrew wrote:
> Else -> Otherwise

Done.

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector.h (right):

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:33: void
ProcessChunk(const int16_t* audio, int length, int sample_rate_hz);
On 2015/06/16 22:18:32, andrew wrote:
> Should length be a size_t?

I tried this, but realized it required changes in SO many files that I added a
TODO.

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector.h:41: // Returns a
vector of RMS values for each chunk. It has the same length as
On 2015/06/16 22:18:32, andrew wrote:
> Vertical space above this line.

Done.

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:35:
TEST(VoiceActivityDetectorTest, DetectsVoiceActivityCorrectly) {
On 2015/06/16 22:18:32, andrew wrote:
> Is there some way we can test this without relying on golden output? Like feed
> in your speech file and verify it returns an average probability higher than
> some threshold. Feed in (generated) noise and it should be below some
threshold?
> Mix of the two and it should be higher than some lower threshold?

That is way better than what I have now. Done.

[Andrew MacDonald, 2015-06-17 04:14:57]

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:23:
const float kMaxNoiseProbability = 0.05f;
Move these three to the tests where they're used.

And just curious, do you actually see values above 0.0?

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:29:
data[i] = std::rand();
std::rand returns between 0 and RAND_MAX. I guess this will work because it
wraps into some random place. Might want to add a comment though.

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:44:
EXPECT_DOUBLE_EQ(vad.last_voice_probability(), kDefaultVoiceValue);
EXPECT_FLOAT_EQ

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:54:
fopen(test::ResourcePath("audio_processing/agc/agc_audio", "pcm").c_str(),
What is actually in this file? Is it mostly speech? I'm a bit surprised you
can't make the threshold higher.

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:75:
int16_t data[kLength10Ms];
nit: Use a vector? Then you don't need to pass the length separately to
GenerateNoise.

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:85: if
(i > kNumChunksPerIsacBlock) {
Do you need to know about this? Why not just check every chunk?

[aluebs-webrtc, 2015-06-17 17:20:45]

Patchset #4 (id:80001) has been deleted

[aluebs-webrtc, 2015-06-17 17:22:04]

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:23:
const float kMaxNoiseProbability = 0.05f;
On 2015/06/17 04:14:57, andrew wrote:
> Move these three to the tests where they're used.
> 
> And just curious, do you actually see values above 0.0?

Done.
And yes, there are a few (2 or 3) values which come up to 0.04 or something.

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:29:
data[i] = std::rand();
On 2015/06/17 04:14:57, andrew wrote:
> std::rand returns between 0 and RAND_MAX. I guess this will work because it
> wraps into some random place. Might want to add a comment though.

Yes, I know, but I assumed the wrapping would still give me noise. Added
comment.

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:44:
EXPECT_DOUBLE_EQ(vad.last_voice_probability(), kDefaultVoiceValue);
On 2015/06/17 04:14:57, andrew wrote:
> EXPECT_FLOAT_EQ

I forgot to change this after the double->float change. Thanks for catching
that.

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:54:
fopen(test::ResourcePath("audio_processing/agc/agc_audio", "pcm").c_str(),
On 2015/06/17 04:14:57, andrew wrote:
> What is actually in this file? Is it mostly speech? I'm a bit surprised you
> can't make the threshold higher.

Yes, there is some utterances with some silence in between. Because of the
silences, noise and non-voiced speech I am actually not surprised that the
threshold is that low. It actually is more like 0.39, but I used 0.3 to be more
robust. Maybe I can tweak that a little more, WDYT? Also, I could measure
max_probability instead of mean_probability, which will be definitively higher,
but I think this gives a better picture. WDYT?

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:75:
int16_t data[kLength10Ms];
On 2015/06/17 04:14:56, andrew wrote:
> nit: Use a vector? Then you don't need to pass the length separately to
> GenerateNoise.

Done.

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:85: if
(i > kNumChunksPerIsacBlock) {
On 2015/06/17 04:14:57, andrew wrote:
> Do you need to know about this? Why not just check every chunk?

Because before the vad has enough data to process an ISAC block it will return
the default value, 1.f, which would ruin the max_probability value.

[Andrew MacDonald, 2015-06-17 21:15:15]

lgtm

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:85: if
(i > kNumChunksPerIsacBlock) {
On 2015/06/17 17:22:04, aluebs-webrtc wrote:
> On 2015/06/17 04:14:57, andrew wrote:
> > Do you need to know about this? Why not just check every chunk?
> 
> Because before the vad has enough data to process an ISAC block it will return
> the default value, 1.f, which would ruin the max_probability value.

Ah OK. Perhaps add a comment to that effect.

[bloch, 2015-06-17 22:27:49]

LGTM

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:36:
const size_t kNumChunksPerIsacBlock = 3;
At what point in the workflow is this value determined? Is it always 3?

https://codereview.webrtc.org/1181933002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/vad/voice_activity_detector.cc (right):

https://codereview.webrtc.org/1181933002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:44: // Resample
to the required rate.
Does this account for an input sample rate greater than 32hz?

https://codereview.webrtc.org/1181933002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:32:
TEST(VoiceActivityDetectorTest, ConstructorSetsDefaultValues) {
Where do you test your resampling (/sample rate)? I know you're testing
higher-level functionality here, but is there somewhere you're testing edge
cases for these method parameters?

[aluebs-webrtc, 2015-06-18 00:49:22]

I added 2 tests for 32kHz. PTAL.

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:36:
const size_t kNumChunksPerIsacBlock = 3;
On 2015/06/17 22:27:49, bloch wrote:
> At what point in the workflow is this value determined? Is it always 3?

That is a restriction that ISAC imposes on the VAD and it is going to be 3 at
least until we rely on another VAD internally, which is not part of the plan. I
agree it is not nice to have these implementation details in the test, but this
is also an old version of the file. If you check the new version, you will see
it is only used to drop the default values at the beginning to avoid ruining the
max probability.

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:85: if
(i > kNumChunksPerIsacBlock) {
On 2015/06/17 21:15:14, andrew wrote:
> On 2015/06/17 17:22:04, aluebs-webrtc wrote:
> > On 2015/06/17 04:14:57, andrew wrote:
> > > Do you need to know about this? Why not just check every chunk?
> > 
> > Because before the vad has enough data to process an ISAC block it will
return
> > the default value, 1.f, which would ruin the max_probability value.
> 
> Ah OK. Perhaps add a comment to that effect.

Done.

https://codereview.webrtc.org/1181933002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/vad/voice_activity_detector.cc (right):

https://codereview.webrtc.org/1181933002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector.cc:44: // Resample
to the required rate.
On 2015/06/17 22:27:49, bloch wrote:
> Does this account for an input sample rate greater than 32hz? 

No, that is what the check in line 43 is for :)
But anyway, this is mostly going to be used only on split data, so 16kHz.

https://codereview.webrtc.org/1181933002/diff/100001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:32:
TEST(VoiceActivityDetectorTest, ConstructorSetsDefaultValues) {
On 2015/06/17 22:27:49, bloch wrote:
> Where do you test your resampling (/sample rate)? I know you're testing
> higher-level functionality here, but is there somewhere you're testing edge
> cases for these method parameters?

That is a good point. Added tests for 32kHz audio and noise. I know those fseek
are horrible, but these files have a lot of silences and clicks I want to avoid
to get a higher voice probability. It is that or adding special files to the
resources. WDYT?

[Andrew MacDonald, 2015-06-18 02:02:52]

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:25:
const size_t kNumChunks = 100u;
Is it worthwhile making this a bit longer?

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:85:
std::vector<int16_t> data(2 * kLength10Ms);
Same here, don't use 2 *.

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:92:
ASSERT_TRUE(pcm_file != NULL);
nullptr

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:93:
ASSERT_EQ(fseek(pcm_file,
I think the fseek's OK, but add a comment explaining why you're doing it.

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:94:
kStartTimeSec * 2 * kSampleRateHz * sizeof(data[0]),
Don't use 2 * kSampleRateHz. Add a new explicit sample rate in this test.

Come to think of it, where are these things coming from? Don't you want to set
them yourself in the test?

[aluebs-webrtc, 2015-06-18 16:32:40]

PTAL

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc
(right):

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:25:
const size_t kNumChunks = 100u;
On 2015/06/18 02:02:51, andrew wrote:
> Is it worthwhile making this a bit longer?

Sure, why not. Now it has approximately the same amount of chunks as the speech
utterance.

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:85:
std::vector<int16_t> data(2 * kLength10Ms);
On 2015/06/18 02:02:51, andrew wrote:
> Same here, don't use 2 *.

Done.

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:92:
ASSERT_TRUE(pcm_file != NULL);
On 2015/06/18 02:02:51, andrew wrote:
> nullptr

Done.

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:93:
ASSERT_EQ(fseek(pcm_file,
On 2015/06/18 02:02:51, andrew wrote:
> I think the fseek's OK, but add a comment explaining why you're doing it.

Of course. Done.

https://codereview.webrtc.org/1181933002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/vad/voice_activity_detector_unittest.cc:94:
kStartTimeSec * 2 * kSampleRateHz * sizeof(data[0]),
On 2015/06/18 02:02:51, andrew wrote:
> Don't use 2 * kSampleRateHz. Add a new explicit sample rate in this test.
> 
> Come to think of it, where are these things coming from? Don't you want to set
> them yourself in the test?

They were coming from vad/common.h, but I agree that I want to set them myself
in the test. Done.

[Andrew MacDonald, 2015-06-18 17:52:58]

lgtm

[aluebs-webrtc, 2015-06-18 19:34:09]

Committed patchset #7 (id:160001) manually as
34be126c1b3ee60ecdb86b1de41a0648347450b2 (presubmit successful).