sdp: Use vector instead of HashMap for SimulcastRid (#532)

This makes sure that rids are not reordered when doing offer <-> answer
webrtc-rs · Feb 10, 2024 · b71c822 · b71c822
1 parent 2d7e56a
commit b71c822
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 25 deletions.
diff --git a/webrtc/src/peer_connection/sdp/mod.rs b/webrtc/src/peer_connection/sdp/mod.rs
@@ -221,8 +221,8 @@ pub(crate) fn track_details_from_sdp(
                 rids: vec![],
                 ..Default::default()
             };
-            for rid in rids.keys() {
-                simulcast_track.rids.push(SmolStr::from(rid));
+            for rid in &rids {
+                simulcast_track.rids.push(SmolStr::from(&rid.id));
             }
             if simulcast_track.rids.len() == tracks_in_media_section.len() {
                 for track in &tracks_in_media_section {
@@ -239,8 +239,8 @@ pub(crate) fn track_details_from_sdp(
     incoming_tracks
 }
 
-pub(crate) fn get_rids(media: &MediaDescription) -> HashMap<String, SimulcastRid> {
-    let mut rids = HashMap::new();
+pub(crate) fn get_rids(media: &MediaDescription) -> Vec<SimulcastRid> {
+    let mut rids = vec![];
     let mut simulcast_attr: Option<String> = None;
     for attr in &media.attributes {
         if attr.key.as_str() == SDP_ATTRIBUTE_RID {
@@ -249,7 +249,7 @@ pub(crate) fn get_rids(media: &MediaDescription) -> HashMap<String, SimulcastRid
                 .as_ref()
                 .ok_or(SimulcastRidParseError::SyntaxIdDirSplit)
                 .and_then(SimulcastRid::try_from)
-                .map(|rid| rids.insert(rid.id.to_owned(), rid))
+                .map(|rid| rids.push(rid))
             {
                 log::warn!("Failed to parse RID: {}", err);
             }
@@ -272,7 +272,7 @@ pub(crate) fn get_rids(media: &MediaDescription) -> HashMap<String, SimulcastRid
                         (sc_id, false)
                     };
 
-                    if let Some(rid) = rids.get_mut(sc_id) {
+                    if let Some(rid) = rids.iter_mut().find(|f| f.id == sc_id) {
                         rid.paused = paused;
                     }
                 }
@@ -551,7 +551,7 @@ pub(crate) async fn add_transceiver_sdp(
         let mut recv_sc_list: Vec<String> = vec![];
         let mut send_sc_list: Vec<String> = vec![];
 
-        for rid in media_section.rid_map.values() {
+        for rid in &media_section.rid_map {
             let rid_syntax = match rid.direction {
                 SimulcastDirection::Send => {
                     // If Send rid, then reply with a recv rid
@@ -749,7 +749,7 @@ pub(crate) struct MediaSection {
     pub(crate) id: String,
     pub(crate) transceivers: Vec<Arc<RTCRtpTransceiver>>,
     pub(crate) data: bool,
-    pub(crate) rid_map: HashMap<String, SimulcastRid>,
+    pub(crate) rid_map: Vec<SimulcastRid>,
     pub(crate) offered_direction: Option<RTCRtpTransceiverDirection>,
 }
 

diff --git a/webrtc/src/peer_connection/sdp/sdp_test.rs b/webrtc/src/peer_connection/sdp/sdp_test.rs
@@ -702,27 +702,20 @@ async fn test_populate_sdp() -> Result<()> {
         )
         .await;
 
-        let mut rid_map = HashMap::new();
-        let rid_id = "ridkey".to_owned();
-        rid_map.insert(
-            rid_id.to_owned(),
+        let rid_map = vec![
             SimulcastRid {
-                id: rid_id,
+                id: "ridkey".to_owned(),
                 direction: SimulcastDirection::Recv,
                 params: "some".to_owned(),
                 paused: false,
             },
-        );
-        let rid_id = "ridpaused".to_owned();
-        rid_map.insert(
-            rid_id.to_owned(),
             SimulcastRid {
-                id: rid_id,
+                id: "ridpaused".to_owned(),
                 direction: SimulcastDirection::Recv,
                 params: "some2".to_owned(),
                 paused: true,
             },
-        );
+        ];
         let media_sections = vec![MediaSection {
             id: "video".to_owned(),
             transceivers: vec![tr],
@@ -758,7 +751,7 @@ async fn test_populate_sdp() -> Result<()> {
             }
 
             let rid_map = get_rids(desc);
-            if let Some(rid) = rid_map.get("ridkey") {
+            if let Some(rid) = rid_map.iter().find(|rid| rid.id == "ridkey") {
                 assert!(!rid.paused, "Rid should be active");
                 assert_eq!(
                     rid.direction,
@@ -767,7 +760,7 @@ async fn test_populate_sdp() -> Result<()> {
                 );
                 found += 1;
             }
-            if let Some(rid) = rid_map.get("ridpaused") {
+            if let Some(rid) = rid_map.iter().find(|rid| rid.id == "ridpaused") {
                 assert!(rid.paused, "Rid should be paused");
                 assert_eq!(
                     rid.direction,
@@ -831,7 +824,7 @@ async fn test_populate_sdp() -> Result<()> {
             id: "video".to_owned(),
             transceivers: vec![tr],
             data: false,
-            rid_map: HashMap::new(),
+            rid_map: vec![],
             ..Default::default()
         }];
 
@@ -949,14 +942,14 @@ async fn test_populate_sdp_reject() -> Result<()> {
             id: "video".to_owned(),
             transceivers: vec![trv],
             data: false,
-            rid_map: HashMap::new(),
+            rid_map: vec![],
             ..Default::default()
         },
         MediaSection {
             id: "audio".to_owned(),
             transceivers: vec![tra],
             data: false,
-            rid_map: HashMap::new(),
+            rid_map: vec![],
             ..Default::default()
         },
     ];
@@ -1035,7 +1028,8 @@ fn test_get_rids() {
 
     assert!(!rids.is_empty(), "Rid mapping should be present");
 
-    assert!(rids.get("f").is_some(), "rid values should contain 'f'");
+    let f = rids.iter().find(|rid| rid.id == "f");
+    assert!(f.is_some(), "rid values should contain 'f'");
 }
 
 #[test]