diff --git a/bin/dbus/cx.ring.Ring.CallManager.xml b/bin/dbus/cx.ring.Ring.CallManager.xml index e87142360..1138c33b5 100644 --- a/bin/dbus/cx.ring.Ring.CallManager.xml +++ b/bin/dbus/cx.ring.Ring.CallManager.xml @@ -704,11 +704,14 @@ -

Notify a media update on the current call.

-

This signal is emitted when a media update is received from the peer. The list - of media included in the request is reported. The client can control the media status - of the answer through the media attributes when when accepting the request.

- The client must subscribe to this signal to handle incoming calls. +

Report an incoming media change request.

+

This signal is emitted when a media change request is received from + the peer. The list of media included in the request is reported. The client + can control the attributes of the media to used in the answer when accepting + the request.

+ The client must subscribe to this signal to handle the media + change requests. +
@@ -1021,20 +1024,26 @@
- + -

Report mediation negotation status.

+

Report mediation negotiation status.

The acceptable states are:
    -
  • NEGOTIATION_SUCCESS: Medias are ready
  • -
  • NEGOTIATION_FAIL: Medias negotion failed
  • +
  • NEGOTIATION_SUCCESS: Media negitiation succeded
  • +
  • NEGOTIATION_FAIL: Media negotiation failed
