Base A/V synchronization on sync_labels.

webrtc/video/call.cc

Index: webrtc/video/call.cc
diff --git a/webrtc/video/call.cc b/webrtc/video/call.cc
index cde41bc77a69d589c2cdb18620fbb981371ef9db..d3c1044dc7ad0d38e90dead325037208378b911c 100644
--- a/webrtc/video/call.cc
+++ b/webrtc/video/call.cc
@@ -109,6 +109,9 @@ class Call : public webrtc::Call, public PacketReceiver {
   void SetBitrateControllerConfig(
       const webrtc::Call::Config::BitrateConfig& bitrate_config);
 
+  void ConfigureSync(const std::string& sync_group)

[mflodman, 2015/06/22 04:53:28]

Would prefer to include AV in this name.

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

I'm not sure I agree. We may want to sync video with video as well, right?

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

Yes, we want this parameter to be for syncing streams in general, limiting this
to a pair of a/v channels is an implementation detail.

+      EXCLUSIVE_LOCKS_REQUIRED(receive_crit_);
+
   const int num_cpu_cores_;
   const rtc::scoped_ptr<ProcessThread> module_process_thread_;
   const rtc::scoped_ptr<ChannelGroup> channel_group_;
@@ -130,6 +133,8 @@ class Call : public webrtc::Call, public PacketReceiver {
       GUARDED_BY(receive_crit_);
   std::set<VideoReceiveStream*> video_receive_streams_
       GUARDED_BY(receive_crit_);
+  std::map<std::string, AudioReceiveStream*> sync_stream_mapping_
+      GUARDED_BY(receive_crit_);
 
   rtc::scoped_ptr<RWLockWrapper> send_crit_;
   std::map<uint32_t, VideoSendStream*> video_send_ssrcs_ GUARDED_BY(send_crit_);
@@ -219,6 +224,7 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream(
     DCHECK(audio_receive_ssrcs_.find(config.rtp.remote_ssrc) ==
         audio_receive_ssrcs_.end());
     audio_receive_ssrcs_[config.rtp.remote_ssrc] = receive_stream;
+    ConfigureSync(config.sync_group);
   }
   return receive_stream;
 }
@@ -234,6 +240,13 @@ void Call::DestroyAudioReceiveStream(
     size_t num_deleted = audio_receive_ssrcs_.erase(
         audio_receive_stream->config().rtp.remote_ssrc);
     DCHECK(num_deleted == 1);
+    const std::string& sync_group = audio_receive_stream->config().sync_group;
+    const auto it = sync_stream_mapping_.find(sync_group);
+    if (it != sync_stream_mapping_.end() &&
+        it->second == audio_receive_stream) {
+      sync_stream_mapping_.erase(it);
+      ConfigureSync(sync_group);
+    }
   }
   delete audio_receive_stream;
 }
@@ -324,8 +337,11 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream(
     video_receive_ssrcs_[it->second.ssrc] = receive_stream;
   video_receive_streams_.insert(receive_stream);
 
+  ConfigureSync(config.sync_group);
+
   if (!network_enabled_)
     receive_stream->SignalNetworkState(kNetworkDown);
+
   return receive_stream;
 }
 
@@ -333,7 +349,6 @@ void Call::DestroyVideoReceiveStream(
     webrtc::VideoReceiveStream* receive_stream) {
   TRACE_EVENT0("webrtc", "Call::DestroyVideoReceiveStream");
   DCHECK(receive_stream != nullptr);
-
   VideoReceiveStream* receive_stream_impl = nullptr;
   {
     WriteLockScoped write_lock(*receive_crit_);
@@ -428,6 +443,34 @@ void Call::SignalNetworkState(NetworkState state) {
   }
 }
 
+void Call::ConfigureSync(const std::string& sync_group) {
+  // Set sync only if there was no previous one.

[mflodman, 2015/06/22 04:53:27]

Meaning we can only sync one audio stream with one video stream in a call, not
multiple pairs?

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

Acknowledged.

+  if (config_.voice_engine == nullptr || sync_group.empty())
+    return;
+
+  AudioReceiveStream* sync_audio_stream = nullptr;
+  // Find existing audio stream.
+  const auto it = sync_stream_mapping_.find(sync_group);
+  if (it != sync_stream_mapping_.end()) {
+    sync_audio_stream = it->second;

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

Shouldn't we fail with an error or warning if two video streams are synced to
one audio stream? Or in general if more than 2 streams are synced together? We
don't really support that.

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

Since we're literally using the SDP sync group we should fire errors based on
what is supported in the relevant spec (https://tools.ietf.org/html/rfc5888 ?).
Whenever we can't support the requested configuration, we should log warnings.

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

I'm not sure what you mean with this. Log errors when an SDP configures
something we don't support?
Agreed. And maybe return an error? Not sure what is expected by a javascript
user in this case, but trying to sync 2 or more video streams to a single audio
stream will break a/v sync for all.

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

Logging a warning should be appropriate. A SDP is correct in requesting more
than two streams to be synchronized, we just don't support it.

+  } else {
+    // No configured audio stream, see if we can find one.
+    for (const auto& kv : audio_receive_ssrcs_) {
+      if (kv.second->config().sync_group == sync_group) {
+        sync_audio_stream = kv.second;
+        break;
+      }
+    }
+  }
+  if (sync_audio_stream)
+    sync_stream_mapping_[sync_group] = sync_audio_stream;
+  for (VideoReceiveStream* video_stream : video_receive_streams_) {

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

Is this syncing all video receive streams to the audio stream if one of them had
a matching group? Doesn't necessarily have to be all that should be synced,
right?

[mflodman, 2015/06/22 04:53:27]

Agree.

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

Yes, huge bug here. Thanks.

I changed it so that it'll only sync if it's the same group, and it will only
sync one pair. The rest will get the channel id -1, and remove sync.

+    video_stream->SetSyncChannel(
+        config_.voice_engine,
+        sync_audio_stream ? sync_audio_stream->config().voe_channel_id : -1);
+  }
+}
+
 PacketReceiver::DeliveryStatus Call::DeliverRtcp(MediaType media_type,
                                                  const uint8_t* packet,
                                                  size_t length) {