Add an endpointer for detecting end of speech.

Add an endpointer for detecting end of speech.
This is based on existing code/math. I have removed all the unused code for our usage and adapted to the chromium coding style.

TEST=unit_tests --gtest_filter=EndpointerTest.*
BUG=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57226

[joth, 2010-08-19 18:00:03]

Some initial comments on the .h files
not read any .cc in detail other than as needed to understand the .h.
would be useful to find the reference for the algorithm and any existing impl, &
tests, before reading it in detail.

Cheers

http://codereview.chromium.org/3117026/diff/10001/11001
File chrome/browser/speech/endpointer/endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/10001/11001#newcode26
chrome/browser/speech/endpointer/endpointer.cc:26:
speech_input_complete_silence_length_ = static_cast<int64>(0.5 *
kSecToMicrosec);
line length

http://codereview.chromium.org/3117026/diff/10001/11002
File chrome/browser/speech/endpointer/endpointer.h (right):

http://codereview.chromium.org/3117026/diff/10001/11002#newcode41
chrome/browser/speech/endpointer/endpointer.h:41: EpStatus Status(int64 *time);
int64* time
units?

Time (or TimeDelta) instead?

http://codereview.chromium.org/3117026/diff/10001/11002#newcode63
chrome/browser/speech/endpointer/endpointer.h:63: static const int
kSecToMicrosec;
remove
use base::Time::kMicrosecondsPerSecond

http://codereview.chromium.org/3117026/diff/10001/11002#newcode70
chrome/browser/speech/endpointer/endpointer.h:70: // Minimum allowable length of
speech input.
units for these 5 lengths?
Could you use TimeDelta instead?

ditto for the setters

http://codereview.chromium.org/3117026/diff/10001/11004
File chrome/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/10001/11004#newcode25
chrome/browser/speech/endpointer/energy_endpointer.cc:25: return
static_cast<float>(sqrt((ssq / num_samples) - (sum * sum)));
is this all original thinking, or can I use your source to help me review?

http://codereview.chromium.org/3117026/diff/10001/11004#newcode38
chrome/browser/speech/endpointer/energy_endpointer.cc:38: data_.reset(new
DecisionPoint[size]);
this is not so nice: you are storing a heap array inside a scoped_ptr which will
free it with delete where it should use delete[]

would a vector<DecisionPoint> work better for you?
this whole function then becomes
const DecisionPoint init = {.decision = initial_state, .time = -1};
data_.clear();
data_.resize(size, init);

and I think you can remove size_ too then.

http://codereview.chromium.org/3117026/diff/10001/11005
File chrome/browser/speech/endpointer/energy_endpointer.h (right):

http://codereview.chromium.org/3117026/diff/10001/11005#newcode45
chrome/browser/speech/endpointer/energy_endpointer.h:45: };
slightly odd naming, but I guess it's convenient to keep this consistent with
elsewhere

http://codereview.chromium.org/3117026/diff/10001/11005#newcode76
chrome/browser/speech/endpointer/energy_endpointer.h:76: void
ProcessAudioFrame(int64 time, const int16* samples, int num_samples);
time units?
Use TimeDelta?

http://codereview.chromium.org/3117026/diff/10001/11005#newcode86
chrome/browser/speech/endpointer/energy_endpointer.h:86: class HistoryRing {
maybe just declare class HistoryRing here, and define it in the .cc

http://codereview.chromium.org/3117026/diff/10001/11005#newcode109
chrome/browser/speech/endpointer/energy_endpointer.h:109:
scoped_ptr<DecisionPoint> data_;
data_ is a fairly generic name. decision_points_ ?

http://codereview.chromium.org/3117026/diff/10001/11005#newcode110
chrome/browser/speech/endpointer/energy_endpointer.h:110: int index_;
comment what this relates to (index of next insertion point in |data_| )
maybe a more specific name would help

http://codereview.chromium.org/3117026/diff/10001/11005#newcode113
chrome/browser/speech/endpointer/energy_endpointer.h:113:
DISALLOW_EVIL_CONSTRUCTORS(HistoryRing);
DISALLOW COPY AND ASSIGN

http://codereview.chromium.org/3117026/diff/10001/11007
File chrome/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/3117026/diff/10001/11007#newcode50
chrome/browser/speech/speech_recognizer.cc:50: int num_samples,
would be nicer as
const std::vector<short> samples

(not worth it if it forces extra copies)

http://codereview.chromium.org/3117026/diff/10001/11007#newcode108
chrome/browser/speech/speech_recognizer.cc:108: endpointer_.StartCapture();
this is a bit missleading: to me this looks like it is telling the endpointer_
to start it's own internal capture (i.e. direct from the hardware)
- calling StartCapture() seems redundant when followed by
StartEnvironmentEstimation() 
- could this happen in StartRecording rather than the c'tor?
- could it be call SetCaptureMode / SetEnvEstimationMode or something?

http://codereview.chromium.org/3117026/diff/10001/11007#newcode151
chrome/browser/speech/speech_recognizer.cc:151:
endpointer_.StartEnvironmentEstimation();
confused.
it seems to me that when the endpointer is in env estimation mode, we'll never
actually send it any audio frames.

For this to work, surely we need to keep audio_controller_ around for as long as
we have this SpeechRecognizer instance, and use some other state to work out
when we're in idle vs. recording mode.

Test code?

http://codereview.chromium.org/3117026/diff/10001/11009
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3117026/diff/10001/11009#newcode2400
chrome/chrome_browser.gypi:2400: 'browser/speech/endpointer/endpointer.cc',
e before s

http://codereview.chromium.org/3117026/diff/10001/11010
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/3117026/diff/10001/11010#newcode1209
chrome/chrome_tests.gypi:1209:
'browser/speech/endpointer/endpointer_unittest.cc',
e comes before s

[Satish, 2010-08-19 22:18:44]

Thanks for the quick review. All comments implemented/addressed with a few
replies below. Please take a fresh look.

http://codereview.chromium.org/3117026/diff/10001/11002
File chrome/browser/speech/endpointer/endpointer.h (right):

http://codereview.chromium.org/3117026/diff/10001/11002#newcode41
chrome/browser/speech/endpointer/endpointer.h:41: EpStatus Status(int64 *time);
On 2010/08/19 18:00:03, joth wrote:
> int64* time
> units?
> 
> Time (or TimeDelta) instead?
> 

Since this is based on existing original code and I'd like to not disturb the
key math/calculation part of the code, I prefer to continue keep these as int64.
So I've added time units here and all other places.

http://codereview.chromium.org/3117026/diff/10001/11004
File chrome/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/10001/11004#newcode38
chrome/browser/speech/endpointer/energy_endpointer.cc:38: data_.reset(new
DecisionPoint[size]);
On 2010/08/19 18:00:03, joth wrote:
> this is not so nice: you are storing a heap array inside a scoped_ptr which
will
> free it with delete where it should use delete[]
> 
> would a vector<DecisionPoint> work better for you?
> this whole function then becomes
> const DecisionPoint init = {.decision = initial_state, .time = -1};
> data_.clear();
> data_.resize(size, init);
> 
> and I think you can remove size_ too then.

vector works well in this case, switched to that.

http://codereview.chromium.org/3117026/diff/10001/11005
File chrome/browser/speech/endpointer/energy_endpointer.h (right):

http://codereview.chromium.org/3117026/diff/10001/11005#newcode86
chrome/browser/speech/endpointer/energy_endpointer.h:86: class HistoryRing {
On 2010/08/19 18:00:03, joth wrote:
> maybe just declare class HistoryRing here, and define it in the .cc

I tried that but scoped_ptr complains about incomplete type when building
endpointer.cc, perhaps because this class is used as a member variable in that
and not a dynamically allocated pointer. So left this here.

http://codereview.chromium.org/3117026/diff/10001/11007
File chrome/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/3117026/diff/10001/11007#newcode50
chrome/browser/speech/speech_recognizer.cc:50: int num_samples,
On 2010/08/19 18:00:03, joth wrote:
> would be nicer as
> const std::vector<short> samples
> 
> (not worth it if it forces extra copies)

Yes it will require extra copies, hence decided in the previous CL not to use a
vector.

http://codereview.chromium.org/3117026/diff/10001/11007#newcode108
chrome/browser/speech/speech_recognizer.cc:108: endpointer_.StartCapture();
On 2010/08/19 18:00:03, joth wrote:
> this is a bit missleading: to me this looks like it is telling the endpointer_
> to start it's own internal capture (i.e. direct from the hardware)
> - calling StartCapture() seems redundant when followed by
> StartEnvironmentEstimation() 
> - could this happen in StartRecording rather than the c'tor?
> - could it be call SetCaptureMode / SetEnvEstimationMode or something?

I have renamed the methods to more appropriate names now and fixed this code as
well. I have also added a huge comment here explaining the logic and a TODO as
well for a future work item related to this.

[joth, 2010-08-23 12:02:01]

LGTM

Have to admit, I didn't spend the time to get my head around every line of the
algorithm. Let me know if I should have :)

http://codereview.chromium.org/3117026/diff/13002/20003
File chrome/browser/speech/endpointer/endpointer_unittest.cc (right):

http://codereview.chromium.org/3117026/diff/13002/20003#newcode40
chrome/browser/speech/endpointer/endpointer_unittest.cc:40: const int kFrameSize
= kSampleRate / kFrameRate;  // 160 samples.
160 samples per frame.
you could capture this as a StaticAssert , given the code below depends on it
being > 100

http://codereview.chromium.org/3117026/diff/13002/20003#newcode78
chrome/browser/speech/endpointer/endpointer_unittest.cc:78: if (120 ==
frame_count) EXPECT_EQ(EP_PRE_SPEECH, ep_status);
break line before the EXPECT...
(x3)

http://codereview.chromium.org/3117026/diff/13002/20003#newcode103
chrome/browser/speech/endpointer/endpointer_unittest.cc:103: for (int
frame_count = 0; frame_count < kNumFrames; ++frame_count) {
this looks almost identical to the above test. Can we factor it out?
maybe use a templated function to test the 2 api layers.

http://codereview.chromium.org/3117026/diff/13002/20003#newcode123
chrome/browser/speech/endpointer/endpointer_unittest.cc:123: if (20 ==
frame_count) EXPECT_EQ(EP_PRE_SPEECH, ep_status);
break line before the EXPECT...
(x3)

http://codereview.chromium.org/3117026/diff/13002/20003#newcode129
chrome/browser/speech/endpointer/endpointer_unittest.cc:129: }
i can't easily tell, does this cover testing wrap around on the internal ring
buffer?
you could pre-fill the buffer with size()-1 of junk samples, to test this case?

http://codereview.chromium.org/3117026/diff/13002/20004
File chrome/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/13002/20004#newcode4
chrome/browser/speech/endpointer/energy_endpointer.cc:4: 
add brief desc & link to source xref

http://codereview.chromium.org/3117026/diff/13002/20004#newcode28
chrome/browser/speech/endpointer/energy_endpointer.cc:28: static int64
Secs2Usecs(float seconds) {
no need for static inside anon namespace

http://codereview.chromium.org/3117026/diff/13002/20004#newcode130
chrome/browser/speech/endpointer/energy_endpointer.cc:130:
offset_confirm_dur_sec_ = params_.offset_window() -
params_.offset_confirm_dur();
overlength

[Satish, 2010-08-23 14:56:09]

Thanks. Replies below. I will submit this once Trausti has looked at it as well.

http://codereview.chromium.org/3117026/diff/13002/20003
File chrome/browser/speech/endpointer/endpointer_unittest.cc (right):

http://codereview.chromium.org/3117026/diff/13002/20003#newcode40
chrome/browser/speech/endpointer/endpointer_unittest.cc:40: const int kFrameSize
= kSampleRate / kFrameRate;  // 160 samples.
On 2010/08/23 12:02:02, joth wrote:
> 160 samples per frame.
> you could capture this as a StaticAssert , given the code below depends on it
> being > 100

Done.

http://codereview.chromium.org/3117026/diff/13002/20003#newcode78
chrome/browser/speech/endpointer/endpointer_unittest.cc:78: if (120 ==
frame_count) EXPECT_EQ(EP_PRE_SPEECH, ep_status);
On 2010/08/23 12:02:02, joth wrote:
> break line before the EXPECT...
> (x3)

Done.

http://codereview.chromium.org/3117026/diff/13002/20003#newcode103
chrome/browser/speech/endpointer/endpointer_unittest.cc:103: for (int
frame_count = 0; frame_count < kNumFrames; ++frame_count) {
On 2010/08/23 12:02:02, joth wrote:
> this looks almost identical to the above test. Can we factor it out?
> maybe use a templated function to test the 2 api layers.
> 

Couldn't use a templated function but moved it to a separate function which
takes a 'FrameProcessor' class and the two tests now implement FrameProcessor.

http://codereview.chromium.org/3117026/diff/13002/20003#newcode123
chrome/browser/speech/endpointer/endpointer_unittest.cc:123: if (20 ==
frame_count) EXPECT_EQ(EP_PRE_SPEECH, ep_status);
On 2010/08/23 12:02:02, joth wrote:
> break line before the EXPECT...
> (x3)

Done.

http://codereview.chromium.org/3117026/diff/13002/20003#newcode129
chrome/browser/speech/endpointer/endpointer_unittest.cc:129: }
On 2010/08/23 12:02:02, joth wrote:
> i can't easily tell, does this cover testing wrap around on the internal ring
> buffer?
> you could pre-fill the buffer with size()-1 of junk samples, to test this
case?

I don't know really, since I don't understand the innards of the algorithm.
These tests were adopted directly from the original code so I'd prefer not
changing the logic unless we  understand the original code.

http://codereview.chromium.org/3117026/diff/13002/20004
File chrome/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/13002/20004#newcode4
chrome/browser/speech/endpointer/energy_endpointer.cc:4: 
On 2010/08/23 12:02:02, joth wrote:
> add brief desc & link to source xref

Done.

http://codereview.chromium.org/3117026/diff/13002/20004#newcode28
chrome/browser/speech/endpointer/energy_endpointer.cc:28: static int64
Secs2Usecs(float seconds) {
On 2010/08/23 12:02:02, joth wrote:
> no need for static inside anon namespace

Done.

http://codereview.chromium.org/3117026/diff/13002/20004#newcode130
chrome/browser/speech/endpointer/energy_endpointer.cc:130:
offset_confirm_dur_sec_ = params_.offset_window() -
params_.offset_confirm_dur();
On 2010/08/23 12:02:02, joth wrote:
> overlength

Done.

[trausti, 2010-08-23 15:04:06]

This endpointer was designed to take a little bit of audio (at least 200ms)
prior to speech for estimation of the backround noise level. This allows it to
be more robust to background noise which will be a problem for desktop
applications.

I'm concerned about not giving the endpointer some initial environment
estimation frames, even for a demo. It even looks like that the
decision_threshold_ doesn't get initialized in this case.

http://codereview.chromium.org/3117026/diff/13002/20004
File chrome/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/13002/20004#newcode142
chrome/browser/speech/endpointer/energy_endpointer.cc:142: // The level of the
first frame will overwrite these values.
This code assumes that the endpointer gets some environment estimation frames.
The parameters are set for 200ms i.e. 20 frames.  If you don't do that, then it
looks like decision_threshold_ will not initialized. I think it should be set
here to be a little more robust to misuse. i.e.
decision_threshold_ = params_.decision_threshold();

[Satish, 2010-08-23 15:18:07]

Thanks.

http://codereview.chromium.org/3117026/diff/13002/20004
File chrome/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/13002/20004#newcode142
chrome/browser/speech/endpointer/energy_endpointer.cc:142: // The level of the
first frame will overwrite these values.
On 2010/08/23 15:04:06, trausti wrote:
> This code assumes that the endpointer gets some environment estimation frames.
> The parameters are set for 200ms i.e. 20 frames.  If you don't do that, then
it
> looks like decision_threshold_ will not initialized. I think it should be set
> here to be a little more robust to misuse. i.e.
> decision_threshold_ = params_.decision_threshold();

When I tested, I saw EnergyEndpointer::Init() calling Restart(true) which sets
decision_threshold_ as you suggested above. So seems like that is sufficient?

[trausti, 2010-08-23 16:09:28]

HI,

I suggest adding a bit of text to the header to describe how to set up the
endpointer, and I suggest changing the logic so that the decision_threshold_ is
updated even if the user doesn't run the endpointer in the EnvironemntEstimation
mode initially.

http://codereview.chromium.org/3117026/diff/36001/21005
File chrome/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/36001/21005#newcode342
chrome/browser/speech/endpointer/energy_endpointer.cc:342: if
(estimating_environment_) {
Change this to:
if (estimating_environment_ || (frame_counter_ < fast_update_frames_)) {

This way, the behavior is predictable, even if the endpointer isn't given some
initial frames for updating the environment noise level.

http://codereview.chromium.org/3117026/diff/36001/21006
File chrome/browser/speech/endpointer/energy_endpointer.h (right):

http://codereview.chromium.org/3117026/diff/36001/21006#newcode22
chrome/browser/speech/endpointer/energy_endpointer.h:22: //
The levels of the speech and background noise are continuously updated. It is
important that the background noise level be estimated initially for robustness
in noisy conditions. The first frames are assumed to be background noise and a
fast update rate is used for the noise level. The duration for fast update is
controlled by the fast_update_dur_ paramter.

If used in noisy conditions, the endpointer should be started and run in the
EnvironmentEstimation mode, for at least 200ms, before switching to
UserInputMode.

[Satish, 2010-08-23 21:47:27]

Added comment to endpointer header from the original code.

http://codereview.chromium.org/3117026/diff/36001/21005
File chrome/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/36001/21005#newcode342
chrome/browser/speech/endpointer/energy_endpointer.cc:342: if
(estimating_environment_) {
On 2010/08/23 16:09:29, trausti wrote:
> Change this to:
> if (estimating_environment_ || (frame_counter_ < fast_update_frames_)) {
> 
> This way, the behavior is predictable, even if the endpointer isn't given some
> initial frames for updating the environment noise level.

Done.

http://codereview.chromium.org/3117026/diff/36001/21006
File chrome/browser/speech/endpointer/energy_endpointer.h (right):

http://codereview.chromium.org/3117026/diff/36001/21006#newcode22
chrome/browser/speech/endpointer/energy_endpointer.h:22: //
On 2010/08/23 16:09:29, trausti wrote:
> The levels of the speech and background noise are continuously updated. It is
> important that the background noise level be estimated initially for
robustness
> in noisy conditions. The first frames are assumed to be background noise and a
> fast update rate is used for the noise level. The duration for fast update is
> controlled by the fast_update_dur_ paramter.
> 
> If used in noisy conditions, the endpointer should be started and run in the
> EnvironmentEstimation mode, for at least 200ms, before switching to
> UserInputMode.

Sorry I am not sure if you are suggesting I add this text to the comment, or if
this is a review comment.

[jorlow, 2010-08-24 08:59:14]

I only skimmed, but didn't see any major issues.  So I'll deffer to Joth and
Trausti on this one.

http://codereview.chromium.org/3117026/diff/40001/25007
File chrome/browser/speech/endpointer/energy_endpointer_params.h (right):

http://codereview.chromium.org/3117026/diff/40001/25007#newcode39
chrome/browser/speech/endpointer/energy_endpointer_params.h:39: void operator =
(const EnergyEndpointerParams& source) {
Do we usually have a space before the =?

[Satish, 2010-08-24 09:01:56]

http://codereview.chromium.org/3117026/diff/40001/25007
File chrome/browser/speech/endpointer/energy_endpointer_params.h (right):

http://codereview.chromium.org/3117026/diff/40001/25007#newcode39
chrome/browser/speech/endpointer/energy_endpointer_params.h:39: void operator =
(const EnergyEndpointerParams& source) {
On 2010/08/24 08:59:17, jorlow wrote:
> Do we usually have a space before the =?

Looks like most code in chrome has no spaces before or after the operator, so it
is like "void operator=(const ...)". I'll remove the spaces here in the next
patch if Trausti has  more comments.

[trausti, 2010-08-24 16:20:00]

>
> Sorry I am not sure if you are suggesting I add this text to the
> comment, or if this is a review comment.
>


Please add this to the comments. I'll approve when that is in.

Cheers,
T.

[Satish, 2010-08-24 16:25:24]

Added.

[trausti, 2010-08-24 17:29:24]

Can you point me to the changes? I don't see them in the previous
links you sent.

Thanks,
T.

On Tue, Aug 24, 2010 at 9:25 AM,  <satish@chromium.org> wrote:
> Added.
>
> http://codereview.chromium.org/3117026/show
>

[jorlow, 2010-08-24 17:34:24]

http://codereview.chromium.org/3117026/diff2/40001:43001/39006

On Tue, Aug 24, 2010 at 6:29 PM, Trausti Kristjansson <trausti@google.com>wrote:

> Can you point me to the changes? I don't see them in the previous
> links you sent.
>
> Thanks,
> T.
>
> On Tue, Aug 24, 2010 at 9:25 AM,  <satish@chromium.org> wrote:
> > Added.
> >
> > http://codereview.chromium.org/3117026/show
> >
>

[trausti, 2010-08-24 18:29:31]

Ok Great.

I trust you will holler if things are not working as expected :-)

Cheers,
Trausti

http://codereview.chromium.org/3117026/diff/13002/20004
File chrome/browser/speech/endpointer/energy_endpointer.cc (right):

http://codereview.chromium.org/3117026/diff/13002/20004#newcode142
chrome/browser/speech/endpointer/energy_endpointer.cc:142: // The level of the
first frame will overwrite these values.
ok that's right.

On 2010/08/23 15:18:07, Satish wrote:
> On 2010/08/23 15:04:06, trausti wrote:
> > This code assumes that the endpointer gets some environment estimation
frames.
> > The parameters are set for 200ms i.e. 20 frames.  If you don't do that, then
> it
> > looks like decision_threshold_ will not initialized. I think it should be
set
> > here to be a little more robust to misuse. i.e.
> > decision_threshold_ = params_.decision_threshold();
> 
> When I tested, I saw EnergyEndpointer::Init() calling Restart(true) which sets
> decision_threshold_ as you suggested above. So seems like that is sufficient?

[tfarina, 2010-08-24 20:41:37]

After the fact!

http://codereview.chromium.org/3117026/diff/48001/49008
File chrome/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/3117026/diff/48001/49008#newcode11
chrome/browser/speech/speech_recognizer.h:11: #include
"chrome/browser/speech/speech_recognition_request.h"
wow!

Please, could you sort these alphabetical?

http://codereview.chromium.org/3117026/diff/48001/49008#newcode13
chrome/browser/speech/speech_recognizer.h:13: #include <list>
These headers should be included before "base/ref_counted.h"