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

AudioBridge: (re) configure remote rtp #3380

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

keremcadirci
Copy link
Contributor

This PR is intended to address the topic discussed in https://janus.discourse.group/t/audiobridge-rtp-join-sip-scenarios/1049/3
This PR enhance slightly SIP interoperability provided by the plain RTP join feature of AudioBridge, such as in the case of an 18x message with SDP. Specifically, during an outbound SIP call, there is currently no way to direct RTP issued from a SIP 18x Ringing or Session Progress (e.g., ringback tone provided by RTP or a custom message sent by the operator during ringing), into the AudioBridge room.

Solution Proposal:

  • Add an rtp object in the configure api
	"request" : "configure",
	[..] 
	"rtp" : {
		"ip" : "<IP address you want media to be sent to>",
		"port" : <port you want media to be sent to>,
		"payload_type" : <payload type to use for RTP packets (optional; only needed in case Opus is used, automatic for G.711)>,
		"audiolevel_ext" : <ID of the audiolevel RTP extension, if used (optional)>,
		"fec" : <true|false, whether FEC should be enabled for the Opus stream (optional; only needed in case Opus is used)>
	}
  • With this request we will not change local RTP port of the plain RTP participant generated with join request
  • We will close existing socket created by join request, then reconnect/rebind the same local RTP port with the the new remote RTP provided by configure request.
  • We will accept rtp object in configure request only if the participant has previously joined with plain rtp.
  • We will not modify any responses/events related to configure request

I believe this approach is backward compatible.

How this enhancement improve SIP interoperability? How will SIP/janus flow will be?

  • Before making an outbound call, make an RTP join request by providing a dummy sipSDP to janus and get janusSDP. Then Inıtiate a SIP Invite with janusSDP.
  • Since we have provided an SDP during Invite, SIP UAS may send a SIP 18x with an SDP. (if SIP Invite does not have SDP then 18x message will not contain any sdp)
  • When SIP UAC receives an 18x with SDP we will send a configure request to janus with 18x SDP. Thus Audio room participants will hear rinback tone or ring message into room
  • When SIP UAC receives an OK with SDP (1) we will send a configure request to janus with SIP OK SDP or (2) make a RE-Invite without SDP and rejoin to janus ....

Implementation & Tests

We have well tested with real word use cases:

Tests successfully made:

  • Backward compatibility: when rtp object is not present everything works as expected.
  • Happy path (configure rtp): We have seen in netstat that the existing socket is correctly changed after configure request. We have seen that both way RTP sessions are OK and both way audio communication is well established. the socket is closed after participant left or after session close.
  • Multiple confiure rtp requests on a single session works fine
  • Error Case: (1) allowRtpParticipants is false on room, (2) join event does not contain rtp

Not Tested:

  • Only pcma is tested: pcmu and opus not tested
  • payload_type changes on configure is not tested

@lminiero
Copy link
Member

To be honest, I don't like this approach, which seems wasteful. It creates a socket already knowing it will be destroyed shortly thereafter. I'd rather see a possible alternative to RTP participants creation, where the plugin is asked to provide a port first, and the application will then provide their own later on (e.g., via a configure). This would map better to an inverted negotiation pattern (where the plugin "offers" and the application "answers"). We have both approaches already on the WebRTC side, for instance, so you can see how that's done for WebRTC to possibly reuse the same methods and just add the required attributes there.

@keremcadirci
Copy link
Contributor Author

Do you suggest something like generate-offer with join request ?

In that case which of two join request would you prefer (or a 3rd suggestion):

  • add generate-rtp-offer attribute
{
	"request" : "join",
	"room" : <numeric ID of the room to join>,
        [..]
        "generate-rtp-offer: <true|false, whether audioroom reserve a local port to receive rtp. configure request should be later usedd to setup the PeerConnection\>
}
  • add generate-offer into rtp object
{
	"request" : "join",
	"room" : <numeric ID of the room to join>,
        [..]
        "rtp" : {
                "generate-offer: <true|false, whether audioroom reserve a local port to receive rtp. configure request should be later usedd to setup the PeerConnection. If true all other rtp properties are discarded\>,
                [..] <other properties are discarded\>
	}
}

@lminiero
Copy link
Member

I think the second one is better, since it adds the property as part of the already existing rtp container. For consistency it should use a generate_offer, though (notice the underscore), since that's how the property is called for WebRTC usage.

@keremcadirci
Copy link
Contributor Author

keremcadirci commented Jun 7, 2024

The fallowing rtp.generate_offer is added

{
	"request" : "join",
	"room" : <numeric ID of the room to join>,
        [..]
        "rtp" : {
                "generate_offer: <true|false, whether audioroom reserve a local port to receive rtp. configure request should be later usedd to setup the PeerConnection. If true all other rtp properties are discarded\>,
                [..] <other properties are discarded\>
	}
}

@lminiero My previous remarks about limitations and tests remains the same

@lminiero
Copy link
Member

That's a LOT of new code... I'm not sure I'm comfortable with that many changes in critical parts of the plugin. I'll need some time to review this.

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.

2 participants