From f8d0e6fc36338f567acbf0db8db8da8671985519 Mon Sep 17 00:00:00 2001 From: nam Date: Wed, 4 Nov 2026 11:21:50 -0600 Subject: [PATCH] Skip RTP padding packets instead of concealing them Padding/probe packets use a sequence number but carry no audio. They were treated as lost, so each one played an invented 20 ms frame (a glitch) and pushed the buffer a frame deeper for drift correction to remove again. Signature in the first live test: lost ~= drift-drops (74 vs 72 per minute). They are now skipped and counted as 'padding'. Co-Authored-By: Claude Opus 5.5 --- README.md | 7 ++++++- call.go | 37 +++++++++++++++++++++++++--------- test/jb_sim_test.go.txt | 2 +- test/pad_test.go.txt | 44 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 11 deletions(-) create mode 100644 test/pad_test.go.txt diff --git a/README.md b/README.md index 3d97666..67220b8 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,12 @@ Why v2: v1 skipped ahead on an underrun and kept playing from an empty buffer, s first hiccup every later wobble was another glitch. It also never corrected for a slow guest clock. In simulation that was ~12 glitches/min with a guest clock 30 ppm slow, against ~0 for v2. The 3 Nov 2026 cohost "jitter" was probably this, but that night was not measured. -The simulation is kept in `test/jb_sim_test.go.txt` (rename to `_test.go` in the package dir to run). +The simulation (and a padding regression test, `test/pad_test.go.txt`) is kept in `test/jb_sim_test.go.txt` (rename to `_test.go` in the package dir to run). + +Padding fix (v2b, same day): browsers send RTP padding/probe packets that use a sequence +number but carry no audio. v1/v2 treated each one as a lost packet and played an invented +frame. That was an audible glitch, plus a frame the drift correction later had to remove +(signature in the logs: lost ~= drift-drops). They are now skipped and counted as `padding`. The guest websocket is pinged every 25 s. Without that, the proxy closed it every 90 s (signalling only, audio unaffected, but the guest saw "reconnecting..."). diff --git a/call.go b/call.go index 7bb6226..518cec6 100644 --- a/call.go +++ b/call.go @@ -266,8 +266,8 @@ func (c *Call) Close() { c.jbMu.Unlock() if jb != nil { k, _, t := jb.snapshot() - log.Printf("call totals: lost %d (fec %d) concealed %d late %d underruns %d drift-drops %d inserted %d, final target %d ms", - k.Lost, k.FEC, k.PLC, k.Late, k.Underruns, k.Dropped, k.Inserted, t) + log.Printf("call totals: lost %d (fec %d) concealed %d late %d underruns %d drift-drops %d inserted %d padding %d, final target %d ms", + k.Lost, k.FEC, k.PLC, k.Late, k.Underruns, k.Dropped, k.Inserted, k.Padding, t) } }) } @@ -285,8 +285,8 @@ func (c *Call) logRx(jb *jitterBuffer) { case <-t.C: } k, d, tg := jb.snapshot() - log.Printf("rx 1m: buffer %d/%d ms, lost %d (fec %d), concealed %d, late %d, underruns %d, drift-drops %d, inserted %d", - d, tg, k.Lost-prev.Lost, k.FEC-prev.FEC, k.PLC-prev.PLC, k.Late-prev.Late, k.Underruns-prev.Underruns, k.Dropped-prev.Dropped, k.Inserted-prev.Inserted) + log.Printf("rx 1m: buffer %d/%d ms, lost %d (fec %d), concealed %d, late %d, underruns %d, drift-drops %d, inserted %d, padding %d", + d, tg, k.Lost-prev.Lost, k.FEC-prev.FEC, k.PLC-prev.PLC, k.Late-prev.Late, k.Underruns-prev.Underruns, k.Dropped-prev.Dropped, k.Inserted-prev.Inserted, k.Padding-prev.Padding) prev = k } } @@ -411,7 +411,7 @@ func (c *Call) startPlay(t *webrtc.TrackRemote) { log.Printf("decoder: %v", err) return } - jb := newJitterBuffer(dec) + jb := newJitterBuffer(dec, uint8(t.PayloadType())) c.jbMu.Lock() c.jb = jb c.jbMu.Unlock() @@ -529,6 +529,8 @@ type jitterBuffer struct { mu sync.Mutex dec *opus.Decoder pkts map[uint16][]byte + skip map[uint16]bool // sequence numbers that carry no audio (padding/probes) + pt uint8 // the negotiated Opus payload type nextSeq uint16 haveSeq bool // nextSeq is meaningful playing bool // false = filling up to target @@ -545,6 +547,7 @@ type jitterBuffer struct { type jbCounters struct { Lost, FEC, PLC, Dropped, Inserted, Underruns, Late int + Padding int // packets with no audio (skipped, not concealed) } const ( @@ -567,14 +570,15 @@ func (j *jitterBuffer) isQuiet(pcm []int16) bool { return r < lim } -func newJitterBuffer(dec *opus.Decoder) *jitterBuffer { - return &jitterBuffer{dec: dec, pkts: map[uint16][]byte{}, target: jbFloor, pcm: make([]int16, 5760*channels), lastBad: time.Now(), floor: quietRMS} +func newJitterBuffer(dec *opus.Decoder, pt uint8) *jitterBuffer { + return &jitterBuffer{dec: dec, pt: pt, pkts: map[uint16][]byte{}, skip: map[uint16]bool{}, target: jbFloor, pcm: make([]int16, 5760*channels), lastBad: time.Now(), floor: quietRMS} } func seqLess(a, b uint16) bool { return int16(a-b) < 0 } func (j *jitterBuffer) reset() { j.pkts = map[uint16][]byte{} + j.skip = map[uint16]bool{} j.haveSeq, j.playing, j.lateRun, j.insertNow = false, false, 0, false } @@ -591,7 +595,17 @@ func (j *jitterBuffer) bump() { func (j *jitterBuffer) push(p *rtp.Packet) { j.mu.Lock() defer j.mu.Unlock() - if len(p.Payload) == 0 { + if len(p.Payload) == 0 || (j.pt != 0 && p.PayloadType != j.pt) { + // Padding / bandwidth probe: it uses a sequence number but carries no audio and no + // time. v1/v2a ignored it, which left a hole that was then "concealed" - an + // invented 20 ms frame that the drift correction later had to remove again. + j.c.Padding++ + if !j.haveSeq || !seqLess(p.SequenceNumber, j.nextSeq) { + if len(j.skip) > 200 { + j.skip = map[uint16]bool{} + } + j.skip[p.SequenceNumber] = true + } return } if j.haveSeq { @@ -695,6 +709,11 @@ func (j *jitterBuffer) next() []int16 { } for { + if j.skip[j.nextSeq] { + delete(j.skip, j.nextSeq) + j.nextSeq++ + continue + } if p, ok := j.pkts[j.nextSeq]; ok { delete(j.pkts, j.nextSeq) j.nextSeq++ @@ -744,6 +763,6 @@ func (j *jitterBuffer) stats() map[string]any { return map[string]any{ "depthMs": d, "targetMs": t, "lost": c.Lost, "fec": c.FEC, "plc": c.PLC, "dropped": c.Dropped, "inserted": c.Inserted, - "underruns": c.Underruns, "late": c.Late, + "underruns": c.Underruns, "late": c.Late, "padding": c.Padding, } } diff --git a/test/jb_sim_test.go.txt b/test/jb_sim_test.go.txt index 7a060d7..65fbe82 100644 --- a/test/jb_sim_test.go.txt +++ b/test/jb_sim_test.go.txt @@ -30,7 +30,7 @@ func sim(t *testing.T, name string, noiseFloor float64, ppm float64, jitterMs fu for i := 0; i < 50; i++ { talkP = append(talkP, mk(i, true)); quietP = append(quietP, mk(i, false)) } payloadFor := func(seq int) []byte { if seq%175 < 125 { return talkP[seq%50] }; return quietP[seq%50] } dec, _ := opus.NewDecoder(rate, channels) - jb := newJitterBuffer(dec) + jb := newJitterBuffer(dec, 111) r := rand.New(rand.NewSource(1)) const dur = 3 * 3600 * 1000.0 type ev struct{ at float64; seq uint16 } diff --git a/test/pad_test.go.txt b/test/pad_test.go.txt new file mode 100644 index 0000000..dc6ba88 --- /dev/null +++ b/test/pad_test.go.txt @@ -0,0 +1,44 @@ +package main + +import ( + "fmt" + "math" + "testing" + + "github.com/pion/rtp" + "gopkg.in/hraban/opus.v2" +) + +func TestPadding(t *testing.T) { + for _, name := range []string{"empty payload", "other payload type"} { + enc, _ := opus.NewEncoder(rate, channels, opus.AppAudio) + dec, _ := opus.NewDecoder(rate, channels) + jb := newJitterBuffer(dec, 111) + pcm := make([]int16, frameSamps*channels) + buf := make([]byte, 4000) + seq, ts := uint16(0), 0.0 + frames := 0 + // 10 minutes of playout; audio packet every 20 ms, plus a padding packet every 40 seqs. + for tms := 0.0; tms < 600000; tms += 20 { + for ts <= tms { + if seq%40 == 39 { + p := &rtp.Packet{Header: rtp.Header{SequenceNumber: seq, PayloadType: 111}} + if name != "empty payload" { + p.PayloadType, p.Payload = 13, []byte{0x40} + } + jb.push(p) + seq++ + continue + } + for k := range pcm { pcm[k] = int16(6000 * math.Sin(float64(frames*frameSamps+k/2)*2*math.Pi*220/rate)) } + n, _ := enc.Encode(pcm, buf) + jb.push(&rtp.Packet{Header: rtp.Header{SequenceNumber: seq, PayloadType: 111}, Payload: append([]byte(nil), buf[:n]...)}) + seq++; frames++; ts += 20 + } + jb.next() + } + c, d, tg := jb.snapshot() + fmt.Printf("%-20s padding=%d lost=%d fec=%d concealed=%d drift-drops=%d inserted=%d underruns=%d buffer=%d/%dms\n", name, c.Padding, c.Lost, c.FEC, c.PLC, c.Dropped, c.Inserted, c.Underruns, d, tg) + if c.Lost+c.PLC+c.Dropped+c.Underruns != 0 { t.Errorf("%s: padding still causes damage", name) } + } +}