Skip to content

fix: broadcast all SRT control packets to all connections#9

Open
datagutt wants to merge 1 commit intomainfrom
fix/send-control-packets-to-all-connections
Open

fix: broadcast all SRT control packets to all connections#9
datagutt wants to merge 1 commit intomainfrom
fix/send-control-packets-to-all-connections

Conversation

@datagutt
Copy link
Member

@datagutt datagutt commented Feb 5, 2026

Summary

  • Broadcasts all SRT control packets (ACKs, NAKs, ACKACK, KEEPALIVE, etc.) to all connections instead of only sending non-ACK packets to the last active connection
  • Previously, NAKs were sent only to last_address() which could be a dead connection, causing retransmission failures

Problem

When a connection dies or becomes unhealthy:

  1. last_address() might still point to the dead connection
  2. NAKs sent to that connection would be lost
  3. The sender never receives the NAK and doesn't retransmit
  4. Packet loss goes unrecovered

Solution

Broadcast all SRT control packets to all connections in the group, matching the existing behavior for ACKs. This ensures NAKs reach the sender through at least one healthy connection.

Safety

  • SRT sender already handles duplicate NAKs safely (marks packet as handled after first NAK)
  • SRT handles duplicate handshakes via state guards (m_bConnecting flag)
  • ACKs already use this broadcast pattern successfully

Summary by CodeRabbit

  • Bug Fixes
    • Control packet broadcasting now includes negative acknowledgements (NAKs) alongside ACKs so all active group connections receive critical control packets.
    • Transmission error messages standardized to "Failed to send SRT packet" for clearer diagnostics.
    • Packet distribution logic refined for more consistent handling of control packets across connection groups.

@coderabbitai
Copy link

coderabbitai bot commented Feb 5, 2026

Walkthrough

ACKs and NAKs are now treated as broadcast-worthy and sent to all connections in a group; other control packets continue to be sent to the group's last_address(). A new helper is_srt_nak was added to classify NAK packets; logging text and explanatory comments were updated.

Changes

Cohort / File(s) Summary
SRT Control Packet Handling
src/protocol/srt_handler.cpp
Broadcast logic extended so ACKs and NAKs are sent to all connections (is_srt_ack(...) or NAK). Non-ACK control packets still target group->last_address(). Updated failure log message to "Failed to send SRT packet" and added comments about ACK/NAK broadcast rationale.
SRT Packet Type Helpers
src/common.c, src/common.h
Added is_srt_nak(void *pkt, int n) declaration and implementation to detect NAK packet types, mirroring the existing ACK helper. No other public signatures changed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I sniff the bytes and hop along,

ACKs and NAKs join the hopping throng,
Sent to all, no message slips,
I tap my paw and fix the rips,
Hoppy packets, all day long! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: broadcast all SRT control packets to all connections' directly matches the PR's main objective and accurately summarizes the core change.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/send-control-packets-to-all-connections

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@datagutt datagutt force-pushed the fix/send-control-packets-to-all-connections branch from 9efff6d to a6fc01d Compare February 6, 2026 23:14
Previously, only ACKs were sent to all connections while NAKs and other
control packets were sent only to the last active connection. If that
connection was dead, NAKs would be lost and retransmissions would fail.

Now ACKs and NAKs are broadcast to all connections, matching Moblin's
approach. Other control packets still go to last_address().

Co-authored-by: servusrene <48084558+servusrene@users.noreply.github.com>
@datagutt datagutt force-pushed the fix/send-control-packets-to-all-connections branch from a6fc01d to 2655cb5 Compare February 6, 2026 23:16
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