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 support for RTCP Receiver Reports #77

Merged
merged 11 commits into from
Feb 29, 2024
Merged

Add support for RTCP Receiver Reports #77

merged 11 commits into from
Feb 29, 2024

Conversation

LVala
Copy link
Member

@LVala LVala commented Feb 23, 2024

This PR only adds RTPReceiver.ReportRecorder module, akin to PeerConnection.TWCCRecorder.
The actual functionality of sending the reports, as well as RTPSender.ReportRecorder for SenderReports, will be implemented in subsequent PRs.

@LVala LVala self-assigned this Feb 23, 2024
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Merging #77 (ee60139) into master (682c434) will increase coverage by 0.37%.
The diff coverage is 98.11%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
+ Coverage   88.43%   88.81%   +0.37%     
==========================================
  Files          29       30       +1     
  Lines        1306     1359      +53     
==========================================
+ Hits         1155     1207      +52     
- Misses        151      152       +1     
Files Coverage Δ
lib/ex_webrtc/rtp_receiver/report_recorder.ex 98.11% <98.11%> (ø)

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 682c434...ee60139. Read the comment docs.

@LVala LVala marked this pull request as ready for review February 26, 2024 12:13
@LVala LVala requested a review from mickel8 February 26, 2024 12:16
Copy link
Member

@mickel8 mickel8 left a comment

Choose a reason for hiding this comment

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

Really nice code! 👍 Clean, concise and easy to understand. Also, very nice comment in the ReportRecorder 👀

I have some questions regarding fraction_lost and total_lost 🤔

lib/ex_webrtc/rtp_receiver/report_recorder.ex Outdated Show resolved Hide resolved
lib/ex_webrtc/rtp_receiver/report_recorder.ex Outdated Show resolved Hide resolved
lib/ex_webrtc/rtp_receiver/report_recorder.ex Show resolved Hide resolved
|> MapSet.size()
|> min(@max_u24)

total_lost = min(recorder.total_lost + lost, @max_u24)
Copy link
Member

Choose a reason for hiding this comment

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

We should count in duplicate packets here?

This number is
      defined to be the number of packets expected less the number of
      packets actually received, where the number of packets received
      includes any which are late or duplicates.  Thus, packets that
      arrive late are not counted as lost, and the loss may be negative
      if there are duplicates.

Copy link
Member Author

@LVala LVala Feb 28, 2024

Choose a reason for hiding this comment

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

Here I was just following what Pion does, if we wanted to include duplicate packets, then the whole lost_packets MapSet is unnecessary, we could just count the number of packets received. To be honest, I'm not really sure why is that, I just assumed that we shouldn't count duplicate packets, but I'll look into this more.

test/ex_webrtc/rtp_receiver/report_recorder_test.exs Outdated Show resolved Hide resolved
@LVala LVala merged commit 5c0c383 into master Feb 29, 2024
4 checks passed
@LVala LVala deleted the rtcp-receiver-reports branch February 29, 2024 13:36
@LVala LVala mentioned this pull request Feb 29, 2024
54 tasks
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