Skip to content

Make RTCRtpTransceiver.mid spec-compliant #375

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jan 4, 2023

Conversation

rogurotus
Copy link
Contributor

The current implementation is:

pub struct RTCRtpTransceiver {
    mid: Mutex<String>,                           //atomic.Value
    sender: Mutex<Option<Arc<RTCRtpSender>>>,     //atomic.Value
    receiver: Mutex<Option<Arc<RTCRtpReceiver>>>, //atomic.Value
    ...
}

Essentially, the empty string is used instead of None (Option<String>).
This PR makes the behavior of mid explicitly optional, and also replaces Mutex with OnceCell.

@rogurotus rogurotus changed the title Make RTCRtpTransceiver.mid more transparent Make RTCRtpTransceiver.mid spec-compliant Dec 21, 2022
@rogurotus
Copy link
Contributor Author

ping @k0nserv

@codecov
Copy link

codecov bot commented Dec 26, 2022

Codecov Report

Base: 60.14% // Head: 61.12% // Increases project coverage by +0.98% 🎉

Coverage data is based on head (8d2edb9) compared to base (6506f2c).
Patch coverage: 48.14% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   60.14%   61.12%   +0.98%     
==========================================
  Files         503      537      +34     
  Lines       47987    49620    +1633     
  Branches    12501    12590      +89     
==========================================
+ Hits        28860    30330    +1470     
- Misses       9911     9968      +57     
- Partials     9216     9322     +106     
Impacted Files Coverage Δ
webrtc/src/rtp_transceiver/rtp_transceiver_test.rs 53.27% <0.00%> (+0.46%) ⬆️
...tc/src/peer_connection/peer_connection_internal.rs 57.43% <50.00%> (+0.54%) ⬆️
sctp/src/queue/pending_queue.rs 33.33% <0.00%> (-55.24%) ⬇️
sctp/src/queue/payload_queue.rs 76.31% <0.00%> (-9.40%) ⬇️
sctp/src/queue/queue_test.rs 65.12% <0.00%> (-3.59%) ⬇️
webrtc/src/mux/mux_func.rs 75.00% <0.00%> (-3.58%) ⬇️
rtcp/src/header.rs 63.93% <0.00%> (-3.28%) ⬇️
sdp/src/lexer/mod.rs 66.66% <0.00%> (-3.04%) ⬇️
data/src/error.rs 17.94% <0.00%> (-2.64%) ⬇️
data/src/message/message_channel_open.rs 79.51% <0.00%> (-2.41%) ⬇️
... and 100 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@k0nserv k0nserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent change 🎉

Just a few comments

@rogurotus rogurotus requested a review from k0nserv January 4, 2023 14:00
@k0nserv
Copy link
Member

k0nserv commented Jan 4, 2023

@rogurotus I'm sorry, I'm a doofus, we can't use let-else as I suggested because of our MSRV policy 🤦🏼

Instead of it can you use match as

let x = match y {
   //Positive match yields value,
   _ => continue; // or break/return etc
}

Copy link
Member

@k0nserv k0nserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@k0nserv k0nserv merged commit 3be403f into webrtc-rs:master Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants