Improved timer implementation in AudioInputController

media/audio/audio_input_controller.cc

Index: media/audio/audio_input_controller.cc
diff --git a/media/audio/audio_input_controller.cc b/media/audio/audio_input_controller.cc
index 6120efb338967706b5ef84e59af5ef8d579c7b22..196beb5bcba7e3e1e6ae11c1432e7b7e7e07d771 100644
--- a/media/audio/audio_input_controller.cc
+++ b/media/audio/audio_input_controller.cc
@@ -25,12 +25,9 @@ AudioInputController::AudioInputController(EventHandler* handler,
       stream_(NULL),
       state_(kEmpty),
       sync_writer_(sync_writer),
-      max_volume_(0.0) {
+      max_volume_(0.0),
+      data_is_active_(false) {
   DCHECK(creator_loop_);
-  no_data_timer_.reset(new base::DelayTimer<AudioInputController>(FROM_HERE,
-      base::TimeDelta::FromSeconds(kTimerResetInterval),
-      this,
-      &AudioInputController::DoReportNoDataError));
 }
 
 AudioInputController::~AudioInputController() {
@@ -105,10 +102,7 @@ void AudioInputController::Record() {
 void AudioInputController::Close(const base::Closure& closed_task) {
   DCHECK(!closed_task.is_null());
   DCHECK(creator_loop_->BelongsToCurrentThread());
-  // See crbug.com/119783: Deleting the timer now to avoid disaster if
-  // AudioInputController is destructed on a thread other than the creator
-  // thread.
-  no_data_timer_.reset();
+
   message_loop_->PostTaskAndReply(
       FROM_HERE, base::Bind(&AudioInputController::DoClose, this), closed_task);
 }
@@ -144,8 +138,13 @@ void AudioInputController::DoCreate(AudioManager* audio_manager,
     return;
   }
 
-  creator_loop_->PostTask(FROM_HERE, base::Bind(
-      &AudioInputController::DoResetNoDataTimer, this));
+  // Create the data timer which will call DoCheckForNoData() after a delay
+  // of |kTimerResetInterval| seconds. The timer is started in DoRecord()
+  // and restarted in each DoCheckForNoData() callback.
+  no_data_timer_.reset(new base::DelayTimer<AudioInputController>(FROM_HERE,

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

should we first that no_data_timer_ is NULL?

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

can we move these code to DoRecord instead, since recording starts from there.

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

Done.

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

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().

+      base::TimeDelta::FromSeconds(kTimerResetInterval),
+      this,
+      &AudioInputController::DoCheckForNoData));
 
   state_ = kCreated;
   handler_->OnCreated(this);
@@ -162,6 +161,10 @@ void AudioInputController::DoRecord() {
     state_ = kRecording;
   }
 
+  // Start the data timer. Once |kTimerResetInterval| seconds have passed,
+  // a callback to DoCheckForNoData() made.
+  DoResetNoDataTimer();

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

We can skip this call if we move the code here: no_data_timer_.reset(new
base::DelayTimer<AudioInputController>(FROM_HERE, ...

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

Actually no since start requires: create + reset.

+
   stream_->Start(this);
   handler_->OnRecording(this);
 }
@@ -169,8 +172,14 @@ void AudioInputController::DoRecord() {
 void AudioInputController::DoClose() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
+  if (no_data_timer_.get()) {

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

maybe we can skip the check, and set it regardlessly.

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

Not sure why you don't like the non-NULL check. Can I keep it?

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

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

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

Done.

+    // Delete the timer on the same thread that created it.
+    no_data_timer_.reset();
+  }
+
   if (state_ != kClosed) {
     DoStopCloseAndClearStream(NULL);
+    SetDataIsActive(false);
 
     if (LowLatencyMode()) {
       sync_writer_->Close();
@@ -219,17 +228,28 @@ void AudioInputController::DoSetAutomaticGainControl(bool enabled) {
   stream_->SetAutomaticGainControl(enabled);
 }
 
-void AudioInputController::DoReportNoDataError() {
-  DCHECK(creator_loop_->BelongsToCurrentThread());
+void AudioInputController::DoCheckForNoData() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
 
-  // Error notifications should be sent on the audio-manager thread.
-  int code = 0;
-  message_loop_->PostTask(FROM_HERE, base::Bind(
-      &AudioInputController::DoReportError, this, code));
+  if (!GetDataIsActive()) {
+    // The data-is-active marker will be false only if it has been more than
+    // one second since a data packet was recorded. This can happen if a
+    // capture device has been removed or disabled.
+    handler_->OnError(this, 0);

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

Shall we delete the timer here?

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

No. You must call Close() to delete it. That is done in the OnError() callback.

+    return;
+  }
+
+  // Mark data as non-active. The flag will be re-enabled in OnData() each
+  // time a data packet is received. Hence, under normal conditions, the
+  // flag will only be disabled during a very short period.
+  SetDataIsActive(false);
+
+  // Restart the timer to ensure that we check the flag in one second again.
+  DoResetNoDataTimer();

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

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.

 }
 
 void AudioInputController::DoResetNoDataTimer() {

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

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.

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

Done.

-  DCHECK(creator_loop_->BelongsToCurrentThread());
+  DCHECK(message_loop_->BelongsToCurrentThread());
   if (no_data_timer_.get())
     no_data_timer_->Reset();
 }
@@ -243,8 +263,9 @@ void AudioInputController::OnData(AudioInputStream* stream, const uint8* data,
       return;
   }
 
-  creator_loop_->PostTask(FROM_HERE, base::Bind(
-      &AudioInputController::DoResetNoDataTimer, this));
+  // Mark data as active to ensure that the periodic calls to
+  // DoCheckForNoData() does not report an error to the event handler.
+  SetDataIsActive(true);
 
   // Use SyncSocket if we are in a low-latency mode.
   if (LowLatencyMode()) {
@@ -285,4 +306,12 @@ void AudioInputController::DoStopCloseAndClearStream(
     done->Signal();
 }
 
+void AudioInputController::SetDataIsActive(bool enabled) {
+  base::subtle::Release_Store(&data_is_active_, enabled);
+}
+
+bool AudioInputController::GetDataIsActive() {
+  return (base::subtle::Acquire_Load(&data_is_active_) == 1);

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

nit: Since in the Set method you use bool, check against false and not 1:

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

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

Done.

+}
+
 }  // namespace media