AudioMixer interface cleanup suggestions

webrtc/modules/audio_mixer/audio_mixer_impl.cc

 /*
  *  Copyright (c) 2012 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_mixer/audio_mixer_impl.h"
 
 #include <algorithm>
 #include <functional>
 #include <utility>
 
 #include "webrtc/modules/audio_mixer/audio_frame_manipulator.h"
 #include "webrtc/modules/utility/include/audio_frame_operations.h"
 #include "webrtc/system_wrappers/include/trace.h"
 
 namespace webrtc {
 namespace {
 
 class SourceFrame {
  public:
+  // !!!: Style guide is against abbreviations. Plus these don't make sense. 'p'
+  // for MixerAudioSource?
   SourceFrame(MixerAudioSource* p, AudioFrame* a, bool m, bool was_mixed_before)
       : audio_source_(p),
         audio_frame_(a),
         muted_(m),
         was_mixed_before_(was_mixed_before) {
+    // !!!: Here would be a good place to check pointers aren't null.
     if (!muted_) {
       energy_ = NewMixerCalculateEnergy(*a);
     }
   }
 
   SourceFrame(MixerAudioSource* p,
               AudioFrame* a,
               bool m,
               bool was_mixed_before,
               uint32_t energy)
       : audio_source_(p),
         audio_frame_(a),
         muted_(m),
         energy_(energy),
         was_mixed_before_(was_mixed_before) {}
 
+  // !!!: Capital S here.
   // a.shouldMixBefore(b) is used to select mixer participants.
   bool shouldMixBefore(const SourceFrame& other) const {
     if (muted_ != other.muted_) {
       return other.muted_;
     }
 
     const auto our_activity = audio_frame_->vad_activity_;
     const auto other_activity = other.audio_frame_->vad_activity_;
 
     if (our_activity != other_activity) {
       return our_activity == AudioFrame::kVadActive;
     }
 
     return energy_ > other.energy_;
   }
 
+  // !!!: You should have accessors for these, so that you can e.g. check that
   MixerAudioSource* audio_source_;
   AudioFrame* audio_frame_;
   bool muted_;
   uint32_t energy_;
   bool was_mixed_before_;
 };
 
+// !!!: The term is usually "Downmix".
 // Remixes a frame between stereo and mono.
 void RemixFrame(AudioFrame* frame, size_t number_of_channels) {
   RTC_DCHECK(number_of_channels == 1 || number_of_channels == 2);
   if (frame->num_channels_ == 1 && number_of_channels == 2) {
     AudioFrameOperations::MonoToStereo(frame);
   } else if (frame->num_channels_ == 2 && number_of_channels == 1) {
     AudioFrameOperations::StereoToMono(frame);
   }
 }
 
@@ skipping 10 matching lines @@
       NewMixerRampOut(source_frame.audio_frame_);
     }
   }
 }
 
 // Mix the AudioFrames stored in audioFrameList into mixed_audio.
 int32_t MixFromList(AudioFrame* mixed_audio,
                     const AudioFrameList& audio_frame_list,
                     int32_t id,
                     bool use_limiter) {
+  // !!!: Don't use WEBRTC_TRACE in new code
   WEBRTC_TRACE(kTraceStream, kTraceAudioMixerServer, id,
                "MixFromList(mixed_audio, audio_frame_list)");
+  // !!!: Consistently use {} for one-liners, or don't.
   if (audio_frame_list.empty())
     return 0;
 
   if (audio_frame_list.size() == 1) {
     mixed_audio->timestamp_ = audio_frame_list.front()->timestamp_;
     mixed_audio->elapsed_time_ms_ = audio_frame_list.front()->elapsed_time_ms_;
   } else {
     // TODO(wu): Issue 3390.
     // Audio frame timestamp is only supported in one channel case.
     mixed_audio->timestamp_ = 0;
@@ skipping 79 matching lines @@
 
   if (sample_rate != kNbInHz && sample_rate != kWbInHz &&
       sample_rate != kSwbInHz && sample_rate != kFbInHz) {
     WEBRTC_TRACE(kTraceError, kTraceAudioMixerServer, id_,
                  "Invalid frequency: %d", sample_rate);
     RTC_NOTREACHED();
     return;
   }
 
   if (OutputFrequency() != sample_rate) {
+    // !!!: Dangerous cast!
     SetOutputFrequency(static_cast<Frequency>(sample_rate));
   }
 
   AudioFrameList mix_list;
   AudioFrameList anonymous_mix_list;
   size_t num_mixed_audio_sources;
   {
     rtc::CritScope lock(&crit_);
     mix_list = GetNonAnonymousAudio();
     anonymous_mix_list = GetAnonymousAudio();
@@ skipping 302 matching lines @@
 }
 
 int AudioMixerImpl::GetOutputAudioLevelFullRange() {
   RTC_DCHECK_RUN_ON(&thread_checker_);
   const int level = audio_level_.LevelFullRange();
   WEBRTC_TRACE(kTraceStateInfo, kTraceAudioMixerServer, id_,
                "GetAudioOutputLevelFullRange() => level=%d", level);
   return level;
 }
 }  // namespace webrtc