-
-
Notifications
You must be signed in to change notification settings - Fork 411
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
Conversation
4dfca1b
to
a18ab7d
Compare
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
2bcc730
to
e6fe473
Compare
7a5a054
to
d94e5ca
Compare
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 |
There was a problem hiding this comment.
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
Will tackle Remote{Inbound,Outbound}RTP in another PR |
d94e5ca
to
7839bbb
Compare
There was a problem hiding this 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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); |
There was a problem hiding this comment.
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
There was a problem hiding this 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!
This PR implements inbound and outbound RTP stats according to the
specification, with some
limitations.
Todo