Skip to content

fix: harden NACK send/receive (bounded index, resend cooldown, recovery)#33

Open
langhuihui wants to merge 1 commit into
restsend:mainfrom
langhuihui:feat/nack-hardening
Open

fix: harden NACK send/receive (bounded index, resend cooldown, recovery)#33
langhuihui wants to merge 1 commit into
restsend:mainfrom
langhuihui:feat/nack-hardening

Conversation

@langhuihui

@langhuihui langhuihui commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Motivation

NACK recovery worked but had several sharp edges under lossy browser WHEP:

  1. Sender buffered packets in a VecDeque and scanned linearly on every NACK seq.
  2. Duplicate / retried NACK reports could burst-retransmit the same primary seq.
  3. Receiver gap detection could emit up to ~32k lost sequence numbers in one report.
  4. get_recovered_count() counted any late packet, not only ones we had NACKed.

This PR hardens those paths without changing the A2 RTX wrap/unwrap behavior (already on main via #32 / 0.3.92). Rebased onto current main so it keeps the lock-free rtx_ssrc_fast hot path.

Design

Sender (DefaultRtpSenderNackHandler)

  • Replace queue-only storage with NackSendBuffer: FIFO order + HashMap<u16, RtpPacket>
    bounded by nack_buffer_size / builder nack(buffer_size).
  • packets_for_nack(seqs, now) applies:
    • O(1) lookup
    • in-report dedup
    • 25ms per-seq cooldown (NACK_RESEND_COOLDOWN)
  • RTX path unchanged: selected primary packets are still wrapped via wrap_rtx_packet
    when set_rtx(Some(...)) is active; otherwise clone-resend. Still uses rtx_config +
    rtx_ssrc_fast from 0.3.92.

Receiver (DefaultRtpReceiverNackHandler)

  • Cap generated NACK lists at MAX_RECEIVER_NACK_GAP (128), keeping the most recent
    missing sequences when the gap is larger.
  • Track pending_nacks; only those seqs increment nack_recovered_count when they arrive.
  • Clear pending on SSRC change.

Behavior when RTX is not negotiated

Unchanged aside from the hardening above: still clone-resends original RTP from the
bounded buffer.

Test coverage

  • Sender buffer stays ≤ max_size; evicted seqs are not retransmittable
  • Duplicate NACK seq within cooldown / same report is suppressed; allowed again after cooldown
  • Receiver huge-gap NACK list length == 128 and covers the newest holes
  • Recovery count only increases for previously NACKed seqs
  • Existing NACK + RTX unit tests remain green (cargo test --lib -- nack_ → 10; -- rtx → 24)

Open questions for maintainers

  1. Is 25ms cooldown acceptable as a fixed constant, or should it be RTT-based / configurable?

Limitations / follow-ups

  • Cooldown is wall-clock (std::time::Instant), not RTT-adaptive.
  • Pending-set eviction under extreme loss is best-effort (does not re-NACK older holes).
  • No ULPFEC/RED (future).

Bound the NACK send buffer with a seq index, suppress duplicate resends
within 25ms, cap receiver NACK gaps at 128 newest holes, and only count
previously NACKed seqs as recovered. RTX wrap path remains unchanged.

Co-authored-by: Cursor <cursoragent@cursor.com>
@langhuihui
langhuihui force-pushed the feat/nack-hardening branch from 2ee7961 to 9551184 Compare July 20, 2026 02:01
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.

1 participant