Base A/V synchronization on sync_labels.

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 84a26c51b8558e02bfb6d25bda85494bc15eee0d..3d7b36fe2c9c8649c0d9245706974070459712a0 100644
--- a/talk/media/webrtc/webrtcvoiceengine.cc
+++ b/talk/media/webrtc/webrtcvoiceengine.cc
@@ -1624,8 +1624,7 @@ class WebRtcVoiceMediaChannel::WebRtcVoiceChannelRenderer
                              webrtc::AudioTransport* voe_audio_transport)
       : channel_(ch),
         voe_audio_transport_(voe_audio_transport),
-        renderer_(NULL) {
-  }
+        renderer_(NULL) {}
   ~WebRtcVoiceChannelRenderer() override { Stop(); }
 
   // Starts the rendering by setting a sink to the renderer to get data
@@ -2479,9 +2478,9 @@ bool WebRtcVoiceMediaChannel::AddSendStream(const StreamParams& sp) {
   // delete the channel in case failure happens below.
   webrtc::AudioTransport* audio_transport =
       engine()->voe()->base()->audio_transport();
-  send_channels_.insert(std::make_pair(
-      sp.first_ssrc(),
-      new WebRtcVoiceChannelRenderer(channel, audio_transport)));
+  send_channels_.insert(
+      std::make_pair(sp.first_ssrc(),
+                     new WebRtcVoiceChannelRenderer(channel, audio_transport)));
 
   // Set the send (local) SSRC.
   // If there are multiple send SSRCs, we can only set the first one here, and
@@ -2574,7 +2573,8 @@ bool WebRtcVoiceMediaChannel::AddRecvStream(const StreamParams& sp) {
     return false;
   }
 
-  TryAddAudioRecvStream(ssrc);
+  DCHECK(receive_stream_params_.find(ssrc) == receive_stream_params_.end());

[stefan-webrtc, 2015/06/16 15:33:39]

Can someone give us a fake sdp and cause a crash here, or is this ssrc always
generated locally?

[the sun, 2015/06/18 08:38:37]

Notice that we look for the ssrc in receive_channels_ a few lines up, and return
gracefully if we find it. This assert is just to make sure receive_channels_ and
receive_sgtream_params_ are in sync.

[stefan-webrtc, 2015/06/22 14:19:42]

Acknowledged.

[pbos-webrtc, 2015/06/24 09:46:09]

I moved the storing of the StreamParams to prevent having to erase them, now
they are stored at the same place as receive_channels_.

+  receive_stream_params_[ssrc] = sp;
 
   // Reuse default channel for recv stream in non-conference mode call
   // when the default channel is not being used.
@@ -2583,9 +2583,10 @@ bool WebRtcVoiceMediaChannel::AddRecvStream(const StreamParams& sp) {
   if (!InConferenceMode() && default_receive_ssrc_ == 0) {
     LOG(LS_INFO) << "Recv stream " << ssrc << " reuse default channel";
     default_receive_ssrc_ = ssrc;
-    receive_channels_.insert(std::make_pair(
-        default_receive_ssrc_,
-        new WebRtcVoiceChannelRenderer(voe_channel(), audio_transport)));
+    WebRtcVoiceChannelRenderer* channel_renderer =
+        new WebRtcVoiceChannelRenderer(voe_channel(), audio_transport);
+    receive_channels_.insert(std::make_pair(ssrc, channel_renderer));
+    TryAddAudioRecvStream(ssrc);
     return SetPlayout(voe_channel(), playout_);
   }
 
@@ -2593,17 +2594,20 @@ bool WebRtcVoiceMediaChannel::AddRecvStream(const StreamParams& sp) {
   int channel = engine()->CreateMediaVoiceChannel();
   if (channel == -1) {
     LOG_RTCERR0(CreateChannel);
+    receive_stream_params_.erase(ssrc);
     return false;
   }
 
   if (!ConfigureRecvChannel(channel)) {
     DeleteChannel(channel);
+    receive_stream_params_.erase(ssrc);
     return false;
   }
 
-  receive_channels_.insert(
-      std::make_pair(
-          ssrc, new WebRtcVoiceChannelRenderer(channel, audio_transport)));
+  WebRtcVoiceChannelRenderer* channel_renderer =
+      new WebRtcVoiceChannelRenderer(channel, audio_transport);
+  receive_channels_.insert(std::make_pair(ssrc, channel_renderer));
+  TryAddAudioRecvStream(ssrc);
 
   LOG(LS_INFO) << "New audio stream " << ssrc
                << " registered to VoiceEngine channel #"
@@ -2694,6 +2698,7 @@ bool WebRtcVoiceMediaChannel::RemoveRecvStream(uint32 ssrc) {
   }
 
   TryRemoveAudioRecvStream(ssrc);
+  receive_stream_params_.erase(ssrc);
 
   // Delete the WebRtcVoiceChannelRenderer object connected to the channel, this
   // will disconnect the audio renderer with the receive channel.
@@ -3450,7 +3455,7 @@ int WebRtcVoiceMediaChannel::GetReceiveChannelNum(uint32 ssrc) {
   ChannelMap::iterator it = receive_channels_.find(ssrc);
   if (it != receive_channels_.end())
     return it->second->channel();
-  return (ssrc == default_receive_ssrc_) ?  voe_channel() : -1;
+  return (ssrc == default_receive_ssrc_) ? voe_channel() : -1;
 }
 
 int WebRtcVoiceMediaChannel::GetSendChannelNum(uint32 ssrc) {
@@ -3623,15 +3628,23 @@ bool WebRtcVoiceMediaChannel::SetHeaderExtension(ExtensionSetterFunction setter,
 
 void WebRtcVoiceMediaChannel::TryAddAudioRecvStream(uint32 ssrc) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  WebRtcVoiceChannelRenderer* channel = receive_channels_[ssrc];
+  DCHECK(channel != nullptr);
+  DCHECK(receive_streams_.find(ssrc) == receive_streams_.end());
   // If we are hooked up to a webrtc::Call, create an AudioReceiveStream too.
-  if (call_ && options_.combined_audio_video_bwe.GetWithDefaultIfUnset(false)) {
-    DCHECK(receive_streams_.find(ssrc) == receive_streams_.end());
-    webrtc::AudioReceiveStream::Config config;
-    config.rtp.remote_ssrc = ssrc;
+  if (!call_) {
+    return;
+  }
+  webrtc::AudioReceiveStream::Config config;
+  config.rtp.remote_ssrc = ssrc;
+  // Only add RTP extensions if we support combined AV BWE.

[stefan-webrtc, 2015/06/16 15:33:39]

A/V BWE

[pbos-webrtc, 2015/06/24 09:46:09]

Done.

+  if (options_.combined_audio_video_bwe.GetWithDefaultIfUnset(false)) {
     config.rtp.extensions = recv_rtp_extensions_;
-    webrtc::AudioReceiveStream* s = call_->CreateAudioReceiveStream(config);
-    receive_streams_.insert(std::make_pair(ssrc, s));
   }
+  config.voe_channel_id = channel->channel();
+  config.sync_group = receive_stream_params_[ssrc].sync_label;

[stefan-webrtc, 2015/06/16 15:33:39]

Does this mean A/V BWE depends on sync labels? I would like some explanations on
this code.

[the sun, 2015/06/18 08:38:37]

No, we create the ARS regardless, but only set rtp extensions on it if we have
combined BWE enabled.

[stefan-webrtc, 2015/06/22 14:19:42]

Yes, I have no clue how I read this code. :)

+  webrtc::AudioReceiveStream* s = call_->CreateAudioReceiveStream(config);
+  receive_streams_.insert(std::make_pair(ssrc, s));
 }
 
 void WebRtcVoiceMediaChannel::TryRemoveAudioRecvStream(uint32 ssrc) {