| Summary: | ASTERISK-28890: res_pjsip_sdp_rtp: Keepalive not supported for video streams | ||||||
| Reporter: | Luke Escude (lukeescude) | Labels: | patch | ||||
| Date Opened: | 2020-05-12 22:57:35 | Date Closed: | 2022-01-20 11:26:56.000-0600 | ||||
| Priority: | Minor | Regression? | |||||
| Status: | Closed/Complete | Components: | Resources/res_pjsip_sdp_rtp | ||||
| Versions: | 17.4.0 | Frequency of Occurrence | |||||
| Related Issues: |
| ||||||
| Environment: | Attachments: | ( 0) video_nat.patch | |||||
| Description: | So I've noticed that RTP Keepalives work just fine for audio, but they don't function for video.
Here are the changes needed to be made for keepalive packets to function for video: In res/res_pjsip_sdp_rtp.c: Change the following (Line 2032): if (media_type != AST_MEDIA_TYPE_AUDIO) { to: if (media_type != AST_MEDIA_TYPE_AUDIO && media_type != AST_MEDIA_TYPE_VIDEO) { ALSO Change the following (Line 2061): if (session->endpoint->media.rtp.keepalive > 0 && session_media->type == AST_MEDIA_TYPE_AUDIO) { to if (session->endpoint->media.rtp.keepalive > 0 && (session_media->type == AST_MEDIA_TYPE_AUDIO || session_media->type == AST_MEDIA_TYPE_VIDEO)) { Sorry guys, I don't know how to make a patch... But these 2 line changes will fix dysfunctional keepalives for video. | ||||||
| Comments: | By: Asterisk Team (asteriskteam) 2020-05-12 22:57:36.235-0500 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. Please note that once your issue enters an open state it has been accepted. As Asterisk is an open source project there is no guarantee or timeframe on when your issue will be looked into. If you need expedient resolution you will need to find and pay a suitable developer. Asking for an update on your issue will not yield any progress on it and will not result in a response. All updates are posted to the issue when they occur. By: Joshua C. Colp (jcolp) 2020-05-13 04:23:42.081-0500 You already previously filed an issue for this, and an individual posted a patch on it. Noone has taken it through the code review process for inclusion yet though, so that issue remains open. By: Luke Escude (lukeescude) 2020-05-13 07:50:10.456-0500 Joshua, Wow completely forgot about that one... is it OK to close that old one and retain this new bug filing? My former engineer’s personal email is attached to it and I don’t want him getting blown up with emails on that old issue. By: Asterisk Team (asteriskteam) 2020-05-13 07:50:10.893-0500 This issue has been reopened as a result of your commenting on it as the reporter. It will be triaged once again as applicable. By: Joshua C. Colp (jcolp) 2020-05-13 07:52:18.871-0500 Done. By: Luke Escude (lukeescude) 2020-05-13 07:55:23.487-0500 Thank you sir! Additionally, I can confirm the keepalive patch works and is stable, we’ve been running it in production for the last few years across multiple asterisk versions. Our original patch file is incorrect of course since the line numbers are off, but making the appropriate changes to the line numbers solves it. I’ll see if I can re-learn patch syntax and post it, haven’t made one in a long time. By: Luke Escude (lukeescude) 2021-06-28 19:40:16.982-0500 Do you guys think this patch could make it into a release soon? By: Joshua C. Colp (jcolp) 2021-06-29 03:53:29.126-0500 All patches go through code review. Until someone takes it through that process, it will not. By: Luke Escude (lukeescude) 2021-06-29 06:51:41.847-0500 Ah gotcha. Is that a difficult process to learn? By: Joshua C. Colp (jcolp) 2021-06-29 06:57:43.125-0500 It's documented on the wiki[1][2]. [1] https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process [2] https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage By: Luke Escude (lukeescude) 2021-06-29 09:09:57.762-0500 Hey folks, here's the official patch file - This works as of Asterisk 16.14 I will read up on the gerrit process and attempt to submit it officially. To recap, this fixes keepalives for video calls. Keepalives work just fine for audio, but they don't exist for video, which isn't ideal if you need Asterisk to punch through NAT like we do (all our PBX systems are behind their own NAT, and all customer endpoints are behind NAT). By: Luke Escude (lukeescude) 2022-01-19 16:27:58.263-0600 Quick update on this ticket, we've been running the patch I submitted previously for years now, and it successfully handles video NAT punchthrough, so the patch is definitely stable. I don't have the time to figure out how to formally submit patches to the Asterisk project, so I'll reach out to someone to submit it. By: Sean Bright (seanbright) 2022-01-19 16:37:52.716-0600 It's up on gerrit now By: Luke Escude (lukeescude) 2022-01-19 16:40:52.453-0600 Oh thank you so much!! By: Friendly Automation (friendly-automation) 2022-01-20 11:26:57.550-0600 Change 17899 merged by Joshua Colp: res_pjsip_sdp_rtp.c: Support keepalive for video streams. [https://gerrit.asterisk.org/c/asterisk/+/17899|https://gerrit.asterisk.org/c/asterisk/+/17899] By: Friendly Automation (friendly-automation) 2022-01-20 11:30:07.579-0600 Change 17898 merged by Joshua Colp: res_pjsip_sdp_rtp.c: Support keepalive for video streams. [https://gerrit.asterisk.org/c/asterisk/+/17898|https://gerrit.asterisk.org/c/asterisk/+/17898] By: Friendly Automation (friendly-automation) 2022-01-20 11:30:24.102-0600 Change 17897 merged by Joshua Colp: res_pjsip_sdp_rtp.c: Support keepalive for video streams. [https://gerrit.asterisk.org/c/asterisk/+/17897|https://gerrit.asterisk.org/c/asterisk/+/17897] By: Friendly Automation (friendly-automation) 2022-01-20 11:30:45.416-0600 Change 17911 merged by Joshua Colp: res_pjsip_sdp_rtp.c: Support keepalive for video streams. [https://gerrit.asterisk.org/c/asterisk/+/17911|https://gerrit.asterisk.org/c/asterisk/+/17911] | ||||||