Fix bug when unplugging an audio input device whilst using speech input.

Fix bug when unplugging an audio input device whilst using speech input on Windows.

Added a three second timeout for not receiving an OnData callback in AudioInputController and a one second timeout for not receiving any callback in PCMWaveInAudioInputStream. Also added a unit test fixture for it.

BUG=79936
TEST=Unplug microphone while using speech input, browser should no longer hang.

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

[scherkus (not reviewing), 2011-06-13 19:04:56]

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:191: void
AudioInputController::ScheduleDataCountCheck() {
consider using DelayTimer from base/timer.h

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
File media/audio/audio_input_controller.h (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller.h:103: AudioInputStream*
getAudioInputStream() { return stream_; }
nit: should be stream()

[Satish, 2011-06-14 14:28:04]

Good stuff!

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:187: // Have not received new data since
last check so report an error.
It would be useful to explain this more and even add http://crbug.com/79936 so
it is clear that this was to fix a real world problem and not a theoretical one.

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
File media/audio/audio_input_controller_unittest.cc (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller_unittest.cc:136: // Play and then wait for
the event to be signaled.
Change 'Play' to 'Record'

http://codereview.chromium.org/7129057/diff/2001/media/audio/win/wavein_input...
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/win/wavein_input...
media/audio/win/wavein_input_win.cc:18: const int kCallbackTimeout = 1000; //
One second.
rename to kStopInputStreamCallbackTimeout to be more specific? Also could
increase to a couple more seconds to reduce false positives

[allanwoj, 2011-06-14 15:03:12]

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:187: // Have not received new data since
last check so report an error.
On 2011/06/14 14:28:04, Satish wrote:
> It would be useful to explain this more and even add http://crbug.com/79936 so
> it is clear that this was to fix a real world problem and not a theoretical
one.

Done.

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:191: void
AudioInputController::ScheduleDataCountCheck() {
On 2011/06/13 19:04:56, scherkus wrote:
> consider using DelayTimer from base/timer.h

Decided to keep it the way it is, this way it makes it easy to see which thread
we should be on.

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
File media/audio/audio_input_controller.h (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller.h:103: AudioInputStream*
getAudioInputStream() { return stream_; }
On 2011/06/13 19:04:56, scherkus wrote:
> nit: should be stream()

Done.

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
File media/audio/audio_input_controller_unittest.cc (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller_unittest.cc:136: // Play and then wait for
the event to be signaled.
On 2011/06/14 14:28:04, Satish wrote:
> Change 'Play' to 'Record'

Done.

http://codereview.chromium.org/7129057/diff/2001/media/audio/win/wavein_input...
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/win/wavein_input...
media/audio/win/wavein_input_win.cc:18: const int kCallbackTimeout = 1000; //
One second.
On 2011/06/14 14:28:04, Satish wrote:
> rename to kStopInputStreamCallbackTimeout to be more specific? Also could
> increase to a couple more seconds to reduce false positives

Done.

[satish1, 2011-06-14 15:39:55]

One more comment -

http://codereview.chromium.org/7129057/diff/7001/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/7129057/diff/7001/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:195:
thread_.message_loop()->PostDelayedTask(
I see the DelayTimer class cancels the timer on destruction. Do we need to
explicitly cancel the pending delayed task here when processing DoClose() ? May
be we also need a check that state_ != kClosed in DoDataCountCheck() and if that
failed return immediately. All this probably gets automatically handled if we
use DelayTimer ?

[scherkus (not reviewing), 2011-06-14 21:22:02]

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:191: void
AudioInputController::ScheduleDataCountCheck() {
On 2011/06/14 15:03:12, allanwoj wrote:
> On 2011/06/13 19:04:56, scherkus wrote:
> > consider using DelayTimer from base/timer.h
> 
> Decided to keep it the way it is, this way it makes it easy to see which
thread
> we should be on.

Not sure I agree... right now I find it more confusing as you're reading/writing
on different threads:
  1) OnData increments on_data_call_count_ on audio callback thread
  2) DoDataCountCheck reads on_data_call_count_ on thread_.message_loop()

Furthermore it's likely that another chromium developer will see DelayTimer and
understand what's happening, as opposed to having to interpret the
data_call_count stuff.

What about posting a task from OnData to thread_ (similar to OnError) to call
DelayTimer::Reset()? That way DelayTimer is only accessed on a single thread and
it takes care of task cancellation, etc...

[allanwoj, 2011-06-15 10:11:40]

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/7129057/diff/2001/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:191: void
AudioInputController::ScheduleDataCountCheck() {
On 2011/06/14 21:22:02, scherkus wrote:
> On 2011/06/14 15:03:12, allanwoj wrote:
> > On 2011/06/13 19:04:56, scherkus wrote:
> > > consider using DelayTimer from base/timer.h
> > 
> > Decided to keep it the way it is, this way it makes it easy to see which
> thread
> > we should be on.
> 
> Not sure I agree... right now I find it more confusing as you're
reading/writing
> on different threads:
>   1) OnData increments on_data_call_count_ on audio callback thread
>   2) DoDataCountCheck reads on_data_call_count_ on thread_.message_loop()
> 
> Furthermore it's likely that another chromium developer will see DelayTimer
and
> understand what's happening, as opposed to having to interpret the
> data_call_count stuff.
> 
> What about posting a task from OnData to thread_ (similar to OnError) to call
> DelayTimer::Reset()? That way DelayTimer is only accessed on a single thread
and
> it takes care of task cancellation, etc...

Oh right, I see what you mean. I've gone ahead and done that. Thanks!

http://codereview.chromium.org/7129057/diff/7001/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:195: kDataCountCheckInterval);
On 2011/06/14 15:39:56, satish1 wrote:
> I see the DelayTimer class cancels the timer on destruction. Do we need to
> explicitly cancel the pending delayed task here when processing DoClose() ?
May
> be we also need a check that state_ != kClosed in DoDataCountCheck() and if
that
> failed return immediately. All this probably gets automatically handled if we
> use DelayTimer ?

Done.

[Satish, 2011-06-15 10:23:26]

LGTM

http://codereview.chromium.org/7129057/diff/18002/media/audio/audio_input_con...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/7129057/diff/18002/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:13: const int kTimerResetInterval = 1; //
One second.
suggest moving both these constants to an anonymous namespace above and removing
the 'static'

http://codereview.chromium.org/7129057/diff/18002/media/audio/audio_input_con...
File media/audio/audio_input_controller.h (right):

http://codereview.chromium.org/7129057/diff/18002/media/audio/audio_input_con...
media/audio/audio_input_controller.h:153: base::DelayTimer<AudioInputController>
timer_;
'timer_' and 'DoTimerReset' seem very generic names. Could you use a more
specific name which indicates the purpose of the timer?

[allanwoj, 2011-06-15 11:11:16]

http://codereview.chromium.org/7129057/diff/18002/media/audio/audio_input_con...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/7129057/diff/18002/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:13: const int kTimerResetInterval = 1; //
One second.
On 2011/06/15 10:23:27, Satish wrote:
> suggest moving both these constants to an anonymous namespace above and
removing
> the 'static'

Done.

http://codereview.chromium.org/7129057/diff/18002/media/audio/audio_input_con...
File media/audio/audio_input_controller.h (right):

http://codereview.chromium.org/7129057/diff/18002/media/audio/audio_input_con...
media/audio/audio_input_controller.h:153: base::DelayTimer<AudioInputController>
timer_;
On 2011/06/15 10:23:27, Satish wrote:
> 'timer_' and 'DoTimerReset' seem very generic names. Could you use a more
> specific name which indicates the purpose of the timer?

Done.

[Satish, 2011-06-15 11:19:58]

LGTM

[scherkus (not reviewing), 2011-06-15 18:27:31]

thanks allanwoj!

LGTM