Repository navigation
fix: RTCP-only SSRC could be mistaken for a new RTP audio source, unnecessarily stopping the DTX timer (was: avoid input DTX buffering for permanent audio mixers) - #2192
Conversation
9f8c168 to
4f655e5
Compare
|
The new clock-continuity tests exposed a problem in recovery from long signalling or scheduler stalls. The mixer’s existing catch-up mechanism can generate enough packets to exceed the encoder’s one-second send-timer window. Scheduling then resets while older packets remain queued, causing sequence numbers and timestamps to interleave within the same SSRC. This change prevents that catch-up burst by skipping overdue timer ticks. It fixes the reproduced failure—96 continuity failures before, zero afterward—but changes stall-recovery behaviour, so it needs review for buffered-audio latency and media-clock alignment during continuous speech and playback. The test also now drains idle media from both endpoints to avoid artificial backpressure in the harness. |
| if (ch->handler->mixer_clock && ch->handler->packet_decoded == packet_decoded_audio_player) | ||
| return false; |
There was a problem hiding this comment.
In general I think this is a good idea, although I'm wondering about non-silence DTX mechanisms. We support filling in DTX gaps with comfort noise or even decoder-specific PCM, and this would bypass it and leave only silence.
| // A burst can exceed the encoder's send-timer window and interleave queued and new packets. | ||
| int64_t now = now_us(); | ||
| int64_t late = now - mp->next_run; | ||
| if (late >= ap->ptime_us) { | ||
| mp->next_run += late / ap->ptime_us * ap->ptime_us; | ||
| rtpe_now = mp->next_run; | ||
| } | ||
|
|
There was a problem hiding this comment.
If you see output packets interleaved and/or in the wrong order, I'd say that indicates some other underlying problem. The existing send_timer etc mechanisms are supposed to take care of packet order. This looks like a hackish workaround that only hides the actual issue.
There was a problem hiding this comment.
Hey @rfuchs!
First of all, I'd link to this https://twitter.com/MarkSimmonsSays/status/684030873734377473
Because, unfortunately, I have no deep knowledge myself about DTX and all these (low level?) codec issues. What I did was that I built a demo where I had a bunch of AMR phones (so mobile phones really) in a mesh-mode "conference", where there is a way to control who is talking at some moment. I also have a bunch of WebRTC participants and we're also playing back some wav files in there. And I had a few small but annoying issues, where the media didn't seem to be distributed properly. Hence I had my Codex agent look into it and try to do small fixes, to get it working properly. I didn't want it to rewrite half of it, for obvious reasons. Classic "works-for-me", I guess 😝...
Some of the PRs, which you have merged already, seemed also to me, like to you, as obvious bugs. Here though it might've discovered something deeper.
I don't think that there were issues with packet reordering. That would've been much easier to spot, or deal with maybe. But genuinely, with 2 phones listening, one was falling behind in audio playback versus the other. And the fixes seemed to work.
I was scratching my brain about how to approach this... I probably don't have the intelligence to get myself smart enough quickly, to understand this DTX thing well enough to help you... I was thinking that maybe we should make a give-agent-tokens to open-source project system, to help maintainers deal with this easier 😉 ? So I'm open for suggestions, including asking my Codex to go deeper, but for that I would_ really need your advise on what exactly should we aim for here.
Cheers!
There was a problem hiding this comment.
If you experience an issue with the code and are not sure about how to fix it, or are not able to for whatever other reason, the best way forward is to make a detailed bug report, which includes all debugging information necessary to reliably reproduce the problem on the maintainer's system.
I understand that you're trying to be helpful by pointing an LLM agent at the problem and telling it to generate a fix, and it may even give you something that makes the issue disappear for you, but the problem is that 1) if you yourself don't understand the underlying causes, you cannot know whether the generated fix is actually a fix, or if perhaps it just masks some other underlying problem, or even if it breaks some other things, and 2) I as the maintainer am also not able to verify the fix, since I have not seen the original problem and have no way to reproduce the behaviour.
Giving even more tokens to an LLM would not solve any of this.
In this particular context, the generated comment in the code makes claims about interleaved queued and new packets, yet you say you haven't actually seen this. Is the maintainer supposed to simply blindly believe what the LLM has spewed out and accept the patch?
There was a problem hiding this comment.
Hey Richard,
look, in general, bug reporting and waiting for someone to have time to look at it rarely works. I'm not talking about rtpengine, but in general. Trying to help myself and making a proof-of-concept small fix was/is much more practical for me, rather than being blocked days/weeks/months and never actually figuring out if this would even work for me. Iterating and retrying allowed me to successfully build the demo and now I know: it works, given a few things are patched.
Of course, this fix might be wrong and I've never claimed otherwise, or pushed you to accept it blindly.
On your points:
-
I think Codex got a reasonable hold of the problem and the result seemed considerably better. It might not be perfect and, again, giving you a crazy big rewrite might've not helped either. I wanted to highlight the issue and patch it reasonably enough. Of course, there will always be someone affected by this and any other change, even if it fixes all bugs.
-
Doesn't the added test reproduce the issue? I'll have Codex running and generating a full bug report as you asked. But I don't want to waste your time, in case just running the test on master exposes it better for you, maybe (?).
(Or maybe the test is bad?)
Of course you are not meant to blindly believe anyone, I sure hope you don't, of course. If you reject the PR, that's fine for both me and my agent, you won't hurt my feelings and my Clanker says it does not have feelings 😝. That's why the test is there.
If now it's not the time to fix or break this, that's fine by me too; hope I did my part by raising it, such that next time you see it, you'll correlate and it would hopefully be easier to mark it as a bug. Or, if you could guide me better, Codex and myself can spend some tokens and hours on it.
I can also run my patched fork for my demos just fine. I'm just trying to make THE rtpengine better.
P.S. I meant interleaved IP packets on the network, sorry about the confusion.
P.P.S. I understand AI-fatigue, but I was just trying to help and be sincere and direct for it. The gates of hell seem to have opened now that, finally, Open Source delivers on the promise that everyone can contribute, not just a chosen few, which still remember or don't have PTSD from Perl 🙃. Honestly, without an agent, I would've never had the courage to approach a new test like that myself.
Cheers,
-Dragos
There was a problem hiding this comment.
The test seems insufficient to me now. So I'll try to to improve it then.
There was a problem hiding this comment.
It's fine if you want to help fix a bug, and it is in fact appreciated, but you cannot expect the maintainer to accept the patch without a detailed description of what the patch is actually supposed to fix. And I don't mean an LLM-waffled "analysis" or summary of whatever you were seeing, but rather an actual bug report with actual log messages and actual packet traces and actual raw data.
I think Codex got a reasonable hold of the problem
Great if that's what you believe, but your job, if you want this patch to be accepted, is to convince me that this actually is the case. If you can't answer a question as to why a particular change in the code was made, then that does not help.
Doesn't the added test reproduce the issue?
Well, I don't know, because I haven't seen the actual issue.
I'll have Codex running and generating a full bug report as you asked.
Please don't. See above.
But I don't want to waste your time, in case just running the test on master exposes it better for you, maybe (?).
Me having to go through all of this is exactly what is wasting my time.
There was a problem hiding this comment.
fair enough - yet I did use the agent to continue investigation and tests, with the interesting results from below.
013eaf1 to
bf225c9
Compare
|
The test was definitely not fully covering my problem, more showing the DTX contributing factor. After replacing that test with one that measures audio-content delay, I could reproduce growing latency on unpatched master with DTX enabled. The initial approach bypassed input DTX processing for permanent mixers. Your comfort-noise comment exposed the problem with that approach: it also bypassed noise generation. Investigating further uncovered a more specific cause: an RTCP-only SSRC could be mistaken for a new RTP audio source, unnecessarily stopping the DTX timer. Repeated restarts added delay. Therefore removed the bypass and replaced it with a much smaller change that excludes report-only SSRCs when checking for an audio-source change, preserving DTX and comfort noise. 
Then, since I was seeing much larger latency, in the order of seconds, even though unfortunately I have lost my old pcaps, managed to find some agent logs and changed the test to replicate my situation more closely. The yet-again updated test also reconstructs admission, private prompts and long silence. Over 10m40s, unpatched master reached 2.96 seconds of delay, while the fixed build peaked at 169 ms. 
(the gap in the graphs show a 20s silence interval) Pushed the changes - does the bug makes sense now? |
vingarzan
changed the title
That does in fact make a lot more sense. I still have to wonder what kind of RTCP reports you're seeing that is triggering this. Is it reports "from" a not-sending SSRC, or is it reports "about" a not-sending SSRC? Providing actual concrete evidence would have allowed me to answer this, but it doesn't really matter. Looks like both kinds would lead to the same result. You may want to look into why you're getting those reports though. Re the proposed fix, I think it would be preferable to use a version of |
|
For context, the original deployment uses standard iPhones and other mobile devices with stock IMS dialers over VoLTE/VoWiFi/VoNR. Their audio legs remain send-enabled, with DTX during silence. Updated as suggested. Unfortunately, this change is deeper then, in lib, so hope this is fine with you, else I won't mind if you change it, or if you'd remove some of the new test fixtures and reduce the tests. Here's the new test results - the before-red/after-blue are a bit changed versus the ones above, I want to put them on the same graph: |
Permanent audio players already generate continuous mixed output and fill silent intervals. When
--dtx-delayisenabled, their input decoders also queue RTP behind an independent DTX timer. In a conference with AMR-WB phones, this can accumulate different speech delays for each source despite continuous outgoing RTP timestamps. A captured call showed approximately 0.68 seconds of engine-local delay for one phone and 2.58 seconds for the other.
Skip input DTX buffering when the decoder feeds a permanent audio player. Record clock ownership in the codec handler and recreate the decoder and its DTX queue when ownership changes. Explicit permanent selection must also override an earlier implicit player and implicit daemon defaults. Keep existing DTX handling for ordinary transcoding and implicitly enabled audio players; disabling DTX globally would affect unrelated media paths.
Add a real-RTP regression with PCMU input, a mono L16/16000 listener, and an observable 200 ms DTX delay. Reconfigure one call and SSRC between permanent, implicit, and ordinary transcoding, checking prompt speech in permanent mode, retained DTX delay in the other modes, and continuous mixer timestamps. Run it under default, transcoding, and play-media daemon settings.