Propagate muted info from VoE Channel to AudioConferenceMixer

webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.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_CONFERENCE_MIXER_INCLUDE_AUDIO_CONFERENCE_MIXER_DEFINES_H_
 #define WEBRTC_MODULES_AUDIO_CONFERENCE_MIXER_INCLUDE_AUDIO_CONFERENCE_MIXER_DEFINES_H_
 
+#include "webrtc/base/checks.h"
 #include "webrtc/modules/include/module_common_types.h"
 #include "webrtc/typedefs.h"
 
 namespace webrtc {
 class MixHistory;
 
 // A callback class that all mixer participants must inherit from/implement.
 class MixerParticipant
 {
 public:
+    enum class AudioFrameInfo {
+      kUnmuted,

[minyue-webrtc, 2016/05/17 14:59:16]

Maybe kNormal is better than kUnmuted

[hlundin-webrtc, 2016/05/17 19:57:51]

Done.

+      kMuted,
+      kError
+    };
+
     // The implementation of this function should update audioFrame with new
     // audio every time it's called.
     //
     // If it returns -1, the frame will not be added to the mix.
     virtual int32_t GetAudioFrame(int32_t id,
-                                  AudioFrame* audioFrame) = 0;
+                                  AudioFrame* audioFrame) {
+      RTC_CHECK(false);

[kwiberg-webrtc, 2016/05/17 12:57:06]

Add a comment such as

  // This function should not be called. It will remain for a short
  // time so that subclasses can override it without getting warnings.

[minyue-webrtc, 2016/05/17 14:59:16]

1. maybe DCHECK?

2. should still have a "return" line or no need?

[hlundin-webrtc, 2016/05/17 19:57:51]

Done.

[hlundin-webrtc, 2016/05/17 19:57:51]

I followed kwiberg's recommendation to use RTC_CHECK. :)
If DCHECK, yes. But with CHECK, we'll see what the try bots say.

[kwiberg-webrtc, 2016/05/18 02:45:06]

This line should never be reached, so there's no reason to optimize by using
DCHECK instead of CHECK.

The Windows bots used to require a return, even though all the other compilers
did not because they recognized that CHECK(false) doesn't return. I haven't
tried it since we updated from VS 2013 to 2015, though. If it does work without
a return statement, go for it!

+    }
+
+    // The implementation of this function should update audio_frame with new
+    // audio every time it's called. The return value should will be interpreted
+    // as follows.

[kwiberg-webrtc, 2016/05/17 12:57:06]

Grammar.

[hlundin-webrtc, 2016/05/17 19:57:51]

Done.

+    // - kUnmuted: the samples in audio_frame are valid and will be used.
+    // - kMuted:   the samples in audio_frame will not be read, but will be

[minyue-webrtc, 2016/05/17 14:59:16]

"will not be read" or "will not be overwritten"?

[hlundin-webrtc, 2016/05/17 19:57:51]

"will not be read". This is the definition of a callback interface, and the
comment promises what the AudioConferenceMixer will do with the callback and the
results of it.

[kwiberg-webrtc, 2016/05/18 02:45:06]

It may be better to say "valid and should be used", and "should not be used".
Because (1) this is instructions for how to use the interface from both ends,
not assertions about how a specific implementation works, and (2) "should not be
read" implies that the caller may not even peek at the values before ignoring
them (or is this what you intended?).

[minyue-webrtc, 2016/05/18 02:53:51]

+1

[hlundin-webrtc, 2016/05/18 06:58:53]

Done.

+    //             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/05/17 12:57:06]

To avoid having to keep comments and enum in sync, move enum here and annotate
each enum case?

[hlundin-webrtc, 2016/05/17 19:57:51]

Style guide says no. But I did it anyway, since we will be back in compliance
when GetAudioFrame is removed.

+    virtual AudioFrameInfo GetAudioFrameWithMuted(int32_t id,
+                                                  AudioFrame* audio_frame) {
+      return GetAudioFrame(id, audio_frame) != 0 ?

[minyue-webrtc, 2016/05/17 14:59:16]

"!=0" => "== -1" from what is written at line 34

[hlundin-webrtc, 2016/05/17 19:57:51]

Done.

+          AudioFrameInfo::kError :
+          AudioFrameInfo::kUnmuted;
+    }
 
     // Returns true if the participant was mixed this mix iteration.
     bool IsMixed() const;
 
     // This function specifies the sampling frequency needed for the AudioFrame
     // for future GetAudioFrame(..) calls.
     virtual int32_t NeededFrequency(int32_t id) const = 0;
 
     MixHistory* _mixHistory;
 protected:
@@ skipping 11 matching lines @@
                                const AudioFrame& generalAudioFrame,
                                const AudioFrame** uniqueAudioFrames,
                                const uint32_t size) = 0;
 protected:
     AudioMixerOutputReceiver() {}
     virtual ~AudioMixerOutputReceiver() {}
 };
 }  // namespace webrtc
 
 #endif // WEBRTC_MODULES_AUDIO_CONFERENCE_MIXER_INCLUDE_AUDIO_CONFERENCE_MIXER_DEFINES_H_