Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RtspSession.update does not update _sessionControlURL #1077

Open
selimb opened this issue Jan 3, 2025 · 3 comments · May be fixed by #1083
Open

RtspSession.update does not update _sessionControlURL #1077

selimb opened this issue Jan 3, 2025 · 3 comments · May be fixed by #1083
Assignees
Labels
bug Something isn't working

Comments

@selimb
Copy link

selimb commented Jan 3, 2025

Describe the bug

I am trying to seek to different times without tearing down and setting up RTSP sessions, and instead reuse the existing one. In theory the RTSP protocol should allow this (as far as I know), and I can almost do it with this library. The problem is that calling .update on RtspSession does not update _sessionControlURL, which is what is used to send SDP commands.

To Reproduce

  1. Apply the following git patch on this repo:
--- a/streams/tests/rtsp-session.test.ts
+++ b/streams/tests/rtsp-session.test.ts
@@ -78,10 +78,12 @@ describe('rtsp-session send method', (test) => {
 
   test('should use the supplied URI', async (ctx) => {
     const uri = 'rtsp://whatever/path'
+    const uri2 = 'rtsp://new/path'
     const s = new RtspSession({ uri })
+    s.update(uri2)
     const done = new Promise((resolve) => (ctx.resolve = resolve))
     s.outgoing.once('data', (req) => {
-      assert.is(req.uri, uri)
+      assert.is(req.uri, uri2)
       ctx.resolve()
     })
     s.send({ method: RTSP_METHOD.DESCRIBE })
  1. Run just test

This outputs:

   FAIL  rtsp-session send method  "should use the supplied URI"
    Expected values to be strictly equal:  (is)

        ++rtsp://new/path         (Expected)
        --rtsp://whatever/path    (Actual)

Environment:

  • OS: Linux
  • Browser: Firefox
  • Version: 13.1.1

Possible solution

I was able to reuse an RTSP session by doing something like this in my code:

// this._pipeline is a `Html5VideoPipeline`
this._pipeline.rtsp.stop();
const uriNew = ...;
this._pipeline.rtsp.update(uriNew);
this._pipeline.rtsp._sessionControlURL = uriNew;
this._pipeline.rtsp.play();

I'm not 100% sure what .update should be used for since it's not used anywhere in this repo, but AFAIK this can be fixed by simply adding the following at the end of the .update implementation:

    this._sessionControlURL = this.uri;

It's probably only safe to do this if the _state = STATE.IDLE though?

I'm happy to work on a PR for this.

@selimb selimb added the bug Something isn't working label Jan 3, 2025
@steabert
Copy link
Member

steabert commented Jan 7, 2025

Note: there are some up-and-coming major changes to this, so I suggest you have a look at the web-streams branch if this is still an issue! The RTSP session has been completely reworked to work more like a request-response flow, and should be easier to understand and modify.

Re-use of the same stream for different sessions is supported, but not well-tested (usually we don't reuse a pipeline), so we welcome any fixes there.

@selimb
Copy link
Author

selimb commented Jan 7, 2025

I'll have a look at the web-streams branch! Thanks.

(usually we don't reuse a pipeline)

Is there any reason why? As far as I can tell it should lead to a much snappier experience, no?

steabert added a commit that referenced this issue Jan 11, 2025
Adds a URI option to the describe method in the RTSP session.
If provided, it's used for that session instead of the URI that
was provided to the constructor. The latter now only serves as
default URI when none is provided to describe.
On teardown, the session control URL is restored to the default.

Fixes #1077
@steabert
Copy link
Member

steabert commented Jan 11, 2025

I'll have a look at the web-streams branch! Thanks.

I've added a feature/fix for this in the linked PR, would that work for you?

(usually we don't reuse a pipeline)

Is there any reason why? As far as I can tell it should lead to a much snappier experience, no?

When setting up a new session with a new URI is expensive already, so just starting a new pipeline isn't really problematic (that is why the URI was part of the constructor). The fix I added allows you to override that URI later on, and only treats the original as the default. However, you stated at the beginning that you want to seek to different times, you shouldn't need to change the URI?

Note that play doesn't allow you to pick a particular stream (it always streams the whole session).

@steabert steabert linked a pull request Jan 11, 2025 that will close this issue
@steabert steabert self-assigned this Jan 11, 2025
steabert added a commit that referenced this issue Jan 13, 2025
Adds a URI option to the describe method in the RTSP session.
If provided, it's used for that session instead of the URI that
was provided to the constructor. The latter now only serves as
default URI when none is provided to describe.
On teardown, the session control URL is restored to the default.

Fixes #1077
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants