Redesign the copresence audio handlers.

components/copresence/mediums/audio/audio_manager.h

Index: components/copresence/mediums/audio/audio_manager.h
diff --git a/components/copresence/mediums/audio/audio_manager.h b/components/copresence/mediums/audio/audio_manager.h
new file mode 100644
index 0000000000000000000000000000000000000000..2450f2a0bb716f9a8f4482a554e00e9aa3653964
--- /dev/null
+++ b/components/copresence/mediums/audio/audio_manager.h
@@ -0,0 +1,153 @@
+// 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.
+
+#ifndef COMPONENTS_COPRESENCE_MEDIUMS_AUDIO_AUDIO_MANAGER_H_
+#define COMPONENTS_COPRESENCE_MEDIUMS_AUDIO_AUDIO_MANAGER_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/cancelable_callback.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_vector.h"
+#include "components/copresence/mediums/audio/audio_recorder.h"
+#include "components/copresence/public/copresence_constants.h"
+#include "components/copresence/timed_map.h"
+
+namespace base {
+class MessageLoop;
+}
+
+namespace media {
+class AudioBus;
+}
+
+namespace copresence {
+
+class AudioPlayer;
+
+class AudioManager {
+ public:
+  typedef base::Callback<void(const std::string&,
+                              AudioType,
+                              const scoped_refptr<media::AudioBusRefCounted>&)>
+      SamplesCallback;

[Daniel Erat, 2014/10/21 19:43:22]

nit: rename this to something like HandleTokenCallback so the name is less
ambiguous?

(at least, i'm guessing that the general flow is that EncodeTokenCallback
generates a token and passes it to this callback.)

[rkc, 2014/10/22 00:06:20]

Nope; EncodeTokenCallback generates encoded samples, which are passed to this
callback, hence it being called SamplesCallback.

+  typedef base::Callback<void(const std::string&,
+                              AudioType,
+                              const SamplesCallback&)> EncodeTokenCallback;
+  typedef base::Callback<void(AudioType, const std::string&)>
+      DecodeSamplesCallback;
+
+  // Initializes the object. Do not use this object before calling this method.
+  virtual void Initialize(const DecodeSamplesCallback& decode_cb,

[Daniel Erat, 2014/10/21 19:43:22]

as discussed earlier, does this need to be part of the interface?

[rkc, 2014/10/22 00:06:20]

I changed my code around to remove Initialize from the interface and move it
only to the Impl classes. This broke one of my tests, without it actually being
a bug in my code at all. The code which did not call Initialize for the stub
classes, made it so that the decode_callback was never set. The
AudioManagerTest.Record test expects that when TriggerDecodeRequest() on the
stub recorder is called, correct program behavior will try to decode samples.
Since we never initialized the stub recorder (which makes this connection), the
code path never gets called and the test fails.

I can of course fix this by moving the passing of the decode callback to the
constructor but then I'd be making a change in production code that I don't
necessarily think is the optimal design, because of the insistence of not having
an Initialize method for stub classes also.

This made me realize that Initialize *does* need to be part of stub
implementations. Since we have paper thin constructors and do all our
construction in Initialize, completely bypassing Initialize doesn't sound like a
good idea. For example, the AudioRecorder *does* need to be initialized and does
need to be initialized by the AudioManager. This is a behavior that we want to
specifically test. If we take out the Initialize methods, this code path is no
longer being tested.

Unless there is a really strong reason to remove Initialize, I'd prefer to keep
it in. I don't see a particularly strong downside to letting a stub object
initialize via the same interface method that the implementation object is.

[Charlie, 2014/10/22 00:28:06]

This is for another CL, but can you remind me why we have the Initialize() calls
at all? Every time we call them, it seems follow this pattern:

if (!foo_ptr_) foo_ptr_.reset(new Foo());
foo_ptr_->Initialize(...);

I also cannot really see what's complex enough about Initialize() that it can't
be done in a constructor. This may actually be a holdover from the test classes
subclassing the prod impls: then we needed a way to set up the state differently
for testing. But we're not doing that anymore.

Summary: Add a TODO to merge Initialize() into the constructors?

[rkc, 2014/10/22 00:47:08]

We do this from tests all the time. We create the stub in the test, pass it to
the class. The class then calls Initialize() (which ends up either initializing
the real class or the stub).

[Charlie, 2014/10/22 01:03:29]

I see. But if Initialize() is really just about setting callbacks, maybe we
should have set_foo_callback() instead, and the class should be resilient to
null callbacks.

As I said, though, this is for another CL.

[rkc, 2014/10/22 14:08:49]

Initialize in this case is about setting callbacks (and connecting callbacks).
The function does exactly what the name indicates - it initializes the class.
Whether this involves setting callbacks or connecting them or doing any other
setup required for the class to function correctly, it will be done here. I I
guess I am not seeing what exactly is wrong with this pattern?

[Charlie, 2014/10/22 16:50:58]

One of the principles of encapsulation is that objects should always be in a
consistent state. This consistency is set up by the constructor and maintained
by all the other member functions. Having the constructor half-initialize the
object, and requiring a call to Initialize() before doing anything else, is
really a hack that we should use if there's no other way. It's also a way to
pretend there's no heavy lifting (e.g. initializing other objects) in the
constructor. See:

http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/

Tracked at P3 in crbug/424735.

+                          const EncodeTokenCallback& encode_cb) = 0;
+
+  virtual void StartPlaying(AudioType type) = 0;
+  virtual void StopPlaying(AudioType type) = 0;
+
+  virtual void StartRecording(AudioType type) = 0;
+  virtual void StopRecording(AudioType type) = 0;
+
+  virtual void SetToken(AudioType type,
+                        const std::string& url_unsafe_token) = 0;
+
+  virtual const std::string& get_token(AudioType type) = 0;

[Daniel Erat, 2014/10/21 19:43:22]

rename to GetToken()

[rkc, 2014/10/22 00:06:20]

Renamed back to GetToken.
Done.

+
+  virtual bool IsRecording(AudioType type) = 0;
+  virtual bool IsPlaying(AudioType type) = 0;
+
+  virtual ~AudioManager(){};

[Daniel Erat, 2014/10/21 19:43:22]

nit: move this up after the typedefs, add a space between () and {}, and remove
the trailing semicolon

[rkc, 2014/10/22 00:06:20]

Done.

+};
+
+// The AudioManagerImpl class manages the playback and recording of tokens. Once
+// playback or recording is started, it is up to the audio manager to handle
+// the specifics of how this is done. In the future, for example, this class
+// may pause recording and playback to implement carrier sense.
+class AudioManagerImpl final : public AudioManager {

[Charlie, 2014/10/21 19:59:42]

Does this need to be in the same .h file? Who references the impl class besides
audio_manager.cc?

URLFetcher, for example, has:
url_fetcher.h
url_fetcher_impl.h
url_fetcher_impl.cc

Same for AudioPlayerImpl and AudioRecorderImpl.

[rkc, 2014/10/22 00:06:20]

Done.

+ public:
+  AudioManagerImpl();
+  virtual ~AudioManagerImpl();
+
+  // AudioManager overrides:
+  virtual void Initialize(const DecodeSamplesCallback& decode_cb,
+                          const EncodeTokenCallback& encode_cb) override;
+  virtual void StartPlaying(AudioType type) override;
+  virtual void StopPlaying(AudioType type) override;
+  virtual void StartRecording(AudioType type) override;
+  virtual void StopRecording(AudioType type) override;
+  virtual void SetToken(AudioType type,
+                        const std::string& url_unsafe_token) override;
+  virtual const std::string& get_token(AudioType type) override;
+  virtual bool IsRecording(AudioType type) override;
+  virtual bool IsPlaying(AudioType type) override;
+
+  void set_player_for_testing(AudioType type, AudioPlayer* player) {
+    player_[type] = player;
+  }
+  void set_recorder_for_testing(AudioRecorder* recorder) {
+    recorder_ = recorder;
+  }
+
+ private:
+  friend class AudioManagerImplTest;
+  FRIEND_TEST_ALL_PREFIXES(AudioManagerImplTest, BasicRecordAndStop);
+  FRIEND_TEST_ALL_PREFIXES(AudioManagerImplTest,
+                           OutOfOrderRecordAndStopMultiple);
+
+  typedef TimedMap<std::string, scoped_refptr<media::AudioBusRefCounted>>
+      SamplesMap;
+
+  // This is the method that the whispernet client needs to call to return
+  // samples to us.
+  void OnTokenEncoded(const std::string& token,
+                      AudioType type,
+                      const scoped_refptr<media::AudioBusRefCounted>& samples);
+
+  // Update our currently playing token with the new token. Change the playing
+  // samples if needed. Prerequisite: Samples corresponding to this token
+  // should already be in the samples cache.
+  void UpdateToken(AudioType type, const std::string& token);
+
+  // Connector callback to forward the audio samples to Whispernet, based on
+  // the type of recording we've been instructed to do.
+  void DecodeSamplesConnector(const std::string& samples);
+
+  // Callbacks to speak with whispernet.
+  DecodeSamplesCallback decode_cb_;
+  EncodeTokenCallback encode_cb_;
+
+  // This cancelable callback is passed to the recorder. The recorder's
+  // destruction will happen on the audio thread, so it can outlive us.
+  base::CancelableCallback<void(const std::string&)> decode_cancelable_cb_;
+
+  // We use the AudioType enum to index into all our data structures that work
+  // on values for both audible and inaudible.
+  static_assert(AUDIBLE == 0, "AudioType::AUDIBLE should be 0.");
+  static_assert(INAUDIBLE == 1, "AudioType::INAUDIBLE should be 1.");
+
+  // Indexed using enum AudioType.
+  bool playing_[2];
+  bool recording_[2];
+
+  // AudioPlayer and AudioRecorder objects are self-deleting. When we call
+  // Finalize on them, they clean themselves up on the Audio thread.
+  // Indexed using enum AudioType.
+  AudioPlayer* player_[2];
+  AudioRecorder* recorder_;
+
+  // Indexed using enum AudioType.
+  std::string token_[2];
+
+  // Cache that holds the encoded samples. After reaching its limit, the cache
+  // expires the oldest samples first.
+  // Indexed using enum AudioType.
+  ScopedVector<SamplesMap> samples_cache_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioManagerImpl);
+};
+
+}  // namespace copresence
+
+#endif  // COMPONENTS_COPRESENCE_MEDIUMS_AUDIO_AUDIO_MANAGER_H_