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

Fix RTX seq num ending up in main receive register #272

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

k0nserv
Copy link
Collaborator

@k0nserv k0nserv commented Aug 9, 2023

When we received blank padding packets on the RTX stream those would end up getting
registered in the receive register for the main stream. This would in
turn cause us to NACK packets that never existed in the main stream.

The relevant code looked like this:

let receipt = if is_repair {
    // Blank padding packets will have empty data.
    if !data.is_empty() { // 1
        // Rewrite the header, and removes the resent seq_no from the body.
        stream.un_rtx(&mut header, &mut data, pt);
    }

    // Now update the "main" register with the repaired packet info.
    // This gives us the extended sequence number of the main stream.
    //
    // We need to do this also for blank padding to schedule the "paused event".
    let receipt = stream.update(now, &header, clock_rate, false); // 2

    // Drop blank padding
    if data.is_empty() {
        return;
    }

    receipt
} else {
  // omitted
}

A blank padding packet with seq no X would skip the !data.is_empty()(1) branch and thus not have the sequence
number in the header rewritten. Then we would call
StreamRx::update with the unmodified sequencer number X, that of the RTX
stream, and it would be registered as received in the ReceiveRegister for the main stream.

@k0nserv
Copy link
Collaborator Author

k0nserv commented Aug 9, 2023

Looking at this a different solve strikes me:

diff --git a/src/session.rs b/src/session.rs
index 30a2fde..6774229 100644
--- a/src/session.rs
+++ b/src/session.rs
@@ -495,7 +495,7 @@ impl Session {
             // This gives us the extended sequence number of the main stream.
             //
             // We need to do this also for blank padding to schedule the "paused event".
-            let receipt = stream.update(now, &header, clock_rate, false);
+            let receipt = stream.update(now, &header, clock_rate, data.is_empty());
 
             // Drop blank padding
             if data.is_empty() {

However, this means we'll register the same SeqNo twice for each blank RTX packet, but I guess that's fine 🤔

@algesten algesten force-pushed the fix/fix-incorrect-seq-number-in-receive-register branch from 0298019 to 2444128 Compare August 9, 2023 18:29
@algesten algesten merged commit 2deae48 into main Aug 9, 2023
10 checks passed
@algesten algesten deleted the fix/fix-incorrect-seq-number-in-receive-register branch August 9, 2023 19:18
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