Redesign the copresence audio handlers.

components/copresence/mediums/audio/audio_manager.cc

Index: components/copresence/mediums/audio/audio_manager.cc
diff --git a/components/copresence/mediums/audio/audio_manager.cc b/components/copresence/mediums/audio/audio_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..30eb98f79dd762cab6bbfc56d172242a26611381
--- /dev/null
+++ b/components/copresence/mediums/audio/audio_manager.cc
@@ -0,0 +1,170 @@
+// Copyright 2014 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 "components/copresence/mediums/audio/audio_manager.h"
+
+#include <algorithm>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/logging.h"
+#include "base/run_loop.h"
+#include "base/strings/string_util.h"
+#include "base/synchronization/waitable_event.h"
+#include "components/copresence/mediums/audio/audio_player.h"
+#include "components/copresence/mediums/audio/audio_recorder.h"
+#include "components/copresence/public/copresence_constants.h"
+#include "content/public/browser/browser_thread.h"
+#include "media/audio/audio_manager.h"
+#include "media/audio/audio_manager_base.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;
+}
+
+const int kSampleExpiryTimeMs = 60 * 60 * 1000;  // 60 minutes.
+const int kMaxSamples = 10000;
+
+}  // namespace
+
+// Public methods.
+
+AudioManager::AudioManager()
+    : playing_{false, false},

[Daniel Erat, 2014/10/17 22:26:00]

has this style been approved?

[rkc, 2014/10/18 00:21:55]

I thought this works with the C++03 standard onwards? I couldn't find this
listed on http://chromium-cpp.appspot.com/ under approved, banned or tbd
sections.

+      recording_{false, false},
+      player_{NULL, NULL},
+      recorder_(NULL) {
+  samples_cache_.resize(2);
+  samples_cache_[AUDIBLE] = new SamplesMap(
+      base::TimeDelta::FromMilliseconds(kSampleExpiryTimeMs), kMaxSamples);
+  samples_cache_[INAUDIBLE] = new SamplesMap(
+      base::TimeDelta::FromMilliseconds(kSampleExpiryTimeMs), kMaxSamples);
+}
+
+void AudioManager::Initialize(const DecodeSamplesCallback& decode_cb,
+                              const EncodeTokenCallback& encode_cb) {
+  decode_cb_ = decode_cb;
+  encode_cb_ = encode_cb;
+
+  if (!player_[AUDIBLE])
+    player_[AUDIBLE] = new AudioPlayer();
+  player_[AUDIBLE]->Initialize();
+
+  if (!player_[INAUDIBLE])
+    player_[INAUDIBLE] = new AudioPlayer();
+  player_[INAUDIBLE]->Initialize();
+
+  decode_cancelable_cb_.Reset(base::Bind(&AudioManager::DecodeSamplesConnector,
+                                         base::Unretained(this)));
+  if (!recorder_)
+    recorder_ = new AudioRecorder();
+  recorder_->Initialize(decode_cancelable_cb_.callback());
+}
+
+AudioManager::~AudioManager() {
+  if (player_[AUDIBLE])
+    player_[AUDIBLE]->Finalize();
+  if (player_[INAUDIBLE])
+    player_[INAUDIBLE]->Finalize();
+  if (recorder_)
+    recorder_->Finalize();
+}
+
+void AudioManager::StartPlaying(AudioType type) {
+  DCHECK_LT(type, BOTH);
+  playing_[type] = true;
+  if (samples_cache_[type]->HasKey(token_[type]) &&

[Charlie, 2014/10/17 22:42:26]

This looks like silent failure. Add a comment explaining why we sometimes call
this when the encoded token isn't available yet, and how UpdateToken() will
re-invoke it when encoding is done.

[rkc, 2014/10/18 00:21:55]

Done.

+      !player_[type]->IsPlaying()) {
+    DCHECK_GT(token_[type].size(), 0u);
+    player_[type]->Play(samples_cache_[type]->GetValue(token_[type]));
+  }
+}
+
+void AudioManager::StopPlaying(AudioType type) {
+  DCHECK_LT(type, BOTH);
+  playing_[type] = false;
+  if (player_[type]->IsPlaying())
+    player_[type]->Stop();
+}
+
+void AudioManager::StartRecording(AudioType type) {
+  DCHECK_LT(type, BOTH);
+  recording_[type] = true;
+  if (!recorder_->IsRecording())
+    recorder_->Record();
+}
+
+void AudioManager::StopRecording(AudioType type) {
+  DCHECK_LT(type, BOTH);
+  recording_[type] = false;
+  if (recorder_->IsRecording())
+    recorder_->Stop();
+}
+
+void AudioManager::SetToken(AudioType type,
+                            const std::string& url_unsafe_token) {
+  DCHECK_LT(type, BOTH);

[Charlie, 2014/10/17 22:42:25]

Why not DCHECK_NE?

[rkc, 2014/10/18 00:21:55]

We specifically want to check that the type is not greater than 1, so LT seems
like the more natural check.
Plus type could be UNKNOWN :)

