Replace AudioConferenceMixer with AudioMixer.

webrtc/audio/audio_state.cc

Index: webrtc/audio/audio_state.cc
diff --git a/webrtc/audio/audio_state.cc b/webrtc/audio/audio_state.cc
index e63f97af2d4fb9fd3123f37532f0553382715984..8d6c84b4997132d0b2b6413f35c68c903ef69f94 100644
--- a/webrtc/audio/audio_state.cc
+++ b/webrtc/audio/audio_state.cc
@@ -13,16 +13,33 @@
 #include "webrtc/base/atomicops.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/modules/audio_device/include/audio_device.h"
+#include "webrtc/modules/audio_mixer/audio_mixer_impl.h"
 #include "webrtc/voice_engine/include/voe_errors.h"
 
 namespace webrtc {
 namespace internal {
 
 AudioState::AudioState(const AudioState::Config& config)
-    : config_(config), voe_base_(config.voice_engine) {
+    : config_(config),
+      voe_base_(config.voice_engine),
+      mixer_(AudioMixerImpl::Create()) {

[aleloi, 2016/10/24 11:59:14]

Alternative: pass a unique_ptr<AudioMixer> and move creation to
webrtcvoiceengine or even higher. That way we could do integration tests with
mixer mocks!

[ossu, 2016/10/25 14:13:33]

I do believe integration testing is supposed to be done without mocks, with
mocks used to facilitate unit testing. If we don't want pluggable mixers
(unsure), it's probably easier to have AudioState create the mixer and instead
make AudioState mockable for the cases where we want to use something else.

With a mockable AudioState, the AudioState implementation could just have an
AudioMixerImpl object, rather than having to call AudioMixerImpl::Create(). If
you wanted to hide the implementation of the AudioMixer, shouldn't the Create
method be on AudioMixer instead of AudioMixerImpl? i.e. AudioMixer::Create()
(which does new AudioMixerImpl).

[the sun, 2016/10/27 10:06:45]

We want the mixer to be pluggable to allow alternative implementations, so it
should be passed in the AudioState::Config instead. Put the creation in
webrtcvoiceengine.cc:MakeAudioStateConfig() for now.

[aleloi, 2016/11/01 15:17:35]

Changes: A mixer instance is passed in the config struct. There are divided
opinions on whether to make it a unique_ptr and remove the refcounting.

   process_thread_checker_.DetachFromThread();
   // Only one AudioState should be created per VoiceEngine.
   RTC_CHECK(voe_base_->RegisterVoiceEngineObserver(*this) != -1);
+
+  // If mixer construction failed, there is no point to continue.
+  RTC_CHECK(mixer_);

[the sun, 2016/10/27 10:06:46]

This should be a DCHECK instead. Program should have terminated already if new
failed.

[aleloi, 2016/11/01 15:17:35]

Done.

+
+  auto* const device = audio_device();
+  if (device) {

[the sun, 2016/10/27 10:06:45]

DCHECK that instead.

[aleloi, 2016/11/01 15:17:35]

Done.

+    audio_transport_proxy_.reset(new AudioTransportProxy(
+        voe_base_->audio_transport(), voe_base_->audio_processing(), mixer()));
+    device->RegisterAudioCallback(nullptr);

[the sun, 2016/10/27 10:06:45]

Doesn't look like this is necessary?

[aleloi, 2016/11/01 15:17:35]

I had a vague memory that it broke something. Webrtc tests pass without
register(nullptr), but I have some chromium issues now that were not there
before.

[aleloi, 2016/11/01 15:17:35]

It is in Chrome.
WebRtcAudioDeviceImpl::RegisterAudioCallback contains this line:
DCHECK_EQ(audio_transport_callback_ == NULL, audio_callback != NULL);

+    device->RegisterAudioCallback(audio_transport_proxy_.get());
+  } else {
+    LOG(LS_ERROR) << "No audio device for sound playout.";
+  }

[aleloi, 2016/10/24 11:59:14]

A few details, some of which should maybe be made into code comments:

All of |audio_transport(), audio_processing(), audio_device()| can be
|nullptr| (e.g. in tests). I've tried to handle that by adding error logs
wherever (here and in audio_transport_proxy) a resource that may be NULL is
used.

If the transport or APM is NULL, we could still receive and produce
audio through the mixer. There are checks in AudioTransportProxy that
make sure never to access the APM and transport in that case.

If the device is NULL, no audio will be played out, requested or
mixed. Then there is no point in constructing an AudioTransportProxy.

[ossu, 2016/10/25 14:13:33]

When will this happen? Only in tests?

[the sun, 2016/10/27 10:06:46]

If possible to avoid these special cases by updating the tests, that could be a
better way. I generally prefer to use empty implementations instead of littering
the code with null checks. The null checks add implicit extra states the class
may be in, which makes it harder to reason about correctness.

[aleloi, 2016/11/01 15:17:35]

@ossu, yes only in tests. I've updated all tests to send an audio device in a
dependence CL.

 }
 
 AudioState::~AudioState() {
@@ -35,6 +52,18 @@ VoiceEngine* AudioState::voice_engine() {
   return config_.voice_engine;
 }
 
+AudioDeviceModule* AudioState::audio_device() {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  if (config_.audio_device_module) {

[the sun, 2016/10/27 10:06:45]

This field appears unused right now. Just stick to calling
voe_base_->audio_device_module(); (and remove this utility method). Also,
comment out the field in AudioState::Config.

[aleloi, 2016/11/01 15:17:35]

IIRC, that broke some test. I'll take a look at it!

+    return config_.audio_device_module;
+  }
+  return voe_base_->audio_device_module();
+}
+
+rtc::scoped_refptr<AudioMixer> AudioState::mixer() const {

[ossu, 2016/10/25 14:13:33]

Is the AudioMixer supposed to be able to outlive AudioState? If not, why is it
reference counted?

[the sun, 2016/10/27 10:06:45]

Ultimately, it will possibly be supplied by an API client and therefore
ownership is shared.

+  return mixer_;
+}
+
 bool AudioState::typing_noise_detected() const {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   rtc::CritScope lock(&crit_sect_);