Add SIPREC recording support (RFC 7865/7866) to SBC#266
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces SIPREC (RFC 7865/7866) session recording to the SEMS SBC by adding a new SBC call_control module (cc_siprec) that forks RTP to an SRS, plus a minimal SIPREC Session Recording Server application (siprec_srs) that receives the recording INVITEs and writes per-leg WAV + metadata XML.
Changes:
- Add
cc_sipreccall_control module with SIPREC dialog handling, metadata generation, and RTP forking to an SRS. - Add
siprec_srsSEMS application to accept SIPREC INVITEs, receive dual RTP streams, and write WAV/XML artifacts. - Extend SBC RTP relay with an
ExtendedCCInterface::onAfterRTPRelayhook; adjust config installation behavior and improve Max-Forwards/registrar logging.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| core/CMakeLists.txt | Changes config install behavior to preserve existing configs and install .dist references. |
| cmake/config.rules.txt | Updates shared config-install macro to install .dist and conditionally install real configs. |
| core/AmB2BSession.cpp | Adds WARN log when rejecting relayed requests with Max-Forwards=0. |
| apps/sbc/SBC.cpp | Adds WARN log when rejecting OoD requests with Max-Forwards=0. |
| apps/sbc/SBCCallLeg.cpp | Adds WARN log for Max-Forwards check; invokes new RTP relay hook for extended CC modules. |
| apps/sbc/ExtendedCCInterface.h | Introduces onAfterRTPRelay hook (RTP-thread callback). |
| apps/sbc/call_control/CMakeLists.txt | Adds siprec call_control subdirectory. |
| apps/sbc/call_control/siprec/* | Implements SIPREC client: SIP dialog, metadata XML builder, RTP forwarder, config + docs, and build rules. |
| apps/siprec_srs/* | Implements minimal SIPREC SRS: SDP negotiation for two legs, RTP receive/decoding, WAV writer, config + docs, and build rules. |
| apps/sbc/call_control/registrar/Registrar.cpp | Improves log messages for retargeting/404 and REGISTER handling. |
| apps/CMakeLists.txt | Adds siprec_srs application to build. |
| README.md | Documents cc_siprec under SBC call_control modules and adds siprec_srs under Recording. |
| .gitignore | Adds .claude to ignored files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Don't flood logs - only log occasionally | ||
| static int err_count = 0; | ||
| if (++err_count % 1000 == 1) { | ||
| ERROR("SIPREC RtpForwarder: sendto() failed: %s (error #%d)\n", | ||
| strerror(errno), err_count); | ||
| } |
There was a problem hiding this comment.
err_count is a function-local static int updated from sendPacket(), which is intended to be called from RTP threads. This introduces a data race (and couples rate limiting across all instances). Use an atomic counter, or move the counter to a per-instance member guarded appropriately.
| int ret = state->dialog->startRecording(srs_uri, caller_uri, callee_uri, | ||
| cs_sdp, call_id); | ||
| if (ret != 0) { | ||
| ERROR("SIPREC: failed to start recording for call %s\n", ltag.c_str()); | ||
| removeState(ltag); | ||
| } |
There was a problem hiding this comment.
The PR description mentions recording_mandatory should reject/terminate the call if the SRS is unreachable, but recording_mandatory_ is never used and failures to start recording only log and remove state. If mandatory recording is required, this code should trigger a concrete call action (e.g., immediately hang up/refuse the call) when startRecording() fails or when the SRS rejects the INVITE.
| if (req.method == SIP_METH_INVITE) { | ||
| if (reply.code >= 200 && reply.code < 300) { | ||
| // 200 OK - parse SDP answer to get SRS RTP ports | ||
| if (reply.body.getLen()) { |
There was a problem hiding this comment.
if (reply.body.getLen()) will be false for multipart bodies because AmMimeBody::getLen() returns 0 for multipart even when parts exist. This can cause SDP parsing to be skipped even though an application/sdp part is present. Use !reply.body.empty() (or just attempt hasContentType() unconditionally) instead of checking getLen().
| if (reply.body.getLen()) { | |
| if (!reply.body.empty()) { |
| leg + 1, bound_port, codec_name.c_str(), pt, wav_path.c_str()); | ||
|
|
||
| leg++; | ||
| if (leg >= 2) break; // only accept first 2 audio m-lines |
There was a problem hiding this comment.
The SDP answer must include the same number of m-lines as the offer (RFC 3264). Breaking out of the loop after 2 legs can produce an answer with fewer m-lines than the offer if the INVITE contains additional media sections. Instead, continue iterating and set remaining m-lines to port=0 (rejected/disabled) to preserve ordering/count.
| if (leg >= 2) break; // only accept first 2 audio m-lines | |
| // Do not break here: per RFC 3264, the SDP answer must preserve the | |
| // number and ordering of m-lines from the offer, so we continue | |
| // iterating over all media sections. |
| // Read payload type from RTP header (byte 1, bits 0-6) | ||
| uint8_t pt = buf[1] & 0x7F; | ||
|
|
||
| // Skip non-audio payload types (CN=13, telephone-event=96-127) | ||
| if (pt == 13 || pt >= 96) continue; | ||
|
|
||
| int payload_len = len - hdr_len; | ||
| uint8_t* payload = buf + hdr_len; | ||
|
|
||
| // Decode G.711 payload to 16-bit PCM based on actual RTP payload type | ||
| int16_t pcm_buf[RTP_MAX_PACKET]; | ||
| for (int i = 0; i < payload_len; i++) { | ||
| if (pt == 0) | ||
| pcm_buf[i] = decodePCMU(payload[i]); | ||
| else | ||
| pcm_buf[i] = decodePCMA(payload[i]); | ||
| } |
There was a problem hiding this comment.
RTP decoding currently ignores the negotiated/expected codec (codec_ is set in init() but never used) and decodes any payload type < 96 as PCMA (except pt=0). This can produce corrupted WAV output (and is unsafe if unexpected PTs arrive). Consider validating pt against the expected PT from SDP and dropping mismatches; if dynamic PTs are possible, the SDP negotiation should enforce/translate to 0/8 or the receiver should accept the negotiated dynamic PT.
| string ltag = call->getLocalTag(); | ||
| SiprecCallState* state = getState(ltag); | ||
| if (state && state->dialog) { | ||
| state->dialog->stopRecording(); | ||
| } |
There was a problem hiding this comment.
getState() returns a raw pointer obtained under sessions_mut_, but it is used after the lock is released. If another thread calls removeState() concurrently, state/state->dialog can become dangling before stopRecording() is invoked. Consider returning a ref-counted handle (or otherwise guaranteeing lifetime) for all getState() call sites.
| if (it != sessions_.end()) state = it->second; | ||
| } | ||
|
|
||
| if (state && state->dialog) { | ||
| state->dialog->forwardRtp(p, is_aleg); |
There was a problem hiding this comment.
state is looked up under sessions_mut_ but then used after the lock is released. Since removeState() can delete the SiprecCallState concurrently (e.g., from onDestroyLeg() while the RTP thread is running), this can become a use-after-free when calling state->dialog->forwardRtp(). Consider keeping the object alive via ref-counting (e.g., shared_ptr), or copying out a stable pointer (e.g., SiprecDialog*) under the lock with a lifetime guarantee, or deferring deletion until RTP forwarding is stopped.
| if (it != sessions_.end()) state = it->second; | |
| } | |
| if (state && state->dialog) { | |
| state->dialog->forwardRtp(p, is_aleg); | |
| if (it != sessions_.end()) | |
| state = it->second; | |
| if (state && state->dialog) { | |
| // Call forwardRtp while holding sessions_mut_ so that removeState() | |
| // cannot delete the SiprecCallState concurrently. | |
| state->dialog->forwardRtp(p, is_aleg); | |
| } |
| // For B-leg, search by b_leg_ltag | ||
| for (it = sessions_.begin(); it != sessions_.end(); ++it) { | ||
| if (it->second->b_leg_ltag == ltag) break; | ||
| } |
There was a problem hiding this comment.
For B-leg packets this performs a linear scan over all active sessions on every RTP packet (for (it = sessions_.begin(); ...)). This is O(N) per packet and will not scale with concurrent calls. Consider maintaining a secondary map keyed by b_leg_ltag (or storing the recording state pointer directly on the call leg) so lookup is O(1).
| SdpMedia m1; | ||
| m1.type = MT_AUDIO; | ||
| m1.port = 0; // SRS will tell us where to send (we use our own UDP socket) | ||
| m1.transport = TP_RTPAVP; | ||
| m1.send = true; | ||
| m1.recv = false; // sendonly | ||
|
|
There was a problem hiding this comment.
Setting the offered m-line port to 0 makes the stream "disabled" per RFC 3264. Some SIPREC SRS implementations will reject/ignore such m-lines, which will break interoperability. If the SRC intends to send RTP, it should advertise a real local RTP port (and ideally send from that port by binding the forwarder socket accordingly).
| #include "AmThread.h" | ||
|
|
||
| #include <string> | ||
| #include <atomic> |
There was a problem hiding this comment.
This header uses fixed-width integer types (uint32_t, uint8_t, int16_t) but doesn't include <cstdint> / <stdint.h>. Relying on indirect includes is non-portable and can break builds on some platforms/toolchains.
| #include <atomic> | |
| #include <atomic> | |
| #include <cstdint> |
Implement a SIPREC Session Recording Client (SRC) as an SBC call_control module (cc_siprec) that forks RTP to a Session Recording Server (SRS), plus a minimal built-in SRS (siprec_srs) for testing. SIP signaling (RFC 7866): - INVITE with Require: siprec and +sip.src Contact feature tag - multipart/mixed body: SDP offer + metadata XML - Content-Disposition: recording-session on metadata parts - Separate sendonly m-lines per CS direction with a=label - Symmetric RTP with allocated local port pairs (RFC 7866 8.1.8) - RTCP port allocation on port+1 (RFC 7866 8.1.1) - Configurable RTP profile: RTP/AVP, RTP/SAVP, RTP/AVPF, RTP/SAVPF - CANCEL for pending INVITE on call teardown - re-INVITE for hold/resume with updated SDP + metadata - BYE with final metadata and stop-time Metadata XML (RFC 7865): - Namespace urn:ietf:params:xml:ns:recording:1 - session, participant, stream elements with base64 IDs - sessionrecordingassoc and participantsessionassoc elements - participantstreamassoc with send/recv text-content stream refs - ISO 8601 timestamps, partial updates on hold Recording indication (RFC 7866 6.1.2): - a=record:on injected into B-leg INVITE SDP CS SDP codec extraction: - Parses initial INVITE SDP to auto-detect codec for RS offer - Falls back to configured codec (PCMA/PCMU) when unavailable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1598442 to
28b11dc
Compare
Implement a SIPREC Session Recording Client (cc_siprec) as an SBC call_control module and a minimal Session Recording Server (siprec_srs) as a standalone SEMS application.
cc_siprec hooks into the SBC's RTP relay path to fork media streams to an SRS via SIPREC signaling. It manages the full SIPREC dialog lifecycle: INVITE with multipart SDP + RFC 7865 metadata XML, hold/resume updates, and BYE on call teardown.
siprec_srs receives recording INVITEs, negotiates dual RTP streams (one per call leg), decodes G.711 A-law/u-law based on the RTP payload type, and writes each leg to a separate 16-bit PCM WAV file alongside the SIPREC metadata XML.