Move RTP/RTCP demuxing logic from BaseChannel to RtpTransport.

webrtc/pc/rtptransport.cc

Index: webrtc/pc/rtptransport.cc
diff --git a/webrtc/pc/rtptransport.cc b/webrtc/pc/rtptransport.cc
index 2ee27e02fb673725ec3c66ab9260d723d6083dda..831ca7a19d78034256ca1c2907f4c5cb8b5bd1a5 100644
--- a/webrtc/pc/rtptransport.cc
+++ b/webrtc/pc/rtptransport.cc
@@ -12,6 +12,8 @@
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/copyonwritebuffer.h"
+#include "webrtc/base/trace_event.h"
+#include "webrtc/media/base/rtputils.h"
 #include "webrtc/p2p/base/packettransportinterface.h"
 
 namespace webrtc {
@@ -28,10 +30,13 @@ void RtpTransport::SetRtpPacketTransport(
   }
   if (rtp_packet_transport_) {
     rtp_packet_transport_->SignalReadyToSend.disconnect(this);
+    rtp_packet_transport_->SignalReadPacket.disconnect(this);
   }
   if (new_packet_transport) {
     new_packet_transport->SignalReadyToSend.connect(
         this, &RtpTransport::OnReadyToSend);
+    new_packet_transport->SignalReadPacket.connect(this,
+                                                   &RtpTransport::OnReadPacket);
   }
   rtp_packet_transport_ = new_packet_transport;
 
@@ -48,10 +53,13 @@ void RtpTransport::SetRtcpPacketTransport(
   }
   if (rtcp_packet_transport_) {
     rtcp_packet_transport_->SignalReadyToSend.disconnect(this);
+    rtcp_packet_transport_->SignalReadPacket.disconnect(this);
   }
   if (new_packet_transport) {
     new_packet_transport->SignalReadyToSend.connect(
         this, &RtpTransport::OnReadyToSend);
+    new_packet_transport->SignalReadPacket.connect(this,
+                                                   &RtpTransport::OnReadPacket);
   }
   rtcp_packet_transport_ = new_packet_transport;
 
@@ -87,6 +95,18 @@ bool RtpTransport::SendPacket(bool rtcp,
   return true;
 }
 
+bool RtpTransport::HandlesPacket(const uint8_t* data, size_t len) {
+  return bundle_filter_.DemuxPacket(data, len);
+}
+
+bool RtpTransport::HandlesPayloadType(int payload_type) const {
+  return bundle_filter_.FindPayloadType(payload_type);
+}
+
+void RtpTransport::AddHandledPayloadType(int payload_type) {
+  bundle_filter_.AddPayloadType(payload_type);
+}
+
 PacketTransportInterface* RtpTransport::GetRtpPacketTransport() const {
   return rtp_packet_transport_;
 }
@@ -142,4 +162,87 @@ void RtpTransport::MaybeSignalReadyToSend() {
   }
 }
 
+void RtpTransport::OnReadPacket(rtc::PacketTransportInternal* transport,
+                                const char* data,
+                                size_t len,
+                                const rtc::PacketTime& packet_time,
+                                int flags) {
+  TRACE_EVENT0("webrtc", "RtpTransport::OnReadPacket");
+
+  // When using RTCP multiplexing we might get RTCP packets on the RTP
+  // transport. We feed RTP traffic into the demuxer to determine if it is RTCP.
+  bool rtcp = PacketIsRtcp(transport, data, len);
+  rtc::CopyOnWriteBuffer packet(data, len);
+
+  if (!WantsPacket(rtcp, &packet)) {
+    return;
+  }
+
+  if (!has_received_packet_ && !rtcp) {
+    has_received_packet_ = true;
+    SignalFirstPacketReceived();

[Taylor Brandstetter, 2017/05/25 16:14:25]

nit: Since ultimately "SignalFirstPacketReceived" turns into an RtpReceiver
signal (meaning it ultimately needs to be per-receiver instead of
per-transport), I think it would make more sense to keep this logic confined to
BaseChannel.

[Zach Stein, 2017/05/30 21:50:55]

Done.

+  }
+
+  // This mutates packet if it is protected.
+  SignalPacketReceived(rtcp, packet, packet_time);
+
+  // Supports early media timeout in VoiceChannel.
+  // TODO(zstein): See if this can be combined with has_received_packet_.
+  if (!received_media_ && !rtcp) {
+    received_media_ = true;

[Taylor Brandstetter, 2017/05/25 16:14:25]

Same sort of situation here. VoiceChannel already has the information it needs
to determine "received media", so storing this flag in RtpTransport doesn't seem
to add value. And later, when it will be possible to have multiple VoiceChannels
(that need independent "received media" flags) using the same RtpTransport, this
would become more complicated.

[Zach Stein, 2017/05/30 21:50:55]

Done.

+  }
+}
+
+// Check the RTP payload type.  If 63 < payload type < 96, it's RTCP.
+// For additional details, see http://tools.ietf.org/html/rfc5761.
+bool IsRtcp(const char* data, int len) {

[Taylor Brandstetter, 2017/05/25 16:14:25]

If we're moving things to rtputils.h, this would be a good candidate.

[Zach Stein, 2017/05/30 21:50:55]

I was only moving methods that are called from multiple files to rtputils. I was
thinking they might end up being only in rtptransport.cc long term, but don't
feel strongly.

+  if (len < 2) {
+    return false;
+  }
+  char pt = data[1] & 0x7F;
+  return (63 < pt) && (pt < 96);
+}
+
+
+bool RtpTransport::PacketIsRtcp(const rtc::PacketTransportInternal* transport,
+                                const char* data,
+                                size_t len) {
+  // TODO(zstein): Can't check if we offered mux from here and we fail
+  // SendEarlyRtcpMuxToRtcpMux if we only check rtcp_mux_enabled. RtcpMuxFilter
+  // had:
+
+  // If we're muxing RTP/RTCP, we must inspect each packet delivered
+  // and determine whether it is RTP or RTCP. We do so by looking at
+  // the RTP payload type (see IsRtcp).  Note that if we offer RTCP
+  // mux, we may receive muxed RTCP before we receive the answer, so
+  // we operate in that state too.
+  // bool offered_mux = ((state_ == ST_SENTOFFER) && offer_enable_);
+  // return (IsActive() || offered_mux) && IsRtcp(data, len);
+
+  // TODO(zstein): We could give RtpTransport a reference to the mux filter
+  // instead of using rtcp_mux_enabled_ here (and some yet to be implemented
+  // logic to know if mux is provisionally enabled). Or can we just always
+  // rely on IsRtcp (and not bother checking rtcp_mux_enabled)?

[Taylor Brandstetter, 2017/05/25 16:14:25]

Let's just do this. The only way we could *not offer RTCP mux is if the
application manually removes "a=rtcp-mux" from the offer. So receiving a
RTCP-muxed packet when RTCP mux wasn't offered is a corner case of a corner
case, which will eventually not even be possible, and thus isn't worth doing
anything about.

[Zach Stein, 2017/05/30 21:50:55]

Done.

Am I understanding correctly that the existing implementation could interpret an
rtp packet incorrectly as an rtcp packet if mux is offered but not supported by
the remote client and the rtp packet is received before the answer?

[Taylor Brandstetter, 2017/05/31 08:25:03]

Even then, the remote client should only use payload types we offered... which
won't be in the range that would cause the packet to be interpreted as RTCP. I
don't think there's any situation where this is actually an issue.

[Zach Stein, 2017/06/01 03:45:58]

Acknowledged.

+
+  return transport == rtcp_packet_transport() ||
+      (/*rtcp_mux_enabled_ &&*/ IsRtcp(data, len));
+}
+
+bool RtpTransport::WantsPacket(bool rtcp,
+                               const rtc::CopyOnWriteBuffer* packet) {
+  // Protect ourselves against crazy data.
+  if (!packet || !cricket::IsValidRtpRtcpPacketSize(rtcp, packet->size())) {
+    LOG(LS_ERROR) << "Dropping incoming "
+                  << cricket::PacketType(rtcp)
+                  << " packet: wrong size=" << packet->size();
+    return false;
+  }
+  if (rtcp) {
+    // Permit all (seemingly valid) RTCP packets.
+    return true;
+  }
+  // Check whether we handle this payload.
+  return HandlesPacket(packet->data(), packet->size());
+}
+
 }  // namespace webrtc