ABANDONED: Remove the default receive channel in WVoE.

talk/media/webrtc/webrtcvoiceengine.cc

Index: talk/media/webrtc/webrtcvoiceengine.cc
diff --git a/talk/media/webrtc/webrtcvoiceengine.cc b/talk/media/webrtc/webrtcvoiceengine.cc
index 40d8442405ad83df68862de2fb03dec13a0d729e..5504d0b98a8bd13e20f60df3e4df7e9fd0a24913 100644
--- a/talk/media/webrtc/webrtcvoiceengine.cc
+++ b/talk/media/webrtc/webrtcvoiceengine.cc
@@ -147,22 +147,36 @@ static const char kAecDumpByAudioOptionFilename[] = "/sdcard/audio.aecdump";
 static const char kAecDumpByAudioOptionFilename[] = "audio.aecdump";
 #endif
 
+namespace {
+
+bool ValidateStreamParams(const StreamParams& sp) {
+  if (sp.ssrcs.empty()) {
+    LOG(LS_ERROR) << "No SSRCs in stream parameters: " << sp.ToString();
+    return false;
+  }
+  if (sp.ssrcs.size() > 1) {
+    LOG(LS_ERROR) << "Multiple SSRCs in stream parameters: " << sp.ToString();
+    return false;
+  }
+  return true;
+}
+
 // Dumps an AudioCodec in RFC 2327-ish format.
-static std::string ToString(const AudioCodec& codec) {
+std::string ToString(const AudioCodec& codec) {
   std::stringstream ss;
   ss << codec.name << "/" << codec.clockrate << "/" << codec.channels
      << " (" << codec.id << ")";
   return ss.str();
 }
 
-static std::string ToString(const webrtc::CodecInst& codec) {
+std::string ToString(const webrtc::CodecInst& codec) {
   std::stringstream ss;
   ss << codec.plname << "/" << codec.plfreq << "/" << codec.channels
      << " (" << codec.pltype << ")";
   return ss.str();
 }
 
-static void LogMultiline(rtc::LoggingSeverity sev, char* text) {
+void LogMultiline(rtc::LoggingSeverity sev, char* text) {
   const char* delim = "\r\n";
   for (char* tok = strtok(text, delim); tok; tok = strtok(NULL, delim)) {
     LOG_V(sev) << tok;
@@ -170,7 +184,7 @@ static void LogMultiline(rtc::LoggingSeverity sev, char* text) {
 }
 
 // Severity is an integer because it comes is assumed to be from command line.
-static int SeverityToFilter(int severity) {
+int SeverityToFilter(int severity) {
   int filter = webrtc::kTraceNone;
   switch (severity) {
     case rtc::LS_VERBOSE:
@@ -188,15 +202,15 @@ static int SeverityToFilter(int severity) {
   return filter;
 }
 
-static bool IsCodec(const AudioCodec& codec, const char* ref_name) {
+bool IsCodec(const AudioCodec& codec, const char* ref_name) {
   return (_stricmp(codec.name.c_str(), ref_name) == 0);
 }
 
-static bool IsCodec(const webrtc::CodecInst& codec, const char* ref_name) {
+bool IsCodec(const webrtc::CodecInst& codec, const char* ref_name) {
   return (_stricmp(codec.plname, ref_name) == 0);
 }
 
-static bool IsCodecMultiRate(const webrtc::CodecInst& codec) {
+bool IsCodecMultiRate(const webrtc::CodecInst& codec) {
   for (size_t i = 0; i < ARRAY_SIZE(kCodecPrefs); ++i) {
     if (IsCodec(codec, kCodecPrefs[i].name) &&
         kCodecPrefs[i].clockrate == codec.plfreq) {
@@ -206,7 +220,7 @@ static bool IsCodecMultiRate(const webrtc::CodecInst& codec) {
   return false;
 }
 
-static bool FindCodec(const std::vector<AudioCodec>& codecs,
+bool FindCodec(const std::vector<AudioCodec>& codecs,
                       const AudioCodec& codec,
                       AudioCodec* found_codec) {
   for (const AudioCodec& c : codecs) {
@@ -220,12 +234,12 @@ static bool FindCodec(const std::vector<AudioCodec>& codecs,
   return false;
 }
 
-static bool IsNackEnabled(const AudioCodec& codec) {
+bool IsNackEnabled(const AudioCodec& codec) {
   return codec.HasFeedbackParam(FeedbackParam(kRtcpFbParamNack,
                                               kParamValueEmpty));
 }
 
-static int SelectPacketSize(const CodecPref& codec_pref, int ptime_ms) {
+int SelectPacketSize(const CodecPref& codec_pref, int ptime_ms) {
   int selected_packet_size_ms = codec_pref.packet_sizes_ms[0];
   for (int packet_size_ms : codec_pref.packet_sizes_ms) {
     if (packet_size_ms && packet_size_ms <= ptime_ms) {
@@ -238,7 +252,7 @@ static int SelectPacketSize(const CodecPref& codec_pref, int ptime_ms) {
 // If the AudioCodec param kCodecParamPTime is set, then we will set it to codec
 // pacsize if it's valid, or we will pick the next smallest value we support.
 // TODO(Brave): Query supported packet sizes from ACM when the API is ready.
-static bool SetPTimeAsPacketSize(webrtc::CodecInst* codec, int ptime_ms) {
+bool SetPTimeAsPacketSize(webrtc::CodecInst* codec, int ptime_ms) {
   for (const CodecPref& codec_pref : kCodecPrefs) {
     if ((IsCodec(*codec, codec_pref.name) &&
         codec_pref.clockrate == codec->plfreq) ||
@@ -255,7 +269,7 @@ static bool SetPTimeAsPacketSize(webrtc::CodecInst* codec, int ptime_ms) {
 }
 
 // Return true if codec.params[feature] == "1", false otherwise.
-static bool IsCodecFeatureEnabled(const AudioCodec& codec,
+bool IsCodecFeatureEnabled(const AudioCodec& codec,
                                   const char* feature) {
   int value;
   return codec.GetParam(feature, &value) && value == 1;
@@ -265,7 +279,7 @@ static bool IsCodecFeatureEnabled(const AudioCodec& codec,
 // otherwise. If the value (either from params or codec.bitrate) <=0, use the
 // default configuration. If the value is beyond feasible bit rate of Opus,
 // clamp it. Returns the Opus bit rate for operation.
-static int GetOpusBitrate(const AudioCodec& codec, int max_playback_rate) {
+int GetOpusBitrate(const AudioCodec& codec, int max_playback_rate) {
   int bitrate = 0;
   bool use_param = true;
   if (!codec.GetParam(kCodecParamMaxAverageBitrate, &bitrate)) {
@@ -298,7 +312,7 @@ static int GetOpusBitrate(const AudioCodec& codec, int max_playback_rate) {
 
 // Returns kOpusDefaultPlaybackRate if params[kCodecParamMaxPlaybackRate] is not
 // defined. Returns the value of params[kCodecParamMaxPlaybackRate] otherwise.
-static int GetOpusMaxPlaybackRate(const AudioCodec& codec) {
+int GetOpusMaxPlaybackRate(const AudioCodec& codec) {
   int value;
   if (codec.GetParam(kCodecParamMaxPlaybackRate, &value)) {
     return value;
@@ -306,7 +320,7 @@ static int GetOpusMaxPlaybackRate(const AudioCodec& codec) {
   return kOpusDefaultMaxPlaybackRate;
 }
 
-static void GetOpusConfig(const AudioCodec& codec, webrtc::CodecInst* voe_codec,
+void GetOpusConfig(const AudioCodec& codec, webrtc::CodecInst* voe_codec,
                           bool* enable_codec_fec, int* max_playback_rate,
                           bool* enable_codec_dtx) {
   *enable_codec_fec = IsCodecFeatureEnabled(codec, kCodecParamUseInbandFec);
@@ -326,7 +340,7 @@ static void GetOpusConfig(const AudioCodec& codec, webrtc::CodecInst* voe_codec,
 // Changes RTP timestamp rate of G722. This is due to the "bug" in the RFC
 // which says that G722 should be advertised as 8 kHz although it is a 16 kHz
 // codec.
-static void MaybeFixupG722(webrtc::CodecInst* voe_codec, int new_plfreq) {
+void MaybeFixupG722(webrtc::CodecInst* voe_codec, int new_plfreq) {
   if (IsCodec(*voe_codec, kG722CodecName)) {
     // If the ASSERT triggers, the codec definition in WebRTC VoiceEngine
     // has changed, and this special case is no longer needed.
@@ -338,7 +352,7 @@ static void MaybeFixupG722(webrtc::CodecInst* voe_codec, int new_plfreq) {
 // Gets the default set of options applied to the engine. Historically, these
 // were supplied as a combination of flags from the channel manager (ec, agc,
 // ns, and highpass) and the rest hardcoded in InitInternal.
-static AudioOptions GetDefaultEngineOptions() {
+AudioOptions GetDefaultEngineOptions() {
   AudioOptions options;
   options.echo_cancellation.Set(true);
   options.auto_gain_control.Set(true);
@@ -348,7 +362,6 @@ static AudioOptions GetDefaultEngineOptions() {
   options.audio_jitter_buffer_max_packets.Set(50);
   options.audio_jitter_buffer_fast_accelerate.Set(false);
   options.typing_detection.Set(true);
-  options.conference_mode.Set(false);
   options.adjust_agc_delta.Set(0);
   options.experimental_agc.Set(false);
   options.extended_filter_aec.Set(false);
@@ -358,9 +371,10 @@ static AudioOptions GetDefaultEngineOptions() {
   return options;
 }
 
-static std::string GetEnableString(bool enable) {
+std::string GetEnableString(bool enable) {
   return enable ? "enable" : "disable";
 }
+} // namespace {
 
 WebRtcVoiceEngine::WebRtcVoiceEngine()
     : voe_wrapper_(new VoEWrapper()),
@@ -862,18 +876,6 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) {
   return true;
 }
 
-struct ResumeEntry {
-  ResumeEntry(WebRtcVoiceMediaChannel *c, bool p, SendFlags s)
-      : channel(c),
-        playout(p),
-        send(s) {
-  }
-
-  WebRtcVoiceMediaChannel *channel;
-  bool playout;
-  SendFlags send;
-};
-
 // TODO(juberti): Refactor this so that the core logic can be used to set the
 // soundclip device. At that time, reinstate the soundclip pause/resume code.
 bool WebRtcVoiceEngine::SetDevices(const Device* in_device,
@@ -1415,7 +1417,8 @@ WebRtcVoiceMediaChannel::WebRtcVoiceMediaChannel(WebRtcVoiceEngine* engine,
       desired_send_(SEND_NOTHING),
       send_(SEND_NOTHING),
       call_(call),
-      default_receive_ssrc_(0) {
+      default_recv_ssrc_(0),
+      default_recv_channel_id_(-1) {
   engine->RegisterChannel(this);
   LOG(LS_VERBOSE) << "WebRtcVoiceMediaChannel::WebRtcVoiceMediaChannel "
                   << voe_channel();
@@ -1430,11 +1433,13 @@ WebRtcVoiceMediaChannel::~WebRtcVoiceMediaChannel() {
 
   // Remove any remaining send streams, the default channel will be deleted
   // later.
-  while (!send_channels_.empty())
+  while (!send_channels_.empty()) {
     RemoveSendStream(send_channels_.begin()->first);
+  }
 
   // Unregister ourselves from the engine.
   engine()->UnregisterChannel(this);
+
   // Remove any remaining streams.
   while (!receive_channels_.empty()) {
     RemoveRecvStream(receive_channels_.begin()->first);
@@ -1464,6 +1469,7 @@ bool WebRtcVoiceMediaChannel::SetRecvParameters(
 }
 
 bool WebRtcVoiceMediaChannel::SetOptions(const AudioOptions& options) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());

[pthatcher1, 2015/10/02 02:33:31]

This is called in the following methods:

WebRtcVoiceMediaChannel::WebRtcVoiceMediaChannel
WebRtcVoiceMediaChannel::SetSendParameters
WebRtcVoiceMediaChannel::SetAudioSend

Can you put thread checks in those as well?

[the sun, 2015/10/02 11:34:19]

Done.

   LOG(LS_INFO) << "Setting voice channel options: "
                << options.ToString();
 
@@ -1486,14 +1492,39 @@ bool WebRtcVoiceMediaChannel::SetOptions(const AudioOptions& options) {
     }
   }
 
+  // TODO(solenberg): !!!!!!
+  SetRecvOptions(voe_channel());

[pthatcher1, 2015/10/02 02:33:31]

Why do we have to call SetRecvOptions with the default channel which is not only
used for sending?

[the sun, 2015/10/02 11:34:20]

Sorry; debugging left overs.

+  for (const auto& ch : receive_channels_) {
+    if (!SetRecvOptions(ch.second->channel())) {
+      return false;
+    }
+  }
+  if (dscp_option_changed) {
+    rtc::DiffServCodePoint dscp = rtc::DSCP_DEFAULT;
+    if (options_.dscp.GetWithDefaultIfUnset(false))
+      dscp = kAudioDscpValue;
+    if (MediaChannel::SetDscp(dscp) != 0) {
+      LOG(LS_WARNING) << "Failed to set DSCP settings for audio channel";
+    }
+  }
+
+  RecreateAudioReceiveStreams();
+
+  LOG(LS_INFO) << "Set voice channel options.  Current options: "
+               << options_.ToString();
+  return true;
+}
+
+bool WebRtcVoiceMediaChannel::SetRecvOptions(int channel_id) {

[pthatcher1, 2015/10/02 02:33:31]

Can you please pass option into here as well? 

[the sun, 2015/10/02 11:34:20]

Done.

+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+
   // Receiver-side auto gain control happens per channel, so set it here from
-  // options. Note that, like conference mode, setting it on the engine won't
-  // have the desired effect, since voice channels don't inherit options from
-  // the media engine when those options are applied per-channel.
+  // options. Note that voice channels don't inherit options from the media
+  // engine when those options are applied per-channel.
   bool rx_auto_gain_control;
-  if (options.rx_auto_gain_control.Get(&rx_auto_gain_control)) {
+  if (options_.rx_auto_gain_control.Get(&rx_auto_gain_control)) {
     if (engine()->voe()->processing()->SetRxAgcStatus(
-            voe_channel(), rx_auto_gain_control,
+            channel_id, rx_auto_gain_control,
             webrtc::kAgcFixedDigital) == -1) {
       LOG_RTCERR1(SetRxAgcStatus, rx_auto_gain_control);
       return false;
@@ -1502,52 +1533,39 @@ bool WebRtcVoiceMediaChannel::SetOptions(const AudioOptions& options) {
                       << " with mode " << webrtc::kAgcFixedDigital;
     }
   }
-  if (options.rx_agc_target_dbov.IsSet() ||
-      options.rx_agc_digital_compression_gain.IsSet() ||
-      options.rx_agc_limiter.IsSet()) {
+  if (options_.rx_agc_target_dbov.IsSet() ||
+      options_.rx_agc_digital_compression_gain.IsSet() ||
+      options_.rx_agc_limiter.IsSet()) {
     webrtc::AgcConfig config;
     // If only some of the options are being overridden, get the current
     // settings for the channel and bail if they aren't available.
-    if (!options.rx_agc_target_dbov.IsSet() ||
-        !options.rx_agc_digital_compression_gain.IsSet() ||
-        !options.rx_agc_limiter.IsSet()) {
+    if (!options_.rx_agc_target_dbov.IsSet() ||
+        !options_.rx_agc_digital_compression_gain.IsSet() ||
+        !options_.rx_agc_limiter.IsSet()) {
       if (engine()->voe()->processing()->GetRxAgcConfig(
-              voe_channel(), config) != 0) {
+              channel_id, config) != 0) {
         LOG(LS_ERROR) << "Failed to get default rx agc configuration for "
-                      << "channel " << voe_channel() << ". Since not all rx "
+                      << "channel " << channel_id << ". Since not all rx "
                       << "agc options are specified, unable to safely set rx "
                       << "agc options.";
         return false;
       }
     }
     config.targetLeveldBOv =
-        options.rx_agc_target_dbov.GetWithDefaultIfUnset(
+        options_.rx_agc_target_dbov.GetWithDefaultIfUnset(
             config.targetLeveldBOv);
     config.digitalCompressionGaindB =
-        options.rx_agc_digital_compression_gain.GetWithDefaultIfUnset(
+        options_.rx_agc_digital_compression_gain.GetWithDefaultIfUnset(
             config.digitalCompressionGaindB);
-    config.limiterEnable = options.rx_agc_limiter.GetWithDefaultIfUnset(
+    config.limiterEnable = options_.rx_agc_limiter.GetWithDefaultIfUnset(
         config.limiterEnable);
     if (engine()->voe()->processing()->SetRxAgcConfig(
-            voe_channel(), config) == -1) {
-      LOG_RTCERR4(SetRxAgcConfig, voe_channel(), config.targetLeveldBOv,
+            channel_id, config) == -1) {
+      LOG_RTCERR4(SetRxAgcConfig, channel_id, config.targetLeveldBOv,
                   config.digitalCompressionGaindB, config.limiterEnable);
       return false;
     }
   }
-  if (dscp_option_changed) {
-    rtc::DiffServCodePoint dscp = rtc::DSCP_DEFAULT;
-    if (options_.dscp.GetWithDefaultIfUnset(false))
-      dscp = kAudioDscpValue;
-    if (MediaChannel::SetDscp(dscp) != 0) {
-      LOG(LS_WARNING) << "Failed to set DSCP settings for audio channel";
-    }
-  }
-
-  RecreateAudioReceiveStreams();
-
-  LOG(LS_INFO) << "Set voice channel options.  Current options: "
-               << options_.ToString();
   return true;
 }
 
@@ -1556,11 +1574,12 @@ bool WebRtcVoiceMediaChannel::SetRecvCodecs(
   // Set the payload types to be used for incoming media.
   LOG(LS_INFO) << "Setting receive voice codecs:";
 
+  std::vector<int> payload_types;
   std::vector<AudioCodec> new_codecs;
   // Find all new codecs. We allow adding new codecs but don't allow changing
   // the payload type of codecs that is already configured since we might
   // already be receiving packets with that payload type.
-  for (const AudioCodec& codec : codecs) {
+  for (const auto& codec : codecs) {
     AudioCodec old_codec;
     if (FindCodec(recv_codecs_, codec, &old_codec)) {
       if (old_codec.id != codec.id) {
@@ -1568,6 +1587,7 @@ bool WebRtcVoiceMediaChannel::SetRecvCodecs(
         return false;
       }
     } else {
+      payload_types.push_back(codec.id);
       new_codecs.push_back(codec);
     }
   }
@@ -1577,6 +1597,13 @@ bool WebRtcVoiceMediaChannel::SetRecvCodecs(
     return true;
   }
 
+  // Verify no codecs have the same payload type.

[pthatcher1, 2015/10/02 02:33:31]

Can you put this in its own helper function?

[the sun, 2015/10/02 11:34:20]

Done.

+  std::sort(payload_types.begin(), payload_types.end());
+  auto it = std::unique(payload_types.begin(), payload_types.end());
+  if (payload_types.end() != it) {
+    return false;
+  }
+
   if (playout_) {
     // Receive codecs can not be changed while playing. So we temporarily
     // pause playout.
@@ -1804,6 +1831,7 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs(
 
 bool WebRtcVoiceMediaChannel::SetSendCodecs(
     const std::vector<AudioCodec>& codecs) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());

[pthatcher1, 2015/10/02 02:33:31]

This is called by 

WebRtcVoiceMediaChannel::SetSendParameters
WebRtcVoiceMediaChannel::AddSendStream

Can you also add checks for those?

[the sun, 2015/10/02 11:34:20]

Done.

   dtmf_allowed_ = false;
   for (const AudioCodec& codec : codecs) {
     // Find the DTMF telephone event "codec".
@@ -1875,17 +1903,11 @@ bool WebRtcVoiceMediaChannel::SetSendCodec(
 
 bool WebRtcVoiceMediaChannel::SetRecvRtpHeaderExtensions(
     const std::vector<RtpHeaderExtension>& extensions) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());

[pthatcher1, 2015/10/02 02:33:31]

This is called by WebRtcVoiceMediaChannel::SetRecvParameters

Can you also add a thread check there?

[the sun, 2015/10/02 11:34:19]

Done.

   if (receive_extensions_ == extensions) {
     return true;
   }
 
-  // The default channel may or may not be in |receive_channels_|. Set the rtp
-  // header extensions for default channel regardless.
-  if (!SetChannelRecvRtpHeaderExtensions(voe_channel(), extensions)) {
-    return false;
-  }
-
-  // Loop through all receive channels and enable/disable the extensions.
   for (const auto& ch : receive_channels_) {
     if (!SetChannelRecvRtpHeaderExtensions(ch.second->channel(), extensions)) {
       return false;
@@ -2000,16 +2022,13 @@ bool WebRtcVoiceMediaChannel::ResumePlayout() {
 }
 
 bool WebRtcVoiceMediaChannel::ChangePlayout(bool playout) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   if (playout_ == playout) {
     return true;
   }
 
   // Change the playout of all channels to the new state.
   bool result = true;
-  if (receive_channels_.empty()) {
-    // Only toggle the default channel if we don't have any other channels.
-    result = SetPlayout(voe_channel(), playout);
-  }
   for (const auto& ch : receive_channels_) {
     if (!SetPlayout(ch.second->channel(), playout)) {
       LOG(LS_ERROR) << "SetPlayout " << playout << " on channel "
@@ -2112,9 +2131,6 @@ void WebRtcVoiceMediaChannel::ConfigureSendChannel(int channel) {
   // Enable RTCP (for quality stats and feedback messages)
   EnableRtcp(channel);
 
-  // Reset all recv codecs; they will be enabled via SetRecvCodecs.
-  ResetRecvCodecs(channel);
-
   // Set RTP header extension for the new channel.
   SetChannelSendRtpHeaderExtensions(channel, send_extensions_);
 }
@@ -2133,6 +2149,7 @@ bool WebRtcVoiceMediaChannel::DeleteChannel(int channel) {
 }
 
 bool WebRtcVoiceMediaChannel::AddSendStream(const StreamParams& sp) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   // If the default channel is already used for sending create a new channel
   // otherwise use the default channel for sending.
   int channel = GetSendChannelNum(sp.first_ssrc());
@@ -2184,13 +2201,10 @@ bool WebRtcVoiceMediaChannel::AddSendStream(const StreamParams& sp) {
   // order to send receiver reports with this SSRC.
   if (IsDefaultChannel(channel)) {
     for (const auto& ch : receive_channels_) {
-      // Only update the SSRC for non-default channels.
-      if (!IsDefaultChannel(ch.second->channel())) {
-        if (engine()->voe()->rtp()->SetLocalSSRC(ch.second->channel(),
-                                                 sp.first_ssrc()) != 0) {
-          LOG_RTCERR2(SetLocalSSRC, ch.second->channel(), sp.first_ssrc());
-          return false;
-        }
+      if (engine()->voe()->rtp()->SetLocalSSRC(ch.second->channel(),
+                                               sp.first_ssrc()) != 0) {
+        LOG_RTCERR2(SetLocalSSRC, ch.second->channel(), sp.first_ssrc());
+        return false;
       }
     }
   }
@@ -2243,51 +2257,43 @@ bool WebRtcVoiceMediaChannel::RemoveSendStream(uint32 ssrc) {
 
 bool WebRtcVoiceMediaChannel::AddRecvStream(const StreamParams& sp) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
-  rtc::CritScope lock(&receive_channels_cs_);
+  LOG(LS_INFO) << "AddRecvStream: " << sp.ToString();
 
-  if (!VERIFY(sp.ssrcs.size() == 1))
+  if (!ValidateStreamParams(sp)) {
     return false;
-  uint32 ssrc = sp.first_ssrc();
+  }
 
+  uint32 ssrc = sp.first_ssrc();
   if (ssrc == 0) {
-    LOG(LS_WARNING) << "AddRecvStream with 0 ssrc is not supported.";
+    LOG(LS_WARNING) << "AddRecvStream with ssrc==0 is not supported.";
     return false;
   }
 
+  // Remove the default receive stream if one had been created with this ssrc;
+  // we'll recreate it then.
+  if (-1 != default_recv_channel_id_ && ssrc == default_recv_ssrc_) {
+    RemoveRecvStream(ssrc);

[pthatcher1, 2015/10/02 02:33:30]

Why remove it and re-add it?  Why not just keep using it as-is?

[the sun, 2015/10/02 11:34:19]

Because we need to recreate the AudioReceiveStream anyway, when reconfiguring.
Going forward, there won't anything left to reconfigure here.

+  }
+
   if (receive_channels_.find(ssrc) != receive_channels_.end()) {
     LOG(LS_ERROR) << "Stream already exists with ssrc " << ssrc;
     return false;
   }
-
   RTC_DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end());
 
-  // Reuse default channel for recv stream in non-conference mode call
-  // when the default channel is not being used.
-  webrtc::AudioTransport* audio_transport =
-      engine()->voe()->base()->audio_transport();
-  if (!InConferenceMode() && default_receive_ssrc_ == 0) {
-    LOG(LS_INFO) << "Recv stream " << ssrc << " reuse default channel";
-    default_receive_ssrc_ = ssrc;
-    WebRtcVoiceChannelRenderer* channel_renderer =
-        new WebRtcVoiceChannelRenderer(voe_channel(), audio_transport);
-    receive_channels_.insert(std::make_pair(ssrc, channel_renderer));
-    receive_stream_params_[ssrc] = sp;
-    AddAudioReceiveStream(ssrc);
-    return SetPlayout(voe_channel(), playout_);
-  }
-
   // Create a new channel for receiving audio data.
   int channel = engine()->CreateMediaVoiceChannel();
   if (channel == -1) {
     LOG_RTCERR0(CreateChannel);
     return false;
   }
-
   if (!ConfigureRecvChannel(channel)) {
     DeleteChannel(channel);
     return false;
   }
 
+  webrtc::AudioTransport* audio_transport =
+      engine()->voe()->base()->audio_transport();
   WebRtcVoiceChannelRenderer* channel_renderer =
       new WebRtcVoiceChannelRenderer(channel, audio_transport);
   receive_channels_.insert(std::make_pair(ssrc, channel_renderer));
@@ -2301,14 +2307,19 @@ bool WebRtcVoiceMediaChannel::AddRecvStream(const StreamParams& sp) {
 }
 
 bool WebRtcVoiceMediaChannel::ConfigureRecvChannel(int channel) {
-  // Configure to use external transport, like our default channel.
+  // Configure to use external transport.
   if (engine()->voe()->network()->RegisterExternalTransport(
           channel, *this) == -1) {
     LOG_RTCERR2(SetExternalTransport, channel, this);
     return false;
   }
 
-  // Use the same SSRC as our default channel (so the RTCP reports are correct).
+  if (!SetRecvOptions(channel)) {
+    return false;
+  }
+
+  // Use the same SSRC as our default (send) channel, so the RTCP reports are
+  // correct.
   unsigned int send_ssrc = 0;
   webrtc::VoERTP_RTCP* rtp = engine()->voe()->rtp();
   if (rtp->GetLocalSSRC(voe_channel(), send_ssrc) == -1) {
@@ -2320,47 +2331,40 @@ bool WebRtcVoiceMediaChannel::ConfigureRecvChannel(int channel) {
     return false;
   }
 
-  // Associate receive channel to default channel (so the receive channel can
-  // obtain RTT from the send channel)
+  // Associate receive channel to default send channel (so the receive channel
+  // can obtain RTT from the send channel)
   engine()->voe()->base()->AssociateSendChannel(channel, voe_channel());
   LOG(LS_INFO) << "VoiceEngine channel #"
                << channel << " is associated with channel #"
                << voe_channel() << ".";
 
-  // Use the same recv payload types as our default channel.
-  ResetRecvCodecs(channel);
-  if (!recv_codecs_.empty()) {
-    for (const auto& codec : recv_codecs_) {
-      webrtc::CodecInst voe_codec;
-      if (engine()->FindWebRtcCodec(codec, &voe_codec)) {
-        voe_codec.pltype = codec.id;
-        voe_codec.rate = 0;  // Needed to make GetRecPayloadType work for ISAC
-        if (engine()->voe()->codec()->GetRecPayloadType(
-            voe_channel(), voe_codec) != -1) {
-          if (engine()->voe()->codec()->SetRecPayloadType(
-              channel, voe_codec) == -1) {
-            LOG_RTCERR2(SetRecPayloadType, channel, ToString(voe_codec));
-            return false;
-          }
-        }
+  // Turn off all supported codecs.

[pthatcher1, 2015/10/02 02:33:30]

I like this in a helper method.  But instead "ResetRecvCodecs(channel)", it
should perhaps go along with the setting of the payload types in a
SetRecvCodecs(recv_channel_id, recv_codecs).

[the sun, 2015/10/02 11:34:20]

Personally I have a problem with "helper" methods. To me, a function signals
reuse; that the code in it is actually used in multiple places. The caller and
reader expects a well defined interface and associated tests (in this case I'd
need to make the function part of the public WVoE interface in order to do
that). I don't create additional classes just to hide a piece of code from one
call site either.

I don't have a problem with long, messy functions, as long as they are divided
up into logical sections with clearly scope variables. When refactoring it
actually makes it easier, since I don't have to look all over the place for who
may be using a function, just to realize: "oh, just in this one place?".

Yes, this is your cue to disagree... :)

+  int ncodecs = engine()->voe()->codec()->NumOfCodecs();
+  for (int i = 0; i < ncodecs; ++i) {
+    webrtc::CodecInst voe_codec;
+    if (engine()->voe()->codec()->GetCodec(i, voe_codec) != -1) {
+      voe_codec.pltype = -1;
+      if (engine()->voe()->codec()->SetRecPayloadType(
+          channel, voe_codec) == -1) {
+        LOG_RTCERR2(SetRecPayloadType, channel, ToString(voe_codec));
+        return false;
       }
     }
   }
 
-  if (InConferenceMode()) {
-    // To be in par with the video, voe_channel() is not used for receiving in
-    // a conference call.
-    if (receive_channels_.empty() && default_receive_ssrc_ == 0 && playout_) {
-      // This is the first stream in a multi user meeting. We can now
-      // disable playback of the default stream. This since the default
-      // stream will probably have received some initial packets before
-      // the new stream was added. This will mean that the CN state from
-      // the default channel will be mixed in with the other streams
-      // throughout the whole meeting, which might be disturbing.
-      LOG(LS_INFO) << "Disabling playback on the default voice channel";
-      SetPlayout(voe_channel(), false);
+  // Only enable those configured for this channel.
+  for (const auto& codec : recv_codecs_) {
+    webrtc::CodecInst voe_codec;
+    if (engine()->FindWebRtcCodec(codec, &voe_codec)) {
+      voe_codec.pltype = codec.id;

[pthatcher1, 2015/10/02 02:33:31]

Why did you remove .rate = 0?

[the sun, 2015/10/02 11:34:20]

It said in a comment: "// Needed to make GetRecPayloadType work for ISAC"
I'm not using GetRecPayloadType anymore.

+      if (engine()->voe()->codec()->SetRecPayloadType(
+          channel, voe_codec) == -1) {
+        LOG_RTCERR2(SetRecPayloadType, channel, ToString(voe_codec));
+        return false;
+      }
     }
   }
+
   SetNack(channel, nack_enabled_);
 
   // Set RTP header extension for the new channel.
@@ -2373,7 +2377,8 @@ bool WebRtcVoiceMediaChannel::ConfigureRecvChannel(int channel) {
 
 bool WebRtcVoiceMediaChannel::RemoveRecvStream(uint32 ssrc) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
-  rtc::CritScope lock(&receive_channels_cs_);
+  LOG(LS_INFO) << "RemoveRecvStream: " << ssrc;
+
   ChannelMap::iterator it = receive_channels_.find(ssrc);
   if (it == receive_channels_.end()) {
     LOG(LS_WARNING) << "Try to remove stream with ssrc " << ssrc
@@ -2391,46 +2396,21 @@ bool WebRtcVoiceMediaChannel::RemoveRecvStream(uint32 ssrc) {
   delete it->second;
   receive_channels_.erase(it);
 
-  if (ssrc == default_receive_ssrc_) {
-    RTC_DCHECK(IsDefaultChannel(channel));
-    // Recycle the default channel is for recv stream.
-    if (playout_)
-      SetPlayout(voe_channel(), false);
-
-    default_receive_ssrc_ = 0;
-    return true;
+  // Deregister default channel, if that's the one being destroyed.
+  if (-1 != default_recv_channel_id_ && ssrc == default_recv_ssrc_) {
+    RTC_DCHECK(channel == default_recv_channel_id_);
+    default_recv_ssrc_ = 0;
+    default_recv_channel_id_ = -1;
   }
 
   LOG(LS_INFO) << "Removing audio stream " << ssrc
                << " with VoiceEngine channel #" << channel << ".";
-  if (!DeleteChannel(channel))
-    return false;
-
-  bool enable_default_channel_playout = false;
-  if (receive_channels_.empty()) {
-    // The last stream was removed. We can now enable the default
-    // channel for new channels to be played out immediately without
-    // waiting for AddStream messages.
-    // We do this for both conference mode and non-conference mode.
-    // TODO(oja): Does the default channel still have it's CN state?
-    enable_default_channel_playout = true;
-  }
-  if (!InConferenceMode() && receive_channels_.size() == 1 &&
-      default_receive_ssrc_ != 0) {
-    // Only the default channel is active, enable the playout on default
-    // channel.
-    enable_default_channel_playout = true;
-  }
-  if (enable_default_channel_playout && playout_) {
-    LOG(LS_INFO) << "Enabling playback on the default voice channel";
-    SetPlayout(voe_channel(), true);
-  }
-
-  return true;
+  return DeleteChannel(channel);
 }
 
 bool WebRtcVoiceMediaChannel::SetRemoteRenderer(uint32 ssrc,
                                                 AudioRenderer* renderer) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   ChannelMap::iterator it = receive_channels_.find(ssrc);
   if (it == receive_channels_.end()) {
     if (renderer) {
@@ -2475,8 +2455,7 @@ bool WebRtcVoiceMediaChannel::SetLocalRenderer(uint32 ssrc,
 
 bool WebRtcVoiceMediaChannel::GetActiveStreams(
     AudioInfo::StreamList* actives) {
-  // In conference mode, the default channel should not be in
-  // |receive_channels_|.
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   actives->clear();
   for (const auto& ch : receive_channels_) {
     int level = GetOutputLevel(ch.second->channel());
@@ -2488,11 +2467,10 @@ bool WebRtcVoiceMediaChannel::GetActiveStreams(
 }
 
 int WebRtcVoiceMediaChannel::GetOutputLevel() {
-  // return the highest output level of all streams
-  int highest = GetOutputLevel(voe_channel());
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  int highest = 0;
   for (const auto& ch : receive_channels_) {
-    int level = GetOutputLevel(ch.second->channel());
-    highest = std::max(level, highest);
+    highest = std::max(GetOutputLevel(ch.second->channel()), highest);
   }
   return highest;
 }
@@ -2524,24 +2502,17 @@ void WebRtcVoiceMediaChannel::SetTypingDetectionParameters(int time_window,
 
 bool WebRtcVoiceMediaChannel::SetOutputScaling(
     uint32 ssrc, double left, double right) {
-  rtc::CritScope lock(&receive_channels_cs_);
-  // Collect the channels to scale the output volume.
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  // Collect channels to scale output volume for (ssrc == 0 means all channels).

[pthatcher1, 2015/10/02 02:33:31]

This "ssrc == 0 means all" was just a GTP thing.  It's no longer used by anyone
like that, so just remove it.

[the sun, 2015/10/02 11:34:20]

But we still need to support "ssrc == 0 means default channel, if available"?
Just looking up the channel from ssrc causes
PeerConnectionInterfaceTest.ReceiveFireFoxOffer to fail.

   std::vector<int> channels;
-  if (0 == ssrc) {  // Collect all channels, including the default one.
-    // Default channel is not in receive_channels_ if it is not being used for
-    // playout.
-    if (default_receive_ssrc_ == 0)
-      channels.push_back(voe_channel());
-    for (const auto& ch : receive_channels_) {
+  for (const auto& ch : receive_channels_) {
+    if (0 == ssrc || ch.first == ssrc) {
       channels.push_back(ch.second->channel());
     }
-  } else {  // Collect only the channel of the specified ssrc.
-    int channel = GetReceiveChannelNum(ssrc);
-    if (-1 == channel) {
-      LOG(LS_WARNING) << "Cannot find channel for ssrc:" << ssrc;
-      return false;
-    }
-    channels.push_back(channel);
+  }
+  if (0 != ssrc && channels.empty()) {
+    LOG(LS_WARNING) << "Cannot find channel for ssrc:" << ssrc;
+    return false;
   }
 
   // Scale the output volume for the collected channels. We first normalize to
@@ -2628,26 +2599,44 @@ void WebRtcVoiceMediaChannel::OnPacketReceived(
     rtc::Buffer* packet, const rtc::PacketTime& packet_time) {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
 
-  // Forward packet to Call as well.
+  uint32 ssrc = 0;
+  if (!GetRtpSsrc(packet->data(), packet->size(), &ssrc)) {
+    return;
+  }
+
+  if (receive_channels_.empty()) {
+    // Create new channel, which will be the default receive channel.
+    StreamParams sp;
+    sp.ssrcs.push_back(ssrc);
+    LOG(LS_INFO) << "Creating default receive stream for SSRC=" << ssrc << ".";
+    if (!AddRecvStream(sp)) {
+      LOG(LS_WARNING) << "Could not create default receive stream.";
+      return;
+    }
+    default_recv_ssrc_ = ssrc;
+    default_recv_channel_id_ = receive_channels_[ssrc]->channel();

[pthatcher1, 2015/10/02 02:33:30]

Why not just call GetRecvChannelNum?

[the sun, 2015/10/02 11:34:19]

Done.

+    RTC_DCHECK(-1 != default_recv_channel_id_);
+  }
+
+  // Forward packet to Call. If the SSRC is unknown we'll return after this.
   const webrtc::PacketTime webrtc_packet_time(packet_time.timestamp,
                                               packet_time.not_before);
-  call_->Receiver()->DeliverPacket(webrtc::MediaType::AUDIO,
-      reinterpret_cast<const uint8_t*>(packet->data()), packet->size(),
-      webrtc_packet_time);
-
-  // Pick which channel to send this packet to. If this packet doesn't match
-  // any multiplexed streams, just send it to the default channel. Otherwise,
-  // send it to the specific decoder instance for that stream.
-  int which_channel =
-      GetReceiveChannelNum(ParseSsrc(packet->data(), packet->size(), false));
-  if (which_channel == -1) {
-    which_channel = voe_channel();
+  webrtc::PacketReceiver::DeliveryStatus delivery_result =
+      call_->Receiver()->DeliverPacket(webrtc::MediaType::AUDIO,
+          reinterpret_cast<const uint8_t*>(packet->data()), packet->size(),
+          webrtc_packet_time);
+  if (webrtc::PacketReceiver::DELIVERY_OK != delivery_result) {
+    return;
   }
 
+  // Find the channel to send this packet to. It must exist since webrtc::Call
+  // was able to demux the packet.
+  int channel = GetReceiveChannelNum(ssrc);
+  RTC_DCHECK(channel != -1);
+
   // Pass it off to the decoder.
   engine()->voe()->network()->ReceivedRTPPacket(
-      which_channel, packet->data(), packet->size(),
-      webrtc::PacketTime(packet_time.timestamp, packet_time.not_before));
+      channel, packet->data(), packet->size(), webrtc_packet_time);
 }
 
 void WebRtcVoiceMediaChannel::OnRtcpReceived(
@@ -2671,17 +2660,16 @@ void WebRtcVoiceMediaChannel::OnRtcpReceived(
     return;
   }
 
-  // If it is a sender report, find the channel that is listening.
-  bool has_sent_to_default_channel = false;
+  // If it is a sender report, find the receive channel that is listening.
   if (type == kRtcpTypeSR) {
-    int which_channel =
-        GetReceiveChannelNum(ParseSsrc(packet->data(), packet->size(), true));
-    if (which_channel != -1) {
+    uint32 ssrc = 0;
+    if (!GetRtcpSsrc(packet->data(), packet->size(), &ssrc)) {
+      return;
+    }
+    int channel = GetReceiveChannelNum(ssrc);

[pthatcher1, 2015/10/02 02:33:31]

Should this be recv_channel_id to be consistent?

[the sun, 2015/10/02 11:34:20]

Done.

+    if (channel != -1) {
       engine()->voe()->network()->ReceivedRTCPPacket(
-          which_channel, packet->data(), packet->size());
-
-      if (IsDefaultChannel(which_channel))
-        has_sent_to_default_channel = true;
+          channel, packet->data(), packet->size());
     }
   }
 
@@ -2689,11 +2677,6 @@ void WebRtcVoiceMediaChannel::OnRtcpReceived(
   // channels. So all RTCP packets must be forwarded all send channels. VoE
   // will filter out RR internally.
   for (const auto& ch : send_channels_) {
-    // Make sure not sending the same packet to default channel more than once.
-    if (IsDefaultChannel(ch.second->channel()) &&
-        has_sent_to_default_channel)
-      continue;
-
     engine()->voe()->network()->ReceivedRTCPPacket(
         ch.second->channel(), packet->data(), packet->size());
   }
@@ -2784,6 +2767,8 @@ bool WebRtcVoiceMediaChannel::SetSendBitrateInternal(int bps) {
 }
 
 bool WebRtcVoiceMediaChannel::GetStats(VoiceMediaInfo* info) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+
   bool echo_metrics_on = false;
   // These can take on valid negative values, so use the lowest possible level
   // as default rather than -1.
@@ -2885,18 +2870,9 @@ bool WebRtcVoiceMediaChannel::GetStats(VoiceMediaInfo* info) {
     info->senders.push_back(sinfo);
   }
 
-  // Build the list of receivers, one for each receiving channel, or 1 in
-  // a 1:1 call.
-  std::vector<int> channels;
+  // Get the SSRC and stats for each receiver.
   for (const auto& ch : receive_channels_) {
-    channels.push_back(ch.second->channel());
-  }
-  if (channels.empty()) {
-    channels.push_back(voe_channel());
-  }
-
-  // Get the SSRC and stats for each receiver, based on our own calculations.
-  for (int ch_id : channels) {
+    int ch_id = ch.second->channel();
     memset(&cs, 0, sizeof(cs));
     if (engine()->voe()->rtp()->GetRemoteSSRC(ch_id, ssrc) != -1 &&
         engine()->voe()->rtp()->GetRTCPStatistics(ch_id, cs) != -1 &&
@@ -2971,7 +2947,7 @@ bool WebRtcVoiceMediaChannel::GetStats(VoiceMediaInfo* info) {
 }
 
 bool WebRtcVoiceMediaChannel::FindSsrc(int channel_num, uint32* ssrc) {
-  rtc::CritScope lock(&receive_channels_cs_);
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());

[pthatcher1, 2015/10/02 02:33:31]

This is called by WebRtcVoiceEngine::CallbackOnError, so
WebRtcVoiceEngine::CallbackOnError will blow up if it's called on a different
thread.  Are we sure it will only be called on the worker thread?

[the sun, 2015/10/02 11:34:20]

Good point. Actually, AFAICT VoE only calls CallbackOnError() with -1 for the
channel argument, so I've removed this code instead.

   RTC_DCHECK(ssrc != NULL);
   if (channel_num == -1 && send_ != SEND_NOTHING) {
     // Sometimes the VoiceEngine core will throw error with channel_num = -1.
@@ -3021,17 +2997,17 @@ int WebRtcVoiceMediaChannel::GetOutputLevel(int channel) {
 }
 
 int WebRtcVoiceMediaChannel::GetReceiveChannelNum(uint32 ssrc) const {

[pthatcher1, 2015/10/02 02:33:31]

Can you rename this GetReceiveChannelId?

[the sun, 2015/10/02 11:34:19]

Yes, also renamed GetSendChannelNum() similarly.

+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   ChannelMap::const_iterator it = receive_channels_.find(ssrc);
   if (it != receive_channels_.end())
     return it->second->channel();
-  return (ssrc == default_receive_ssrc_) ? voe_channel() : -1;
+  return -1;
 }
 
 int WebRtcVoiceMediaChannel::GetSendChannelNum(uint32 ssrc) const {
   ChannelMap::const_iterator it = send_channels_.find(ssrc);
   if (it != send_channels_.end())
     return it->second->channel();
-
   return -1;
 }
 
@@ -3091,22 +3067,6 @@ bool WebRtcVoiceMediaChannel::EnableRtcp(int channel) {
   return true;
 }
 
-bool WebRtcVoiceMediaChannel::ResetRecvCodecs(int channel) {
-  int ncodecs = engine()->voe()->codec()->NumOfCodecs();
-  for (int i = 0; i < ncodecs; ++i) {
-    webrtc::CodecInst voe_codec;
-    if (engine()->voe()->codec()->GetCodec(i, voe_codec) != -1) {
-      voe_codec.pltype = -1;
-      if (engine()->voe()->codec()->SetRecPayloadType(
-          channel, voe_codec) == -1) {
-        LOG_RTCERR2(SetRecPayloadType, channel, ToString(voe_codec));
-        return false;
-      }
-    }
-  }
-  return true;
-}
-
 bool WebRtcVoiceMediaChannel::SetPlayout(int channel, bool playout) {
   if (playout) {
     LOG(LS_INFO) << "Starting playout for channel #" << channel;
@@ -3121,16 +3081,6 @@ bool WebRtcVoiceMediaChannel::SetPlayout(int channel, bool playout) {
   return true;
 }
 
-uint32 WebRtcVoiceMediaChannel::ParseSsrc(const void* data, size_t len,
-                                        bool rtcp) {
-  size_t ssrc_pos = (!rtcp) ? 8 : 4;
-  uint32 ssrc = 0;
-  if (len >= (ssrc_pos + sizeof(ssrc))) {
-    ssrc = rtc::GetBE32(static_cast<const char*>(data) + ssrc_pos);
-  }
-  return ssrc;
-}
-
 // Convert VoiceEngine error code into VoiceMediaChannel::Error enum.
 VoiceMediaChannel::Error
     WebRtcVoiceMediaChannel::WebRtcErrorToChannelError(int err_code) {
@@ -3219,26 +3169,12 @@ void WebRtcVoiceMediaChannel::RemoveAudioReceiveStream(uint32 ssrc) {
 
 bool WebRtcVoiceMediaChannel::SetRecvCodecsInternal(
     const std::vector<AudioCodec>& new_codecs) {
-  for (const AudioCodec& codec : new_codecs) {
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());

[pthatcher1, 2015/10/02 02:33:30]

This is called by  WebRtcVoiceMediaChannel::SetRecvCodecs, so can you put a
thread check there?

[the sun, 2015/10/02 11:34:20]

Done.

+  for (const auto& codec : new_codecs) {

[pthatcher1, 2015/10/02 02:33:30]

I prefer having the (non-auto) type on for loops

[the sun, 2015/10/02 11:34:20]

Done.

     webrtc::CodecInst voe_codec;
     if (engine()->FindWebRtcCodec(codec, &voe_codec)) {
       LOG(LS_INFO) << ToString(codec);
       voe_codec.pltype = codec.id;
-      if (default_receive_ssrc_ == 0) {
-        // Set the receive codecs on the default channel explicitly if the
-        // default channel is not used by |receive_channels_|, this happens in
-        // conference mode or in non-conference mode when there is no playout
-        // channel.
-        // TODO(xians): Figure out how we use the default channel in conference
-        // mode.
-        if (engine()->voe()->codec()->SetRecPayloadType(
-            voe_channel(), voe_codec) == -1) {
-          LOG_RTCERR2(SetRecPayloadType, voe_channel(), ToString(voe_codec));
-          return false;
-        }
-      }
-
-      // Set the receive codecs on all receiving channels.
       for (const auto& ch : receive_channels_) {
         if (engine()->voe()->codec()->SetRecPayloadType(
                 ch.second->channel(), voe_codec) == -1) {