Improved timer implementation in AudioInputController

Improved timer implementation in AudioInputController since AudioInputController::DoResetNoDataTimer() has an extreme task execution count in chrome://profiler.

There is no longer a PostTask in OnData() to reset the data timer. Instead, a simple flag is set and this flag is checked, and reset, on a periodic basis. If packets no longer are generated, the cleared flag will be detected an and OnError() callback will be given to the event handler.

I have verified the new design in different ways:

1) Verified that no AudioInputController function pops up in the profiler.
2) media_unittest --gtest_filter=AudioInputControllerTest*RecordAndError
3) Tried out the speech input API and the removed the microphone while recording and verified that it worked.
4) Same as 3) but for WebRTC in loopback using the https://apprtc.appspot.com/?debug=loopback demo.

BUG=123588
TEST=media_unittest --gtest_filter=AudioInputControllerTest*RecordAndError

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

[henrika (OOO until Aug 14), 2012-04-17 11:40:43]

This CL ensures that AudioInputController tasks no longer exists among the top
counts in the profiler. It also simplifies the old design since the timer now
lives on the audio thread. It removes PostTask() calls and should also guarantee
that the timer is always destroyed on the same thread as the creating audio
thread.

[henrika (OOO until Aug 14), 2012-04-17 11:42:21]

CC: satish and primiano for Speech Input FYI.

[tommi (sloooow) - chröme, 2012-04-17 11:58:31]

great stuff! is the unit test missing or am I misunderstanding the description? 
(a unit test would of course be great!)

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
media/audio/audio_input_controller.cc:144: no_data_timer_.reset(new
base::DelayTimer<AudioInputController>(FROM_HERE,
should we first that no_data_timer_ is NULL?

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
media/audio/audio_input_controller.cc:251: void
AudioInputController::DoResetNoDataTimer() {
Can we just remove this method and call no_data_timer_->Reset() directly
instead?  The thread check is already there in those places and no_data_timer_
should never be NULL (or else something went wrong).

Also (just fyi) the naming convention of DoSomething is conventionally used when
a task has been posted and the "Something" method name has already been taken. 
So, since this method isn't used for PostTask anymore, it should simply be
called ResetNoDataTimer.

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
media/audio/audio_input_controller.cc:314: return
(base::subtle::Acquire_Load(&data_is_active_) == 1);
nit: Since in the Set method you use bool, check against false and not 1:

return base::subtle::Acquire_Load(&data_is_active_) != false

[no longer working on chromium, 2012-04-17 12:20:41]

Great work, thanks for cleaning it up.

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
media/audio/audio_input_controller.cc:144: no_data_timer_.reset(new
base::DelayTimer<AudioInputController>(FROM_HERE,
can we move these code to DoRecord instead, since recording starts from there.

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
media/audio/audio_input_controller.cc:166: DoResetNoDataTimer();
We can skip this call if we move the code here: no_data_timer_.reset(new
base::DelayTimer<AudioInputController>(FROM_HERE, ...

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
media/audio/audio_input_controller.cc:175: if (no_data_timer_.get()) {
maybe we can skip the check, and set it regardlessly.

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
media/audio/audio_input_controller.cc:238: handler_->OnError(this, 0);
Shall we delete the timer here?

https://chromiumcodereview.appspot.com/9956169/diff/1/media/audio/audio_input...
media/audio/audio_input_controller.cc:248: DoResetNoDataTimer();
can we just called no_data_timer_->Reset() here, any possibility that
no_data_timer_ is NULL when this is called?

[henrika (OOO until Aug 14), 2012-04-17 13:03:27]

Done.

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:144: no_data_timer_.reset(new
base::DelayTimer<AudioInputController>(FROM_HERE,
On 2012/04/17 11:58:31, tommi wrote:
> should we first that no_data_timer_ is NULL?

Done.

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:144: no_data_timer_.reset(new
base::DelayTimer<AudioInputController>(FROM_HERE,
It is possible but I felt it was more symmetric to use DoCreate() and DoClose()
for create/delete. There is no StopRecording. Now I create in DoCreate(), start
the timer in DoRecord() and stop/destroy the timer in DoClose().

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:166: DoResetNoDataTimer();
Actually no since start requires: create + reset.

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:175: if (no_data_timer_.get()) {
Not sure why you don't like the non-NULL check. Can I keep it?

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:238: handler_->OnError(this, 0);
No. You must call Close() to delete it. That is done in the OnError() callback.

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:248: DoResetNoDataTimer();
On 2012/04/17 12:20:41, xians1 wrote:
> can we just called no_data_timer_->Reset() here, any possibility that
> no_data_timer_ is NULL when this is called?

Done.

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:251: void
AudioInputController::DoResetNoDataTimer() {
On 2012/04/17 11:58:31, tommi wrote:
> Can we just remove this method and call no_data_timer_->Reset() directly
> instead?  The thread check is already there in those places and no_data_timer_
> should never be NULL (or else something went wrong).
> 
> Also (just fyi) the naming convention of DoSomething is conventionally used
when
> a task has been posted and the "Something" method name has already been taken.

> So, since this method isn't used for PostTask anymore, it should simply be
> called ResetNoDataTimer.

Done.

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:314: return
(base::subtle::Acquire_Load(&data_is_active_) == 1);
On 2012/04/17 11:58:31, tommi wrote:
> nit: Since in the Set method you use bool, check against false and not 1:
> 
> return base::subtle::Acquire_Load(&data_is_active_) != false

Done.

[henrika (OOO until Aug 14), 2012-04-17 13:05:03]

Tommi,

I made the "TEST=" section more clear regarding the unit test.

[tommi (sloooow) - chröme, 2012-04-17 13:12:49]

lgtm with two comments.

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:175: if (no_data_timer_.get()) {
On 2012/04/17 13:03:27, henrika wrote:
> Not sure why you don't like the non-NULL check. Can I keep it?

There is already a NULL check inside reset(), so we shouldn't check it here as
well.

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

http://codereview.chromium.org/9956169/diff/5002/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:141: if (!no_data_timer_.get()) {
sorry, what I meant was this:

DCHECK(!no_data_timer_.get());
no_data_timer_.reset(...);

[henrika (OOO until Aug 14), 2012-04-17 13:38:31]

Thanks!

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
File media/audio/audio_input_controller.cc (right):

http://codereview.chromium.org/9956169/diff/1/media/audio/audio_input_control...
media/audio/audio_input_controller.cc:175: if (no_data_timer_.get()) {
On 2012/04/17 13:12:49, tommi wrote:
> On 2012/04/17 13:03:27, henrika wrote:
> > Not sure why you don't like the non-NULL check. Can I keep it?
> 
> There is already a NULL check inside reset(), so we shouldn't check it here as
> well.

Done.

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

http://codereview.chromium.org/9956169/diff/5002/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:141: if (!no_data_timer_.get()) {
On 2012/04/17 13:12:49, tommi wrote:
> sorry, what I meant was this:
> 
> DCHECK(!no_data_timer_.get());
> no_data_timer_.reset(...);

Done.

[no longer working on chromium, 2012-04-17 13:40:15]

lgtm

[primiano CORP (USE chromium), 2012-04-17 13:52:08]

Thanks for the CC.
No problem on our side since OnError is reposted anyway in our
SpeechRecognizerImpl.


On Tue, Apr 17, 2012 at 2:40 PM, <xians@chromium.org> wrote:

> lgtm
>
>
http://codereview.chromium.**org/9956169/<http://codereview.chromium.org/9956...
>



-- 
Primiano Tucci
Software Engineer Intern.
Google UK Limited
Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W
9TQ

[henrika (OOO until Aug 14), 2012-04-17 13:56:11]

Great. I just wanted to let you know about the change since there had also been
some changes lately in the audio_input_controller regarding the timer member and
how it was destructed.

[commit-bot: I haz the power, 2012-04-17 14:38:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/henrika@chromium.org/9956169/7003

[commit-bot: I haz the power, 2012-04-17 16:03:48]

Change committed as 132583