Skip to content

Split RTCP compound packet before forwarding#359

Merged
Sean-Der merged 1 commit into
mainfrom
raja_srtp_compound
Jul 7, 2026
Merged

Split RTCP compound packet before forwarding#359
Sean-Der merged 1 commit into
mainfrom
raja_srtp_compound

Conversation

@boks1971

@boks1971 boks1971 commented Dec 29, 2025

Copy link
Copy Markdown
Contributor
Split RTCP compound packet before forwarding

When a compound RTCP packet is received, it is forwarded as is to the
read streams. Downstream components have to check for the SSRC in the
packets matching the track(s) it is handling.

This leads to situations where downstream components could see duplicate
RTCP reports. Happens when there is a downstream handler for all SSRCs.
It receives the compound packet, unmarshals it and invokes the handlers.
As it is fielding all SSRCs, it will get the same compound packet `n`
times and invoke the handlers `n` times.

This PR splits up the compound packet and forwards individual packets to
avoid the end handlers from seeing duplicates. API wise, it is
compatible as it still emits an encoded/marshaled packet. But, this does
add a marshaling step.

Packets that have no destination SSRC of their own (e.g. unknown packet
types parsed as rtcp.RawPacket, or receiver reports with no reception
reports) are delivered to every stream the compound packet is addressed
to, instead of being dropped. Such packets can still be seen once per
compound packet by each of those streams, matching the old behavior.

Add unit tests. Before this change, the compound packet test fails at
the point where the test tries to read the CNAME packet and sees an
incorrect type, and the empty destination SSRC test blocks forever
waiting for the rtcp.RawPacket to be forwarded.

@boks1971 boks1971 requested review from JoTurk and Sean-Der December 29, 2025 04:25
@codecov

codecov Bot commented Dec 29, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.62%. Comparing base (1cd193a) to head (4042833).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
+ Coverage   82.54%   83.62%   +1.08%     
==========================================
  Files          19       19              
  Lines        1472     1484      +12     
==========================================
+ Hits         1215     1241      +26     
+ Misses        142      135       -7     
+ Partials      115      108       -7     
Flag Coverage Δ
go 83.62% <100.00%> (+1.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@boks1971 boks1971 force-pushed the raja_srtp_compound branch 7 times, most recently from 6faec8e to 40818b0 Compare December 29, 2025 06:07
@boks1971 boks1971 requested a review from Juliapixel December 29, 2025 06:09
@Sean-Der Sean-Der force-pushed the raja_srtp_compound branch 4 times, most recently from 62eb429 to ee8f199 Compare July 7, 2026 17:29
Sean-Der
Sean-Der previously approved these changes Jul 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR changes SRTCP inbound RTCP handling to split compound RTCP packets and forward the individual RTCP packets to the appropriate read streams, reducing downstream duplication when handlers subscribe broadly.

Changes:

  • Update RTCP decrypt/forwarding logic to Unmarshal compound RTCP, then re-Marshal and forward each contained packet separately.
  • Add routing behavior for packets with empty DestinationSSRC() (e.g., rtcp.RawPacket) to be forwarded to the compound packet’s addressed SSRC set.
  • Add unit tests covering compound packet splitting, empty-destination forwarding, and decrypt error paths.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
session_srtcp.go Splits decrypted compound RTCP into individual packets and forwards them per-destination (with special handling for empty-destination packets).
session_srtcp_test.go Adds tests validating per-packet forwarding behavior and error handling scenarios introduced by the new remarshal step.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread session_srtcp.go Outdated
Comment thread session_srtcp_test.go
Comment thread session_srtcp.go
When a compound RTCP packet is received, it is forwarded as is to the
read streams. Downstream components have to check for the SSRC in the
packets matching the track(s) it is handling.

This leads to situations where downstream components could see duplicate
RTCP reports. Happens when there is a downstream handler for all SSRCs.
It receives the compound packet, unmarshals it and invokes the handlers.
As it is fielding all SSRCs, it will get the same compound packet `n`
times and invoke the handlers `n` times.

This PR splits up the compound packet and forwards individual packets to
avoid the end handlers from seeing duplicates. API wise, it is
compatible as it still emits an encoded/marshaled packet. But, this does
add a marshaling step.

Packets that have no destination SSRC of their own (e.g. unknown packet
types parsed as rtcp.RawPacket, or receiver reports with no reception
reports) are delivered to every stream the compound packet is addressed
to, instead of being dropped. Such packets can still be seen once per
compound packet by each of those streams, matching the old behavior.

Add unit tests. Before this change, the compound packet test fails at
the point where the test tries to read the CNAME packet and sees an
incorrect type, and the empty destination SSRC test blocks forever
waiting for the rtcp.RawPacket to be forwarded.
@Sean-Der Sean-Der force-pushed the raja_srtp_compound branch from 28a21ff to 4042833 Compare July 7, 2026 19:46
@Sean-Der Sean-Der self-requested a review July 7, 2026 19:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@Sean-Der Sean-Der merged commit 5f45b20 into main Jul 7, 2026
20 checks passed
@Sean-Der Sean-Der deleted the raja_srtp_compound branch July 7, 2026 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants