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

Making the timeout parameters for streaming plugin for RTSP play out configurable #2598

Merged
merged 11 commits into from
Mar 25, 2021

Conversation

pontscho
Copy link
Contributor

I found that, the media server which serve stream on RTSP is a little bit buggy and can't possible to change the session timeout for the audio/video stream the connection checking waits too much time. And the timeouts for cURL was also hardcoded. This patch makes this values configurable by both the config file and the controller API.

…ection/communication timeout, no media timeout and reconnection delay.
@januscla
Copy link

Thanks for your contribution, @pontscho! Please make sure you sign our CLA, as it's a required step before we can merge this.

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I added some comments below.

conf/janus.plugin.streaming.jcfg.sample.in Outdated Show resolved Hide resolved
plugins/janus_streaming.c Outdated Show resolved Hide resolved
plugins/janus_streaming.c Outdated Show resolved Hide resolved
plugins/janus_streaming.c Outdated Show resolved Hide resolved
plugins/janus_streaming.c Show resolved Hide resolved
plugins/janus_streaming.c Outdated Show resolved Hide resolved
conf/janus.plugin.streaming.jcfg.sample.in Show resolved Hide resolved
conf/janus.plugin.streaming.jcfg.sample.in Show resolved Hide resolved
conf/janus.plugin.streaming.jcfg.sample.in Show resolved Hide resolved
conf/janus.plugin.streaming.jcfg.sample.in Show resolved Hide resolved
@lminiero
Copy link
Member

The code needs fixing too: the automated build failed here,

plugins/janus_streaming.c: In function ‘janus_streaming_create_rtp_source’:
plugins/janus_streaming.c:6057:17: error: ‘janus_streaming_rtp_source’ {aka ‘struct janus_streaming_rtp_source’} has no member named ‘ka_timeout’
 6057 |  live_rtp_source->ka_timeout = 0;

which probably means you're not using #ifdef HAVE_LIBCURL where you should.

@pontscho
Copy link
Contributor Author

The code needs fixing too: the automated build failed here,

plugins/janus_streaming.c: In function ‘janus_streaming_create_rtp_source’:
plugins/janus_streaming.c:6057:17: error: ‘janus_streaming_rtp_source’ {aka ‘struct janus_streaming_rtp_source’} has no member named ‘ka_timeout’
 6057 |  live_rtp_source->ka_timeout = 0;

which probably means you're not using #ifdef HAVE_LIBCURL where you should.

Yes, you right, I forget to compile this thing without curl.

plugins/janus_streaming.c Outdated Show resolved Hide resolved
plugins/janus_streaming.c Outdated Show resolved Hide resolved
plugins/janus_streaming.c Outdated Show resolved Hide resolved
plugins/janus_streaming.c Outdated Show resolved Hide resolved
Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fixes! I added a couple on notes inline, that are just related to style though, so nothing important. I plan to make some tests with my RTSP camera later today to see if I notice regressions or all still works 👍

plugins/janus_streaming.c Show resolved Hide resolved
plugins/janus_streaming.c Outdated Show resolved Hide resolved
# Connection timeout for cURL call gathering the RTSP information.
#
# Default value is 5 seconds.
# rtsp_conn_timeout = 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the stuff you added to the rtsp-test sample can be removed, as it's already documented on top.

Copy link
Contributor Author

@pontscho pontscho Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

S***. I've added a wrong config file earlier and I lost in that changes. I've fixed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem! 😄 Thanks for the quick fixes!

@lminiero
Copy link
Member

Works for me, merging, thanks again! 👍

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.

4 participants