Wire up transport sequence number / send time callbacks to webrtc via libjingle.

talk/session/media/channel.cc

Index: talk/session/media/channel.cc
diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc
index fc998c2531e0d564447a359da9363d117ffe6d5c..3e731fd8986a2c58be8ee3eb195f5459747109bc 100644
--- a/talk/session/media/channel.cc
+++ b/talk/session/media/channel.cc
@@ -30,6 +30,7 @@
 #include "talk/media/base/constants.h"
 #include "talk/media/base/rtputils.h"
 #include "webrtc/p2p/base/transportchannel.h"
+#include "talk/app/webrtc/mediacontroller.h"
 #include "talk/session/media/channelmanager.h"
 #include "webrtc/base/bind.h"
 #include "webrtc/base/buffer.h"
@@ -37,6 +38,8 @@
 #include "webrtc/base/common.h"
 #include "webrtc/base/dscp.h"
 #include "webrtc/base/logging.h"
+#include "webrtc/call.h"
+#include "webrtc/common_types.h"
 
 namespace cricket {
 
@@ -67,7 +70,7 @@ static void SafeSetError(const std::string& message, std::string* error_desc) {
 
 struct PacketMessageData : public rtc::MessageData {
   rtc::Buffer packet;
-  rtc::DiffServCodePoint dscp;
+  rtc::PacketOptions options;
 };
 
 struct ScreencastEventMessageData : public rtc::MessageData {
@@ -171,12 +174,14 @@ void RtpSendParametersFromMediaDescription(
 
 BaseChannel::BaseChannel(rtc::Thread* thread,
                          MediaChannel* media_channel,
+                         webrtc::MediaControllerInterface* media_controller,
                          TransportController* transport_controller,
                          const std::string& content_name,
                          bool rtcp)
     : worker_thread_(thread),
       transport_controller_(transport_controller),
       media_channel_(media_channel),
+      media_controller_(media_controller),
       content_name_(content_name),
       rtcp_transport_enabled_(rtcp),
       transport_channel_(nullptr),
@@ -344,6 +349,7 @@ void BaseChannel::ConnectToTransportChannel(TransportChannel* tc) {
   tc->SignalWritableState.connect(this, &BaseChannel::OnWritableState);
   tc->SignalReadPacket.connect(this, &BaseChannel::OnChannelRead);
   tc->SignalReadyToSend.connect(this, &BaseChannel::OnReadyToSend);
+  media_controller_->ConnectTransportChannel(tc);

[pthatcher1, 2015/10/07 16:44:38]

This isn't quite the right place to put this, since it will be called every time
bundle configuration changes.  I think it would be better to put it in
WebRtcSession::CreateChannels after the BaseChannels are created.  At that
point, get the transport channel from the audio and video channels.

[stefan-webrtc, 2015/10/08 12:53:45]

Ok, that won't cause problems if the transport channel changes somehow? Seems
like since we have BaseChannel::SetTransport we may create new transport
channels after WebRtcSession::CreateChannels has returned?

I guess if I connect the two after the call to EnableBundle we should be fine?

[pthatcher1, 2015/10/08 20:27:55]

You can connect before or after EnableBundle.  The only way the transport
channel can change is if it changes to a transport channel from another
BaseChannel.   

That does mean we should worry about the case where they are bundled onto the
BaseChannel for data (the DataChannel).  In that case, you'd need to either
listen to the TransportChannel of the DataChannel or do it post-BUNDLE.  But
BUNDLE can happen at any time, so that's not really safe to rely on.

[stefan-webrtc, 2015/10/09 14:18:44]

Acknowledged.

 }
 
 void BaseChannel::DisconnectFromTransportChannel(TransportChannel* tc) {
@@ -352,6 +358,7 @@ void BaseChannel::DisconnectFromTransportChannel(TransportChannel* tc) {
   tc->SignalWritableState.disconnect(this);
   tc->SignalReadPacket.disconnect(this);
   tc->SignalReadyToSend.disconnect(this);
+  media_controller_->DisconnectTransportChannel(tc);

[pthatcher1, 2015/10/07 16:44:38]

Woh, this won't work.  If two BaseChannels share a TransportChannel and one of
them is deleted, it will disconnect, and then the media controller will also
disconnect.

I think the easiest thing is to just not have a DisconnectTransportChannel
method.  The worst thing that will happen is that MediaController will have
dangling pointers in transport_channels_.  But as long as we document that, I
think it's fine.  

[stefan-webrtc, 2015/10/07 16:55:25]

Very good point. I will look at these comments later today or tomorrow.

[stefan-webrtc, 2015/10/08 12:53:45]

I assume media controller is guaranteed to outlive transport channels then?

[pthatcher1, 2015/10/08 20:27:55]

Yes, a MediaController has to outlive all the BaseChannels and once all the
BaseChannels are destroyed, then the TransportChannels are as well.

However, that is a bit subtle and probably easy to screw up in the future.  We
could fix it, but it would be a bit complex.  Now that I read your code in
WebRtcSession, I realize that it probably makes more sense to use WebRtcSession
as our temporary bind point and not MediaController.  I apologize for jumping
around on the design so much on this, but I think this will be a lot better.
Something like this:

1.  WebRtcSession connects to TransportChannel::OnSentPacket in three places:
  A.  At the end of CreateVoiceChannel.
  B.  At the end of CreateVideoChannel.
  C.  At the end of CreateDataChannel.
2.  WebRtcSession never disconnects from TransportChannel::OnSentPacket because
the session is guaranteed to outlive the TransportChannel since it owns the
TransportController.
3.  WebRtcSession::OnSentPacket_w can then call
media_controller_->OnSentPacket() or
media_controller_->call_w()->OnSentPacket(), whichever is easier.


This way, it's safer and the MediaController doesn't have to tack the
TransportChannel lifetimes.  


Sorry again for the jumping around to different designs.  But it looks like we
can end up in a good state.

[stefan-webrtc, 2015/10/09 14:18:44]

No problem, I prefer to do the right thing here and simplify for the future
improvements you have planned. And of course it would be better to avoid random
crashes... :) This is difficult to get right, and I wouldn't have been able to
do it in twice the time without your help.

I managed to get this working, and with a bit of code reading I managed to write
a test.

 }
 
 bool BaseChannel::Enable(bool enable) {
@@ -431,13 +438,13 @@ bool BaseChannel::IsReadyToSend() const {
 }
 
 bool BaseChannel::SendPacket(rtc::Buffer* packet,
-                             rtc::DiffServCodePoint dscp) {
-  return SendPacket(false, packet, dscp);
+                             const rtc::PacketOptions& options) {
+  return SendPacket(false, packet, options);
 }
 
 bool BaseChannel::SendRtcp(rtc::Buffer* packet,
-                           rtc::DiffServCodePoint dscp) {
-  return SendPacket(true, packet, dscp);
+                           const rtc::PacketOptions& options) {
+  return SendPacket(true, packet, options);
 }
 
 int BaseChannel::SetOption(SocketType type, rtc::Socket::Option opt,
@@ -506,8 +513,9 @@ bool BaseChannel::PacketIsRtcp(const TransportChannel* channel,
           rtcp_mux_filter_.DemuxRtcp(data, static_cast<int>(len)));
 }
 
-bool BaseChannel::SendPacket(bool rtcp, rtc::Buffer* packet,
-                             rtc::DiffServCodePoint dscp) {
+bool BaseChannel::SendPacket(bool rtcp,
+                             rtc::Buffer* packet,
+                             const rtc::PacketOptions& options) {
   // SendPacket gets called from MediaEngine, typically on an encoder thread.
   // If the thread is not our worker thread, we will post to our worker
   // so that the real work happens on our worker. This avoids us having to
@@ -520,7 +528,7 @@ bool BaseChannel::SendPacket(bool rtcp, rtc::Buffer* packet,
     int message_id = (!rtcp) ? MSG_RTPPACKET : MSG_RTCPPACKET;
     PacketMessageData* data = new PacketMessageData;
     data->packet = packet->Pass();
-    data->dscp = dscp;
+    data->options = options;
     worker_thread_->Post(this, message_id, data);
     return true;
   }
@@ -543,7 +551,8 @@ bool BaseChannel::SendPacket(bool rtcp, rtc::Buffer* packet,
     return false;
   }
 
-  rtc::PacketOptions options(dscp);
+  rtc::PacketOptions updated_options;
+  updated_options = options;
   // Protect if needed.
   if (srtp_filter_.IsActive()) {
     bool res;
@@ -559,21 +568,22 @@ bool BaseChannel::SendPacket(bool rtcp, rtc::Buffer* packet,
       res = srtp_filter_.ProtectRtp(
           data, len, static_cast<int>(packet->capacity()), &len);
 #else
-      options.packet_time_params.rtp_sendtime_extension_id =
+      updated_options.packet_time_params.rtp_sendtime_extension_id =
           rtp_abs_sendtime_extn_id_;
       res = srtp_filter_.ProtectRtp(
           data, len, static_cast<int>(packet->capacity()), &len,
-          &options.packet_time_params.srtp_packet_index);
+          &updated_options.packet_time_params.srtp_packet_index);
       // If protection succeeds, let's get auth params from srtp.
       if (res) {
         uint8* auth_key = NULL;
         int key_len;
         res = srtp_filter_.GetRtpAuthParams(
-            &auth_key, &key_len, &options.packet_time_params.srtp_auth_tag_len);
+            &auth_key, &key_len,
+            &updated_options.packet_time_params.srtp_auth_tag_len);
         if (res) {
-          options.packet_time_params.srtp_auth_key.resize(key_len);
-          options.packet_time_params.srtp_auth_key.assign(auth_key,
-                                                          auth_key + key_len);
+          updated_options.packet_time_params.srtp_auth_key.resize(key_len);
+          updated_options.packet_time_params.srtp_auth_key.assign(
+              auth_key, auth_key + key_len);
         }
       }
 #endif
@@ -1252,7 +1262,8 @@ void BaseChannel::OnMessage(rtc::Message *pmsg) {
     case MSG_RTPPACKET:
     case MSG_RTCPPACKET: {
       PacketMessageData* data = static_cast<PacketMessageData*>(pmsg->pdata);
-      SendPacket(pmsg->message_id == MSG_RTCPPACKET, &data->packet, data->dscp);
+      SendPacket(pmsg->message_id == MSG_RTCPPACKET, &data->packet,
+                 data->options);
       delete data;  // because it is Posted
       break;
     }
@@ -1278,11 +1289,13 @@ void BaseChannel::FlushRtcpMessages() {
 VoiceChannel::VoiceChannel(rtc::Thread* thread,
                            MediaEngineInterface* media_engine,
                            VoiceMediaChannel* media_channel,
+                           webrtc::MediaControllerInterface* media_controller,
                            TransportController* transport_controller,
                            const std::string& content_name,
                            bool rtcp)
     : BaseChannel(thread,
                   media_channel,
+                  media_controller,
                   transport_controller,
                   content_name,
                   rtcp),
@@ -1596,11 +1609,13 @@ void VoiceChannel::GetSrtpCryptoSuiteNames(
 
 VideoChannel::VideoChannel(rtc::Thread* thread,
                            VideoMediaChannel* media_channel,
+                           webrtc::MediaControllerInterface* media_controller,
                            TransportController* transport_controller,
                            const std::string& content_name,
                            bool rtcp)
     : BaseChannel(thread,
                   media_channel,
+                  media_controller,
                   transport_controller,
                   content_name,
                   rtcp),
@@ -1991,6 +2006,7 @@ DataChannel::DataChannel(rtc::Thread* thread,
                          bool rtcp)
     : BaseChannel(thread,
                   media_channel,
+                  nullptr,
                   transport_controller,
                   content_name,
                   rtcp),