Adds SpeechRecognizer which provides a simple interface to record and recognize speech.

Adds SpeechRecognizer which provides a simple interface to record and recognize speech.
Also added a unit test for checking the callbacks fire as expected.
TEST=unit_tests --gtest_filter=SpeechRecognizerTest.*
BUG=none

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

[joth, 2010-08-11 11:30:40]

LG - couple suggestions

http://codereview.chromium.org/3124009/diff/1/2
File chrome/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/3124009/diff/1/2#newcode41
chrome/browser/speech/speech_recognizer.cc:41: AudioManager::AUDIO_PCM_LINEAR,
1, AudioManager::kTelephoneSampleRate, 16,
comment the 1 & 16? (and the units of the result of the expression)

http://codereview.chromium.org/3124009/diff/1/2#newcode50
chrome/browser/speech/speech_recognizer.cc:50: void
SpeechRecognizer::StopRecording(bool abortRecognition) {
abort_recording

http://codereview.chromium.org/3124009/diff/1/2#newcode50
chrome/browser/speech/speech_recognizer.cc:50: void
SpeechRecognizer::StopRecording(bool abortRecognition) {
might be worth a DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)) here (and
in all the API methods?)

http://codereview.chromium.org/3124009/diff/1/2#newcode52
chrome/browser/speech/speech_recognizer.cc:52: bool sendRecognitionDone =
(abortRecognition &&
both vars: send_recxxxx_done

first one could be const too

i think the second would be clearer as notify_xxx (or needs_xxxx_notification) 
as it's not 'sending' anything over the network.

http://codereview.chromium.org/3124009/diff/1/2#newcode53
chrome/browser/speech/speech_recognizer.cc:53: (audio_controller_.get() ||
request_.get()));
I wonder if we should we assert either audio controller of request exist.
normally it's nice to have stop/cancel be a no-opt if not started, however as we
guarantee to callback DidRecognitionComplete at the end of  a [valid] sequence,
I can imagine confusion if you don't get this callback when calling Stop after
an unsuccessful Start() call, for example.

http://codereview.chromium.org/3124009/diff/1/2#newcode62
chrome/browser/speech/speech_recognizer.cc:62:
delegate_->DidCompleteRecording(render_view_id_);
the other locations you guard against delete in the callback. do you need it
here too? would need some way to abort the rest of this method too (seems messy)
or can these callbacks be deferred & batched up?

http://codereview.chromium.org/3124009/diff/1/2#newcode64
chrome/browser/speech/speech_recognizer.cc:64: if (abortRecognition) {
I wonder if this block would be easier to read as:
<calculate total buffer length>
if (abort || length == 0) {
  request_.reset();
  delegate_->DidCompleteRecognition();
  return;
}

// now kick off request, and we know we do not need any callback to
DidCompleteRecognition

http://codereview.chromium.org/3124009/diff/1/2#newcode84
chrome/browser/speech/speech_recognizer.cc:84: std::string data(length, char());
this needlessly zero initializes the string
how about
string data;
data.reserve(length);
for each audio buffer:
  data.append(*it);

this is much easier if audio_ is a deque of strings :-)

http://codereview.chromium.org/3124009/diff/1/2#newcode102
chrome/browser/speech/speech_recognizer.cc:102: }
else?
recording completed, but we already had an http request. Seems odd (invalid)
situation to be in?

http://codereview.chromium.org/3124009/diff/1/2#newcode130
chrome/browser/speech/speech_recognizer.cc:130: // accessing the data.
really need this even without the streaming POST, as the client may call Stop()
whist we're in this callback (thereby making a collision on the update to the
deque)
(aside: in the streaming case we'll need to post a task to the IO thread to pump
the upload stream, which may as well take a copy of the data as a param thereby
eliminating the need for the deque or any locking)

http://codereview.chromium.org/3124009/diff/1/2#newcode133
chrome/browser/speech/speech_recognizer.cc:133: buffer.resize(size);
again this needlessly zero initializes the vector.
buffer.assign(data, data + size);

(or if it's a deque of string:
buffer.assign(reintepret_cast<const char*>(data), size);

http://codereview.chromium.org/3124009/diff/1/3
File chrome/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/3124009/diff/1/3#newcode30
chrome/browser/speech/speech_recognizer.h:30: virtual void
DidCompleteRecording(int render_view_id) = 0;
comment what this call back means. 
Also call out the fact this may be called-back synchronously from within a
StopRecording() call, and unlike didcompleterecognition, can't be used to cancel
/ free

http://codereview.chromium.org/3124009/diff/1/3#newcode43
chrome/browser/speech/speech_recognizer.h:43: // Starts audio recording and does
recognition after recording ends.
can you call this a second time, after the previous one completes, or is this
class single use?

http://codereview.chromium.org/3124009/diff/1/3#newcode48
chrome/browser/speech/speech_recognizer.h:48: void StopRecording(bool
abortRecognition);
abort_recognition

http://codereview.chromium.org/3124009/diff/1/3#newcode66
chrome/browser/speech/speech_recognizer.h:66: AudioBufferQueue audio_;  //
Buffer holding the audio as it gets recorded.
perhaps audio_buffers_ ?

http://codereview.chromium.org/3124009/diff/1/3#newcode73
chrome/browser/speech/speech_recognizer.h:73: // This typedef is to workaround
the issue with certain versions of
which typedef?

http://codereview.chromium.org/3124009/diff/1/4
File chrome/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/3124009/diff/1/4#newcode83
chrome/browser/speech/speech_recognizer_unittest.cc:83: // Normal case where
recording is stopped before any audio data was received.
comment wrong

http://codereview.chromium.org/3124009/diff/1/4#newcode87
chrome/browser/speech/speech_recognizer_unittest.cc:87: // Start recording, give
some data and then abort.
Don't think you mean abort? looks like stop (no abort).

could add an abort case too (i.e. check that no request is created)

[Satish, 2010-08-11 15:15:25]

Thanks for the quick review, the comments were very helpful. Some replies below,
and the code and tests have changed quite a lot. Please take a new look.

http://codereview.chromium.org/3124009/diff/1/2
File chrome/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/3124009/diff/1/2#newcode53
chrome/browser/speech/speech_recognizer.cc:53: (audio_controller_.get() ||
request_.get()));
On 2010/08/11 11:30:40, joth wrote:
> I wonder if we should we assert either audio controller of request exist.
> normally it's nice to have stop/cancel be a no-opt if not started, however as
we
> guarantee to callback DidRecognitionComplete at the end of  a [valid]
sequence,
> I can imagine confusion if you don't get this callback when calling Stop after
> an unsuccessful Start() call, for example.

I prefer to err on the cautious side here and return normally since we have
plenty of layers and IPC between here and the html page, so for e.g. before the
page received a DidCompleteRecording event the user may accidentally press the
stop button twice resulting in two of these calls.

http://codereview.chromium.org/3124009/diff/1/2#newcode62
chrome/browser/speech/speech_recognizer.cc:62:
delegate_->DidCompleteRecording(render_view_id_);
On 2010/08/11 11:30:40, joth wrote:
> the other locations you guard against delete in the callback. do you need it
> here too? would need some way to abort the rest of this method too (seems
messy)
> or can these callbacks be deferred & batched up?

Not required here since the object should not be destroyed in this call,
SetRecognitionResult and DidCompleteRecognition callbacks are yet to be sent.
This is documented in the delegate declaration

http://codereview.chromium.org/3124009/diff/1/2#newcode64
chrome/browser/speech/speech_recognizer.cc:64: if (abortRecognition) {
On 2010/08/11 11:30:40, joth wrote:
> I wonder if this block would be easier to read as:
> <calculate total buffer length>
> if (abort || length == 0) {
>   request_.reset();
>   delegate_->DidCompleteRecognition();
>   return;
> }
> 
> // now kick off request, and we know we do not need any callback to
> DidCompleteRecognition

Good idea, I've made this change.

http://codereview.chromium.org/3124009/diff/1/2#newcode130
chrome/browser/speech/speech_recognizer.cc:130: // accessing the data.
On 2010/08/11 11:30:40, joth wrote:
> really need this even without the streaming POST, as the client may call
Stop()
> whist we're in this callback (thereby making a collision on the update to the
> deque)
> (aside: in the streaming case we'll need to post a task to the IO thread to
pump
> the upload stream, which may as well take a copy of the data as a param
thereby
> eliminating the need for the deque or any locking)
> 

Good idea, I've changed this to allocate a std::string and post a task now.
|audio_buffers_ | is now a list since there is no need for a deque, and its
members are std::string* since a scoped_ptr could not be placed in these
containers.

http://codereview.chromium.org/3124009/diff/1/3
File chrome/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/3124009/diff/1/3#newcode43
chrome/browser/speech/speech_recognizer.h:43: // Starts audio recording and does
recognition after recording ends.
On 2010/08/11 11:30:40, joth wrote:
> can you call this a second time, after the previous one completes, or is this
> class single use?
> 

Yes can be reused, added comment

http://codereview.chromium.org/3124009/diff/1/3#newcode48
chrome/browser/speech/speech_recognizer.h:48: void StopRecording(bool
abortRecognition);
On 2010/08/11 11:30:40, joth wrote:
> abort_recognition 

I have split this into 2 methods - CancelRecognition() and StopRecording() since
I noticed there was some confusion on what is being done where and the boolean
parameter did not help in differentiating.

http://codereview.chromium.org/3124009/diff/1/4
File chrome/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/3124009/diff/1/4#newcode87
chrome/browser/speech/speech_recognizer_unittest.cc:87: // Start recording, give
some data and then abort.
On 2010/08/11 11:30:40, joth wrote:
> Don't think you mean abort? looks like stop (no abort).
> 
> could add an abort case too (i.e. check that no request is created)
> 

No I did mean abort, as indicated by the "StopRecording(true)" call below. To
clarify I have added CancelRecognition and StopRecording methods now to
SpeechRecognizer.

[Paweł Hajdan Jr., 2010-08-11 17:03:06]

Drive-by with some test nits. Please let me take another look before committing.

http://codereview.chromium.org/3124009/diff/7001/8003
File chrome/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/3124009/diff/7001/8003#newcode47
chrome/browser/speech/speech_recognizer_unittest.cc:47: result_received_ =
false;
nit: Why not do the initializations in the ctor's initializer list?

http://codereview.chromium.org/3124009/diff/7001/8003#newcode98
chrome/browser/speech/speech_recognizer_unittest.cc:98: controller =
audio_input_controller_factory_.controller();
nit: Duplicated code from above?

http://codereview.chromium.org/3124009/diff/7001/8003#newcode103
chrome/browser/speech/speech_recognizer_unittest.cc:103: EXPECT_EQ(true,
recording_complete_);
nit: The right way to say it is EXPECT_TRUE, also fix code below.

[Satish, 2010-08-11 17:06:36]

http://codereview.chromium.org/3124009/diff/7001/8003
File chrome/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/3124009/diff/7001/8003#newcode47
chrome/browser/speech/speech_recognizer_unittest.cc:47: result_received_ =
false;
On 2010/08/11 17:03:06, Paweł Hajdan Jr. wrote:
> nit: Why not do the initializations in the ctor's initializer list?

Constructor gets invoked only once, Setup() gets invoked at the start of each
test (5 tests in this file) and these variables need to be initialised for each
test

http://codereview.chromium.org/3124009/diff/7001/8003#newcode98
chrome/browser/speech/speech_recognizer_unittest.cc:98: controller =
audio_input_controller_factory_.controller();
On 2010/08/11 17:03:06, Paweł Hajdan Jr. wrote:
> nit: Duplicated code from above?

will fix.

http://codereview.chromium.org/3124009/diff/7001/8003#newcode103
chrome/browser/speech/speech_recognizer_unittest.cc:103: EXPECT_EQ(true,
recording_complete_);
On 2010/08/11 17:03:06, Paweł Hajdan Jr. wrote:
> nit: The right way to say it is EXPECT_TRUE, also fix code below.

will fix.

[joth, 2010-08-12 13:27:11]

LG. Just one design question on whether there's a specific need to make
callbacks from within cancel?

http://codereview.chromium.org/3124009/diff/7001/8001
File chrome/browser/speech/speech_recognizer.cc (right):

http://codereview.chromium.org/3124009/diff/7001/8001#newcode37
chrome/browser/speech/speech_recognizer.cc:37: }
ReleaseAudioBuffers(); (or DCHECK it's empty)

http://codereview.chromium.org/3124009/diff/7001/8001#newcode89
chrome/browser/speech/speech_recognizer.cc:89: if (cancel_recognition ||
audio_buffer_length == 0) {
as we now ensure empty buffers are never added to the buffer queue we could just
say if (cancel_recognition || audio_buffers_.empty())

http://codereview.chromium.org/3124009/diff/7001/8001#newcode130
chrome/browser/speech/speech_recognizer.cc:130: NewRunnableMethod(this,
&SpeechRecognizer::HandleOnError, error_code));
line up NewRunableMethod with the open paren

http://codereview.chromium.org/3124009/diff/7001/8001#newcode145
chrome/browser/speech/speech_recognizer.cc:145: NewRunnableMethod(this,
&SpeechRecognizer::HandleOnData, str_data));
line up NewRunableMethod with the open paren

Note str_data will be leaked if the IO thread is closing down when this is
called. I guess that's not a big deal in this specific case...

http://codereview.chromium.org/3124009/diff/7001/8002
File chrome/browser/speech/speech_recognizer.h (right):

http://codereview.chromium.org/3124009/diff/7001/8002#newcode57
chrome/browser/speech/speech_recognizer.h:57: // gets discarded.
Maybe called at any time.
If a recognition is currently in progress Delegate::DidCompleteRecognition will
be called back from within this call.

Aside: I normally find Cancel() type methods much easier to use if they
guarantee no more callbacks will be made. It means they are immediately safe to
use from the client's destructor without having to put any special-case logic in
the callback handler to ensure we're not midway through destruction.

http://codereview.chromium.org/3124009/diff/7001/8002#newcode85
chrome/browser/speech/speech_recognizer.h:85: AudioBufferQueue audio_buffers_; 
// Buffer holding the recorded audio.
// Owns the strings inside the list.

[Paweł Hajdan Jr., 2010-08-12 20:26:08]

http://codereview.chromium.org/3124009/diff/7001/8003
File chrome/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/3124009/diff/7001/8003#newcode47
chrome/browser/speech/speech_recognizer_unittest.cc:47: result_received_ =
false;
On 2010/08/11 17:06:37, Satish wrote:
> On 2010/08/11 17:03:06, Paweł Hajdan Jr. wrote:
> > nit: Why not do the initializations in the ctor's initializer list?
> 
> Constructor gets invoked only once, Setup() gets invoked at the start of each
> test (5 tests in this file) and these variables need to be initialised for
each
> test

Not true. The test fixture is recreated for each individual test. Please refer
to the gtest documentation.

[Satish, 2010-08-12 20:34:18]

http://codereview.chromium.org/3124009/diff/7001/8003
File chrome/browser/speech/speech_recognizer_unittest.cc (right):

http://codereview.chromium.org/3124009/diff/7001/8003#newcode47
chrome/browser/speech/speech_recognizer_unittest.cc:47: result_received_ =
false;
On 2010/08/12 20:26:08, Paweł Hajdan Jr. wrote:
> On 2010/08/11 17:06:37, Satish wrote:
> > On 2010/08/11 17:03:06, Paweł Hajdan Jr. wrote:
> > > nit: Why not do the initializations in the ctor's initializer list?
> > 
> > Constructor gets invoked only once, Setup() gets invoked at the start of
each
> > test (5 tests in this file) and these variables need to be initialised for
> each
> > test
> 
> Not true. The test fixture is recreated for each individual test. Please refer
> to the gtest documentation.

You're correct, is it ok if I make this change in a future CL?