Propagate muted info from VoE Channel to AudioConferenceMixer

webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h

Index: webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h
diff --git a/webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h b/webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h
index 5d58f42435ef6ee6cfc302b3dcd57b74e744a075..80d03361c31ac56b9c8bbecee2db6e430f46773f 100644
--- a/webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h
+++ b/webrtc/modules/audio_conference_mixer/include/audio_conference_mixer_defines.h
@@ -11,6 +11,7 @@
 #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"
 
@@ -21,12 +22,35 @@ class MixHistory;
 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;