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

Add support for TCP interleaved clients #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slyoldfox
Copy link
Contributor

Just polling how you feel about this PR. After my last commit, looked into TCP support and it seems like adding support for TCP/interleaved clients was reasonably straightforward.

This does depend on an open PR watson/rtsp-stream#7 which filters interleaved packets from the rtsp-stream Decoder. A TEARDOWN request would error if the interleaved packets weren't filtered out for the Decoder.

The commit which fixes this (watson/rtsp-stream@7971c99) has been around since 2018, I just kicked off the PR to see if @watson merges it.

I have created a forked version 1.0.1 which incorporates that commit:
https://www.npmjs.com/package/@slyoldfox/rtsp-stream

This also explain the need for the overrides inside the package.json inside this PR.

The whole thing has been forked in a 2.1.0-interleaved version at https://www.npmjs.com/package/@slyoldfox/rtsp-streaming-server?activeTab=versions in case the PRs don't come through.

Been testing this for a little while for my project (https://github.com/slyoldfox/c300x-controller) and it seems like interleaving successfully works through a NAT port-forwarding setup for my doorbell.

@chriswiggins
Copy link
Owner

Nice one! I didn't realise that interleaved literally only added a single magic byte.

Let me look at this over the next couple of days and will get back to you

@slyoldfox
Copy link
Contributor Author

Sure thing, some more comments from my side which I wasn't too sure about:

I didn't opt to create a super class (RtpClient e.g.) at this point for Client and InterleavedTcpClient - because they do share some logic like id, stream and mount - because I didn't want to change the signature of the Client.
That's why I opted for the duplication and clear seperation of logic into two classes.
Also didn't rename Client to UdpClient at this point, which might make it more clear, for the same reason.

Also (not sure about this one) this.socket is assigned in the setup(req) method. I haven't checked what the RFC says about socket reuse and if for example a play() request can come from a different socket (with the same session) than from a setup() request.
If that's the case, we should pass RtspRequest to client.play(req) and assign the socket in the play(req) method of the InterleavedTcpClient - and thus changing the signature of play() on Client as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants