Binding media stream audio track to speech recognition [browser]

Chrome support for providing media stream audio track to speech recognition - browser implementation.

  WebKit CL      : http://crrev.com/448163003
  Renderer CL    : http://crrev.com/499233003
[ Browser CL ]

SpeechRecognitionAudioReceiver allocates and maps the SharedMemory for audio, prepares
the SyncSocket and passes those handles to the client which hands them down to the renderer via IPC. Renderer creates the |SpeechRecognitionAudioSink|
using those handles.

Audio data from the renderer gets delivered to the browser via SyncSocket and SharedMemory listening on a separate |audio_thread_|.

Data gets converted to audio chunks which then get passed
to the speech recognition engine (as the default implementation would).

Synchronization is done via |SyncSocket.Receive()| and incrementing the buffer index in shared memory. Making sure all audio is delivered in order.

Design doc: http://goo.gl/9Ot3PC

BUG=408940

[burnik, 2014-09-17 09:56:59]

Patchset #1 (id:1) has been deleted

[burnik, 2014-09-19 14:21:05]

Patchset #1 (id:20001) has been deleted

[burnik, 2014-09-26 09:54:28]

burnik@chromium.org changed reviewers:
+ hans@chromium.org, jochen@chromium.org, tommi@chromium.org, xians@chromium.org

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

first set of comments.  I think it would make sense for xians to review this
first and then give other reviewers a ping with information on what specifically
each reviewer should review.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.cc:161: //
TODO(burnik): This needs to be tested on Windows.
has this been done?

is there a unit test?

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:286:
base::ThreadRestrictions::SetIOAllowed(true);
can we avoid this synchronous operation by e.g. passing object ownership to a
closure that does this on a thread where blocking IO is allowed?

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:558: if (using_audio_track_)
{}

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:636: return false;
free shared_memory_?

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:660: return false;
free resources hitherto allocated?

same for all other early returns.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:773: if (!using_audio_track_)
CloseAudioControllerAsynchronously();
two lines

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognizer_impl.h (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.h:63: void
set_using_audio_track(bool using_audio_track) {
thread check?
(and move to cc file)

[no longer working on chromium, 2014-10-02 11:55:39]

xians@chromium.org changed reviewers:
+ primiano@chromium.org

[no longer working on chromium, 2014-10-02 11:55:40]

Some more work is needed.

+ Primiano.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:632: base::SharedMemoryHandle
foreign_memory_handle;
nit, add an empty line

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:638: media::AudioInputBuffer*
buffer =
nit add an empty line.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:645: // Reusing the audio
converter only to create chunks from the bus.
empty line.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:657: if
(!base::SyncSocket::CreatePair(audio_sync_socket_.get(),
move these sockets code right after the shared memory code, so we don't create
any resource upon failure.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:674:
audio_sync_thread_->Start();
why is it called audio_sync_thread_? how about audio_track_thread_ or simply
audio_thread_

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:677: if
(!audio_sync_thread_->message_loop_proxy()
add a DCHECK, like:
bool ret = audio_sync_thread_->message_loop_proxy()->PostTask(
    FROM_HERE,
    base::Bind(&SpeechRecognizerImpl::OnAudioTrackData,
base::Unretained(this)));
DCHECK(ret);

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:685:
listener()->OnAudioTrackReady(session_id(), params, foreign_memory_handle,
OnAudioTrackReady sounds a wrong name, change it to 
OnAudioStreamReady

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:699:
audio_sync_socket_->Receive(buffer_index_, sizeof(*buffer_index_));
Receive will block the thread. In case there is no data coming from the renderer
any more, you will wait infinitely here when shutting down the thread.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:703: return;
I think this is wrong, you silently stop the audio thread here when getting an
error from Receive(), you should either post a task to check it in a later state
or notify the client that the session fails. I prefer the first solution here.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:908:
DCHECK(!using_audio_track_);
what is the alternative of CloseAudioControllerAsynchronously() when the audio
track is being  used?

https://codereview.chromium.org/549373003/diff/60001/content/public/browser/s...
File content/public/browser/speech_recognition_event_listener.h (right):

https://codereview.chromium.org/549373003/diff/60001/content/public/browser/s...
content/public/browser/speech_recognition_event_listener.h:67: // TODO(burnik):
Make pure virtual and update the unit test.
you can address this TODO in this CL?

https://codereview.chromium.org/549373003/diff/60001/content/public/browser/s...
File content/public/browser/speech_recognition_session_config.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/public/browser/s...
content/public/browser/speech_recognition_session_config.cc:19:
max_hypotheses(kDefaultMaxHypotheses) { }
nit, you don't need to change this format.

[burnik, 2014-10-02 16:04:12]

Good review round. I would like to continue the discussion on a few matters.
Please read and reply to questions.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognition_dispatcher_host.cc:161: //
TODO(burnik): This needs to be tested on Windows.
On 2014/10/02 11:23:19, tommi wrote:
> has this been done?
> 
> is there a unit test?

Almost ready with recognizer impl unit test and the browser test. I guess FYI
bots can help with this?

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:286:
base::ThreadRestrictions::SetIOAllowed(true);
On 2014/10/02 11:23:19, tommi wrote:
> can we avoid this synchronous operation by e.g. passing object ownership to a
> closure that does this on a thread where blocking IO is allowed?

Ok, but what would happen if the object goes away (dtor completes)
before that posted task completes? Would the socket and thread still
shutdown/stop?

Is it good practice to post tasks in the destructor?
Or maybe I misunderstood, do you have an example?

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:558: if (using_audio_track_)
On 2014/10/02 11:23:19, tommi wrote:
> {}

Done.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:632: base::SharedMemoryHandle
foreign_memory_handle;
On 2014/10/02 11:55:40, xians1 wrote:
> nit, add an empty line

Done.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:636: return false;
On 2014/10/02 11:23:19, tommi wrote:
> free shared_memory_?

Added:
shared_memory_.reset();

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:638: media::AudioInputBuffer*
buffer =
On 2014/10/02 11:55:40, xians1 wrote:
> nit add an empty line.

Done.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:645: // Reusing the audio
converter only to create chunks from the bus.
On 2014/10/02 11:55:40, xians1 wrote:
> empty line.

Done.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:657: if
(!base::SyncSocket::CreatePair(audio_sync_socket_.get(),
On 2014/10/02 11:55:40, xians1 wrote:
> move these sockets code right after the shared memory code, so we don't create
> any resource upon failure.

Done.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:660: return false;
On 2014/10/02 11:23:19, tommi wrote:
> free resources hitherto allocated?
> 
> same for all other early returns.

Done.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:674:
audio_sync_thread_->Start();
On 2014/10/02 11:55:40, xians1 wrote:
> why is it called audio_sync_thread_? how about audio_track_thread_ or simply
> audio_thread_

Done. audio_thread_.

I think keyword "track" should be removed.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:677: if
(!audio_sync_thread_->message_loop_proxy()
On 2014/10/02 11:55:40, xians1 wrote:
> add a DCHECK, like:
> bool ret = audio_sync_thread_->message_loop_proxy()->PostTask(
>     FROM_HERE,
>     base::Bind(&SpeechRecognizerImpl::OnAudioTrackData,
> base::Unretained(this)));
> DCHECK(ret);

This will then be an unused variable in the Relase build.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:685:
listener()->OnAudioTrackReady(session_id(), params, foreign_memory_handle,
On 2014/10/02 11:55:40, xians1 wrote:
> OnAudioTrackReady sounds a wrong name, change it to 
> OnAudioStreamReady

How about OnSharedAudioBusReady or OnSharedBusReady?

Both stream and track are unknowns to this class...

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:688: return true;
Regardin the "bool ret":
I could return ret; here. What do you think?

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:699:
audio_sync_socket_->Receive(buffer_index_, sizeof(*buffer_index_));
On 2014/10/02 11:55:40, xians1 wrote:
> Receive will block the thread. In case there is no data coming from the
renderer
> any more, you will wait infinitely here when shutting down the thread.

This would indicate the renderer is not sending data but the socket is still
open.
That means the AudioSink is still alive but not sending anything. How??

How does AudioDeviceThread handle this?

Another problem would be that we don't wait enough with the timeout -- for
example when we signal back the renderer to create the AudioSink, if it takes
more time than we wait here, we will fail to receive and the session will fail.
This leads to multiple calls to Receive or Reposting the task, but then it's
unclear when we should stop trying to receive...

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:703: return;
On 2014/10/02 11:55:40, xians1 wrote:
> I think this is wrong, you silently stop the audio thread here when getting an
> error from Receive(), you should either post a task to check it in a later
state
> or notify the client that the session fails. I prefer the first solution here.

Are you sure about this? Can a socket receive again after it hadn't been able to
receive? I believe that a receive with 0 bytes indicates the socket is closed.

Second point would go into direction of stopping the recognition which then IPCs
the renderer. Do you think we should post an abort event? BTW, as I recall, this
occurs when page reloads or goes away.

Third question: Is it possible that a Receive could transfer less than 4B? --
the very reason for the DCHECK here.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:773: if (!using_audio_track_)
CloseAudioControllerAsynchronously();
On 2014/10/02 11:23:19, tommi wrote:
> two lines

Done.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:908:
DCHECK(!using_audio_track_);
On 2014/10/02 11:55:40, xians1 wrote:
> what is the alternative of CloseAudioControllerAsynchronously() when the audio
> track is being  used?

The audio track is managed by the renderer and the cleanup should be there
regarding audio.

We may clean up the thread and the socket right away but does that mean we
should do the same for other members?

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognizer_impl.h (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.h:63: void
set_using_audio_track(bool using_audio_track) {
On 2014/10/02 11:23:19, tommi wrote:
> thread check?
> (and move to cc file)

Done.

https://codereview.chromium.org/549373003/diff/60001/content/public/browser/s...
File content/public/browser/speech_recognition_event_listener.h (right):

https://codereview.chromium.org/549373003/diff/60001/content/public/browser/s...
content/public/browser/speech_recognition_event_listener.h:67: // TODO(burnik):
Make pure virtual and update the unit test.
On 2014/10/02 11:55:40, xians1 wrote:
> you can address this TODO in this CL?

Yes. This is already done. Will come along with the tests in upcoming patchsets.

https://codereview.chromium.org/549373003/diff/60001/content/public/browser/s...
File content/public/browser/speech_recognition_session_config.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/public/browser/s...
content/public/browser/speech_recognition_session_config.cc:19:
max_hypotheses(kDefaultMaxHypotheses) { }
On 2014/10/02 11:55:40, xians1 wrote:
> nit, you don't need to change this format.

Done.

[no longer working on chromium, 2014-10-03 08:57:53]

The new code in the SpeechRecognizerImpl is getting complicated and hard to
read, you should wrap all the socket and thread code into a new class (either a
sub class of SpeechRecognizerImpl or completely new class).

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:286:
base::ThreadRestrictions::SetIOAllowed(true);
On 2014/10/02 16:04:11, burnik wrote:
> On 2014/10/02 11:23:19, tommi wrote:
> > can we avoid this synchronous operation by e.g. passing object ownership to
a
> > closure that does this on a thread where blocking IO is allowed?
> 
> Ok, but what would happen if the object goes away (dtor completes)
> before that posted task completes? Would the socket and thread still
> shutdown/stop?
> 
> Is it good practice to post tasks in the destructor?
> Or maybe I misunderstood, do you have an example?

I think there are two ways doing that:
1) in browser_main_loop.cc, make sure speech_recognition_manager_ gets deleted
before file thread goes away, and post a task to the file thread, which is IO
allowed, there it closes the audio thread.
2)
speech_recognition_manager_ is depending on AudioManager, shuts the thread down
using the audio thread of the AudioManager.
Tommi, do you know if the audio thread of the AudioManager is IO allowed?

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:703: return;
On 2014/10/02 16:04:11, burnik wrote:
> On 2014/10/02 11:55:40, xians1 wrote:
> > I think this is wrong, you silently stop the audio thread here when getting
an
> > error from Receive(), you should either post a task to check it in a later
> state
> > or notify the client that the session fails. I prefer the first solution
here.
> 
> Are you sure about this? Can a socket receive again after it hadn't been able
to
> receive? I believe that a receive with 0 bytes indicates the socket is closed.
> 

Yes, I can sure that once you return here, the OnAudioTrackData will not be
executed any more. You can test it by adding logs there.

> Second point would go into direction of stopping the recognition which then
IPCs
> the renderer. Do you think we should post an abort event? BTW, as I recall,
this
> occurs when page reloads or goes away.
> 

You should test the code to see if the session will be closed when the page
reloads or goes away.
If yes, you don't need to post an abort event. Otherwise you might need to do
it.
Please test it first, then we start the discussion there.

> Third question: Is it possible that a Receive could transfer less than 4B? --
> the very reason for the DCHECK here.
If the renderer sends only 4B, then the receiver should receive only 4B or 0
(renderer socket is closed). Unless the renderer is hacked and sends different
bytes.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:908:
DCHECK(!using_audio_track_);
On 2014/10/02 16:04:11, burnik wrote:
> On 2014/10/02 11:55:40, xians1 wrote:
> > what is the alternative of CloseAudioControllerAsynchronously() when the
audio
> > track is being  used?
> 
> The audio track is managed by the renderer and the cleanup should be there
> regarding audio.
> 
> We may clean up the thread and the socket right away but does that mean we
> should do the same for other members?

The audio thread and its relevant resource like shared memory, sockets are
belonging to this SpeechRecognizerImpl, how can the renderer cleans up those
resource?

You need to do the same things as how CloseAudioControllerAsynchronously() does,
and closes all the resource when the session is closed.

[no longer working on chromium, 2014-10-03 09:13:18]

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:699:
audio_sync_socket_->Receive(buffer_index_, sizeof(*buffer_index_));
On 2014/10/02 16:04:11, burnik wrote:
> On 2014/10/02 11:55:40, xians1 wrote:
> > Receive will block the thread. In case there is no data coming from the
> renderer
> > any more, you will wait infinitely here when shutting down the thread.
> 
> This would indicate the renderer is not sending data but the socket is still
> open.
> That means the AudioSink is still alive but not sending anything. How??
> 
> How does AudioDeviceThread handle this?
> 
> Another problem would be that we don't wait enough with the timeout -- for
> example when we signal back the renderer to create the AudioSink, if it takes
> more time than we wait here, we will fail to receive and the session will
fail.
> This leads to multiple calls to Receive or Reposting the task, but then it's
> unclear when we should stop trying to receive...
> 
> 

It is better for you do some tests here. Like closing the tab, and print logs
here to see how it behaves.

If the socket gets closed automatically after some time, then we don't need the
timeout.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:703: return;
hmm, I actually answer my own question, if the socket should only receive 4B or
0 (socket is shutdown), then doing a return here is correct.

again, test the behaviours, and we can have the discussion after making sure
what happens there.

[burnik, 2014-10-03 09:45:55]

burnik@chromium.org changed reviewers:
- hans@chromium.org, jochen@chromium.org, primiano@chromium.org

[burnik, 2014-10-06 08:32:50]

Moved most of the code to SpeechRecognitionAudioReceiver. I would appreciate any
advice on how to stop the new audio thread without making it a synchronous
operation on the IO thread where this object is instantiated.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:286:
base::ThreadRestrictions::SetIOAllowed(true);
On 2014/10/03 08:57:53, xians1 wrote:
> On 2014/10/02 16:04:11, burnik wrote:
> > On 2014/10/02 11:23:19, tommi wrote:
> > > can we avoid this synchronous operation by e.g. passing object ownership
to
> a
> > > closure that does this on a thread where blocking IO is allowed?
> > 
> > Ok, but what would happen if the object goes away (dtor completes)
> > before that posted task completes? Would the socket and thread still
> > shutdown/stop?
> > 
> > Is it good practice to post tasks in the destructor?
> > Or maybe I misunderstood, do you have an example?
> 
> I think there are two ways doing that:
> 1) in browser_main_loop.cc, make sure speech_recognition_manager_ gets deleted
> before file thread goes away, and post a task to the file thread, which is IO
> allowed, there it closes the audio thread.
> 2)
> speech_recognition_manager_ is depending on AudioManager, shuts the thread
down
> using the audio thread of the AudioManager.
> Tommi, do you know if the audio thread of the AudioManager is IO allowed?

I've tried a few different approaches to avoid this.
1) Using audio managers task runner.
2) Using the BrowserThread::FILE task runner.

Both approaches produce same or similar issues.

The issue here is that we cannot allow the object(speech_recognizer_impl) to go
away before the thread completes. If we do so, we are freeing the members and
causing segfault in the thread task (crashing the browser with exit code -11 and
no stack trace). That's why this needs to be a synchronous operation.

Next patchset moves this to SpeechRecognitionAudioReceiver and does it more
carefully not to block for too long.

A way out would be I guess having a singleton which could be reused during the
browser process lifetime.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:699:
audio_sync_socket_->Receive(buffer_index_, sizeof(*buffer_index_));
On 2014/10/03 09:13:18, xians1 wrote:
> On 2014/10/02 16:04:11, burnik wrote:
> > On 2014/10/02 11:55:40, xians1 wrote:
> > > Receive will block the thread. In case there is no data coming from the
> > renderer
> > > any more, you will wait infinitely here when shutting down the thread.
> > 
> > This would indicate the renderer is not sending data but the socket is still
> > open.
> > That means the AudioSink is still alive but not sending anything. How??
> > 
> > How does AudioDeviceThread handle this?
> > 
> > Another problem would be that we don't wait enough with the timeout -- for
> > example when we signal back the renderer to create the AudioSink, if it
takes
> > more time than we wait here, we will fail to receive and the session will
> fail.
> > This leads to multiple calls to Receive or Reposting the task, but then it's
> > unclear when we should stop trying to receive...
> > 
> > 
> 
> It is better for you do some tests here. Like closing the tab, and print logs
> here to see how it behaves.
> 
> If the socket gets closed automatically after some time, then we don't need
the
> timeout.

Reimplemented as ReceiveWithTimeout in patchset 2.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:703: return;
On 2014/10/03 09:13:18, xians1 wrote:
> hmm, I actually answer my own question, if the socket should only receive 4B
or
> 0 (socket is shutdown), then doing a return here is correct.
> 
> again, test the behaviours, and we can have the discussion after making sure
> what happens there.
> 

Acknowledged.

https://codereview.chromium.org/549373003/diff/60001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.cc:908:
DCHECK(!using_audio_track_);
On 2014/10/03 08:57:53, xians1 wrote:
> On 2014/10/02 16:04:11, burnik wrote:
> > On 2014/10/02 11:55:40, xians1 wrote:
> > > what is the alternative of CloseAudioControllerAsynchronously() when the
> audio
> > > track is being  used?
> > 
> > The audio track is managed by the renderer and the cleanup should be there
> > regarding audio.
> > 
> > We may clean up the thread and the socket right away but does that mean we
> > should do the same for other members?
> 
> The audio thread and its relevant resource like shared memory, sockets are
> belonging to this SpeechRecognizerImpl, how can the renderer cleans up those
> resource?
> 
> You need to do the same things as how CloseAudioControllerAsynchronously()
does,
> and closes all the resource when the session is closed.

Acknowledged.

[burnik, 2014-10-12 20:30:39]

Refactor SpeechRecognitionAudioReceiver

[burnik, 2014-10-12 20:45:00]

Patchset #4 (id:100001) has been deleted

[burnik, 2014-10-12 20:46:00]

Refactor SpeechRecognitionAudioReceiver

[burnik, 2014-10-12 21:04:26]

Revert recognizer impl unit test + nits

[burnik, 2014-10-12 23:23:22]

Remove non-effective PostDelayedTask

[burnik, 2014-10-12 23:28:59]

Include headers - fix for trybot builds

[burnik, 2014-10-13 00:55:33]

Fix recognizer impl unit test on Win

[burnik, 2014-10-14 13:56:42]

Audio Receiver unit test

[no longer working on chromium, 2014-10-15 12:14:29]

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.cc (right):

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:17:
SpeechRecognitionAudioReceiver* SpeechRecognitionAudioReceiver::Create(
use scoped_ptr<SpeechRecognitionAudioReceiver>

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:167: size_t
bytes_received = receiving_socket_->ReceiveWithTimeout(
I think the best solution here is to use a short timeout and PostDelayedTask.

For example:
// Use 3ms as timeout.
static const base::TimeDelta kReceiveTimeout =
base::TimeDelta::FromMilliseconds(3);
size_t bytes_received = receiving_socket_->ReceiveWithTimeout(..)
const base::TimeDelta next_receive_time;
if (bytes_received == kSize) {
  ..

  // Schedule next receive to be 90ms later.
  next_receive_time = base::TimeDelta::FromMilliseconds(90);
} else {
  ....
  // Schedule next receive to be 5ms later.
  next_receive_time = base::TimeDelta::FromMilliseconds(5);
}

PostDelayedTask(FROM_HERE, base::Bind(&SpeechRecognitionAudioReceiver::Receive,
weak_ptr_factory_.GetWeakPtr()), next_receive_time);

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.h (right):

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.h:76: void
set_listening(const bool listening);
s/set_listening/SetListening/g,
hacker_style is preferred for one line of code implemented in the header file.

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.h:79: bool listening();
the same here

[no longer working on chromium, 2014-10-15 12:15:12]

It looks much better, probably we should discuss if we should use the audio
thread in AudioManager instead of making new audio thread.

[burnik, 2014-10-15 14:50:36]

xians' comments

[burnik, 2014-10-15 14:52:28]

Nits done.

Tommi: Any inputs or feedback would be appreciated.

https://codereview.chromium.org/549373003/diff/80001/content/browser/speech/s...
File content/browser/speech/speech_recognizer_impl.h (right):

https://codereview.chromium.org/549373003/diff/80001/content/browser/speech/s...
content/browser/speech/speech_recognizer_impl.h:47:
SpeechRecognizerImpl(SpeechRecognitionEventListener* listener, int session_id,
Will move each param on it's own line.

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.cc (right):

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:17:
SpeechRecognitionAudioReceiver* SpeechRecognitionAudioReceiver::Create(
On 2014/10/15 12:14:28, xians1 wrote:
> use scoped_ptr<SpeechRecognitionAudioReceiver>

Done.

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:167: size_t
bytes_received = receiving_socket_->ReceiveWithTimeout(
On 2014/10/15 12:14:28, xians1 wrote:
> I think the best solution here is to use a short timeout and PostDelayedTask.
> 
> For example:
> // Use 3ms as timeout.
> static const base::TimeDelta kReceiveTimeout =
> base::TimeDelta::FromMilliseconds(3);
> size_t bytes_received = receiving_socket_->ReceiveWithTimeout(..)
> const base::TimeDelta next_receive_time;
> if (bytes_received == kSize) {
>   ..
> 
>   // Schedule next receive to be 90ms later.
>   next_receive_time = base::TimeDelta::FromMilliseconds(90);
> } else {
>   ....
>   // Schedule next receive to be 5ms later.
>   next_receive_time = base::TimeDelta::FromMilliseconds(5);
> }
> 
> PostDelayedTask(FROM_HERE,
base::Bind(&SpeechRecognitionAudioReceiver::Receive,
> weak_ptr_factory_.GetWeakPtr()), next_receive_time);

Yeah, I've tried that approach. The delay seems not to occur. I'll try it again
to see if it only applies to either browser thread or the audio thread. However,
it is a bit tricky.

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.h (right):

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.h:76: void
set_listening(const bool listening);
On 2014/10/15 12:14:28, xians1 wrote:
> s/set_listening/SetListening/g,
> hacker_style is preferred for one line of code implemented in the header file.

Done. However, this confuses me a bit according to the Google C++ style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

https://codereview.chromium.org/549373003/diff/800001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.h:79: bool listening();
On 2014/10/15 12:14:29, xians1 wrote:
> the same here

Done.

[burnik, 2014-10-16 16:26:07]

Move thread to recognizer_impl. Use PostDelayedTask.

[tommi (sloooow) - chröme, 2014-10-16 21:20:23]

will take a closer look tomorrow

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.cc (right):

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:63: Stop();
is this method only called internally? (if so, can we make it private?)

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:621: audio_thread_.reset(new
base::Thread("AudioReceiverThread"));
what if this is already set? (should we at least DCHECK that it's null first?)

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:623: DLOG(ERROR) << "Cannot
start new audio thread";
free audio_thread_?

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:673: const bool
previous_io_allowed = base::ThreadRestrictions::SetIOAllowed(true);
ScopedAllowIO

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
File content/browser/speech/speech_recognizer_impl.h (right):

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.h:21: class Thread;
no indent

[burnik, 2014-10-17 10:46:56]

Tommi's comments

[burnik, 2014-10-17 10:48:02]

One or two open questions. 
Prepping the unit test to support the audio thread as well.

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.cc (right):

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:63: Stop();
On 2014/10/16 21:20:23, tommi wrote:
> is this method only called internally? (if so, can we make it private?)

It's called by the client as well. Maybe just put a DCHECK(!IsListening()) here?

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:621: audio_thread_.reset(new
base::Thread("AudioReceiverThread"));
On 2014/10/16 21:20:23, tommi wrote:
> what if this is already set? (should we at least DCHECK that it's null first?)

Done. However:
That would indicate the FSM contract is broken, would fail earlier when checking
if audio is being captured.

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:623: DLOG(ERROR) << "Cannot
start new audio thread";
On 2014/10/16 21:20:23, tommi wrote:
> free audio_thread_?

Done.

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:673: const bool
previous_io_allowed = base::ThreadRestrictions::SetIOAllowed(true);
On 2014/10/16 21:20:23, tommi wrote:
> ScopedAllowIO

Unfortunately, ScopedAllowIO is deprecated and is part of the pre-submit checks
which block the CL. I realize this is usually discouraged, but here it's
commented and does not really block.

Side note:
Starting and stopping the thread on the BrowserThread::FILE would make this very
complicated (async callbacks calling async callbacks) and would probably require
a lock which would again lead us back to a synchronous operation. If the rest of
the class was implemented on the FILE thread, we would have no such issue
(although other problems would most probably emerge).

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
File content/browser/speech/speech_recognizer_impl.h (right):

https://codereview.chromium.org/549373003/diff/840001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.h:21: class Thread;
On 2014/10/16 21:20:23, tommi wrote:
> no indent

Done.

[no longer working on chromium, 2014-10-17 12:28:11]

Looking better, some comments.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.cc (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:18: const
media::AudioParameters& audio_params,
fix the indentation.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:138: void
SpeechRecognitionAudioReceiver::Stop() {
you need to invalidate the weak pointer in this method.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:157: static const
size_t kSize = sizeof(media::AudioInputBufferParameters().size);
use sizeof(GetAudioBuffer()->params.size)

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:196: const
base::TimeDelta time_delta = base::TimeTicks::Now() - last_receive_time;
I am a bit confused here, what does this time_delta mean? last_receive_time is
declared and assigned to base::TimeTicks::Now() in line 166, time_delta will be
very small since there is no operation between last_receive_time and
base::TimeTicks::Now()

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.h (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.h:52: void Listen();
s/Listen()/StartListening()/g

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_dispatcher_host.cc:281: const
media::AudioParameters& params,
include what are being used: media::AudioParameters and base::SharedMemoryHandle

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_manager_impl.cc:438: const
base::SharedMemoryHandle memory,
same here.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:66: bool need_conversion_;
I don't think you need this extra flag, just change audio_converter to use
scoped_ptr

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:134: need_conversion_ =
!input_parameters_.Equals(output_parameters_);
just do:
if (!input_params.Equals(output_params)) {
  audio_converter_.reset(new AudioConverter(input_params, output_params,
false));
  audio_converter_->AddInput(this);
}

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:140:
audio_converter_.RemoveInput(this);
remember to add if (audio_converter_)

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:149: if (need_conversion_) {
if (audio_converter_) {

} else {
  data->CopyTo(output_bus_.get());
}

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:283: ResetAudioReceiver();
do we create audio_controller_ when AudioReceiver is being used?

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:556: :
Abort(SpeechRecognitionError(SPEECH_RECOGNITION_ERROR_AUDIO));
I think the format should be:
return StartRecordingUsingAudioReceiver(output_parameters) ?
    STATE_STARTING :
Abort(SpeechRecognitionError(SPEECH_RECOGNITION_ERROR_AUDIO));

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:642: ResetAudioReceiver();
ResetAudioReceiver() will call audio_receiver_->Stop(), so it will crash.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:677: audio_thread_->Stop();
I don't think you guarantee the audio_thread_ and audio_receiver_ are not NULL
when this method is called?

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognizer_impl_unittest.cc (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl_unittest.cc:33: class
SpeechRecognizerImplTest : public SpeechRecognitionEventListener,
you probably need to add at least one test case to this SpeechRecognizerImplTest
to test your code in the recognizer, like creating the receiver and destroy it.

[burnik, 2014-10-23 13:50:33]

Refactoring and nits

[burnik, 2014-10-23 13:56:37]

A bit of refactoring is done. The failing unit tests are still in this patch.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.cc (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:18: const
media::AudioParameters& audio_params,
On 2014/10/17 12:28:10, xians1 wrote:
> fix the indentation.

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:138: void
SpeechRecognitionAudioReceiver::Stop() {
On 2014/10/17 12:28:10, xians1 wrote:
> you need to invalidate the weak pointer in this method.

Moved away from the weak ptr factory.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:157: static const
size_t kSize = sizeof(media::AudioInputBufferParameters().size);
On 2014/10/17 12:28:10, xians1 wrote:
> use sizeof(GetAudioBuffer()->params.size)

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.cc:196: const
base::TimeDelta time_delta = base::TimeTicks::Now() - last_receive_time;
On 2014/10/17 12:28:10, xians1 wrote:
> I am a bit confused here, what does this time_delta mean? last_receive_time is
> declared and assigned to base::TimeTicks::Now() in line 166, time_delta will
be
> very small since there is no operation between last_receive_time and
> base::TimeTicks::Now()

Removed timing.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognition_audio_receiver.h (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_audio_receiver.h:52: void Listen();
On 2014/10/17 12:28:10, xians1 wrote:
> s/Listen()/StartListening()/g

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_dispatcher_host.cc:281: const
media::AudioParameters& params,
On 2014/10/17 12:28:10, xians1 wrote:
> include what are being used: media::AudioParameters and
base::SharedMemoryHandle

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognition_manager_impl.cc:438: const
base::SharedMemoryHandle memory,
On 2014/10/17 12:28:10, xians1 wrote:
> same here.

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
File content/browser/speech/speech_recognizer_impl.cc (right):

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:66: bool need_conversion_;
On 2014/10/17 12:28:11, xians1 wrote:
> I don't think you need this extra flag, just change audio_converter to use
> scoped_ptr

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:134: need_conversion_ =
!input_parameters_.Equals(output_parameters_);
On 2014/10/17 12:28:11, xians1 wrote:
> just do:
> if (!input_params.Equals(output_params)) {
>   audio_converter_.reset(new AudioConverter(input_params, output_params,
> false));
>   audio_converter_->AddInput(this);
> }

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:140:
audio_converter_.RemoveInput(this);
On 2014/10/17 12:28:11, xians1 wrote:
> remember to add if (audio_converter_)

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:149: if (need_conversion_) {
On 2014/10/17 12:28:10, xians1 wrote:
> if (audio_converter_) {
> 
> } else {
>   data->CopyTo(output_bus_.get());
> }

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:283: ResetAudioReceiver();
On 2014/10/17 12:28:10, xians1 wrote:
> do we create audio_controller_ when AudioReceiver is being used?

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:556: :
Abort(SpeechRecognitionError(SPEECH_RECOGNITION_ERROR_AUDIO));
On 2014/10/17 12:28:10, xians1 wrote:
> I think the format should be:
> return StartRecordingUsingAudioReceiver(output_parameters) ?
>     STATE_STARTING :
> Abort(SpeechRecognitionError(SPEECH_RECOGNITION_ERROR_AUDIO));

Done.

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:642: ResetAudioReceiver();
On 2014/10/17 12:28:11, xians1 wrote:
> ResetAudioReceiver() will call audio_receiver_->Stop(), so it will crash.

Fixed in ResetAudioReceiver().

https://codereview.chromium.org/549373003/diff/860001/content/browser/speech/...
content/browser/speech/speech_recognizer_impl.cc:677: audio_thread_->Stop();
On 2014/10/17 12:28:11, xians1 wrote:
> I don't think you guarantee the audio_thread_ and audio_receiver_ are not NULL
> when this method is called?

Fixed.

[burnik, 2014-10-23 14:39:54]

SetListening(true) called on IO thread

[burnik, 2014-10-23 17:50:25]

Simplify unit test

[burnik, 2014-10-24 09:01:42]

Retry tests

[burnik, 2014-11-02 23:51:58]

Waitable event for synchronous start

[burnik, 2014-11-03 02:26:14]

Wait with no timeout

[aplier951, 2016-06-09 20:52:26]

aplier951@gmail.com changed reviewers:
+ aplier951@gmail.com