Add allocation-free API for routing RTCP - #223
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
+ Coverage 81.31% 84.62% +3.30%
==========================================
Files 22 22
Lines 1488 1665 +177
==========================================
+ Hits 1210 1409 +199
+ Misses 278 256 -22
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.
|
@kcaffrey Should this be the default API? I also wanted to evaluate if we could improve RTP performance with a new API that is allocation free. Things like getting SSRC shouldn't require copying/parsing |
@Sean-Der I think in many contexts this API should be the default, as parsing often doesn't need to materialize a full |
JoTurk
left a comment
JoTurk
left a comment
There was a problem hiding this comment.
The code looks good, and i don't see any issue. Other than the duplicated API comment, +1 for switching to this api.
@JoTurk I can't spot the duplicated comment you are referring to, mind pointing it out for me so I can fix it up? Thanks! |
|
@kcaffrey sorry I'm not sure why my review comment got lost, but I was commenting about some duplicated implementations, now we have a duplicated implementation for many RTCP format in umarshal and partly in the new APIs, maybe we can de-duplicate code to make it easy to maintain and test? Also related to Sean comment maybe we can rework this library APIs and tag a new major release? |
Ah yes, I didn't particularly like the duplication either. However, I couldn't really come up with a clean way to share code without introducing complexity or possible perf hits, so I added tests (
Would you want that in lieu of this PR or as a follow-on effort? I'll admit I don't have a great handle on what a good end state for a reworked API should be; I was mostly focused on the use case of "validate and extract ssrc". I imagine there are several other use cases that would need to be considered for a full rework? |
|
@JoTurk @Sean-Der I gave some thought to trying to rework the library API, but anything reasonable I could come up with I think would need a version bump of the pion/webrtc library itself, which doesn't seem great. Is there anything I can do to get this PR into a mergeable state mostly as-is with the current shape of the API? This (and the corresponding usage in srtp) is the last remaining per-packet allocation I am aware of outside interceptors, so getting this merged (and following up with the srtp change) would allow me to add a test to pion/webrtc to pin per-packet allocations to zero (when not using interceptors). This would be great to prevent performance regressions. Thanks! |
|
sounds good from my side |
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.
Make sure various malformed packets return an error from ParseDestinationSSRC
0abdf9b to
00fb36e
Compare
|
This is rebased and good to go from my perspective as soon as it gets a review approval. Thanks! |
JoTurk
left a comment
JoTurk
left a comment
There was a problem hiding this comment.
Please wait a bit for sean to response before merging :)
Previously, receiving an RTCP packet would allocate in order to unmarshal and remarshal the packet and collect the destination SSRCs. The unmarshal and remarshal was needed to properly handle compound packets (see pion#359), and was performed even for non-compound packets. This change uses the new API provided by pion/rtcp#223 to both split compound packets and collect destination SSRCs without allocations (by reusing slices, which is safe as decrypt is only called from a single goroutine). There is a small intentional behavior change regarding invalid packets: previously these were skipped due to failures in either Unmarshal or Marshal, but now they are forwarded to the receiver so long as the compound framing is valid. This is consistent with RFC 3550 and RFC 5506 (see pion/rtcp#223 for more information on this). A new test pins round-trip allocations to zero for RTCP, matching the existing test for RTP. This test also uncovered an allocation for GCM due to the AAD byte array escaping when sliced and passed as a parameter to another function, which is also fixed in this PR. NOTE: Authentication-only GCM still allocates, but is left as-is as it isn't intended for production use anyway.
Previously, receiving an RTCP packet would allocate in order to unmarshal and remarshal the packet and collect the destination SSRCs. The unmarshal and remarshal was needed to properly handle compound packets (see #359), and was performed even for non-compound packets. This change uses the new API provided by pion/rtcp#223 to both split compound packets and collect destination SSRCs without allocations (by reusing slices, which is safe as decrypt is only called from a single goroutine). There is a small intentional behavior change regarding invalid packets: previously these were skipped due to failures in either Unmarshal or Marshal, but now they are forwarded to the receiver so long as the compound framing is valid. This is consistent with RFC 3550 and RFC 5506 (see pion/rtcp#223 for more information on this). A new test pins round-trip allocations to zero for RTCP, matching the existing test for RTP. This test also uncovered an allocation for GCM due to the AAD byte array escaping when sliced and passed as a parameter to another function, which is also fixed in this PR. NOTE: Authentication-only GCM still allocates, but is left as-is as it isn't intended for production use anyway.
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.