[Charlie, 2014/10/18 00:44:11]

Ah, I'm used to the proto convention where UNKNOWN is always 0. Anyway, we don't
want to depend on the numeric values of the enum. Do this instead:

DCHECK(type == AUDIBLE || type == INAUDIBLE);

[rkc, 2014/10/18 00:51:30]

Done.

+  std::string token = FromUrlSafe(url_unsafe_token);
+  if (!samples_cache_[type]->HasKey(token)) {
+    encode_cb_.Run(
+        token,
+        type,
+        base::Bind(&AudioManager::OnTokenEncoded, base::Unretained(this)));
+  } else {
+    UpdateToken(type, token);
+  }
+}
+
+// Private methods.
+
+void AudioManager::OnTokenEncoded(
+    const std::string& token,
+    AudioType type,
+    const scoped_refptr<media::AudioBusRefCounted>& samples) {
+  samples_cache_[type]->Add(token, samples);
+  UpdateToken(type, token);
+}
+
+void AudioManager::UpdateToken(AudioType type, const std::string& token) {
+  DCHECK_LT(type, BOTH);
+  if (token_[type] == token)
+    return;
+
+  // Update token.
+  token_[type] = token;
+
+  // If we are supposed to be playing this token type at this moment, switch
+  // out playback with the new samples.
+  if (playing_[type]) {
+    if (player_[type]->IsPlaying())
+      player_[type]->Stop();
+    StartPlaying(type);
+  }
+}
+
+void AudioManager::DecodeSamplesConnector(const std::string& samples) {
+  AudioType decode_type = UNKNOWN;
+
+  if (recording_[AUDIBLE])
+    decode_type = AUDIBLE;
+  if (recording_[INAUDIBLE])
+    decode_type = INAUDIBLE;
+  if (recording_[AUDIBLE] && recording_[INAUDIBLE])

[Daniel Erat, 2014/10/17 22:26:00]

overwriting the previous values looks a bit weird. maybe just do something like
this?

  if (recording_[AUDIBLE])
    decode_type = recording_[INAUDIBLE] ? BOTH : AUDIBLE;
  else if (recording_[INAUDIBLE])
    decode_type = INAUDIBLE;

[Charlie, 2014/10/17 22:42:26]

Or:

if (recording_[AUDIBLE] && recording_[INAUDIBLE])
  decode_type = BOTH;
else if (recording_[AUDIBLE])
  decode_type = AUDIBLE;
else if (recording_[INAUDIBLE])
  decode_type = INAUDIBLE;

[rkc, 2014/10/18 00:21:55]

Done.

[rkc, 2014/10/18 00:21:55]

Done.

+    decode_type = BOTH;
+
+  decode_cb_.Run(decode_type, samples);
+}
+
+}  // namespace copresence