Improved timer implementation in AudioInputController

media/audio/audio_input_controller.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/audio/audio_input_controller.h"
 
 #include "base/bind.h"
 #include "base/threading/thread_restrictions.h"
 #include "media/base/limits.h"
 
 namespace {
 const int kMaxInputChannels = 2;
 const int kTimerResetInterval = 1;  // One second.
 }
 
 namespace media {
 
 // static
 AudioInputController::Factory* AudioInputController::factory_ = NULL;
 
 AudioInputController::AudioInputController(EventHandler* handler,
                                            SyncWriter* sync_writer)
     : creator_loop_(base::MessageLoopProxy::current()),
       handler_(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() {
   DCHECK(kClosed == state_ || kCreated == state_ || kEmpty == state_);
 }
 
 // static
 scoped_refptr<AudioInputController> AudioInputController::Create(
     AudioManager* audio_manager,
     EventHandler* event_handler,
@@ skipping 54 matching lines @@
 }
 
 void AudioInputController::Record() {
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &AudioInputController::DoRecord, this));
 }
 
 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);
 }
 
 void AudioInputController::SetVolume(double volume) {
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &AudioInputController::DoSetVolume, this, volume));
 }
 
 void AudioInputController::SetAutomaticGainControl(bool enabled) {
@@ skipping 15 matching lines @@
   }
 
   if (stream_ && !stream_->Open()) {
     stream_->Close();
     stream_ = NULL;
     // TODO(satish): Define error types.
     handler_->OnError(this, 0);
     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);
 }
 
 void AudioInputController::DoRecord() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   if (state_ != kCreated)
     return;
 
   {
     base::AutoLock auto_lock(lock_);
     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);
 }
 
 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();
     }
 
     state_ = kClosed;
   }
 }
 
 void AudioInputController::DoReportError(int code) {
@@ skipping 28 matching lines @@
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_NE(state_, kRecording);
 
   // Ensure that the AGC state only can be modified before streaming starts.
   if (state_ != kCreated || state_ == kRecording)
     return;
 
   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();
 }
 
 void AudioInputController::OnData(AudioInputStream* stream, const uint8* data,
                                   uint32 size, uint32 hardware_delay_bytes,
                                   double volume) {
   {
     base::AutoLock auto_lock(lock_);
     if (state_ != kRecording)
       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()) {
     sync_writer_->Write(data, size, volume);
     sync_writer_->UpdateRecordedBytes(hardware_delay_bytes);
     return;
   }
 
   handler_->OnData(this, data, size);
 }
@@ skipping 20 matching lines @@
     stream_->Stop();
     stream_->Close();
     stream_ = NULL;
   }
 
   // Should be last in the method, do not touch "this" from here on.
   if (done != NULL)
     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