Skip to content
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

Add set_sender_codec #190

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add set_sender_codec #190

wants to merge 4 commits into from

Conversation

mickel8
Copy link
Member

@mickel8 mickel8 commented Jan 30, 2025

This PR allows to set codec that should be used for sending with the following remarks:

  • when the first RTP packet is sent, the clock rate used by RTP sender is "locked" meaning that any subsequent call to the set_sender_codec must use a codec with the same clock rate. This is because using a new clock rate requires a new SSRC. Also, ReportRecorder, which generates RTCP reports relies on clock rate so changing it after we started sending packets could result in incorrect reports
  • when a codec was selected, and as a result of renegotiation it is no longer supported, user must call set_sender_codec with a new codec that is supported

It also:

  • moves initialization of ReportRecorder so that now, it is initialized when the first packet is sent
  • adds checking FMTP parameters when negotiating codecs (if codec's sdp_fmtp_line is not nil, otherwise sdp_fmtp_line is always assumed to be the same)

RFC considerations:

  • Codecs listed in the remote description are in the preference order i.e. the first codec is the most preferred to receive by the remote side, and we as a sender SHOULD use it for sending.
  • However, RFC 3264 (sec 6.1 and 7.0) allows for changing codec on the fly (e.g. during silence periods although Opus solves this problem and I have never seen anyone doing this) and receiver MUST be prepared to handle this situation
  • Relying on these requirements, we allow user to set the codec used for sending

Base automatically changed from refactor to master January 30, 2025 15:34
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 89.85507% with 7 lines in your changes missing coverage. Please review.

Project coverage is 84.88%. Comparing base (7f3b1bd) to head (61e3a04).

Files with missing lines Patch % Lines
lib/ex_webrtc/rtp_sender.ex 90.32% 3 Missing ⚠️
lib/ex_webrtc/rtp_transceiver.ex 66.66% 3 Missing ⚠️
lib/ex_webrtc/peer_connection.ex 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #190      +/-   ##
==========================================
+ Coverage   84.75%   84.88%   +0.13%     
==========================================
  Files          49       49              
  Lines        2512     2554      +42     
==========================================
+ Hits         2129     2168      +39     
- Misses        383      386       +3     
Files with missing lines Coverage Δ
lib/ex_webrtc/peer_connection/configuration.ex 95.20% <100.00%> (+0.31%) ⬆️
lib/ex_webrtc/rtp_sender/report_recorder.ex 97.05% <100.00%> (+0.08%) ⬆️
lib/ex_webrtc/peer_connection.ex 86.12% <91.66%> (+0.07%) ⬆️
lib/ex_webrtc/rtp_sender.ex 93.51% <90.32%> (+0.11%) ⬆️
lib/ex_webrtc/rtp_transceiver.ex 89.41% <66.66%> (-0.95%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f3b1bd...61e3a04. Read the comment docs.

@mickel8 mickel8 force-pushed the set_sender_codec branch 4 times, most recently from cd540df to a4f6fae Compare January 31, 2025 14:52
@mickel8 mickel8 requested a review from sgfn January 31, 2025 16:34
Comment on lines -130 to -133
report_recorder = %ReportRecorder{
sender.report_recorder
| clock_rate: codec && codec.clock_rate
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Clock rate of ReportRecorder shouldn't change as it may result in incorrect RTCP reports. Instead, we probably should generate a new ssrc and start sending new RTCP reports. Anyways, we don't allow for codec change that has different clock rate so we can safely delete this code

@mickel8 mickel8 marked this pull request as ready for review January 31, 2025 16:48
@mickel8 mickel8 removed the request for review from sgfn February 3, 2025 12:45
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.

1 participant