+ + + + The updated attribute list of the negotiated media. + +
diff --git a/bin/dbus/dbusclient.cpp b/bin/dbus/dbusclient.cpp index 59a1f3031..13cf4a7f8 100644 --- a/bin/dbus/dbusclient.cpp +++ b/bin/dbus/dbusclient.cpp @@ -203,7 +203,7 @@ DBusClient::initLibrary(int flags) exportable_callback( bind(&DBusCallManager::remoteRecordingChanged, callM, _1, _2, _3)), exportable_callback( - bind(&DBusCallManager::mediaNegotiationStatus, callM, _1, _2))}; + bind(&DBusCallManager::mediaNegotiationStatus, callM, _1, _2, _3))}; // Configuration event handlers const std::map configEvHandlers = { diff --git a/bin/jni/callmanager.i b/bin/jni/callmanager.i index a7ae0bd43..83535710a 100644 --- a/bin/jni/callmanager.i +++ b/bin/jni/callmanager.i @@ -53,7 +53,8 @@ public: virtual void peerHold(const std::string& call_id, bool holding){} virtual void connectionUpdate(const std::string& id, int state){} virtual void remoteRecordingChanged(const std::string& call_id, const std::string& peer_number, bool state){} - virtual void mediaNegotiationStatus(const std::string& call_id, const std::string& event){} + virtual void mediaNegotiationStatus(const std::string& call_id, const std::string& event, + const std::vector>& mediaList){} }; @@ -155,5 +156,6 @@ public: virtual void peerHold(const std::string& call_id, bool holding){} virtual void connectionUpdate(const std::string& id, int state){} virtual void remoteRecordingChanged(const std::string& call_id, const std::string& peer_number, bool state){} - virtual void mediaNegotiationStatus(const std::string& call_id, const std::string& event){} + virtual void mediaNegotiationStatus(const std::string& call_id, const std::string& event, + const std::vector>& mediaList){} }; diff --git a/bin/jni/jni_interface.i b/bin/jni/jni_interface.i index 8687220dd..ec6cfb89c 100644 --- a/bin/jni/jni_interface.i +++ b/bin/jni/jni_interface.i @@ -255,7 +255,7 @@ void init(ConfigurationCallback* confM, Callback* callM, PresenceCallback* presM exportable_callback(bind(&Callback::peerHold, callM, _1, _2)), exportable_callback(bind(&Callback::connectionUpdate, callM, _1, _2)), exportable_callback(bind(&Callback::remoteRecordingChanged, callM, _1, _2, _3)), - exportable_callback(bind(&Callback::mediaNegotiationStatus, callM, _1, _2)) + exportable_callback(bind(&Callback::mediaNegotiationStatus, callM, _1, _2, _3)) }; // Configuration event handlers diff --git a/src/dring/callmanager_interface.h b/src/dring/callmanager_interface.h index 541f5e363..b59621e22 100644 --- a/src/dring/callmanager_interface.h +++ b/src/dring/callmanager_interface.h @@ -268,7 +268,9 @@ struct DRING_PUBLIC CallSignal struct DRING_PUBLIC MediaNegotiationStatus { constexpr static const char* name = "MediaNegotiationStatus"; - using cb_type = void(const std::string&, const std::string&); + using cb_type = void(const std::string&, + const std::string&, + const std::vector>&); }; }; diff --git a/src/sip/sdp.cpp b/src/sip/sdp.cpp index 0674634d1..e1127e72b 100644 --- a/src/sip/sdp.cpp +++ b/src/sip/sdp.cpp @@ -1004,6 +1004,9 @@ Sdp::getMediaAttributeListFromSdp(const pjmedia_sdp_session* sdpSession) std::vector mediaList; for (unsigned idx = 0; idx < sdpSession->media_count; idx++) { + unsigned audioIdx = 0; + unsigned videoIdx = 0; + mediaList.emplace_back(MediaAttribute {}); auto& mediaAttr = mediaList.back(); @@ -1035,8 +1038,11 @@ Sdp::getMediaAttributeListFromSdp(const pjmedia_sdp_session* sdpSession) // and the crypto materials are present. mediaAttr.secure_ = transp == MediaTransport::RTP_SAVP and not getCrypto(media).empty(); - mediaAttr.label_ = mediaAttr.type_ == MediaType::MEDIA_AUDIO ? "audio_" : "video_"; - mediaAttr.label_ += std::to_string(idx); + if (mediaAttr.type_ == MediaType::MEDIA_AUDIO) { + mediaAttr.label_ = "audio_" + std::to_string(audioIdx++); + } else if (mediaAttr.type_ == MediaType::MEDIA_VIDEO) { + mediaAttr.label_ = "video_" + std::to_string(videoIdx++); + } } return mediaList; diff --git a/src/sip/sipcall.cpp b/src/sip/sipcall.cpp index 9d67ac53f..5f86220a7 100644 --- a/src/sip/sipcall.cpp +++ b/src/sip/sipcall.cpp @@ -219,8 +219,9 @@ SIPCall::configureRtpSession(const std::shared_ptr& rtpSession, const MediaDescription& localMedia, const MediaDescription& remoteMedia) { + assert(rtpSession); if (not rtpSession) - throw std::runtime_error("Must have a valid Audio RTP Session"); + throw std::runtime_error("Must have a valid RTP Session"); // Configure the media stream auto new_mtu = transport_->getTlsMtu(); @@ -1728,15 +1729,12 @@ SIPCall::isVideoMuted() const } void -SIPCall::startAllMedia() +SIPCall::updateNegotiatedMedia() { - if (!transport_ or !sdp_) - return; - JAMI_WARN("[call:%s] startAllMedia()", getCallId().c_str()); - if (isSecure() && not transport_->isSecure()) { - JAMI_ERR("[call:%s] Can't perform secure call over insecure SIP transport", - getCallId().c_str()); - onFailure(EPROTONOSUPPORT); + JAMI_DBG("[call:%s] updating negotiated media", getCallId().c_str()); + + if (not transport_ or not sdp_) { + JAMI_ERR("[call:%s] the call is in invalid state", getCallId().c_str()); return; } @@ -1744,16 +1742,34 @@ SIPCall::startAllMedia() bool peer_holding {true}; int streamIdx = -1; - // reset - readyToRecord_ = false; - resetMediaReady(); - bool isVideoEnabled = false; - for (const auto& slot : slots) { streamIdx++; const auto& local = slot.first; const auto& remote = slot.second; + // Skip disabled media + if (not local.enabled) { + JAMI_DBG("[call:%s] [SDP:slot#%u] The media is disabled, skipping", + getCallId().c_str(), + streamIdx); + continue; + } + + if (static_cast(streamIdx) >= rtpStreams_.size()) { + throw std::runtime_error("Stream index is out-of-range"); + } + + auto const& rtpStream = rtpStreams_[streamIdx]; + + if (not rtpStream.mediaAttribute_) { + throw std::runtime_error("Missing media attribute"); + } + + rtpStream.mediaAttribute_->enabled_ = local.enabled and remote.enabled; + + if (not rtpStream.rtpSession_) + throw std::runtime_error("Must have a valid RTP Session"); + if (local.type != MEDIA_AUDIO && local.type != MEDIA_VIDEO) { JAMI_ERR("[call:%s] Unexpected media type %u", getCallId().c_str(), local.type); throw std::runtime_error("Invalid media attribute"); @@ -1766,49 +1782,38 @@ SIPCall::startAllMedia() continue; } - if (!local.codec) { + if (local.enabled and not local.codec) { JAMI_WARN("[call:%s] [SDP:slot#%u] Missing local codec", getCallId().c_str(), streamIdx); continue; } - if (!remote.codec) { + if (remote.enabled and not remote.codec) { JAMI_WARN("[call:%s] [SDP:slot#%u] Missing remote codec", getCallId().c_str(), streamIdx); continue; } - if (isSecure() && (not local.crypto || not remote.crypto)) { - JAMI_WARN( - "[call:%s] [SDP:slot#%u] Secure mode but no crypto attributes. Ignoring the media", - getCallId().c_str(), - streamIdx); + if (isSecure() and local.enabled and not local.crypto) { + JAMI_WARN("[call:%s] [SDP:slot#%u] Secure mode but no local crypto attributes. " + "Ignoring the media", + getCallId().c_str(), + streamIdx); continue; } - auto const& rtpStream = rtpStreams_[streamIdx]; - if (not rtpStream.mediaAttribute_) { - throw std::runtime_error("Missing media attribute"); - } - - // Configure the media. - rtpStream.mediaAttribute_->enabled_ = true; - if (rtpStream.mediaAttribute_->type_ == MEDIA_VIDEO) - isVideoEnabled = true; - configureRtpSession(rtpStream.rtpSession_, rtpStream.mediaAttribute_, local, remote); - - // Not restarting media loop on hold as it's a huge waste of CPU ressources - // because of the audio loop - if (getState() != CallState::HOLD) { - if (isIceRunning()) { - // Create sockets for RTP and RTCP, and start the session. - auto compId = ICE_COMP_ID_RTP + streamIdx * ICE_COMP_COUNT_PER_STREAM; - rtpStream.rtpSession_->start(newIceSocket(compId), newIceSocket(compId + 1)); - } else - rtpStream.rtpSession_->start(nullptr, nullptr); + if (isSecure() and remote.enabled and not remote.crypto) { + JAMI_WARN("[call:%s] [SDP:slot#%u] Secure mode but no crypto remote attributes. " + "Ignoring the media", + getCallId().c_str(), + streamIdx); + continue; } // Aggregate holding info over all remote streams peer_holding &= remote.onHold; + + // Configure the media. + configureRtpSession(rtpStream.rtpSession_, rtpStream.mediaAttribute_, local, remote); } if (not isSubcall() and peerHolding_ != peer_holding) { @@ -1816,7 +1821,64 @@ SIPCall::startAllMedia() emitSignal(getCallId(), peerHolding_); } + // Notify using the parent Id if it's a subcall. + auto callId = isSubcall() ? parent_->getCallId() : getCallId(); + emitSignal( + callId, + MediaNegotiationStatusEvents::NEGOTIATION_SUCCESS, + MediaAttribute::mediaAttributesToMediaMaps(getMediaAttributeList())); +} + +void +SIPCall::startAllMedia() +{ + JAMI_DBG("[call:%s] starting the media", getCallId().c_str()); + + if (not transport_ or not sdp_) { + JAMI_ERR("[call:%s] the call is in invalid state", getCallId().c_str()); + return; + } + + if (isSecure() && not transport_->isSecure()) { + JAMI_ERR("[call:%s] Can't perform secure call over insecure SIP transport", + getCallId().c_str()); + onFailure(EPROTONOSUPPORT); + return; + } + + // reset + readyToRecord_ = false; + resetMediaReady(); #ifdef ENABLE_VIDEO + bool isVideoEnabled = false; +#endif + + for (auto iter = rtpStreams_.begin(); iter != rtpStreams_.end(); iter++) { + if (not iter->mediaAttribute_) { + throw std::runtime_error("Missing media attribute"); + } + +#ifdef ENABLE_VIDEO + if (iter->mediaAttribute_->type_ == MEDIA_VIDEO) + isVideoEnabled = true; +#endif + + // Not restarting media loop on hold as it's a huge waste of CPU ressources + // because of the audio loop + if (getState() != CallState::HOLD) { + if (isIceRunning()) { + // Create sockets for RTP and RTCP, and start the session. + auto streamIdx = std::distance(rtpStreams_.begin(), iter); + auto compId = ICE_COMP_ID_RTP + streamIdx * ICE_COMP_COUNT_PER_STREAM; + iter->rtpSession_->start(newIceSocket(compId), newIceSocket(compId + 1)); + } else { + iter->rtpSession_->start(nullptr, nullptr); + } + } + } + +#ifdef ENABLE_VIDEO + // TODO. Move this elsewhere (when adding participant to conf?) if (!isVideoEnabled && !getConfId().empty()) { auto conference = Manager::instance().getConferenceFromID(getConfId()); conference->attachVideo(getReceiveVideoFrameActiveWriter().get(), getCallId()); @@ -2100,21 +2162,22 @@ SIPCall::onMediaNegotiationComplete() // to a negotiated transport. runOnMainThread([w = weak()] { if (auto this_ = w.lock()) { - // Notify using the parent Id if it's a subcall. - auto callId = this_->isSubcall() ? this_->parent_->getCallId() : this_->getCallId(); - emitSignal( - callId, MediaNegotiationStatusEvents::NEGOTIATION_SUCCESS); - std::lock_guard lk {this_->callMutex_}; JAMI_WARN("[call:%s] media changed", this_->getCallId().c_str()); // The call is already ended, so we don't need to restart medias if (not this_->inviteSession_ or this_->inviteSession_->state == PJSIP_INV_STATE_DISCONNECTED or not this_->sdp_) return; - // If ICE is not used, start medias now + // If ICE is not used, start media now auto rem_ice_attrs = this_->sdp_->getIceAttributes(); if (rem_ice_attrs.ufrag.empty() or rem_ice_attrs.pwd.empty()) { - JAMI_WARN("[call:%s] no remote ICE for media", this_->getCallId().c_str()); + JAMI_DBG("[call:%s] No ICE, starting media using default ", + this_->getCallId().c_str()); + + // Update the negotiated media. + this_->updateNegotiatedMedia(); + + // Start the media. this_->stopAllMedia(); this_->startAllMedia(); return; @@ -2172,6 +2235,10 @@ SIPCall::onIceNegoSucceed() // can stop a call. So do not start medias if (not inviteSession_ or inviteSession_->state == PJSIP_INV_STATE_DISCONNECTED or not sdp_) return; + + // Update the negotiated media. + updateNegotiatedMedia(); + // Nego succeed: move to the new media transport stopAllMedia(); { diff --git a/src/sip/sipcall.h b/src/sip/sipcall.h index b019b529f..3eb6c3938 100644 --- a/src/sip/sipcall.h +++ b/src/sip/sipcall.h @@ -335,7 +335,7 @@ private: void setCallMediaLocal(); void startIceMedia(); void onIceNegoSucceed(); - + void updateNegotiatedMedia(); void startAllMedia(); void stopAllMedia(); diff --git a/test/unitTest/media_negotiation/media_negotiation.cpp b/test/unitTest/media_negotiation/media_negotiation.cpp index ceba1934b..0c157b7ca 100644 --- a/test/unitTest/media_negotiation/media_negotiation.cpp +++ b/test/unitTest/media_negotiation/media_negotiation.cpp @@ -82,6 +82,7 @@ struct CallData std::string userName_ {}; std::string alias_ {}; std::string callId_ {}; + bool enableMultiStream_ {true}; std::vector signals_; std::condition_variable cv_ {}; std::mutex mtx_; @@ -111,11 +112,13 @@ private: void audio_and_video_then_mute_video(); void audio_only_then_add_video(); void audio_and_video_then_mute_audio(); + void audio_only_then_add_video_but_peer_disabled_multistream(); CPPUNIT_TEST_SUITE(MediaNegotiationTest); CPPUNIT_TEST(audio_and_video_then_mute_video); CPPUNIT_TEST(audio_only_then_add_video); CPPUNIT_TEST(audio_and_video_then_mute_audio); + CPPUNIT_TEST(audio_only_then_add_video_but_peer_disabled_multistream); CPPUNIT_TEST_SUITE_END(); // Event/Signal handlers @@ -126,6 +129,10 @@ private: const std::string& callId, const std::vector mediaList, CallData& callData); + // For backward compatibility test cases + static void onIncomingCall(const std::string& accountId, + const std::string& callId, + CallData& callData); static void onMediaChangeRequested(const std::string& accountId, const std::string& callId, const std::vector mediaList, @@ -253,6 +260,36 @@ MediaNegotiationTest::onIncomingCallWithMedia(const std::string& accountId, callData.cv_.notify_one(); } +void +MediaNegotiationTest::onIncomingCall(const std::string& accountId, + const std::string& callId, + CallData& callData) +{ + CPPUNIT_ASSERT_EQUAL(callData.accountId_, accountId); + + JAMI_INFO("Signal [%s] - user [%s] - call [%s]", + DRing::CallSignal::IncomingCall::name, + callData.alias_.c_str(), + callId.c_str()); + + // NOTE. + // We shouldn't access shared_ptr as this event is supposed to mimic + // the client, and the client have no access to this type. But here, we only + // needed to check if the call exists. This is the most straightforward and + // reliable way to do it until we add a new API (like hasCall(id)). + if (not Manager::instance().getCallFromCallID(callId)) { + JAMI_WARN("Call with ID [%s] does not exist!", callId.c_str()); + callData.callId_ = {}; + return; + } + + std::unique_lock lock {callData.mtx_}; + callData.callId_ = callId; + callData.signals_.emplace_back(CallData::Signal(DRing::CallSignal::IncomingCall::name)); + + callData.cv_.notify_one(); +} + void MediaNegotiationTest::onMediaChangeRequested(const std::string& accountId, const std::string& callId, @@ -448,6 +485,7 @@ MediaNegotiationTest::configureScenario(CallData& aliceData, CallData& bobData) auto const& account = Manager::instance().getAccount(aliceData.accountId_); aliceData.userName_ = account->getAccountDetails()[ConfProperties::USERNAME]; aliceData.alias_ = account->getAccountDetails()[ConfProperties::ALIAS]; + account->enableMultiStream(aliceData.enableMultiStream_); } { @@ -455,6 +493,7 @@ MediaNegotiationTest::configureScenario(CallData& aliceData, CallData& bobData) auto const& account = Manager::instance().getAccount(bobData.accountId_); bobData.userName_ = account->getAccountDetails()[ConfProperties::USERNAME]; bobData.alias_ = account->getAccountDetails()[ConfProperties::ALIAS]; + account->enableMultiStream(bobData.enableMultiStream_); } std::map> signalHandlers; @@ -473,6 +512,14 @@ MediaNegotiationTest::configureScenario(CallData& aliceData, CallData& bobData) user == aliceData.alias_ ? aliceData : bobData); })); + signalHandlers.insert(DRing::exportable_callback( + [&](const std::string& accountId, const std::string& callId, const std::string&) { + auto user = getUserAlias(callId); + if (not user.empty()) { + onIncomingCall(accountId, callId, user == aliceData.alias_ ? aliceData : bobData); + } + })); + signalHandlers.insert(DRing::exportable_callback( [&](const std::string& accountId, const std::string& callId, @@ -500,7 +547,9 @@ MediaNegotiationTest::configureScenario(CallData& aliceData, CallData& bobData) })); signalHandlers.insert(DRing::exportable_callback( - [&](const std::string& callId, const std::string& event) { + [&](const std::string& callId, + const std::string& event, + const std::vector>& mediaList) { auto user = getUserAlias(callId); if (not user.empty()) onMediaNegotiationStatus(callId, @@ -533,8 +582,12 @@ MediaNegotiationTest::testWithScenario(CallData& aliceData, bobData.accountId_.c_str()); // Wait for incoming call signal. - CPPUNIT_ASSERT_EQUAL(true, - waitForSignal(bobData, DRing::CallSignal::IncomingCallWithMedia::name)); + if (bobData.enableMultiStream_) { + CPPUNIT_ASSERT(waitForSignal(bobData, DRing::CallSignal::IncomingCallWithMedia::name)); + } else { + CPPUNIT_ASSERT(waitForSignal(bobData, DRing::CallSignal::IncomingCall::name)); + } + // Answer the call. { auto const& mediaList = MediaAttribute::mediaAttributesToMediaMaps(scenario.answer_); @@ -786,6 +839,47 @@ MediaNegotiationTest::audio_and_video_then_mute_audio() JAMI_INFO("=== End test %s ===", __FUNCTION__); } +void +MediaNegotiationTest::audio_only_then_add_video_but_peer_disabled_multistream() +{ + JAMI_INFO("=== Begin test %s ===", __FUNCTION__); + + // Disable multi-stream on Bob's side + bobData_.enableMultiStream_ = false; + + configureScenario(aliceData_, bobData_); + + MediaAttribute defaultAudio(MediaType::MEDIA_AUDIO); + defaultAudio.label_ = "main audio"; + + MediaAttribute defaultVideo(MediaType::MEDIA_VIDEO); + defaultVideo.label_ = "main video"; + + { + MediaAttribute audio(defaultAudio); + MediaAttribute video(defaultVideo); + + TestScenario scenario; + // First offer/answer + scenario.offer_.emplace_back(audio); + scenario.answer_.emplace_back(audio); + + // Updated offer/answer + scenario.offerUpdate_.emplace_back(audio); + scenario.offerUpdate_.emplace_back(video); + scenario.answerUpdate_.emplace_back(audio); + scenario.answerUpdate_.emplace_back(video); + scenario.expectMediaRenegotiation_ = false; + scenario.expectMediaChangeRequest_ = false; + + testWithScenario(aliceData_, bobData_, scenario); + } + + DRing::unregisterSignalHandlers(); + + JAMI_INFO("=== End test %s ===", __FUNCTION__); +} + } // namespace test } // namespace jami