Redesign the copresence audio handlers.

components/copresence/handlers/audio/audio_directive_handler.cc

Index: components/copresence/handlers/audio/audio_directive_handler.cc
diff --git a/components/copresence/handlers/audio/audio_directive_handler.cc b/components/copresence/handlers/audio/audio_directive_handler.cc
index 3d2e7c65aac69b8150c564c92550d5f20a486639..736fb915c54240a1797bb32bcbf6bea7fe6764d8 100644
--- a/components/copresence/handlers/audio/audio_directive_handler.cc
+++ b/components/copresence/handlers/audio/audio_directive_handler.cc
@@ -7,67 +7,48 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/strings/string_util.h"
+#include "base/time/default_tick_clock.h"
+#include "base/time/tick_clock.h"
 #include "base/time/time.h"

[Daniel Erat, 2014/10/22 16:34:34]

don't need this here; it's already included by the header

[rkc, 2014/10/22 18:21:47]

Done.

-#include "components/copresence/mediums/audio/audio_player.h"
-#include "components/copresence/mediums/audio/audio_recorder.h"
+#include "base/timer/timer.h"
+#include "components/copresence/mediums/audio/audio_manager_impl.h"
 #include "components/copresence/proto/data.pb.h"
+#include "components/copresence/public/copresence_constants.h"
 #include "media/base/audio_bus.h"
 
+namespace copresence {
+
 namespace {
 
-// UrlSafe is defined as:
-// '/' represented by a '_' and '+' represented by a '-'
-// TODO(rkc): Move this processing to the whispernet wrapper.
-std::string FromUrlSafe(std::string token) {
-  base::ReplaceChars(token, "-", "+", &token);
-  base::ReplaceChars(token, "_", "/", &token);
-  return token;
-}
+base::TimeTicks GetEarliestEventTime(AudioDirectiveList* list,
+                                     base::TimeTicks event_time) {
+  if (!list->GetActiveDirective())
+    return event_time;
 
-const int kSampleExpiryTimeMs = 60 * 60 * 1000;  // 60 minutes.
-const int kMaxSamples = 10000;
+  if (event_time.is_null())
+    return list->GetActiveDirective()->end_time;
+  else
+    return std::min(list->GetActiveDirective()->end_time, event_time);
+}
 
 }  // namespace
 
-namespace copresence {
-
 // Public methods.
 
-AudioDirectiveHandler::AudioDirectiveHandler(
-    const AudioRecorder::DecodeSamplesCallback& decode_cb,
-    const AudioDirectiveHandler::EncodeTokenCallback& encode_cb)
-    : player_audible_(NULL),
-      player_inaudible_(NULL),
-      recorder_(NULL),
-      decode_cb_(decode_cb),
-      encode_cb_(encode_cb),
-      samples_cache_audible_(
-          base::TimeDelta::FromMilliseconds(kSampleExpiryTimeMs),
-          kMaxSamples),
-      samples_cache_inaudible_(
-          base::TimeDelta::FromMilliseconds(kSampleExpiryTimeMs),
-          kMaxSamples) {
+AudioDirectiveHandler::AudioDirectiveHandler()
+    : audio_event_timer_(new base::OneShotTimer<AudioDirectiveHandler>),
+      clock_(new base::DefaultTickClock) {
 }
 
 AudioDirectiveHandler::~AudioDirectiveHandler() {
-  if (player_audible_)
-    player_audible_->Finalize();
-  if (player_inaudible_)
-    player_inaudible_->Finalize();
-  if (recorder_)
-    recorder_->Finalize();
 }
 
-void AudioDirectiveHandler::Initialize() {
-  player_audible_ = new AudioPlayer();
-  player_audible_->Initialize();
-
-  player_inaudible_ = new AudioPlayer();
-  player_inaudible_->Initialize();
-
-  recorder_ = new AudioRecorder(decode_cb_);
-  recorder_->Initialize();
+void AudioDirectiveHandler::Initialize(
+    const AudioManager::DecodeSamplesCallback& decode_cb,
+    const AudioManager::EncodeTokenCallback& encode_cb) {
+  if (!audio_manager_.get())

[Daniel Erat, 2014/10/22 16:34:34]

nit: just do "if (!audio_manager_)"

[rkc, 2014/10/22 18:21:47]

Done.

+    audio_manager_.reset(new AudioManagerImpl());
+  audio_manager_->Initialize(decode_cb, encode_cb);
 }
 
 void AudioDirectiveHandler::AddInstruction(const TokenInstruction& instruction,
@@ -77,144 +58,114 @@ void AudioDirectiveHandler::AddInstruction(const TokenInstruction& instruction,
     case TRANSMIT:
       DVLOG(2) << "Audio Transmit Directive received. Token: "
                << instruction.token_id()
+               << " with medium= " << instruction.medium()

[Daniel Erat, 2014/10/22 16:34:34]

nit: remove extra space after '=' to match the rest of this string

[rkc, 2014/10/22 18:21:47]

Done.

                << " with TTL=" << ttl.InMilliseconds();
       switch (instruction.medium()) {
         case AUDIO_ULTRASOUND_PASSBAND:
-          transmits_list_inaudible_.AddDirective(op_id, ttl);
-          PlayToken(instruction.token_id(), false);
+          transmits_list_[INAUDIBLE].AddDirective(op_id, ttl);
+          audio_manager_->SetToken(INAUDIBLE, instruction.token_id());
           break;
         case AUDIO_AUDIBLE_DTMF:
-          transmits_list_audible_.AddDirective(op_id, ttl);
-          PlayToken(instruction.token_id(), true);
+          transmits_list_[AUDIBLE].AddDirective(op_id, ttl);
+          audio_manager_->SetToken(AUDIBLE, instruction.token_id());
           break;
         default:
           NOTREACHED();
       }
       break;
     case RECEIVE:
-      DVLOG(2) << "Audio Receive Directive received. TTL="
-               << ttl.InMilliseconds();
-      receives_list_.AddDirective(op_id, ttl);
-      ProcessNextReceive();
+      DVLOG(2) << "Audio Receive Directive received."
+               << " with medium= " << instruction.medium()

[Daniel Erat, 2014/10/22 16:34:34]

same here

[rkc, 2014/10/22 18:21:47]

Done.

+               << " with TTL=" << ttl.InMilliseconds();
+      switch (instruction.medium()) {
+        case AUDIO_ULTRASOUND_PASSBAND:
+          receives_list_[INAUDIBLE].AddDirective(op_id, ttl);
+          break;
+        case AUDIO_AUDIBLE_DTMF:
+          receives_list_[AUDIBLE].AddDirective(op_id, ttl);
+          break;
+        default:
+          NOTREACHED();
+      }
       break;
     case UNKNOWN_TOKEN_INSTRUCTION_TYPE:
     default:
-      LOG(WARNING) << "Unknown Audio Transmit Directive received.";
+      LOG(WARNING) << "Unknown Audio Transmit Directive received. type = "
+                   << instruction.token_instruction_type();
   }
+  ProcessNextInstruction();
 }
 
 void AudioDirectiveHandler::RemoveInstructions(const std::string& op_id) {
-  transmits_list_audible_.RemoveDirective(op_id);
-  transmits_list_inaudible_.RemoveDirective(op_id);
-  receives_list_.RemoveDirective(op_id);
+  transmits_list_[AUDIBLE].RemoveDirective(op_id);
+  transmits_list_[INAUDIBLE].RemoveDirective(op_id);
+  receives_list_[AUDIBLE].RemoveDirective(op_id);
+  receives_list_[INAUDIBLE].RemoveDirective(op_id);
 
-  ProcessNextTransmit();
-  ProcessNextReceive();
+  ProcessNextInstruction();
 }
 
 // Private methods.
 
-void AudioDirectiveHandler::ProcessNextTransmit() {
-  // If we have an active directive for audible or inaudible audio, ensure that
-  // we are playing our respective token; if we do not have a directive, then
-  // make sure we aren't playing. This is duplicate code, but for just two
-  // elements, it has hard to make a case for processing a loop instead.
-
-  scoped_ptr<AudioDirective> audible_transmit(
-      transmits_list_audible_.GetActiveDirective());
-  if (audible_transmit && !player_audible_->IsPlaying() &&
-      samples_cache_audible_.HasKey(current_token_audible_)) {
-    DVLOG(3) << "Playing audible for op_id: " << audible_transmit->op_id;
-    player_audible_->Play(
-        samples_cache_audible_.GetValue(current_token_audible_));
-    stop_audible_playback_timer_.Start(
+void AudioDirectiveHandler::ProcessNextInstruction() {
+  DCHECK(audio_event_timer_);
+  audio_event_timer_->Stop();
+
+  // Change audio_manager_ state for audible transmits.

[Daniel Erat, 2014/10/22 16:34:34]

nit: s/audio_manager_/|audio_manager_|/

[rkc, 2014/10/22 18:21:47]

Done.

+  if (transmits_list_[AUDIBLE].GetActiveDirective())
+    audio_manager_->StartPlaying(AUDIBLE);
+  else
+    audio_manager_->StopPlaying(AUDIBLE);
+
+  // Change audio_manager_ state for inaudible transmits.
+  if (transmits_list_[INAUDIBLE].GetActiveDirective())
+    audio_manager_->StartPlaying(INAUDIBLE);
+  else
+    audio_manager_->StopPlaying(INAUDIBLE);
+
+  // Change audio_manager_ state for audible receives.
+  if (receives_list_[AUDIBLE].GetActiveDirective())
+    audio_manager_->StartRecording(AUDIBLE);
+  else
+    audio_manager_->StopRecording(AUDIBLE);
+
+  // Change audio_manager_ state for inaudible receives.
+  if (receives_list_[INAUDIBLE].GetActiveDirective())
+    audio_manager_->StartRecording(INAUDIBLE);
+  else
+    audio_manager_->StopRecording(INAUDIBLE);
+
+  base::TimeTicks next_event_time;
+  if (GetNextInstructionExpiry(&next_event_time)) {
+    // This timer will be destructed before us, hence it is safe to use

[Daniel Erat, 2014/10/22 16:34:34]

nit: i don't think that this comment is necessary; passing
base::Unretained(this) to a timer is a common pattern

[rkc, 2014/10/22 18:21:47]

Done.

+    // unretained here.
+    audio_event_timer_->Start(
         FROM_HERE,
-        audible_transmit->end_time - base::Time::Now(),
-        this,
-        &AudioDirectiveHandler::ProcessNextTransmit);
-  } else if (!audible_transmit && player_audible_->IsPlaying()) {
-    DVLOG(3) << "Stopping audible playback.";
-    current_token_audible_.clear();
-    stop_audible_playback_timer_.Stop();
-    player_audible_->Stop();
-  }
-
-  scoped_ptr<AudioDirective> inaudible_transmit(
-      transmits_list_inaudible_.GetActiveDirective());
-  if (inaudible_transmit && !player_inaudible_->IsPlaying() &&
-      samples_cache_inaudible_.HasKey(current_token_inaudible_)) {
-    DVLOG(3) << "Playing inaudible for op_id: " << inaudible_transmit->op_id;
-    player_inaudible_->Play(
-        samples_cache_inaudible_.GetValue(current_token_inaudible_));
-    stop_inaudible_playback_timer_.Start(
-        FROM_HERE,
-        inaudible_transmit->end_time - base::Time::Now(),
-        this,
-        &AudioDirectiveHandler::ProcessNextTransmit);
-  } else if (!inaudible_transmit && player_inaudible_->IsPlaying()) {
-    DVLOG(3) << "Stopping inaudible playback.";
-    current_token_inaudible_.clear();
-    stop_inaudible_playback_timer_.Stop();
-    player_inaudible_->Stop();
-  }
-}
-
-void AudioDirectiveHandler::ProcessNextReceive() {
-  scoped_ptr<AudioDirective> receive(receives_list_.GetActiveDirective());
-
-  if (receive && !recorder_->IsRecording()) {
-    DVLOG(3) << "Recording for op_id: " << receive->op_id;
-    recorder_->Record();
-    stop_recording_timer_.Start(FROM_HERE,
-                                receive->end_time - base::Time::Now(),
-                                this,
-                                &AudioDirectiveHandler::ProcessNextReceive);
-  } else if (!receive && recorder_->IsRecording()) {
-    DVLOG(3) << "Stopping Recording";
-    stop_recording_timer_.Stop();
-    recorder_->Stop();
+        next_event_time - clock_->NowTicks(),
+        base::Bind(&AudioDirectiveHandler::ProcessNextInstruction,
+                   base::Unretained(this)));
   }
 }
 
-void AudioDirectiveHandler::PlayToken(const std::string token, bool audible) {
-  std::string valid_token = FromUrlSafe(token);
-
-  // If the token has been encoded already, use the cached samples.
-  if (audible && samples_cache_audible_.HasKey(valid_token)) {
-    current_token_audible_ = token;
-    ProcessNextTransmit();
-  } else if (!audible && samples_cache_inaudible_.HasKey(valid_token)) {
-    current_token_inaudible_ = token;
-    ProcessNextTransmit();
-  } else {
-    // Otherwise, encode the token and then play it.
-    encode_cb_.Run(valid_token,
-                   audible,
-                   base::Bind(&AudioDirectiveHandler::PlayEncodedToken,
-                              base::Unretained(this)));
-  }
-}
-
-void AudioDirectiveHandler::PlayEncodedToken(
-    const std::string& token,
-    bool audible,
-    const scoped_refptr<media::AudioBusRefCounted>& samples) {
-  DVLOG(3) << "Token " << token << "[audible:" << audible << "] encoded.";
-  if (audible) {
-    samples_cache_audible_.Add(token, samples);
-    current_token_audible_ = token;
-    // Force process transmits to pick up the new token.
-    if (player_audible_->IsPlaying())
-      player_audible_->Stop();
-  } else {
-    samples_cache_inaudible_.Add(token, samples);
-    current_token_inaudible_ = token;
-    // Force process transmits to pick up the new token.
-    if (player_inaudible_->IsPlaying())
-      player_inaudible_->Stop();
-  }
-
-  ProcessNextTransmit();
+bool AudioDirectiveHandler::GetNextInstructionExpiry(
+    base::TimeTicks* next_event_ptr) {
+  DCHECK(next_event_ptr);
+  base::TimeTicks& next_event = *next_event_ptr;

[Daniel Erat, 2014/10/22 16:34:34]

nit: rename the argument to |expiry_out| and get rid of the reference? doesn't
look like it'd wrap:

  *expiry_out = GetEarliestEventTime(&transmits_list_[INAUDIBLE], *expiry_out);

[rkc, 2014/10/22 18:21:47]

Renaming to expiry does prevent wrapping on all lines.
Done.

+
+  next_event =
+      GetEarliestEventTime(&transmits_list_[AUDIBLE], base::TimeTicks());
+  next_event = GetEarliestEventTime(&transmits_list_[INAUDIBLE], next_event);
+  next_event = GetEarliestEventTime(&receives_list_[AUDIBLE], next_event);
+  next_event = GetEarliestEventTime(&receives_list_[INAUDIBLE], next_event);
+
+  // If our next event is still at max, means we don't have any active

[Daniel Erat, 2014/10/22 16:34:34]

this max comment is obsolete

[rkc, 2014/10/22 18:21:47]

Done.

+  // directives; or that they are all due to expire a bazzilion years from now.

[Daniel Erat, 2014/10/22 16:34:34]

s/bazzilion/bazillion/

[rkc, 2014/10/22 18:21:47]

I didn't know there was a widely accepted spelling for bazillion :)
Outdated comment removed though.
Done.

+  // The latter is not possible since the max life of a directive is 1 day,
+  // this is enforced by the API.
+  if (next_event.is_null())

[Daniel Erat, 2014/10/22 16:34:34]

nit: "return !expiry_out->is_null();"

[rkc, 2014/10/22 18:21:47]

Done.

+    return false;
+  else
+    return true;
 }
 
 }  // namespace copresence