#226 Update to allow for use of rtcp-mux where RTP is multiplexed with RTCP#227
#226 Update to allow for use of rtcp-mux where RTP is multiplexed with RTCP#227hamptonng wants to merge 6 commits intosipcapture:masterfrom
Conversation
|
Neil Hampton seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Not sure why, but sometimes when I run this after this change I get the following error after an RTCP message is detected: 2022/04/08 14:30:26.608643 hep.go:92: ERR write udp 10.202.118.101:42832->10.202.64.185:9060: write: message too long This doesn't happen on every run of heplify, but when it happens it is for every RTCP message and doesn't stop until I restart heplify. |
|
@kYroL01 can we merge ? |
|
@adubovikov not yet because I spotted a little bug and I need to test better. The PR is correct for that case but for default not, so I'm reverting a change that I think create the problem, do another full test and then we can merge |
There was a problem hiding this comment.
Pull request overview
Updates SIP/SDP correlation and UDP RTCP detection to support rtcp-mux scenarios where RTCP may arrive on the RTP port.
Changes:
- Detect RTCP packets without relying on odd/even port heuristics.
- Parse
a=rtcp-muxfrom SDP and attempt to cache Call-ID correlation on the RTP port as well. - Minor Makefile whitespace/formatting cleanup.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| decoder/decoder.go | Updates UDP RTCP detection logic to work when RTCP shares the RTP port. |
| decoder/correlator.go | Adds SDP parsing for a=rtcp-mux and caches CID mappings intended for muxed RTCP. |
| Makefile | Whitespace/formatting fix in clean target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // If rtcp-mux is in use add an entry for the rtp port | ||
| if rtcpMux && len(rtcpIP)>0 && len(rtpPort)>0 && string(rtpPort)!=string(rtcpPort) { | ||
| cacheCID(srcIPb, rtcpIP, rtpPort, callID) | ||
| } |
There was a problem hiding this comment.
rtpPort used in the rtcp-mux caching block is never populated: in the case 'm' branch the code declares a new local rtpPort := ... (shadowing the outer variable). As a result, len(rtpPort) > 0 here will always be false and the rtcp-mux mapping won’t be added. Fix by assigning to the outer rtpPort (and resetting it appropriately when starting a new media section), rather than using := inside the m= parsing.
| // If rtcp-mux is indicated, RTCP can appear on the RTP port | ||
| if bytes.HasPrefix(line, []byte("a=rtcp-mux")) { | ||
| rtcpMux = true | ||
| continue sdpLoop | ||
| } |
There was a problem hiding this comment.
rtcpMux is treated as a single boolean for the whole SDP parse and is never reset when a new m= section begins. If the SDP contains multiple media sections, a single a=rtcp-mux line can incorrectly affect later sections. Consider scoping the flag to the current media section (reset on each m=) and only applying it for the audio media you’re extracting ports from.
| // If rtcp-mux is indicated, RTCP can appear on the RTP port | ||
| if bytes.HasPrefix(line, []byte("a=rtcp-mux")) { | ||
| rtcpMux = true | ||
| continue sdpLoop | ||
| } |
There was a problem hiding this comment.
The new a=rtcp-mux handling introduces new correlation behavior (cache RTCP CID on the RTP port), but there’s no test coverage ensuring it works end-to-end. Please add/extend a decoder test to include a=rtcp-mux in the SDP and then send an RTCP packet on the RTP port to verify the CID is cached and correlation succeeds.
| if (udp.Payload[0]&0xc0)>>6 == 2 && | ||
| (udp.Payload[1] == 200 || udp.Payload[1] == 201 || udp.Payload[1] == 207) { | ||
| pkt.Payload, pkt.CID = correlateRTCP(pkt.SrcIP, pkt.SrcPort, pkt.DstIP, pkt.DstPort, udp.Payload) | ||
| if pkt.Payload != nil { | ||
| pkt.ProtoType = 5 | ||
| atomic.AddUint64(&d.rtcpCount, 1) | ||
| PacketQueue <- pkt | ||
| return | ||
| } | ||
| atomic.AddUint64(&d.rtcpFailCount, 1) | ||
| return |
There was a problem hiding this comment.
Dropping the port-parity check means this RTCP gate can now match valid RTP packets (e.g., marker=1 and payload-type=72 yields a second byte of 200). Because ParseRTCP can still return non-nil JSON even when it flags a fishy header, this can lead to misclassifying RTP as RTCP and polluting the RTCP correlation cache. Tighten the RTCP detection before calling correlateRTCP (e.g., require len(payload) >= 28, accept the full RTCP PT range 200–207, and validate the RTCP length field fits within the UDP payload) so non-RTCP packets don’t enter the RTCP correlation path.
| if (udp.Payload[0]&0xc0)>>6 == 2 && | |
| (udp.Payload[1] == 200 || udp.Payload[1] == 201 || udp.Payload[1] == 207) { | |
| pkt.Payload, pkt.CID = correlateRTCP(pkt.SrcIP, pkt.SrcPort, pkt.DstIP, pkt.DstPort, udp.Payload) | |
| if pkt.Payload != nil { | |
| pkt.ProtoType = 5 | |
| atomic.AddUint64(&d.rtcpCount, 1) | |
| PacketQueue <- pkt | |
| return | |
| } | |
| atomic.AddUint64(&d.rtcpFailCount, 1) | |
| return | |
| // RTCP detection gate: ensure payload is large enough, has RTCP v2, | |
| // uses an RTCP payload type (200–207), and that the RTCP length | |
| // field describes a packet fully contained in the UDP payload. | |
| if len(udp.Payload) >= 28 { | |
| if (udp.Payload[0]&0xc0)>>6 == 2 { | |
| pt := udp.Payload[1] | |
| if pt >= 200 && pt <= 207 && len(udp.Payload) >= 4 { | |
| rtcpLenField := int(udp.Payload[2])<<8 | int(udp.Payload[3]) | |
| rtcpTotalLen := (rtcpLenField + 1) * 4 | |
| if rtcpTotalLen <= len(udp.Payload) { | |
| pkt.Payload, pkt.CID = correlateRTCP(pkt.SrcIP, pkt.SrcPort, pkt.DstIP, pkt.DstPort, udp.Payload) | |
| if pkt.Payload != nil { | |
| pkt.ProtoType = 5 | |
| atomic.AddUint64(&d.rtcpCount, 1) | |
| PacketQueue <- pkt | |
| return | |
| } | |
| atomic.AddUint64(&d.rtcpFailCount, 1) | |
| return | |
| } | |
| } | |
| } |
| if (udp.Payload[0]&0xc0)>>6 == 2 && | ||
| (udp.Payload[1] == 200 || udp.Payload[1] == 201 || udp.Payload[1] == 207) { | ||
| pkt.Payload, pkt.CID = correlateRTCP(pkt.SrcIP, pkt.SrcPort, pkt.DstIP, pkt.DstPort, udp.Payload) | ||
| if pkt.Payload != nil { | ||
| pkt.ProtoType = 5 | ||
| atomic.AddUint64(&d.rtcpCount, 1) | ||
| PacketQueue <- pkt | ||
| return | ||
| } | ||
| atomic.AddUint64(&d.rtcpFailCount, 1) | ||
| return | ||
| } |
There was a problem hiding this comment.
With the previous even-port RTP fast-path removed, RTP packets that aren’t detected as RTCP will now fall through and be counted as unknown (and still incur extractCID / SIP heuristics work). If the intent is still to ignore RTP payloads, consider adding an early return for “definitely not RTCP” packets after the RTCP check (so RTP traffic doesn’t inflate unknownCount / CPU).
| if (udp.Payload[0]&0xc0)>>6 == 2 && | |
| (udp.Payload[1] == 200 || udp.Payload[1] == 201 || udp.Payload[1] == 207) { | |
| pkt.Payload, pkt.CID = correlateRTCP(pkt.SrcIP, pkt.SrcPort, pkt.DstIP, pkt.DstPort, udp.Payload) | |
| if pkt.Payload != nil { | |
| pkt.ProtoType = 5 | |
| atomic.AddUint64(&d.rtcpCount, 1) | |
| PacketQueue <- pkt | |
| return | |
| } | |
| atomic.AddUint64(&d.rtcpFailCount, 1) | |
| return | |
| } | |
| // RTCP detection | |
| if (udp.Payload[0]&0xc0)>>6 == 2 && | |
| (udp.Payload[1] == 200 || udp.Payload[1] == 201 || udp.Payload[1] == 207) { | |
| pkt.Payload, pkt.CID = correlateRTCP(pkt.SrcIP, pkt.SrcPort, pkt.DstIP, pkt.DstPort, udp.Payload) | |
| if pkt.Payload != nil { | |
| pkt.ProtoType = 5 | |
| atomic.AddUint64(&d.rtcpCount, 1) | |
| PacketQueue <- pkt | |
| return | |
| } | |
| atomic.AddUint64(&d.rtcpFailCount, 1) | |
| return | |
| } | |
| // If the packet uses RTP/RTCP version 2 but is not a handled RTCP type, | |
| // and the payload type is in a typical RTP range (excluding RTCP 72–79), | |
| // treat it as RTP and ignore it to avoid inflating unknownCount / CPU. | |
| if (udp.Payload[0]&0xc0)>>6 == 2 { | |
| pt := udp.Payload[1] & 0x7F | |
| if pt < 64 || (pt >= 96 && pt <= 127) { | |
| return | |
| } | |
| } |
@hamptonng Please sign this |
|
implemented in v2.0 |
No description provided.