AudioMixer interface cleanup suggestions

webrtc/modules/audio_mixer/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_
 
 #include <memory>
 
 #include "webrtc/modules/audio_mixer/audio_mixer_defines.h"
 #include "webrtc/modules/include/module.h"
 #include "webrtc/modules/include/module_common_types.h"
 
 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,
     kSwbInHz = 32000,
     kFbInHz = 48000,
     kDefaultFrequency = kWbInHz
   };
 
   // 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())
   // before this function can be successfully called.
   virtual int32_t SetAnonymousMixabilityStatus(MixerAudioSource* audio_source,
                                                bool mixable) = 0;
 
   // Performs mixing by asking registered audio sources for audio. The
   // mixed result is placed in the provided AudioFrame. Can 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,
-                   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;
 
   // Return value between 0 and 0x7fff is returned by voe::AudioLevel.
   virtual int GetOutputAudioLevelFullRange() = 0;
 
+  // !!!: Remove below this point.
  protected:
   AudioMixer() {}
 
  private:
   RTC_DISALLOW_COPY_AND_ASSIGN(AudioMixer);
 };
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_MIXER_AUDIO_MIXER_H_