Add allocation-free API for routing RTCP - #223
Open
kcaffrey wants to merge 3 commits into
Open
Conversation
In order to route RTCP packets, a caller must currently perform a full unmarshal to split a compound packet into individual feedbacks and then call DestinationSSRC on each parsed feedback. Many feedback messages, however, allocate in their Unmarshal methods. For example, TWCC allocates once per recv delta, or in other words, once for every packet sent to the remote. To avoid these allocations that scale per packet, a new UnmarshalRaw method is introduced to split a payload into a slice of unparsed RawPackets. RawPacket also gains a ParseDestinationSSRC method that does a wire parse similar to Unmarshal, but only pulls out SSRCs (matching the behavior of DestinationSSRC on the full unmarshalled feedback). Unlike Unmarshal, UnmarshalRaw (and the append variant) only validates the framing of a compound packet, in line with the recommendations in RFC 3550 section 6.1 and appendix A.2. The intended usage for this API is in the srtp package, where we currently unmarshal and remarshal each feedback. This will be replaced by an UnmarshalRaw + ParseDestinationSSRC. There will be a minor behavior difference in srtp using this new API on account of only validating framing rather than full packet contents, which is that previously one bad packet in a compound could prevent the entire batch from being routed. Now, as long as the framing is valid according to RFC 3550 (amended by RFC 5506 for reduced size packets), the packets will be forwarded as-is with no modification (as long as they can be parsed sufficiently to get the destination SSRC). I don't think this behavior change will be breaking. A test ensures that UnmarshalRaw + ParseDestinationSSRC exactly matches the behavior of Unmarshal + DestinationSSRC for every feedback type successfully parsed by Unmarshal, as well as a second test ensuring that the test covers every packet type that Unmarshal can handle. A fuzz test adds some extra safety on top. Allocations are pinned to zero with a third test.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
+ Coverage 77.50% 81.01% +3.50%
==========================================
Files 22 22
Lines 2032 2239 +207
==========================================
+ Hits 1575 1814 +239
+ Misses 359 319 -40
- Partials 98 106 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces an allocation-free routing path for RTCP by splitting compound RTCP datagrams into raw packet views and providing a lightweight destination-SSRC parser that avoids full unmarshaling (and its associated per-packet allocations).
Changes:
- Added
UnmarshalRaw/AppendRawPacketsto split RTCP datagrams into[]RawPacketwithout parsing packet bodies. - Added
RawPacket.ParseDestinationSSRCplus per-packet-type helpers to wire-parse only the SSRCs needed for routing. - Added comprehensive corpus, coverage, fuzz, and allocation tests to ensure behavior matches
Unmarshal+DestinationSSRCand remains allocation-free.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| source_description.go | Adds SDES SSRC extraction helper for allocation-free destination parsing. |
| sender_report.go | Adds SR SSRC extraction helper (report-block SSRCs + sender SSRC). |
| rfc8888.go | Adds allocation-free destination SSRC parsing for RFC8888 CCFB report blocks. |
| receiver_report.go | Adds RR SSRC extraction helper (report-block SSRCs). |
| receiver_estimated_maximum_bitrate.go | Adds REMB SSRC extraction helper for allocation-free destination parsing. |
| raw_packet.go | Adds UnmarshalRaw/AppendRawPackets and RawPacket.ParseDestinationSSRC routing API. |
| raw_packet_test.go | Adds corpus/coverage/fuzz/allocation tests validating the new routing API. |
| goodbye.go | Adds BYE SSRC extraction helper. |
| full_intra_request.go | Adds FIR SSRC extraction helper. |
| extended_report.go | Adds XR SSRC extraction helper and supporting constants. |
| application_defined.go | Adds APP SSRC extraction helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Make sure various malformed packets return an error from ParseDestinationSSRC
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to route RTCP packets, a caller must currently perform a full unmarshal to split a compound packet into individual feedbacks and then call DestinationSSRC on each parsed feedback. Many feedback messages, however, allocate in their Unmarshal methods. For example, TWCC allocates once per recv delta, or in other words, once for every packet sent to the remote.
To avoid these allocations that scale per packet, a new UnmarshalRaw method is introduced to split a payload into a slice of unparsed RawPackets. RawPacket also gains a ParseDestinationSSRC method that does a wire parse similar to Unmarshal, but only pulls out SSRCs (matching the behavior of DestinationSSRC on the full unmarshalled feedback). Unlike Unmarshal, UnmarshalRaw (and the append variant) only validates the framing of a compound packet, in line with the recommendations in RFC 3550 section 6.1 and appendix A.2.
The intended usage for this API is in the srtp package, where we currently unmarshal and remarshal each feedback. This will be replaced by an UnmarshalRaw + ParseDestinationSSRC. There will be a minor behavior difference in srtp using this new API on account of only validating framing rather than full packet contents, which is that previously one bad packet in a compound could prevent the entire batch from being routed. Now, as long as the framing is valid according to RFC 3550 (amended by RFC 5506 for reduced size packets), the packets will be forwarded as-is with no modification (as long as they can be parsed sufficiently to get the destination SSRC). I don't think this behavior change will be breaking.
A test ensures that UnmarshalRaw + ParseDestinationSSRC exactly matches the behavior of Unmarshal + DestinationSSRC for every feedback type successfully parsed by Unmarshal, as well as a second test ensuring that the test covers every packet type that Unmarshal can handle. A fuzz test adds some extra safety on top. Allocations are pinned to zero with a third test.