rx register rewrite #348

davibe · 2023-09-13T15:10:51Z

separate nack and RR generation
remove misorder delay
remove probation period
simplify handing nack to avoiding unnecessary operations

algesten

Overall looks good.

I don't like the name ReceptionRegister, because it's just some jitter calculation.
Let's not make a separate module. Rename register/mod.rs back to register.rs and merge ReceptionRegister (and rename it) into register.rs
You can have the nack in a separate. Call it something like register_nack.rs to tie them together by name.

k0nserv

Some assorted thoughts, mostly just nitpicks and potential performance improvements

src/streams/register/mod.rs

src/streams/register_nack.rs

k0nserv · 2023-09-14T10:31:23Z

src/streams/register_nack.rs

+            return None;
+        }
+
+        let mut nacks = vec![];


ReportList is designed to have a fixed size so it seems wasteful to allocate here. Instead we can use ReportList::new and then ReportList::push.

Also the result of this method gets pushed onto a &mut VecDeque<Rtcp> that is allocated a single time in Session. We could entirely avoid allocating here if we changed the signature to

pub fn nack_report(&mut self, on_nack: F) where F: FnMut(nack: Nack)

Since this is tightly coupled to the RTCP type, I don't mind making it produce the specific type directly.

So

pub fn nack_report(&mut self, on_nack: F) where F: FnMut(rtcp: Rtcp)

?

Either the way we collect feedback through the rest of handle_timeout

pub fn nack_report(&mut self, feedback: &mut VecDeque<Rtcp>) { … }

or

pub fn nack_report(&mut self) -> Option<Rtcp> { … }

Aight, I guess the former is slightly more flexible since nack_report could, in theory, produce two instances of Rtcp. But I guess we might not want to cause that much NACKing anyway

src/streams/register_nack.rs

k0nserv · 2023-09-14T10:37:25Z

src/streams/register_nack.rs

+        assert_update(&mut reg, 10, true, true, 10..10);
+
+        // packet before window start is ignored
+        assert_update(&mut reg, 9, false, false, 10..10);


Does this means we could lose out on some packets if we get unlucky with re-ordering at the start of an RTP stream? e.g. if packets 11, 10, 9 arrive in quick succession in that order we'd ignore 10 and 9. At the start of a video stream these are also likely to be part of a keyframe so their loss is particularly problematic.

This was already the case, so it's not better or worse.

Is it something we want to address long term?

Could we detect and warn! to see how much better it'd be?

@k0nserv we would not ignore packets as you wrote. The comment there is from the perspective of nacking. We would not consider them for nacking.
So 11, 10, 9 is fine (no loss). An issue would be if we receive 11, 9 at the start because we would not nack 10. However even if we received 11, 10, we would not nack 9 because .. how far back should we go without knowing ?

Ah right, this is just about nacking. Is it generally true that if the browser sends a group of packets that correspond to a frame with the sequence numbers n..=n+20 if n + 5 arrives first we will never NACK n, n + 1, n + 2, n + 3, n + 4, so if any of those are lost they are lost forever?

yes. also what i said before is not 100% accurate, because we also don't flush them out in RTP mode (based on the fact that the register does not return is_new: true).

Needs more thought.

I have made it so that the active window extends back if the packet is within capacity, or warn! otherwise

There are issues with this, I have ideas how to solve but I put it in a separate PR.

algesten

If we are merging everything together again, I want the name register.rs :)

src/streams/register_nack.rs

algesten · 2023-09-14T10:59:25Z

src/streams/register_nack.rs

+            return None;
+        }
+
+        let mut nacks = vec![];


Since this is tightly coupled to the RTCP type, I don't mind making it produce the specific type directly.

- separate nack and RR generation - remove misorder delay - remove probation period - simplify handing nack to avoiding unnecessary operations

algesten

LGTM! ✨

davibe force-pushed the feature/new-rx-register branch 4 times, most recently from 9ebbee8 to 1b61c7e Compare September 13, 2023 16:15

davibe marked this pull request as ready for review September 13, 2023 16:20

algesten reviewed Sep 13, 2023

View reviewed changes

davibe force-pushed the feature/new-rx-register branch from 1b61c7e to 5b1d8fe Compare September 14, 2023 07:02

k0nserv approved these changes Sep 14, 2023

View reviewed changes

algesten reviewed Sep 14, 2023

View reviewed changes

davibe force-pushed the feature/new-rx-register branch 2 times, most recently from 8cac017 to 8a2e6d4 Compare September 14, 2023 21:46

davibe added 2 commits September 15, 2023 00:11

rx register rewrite

c910631

- separate nack and RR generation - remove misorder delay - remove probation period - simplify handing nack to avoiding unnecessary operations

nack register: reduce memory usage significantly

8f0b568

davibe force-pushed the feature/new-rx-register branch from 8a2e6d4 to c10efc5 Compare September 14, 2023 22:12

davibe added 2 commits September 15, 2023 00:15

nack register: less allocations on nack report

59ef2c7

stats: fix over-reporting of nacks

35449b6

davibe force-pushed the feature/new-rx-register branch from c10efc5 to 35449b6 Compare September 14, 2023 22:16

davibe requested a review from algesten September 15, 2023 06:20

algesten approved these changes Sep 15, 2023

View reviewed changes

davibe merged commit 49a1898 into main Sep 15, 2023
10 checks passed

davibe deleted the feature/new-rx-register branch September 15, 2023 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rx register rewrite #348

rx register rewrite #348

davibe commented Sep 13, 2023

algesten left a comment

k0nserv left a comment

k0nserv Sep 14, 2023 •

edited

Loading

algesten Sep 14, 2023

k0nserv Sep 14, 2023

algesten Sep 14, 2023

k0nserv Sep 14, 2023

k0nserv Sep 14, 2023 •

edited

Loading

davibe Sep 14, 2023

algesten Sep 14, 2023

k0nserv Sep 14, 2023

algesten Sep 14, 2023

davibe Sep 14, 2023 •

edited

Loading

k0nserv Sep 14, 2023

davibe Sep 14, 2023 •

edited

Loading

davibe Sep 14, 2023 •

edited

Loading

davibe Sep 14, 2023

algesten left a comment

algesten Sep 14, 2023

algesten left a comment

rx register rewrite #348

rx register rewrite #348

Conversation

davibe commented Sep 13, 2023

algesten left a comment

Choose a reason for hiding this comment

k0nserv left a comment

Choose a reason for hiding this comment

k0nserv Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k0nserv Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davibe Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davibe Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

davibe Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algesten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algesten left a comment

Choose a reason for hiding this comment

k0nserv Sep 14, 2023 •

edited

Loading

k0nserv Sep 14, 2023 •

edited

Loading

davibe Sep 14, 2023 •

edited

Loading

davibe Sep 14, 2023 •

edited

Loading

davibe Sep 14, 2023 •

edited

Loading