fix(rtsp): echo OPTIONS Session header in DESCRIBE#523
Conversation
HMS edge nodes assign a session during OPTIONS and drop the connection when DESCRIBE omits it. Add an E2E mock and regression test. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep both IPv6 host support from main and HMS OPTIONS Session mock. Co-authored-by: Cursor <cursoragent@cursor.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a069424a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (session->session_id[0] != '\0') { | ||
| snprintf(extra_headers, sizeof(extra_headers), "Accept: application/sdp\r\nSession: %s\r\n", session->session_id); | ||
| } else { |
There was a problem hiding this comment.
When an OPTIONS response establishes session->session_id, this path now echoes it only on DESCRIBE, but the subsequent SETUP request is still constructed with just Transport: in the RTSP_STATE_DESCRIBED branch. HMS-style servers that require the OPTIONS-issued Session to be echoed after DESCRIBE will still reject or close on SETUP, and the new mock does not catch this because it only enforces the header on DESCRIBE.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5c7db74. Session injection is now centralized in rtsp_prepare_request(), so every request after the session is established (DESCRIBE, SETUP, PLAY, keepalive, TEARDOWN) echoes it, per RFC 2326 §12.37. The mock now also enforces the Session echo on all post-OPTIONS requests, and the e2e test asserts DESCRIBE/SETUP/PLAY all carry it.
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://thankful-water-0a297bf00-523.eastasia.1.azurestaticapps.net |
There was a problem hiding this comment.
Pull request overview
This PR updates the RTSP client state machine to echo a Session header learned during the initial OPTIONS exchange in the subsequent DESCRIBE request, improving compatibility with HMS-style RTSP edge nodes that establish a session before DESCRIBE. It also extends the E2E mock RTSP server to simulate this behavior and adds a regression test.
Changes:
- Echo
SessionfromOPTIONS(if present) in the followingDESCRIBErequest (src/rtsp.c). - Extend the mock RTSP server to optionally return a
SessioninOPTIONSand require it inDESCRIBE(e2e/helpers/mock_rtsp.py). - Add an E2E regression test covering the OPTIONS→DESCRIBE Session echo (
e2e/test_rtsp_transport.py).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/rtsp.c | Adds conditional inclusion of Session header when preparing DESCRIBE after OPTIONS. |
| e2e/helpers/mock_rtsp.py | Adds options_session_id mode to simulate servers assigning session at OPTIONS time. |
| e2e/test_rtsp_transport.py | Adds regression test asserting DESCRIBE echoes the Session value from OPTIONS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (session->session_id[0] != '\0') { | ||
| snprintf(extra_headers, sizeof(extra_headers), "Accept: application/sdp\r\nSession: %s\r\n", session->session_id); | ||
| } else { |
There was a problem hiding this comment.
Good catch — fixed in 5c7db74. rtsp_handle_redirect() now clears session_id before connecting to the new server, so a session learned from the previous server is never echoed to the redirected-to server.
| "RTSP/1.0 200 OK\r\nCSeq: %s\r\n" | ||
| "Public: OPTIONS, DESCRIBE, SETUP, PLAY, TEARDOWN\r\n\r\n" % cseq | ||
| "%s" | ||
| "Public: DESCRIBE, SETUP, TEARDOWN, PLAY, PAUSE\r\n\r\n" % (cseq, session_line) |
There was a problem hiding this comment.
Fixed in 5c7db74 — restored OPTIONS in the mock's Public header (back to the original list).
Address PR review feedback: - Inject the Session header in rtsp_prepare_request() whenever a session is established, so SETUP (and every other request) also echoes a Session assigned at OPTIONS time, per RFC 2326 section 12.37 - Clear stale session_id on redirect so requests to the redirected-to server don't carry the previous server's session - Mock: restore OPTIONS in the Public header and enforce the Session echo on all post-OPTIONS requests, not just DESCRIBE Co-authored-by: Cursor <cursoragent@cursor.com>
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://thankful-water-0a297bf00-523.eastasia.1.azurestaticapps.net |
Summary
Sessionheader from OPTIONS into the subsequent DESCRIBE request, fixing playback against HMS-style RTSP edge nodes that assign a session before DESCRIBE.options_session_idmode and add a regression test intest_rtsp_transport.py.Fixes #502 (RTSP unicast failure on HMS edge nodes).
Test plan
uv run pytest e2e/test_rtsp_transport.py::TestRTSPTCPStream::test_describe_echoes_options_session -vMade with Cursor