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 helper threads to RTSP mountpoints (fixes #2359) #2361

Merged
merged 1 commit into from
Sep 25, 2020

Conversation

lminiero
Copy link
Member

As the title says, this PR is supposed to extend the existing helper threads in the Streaming plugin to RTSP mountpoints as well, rather than just RTP. Both mountpoint types actually work the same way behind the curtains, as far as RTP is concerned, so what was missing was only a way to configure helpers when creating RTSP mountpoints in the first place. Not tested yet, so feedback welcome.

@jreich888
Copy link

Hi Lorenzo, haven't tried the code yet, but I noticed three places where the helper threads are used but have a check for RTP type first. I'm not sure if they are critical or not, or if they are unrelated to the RTSP case. They seem to be too far from the diffs to make a comment in the github interface, but please take a look around lines 3794, 4517, and 4928. Not sure if there are others.

@lminiero
Copy link
Member Author

RTSP mountpoints are RTP sources, so it will work.

@lminiero
Copy link
Member Author

Made a quick test with a gstreamer-based RTSP server and it seems to be working as expected. Looking forward to your feedback before we merge.

@jreich888
Copy link

Hi Lorenzo, I merged it here and gave it a try. Ran it for a few hours, was able to confirm with ps that the threads were created, and everything looked good. However, I was also running with larger socket buffers, so not conclusive what helped more, but didn't see any problems. Did lose a couple dozen packets over the few hours, but that is orders of magnitude less than what I was seeing before. Overall, looks good, thanks for the quick response on this request!

@lminiero
Copy link
Member Author

@jreich888 any update on this?

@lminiero
Copy link
Member Author

Whoops, sorry, I must have missed your comment: didn't get a notification about that, so sorry for the ping. Merging then 👍

@lminiero lminiero merged commit bf0a59a into master Sep 25, 2020
@lminiero lminiero deleted the rtsp-helpers branch September 25, 2020 14:56
@jreich888
Copy link

Whoops, sorry, I must have missed your comment: didn't get a notification about that, so sorry for the ping. Merging then 👍

No worries, and thanks again for making this change so quickly. 👍

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.

None yet

2 participants