Adding AudioDeviceDataObserver interface

webrtc/modules/audio_device/audio_transport_capture.cc

+/*
+ *  Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/modules/audio_device/include/audio_transport_capture.h"
+
+namespace webrtc {
+
+namespace {
+
+// A wrapper over AudioDeviceModule that registers itself as AudioTransport
+// callback and redirects the PCM data to AudioTransportCapture callback.
+class ADMWrapper : public AudioDeviceModule,
+                   public AudioTransport {
+ public:
+  explicit ADMWrapper(
+      const int32_t id,
+      const AudioLayer audio_layer,
+      AudioTransportCapture* capture_cb)

[the sun, 2017/03/21 21:42:50]

No need for this extra interface.

+      : impl_(AudioDeviceModule::Create(id, audio_layer)),
+        capture_callback_(capture_cb) {
+    // Register self as the audio transport callback for underlying ADM impl.
+    auto res = impl_->RegisterAudioCallback(this);
+    is_valid_ = (impl_.get() != nullptr) && (res == 0);
+  }
+  virtual ~ADMWrapper() {
+    audio_transport_sink_ = nullptr;
+    capture_callback_ = nullptr;
+  }
+
+  bool IsValid() {

[the sun, 2017/03/21 21:42:50]

When is this useful?

[lliuu, 2017/03/22 21:26:14]

I don't want to end up with the situation where the internal impl_ fails to
initialize in AudioDeviceModule::Create and we still return a valid ADMWrapper
pointer to the user. This also is relevant regarding whether we want to provide
a static Create function below

+    return is_valid_;
+  }
+
+  // RefCountedObject methods overrides.

[henrika_webrtc, 2017/03/23 09:08:52]

How are these related to RefCountedObject?

[lliuu, 2017/03/24 00:12:58]

Hi Henrik, AudioDeviceModule inherits from RefCountedObject. I've found this
kind of a headache that creating subclasses of ADM also means having overriding
RefCountedObject, is there some way we can avoid doing it?

[henrika_webrtc, 2017/03/29 07:42:14]

Don't think you can at this stage but I would like to get rid of that dependency
as well.

My comment was based on that it looks like the methods you override are from
"webrtc/modules/include/module.h" and not from RefCount.

[tommi, 2017/03/29 08:44:33]

Correct - btw lliuu - AudioDeviceModule inherits from RefCountedModule not
RefCountedObject. Although the names are similar, those classes are very
different.  The former is a pure interface, the latter is a template
implementation to satisfy an implementation of RefCountInterface. 
RefCountInterface has more in common with RefCountedModule, but they're still
different classes, so it's important to not confuse them.

+  int64_t TimeUntilNextProcess() override {
+    return impl_->TimeUntilNextProcess();
+  }
+  void Process() override {
+    return impl_->Process();
+  }
+
+  // AudioTransport methods overrides.
+  int32_t RecordedDataIsAvailable(
+      const void* audioSamples, const size_t nSamples,
+      const size_t nBytesPerSample, const size_t nChannels,
+      const uint32_t samplesPerSec, const uint32_t totalDelayMS,
+      const int32_t clockDrift, const uint32_t currentMicLevel,
+      const bool keyPressed, uint32_t& newMicLevel) override {
+    int32_t res = 0;
+    // Capture PCM data before sending to sink.
+    if (capture_callback_) {
+      capture_callback_->CaptureRecordedData(audioSamples, nSamples,

[the sun, 2017/03/21 21:42:50]

No need for this new interface - just pass the call on like you do below.

A user would subclass the ADMWrapper:

class MyInterceptingADM : public ADMWrapper {
public:
  ...
  int32_t RecordedDataIsAvailable(...) {
    // Do what I please with the data

    // Then call base class.
    return ADMWrapper::RecordedDataIsAvailable(...);
  }

[lliuu, 2017/03/22 18:50:30]

Fredrik, what do you think of adding pure virtual CaptureRecordedData to
AdmWrapper, and the user can just provide their own implementation without
needing to call base class? In this way we don't have to depend on the user to
do the right thing 

[lliuu, 2017/03/22 20:34:37]

It also occurred to me that if we're letting the user to override
RecordedDataIsAvailable and NeedMorePlayData, s/he would need to make sure to
call the base functions in different orders in those 2 override functions, which
might not be intuitive

[lliuu, 2017/03/22 21:26:14]

I'd like to clarify the last comment: I meant that if user overrides
NeedMorePlayData below, they need to call the base function *before* their
custom logic, which might not be obvious to the user

[the sun, 2017/03/28 23:33:45]

I see, those are fair points.

+                                             nBytesPerSample, nChannels,
+                                             samplesPerSec);
+    }
+
+    // Send to sink.
+    if (audio_transport_sink_) {
+      res = audio_transport_sink_->RecordedDataIsAvailable(
+          audioSamples, nSamples, nBytesPerSample,
+          nChannels, samplesPerSec, totalDelayMS,
+          clockDrift, currentMicLevel, keyPressed,
+          newMicLevel);
+    }
+
+    return res;
+  }
+
+  int32_t NeedMorePlayData(const size_t nSamples, const size_t nBytesPerSample,
+                           const size_t nChannels, const uint32_t samplesPerSec,
+                           void* audioSamples, size_t& nSamplesOut,
+                           int64_t* elapsed_time_ms,
+                           int64_t* ntp_time_ms) override {
+    int32_t res = 0;
+    // Request data from sink.

[henrika_webrtc, 2017/03/23 09:08:52]

How can we request data *from* a sink?

[lliuu, 2017/03/24 00:12:58]

Good point. Here "sink" means the actual sink for audio transport callback
functions, but I can see this could easily lead to confusions. Will rename it

+    if (audio_transport_sink_) {
+      res = audio_transport_sink_->NeedMorePlayData(
+          nSamples, nBytesPerSample, nChannels,
+          samplesPerSec, audioSamples, nSamplesOut,
+          elapsed_time_ms, ntp_time_ms);
+    }
+
+    // Capture after data requested.
+    if (capture_callback_) {
+      capture_callback_->CapturePlayoutData(audioSamples, nSamples,
+                                            nBytesPerSample, nChannels,
+                                            samplesPerSec);
+    }
+
+    return res;
+  }
+
+  void PushCaptureData(int voe_channel, const void* audio_data,
+                       int bits_per_sample, int sample_rate,
+                       size_t number_of_channels,
+                       size_t number_of_frames) override {
+    if (audio_transport_sink_) {
+      audio_transport_sink_->PushCaptureData(voe_channel, audio_data,
+                                             bits_per_sample, sample_rate,
+                                             number_of_channels,
+                                             number_of_frames);
+    }
+  }
+
+  void PullRenderData(int bits_per_sample, int sample_rate,
+                      size_t number_of_channels, size_t number_of_frames,
+                      void* audio_data, int64_t* elapsed_time_ms,
+                      int64_t* ntp_time_ms) override {
+    if (audio_transport_sink_) {
+      audio_transport_sink_->PullRenderData(
+          bits_per_sample, sample_rate, number_of_channels,
+          number_of_frames, audio_data, elapsed_time_ms,
+          ntp_time_ms);
+    }
+  }
+
+  // Override AudioDeviceModule's RegisterAudioCallback method to remember the
+  // actual audio transport sink (e.g.: voice engine).
+  int32_t RegisterAudioCallback(AudioTransport* audioCallback) override {
+    // Remember the audio callback to forward PCM data
+    audio_transport_sink_ = audioCallback;

[the sun, 2017/03/21 21:42:50]

audio_transport_sink_ will need to be protected by a lock

[lliuu, 2017/03/22 18:50:30]

Acknowledged.

[lliuu, 2017/03/22 21:26:14]

Actually could you clarify what is the race condition that needs to be protected
by a lock? The audio transport callbacks don't seem to be protected in the
default ADM/VOE implementations, and I don't feel there's the need to lock
between RegisterAudioCallback and the callback functions?

[the sun, 2017/03/28 23:33:45]

Right you are. That's the same implicit contract that I was referring to in
another comment. :)

+    return 0;
+  }
+
+  // AudioDeviceModule pass through method overrides.
+  int32_t ActiveAudioLayer(AudioLayer* audioLayer) const override {
+    return impl_->ActiveAudioLayer(audioLayer);
+  }
+  ErrorCode LastError() const override {
+    return impl_->LastError();
+  }
+  int32_t RegisterEventObserver(AudioDeviceObserver* eventCallback) override {
+    return impl_->RegisterEventObserver(eventCallback);
+  }
+  int32_t Init() override {
+    return impl_->Init();
+  }
+  int32_t Terminate() override {
+    return impl_->Terminate();
+  }
+  bool Initialized() const override {
+    return impl_->Initialized();
+  }
+  int16_t PlayoutDevices() override {
+    return impl_->PlayoutDevices();
+  }
+  int16_t RecordingDevices() override {
+    return impl_->RecordingDevices();
+  }
+  int32_t PlayoutDeviceName(uint16_t index,
+                            char name[kAdmMaxDeviceNameSize],
+                            char guid[kAdmMaxGuidSize]) override {
+    return impl_->PlayoutDeviceName(index, name, guid);
+  }
+  int32_t RecordingDeviceName(uint16_t index,
+                              char name[kAdmMaxDeviceNameSize],
+                              char guid[kAdmMaxGuidSize]) override {
+    return impl_->RecordingDeviceName(index, name, guid);
+  }
+  int32_t SetPlayoutDevice(uint16_t index) override {
+    return impl_->SetPlayoutDevice(index);
+  }
+  int32_t SetPlayoutDevice(WindowsDeviceType device) override {
+    return impl_->SetPlayoutDevice(device);
+  }
+  int32_t SetRecordingDevice(uint16_t index) override {
+    return impl_->SetRecordingDevice(index);
+  }
+  int32_t SetRecordingDevice(WindowsDeviceType device) override {
+    return impl_->SetRecordingDevice(device);
+  }
+  int32_t PlayoutIsAvailable(bool* available) override {
+    return impl_->PlayoutIsAvailable(available);
+  }
+  int32_t InitPlayout() override {
+    return impl_->InitPlayout();
+  }
+  bool PlayoutIsInitialized() const override {
+    return impl_->PlayoutIsInitialized();
+  }
+  int32_t RecordingIsAvailable(bool* available) override {
+    return impl_->RecordingIsAvailable(available);
+  }
+  int32_t InitRecording() override {
+    return impl_->InitRecording();
+  }
+  bool RecordingIsInitialized() const override {
+    return impl_->RecordingIsInitialized();
+  }
+  int32_t StartPlayout() override {
+    return impl_->StartPlayout();
+  }
+  int32_t StopPlayout() override {
+    return impl_->StopPlayout();
+  }
+  bool Playing() const override {
+    return impl_->Playing();
+  }
+  int32_t StartRecording() override {
+    return impl_->StartRecording();
+  }
+  int32_t StopRecording() override {
+    return impl_->StopRecording();
+  }
+  bool Recording() const override {
+    return impl_->Recording();
+  }
+  int32_t SetAGC(bool enable) override {
+    return impl_->SetAGC(enable);
+  }
+  bool AGC() const override {
+    return impl_->AGC();
+  }
+  int32_t SetWaveOutVolume(uint16_t volumeLeft,
+                           uint16_t volumeRight) override {
+    return impl_->SetWaveOutVolume(volumeLeft, volumeRight);
+  }
+  int32_t WaveOutVolume(uint16_t* volumeLeft,
+                        uint16_t* volumeRight) const override {
+    return impl_->WaveOutVolume(volumeLeft, volumeRight);
+  }
+  int32_t InitSpeaker() override {
+    return impl_->InitSpeaker();
+  }
+  bool SpeakerIsInitialized() const override {
+    return impl_->SpeakerIsInitialized();
+  }
+  int32_t InitMicrophone() override {
+    return impl_->InitMicrophone();
+  }
+  bool MicrophoneIsInitialized() const override {
+    return impl_->MicrophoneIsInitialized();
+  }
+  int32_t SpeakerVolumeIsAvailable(bool* available) override {
+    return impl_->SpeakerVolumeIsAvailable(available);
+  }
+  int32_t SetSpeakerVolume(uint32_t volume) override {
+    return impl_->SetSpeakerVolume(volume);
+  }
+  int32_t SpeakerVolume(uint32_t* volume) const override {
+    return impl_->SpeakerVolume(volume);
+  }
+  int32_t MaxSpeakerVolume(uint32_t* maxVolume) const override {
+    return impl_->MaxSpeakerVolume(maxVolume);
+  }
+  int32_t MinSpeakerVolume(uint32_t* minVolume) const override {
+    return impl_->MinSpeakerVolume(minVolume);
+  }
+  int32_t SpeakerVolumeStepSize(uint16_t* stepSize) const override {
+    return impl_->SpeakerVolumeStepSize(stepSize);
+  }
+  int32_t MicrophoneVolumeIsAvailable(bool* available) override {
+    return impl_->MicrophoneVolumeIsAvailable(available);
+  }
+  int32_t SetMicrophoneVolume(uint32_t volume) override {
+    return impl_->SetMicrophoneVolume(volume);
+  }
+  int32_t MicrophoneVolume(uint32_t* volume) const override {
+    return impl_->MicrophoneVolume(volume);
+  }
+  int32_t MaxMicrophoneVolume(uint32_t* maxVolume) const override {
+    return impl_->MaxMicrophoneVolume(maxVolume);
+  }
+  int32_t MinMicrophoneVolume(uint32_t* minVolume) const override {
+    return impl_->MinMicrophoneVolume(minVolume);
+  }
+  int32_t MicrophoneVolumeStepSize(uint16_t* stepSize) const override {
+    return impl_->MicrophoneVolumeStepSize(stepSize);
+  }
+  int32_t SpeakerMuteIsAvailable(bool* available) override {
+    return impl_->SpeakerMuteIsAvailable(available);
+  }
+  int32_t SetSpeakerMute(bool enable) override {
+    return impl_->SetSpeakerMute(enable);
+  }
+  int32_t SpeakerMute(bool* enabled) const override {
+    return impl_->SpeakerMute(enabled);
+  }
+  int32_t MicrophoneMuteIsAvailable(bool* available) override {
+    return impl_->MicrophoneMuteIsAvailable(available);
+  }
+  int32_t SetMicrophoneMute(bool enable) override {
+    return impl_->SetMicrophoneMute(enable);
+  }
+  int32_t MicrophoneMute(bool* enabled) const override {
+    return impl_->MicrophoneMute(enabled);
+  }
+  int32_t MicrophoneBoostIsAvailable(bool* available) override {
+    return impl_->MicrophoneBoostIsAvailable(available);
+  }
+  int32_t SetMicrophoneBoost(bool enable) override {
+    return impl_->SetMicrophoneBoost(enable);
+  }
+  int32_t MicrophoneBoost(bool* enabled) const override {
+    return impl_->MicrophoneBoost(enabled);
+  }
+  int32_t StereoPlayoutIsAvailable(bool* available) const override {
+    return impl_->StereoPlayoutIsAvailable(available);
+  }
+  int32_t SetStereoPlayout(bool enable) override {
+    return impl_->SetStereoPlayout(enable);
+  }
+  int32_t StereoPlayout(bool* enabled) const override {
+    return impl_->StereoPlayout(enabled);
+  }
+  int32_t StereoRecordingIsAvailable(bool* available) const override {
+    return impl_->StereoRecordingIsAvailable(available);
+  }
+  int32_t SetStereoRecording(bool enable) override {
+    return impl_->SetStereoRecording(enable);
+  }
+  int32_t StereoRecording(bool* enabled) const override {
+    return impl_->StereoRecording(enabled);
+  }
+  int32_t SetRecordingChannel(const ChannelType channel) override {
+    return impl_->SetRecordingChannel(channel);
+  }
+  int32_t RecordingChannel(ChannelType* channel) const override {
+    return impl_->RecordingChannel(channel);
+  }
+  int32_t SetPlayoutBuffer(const BufferType type,
+                           uint16_t sizeMS) override {
+    return impl_->SetPlayoutBuffer(type, sizeMS);
+  }
+  int32_t PlayoutBuffer(BufferType* type, uint16_t* sizeMS) const override {
+    return impl_->PlayoutBuffer(type, sizeMS);
+  }
+  int32_t PlayoutDelay(uint16_t* delayMS) const override {
+    return impl_->PlayoutDelay(delayMS);
+  }
+  int32_t RecordingDelay(uint16_t* delayMS) const override {
+    return impl_->RecordingDelay(delayMS);
+  }
+  int32_t CPULoad(uint16_t* load) const override {
+    return impl_->CPULoad(load);
+  }
+  int32_t StartRawOutputFileRecording(
+      const char pcmFileNameUTF8[kAdmMaxFileNameSize]) override {
+    return impl_->StartRawOutputFileRecording(pcmFileNameUTF8);
+  }
+  int32_t StopRawOutputFileRecording() override {
+    return impl_->StopRawOutputFileRecording();
+  }
+  int32_t StartRawInputFileRecording(
+      const char pcmFileNameUTF8[kAdmMaxFileNameSize]) override {
+    return impl_->StartRawInputFileRecording(pcmFileNameUTF8);
+  }
+  int32_t StopRawInputFileRecording() override {
+    return impl_->StopRawInputFileRecording();
+  }
+  int32_t SetRecordingSampleRate(const uint32_t samplesPerSec) override {
+    return impl_->SetRecordingSampleRate(samplesPerSec);
+  }
+  int32_t RecordingSampleRate(uint32_t* samplesPerSec) const override {
+    return impl_->RecordingSampleRate(samplesPerSec);
+  }
+  int32_t SetPlayoutSampleRate(const uint32_t samplesPerSec) override {
+    return impl_->SetPlayoutSampleRate(samplesPerSec);
+  }
+  int32_t PlayoutSampleRate(uint32_t* samplesPerSec) const override {
+    return impl_->PlayoutSampleRate(samplesPerSec);
+  }
+  int32_t ResetAudioDevice() override {
+    return impl_->ResetAudioDevice();
+  }
+  int32_t SetLoudspeakerStatus(bool enable) override {
+    return impl_->SetLoudspeakerStatus(enable);
+  }
+  int32_t GetLoudspeakerStatus(bool* enabled) const override {
+    return impl_->GetLoudspeakerStatus(enabled);
+  }
+  bool BuiltInAECIsAvailable() const override {
+    return impl_->BuiltInAECIsAvailable();
+  }
+  bool BuiltInAGCIsAvailable() const override {
+    return impl_->BuiltInAGCIsAvailable();
+  }
+  bool BuiltInNSIsAvailable() const override {
+    return impl_->BuiltInNSIsAvailable();
+  }
+  int32_t EnableBuiltInAEC(bool enable) override {
+    return impl_->EnableBuiltInAEC(enable);
+  }
+  int32_t EnableBuiltInAGC(bool enable) override {
+    return impl_->EnableBuiltInAGC(enable);
+  }
+  int32_t EnableBuiltInNS(bool enable) override {
+    return impl_->EnableBuiltInNS(enable);
+  }
+// Only supported on iOS.
+#if defined(WEBRTC_IOS)
+  int GetPlayoutAudioParameters(AudioParameters* params) const override {
+    return impl_->GetPlayoutAudioParameters(params);
+  }
+  int GetRecordAudioParameters(AudioParameters* params) const override {
+    return impl_->GetRecordAudioParameters(params);
+  }
+#endif  // WEBRTC_IOS
+
+ protected:
+  rtc::scoped_refptr<AudioDeviceModule> impl_;
+  AudioTransportCapture* capture_callback_ = nullptr;
+  AudioTransport* audio_transport_sink_ = nullptr;
+  bool is_valid_ = false;
+};
+
+
+}  // namespace
+
+rtc::scoped_refptr<AudioDeviceModule>
+    CreateAudioDeviceWithTransportCapture(

[the sun, 2017/03/21 21:42:50]

No need for this create function if we can agree to use sub classing and avoid
the AudioTransportCapture interface.

[lliuu, 2017/03/22 18:50:30]

AudioDeviceModule inherits from RefCountedModule, and I think user might
actually want to have a create function that returns a ref counted pointer? I
could move it to a static member function AdmWrapper::Create, does that sound
like a good solution?

+        const int32_t id,
+        const AudioDeviceModule::AudioLayer audio_layer,
+        AudioTransportCapture* proc_callback) {
+  rtc::scoped_refptr<ADMWrapper>
+      audio_device(
+          new rtc::RefCountedObject<ADMWrapper>(
+              id, audio_layer, proc_callback));
+
+  if (!audio_device->IsValid()) {
+    return nullptr;
+  }
+
+  return audio_device;
+}
+
+}  // namespace webrtc