Add support for "progress" request on SIP Plugin

[adnan-mujagic]

We are currently using Janus to make calls towards a third party application. This integration works in a following way:

• We send INVITE towards Janus.
• Janus sends an incomingcall event to our backend app with SDP offer.
• We send this offer to the third party backend.
• Third party backend responds with SDP answer, but won't forward the call to the end user application until media can flow (Currently that means that our backend has to send accept request to Janus)

Our platform changes the call states based on the responses received by Janus, and in case of accept, Janus sends 200 OK, which we map to an "established" state. However, since at this moment the call is actually ringing on the end user device, it would be a lot more appropriate to have a new request which would respond with 183 Session Progress, which is what we would map into a "ringing" state. That is exactly what was added in this pull request.

This solution was tested in production and confirmed it works as we would expect for our use case.

We think that having a way to respond with 183 Session Progress is a common use case and would be a useful addition to the SIP Plugin.

Regarding the pull request itself, here is a summary of work that was done:

• added a new request named progress.
• the progress request is handled the same way as accept, with the key difference that the SIP response send by Janus is 183 Session Progress, and emits the progressed event once concluded.
• also added a new status for sessions in this state.

[januscla]

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

[lminiero]

Thanks for your contribution! A few people have mentioned a desire to be able to reply with a 183 in the past few months, as we currently only support 183 on the way in for early-media but not the other way around (which IIUC is what your patch would add support for).

I'll need some time to review as I can see quite a few changes, but a quick question to start: why a new method called progress, and not just a new property in accept to state you only want 183 for now? From a quick glance it looks like the two blocks share a lot of functionality, and so that may reduce the number of changes.

[lminiero]

why a new method called progress, and not just a new property in accept to state you only want 183 for now

Note that you could do the same with a

	} else if(!strcasecmp(request_text, "accept") || !strcasecmp(request_text, "progress")) {

where, on the same code, you could then selectivtly do or not do some things depending on the request you received. But again this only works if the two requests actually do share a lot of code, which I haven't checked yet.

[adnan-mujagic]

Hi,

thank you for a quick response, yes, the two requests do share majority of the code. We would prefer to have it as a separate request, since it would be a strange usage to send two accept requests, where the first one has flag progress = true, and the subsequent one has progress = false.

However, I do agree that we can add it into the same block as accept. Give me some time to apply that change and verify the behaviour remains unchanged.

[adnan-mujagic]

Hello again, just to notify you that I've applied the changes and tested that the behaviour is still correct, hopefully now it is a bit easier to review.

[lminiero]

Thanks for the changes, the code is indeed much more readable now!

I'm wondering if this new state (janus_sip_call_status_progress) should be part of a few checks we make in other places on the state. We have many checks in place that check for janus_sip_call_status_incall: a good example is the janus_sip_call_is_established() function, that currently acts as a fence for deciding whether to relay traffic (which means that your 183 would not work for early media as it is, for instance). As such, I'd check all the places where state is checked, to check whether or not progress should (or should not) be a trigger for those too.

[lminiero]

This can probably be a ternary, rather than an if/else, e.g.

JANUS_VALIDATE_JSON_OBJECT(root, (progress ? progress_parameters : accept_parameters),

[adnan-mujagic]

Heyy, just wanted to give a small update here, I initially tried to do this using a ternary but was getting errors and it took more effort to fix than to rewrite using the if/else.

Regarding the statuses, I am checking the places wherejanus_sip_call_status_incall is used and the janus_sip_call_is_established() function, will be working on this in the next few days to try and decide where we should take the new state into account as well.

[adnan-mujagic]

Small update here: you were right, I should have just copy pasted your suggestion and it would have worked.

However, correct me if I am wrong, but since this JANUS_VALIDATE_JSON_OBJECT is defined as a macro, passing in an expression such as progress ? progress_parameters : accept_parameters would insert this expression wherever that parameter is used inside the macro, which is an unnecessary overhead.

The way I have changed it now is that first we compute the desired params using a ternary operator, then pass the result as a parameter to the macro.

[adnan-mujagic]

Just to make it a bit easier to review, in the latest changes I added janus_sip_call_status_progress as a valid status for

• janus_sip_incoming_rtp method
• janus_sip_incoming_rtcp method
• decline request
• hangup request
• record request (because of the presumption it makes sense to start recording with early media that was already there)

[lminiero]

Thanks for the changes! I think the only thing it's missing, now is a reference to this new API (and the new events that may come out of it) to the documentation.

[adnan-mujagic]

Thanks for the changes! I think the only thing it's missing, now is a reference to this new API (and the new events that may come out of it) to the documentation.

I have documented the newly introduced progress request, as well as the progressing event that comes as a result.

[lminiero]

Thanks! I'll mention the PR in a couple of posts on the group where early media was discussed, just in case we can get feedback/test result from other people interested in using the feature, and then we can merge it next week.

[adnan-mujagic]

Thanks! I'll mention the PR in a couple of posts on the group where early media was discussed, just in case we can get feedback/test result from other people interested in using the feature, and then we can merge it next week.

Heyy, just wondering if there are any news regarding this PR?

[lminiero]

I was waiting for more feedback from other users, but we got none. At this point, next week I'm planning to tag a new version, so after I do that I'll merge this in.