Add sip_probe.py: minimal SIP UAC for capturing majestic RTP/RTCP - #1
Open
openipc-ai wants to merge 1 commit into
Open
Add sip_probe.py: minimal SIP UAC for capturing majestic RTP/RTCP#1openipc-ai wants to merge 1 commit into
openipc-ai wants to merge 1 commit into
Conversation
A diagnostic probe that places a P2P SIP call to a majestic camera and captures the RTP + RTCP the camera sends, analyzing RTP timestamp clock rates and RTCP Sender Reports. Used to diagnose the Linphone 6.2.2 SIP call-drop: it showed majestic sends clean RTP but zero RTCP SR on non-Hisi SoCs (widgetii/majestic#398). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DStUrnmn8TqWrobQ7D2wmX
PR Summary by QodoAdd SIP probe tool to capture and analyze Majestic RTP/RTCP (SR/clock rates)
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. RTCP port bind race
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+163
to
+166
| audc = socket.socket(socket.AF_INET, socket.SOCK_DGRAM); audc.bind(("0.0.0.0", aport + 1)) | ||
| vid = socket.socket(socket.AF_INET, socket.SOCK_DGRAM); vid.bind(("0.0.0.0", 0)) | ||
| vport = vid.getsockname()[1] | ||
| vidc = socket.socket(socket.AF_INET, socket.SOCK_DGRAM); vidc.bind(("0.0.0.0", vport + 1)) |
There was a problem hiding this comment.
1. Rtcp port bind race 🐞 Bug ☼ Reliability
sip_probe binds RTP to an ephemeral port and then binds RTCP to (RTP+1) without reserving the pair, so the RTCP bind can fail if that adjacent port is already in use, crashing the probe before the call. This also makes the probe flaky across runs/hosts because the port selection is nondeterministic and not validated/retried.
Agent Prompt
### Issue description
`sip_probe.py` binds RTP sockets to port 0 (ephemeral) and then immediately binds RTCP sockets to `rtp_port + 1`. The `+1` port is not guaranteed to be free, so the bind can fail and crash the script.
### Issue Context
This probe is intended for quick diagnosis; failing nondeterministically due to port collisions will waste time and produce confusing failures.
### Fix Focus Areas
- harness/sip_probe.py[155-169]
### Suggested direction
- Implement a small helper to allocate a port pair by trying an even base port and binding both RTP and RTCP in a retry loop (closing any partially-created sockets on failure).
- Alternatively, allow explicit `--audio-port`/`--video-port` and validate that `port+1` is available before sending the SDP.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+231
to
+233
| if not ok_headers: | ||
| L("[probe] no 200 OK — aborting"); return | ||
|
|
There was a problem hiding this comment.
2. Leaked files/sockets on abort 🐞 Bug ☼ Reliability
sip_probe opens log/output files and creates multiple sockets but never closes them; on the early abort path (no 200 OK) it returns immediately, leaking resources. Re-running the probe repeatedly in a harness environment can exhaust file descriptors or leave confusing open handles until process exit.
Agent Prompt
### Issue description
The script opens `sip.log` and `audio_rtp.bin` and creates multiple sockets, but does not close them; an early `return` path leaks resources.
### Issue Context
This is a diagnostic tool likely to be run iteratively. Clean shutdown improves reliability and avoids FD exhaustion in long-lived harness environments.
### Fix Focus Areas
- harness/sip_probe.py[155-233]
- harness/sip_probe.py[248-287]
### Suggested direction
- Wrap sockets/files in `contextlib.ExitStack()` or `try/finally` to ensure all are closed.
- Replace `log = open(...)` / `araw = open(...)` with `with open(...) as ...` (or register them with ExitStack) so the early-abort path also closes them.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+265
to
+266
| except (BlockingIOError, socket.error): | ||
| pass |
There was a problem hiding this comment.
3. Socket errors silently ignored 🐞 Bug ◔ Observability
The media capture loop catches socket.error broadly and ignores it, which can hide real failures (e.g., bad file descriptor, invalid state) and lead to misleading “no RTCP SR” conclusions. This reduces debuggability because the probe will continue running without surfacing the underlying I/O error.
Agent Prompt
### Issue description
The media receive loop catches `socket.error` and discards it. For non-blocking UDP reads, only `BlockingIOError` (and optionally EWOULDBLOCK/EAGAIN) should be ignored; other errors should be logged or raised.
### Issue Context
This probe is used to diagnose subtle RTP/RTCP behavior; silently masking I/O errors can invalidate its conclusions.
### Fix Focus Areas
- harness/sip_probe.py[251-267]
### Suggested direction
- Catch `BlockingIOError` separately.
- For other `OSError`, log `errno` and either continue (if known transient) or abort.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+148
to
+151
| ap.add_argument("--user", default="222") | ||
| ap.add_argument("--password", default="1234") | ||
| ap.add_argument("--duration", type=float, default=12) | ||
| ap.add_argument("--outdir", required=True) |
There was a problem hiding this comment.
4. Password exposed via argv 🐞 Bug ⛨ Security
The probe takes --password as a command-line flag with a default value, which encourages passing credentials via argv where they can be visible to other local users/process tooling. This is avoidable by reading from an env var or prompting when omitted.
Agent Prompt
### Issue description
Credentials are accepted via `--password` (argv) and default to a real-looking value. Passing secrets via argv is commonly observable (process list, shell history, CI logs).
### Issue Context
Even though this is a harness tool, small changes can prevent accidental credential exposure.
### Fix Focus Areas
- harness/sip_probe.py[144-152]
### Suggested direction
- Prefer `--password-env VAR` (defaulting to something like `MAJESTIC_PASSWORD`) or read from `os.environ`.
- If password is not provided, prompt via `getpass.getpass()`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Adds
harness/sip_probe.py, a minimal SIP UAC that places a P2P call to a majestic camera and captures the RTP + RTCP it sends, analyzing RTP timestamp clock rates and RTCP Sender Reports.Used to diagnose the Linphone 6.2.2 (Android) SIP call-drop: the probe showed majestic sends clean RTP (audio 8 kHz, video 90 kHz, zero loss) but zero RTCP Sender Reports on non-Hisilicon SoCs — the trigger for Linphone's adaptive jitter buffer diverging and dropping the call. Filed upstream as widgetii/majestic#398.
🤖 Generated with Claude Code