Address feedback

Author: algesten

webrtc/CHANGELOG.md

@@ -5,7 +5,8 @@
 * Added more stats to `RemoteInboundRTPStats` and `RemoteOutboundRTPStats` [#282](https://github.com/webrtc-rs/webrtc/pull/282) by [@k0nserv](https://github.com/k0nserv).
 * Don't register `video/rtx` codecs in `MediaEngine::register_default_codecs`. These weren't actually support and prevented RTX in the existing RTP stream from being used. Long term we should support RTX via this method, this is tracked in [#295](https://github.com/webrtc-rs/webrtc/issues/295). [#294 Remove video/rtx codecs](https://github.com/webrtc-rs/webrtc/pull/294) contributed by [k0nserv](https://github.com/k0nserv)
 * Add IP filter to WebRTC `SettingEngine` [#306](https://github.com/webrtc-rs/webrtc/pull/306)
-
+* Stop sequence numbers from increasing in `TrackLocalStaticSample` while the bound `RTCRtpSender` have
+directions that should not send. [#316](https://github.com/webrtc-rs/webrtc/pull/316)
 
 ## 0.5.1
 

webrtc/src/track/track_local/mod.rs

@@ -105,30 +105,30 @@ pub(crate) struct TrackBinding {
     ssrc: SSRC,
     payload_type: PayloadType,
     write_stream: Option<Arc<dyn TrackLocalWriter + Send + Sync>>,
-    paused: Arc<AtomicBool>,
+    sender_paused: Arc<AtomicBool>,
 }
 
 impl TrackBinding {
-    pub fn is_paused(&self) -> bool {
-        self.paused.load(Ordering::SeqCst)
+    pub fn is_sender_paused(&self) -> bool {
+        self.sender_paused.load(Ordering::SeqCst)
     }
 }
 
 pub(crate) struct InterceptorToTrackLocalWriter {
     pub(crate) interceptor_rtp_writer: Mutex<Option<Arc<dyn RTPWriter + Send + Sync>>>,
-    paused: Arc<AtomicBool>,
+    sender_paused: Arc<AtomicBool>,
 }
 
 impl InterceptorToTrackLocalWriter {
     pub(crate) fn new(paused: Arc<AtomicBool>) -> Self {
         InterceptorToTrackLocalWriter {
             interceptor_rtp_writer: Mutex::new(None),
-            paused,
+            sender_paused: paused,
         }
     }
 
-    fn is_paused(&self) -> bool {
-        self.paused.load(Ordering::SeqCst)
+    fn is_sender_paused(&self) -> bool {
+        self.sender_paused.load(Ordering::SeqCst)
     }
 }
 
@@ -141,7 +141,7 @@ impl std::fmt::Debug for InterceptorToTrackLocalWriter {
 #[async_trait]
 impl TrackLocalWriter for InterceptorToTrackLocalWriter {
     async fn write_rtp(&self, pkt: &rtp::packet::Packet) -> Result<usize> {
-        if self.is_paused() {
+        if self.is_sender_paused() {
             return Ok(0);
         }
 

webrtc/src/track/track_local/track_local_static_rtp.rs

@@ -31,12 +31,16 @@ impl TrackLocalStaticRTP {
 
     pub async fn any_binding_paused(&self) -> bool {
         let bindings = self.bindings.lock().await;
-        bindings.iter().any(|b| b.paused.load(Ordering::SeqCst))
+        bindings
+            .iter()
+            .any(|b| b.sender_paused.load(Ordering::SeqCst))
     }
 
     pub async fn all_binding_paused(&self) -> bool {
         let bindings = self.bindings.lock().await;
-        bindings.iter().all(|b| b.paused.load(Ordering::SeqCst))
+        bindings
+            .iter()
+            .all(|b| b.sender_paused.load(Ordering::SeqCst))
     }
 }
 
@@ -60,7 +64,7 @@ impl TrackLocal for TrackLocalStaticRTP {
                     payload_type: codec.payload_type,
                     write_stream: t.write_stream(),
                     id: t.id(),
-                    paused: t.paused.clone(),
+                    sender_paused: t.paused.clone(),
                 }));
             }
 
@@ -123,6 +127,11 @@ impl TrackLocalWriter for TrackLocalStaticRTP {
     /// If one PeerConnection fails the packets will still be sent to
     /// all PeerConnections. The error message will contain the ID of the failed
     /// PeerConnections so you can remove them
+    ///
+    /// If the RTCRtpSender direction is such that no packets should be sent, any call to this
+    /// function are blocked internally. Care must be taken to not increase the sequence number
+    /// while the sender is paused. While the actual _sending_ is blocked, the receiver will
+    /// miss out when the sequence number "rolls over", which in turn will break SRTP.
     async fn write_rtp(&self, p: &rtp::packet::Packet) -> Result<usize> {
         let mut n = 0;
         let mut write_errs = vec![];
@@ -133,7 +142,8 @@ impl TrackLocalWriter for TrackLocalStaticRTP {
             bindings.clone()
         };
         for b in bindings {
-            if b.is_paused() {
+            if b.is_sender_paused() {
+                // See caveat in function doc.
                 continue;
             }
             pkt.header.ssrc = b.ssrc;

webrtc/src/track/track_local/track_local_static_sample.rs

@@ -71,7 +71,7 @@ impl TrackLocalStaticSample {
             // the packet. I.e. we get the same sequence number per multiple SSRC, which is not good
             // for SRTP, but that's how it works.
             //
-            // Chrome has further a problem with regards to jumps in sequence number. Consider this:
+            // SRTP has a further problem with regards to jumps in sequence number. Consider this:
             //
             // 1. Create track local
             // 2. Bind track local to track 1.
@@ -80,9 +80,11 @@ impl TrackLocalStaticSample {
             // 5. Keep sending...
             //
             // At this point, the track local will keep incrementing the sequence number, because we have
-            // one binding that is still active. However Chrome can only accept a relatively small jump
-            // in SRTP key deriving, which means if this pause state of one binding persists for a longer
-            // time, the track can never be resumed (against Chrome).
+            // one binding that is still active. However SRTP hmac verifying (tag), can only accept a
+            // relatively small jump in sequence numbers since it uses the ROC (i.e. how many times the
+            // sequence number has rolled over), which means if this pause state of one binding persists
+            // for a longer time, the track can never be resumed since the receiver would have missed
+            // the rollovers.
             if !internal.did_warn_about_wonky_pause {
                 internal.did_warn_about_wonky_pause = true;
                 warn!("Detected multiple track bindings where only one was paused");