Split audio mixer into interface and implementation.

webrtc/api/audio/audio_mixer.h

 /*
  *  Copyright (c) 2011 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.
  */
 
-#ifndef WEBRTC_MODULES_AUDIO_MIXER_AUDIO_MIXER_H_
-#define WEBRTC_MODULES_AUDIO_MIXER_AUDIO_MIXER_H_
+#ifndef WEBRTC_API_AUDIO_AUDIO_MIXER_H_
+#define WEBRTC_API_AUDIO_AUDIO_MIXER_H_
 
 #include <memory>
 
 #include "webrtc/base/refcount.h"
 #include "webrtc/modules/include/module_common_types.h"
 
 namespace webrtc {
 
 class AudioMixer : public rtc::RefCountInterface {
  public:
   // A callback class that all mixer participants must inherit from/implement.
   class Source {
    public:
     enum class AudioFrameInfo {
       kNormal,  // The samples in audio_frame are valid and should be used.
       kMuted,   // The samples in audio_frame should not be used, but should be
       // implicitly interpreted as zero. Other fields in audio_frame
       // may be read and should contain meaningful values.
       kError  // audio_frame will not be used.

[kwiberg-webrtc, 2016/10/14 07:52:22]

Nits: Comment indentation. Capitalize first word in all sentences. Comma after
last enum too.

[aleloi, 2016/10/14 09:50:32]

Done.

     };
 
     struct AudioFrameWithInfo {
       AudioFrame* audio_frame;
       AudioFrameInfo audio_frame_info;
     };
 
     // The implementation of GetAudioFrameWithInfo should update
     // audio_frame with new audio every time it's called. Implementing
     // classes are allowed to return the same AudioFrame pointer on
     // different calls. The pointer must stay valid until the next
     // mixing call or until this audio source is disconnected from the
     // mixer. The mixer may modify the contents of the passed
     // AudioFrame pointer at any time until the next call to
     // GetAudioFrameWithInfo, or until the source is removed from the
     // mixer.
     virtual AudioFrameWithInfo GetAudioFrameWithInfo(int sample_rate_hz) = 0;

[kwiberg-webrtc, 2016/10/14 07:52:22]

Is it too late to change this? Returning pointers to things that the caller may
read (or worse, modify) for some time afterwards makes the code hard to reason
about.

One alternative that works in many cases is to take a callback (e.g. a
FunctionView) that receives (a pointer to) the stuff as an argument; the
callback may then manipulate the thing while it's running.

However, in this case I guess it'd also be OK for the caller to pass an
AudioFrame* to be filled in?

[the sun, 2016/10/14 08:41:04]

I think the latter suggestion is a good one. Then we could avoid the additional
AudioFrame buffer inside voe::Channel and the mixer can be as dumb/clever as it
wants juggling buffers (the simplest impl would just need two buffers).

[aleloi, 2016/10/14 09:50:32]

I agree that passing an AudioFrame* owned by the mixer to the source
is a good simplification of the interface! It definitely becomes
simpler.

Here is how it can be implemented (it doesn't require any large
changes). We already added an 'audio source wrapper' data structure
currently called 'SourceStatus' which persists across mix iterations
and keeps track of whether the source was mixed the last time. An
AudioFrame can easily be added to it. The AudioFrame pointer is then
valid from the point AddSource is called until RemoveSource is called
on the same source.

This change would move back to how (that part of) the mixer interface
looked before the change in
https://codereview.webrtc.org/2127763002/. The reason for the change
in the first place was to remove the memory pool from the mixer
without having to copy audio frames.

There is one (solvable) problem though: I can't just add a non-pointer
AudioFrame to SourceStatus, because the SourceStatuses are stored in a
vector, and AudioFrame can't be copied. So the mixer would have to
keep lots of AudioFrames somewhere else :(

[aleloi, 2016/10/14 09:50:33]

I'm not sure I understand the implications of the callback
suggestion. Is the main point to only allow reading/modifying the
frame from inside the callback?

At some point in the mixing algorithm, the mixer has to look at all
frames to select which ones to use. But only one frame at a time would
be available in the callback. The callback would then need to leak the
frame pointers elswhere.

This is how I understand the callback suggestion:
Suppose GetAudioFrame takes a callback DoThisWithTheFrame,
which takes a frame pointer.

Mixer::GetAudio() {
  for each source:
    source.GetAudio(DoThisWithTheFrame)
}

DoThisWithTheFrame(frame_ptr) {
  add frame to mixer data structures
}

mix_result Mixer::Mix() {
  GetAudio();
  select what frames to use from internal data structures
  mix frames together
}

 
     // A way for a mixer implementation do distinguish participants.

[kwiberg-webrtc, 2016/10/14 07:52:22]

to

[aleloi, 2016/10/14 09:50:32]

Done.

     virtual int ssrc() = 0;

[kwiberg-webrtc, 2016/10/14 07:52:22]

Nit: virtual methods should never be lower-case, because the interface can't
guarantee that they'll be cheap to call.

[aleloi, 2016/10/14 09:50:33]

Done.

 
    protected:
     virtual ~Source() {}
   };
 
   // Since the mixer is reference counted, the destructor may be
   // called from any thread.
   ~AudioMixer() override {}
 
   // Returns true if adding/removing was successful. A source is never
   // added twice and removal is never attempted if a source has not
   // been successfully added to the mixer. Addition and removal can
   // happen on different threads.
   virtual bool AddSource(Source* audio_source) = 0;
   virtual bool RemoveSource(Source* audio_source) = 0;
 
   // Performs mixing by asking registered audio sources for audio. The
   // mixed result is placed in the provided AudioFrame. Will only be
   // called from a single thread. The rate and channels arguments
   // specify the rate and number of channels of the mix result.
   virtual void Mix(int sample_rate_hz,
                    size_t number_of_channels,
                    AudioFrame* audio_frame_for_mixing) = 0;
 };
 }  // namespace webrtc
 
-#endif  // WEBRTC_MODULES_AUDIO_MIXER_AUDIO_MIXER_H_
+#endif  // WEBRTC_API_AUDIO_AUDIO_MIXER_H_