AudioMixer interface cleanup suggestions

webrtc/modules/audio_mixer/audio_mixer.h

Index: webrtc/modules/audio_mixer/audio_mixer.h
diff --git a/webrtc/modules/audio_mixer/audio_mixer.h b/webrtc/modules/audio_mixer/audio_mixer.h
index f84fe8cfc75006e69cc9d2ace52481974728862e..f7b88bf55868f71f3cff78ab28fb8fa06ef40665 100644
--- a/webrtc/modules/audio_mixer/audio_mixer.h
+++ b/webrtc/modules/audio_mixer/audio_mixer.h
@@ -19,9 +19,16 @@
 
 namespace webrtc {
 
+// !!!: Inherit from public rtc::RefCountInterface (see e.g. AudioDecoderFactory)
+// !!!: Document threading assumptions.
+// so life time of the mixer is less ambiguous when implemented externally.
 class AudioMixer {
  public:
+  // !!!: This is an implementation detail of the particular mixer we typically have (top 3).
+  // Move to audio_mixer_impl.h
   static const int kMaximumAmountOfMixedAudioSources = 3;
+  // !!!: Can we remove this and use an int instead? Just giving the rate in hz,
+  // rather than an enum?
   enum Frequency {
     kNbInHz = 8000,
     kWbInHz = 16000,
@@ -31,15 +38,26 @@ class AudioMixer {
   };
 
   // Factory method. Constructor disabled.
+  // !!!: Move out from class and rename to CreateTopThreeMixer()
   static std::unique_ptr<AudioMixer> Create(int id);
   virtual ~AudioMixer() {}
 
+  // !!!: Cut in two functions: AddSource(), RemoveSource(). What is mixability
+  // status and what does it do with ownership of the objects? Is the return
+  // value necessary?
+
   // Add/remove audio sources as candidates for mixing.
   virtual int32_t SetMixabilityStatus(MixerAudioSource* audio_source,
                                       bool mixable) = 0;
+  // !!!: Only used by unit test - remove from interface.
   // Returns true if an audio source is a candidate for mixing.
   virtual bool MixabilityStatus(const MixerAudioSource& audio_source) const = 0;
 
+
+  // !!!: Name this something more sensible and add a todo that it should be
+  // deprecated. It is only here to support the VoEFile API, which is legacy.
+  // Also, what is the return value?
+
   // Inform the mixer that the audio source should always be mixed and not
   // count toward the number of mixed audio sources. Note that an audio source
   // must have been added to the mixer (by calling SetMixabilityStatus())
@@ -52,13 +70,15 @@ class AudioMixer {
   // 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,
-                   size_t number_of_channels,
+                   size_t number_of_channels,  // !!!: There's a num_channels in AudioFrame. Either use that or get rid of AudioFrame.
                    AudioFrame* audio_frame_for_mixing) = 0;
 
+  // !!!: Remove from public interface.
   // Returns true if the audio source is mixed anonymously.
   virtual bool AnonymousMixabilityStatus(
       const MixerAudioSource& audio_source) const = 0;
 
+  // !!!: Can we instead return a float on [0.0, 1.0] and do any legacy adaptations in the legacy code?
   // Output level functions for VoEVolumeControl. Return value
   // between 0 and 9 is returned by voe::AudioLevel.
   virtual int GetOutputAudioLevel() = 0;
@@ -66,6 +86,7 @@ class AudioMixer {
   // Return value between 0 and 0x7fff is returned by voe::AudioLevel.
   virtual int GetOutputAudioLevelFullRange() = 0;
 
+  // !!!: Remove below this point.
  protected:
   AudioMixer() {}