Audio fix#16
Open
Paras-Sood wants to merge 10 commits into
Open
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## Root Cause
Analysis of all 36 WAV files in `/Users/kinshuk.bairagi/Desktop/rec/` confirms the pattern:
- **7 legs are broken** (89-98.5% zeros, inflated to 1000-3600 seconds)
- **All broken legs** belong to the "mostly-listening" party in a call
- **Counterpart legs** for the same call are small and healthy (0-5% zeros)
- **Combined WAVs** inherit the problem (e.g. `0287...wav` is 86.1% zeros)
**What's happening:** G.729 Annex B uses DTX (Discontinuous Transmission) -- when a party is silent, the transmitter stops sending RTP packets entirely. When the next packet arrives (comfort noise or brief speech), the RTP sequence number has jumped by hundreds. The PLC code at [rtp.go](siprec/pkg/media/rtp.go) line 614-648 interprets every such jump as packet loss and inserts `min(gap, maxPLC=100)` frames of silence (up to 2 seconds each time). For a party who is mostly listening, this happens every few seconds, inflating a 15-second file to 1000+ seconds.
**Evidence from `0bbf...10.wav`:**
- 50,487 frames total, 49,248 are exact zeros (97.5%)
- Non-zero frames occur every ~100 frames (exactly `maxPLC`)
- Silence blocks are consistently ~2000ms = 100 x 20ms
## Fix: Distinguish DTX Gaps from Real Packet Loss
The key insight: **DTX gaps have large wall-clock inter-arrival times** (the transmitter literally stopped sending). **Real packet loss during active speech** has normal arrival rates (~20ms apart) with sequence gaps.
### Changes in [rtp.go](siprec/pkg/media/rtp.go)
**A. Track last packet arrival time** alongside `lastSeq`:
- Add `var lastArrivalTime time.Time` next to `lastSeq` (line 499)
- Update it when `lastSeq` is updated (line 678-681)
**B. Gate PLC on inter-arrival time:**
- Before computing PLC silence (line 614), check if `arrival.Sub(lastArrivalTime) > dtxThreshold`
- If the gap between consecutive packet arrivals exceeds a threshold (e.g. **60ms** -- 3x the normal 20ms packet interval), treat it as a DTX gap and skip PLC entirely
- Only apply PLC when the inter-arrival gap is small (real packet loss during active speech)
**C. Reduce `maxPLC` from 100 to 10:**
- Even for genuine packet loss, 2 seconds of silence is far too much
- Cap at 10 packets (200ms) which is more than enough for transient network loss and won't be audible as a major disruption
- Line 631: `const maxPLC = 10`
### Summary of the logic
```
if lastSeq != nil && seq != expectedNext && !isReordered {
arrivalGap := arrival.Sub(lastArrivalTime)
if arrivalGap <= dtxThreshold {
// Packets arriving at normal rate but with seq gap = real packet loss
// Insert PLC silence (capped at maxPLC=10)
}
// else: large arrival gap = DTX silence suppression, skip PLC
}
```
Ensure WAV files are finalized before the RTP goroutine cleanup by deferring a call to WAVWriter.Finalize() (with warning log on error) in StartRTPForwarding. Also make WAVReader.parseHeader tolerant of unfinalized WAVs: if the data chunk size is zero, infer the data size from the remaining file length so CombineWAVRecordings can still produce playable output. This avoids corrupt/empty recordings when goroutines exit or headers weren't updated.
Root cause: The G.729 decoder's synthesis filter occasionally goes unstable (typically after DTX gaps or corrupted frames), producing a 2kHz square wave oscillating between +32767 and -32768. This sounds like harsh buzzing/distortion. Fix: Added isG729Oscillation() that detects the unstable pattern per decoded frame: >50% of samples at |value| >= 30000 (railed to max) >25% sign changes (high-frequency alternation near Nyquist) When both conditions are met, the frame is replaced with silence. Normal speech clipping (loud voice) is never flagged because it has far fewer railed samples (2-20 out of 80) and normal sign-change rates. The check is integrated directly into DecodeG729WithSSRC -- after each decoder.Decode(), if the output is oscillation, it's zeroed:
Problem: The arrival-time-based DTX detection (arrivalGap > 60ms) failed for calls where the PBX buffers and releases packets in bursts. Packets would arrive with near-zero wall-clock gaps but large sequence gaps. The PLC code treated these as real packet loss and inserted 200ms of silence per gap -- 652 times in this file alone. Fix: Replaced arrival-time heuristic with RTP timestamp gap detection. The RTP timestamp is set by the source and reflects the actual audio timeline, regardless of network jitter or PBX buffering: During DTX silence: the RTP timestamp jumps by hundreds of milliseconds worth of samples (e.g., 2 seconds = 16000 at 8kHz). This exceeds dtxTimestampThreshold (60ms worth = 480 samples) → PLC skipped. During real packet loss: consecutive packets have normal timestamp deltas (~160 per 20ms at 8kHz). This is within the threshold → PLC correctly inserts up to maxPLC=10 frames of silence. The arrival-time variable (lastArrivalTime) was replaced with lastTimestamp (uint32) and hasLastTimestamp (bool) tracking the previous packet's RTP timestamp.
Avoid decoding non-audio RTP payload types (e.g. event/Dtmf PTs) once the audio codec PT is known: such packets are early-returned and, if dtmfCh is present, a brief AcousticEvent with the payload_type is emitted. Add lastDecodedPCMSize to track the actual PCM bytes produced by the last decoded packet and prefer it when computing PLC silence length (falling back to PCMBytesPerPacket if unknown). This prevents decode errors/garbage from event payloads, avoids spurious PLC insertions, and ensures inserted silence has the correct byte length.
📝 WalkthroughWalkthroughRefactors G.729 decoding to a per-stream stateful model, adds per-stream PLC/gap/reorder handling in RTP processing, finalizes WAV recordings on RTP goroutine exit, adds a fallback for unfinalized WAV data chunk sizes, and introduces port allocation cooldown logic with accompanying tests and SIP hold/resume SSRC resets. Changes
Sequence DiagramsequenceDiagram
participant RTP as RTP Handler
participant Gap as Gap Detector
participant PLC as PLC Engine
participant G729 as G729StreamDecoder
participant WAV as WAV Writer
RTP->>RTP: Receive RTP packet
RTP->>Gap: Check gap/reorder vs last seq/timestamp
Gap-->>RTP: gap/reorder info
alt Reordered G.729 packet
RTP->>RTP: Drop packet (avoid G.729 state corruption)
else Gap/Loss detected
RTP->>PLC: Estimate missing packets / pcmBytesPerPacket
PLC->>G729: ConcealPackets(numPackets)
G729-->>PLC: PCM concealment frames
PLC->>WAV: Write concealment PCM or silence
end
alt G.729 payload
RTP->>G729: Decode(payload, ssrc)
G729->>G729: Reset state if SSRC changed
G729-->>RTP: Decoded PCM
else Other codec
RTP->>RTP: Use standard decode path
end
RTP->>WAV: Write decoded PCM
RTP->>RTP: Update last seq/timestamp (if not reordered)
RTP->>WAV: Finalize WAV on goroutine exit
WAV->>WAV: Update headers
G729->>G729: Close() (deferred cleanup)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/media/audio_conversion.go`:
- Around line 561-567: DecodeG729WithSSRC currently ignores the ssrc parameter
and always calls decodeG729Packet, breaking its SSRC-aware contract; restore
per-SSRC behavior by routing through a SSRC-keyed G.729 stream decoder instead
of the one-shot decoder. Implement a small registry
(map[uint32]*G729StreamDecoder) or a getOrCreateStreamDecoder helper and have
DecodeG729WithSSRC look up the decoder for the provided ssrc, create a new
G729StreamDecoder if missing, and use its Decode method on payload;
alternatively, if you intend to change semantics, rename/deprecate
DecodeG729WithSSRC and update callers. Reference the existing symbols
DecodeG729WithSSRC, decodeG729Packet, and G729StreamDecoder when making the
change.
In `@pkg/media/rtp.go`:
- Line 739: When RecordingPaused is true the code never advances lastSeq and
lastTimestamp, so the first packet after resume is treated as a large gap and
filled with PLC; update the packet handling logic (the branch that reads/sets
lastDecodedPCMSize and the similar block around lastSeq/lastTimestamp at lines
noted) to detect RecordingPaused and, on resume, set lastSeq and lastTimestamp
to the current packet's sequence/timestamp (and reset any gap counters) instead
of backfilling silence. Modify the same logic for the other occurrence (the
block at 768-773) so both places skip gap-detection/PLC when coming out of
RecordingPaused.
- Around line 577-597: current code uses 0 as the "audio established" sentinel
which conflicts with valid PT 0 (PCMU) and also reports every mismatched PT as
DTMF; change the sentinel logic so establishment is tracked explicitly (e.g.,
replace currentPayloadType with a nullable/int initialized to -1 or add a
boolean like audioPayloadEstablished) and check that flag instead of comparing
to 0 (referencing currentPayloadType and rtpPacket.PayloadType). Additionally,
only emit an AcousticEvent to dtmfCh when the mismatched payload type is
actually the configured telephone-event PT (use your
telephoneEventPT/configuredDtmfPT variable or add one) rather than on any PT
mismatch; otherwise skip the packet. Ensure references in code:
currentPayloadType, rtpPacket.PayloadType, dtmfCh, and AcousticEvent are updated
accordingly.
- Around line 707-711: The code only drops reordered packets for G.729
(isReordered && isG729) but lets late PCMU/PCMA/Opus packets through, which
corrupts recordings; change the logic to drop any reordered/late packet
regardless of codec by returning whenever isReordered is true (remove the isG729
guard) so late arrivals are discarded before they can be written/concatenated.
- Around line 644-695: The code treats PCM sizes as mono (dividing by 2) causing
undercounting for stereo; update all sample-frame calculations to be
channel-aware: compute framesPerPacket := lastDecodedPCMSize / (2 * channels)
(use the actual channel count for the stream/recording), use framesPerPacket
instead of lastDecodedPCMSize/2 when computing expectedSamples, set
bytesPerPacket := 2 * channels * frameDurationFrames when passing to
g729StreamDec.ConcealPackets, compute concealedSamples := len(concealPCM) / (2 *
channels), and allocate silence as make([]byte, remainingSamples * 2 * channels)
before writing to recordingWriter so gap accounting and inserted silence match
stereo/multi-channel streams.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24eec20d-8142-459a-bdbc-f8e752e289da
📒 Files selected for processing (3)
pkg/media/audio_conversion.gopkg/media/rtp.gopkg/media/wav_reader.go
Comment on lines
+561
to
567
| // DecodeG729WithSSRC is a stateless convenience wrapper that decodes a G.729 | ||
| // payload using a fresh decoder. It exists for backward compatibility; callers | ||
| // that process a continuous RTP stream should use G729StreamDecoder instead to | ||
| // maintain proper inter-packet decoder state. | ||
| func DecodeG729WithSSRC(payload []byte, ssrc uint32) ([]byte, error) { | ||
| return decodeG729Packet(payload) | ||
| } |
There was a problem hiding this comment.
DecodeG729WithSSRC no longer honors its SSRC-aware contract.
This now ignores ssrc and routes every call through the one-shot decoder, so any caller still feeding a continuous stream through this exported API loses inter-packet decoder state. Either preserve the SSRC-keyed compatibility layer behind this name or rename/deprecate it instead of silently changing semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/media/audio_conversion.go` around lines 561 - 567, DecodeG729WithSSRC
currently ignores the ssrc parameter and always calls decodeG729Packet, breaking
its SSRC-aware contract; restore per-SSRC behavior by routing through a
SSRC-keyed G.729 stream decoder instead of the one-shot decoder. Implement a
small registry (map[uint32]*G729StreamDecoder) or a getOrCreateStreamDecoder
helper and have DecodeG729WithSSRC look up the decoder for the provided ssrc,
create a new G729StreamDecoder if missing, and use its Decode method on payload;
alternatively, if you intend to change semantics, rename/deprecate
DecodeG729WithSSRC and update callers. Reference the existing symbols
DecodeG729WithSSRC, decodeG729Packet, and G729StreamDecoder when making the
change.
Comment on lines
+577
to
+597
| // Skip non-audio payload types: once the audio codec PT is | ||
| // established, packets with a different PT are event payloads | ||
| // (e.g. RFC 2833 DTMF on a dynamic PT) — not audio. Decoding | ||
| // them with the audio codec produces errors or garbage and | ||
| // creates sequence-number gaps that trigger spurious PLC. | ||
| if currentPayloadType != 0 && byte(rtpPacket.PayloadType) != currentPayloadType { | ||
| if dtmfCh != nil { | ||
| select { | ||
| case dtmfCh <- AcousticEvent{ | ||
| Type: "dtmf", | ||
| Confidence: 0.9, | ||
| Timestamp: time.Now(), | ||
| Details: map[string]interface{}{ | ||
| "payload_type": rtpPacket.PayloadType, | ||
| }, | ||
| }: | ||
| default: | ||
| } | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Don't use payload type 0 as the "audio established" sentinel.
PT 0 is valid PCMU, so currentPayloadType != 0 never becomes true on common PCMU streams. On those calls, 101/13/etc. still fall through to the audio decoder, and this branch also reports every mismatched PT as DTMF before confirming it is actually telephone-event.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/media/rtp.go` around lines 577 - 597, current code uses 0 as the "audio
established" sentinel which conflicts with valid PT 0 (PCMU) and also reports
every mismatched PT as DTMF; change the sentinel logic so establishment is
tracked explicitly (e.g., replace currentPayloadType with a nullable/int
initialized to -1 or add a boolean like audioPayloadEstablished) and check that
flag instead of comparing to 0 (referencing currentPayloadType and
rtpPacket.PayloadType). Additionally, only emit an AcousticEvent to dtmfCh when
the mismatched payload type is actually the configured telephone-event PT (use
your telephoneEventPT/configuredDtmfPT variable or add one) rather than on any
PT mismatch; otherwise skip the packet. Ensure references in code:
currentPayloadType, rtpPacket.PayloadType, dtmfCh, and AcousticEvent are updated
accordingly.
Comment on lines
+644
to
+695
| // Derive expected per-packet timestamp increment from the | ||
| // last decode's PCM output to adapt to 10/20/40 ms packing. | ||
| expectedSamples := uint32(lastDecodedPCMSize / 2) | ||
| minExp := uint32(sampleRate / 100) // 10 ms floor | ||
| maxExp := uint32(sampleRate / 10) // 100 ms ceiling | ||
| if expectedSamples < minExp || expectedSamples > maxExp { | ||
| expectedSamples = uint32(sampleRate / 50) // 20 ms default | ||
| } | ||
|
|
||
| // Upper bound rejects unsigned underflow (timestamp < last | ||
| // wraps to ~2^32 which far exceeds 120 s of samples). | ||
| maxReasonableGap := uint32(sampleRate * 120) | ||
|
|
||
| // Use stateful decoder for G.729 to maintain decoder state across packets | ||
| if tsGap > expectedSamples && tsGap <= maxReasonableGap { | ||
| gapSamples := int(tsGap - expectedSamples) | ||
|
|
||
| var lostPackets int | ||
| if seq > expectedNext { | ||
| lostPackets = int(seq - expectedNext) | ||
| } else { | ||
| lostPackets = int(seq) + (65536 - int(expectedNext)) | ||
| } | ||
|
|
||
| // For G.729, advance the decoder through up to 10 lost | ||
| // frames so the predictor state stays consistent. | ||
| concealedSamples := 0 | ||
| if g729StreamDec != nil && isG729 { | ||
| concealCount := lostPackets | ||
| const maxConceal = 10 | ||
| if concealCount > maxConceal { | ||
| concealCount = maxConceal | ||
| } | ||
| bytesPerPacket := lastDecodedPCMSize | ||
| if bytesPerPacket <= 0 { | ||
| bytesPerPacket = 320 | ||
| } | ||
| concealPCM := g729StreamDec.ConcealPackets(concealCount, bytesPerPacket) | ||
| if len(concealPCM) > 0 { | ||
| if _, writeErr := recordingWriter.Write(concealPCM); writeErr != nil { | ||
| forwarder.Logger.WithError(writeErr).WithField("call_uuid", callUUID).Debug("PLC concealment write failed") | ||
| } | ||
| concealedSamples = len(concealPCM) / 2 | ||
| } | ||
| } | ||
|
|
||
| // Fill remaining gap with silence to keep both legs | ||
| // time-aligned in the combined stereo recording. | ||
| remainingSamples := gapSamples - concealedSamples | ||
| if remainingSamples > 0 { | ||
| silence := make([]byte, remainingSamples*2) | ||
| if _, writeErr := recordingWriter.Write(silence); writeErr != nil { | ||
| forwarder.Logger.WithError(writeErr).WithField("call_uuid", callUUID).Debug("Gap silence write failed") |
There was a problem hiding this comment.
Gap accounting needs channel-aware frame counts.
RTP timestamps advance per sample frame, but lastDecodedPCMSize / 2 and len(concealPCM) / 2 count individual channel samples. Stereo codecs therefore under-detect loss and size silence incorrectly; a single lost 20 ms 48 kHz stereo packet yields tsGap == expectedSamples here, so no concealment is inserted at all.
Suggested fix
- expectedSamples := uint32(lastDecodedPCMSize / 2)
+ channels := currentChannels
+ if channels <= 0 {
+ channels = 1
+ }
+ bytesPerFrame := channels * 2
+ expectedSamples := uint32(lastDecodedPCMSize / bytesPerFrame)
@@
- concealedSamples = len(concealPCM) / 2
+ concealedSamples = len(concealPCM) / bytesPerFrame
@@
- silence := make([]byte, remainingSamples*2)
+ silence := make([]byte, remainingSamples*bytesPerFrame)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/media/rtp.go` around lines 644 - 695, The code treats PCM sizes as mono
(dividing by 2) causing undercounting for stereo; update all sample-frame
calculations to be channel-aware: compute framesPerPacket := lastDecodedPCMSize
/ (2 * channels) (use the actual channel count for the stream/recording), use
framesPerPacket instead of lastDecodedPCMSize/2 when computing expectedSamples,
set bytesPerPacket := 2 * channels * frameDurationFrames when passing to
g729StreamDec.ConcealPackets, compute concealedSamples := len(concealPCM) / (2 *
channels), and allocate silence as make([]byte, remainingSamples * 2 * channels)
before writing to recordingWriter so gap accounting and inserted silence match
stereo/multi-channel streams.
Comment on lines
+707
to
+711
| // G.729 is stateful: decoding a reordered packet with stale | ||
| // predictor state corrupts subsequent frames. Drop it. | ||
| if isReordered && isG729 { | ||
| return | ||
| } |
There was a problem hiding this comment.
Late packets still corrupt non-G.729 recordings.
By the time a reordered packet arrives, this function may already have written PLC/silence and newer audio. Letting late PCMU/PCMA/Opus packets through appends them at the tail and stretches the recording, even though the G.729 state issue is the most obvious case.
Suggested fix
- if isReordered && isG729 {
+ if isReordered {
return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/media/rtp.go` around lines 707 - 711, The code only drops reordered
packets for G.729 (isReordered && isG729) but lets late PCMU/PCMA/Opus packets
through, which corrupts recordings; change the logic to drop any reordered/late
packet regardless of codec by returning whenever isReordered is true (remove the
isG729 guard) so late arrivals are discarded before they can be
written/concatenated.
| } | ||
| } | ||
| } | ||
| lastDecodedPCMSize = len(pcm) |
There was a problem hiding this comment.
Paused recording is still treated as packet loss on resume.
While RecordingPaused is true, lastSeq and lastTimestamp never advance. The first packet after resume then sees the whole paused interval as a network gap and backfills it with silence/PLC, which defeats the pause.
Suggested fix
+ advanceTimeline := func() {
+ if !isReordered {
+ seq := rtpPacket.SequenceNumber
+ lastSeq = &seq
+ lastTimestamp = rtpPacket.Timestamp
+ hasLastTimestamp = true
+ }
+ }
+
forwarder.pauseMutex.RLock()
paused := forwarder.RecordingPaused
forwarder.pauseMutex.RUnlock()
if paused {
+ advanceTimeline()
if metrics.IsMetricsEnabled() {
metrics.RecordRTPDroppedPackets("recording_paused", 1)
}
return
}
@@
- if !isReordered {
- seq := rtpPacket.SequenceNumber
- lastSeq = &seq
- lastTimestamp = rtpPacket.Timestamp
- hasLastTimestamp = true
- }
+ advanceTimeline()Also applies to: 768-773
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/media/rtp.go` at line 739, When RecordingPaused is true the code never
advances lastSeq and lastTimestamp, so the first packet after resume is treated
as a large gap and filled with PLC; update the packet handling logic (the branch
that reads/sets lastDecodedPCMSize and the similar block around
lastSeq/lastTimestamp at lines noted) to detect RecordingPaused and, on resume,
set lastSeq and lastTimestamp to the current packet's sequence/timestamp (and
reset any gap counters) instead of backfilling silence. Modify the same logic
for the other occurrence (the block at 768-773) so both places skip
gap-detection/PLC when coming out of RecordingPaused.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/media/rtp.go`:
- Around line 687-705: The current branch that writes silence for large gaps (in
the block referencing recordingWriter, tsGap, expectedDelta, sampleRate and
constants minLargeGapSeconds/maxLargeGapSeconds) ignores gaps between ~60ms and
3s; update the logic to handle this intermediate range by either lowering
minLargeGapSeconds (or computing minLargeGap from the PLC threshold) or adding
an explicit branch that inserts appropriate PLC/silence for tsGap values
>shortGapThreshold and <minLargeGapSeconds; ensure you use the same gapSamples =
int(tsGap - expectedDelta) calculation, write the silence to recordingWriter,
and log write errors via forwarder.Logger.WithError(...).WithField("call_uuid",
callUUID).Debug(...) as in the existing large-gap path.
Comment on lines
+687
to
+705
| } else if recordingWriter != nil { | ||
| // Large gap (ringing / hold): insert silence to keep | ||
| // both legs time-aligned in the combined stereo recording. | ||
| // Guard against unsigned underflow (timestamp < last) | ||
| // by requiring tsGap to fall within a plausible range. | ||
| const minLargeGapSeconds = 3 | ||
| const maxLargeGapSeconds = 120 | ||
| minLargeGap := uint32(sampleRate * minLargeGapSeconds) | ||
| maxLargeGap := uint32(sampleRate * maxLargeGapSeconds) | ||
| if tsGap >= minLargeGap && tsGap <= maxLargeGap { | ||
| gapSamples := int(tsGap - expectedDelta) | ||
| if gapSamples > 0 { | ||
| silence := make([]byte, gapSamples*2) | ||
| if _, writeErr := recordingWriter.Write(silence); writeErr != nil { | ||
| forwarder.Logger.WithError(writeErr).WithField("call_uuid", callUUID).Debug("DTX gap silence write failed") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Gaps between 60ms and 3 seconds are silently ignored.
The code handles two gap categories:
- Short gaps (≤60ms): PLC/silence inserted
- Large gaps (3–120 seconds): silence inserted for time alignment
Gaps in the 60ms–3s range fall through with no handling. This could occur during brief hold, codec negotiation, or network congestion, causing the recording timeline to drift relative to the other call leg.
Consider lowering minLargeGapSeconds or adding explicit handling for this intermediate range.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/media/rtp.go` around lines 687 - 705, The current branch that writes
silence for large gaps (in the block referencing recordingWriter, tsGap,
expectedDelta, sampleRate and constants minLargeGapSeconds/maxLargeGapSeconds)
ignores gaps between ~60ms and 3s; update the logic to handle this intermediate
range by either lowering minLargeGapSeconds (or computing minLargeGap from the
PLC threshold) or adding an explicit branch that inserts appropriate PLC/silence
for tsGap values >shortGapThreshold and <minLargeGapSeconds; ensure you use the
same gapSamples = int(tsGap - expectedDelta) calculation, write the silence to
recordingWriter, and log write errors via
forwarder.Logger.WithError(...).WithField("call_uuid", callUUID).Debug(...) as
in the existing large-gap path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/media/rtp.go (1)
770-790:⚠️ Potential issue | 🟠 MajorPayload type 0 (PCMU) is a valid audio codec — using it as a sentinel causes issues.
When
currentPayloadTypeis set to 0 (PCMU), the conditioncurrentPayloadType != 0is false, so the early-skip branch never triggers for PCMU streams. This means:
- PT 101 (DTMF) packets still fall through to the audio decoder on PCMU calls
- Every mismatched PT is reported as DTMF before confirming it's actually
telephone-eventConsider using a separate boolean flag (e.g.,
audioPayloadEstablished) or initializingcurrentPayloadTypeto an invalid sentinel like 255 or -1 (if using int).🔧 Suggested approach
Track establishment with a separate flag rather than relying on the payload type value:
+ var audioPayloadEstablished bool // ... in the packet processing loop: - if currentPayloadType != 0 && byte(rtpPacket.PayloadType) != currentPayloadType { + if audioPayloadEstablished && byte(rtpPacket.PayloadType) != currentPayloadType {Set
audioPayloadEstablished = truewhen the codec is first established.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/media/rtp.go` around lines 770 - 790, The code currently uses currentPayloadType==0 as a sentinel which breaks for valid PT 0 (PCMU); change the logic to use an explicit boolean (e.g., audioPayloadEstablished) to indicate when the audio codec/PT is established (set audioPayloadEstablished = true when you first fix currentPayloadType in the codec-negotiation or first-audio-packet handling), then replace the check `if currentPayloadType != 0 && byte(rtpPacket.PayloadType) != currentPayloadType` with a check using audioPayloadEstablished (e.g., `if audioPayloadEstablished && byte(rtpPacket.PayloadType) != currentPayloadType`) so mismatched PTs are skipped correctly and dtmfCh handling remains unchanged.
🧹 Nitpick comments (4)
pkg/sip/hold_resume_bug_test.go (1)
74-87: Prefer condition-based wait over fixed sleep for timeout transition.The hard sleep at Line 76 can make this test timing-sensitive. Waiting on
RTPSuspendeddirectly is more deterministic.♻️ Suggested test stabilization
- // MonitorRTPTimeout has a hardcoded 1s ticker, so we must wait > 1s - time.Sleep(1500 * time.Millisecond) + // Wait until timeout processing marks the forwarder suspended + require.Eventually(t, func() bool { + return atomic.LoadInt32(&forwarder.RTPSuspended) == 1 + }, 3*time.Second, 25*time.Millisecond, "forwarder did not enter RTP-suspended state") @@ - require.Equal(t, int32(1), atomic.LoadInt32(&forwarder.RTPSuspended), - "Forwarder should be in RTP-suspended state after timeout") + require.Equal(t, int32(1), atomic.LoadInt32(&forwarder.RTPSuspended), + "Forwarder should be in RTP-suspended state after timeout")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sip/hold_resume_bug_test.go` around lines 74 - 87, Replace the fixed time.Sleep with a condition-based wait that polls the forwarder's RTPSuspended flag and the StopChan to deterministically detect the timeout transition: repeatedly check atomic.LoadInt32(&forwarder.RTPSuspended) until it becomes 1 (or a short deadline elapses) and fail if forwarder.StopChan is closed during the wait; then assert equality as before. Use a small polling interval (e.g., 10–50ms) and a total timeout slightly above the expected MonitorRTPTimeout to avoid flakiness, referencing forwarder, RTPSuspended, and StopChan when modifying the test logic.pkg/media/rtp.go (1)
528-536: Consider documenting the rationale for the threshold values.The constants
ssrcCorrectionThreshold = 50andssrcCorrectionInactivity = 30are critical tuning parameters. Consider adding brief comments explaining why these specific values were chosen (e.g., "50 packets ≈ 1 second at 20ms ptime" or similar).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/media/rtp.go` around lines 528 - 536, The constants ssrcCorrectionThreshold and ssrcCorrectionInactivity lack rationale; add brief inline comments next to these constants documenting why their values were chosen (e.g., relate 50 packets to expected packet rate/ptime like "≈1s at 20ms ptime" and explain that 30 consecutive non-locked packets represents required inactivity window), so anyone reading the SSRC correction logic (variables ssrcLockedAt, ssrcCorrectionCount, alternateSSRC, alternateSSRCCount, packetsSinceLastLockedSSRC) can understand the timing assumptions and tuning intent.pkg/media/port_manager.go (2)
150-153: Redundant even-port check — the loop already steps by 2 from an even start.The
if port%2 != 0 { continue }check on line 151-153 is unnecessary because the loop starts atpm.minPort(which must be even based on the RFC 3550 requirement) and increments by 2. The same redundancy exists at lines 192-194.♻️ Suggested simplification
for port := pm.minPort; port <= pm.maxPort-1; port += 2 { - if port%2 != 0 { - continue - } rtpPort := portIf you want to be defensive against misconfigured
minPort, consider adding validation inNewPortManagerinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/media/port_manager.go` around lines 150 - 153, The for-loops in the PortManager methods iterate from pm.minPort with step += 2 but still contain an unnecessary parity guard (if port%2 != 0 { continue }); remove that redundant check from those loops (referencing the loops that use "for port := pm.minPort; port <= pm.maxPort-1; port += 2"). To be defensive, add validation in NewPortManager to assert/normalize pm.minPort is even (or return an error) and also validate min<=max and range bounds so the loops can assume an even start; update NewPortManager accordingly (the constructor that initializes a PortManager instance).
168-188: Iteration overcoolingDownmap has non-deterministic order.When falling back to cooling-down ports, iterating over
coolingDown(a map) yields ports in random order. This could lead to non-reproducible allocation patterns in tests or debugging. Consider iterating in sorted order or using the sameminPort..maxPortscan filtered bycoolingDown[port].♻️ Suggested approach for deterministic fallback
- // Fallback — use cooling-down port pairs rather than failing - for port := range coolingDown { - if port%2 != 0 { - continue - } + // Fallback — use cooling-down port pairs in deterministic order + for port := pm.minPort; port <= pm.maxPort-1; port += 2 { + if !coolingDown[port] { + continue + } rtpPort := port🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/media/port_manager.go` around lines 168 - 188, The fallback loop iterates the coolingDown map in non-deterministic order; change it to a deterministic scan (either collect and sort coolingDown keys or iterate port := pm.minPort; port <= pm.maxPort; port += 2 and check coolingDown[port]) so allocations are reproducible. Keep the same allocation checks (pm.usedPorts, portAvailable), marking pm.usedPorts[rtpPort] and pm.usedPorts[rtcpPort], updating pm.stats.AllocationCount/ReuseHits and calling pm.updateStats() before returning the &PortPair{RTPPort: rtpPort, RTCPPort: rtcpPort}. Use the existing symbols coolingDown, pm.minPort/pm.maxPort, pm.usedPorts, portAvailable, PortPair, pm.stats and pm.updateStats in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/sip/custom_server.go`:
- Around line 2945-2950: The log reads fwd.RemoteSSRC unsafely causing a data
race; either remove "ssrc" from the logger.WithFields call in the pause/hold
logging or add and use a synchronized accessor on media.RTPForwarder (e.g.,
RemoteSSRC() protected by remoteMutex) and call that accessor instead of
accessing the field directly; update the logging site that uses
logger.WithFields (the call referencing
"call_id","forwarder","local_port","ssrc") to use the safe approach to eliminate
the race.
---
Duplicate comments:
In `@pkg/media/rtp.go`:
- Around line 770-790: The code currently uses currentPayloadType==0 as a
sentinel which breaks for valid PT 0 (PCMU); change the logic to use an explicit
boolean (e.g., audioPayloadEstablished) to indicate when the audio codec/PT is
established (set audioPayloadEstablished = true when you first fix
currentPayloadType in the codec-negotiation or first-audio-packet handling),
then replace the check `if currentPayloadType != 0 &&
byte(rtpPacket.PayloadType) != currentPayloadType` with a check using
audioPayloadEstablished (e.g., `if audioPayloadEstablished &&
byte(rtpPacket.PayloadType) != currentPayloadType`) so mismatched PTs are
skipped correctly and dtmfCh handling remains unchanged.
---
Nitpick comments:
In `@pkg/media/port_manager.go`:
- Around line 150-153: The for-loops in the PortManager methods iterate from
pm.minPort with step += 2 but still contain an unnecessary parity guard (if
port%2 != 0 { continue }); remove that redundant check from those loops
(referencing the loops that use "for port := pm.minPort; port <= pm.maxPort-1;
port += 2"). To be defensive, add validation in NewPortManager to
assert/normalize pm.minPort is even (or return an error) and also validate
min<=max and range bounds so the loops can assume an even start; update
NewPortManager accordingly (the constructor that initializes a PortManager
instance).
- Around line 168-188: The fallback loop iterates the coolingDown map in
non-deterministic order; change it to a deterministic scan (either collect and
sort coolingDown keys or iterate port := pm.minPort; port <= pm.maxPort; port +=
2 and check coolingDown[port]) so allocations are reproducible. Keep the same
allocation checks (pm.usedPorts, portAvailable), marking pm.usedPorts[rtpPort]
and pm.usedPorts[rtcpPort], updating pm.stats.AllocationCount/ReuseHits and
calling pm.updateStats() before returning the &PortPair{RTPPort: rtpPort,
RTCPPort: rtcpPort}. Use the existing symbols coolingDown,
pm.minPort/pm.maxPort, pm.usedPorts, portAvailable, PortPair, pm.stats and
pm.updateStats in the change.
In `@pkg/media/rtp.go`:
- Around line 528-536: The constants ssrcCorrectionThreshold and
ssrcCorrectionInactivity lack rationale; add brief inline comments next to these
constants documenting why their values were chosen (e.g., relate 50 packets to
expected packet rate/ptime like "≈1s at 20ms ptime" and explain that 30
consecutive non-locked packets represents required inactivity window), so anyone
reading the SSRC correction logic (variables ssrcLockedAt, ssrcCorrectionCount,
alternateSSRC, alternateSSRCCount, packetsSinceLastLockedSSRC) can understand
the timing assumptions and tuning intent.
In `@pkg/sip/hold_resume_bug_test.go`:
- Around line 74-87: Replace the fixed time.Sleep with a condition-based wait
that polls the forwarder's RTPSuspended flag and the StopChan to
deterministically detect the timeout transition: repeatedly check
atomic.LoadInt32(&forwarder.RTPSuspended) until it becomes 1 (or a short
deadline elapses) and fail if forwarder.StopChan is closed during the wait; then
assert equality as before. Use a small polling interval (e.g., 10–50ms) and a
total timeout slightly above the expected MonitorRTPTimeout to avoid flakiness,
referencing forwarder, RTPSuspended, and StopChan when modifying the test logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 086e1ed1-3488-4f01-9013-606054a2eefb
📒 Files selected for processing (6)
pkg/media/port_manager.gopkg/media/port_manager_test.gopkg/media/rtp.gopkg/media/types.gopkg/sip/custom_server.gopkg/sip/hold_resume_bug_test.go
Comment on lines
+2945
to
+2950
| logger.WithFields(logrus.Fields{ | ||
| "call_id": message.CallID, | ||
| "forwarder": i, | ||
| "local_port": fwd.LocalPort, | ||
| "ssrc": fwd.RemoteSSRC, | ||
| }).Debug("Forwarder paused for hold") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify direct RemoteSSRC field accesses and mutex-protected SSRC operations.
rg -n --type=go '\.RemoteSSRC\b'
rg -n --type=go 'remoteMutex\.(Lock|RLock)\('Repository: voiceip/siprec
Length of output: 1562
🏁 Script executed:
# Check RTPForwarder struct definition to see RemoteSSRC field and synchronization model
rg -n --type=go -B2 -A15 'type RTPForwarder struct' pkg/media/Repository: voiceip/siprec
Length of output: 1403
🏁 Script executed:
# Get context around line 2949 in custom_server.go (lines 2940-2960)
sed -n '2940,2960p' pkg/sip/custom_server.go | cat -nRepository: voiceip/siprec
Length of output: 886
🏁 Script executed:
# Compare logging at lines 1189, 1213, 1230 in rtp.go to see similar patterns
sed -n '1185,1195p' pkg/media/rtp.go | cat -n
sed -n '1209,1219p' pkg/media/rtp.go | cat -n
sed -n '1226,1236p' pkg/media/rtp.go | cat -nRepository: voiceip/siprec
Length of output: 1761
🏁 Script executed:
# Get full RTPForwarder struct definition
sed -n '20,100p' pkg/media/types.go | cat -nRepository: voiceip/siprec
Length of output: 4258
🏁 Script executed:
# Check RemoteSSRC field specifically and any mutex comments
rg -n --type=go -B1 -A1 'RemoteSSRC' pkg/media/types.goRepository: voiceip/siprec
Length of output: 529
🏁 Script executed:
# Examine the remoteMutex Lock/Unlock patterns around RemoteSSRC writes
sed -n '568,580p' pkg/media/rtp.go | cat -nRepository: voiceip/siprec
Length of output: 586
🏁 Script executed:
# Search for remoteMutex field declaration in RTPForwarder struct
rg -n --type=go 'remoteMutex' pkg/media/types.goRepository: voiceip/siprec
Length of output: 140
🏁 Script executed:
# Check for any getter method that safely exposes RemoteSSRC
rg -n --type=go 'func.*GetRemoteSSRC|func.*RemoteSSRC' pkg/media/Repository: voiceip/siprec
Length of output: 123
🏁 Script executed:
# Get lines 81-150 of types.go to see if remoteMutex is declared
sed -n '81,150p' pkg/media/types.go | cat -nRepository: voiceip/siprec
Length of output: 2730
Unsynchronized RemoteSSRC read in hold logging exposes data race.
At line 2949, fwd.RemoteSSRC is read directly without mutex protection. The RTPForwarder struct protects RemoteSSRC with remoteMutex on writes (lines 570, 635, 1405, etc. in rtp.go) but multiple logging statements (lines 1189, 1213, 1230 in rtp.go and line 2949 here) read the field unprotected, creating a classic data race under concurrent RTP packet processing. Remove this field from log output to eliminate the race, or expose a synchronized getter method on media.RTPForwarder.
🔧 Safe fix
logger.WithFields(logrus.Fields{
"call_id": message.CallID,
"forwarder": i,
"local_port": fwd.LocalPort,
- "ssrc": fwd.RemoteSSRC,
}).Debug("Forwarder paused for hold")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger.WithFields(logrus.Fields{ | |
| "call_id": message.CallID, | |
| "forwarder": i, | |
| "local_port": fwd.LocalPort, | |
| "ssrc": fwd.RemoteSSRC, | |
| }).Debug("Forwarder paused for hold") | |
| logger.WithFields(logrus.Fields{ | |
| "call_id": message.CallID, | |
| "forwarder": i, | |
| "local_port": fwd.LocalPort, | |
| }).Debug("Forwarder paused for hold") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sip/custom_server.go` around lines 2945 - 2950, The log reads
fwd.RemoteSSRC unsafely causing a data race; either remove "ssrc" from the
logger.WithFields call in the pause/hold logging or add and use a synchronized
accessor on media.RTPForwarder (e.g., RemoteSSRC() protected by remoteMutex) and
call that accessor instead of accessing the field directly; update the logging
site that uses logger.WithFields (the call referencing
"call_id","forwarder","local_port","ssrc") to use the safe approach to eliminate
the race.
kingster
pushed a commit
that referenced
this pull request
Mar 19, 2026
… decoder This commit adds features from voiceip/siprec PR #16: Per-stream G.729 Decoder: - Replace global G729DecoderPool with per-goroutine G729StreamDecoder - Eliminates cross-call state leakage and race conditions - Add ConcealPackets() for proper PLC that advances decoder state Port Allocation Cooldown: - Avoid recently freed ports to prevent stale RTP crosstalk - Fresh ports preferred; cooling-down ports used as fallback SSRC Validation & Correction: - Lock SSRC from first RTP packet, drop mismatched packets - Smart correction when locked SSRC goes silent and alternate has sustained traffic (handles first-packet poisoning and silent SSRC changes during hold/unhold) - Block correction during hold or RTP gap states SIPREC Forwarder Survival: - SIPREC forwarders survive RTP timeout (stay alive until BYE) - RTPSuspended atomic flag tracks gap state - ResetRemoteSSRC() API for SIP signaling events Hold/Resume Improvements: - Call ResetRemoteSSRC on re-INVITE and UPDATE resume - Keep SSRC locked during hold to prevent stale traffic acceptance - Enhanced logging for forwarder state changes PLC Improvements: - Run PLC before decode to advance G.729 decoder state - G.729 uses ConcealPackets; other codecs use silence - Large gaps (3-120s) insert silence for leg alignment - Drop reordered G.729 packets to prevent decoder corruption Other: - Skip non-audio payload types (DTMF) to prevent spurious PLC - WAV finalization defer ensures playable recordings on exit
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
2 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes
Stability
Tests