Skip to content

Add RTP Stats to stats report #225

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 6 commits into from
Aug 26, 2022
Merged

Add RTP Stats to stats report #225

merged 6 commits into from
Aug 26, 2022

Conversation

k0nserv
Copy link
Member

@k0nserv k0nserv commented Jul 20, 2022

This PR implements inbound and outbound RTP stats according to the
specification, with some
limitations.

Todo

  • Inbound RTP
  • Outbound RTP
  • Tests

@k0nserv k0nserv force-pushed the feature/add-rtp-stats branch from 4dfca1b to a18ab7d Compare July 20, 2022 13:37
@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #225 (ec4ac28) into master (5614253) will increase coverage by 0.22%.
The diff coverage is 67.92%.

@@            Coverage Diff             @@
##           master     #225      +/-   ##
==========================================
+ Coverage   56.05%   56.27%   +0.22%     
==========================================
  Files         500      500              
  Lines       46523    47034     +511     
  Branches    12714    12779      +65     
==========================================
+ Hits        26077    26470     +393     
- Misses       9920     9935      +15     
- Partials    10526    10629     +103     
Impacted Files Coverage Δ
interceptor/src/report/receiver/mod.rs 100.00% <ø> (ø)
webrtc/src/stats/mod.rs 43.47% <0.00%> (-11.80%) ⬇️
webrtc/src/peer_connection/peer_connection_test.rs 56.12% <57.14%> (+1.53%) ⬆️
interceptor/src/stats/interceptor.rs 74.38% <73.05%> (+3.74%) ⬆️
...tc/src/peer_connection/peer_connection_internal.rs 51.29% <79.16%> (+3.73%) ⬆️
interceptor/src/stats/mod.rs 81.18% <81.31%> (+11.42%) ⬆️
.../src/rtp_transceiver/rtp_sender/rtp_sender_test.rs 51.79% <83.33%> (+4.17%) ⬆️
...c/src/track/track_local/track_local_static_test.rs 54.42% <83.33%> (+1.03%) ⬆️
.../transport_layer_nack/transport_layer_nack_test.rs 82.00% <100.00%> (+0.75%) ⬆️
dtls/src/curve/named_curve.rs 72.50% <0.00%> (-15.38%) ⬇️
... and 15 more

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

@k0nserv k0nserv mentioned this pull request Aug 22, 2022
5 tasks
@k0nserv k0nserv force-pushed the feature/add-rtp-stats branch 3 times, most recently from 2bcc730 to e6fe473 Compare August 23, 2022 14:22
@k0nserv k0nserv changed the title Add Inbound RTP Stats to stats report Add RTP Stats to stats report Aug 24, 2022
@k0nserv k0nserv force-pushed the feature/add-rtp-stats branch from 7a5a054 to d94e5ca Compare August 24, 2022 15:47
Some(StatsReportType::Transport(ice_transport_stats)) => {
assert!(ice_transport_stats.bytes_received > 0);
assert!(ice_transport_stats.bytes_sent > 0);
}
Some(_other) => panic!("found the wrong type"),
None => panic!("missed it"),
}
let outbound_stats = offer_stats
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll explore extending this test to also have some RTCP components to it

@k0nserv
Copy link
Member Author

k0nserv commented Aug 24, 2022

Will tackle Remote{Inbound,Outbound}RTP in another PR

@k0nserv k0nserv force-pushed the feature/add-rtp-stats branch from d94e5ca to 7839bbb Compare August 24, 2022 17:08
Copy link
Contributor

@lolgesten lolgesten left a comment

Choose a reason for hiding this comment

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

Some questions


while self.bitfield != 0 {
if (self.bitfield & (1 << i)) != 0 {
self.bitfield &= !(1 << i);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks strange. If we have this:

packet_id: 123
bitfield: `00000000 00000101`

We expect the iterator to produce 123, 124, and 126.

But it looks like self.bitfield is "destroyed" on the first iteration which means the result would be 123, 124 – and missing the last value.

Copy link
Member Author

Choose a reason for hiding this comment

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

This specific line zeroes out the bit in question which ensure we eventually hit the loop condition.

Keep in mind that we are within a next method here, as such we only run until we can produce a value, subsequent calls to next should produce another value if we do indeed have more values to produce.

I'll write a test with a gap like you pointed out to verify

Copy link
Member Author

@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.

Thanks for the review!


while self.bitfield != 0 {
if (self.bitfield & (1 << i)) != 0 {
self.bitfield &= !(1 << i);
Copy link
Member Author

Choose a reason for hiding this comment

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

This specific line zeroes out the bit in question which ensure we eventually hit the loop condition.

Keep in mind that we are within a next method here, as such we only run until we can produce a value, subsequent calls to next should produce another value if we do indeed have more values to produce.

I'll write a test with a gap like you pointed out to verify

Copy link
Member

@algesten algesten left a comment

Choose a reason for hiding this comment

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

I prefer the alternative impl for NackIter, but other than that. Good to go!

@k0nserv k0nserv merged commit ce16403 into master Aug 26, 2022
@k0nserv k0nserv deleted the feature/add-rtp-stats branch August 26, 2022 13:30
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.

5 participants