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

SIP plugin: add missing SIP Contact header in re-INVITE request #3286

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

ycherniavskyi
Copy link
Contributor

Upon transitioning to a newer version of Sofia SIP (version >= 1.13, inferred from the query_contact_header variable initialization comment), we began encountering errors from certain soft-switches, such as "400 Syntax Check For Dialog Failed: ('header', 'contact')" when executing the "hold" request.

During my investigation, I came across #2708 and #2973 which address this type of issue, albeit not comprehensively.

The first commit in this MR rectifies the "hold" request issue. Following that, the second commit applies the same technique to all nua_invite function calls proactively and also carries out minor refactoring to ensure code consistency.

@lminiero
Copy link
Member

lminiero commented Nov 4, 2023

Thanks! I'm not entirely sure about the changes. What I noticed is that we seem to in some cases (correctly) refer to the different purpose of handles (master vs helper), while in others we just assume the handle owns the stack without taking into account it might be a helper (and so not own a SIP stack instance of its own). I guess you're not using helpers and so never encountered a problem, but it's something that would probably need to be taken into account as part of this PR (especially considering you're adding this to more requests).

@ycherniavskyi
Copy link
Contributor Author

Understood.

I propose we divide this PR into two distinct segments:

  1. Inclusion of the Contact header: According to "Table 2: Summary of header fields" in RFC3261 (page 161), the Contact header is a required header for each nua_invite. If this interpretation aligns with your understanding, we can proceed to the next point.
  2. The logic for constructing the contact_header variable differs between nua_subscribe and nua_invite: The current PR adopts the existing approach from nua_invite for consistency. However, if the consensus is to utilize the method of creating contact_header akin to what's used for nua_subscribe, then I will introduce a helper function to harmonize its construction across all usage places.

@ycherniavskyi
Copy link
Contributor Author

@lminiero, could you kindly advise on any further evidence or explanations required to facilitate the acceptance of this PR?

@lminiero
Copy link
Member

lminiero commented Nov 12, 2023 via email

@lminiero
Copy link
Member

Looks good to me! Have you tested this with and without helpers?

@lminiero lminiero added the multistream Related to Janus 1.x label Nov 16, 2023
@evilmuscleman-super
Copy link

111

@lminiero
Copy link
Member

@ycherniavskyi I did a couple of tests with master and helper sessions, using INVITE and hold/unhold, and it seems to be working fine. If you could do some more tests with the other requests you updated, that might help, since my SIP setup is quite limited. Once that's done, I think it will be good to merge for me 👍

@lminiero
Copy link
Member

lminiero commented Dec 6, 2023

I see no replies to my request. Rather than wait forever for feedback, I'll merge, and in case issues are reported, revert the commit. Thanks 👍

@lminiero lminiero merged commit 99a64f2 into meetecho:master Dec 6, 2023
8 checks passed
mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this pull request Dec 8, 2023
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | minor | `v1.1.4` -> `v1.2.1` |

---

### Release Notes

<details>
<summary>meetecho/janus-gateway (meetecho/janus-gateway)</summary>

### [`v1.2.1`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v121---2023-12-06)

[Compare Source](meetecho/janus-gateway@v1.2.0...v1.2.1)

-   Added support for abs-capture-time RTP extension \[[PR-3161](meetecho/janus-gateway#3161)]
-   Fixed truncated label in datachannels (thanks [@&#8203;veeting](https://github.com/veeting)!) \[[PR-3265](meetecho/janus-gateway#3265)]
-   Support larger values for SDP attributes (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3282](meetecho/janus-gateway#3282)]
-   Fixed rare crash in VideoRoom plugin (thanks [@&#8203;tmatth](https://github.com/tmatth)!) \[[PR-3259](meetecho/janus-gateway#3259)]
-   Don't create VideoRoom subscriptions to publisher streams with no associated codecs
-   Added suspend/resume participant API to AudioBridge \[[PR-3301](meetecho/janus-gateway#3301)]
-   Fixed rare crash in AudioBridge
-   Fixed rare crash in Streaming plugin when doing ICE restarts \[[Issue-3288](meetecho/janus-gateway#3288)]
-   Allow SIP and NoSIP plugins to bind media to a specific address (thanks [@&#8203;pawnnail](https://github.com/pawnnail)!) \[[PR-3263](meetecho/janus-gateway#3263)]
-   Removed advertised support for SIP UPDATE in SIP plugin
-   Parse RFC2833 DTMF to custom events in SIP plugin (thanks [@&#8203;ywmoyue](https://github.com/ywmoyue)!) \[[PR-3280](meetecho/janus-gateway#3280)]
-   Fixed missing Contact header in some dialogs in SIP plugin (thanks [@&#8203;ycherniavskyi](https://github.com/ycherniavskyi)!) \[[PR-3286](meetecho/janus-gateway#3286)]
-   Properly set mid when notifying about ended tracks in janus.js
-   Fixed broken restamping in janus-pp-rec
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

### [`v1.2.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v120---2023-08-09)

[Compare Source](meetecho/janus-gateway@v1.1.4...v1.2.0)

-   Added support for VP9/AV1 simulcast, and fixed broken AV1/SVC support \[[PR-3218](meetecho/janus-gateway#3218)]
-   Fixed RTCP out quality stats \[[PR-3228](meetecho/janus-gateway#3228)]
-   Default link quality stats to 100
-   Added support for ICE consent freshness \[[PR-3234](meetecho/janus-gateway#3234)]
-   Fixed rare race condition in VideoRoom \[[PR-3219](meetecho/janus-gateway#3219)] \[[PR-3247](meetecho/janus-gateway#3247)]
-   Use speexdsp's jitter buffer in the AudioBridge \[[PR-3233](meetecho/janus-gateway#3233)]
-   Fixed crash in Streaming plugin on mountpoints with too many streams \[[Issue-3225](meetecho/janus-gateway#3225)]
-   Support for batched configure requests in Streaming plugin (thanks [@&#8203;petarminchev](https://github.com/petarminchev)!) \[[PR-3239](meetecho/janus-gateway#3239)]
-   Fixed rare deadlock in Streamin plugin \[[PR-3250](meetecho/janus-gateway#3250)]
-   Fix simulated leave message for longer string ID rooms in TextRoom (thanks [@&#8203;adnanel](https://github.com/adnanel)!) [PR-3243](meetecho/janus-gateway#3243)]
-   Notify about count of sessions, handles and PeerConnections on a regular basis, when event handlers are enabled \[[PR-3221](meetecho/janus-gateway#3221)]
-   Fixed broken Insertable Streams for recvonly streams when answering in janus.js
-   Added background selector and blur support to Virtual Background demo
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4zNC4wIiwidXBkYXRlZEluVmVyIjoiMzcuODEuNCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/122
Co-authored-by: renovate-bot <[email protected]>
Co-committed-by: renovate-bot <[email protected]>
@ycherniavskyi
Copy link
Contributor Author

@lminiero, apologies for the delayed response. I wanted to conduct thorough testing in my environment before commenting. As of today, I've completed my tests and am ready to report that everything works as expected (in my environment) 😊.

@ycherniavskyi ycherniavskyi deleted the sip-contact-for-hold-unhold branch December 11, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants