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

First attempt at implementing wildcard paths #52

Closed
wants to merge 1 commit into from

Conversation

xieliwei
Copy link
Contributor

@xieliwei xieliwei commented Aug 2, 2020

Apologies for the duplicate earlier on, should have been less jumpy about opening an issue!

First attempt at getting this working, based on my limited understanding of RTSP and the code structure.

This patch introduces a new configuration item: isWildcard, that causes the configured path to be matched against any path that begins with it. The runOnInit/runOnDemand/runOnPublish/runOnRead callers have also been modified to set an environment variable RTSP_SERVER_PATH with the calling path. (For runOnInit, it will be called with the configured path, of course)

findConfForPath() has been modified to return an additional string - the configured path that was matched. It isn't necessary at the moment (only used to print out logs), but might be useful to have.

There are some doubts I have:

  • Possibly incorrect behaviour if isWildcard is set for the 'all' path
  • Unsure if there will be odd interactions with sourceOnDemand, thus currently there is a check to make sure that sourceOnDemand is off when isWildcard is on
  • It seems like it should be sufficient to handle adding the dynamic path during the describe event, but I may be wrong
  • There is a check to only destroy the dynamic path when both the producer and consumers are disconnected, but it possibly does not handle multiple clients connecting to the same path correctly
  • I have only tested this for my use case, which is with dynamically generated recorder streams, and only one client per path
  • The environment variable name should probably be a configuration item, and it should be possible to turn the feature off

Do let me know how I can improve the patch for acceptance, thanks!

(Addresses #47 )

@aler9
Copy link
Member

aler9 commented Aug 3, 2020

Hello, first of all thanks for your contribution.
If possible, i'd split the work in two separate PRs, since you're addressing two separate features:

  • one for wildcards in paths
  • one for variables in commands. Environment variables are OK, since they also allow writing scripts in one line like this:
runOnDemand: ffmpeg -i ... -f rtsp rtsp://localhost:8554/$RTSP_SERVER_PATH

@xieliwei
Copy link
Contributor Author

xieliwei commented Aug 4, 2020

Done, I've opened two new pull requests. If they're okay, do proceed to close this one.

(Unrelated to this, the latest changes to UDP handling seems to have broken something on our streaming setup fed with gstreamer rtspclientsink. I'll try to find time next week to investigate this.)

@aler9 aler9 closed this Aug 5, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

This issue is being locked automatically because it has been closed for more than 6 months.
Please open a new issue in case you encounter a similar problem.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants