Fix over-NACK due to NackPair lost_packets not resetting (#372)

Co-authored-by: Olof Kraigher <[email protected]>
webrtc-rs · Dec 19, 2022 · e32feda · e32feda
1 parent 630c46f
commit e32feda
Show file tree

Hide file tree

Showing 7 changed files with 39 additions and 14 deletions.
diff --git a/interceptor/CHANGELOG.md b/interceptor/CHANGELOG.md
@@ -1,6 +1,7 @@
 # interceptor changelog
 
 ## Unreleased
+- [#372 Fix over-NACK due not resetting lost_packets bitmask](https://github.com/webrtc-rs/webrtc/pull/372/)
 
 ## v0.8.1
 
@@ -9,7 +10,7 @@
 * Don't generate empty TWCC packets that libWebRTC will ignore. [#324](https://github.com/webrtc-rs/webrtc/pull/324) by [@k0nserv](https://github.com/k0nserv).
 * Increased minimum support rust version to `1.60.0`.
 * Increased required `webrtc-util` version to `0.7.0`.
- 
+
 
 ## v0.8.0
 

diff --git a/interceptor/Cargo.toml b/interceptor/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "interceptor"
-version = "0.8.1"
+version = "0.8.2"
 authors = ["Rain Liu <[email protected]>"]
 edition = "2018"
 description = "A pure Rust implementation of Pluggable RTP/RTCP processors"
@@ -13,7 +13,7 @@ rust-version = "1.60.0"
 [dependencies]
 util = { version = "0.7.0", path = "../util", package = "webrtc-util", default-features = false, features = ["marshal", "sync"] }
 rtp = { version = "0.6.7", path = "../rtp" }
-rtcp = { version = "0.7.0", path = "../rtcp" }
+rtcp = { version = "0.7.2", path = "../rtcp" }
 srtp = { version = "0.9.0", path = "../srtp", package = "webrtc-srtp" }
 
 tokio = { version = "1.19", features = ["sync", "time"] }

diff --git a/rtcp/CHANGELOG.md b/rtcp/CHANGELOG.md
@@ -1,6 +1,7 @@
 # rtcp changelog
 
 ## Unreleased
+- [#372 Fix over-NACK due not resetting lost_packets bitmask](https://github.com/webrtc-rs/webrtc/pull/372/)
 
 ## v0.7.1
 

diff --git a/rtcp/Cargo.toml b/rtcp/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "rtcp"
-version = "0.7.1"
+version = "0.7.2"
 authors = ["Rain Liu <[email protected]>", "Michael Uti <[email protected]>"]
 edition = "2018"
 description = "A pure Rust implementation of RTCP"

diff --git a/rtcp/src/transport_feedbacks/transport_layer_nack/mod.rs b/rtcp/src/transport_feedbacks/transport_layer_nack/mod.rs
@@ -60,6 +60,13 @@ impl Iterator for NackIterator {
 }
 
 impl NackPair {
+    pub fn new(seq: u16) -> Self {
+        Self {
+            packet_id: seq,
+            lost_packets: Default::default(),
+        }
+    }
+
     /// PacketList returns a list of Nack'd packets that's referenced by a NackPair
     pub fn packet_list(&self) -> Vec<u16> {
         self.into_iter().collect()
@@ -243,11 +250,7 @@ pub fn nack_pairs_from_sequence_numbers(seq_nos: &[u16]) -> Vec<NackPair> {
         return vec![];
     }
 
-    let mut nack_pair = NackPair {
-        packet_id: seq_nos[0],
-        ..Default::default()
-    };
-
+    let mut nack_pair = NackPair::new(seq_nos[0]);
     let mut pairs = vec![];
 
     for &seq in seq_nos.iter().skip(1) {
@@ -256,7 +259,7 @@ pub fn nack_pairs_from_sequence_numbers(seq_nos: &[u16]) -> Vec<NackPair> {
         }
         if seq <= nack_pair.packet_id || seq > nack_pair.packet_id.saturating_add(16) {
             pairs.push(nack_pair);
-            nack_pair.packet_id = seq;
+            nack_pair = NackPair::new(seq);
             continue;
         }
 
@@ -267,4 +270,4 @@ pub fn nack_pairs_from_sequence_numbers(seq_nos: &[u16]) -> Vec<NackPair> {
     pairs.push(nack_pair);
 
     pairs
-}
+}
diff --git a/rtcp/src/transport_feedbacks/transport_layer_nack/transport_layer_nack_test.rs b/rtcp/src/transport_feedbacks/transport_layer_nack/transport_layer_nack_test.rs
@@ -343,7 +343,7 @@ fn test_transport_layer_nack_pair_generation() {
                 },
                 NackPair {
                     packet_id: 99,
-                    lost_packets: 1,
+                    lost_packets: 0,
                 },
             ],
         ),
@@ -359,3 +359,23 @@ fn test_transport_layer_nack_pair_generation() {
         )
     }
 }
+
+/// This test case reproduced a bug in the implementation
+#[test]
+fn test_lost_packets_is_reset_when_crossing_16_bit_boundary() {
+    let seq: Vec<_> = (0u16..=17u16).collect();
+    assert_eq!(
+        nack_pairs_from_sequence_numbers(&seq),
+        vec![
+            NackPair {
+                packet_id: 0,
+                lost_packets: 0b1111_1111_1111_1111,
+            },
+            NackPair {
+                packet_id: 17,
+                // Was 0xffff before fixing the bug
+                lost_packets: 0b0000_0000_0000_0000,
+            }
+        ],
+    )
+}
diff --git a/webrtc/Cargo.toml b/webrtc/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "webrtc"
-version = "0.6.0"
+version = "0.6.1"
 authors = ["Rain Liu <[email protected]>"]
 edition = "2018"
 description = "A pure Rust implementation of WebRTC API"
@@ -17,7 +17,7 @@ rust-version = "1.60.0"
 data = { version = "0.6.0", path = "../data", package = "webrtc-data" }
 dtls = { version = "0.7.0", path = "../dtls", package = "webrtc-dtls" }
 ice = { version = "0.9.0", path = "../ice", package = "webrtc-ice" }
-interceptor = { version = "0.8.0", path = "../interceptor" }
+interceptor = { version = "0.8.2", path = "../interceptor" }
 mdns = { version = "0.5.0", path = "../mdns", package = "webrtc-mdns" }
 media = { version = "0.5.0", path = "../media", package = "webrtc-media" }
 rtcp = { version = "0.7.0", path = "../rtcp" }