From d02e768e3e79109c35ee1807bb62a9f6b1afc25a Mon Sep 17 00:00:00 2001
From: Mohamed Chibani <mohamed.chibani@savoirfairelinux.com>
Date: Thu, 22 Jul 2021 17:03:20 -0400
Subject: [PATCH] media attribute: update media source only if set
When a new media list is provided (during a media change request
for instance), the media source and source type will be updated
only if set in the new media attribute list. This will prevent unwanted
overwriting of media source and source type.
This patch will fix the ut_media_negotiation unit test.
Gitlab: #576
Change-Id: If6f994def1e113265eeff0cce93fe5d314dffd5d
---
src/sip/sipcall.cpp | 22 +-
src/sip/sipcall.h | 1 +
test/unitTest/media_control/media_control.cpp | 376 ------------------
.../media_negotiation/media_negotiation.cpp | 22 +-
4 files changed, 27 insertions(+), 394 deletions(-)
delete mode 100644 test/unitTest/media_control/media_control.cpp
diff --git a/src/sip/sipcall.cpp b/src/sip/sipcall.cpp
index e60e216d06..fb8cad6483 100644
--- a/src/sip/sipcall.cpp
+++ b/src/sip/sipcall.cpp
@@ -821,7 +821,7 @@ SIPCall::answer(const std::vector<DRing::MediaMap>& mediaList)
// Apply the media attributes provided by the user.
for (size_t idx = 0; idx < mediaAttrList.size(); idx++) {
- rtpStreams_[idx].mediaAttribute_ = std::make_shared<MediaAttribute>(mediaAttrList[idx]);
+ updateMediaStream(mediaAttrList[idx], idx);
}
if (not inviteSession_)
@@ -2098,6 +2098,8 @@ SIPCall::updateMediaStream(const MediaAttribute& newMediaAttr, size_t streamIdx)
auto const& mediaAttr = rtpStream.mediaAttribute_;
assert(mediaAttr);
+ bool notify = false;
+
if (newMediaAttr.muted_ == mediaAttr->muted_) {
// Nothing to do. Already in the desired state.
JAMI_DBG("[call:%s] [%s] already %s",
@@ -2105,30 +2107,34 @@ SIPCall::updateMediaStream(const MediaAttribute& newMediaAttr, size_t streamIdx)
mediaAttr->label_.c_str(),
mediaAttr->muted_ ? "muted " : "un-muted ");
- return;
+ } else {
+ notify = true;
}
// Update
mediaAttr->muted_ = newMediaAttr.muted_;
- mediaAttr->sourceUri_ = newMediaAttr.sourceUri_;
+ // Only update source and type if actually set.
+ if (not newMediaAttr.sourceUri_.empty())
+ mediaAttr->sourceUri_ = newMediaAttr.sourceUri_;
+ if (newMediaAttr.sourceType_ != MediaSourceType::NONE)
+ mediaAttr->sourceType_ = newMediaAttr.sourceType_;
JAMI_DBG("[call:%s] %s [%s]",
getCallId().c_str(),
mediaAttr->muted_ ? "muting" : "un-muting",
mediaAttr->label_.c_str());
- if (mediaAttr->type_ == MediaType::MEDIA_AUDIO) {
+ if (notify and mediaAttr->type_ == MediaType::MEDIA_AUDIO) {
rtpStream.rtpSession_->setMuted(mediaAttr->muted_);
+ setMute(mediaAttr->muted_);
if (not isSubcall())
emitSignal<DRing::CallSignal::AudioMuted>(getCallId(), mediaAttr->muted_);
- setMute(mediaAttr->muted_);
return;
}
#ifdef ENABLE_VIDEO
- if (mediaAttr->type_ == MediaType::MEDIA_VIDEO) {
- if (not isSubcall())
- emitSignal<DRing::CallSignal::VideoMuted>(getCallId(), mediaAttr->muted_);
+ if (notify and mediaAttr->type_ == MediaType::MEDIA_VIDEO and not isSubcall()) {
+ emitSignal<DRing::CallSignal::VideoMuted>(getCallId(), mediaAttr->muted_);
}
#endif
}
diff --git a/src/sip/sipcall.h b/src/sip/sipcall.h
index d2d13b996e..c5b0954d7c 100644
--- a/src/sip/sipcall.h
+++ b/src/sip/sipcall.h
@@ -346,6 +346,7 @@ private:
bool hold();
bool unhold();
+
// Update the attributes of a media stream
void updateMediaStream(const MediaAttribute& newMediaAttr, size_t streamIdx);
void updateAllMediaStreams(const std::vector<MediaAttribute>& mediaAttrList);
diff --git a/test/unitTest/media_control/media_control.cpp b/test/unitTest/media_control/media_control.cpp
deleted file mode 100644
index 1a4124c740..0000000000
--- a/test/unitTest/media_control/media_control.cpp
+++ /dev/null
@@ -1,376 +0,0 @@
-/*
- * Copyright (C) 2021 Savoir-faire Linux Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 3 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program. If not, see <https://www.gnu.org/licenses/>.
- */
-
-#include "manager.h"
-#include "jamidht/connectionmanager.h"
-#include "jamidht/jamiaccount.h"
-#include "../../test_runner.h"
-#include "jami.h"
-#include "call_const.h"
-#include "account_const.h"
-#include "sip/sipcall.h"
-#include "sip/sdp.h"
-
-#include <cppunit/TestAssert.h>
-#include <cppunit/TestFixture.h>
-#include <cppunit/extensions/HelperMacros.h>
-
-#include <condition_variable>
-#include <string>
-
-#include "common.h"
-
-using namespace DRing::Account;
-using namespace DRing::Call;
-
-namespace jami {
-namespace test {
-
-struct CallData
-{
- std::string userName_ {};
- std::string alias_ {};
- std::shared_ptr<JamiAccount> account_;
- std::shared_ptr<SIPCall> sipCall_;
- std::vector<MediaAttribute> mediaAttrList_ {};
- std::string signal_ {};
- std::string event_ {};
- std::condition_variable cv_ {};
-};
-
-struct TestScenario
-{
- TestScenario(const std::vector<MediaAttribute>& offer,
- const std::vector<MediaAttribute>& answer,
- const std::vector<MediaAttribute>& offerUpdate,
- const std::vector<MediaAttribute>& answerUpdate)
- : offer_(std::move(offer))
- , answer_(std::move(answer))
- , offerUpdate_(std::move(offerUpdate))
- , answerUpdate_(std::move(answerUpdate))
- {}
-
- TestScenario() {};
-
- std::vector<MediaAttribute> offer_;
- std::vector<MediaAttribute> answer_;
- std::vector<MediaAttribute> offerUpdate_;
- std::vector<MediaAttribute> answerUpdate_;
-};
-
-/**
- * Basic tests for media negotiation.
- */
-class MediaControlTest : public CppUnit::TestFixture
-{
-public:
- MediaControlTest()
- {
- // Init daemon
- DRing::init(DRing::InitFlag(DRing::DRING_FLAG_DEBUG | DRing::DRING_FLAG_CONSOLE_LOG));
- if (not Manager::instance().initialized)
- CPPUNIT_ASSERT(DRing::start("jami-sample.yml"));
- }
- ~MediaControlTest() { DRing::fini(); }
-
- static std::string name() { return "MediaControlTest"; }
- void setUp();
- void tearDown();
-
-private:
- // Test cases.
- void testCallWithMediaList();
-
- CPPUNIT_TEST_SUITE(MediaControlTest);
- CPPUNIT_TEST(testCallWithMediaList);
- CPPUNIT_TEST_SUITE_END();
-
- // Helpers
- void testWithScenario(CallData& aliceData, CallData& bobData, const TestScenario& scenario);
- // Wait for a signal from the callbacks. Some signals also report the event that
- // triggered the signal a like the StateChange signal.
- bool waitForSignal(CallData& callData,
- const std::string& signal,
- const std::string& expectedEvent = {});
-
- // Event/Signal handlers
- void onCallStateChange(const std::string& callId, const std::string& state, CallData& callData);
- void onIncomingCall(const std::string& accountId, const std::string& callId, CallData& callData);
- void onVideoMuted(const std::string& callId, bool muted, CallData& callData);
-
-private:
- std::string aliceAccountId_;
- std::string bobAccountId_;
- std::mutex mtx_;
-};
-
-CPPUNIT_TEST_SUITE_NAMED_REGISTRATION(MediaControlTest, MediaControlTest::name());
-
-void
-MediaControlTest::setUp()
-{
- auto actors = load_actors_and_wait_for_announcement("actors/alice-bob-no-upnpp.yml");
- aliceAccountId_ = actors["alice"];
- bobAccountId_ = actors["bob"];
-}
-
-void
-MediaControlTest::tearDown()
-{
- wait_for_removal_of({aliceAccountId_, bobAccountId_});
-}
-
-void
-MediaControlTest::onIncomingCall(const std::string& accountId,
- const std::string& callId,
- CallData& callData)
-{
- CPPUNIT_ASSERT_EQUAL(callData.account_->getAccountID(), accountId);
-
- JAMI_DBG("%s [%s] received an incoming call [%s]",
- callData.alias_.c_str(),
- accountId.c_str(),
- callId.c_str());
- auto call = Manager::instance().getCallFromCallID(callId);
-
- Manager::instance().answerCall(callId);
-
- callData.sipCall_ = std::dynamic_pointer_cast<SIPCall>(call);
- callData.event_ = StateEvent::INCOMING;
-}
-
-void
-MediaControlTest::onCallStateChange(const std::string& callId,
- const std::string& state,
- CallData& callData)
-{
- auto call = Manager::instance().getCallFromCallID(callId);
-
- // Interested only in main (parent) call.
- if (not call or call->isSubcall())
- return;
-
- auto account = call->getAccount().lock();
- assert(account);
-
- JAMI_DBG("Signal [Call State Change] - user [%s] - call [%s] - state [%s]",
- account->getAccountDetails()[ConfProperties::ALIAS].c_str(),
- callId.c_str(),
- state.c_str());
-
- if (account->getAccountID() != callData.account_->getAccountID())
- return;
-
- callData.event_ = state;
- callData.signal_ = DRing::CallSignal::StateChange::name;
-
- if (state == "CURRENT" or state == "OVER" or state == "HUNGUP") {
- callData.cv_.notify_one();
- }
-}
-
-void
-MediaControlTest::onVideoMuted(const std::string& callId, bool muted, CallData& callData)
-{
- auto call = Manager::instance().getCallFromCallID(callId);
- if (not call or call->isSubcall())
- return;
- auto account = call->getAccount().lock();
- assert(account);
-
- JAMI_INFO("Signal [Video Muted] - user [%s] - call [%s] - state [%s]",
- account->getAccountDetails()[ConfProperties::ALIAS].c_str(),
- call->getCallId().c_str(),
- muted ? "Mute" : "Un-mute");
-
- if (account->getAccountID() != callData.account_->getAccountID())
- return;
-
- callData.mediaAttrList_ = call->getMediaAttributeList();
-
- callData.signal_ = DRing::CallSignal::VideoMuted::name;
- callData.cv_.notify_one();
-}
-
-bool
-MediaControlTest::waitForSignal(CallData& callData,
- const std::string& expectedSignal,
- const std::string& expectedEvent)
-{
- const std::chrono::seconds TIME_OUT {20};
- std::unique_lock<std::mutex> lock {mtx_};
-
- auto res = callData.cv_.wait_for(lock, TIME_OUT, [&] {
- bool pred = callData.signal_ == expectedSignal;
- if (not expectedEvent.empty()) {
- pred = pred and callData.event_ == expectedEvent;
- }
- return pred;
- });
-
- if (not res) {
- std::string msg(expectedSignal);
- if (not expectedEvent.empty()) {
- msg.append("::");
- msg.append(expectedEvent);
- }
-
- JAMI_ERR("Waiting for Signal [%s] timed-out!", expectedSignal.c_str());
- }
-
- return res;
-}
-void
-MediaControlTest::testWithScenario(CallData& aliceData,
- CallData& bobData,
- const TestScenario& scenario)
-{
- JAMI_INFO("ALICE [%s] calls Bob [%s] and wait for BOB to answer",
- aliceData.account_->getAccountID().c_str(),
- bobData.account_->getAccountID().c_str());
-
- aliceData.sipCall_ = std::dynamic_pointer_cast<SIPCall>(
- aliceData.account_->newOutgoingCall(bobData.userName_, scenario.offer_));
- assert(aliceData.sipCall_);
-
- // Wait for the StateChange signal.
- CPPUNIT_ASSERT_EQUAL(true,
- waitForSignal(aliceData,
- DRing::CallSignal::StateChange::name,
- StateEvent::CURRENT));
-
- JAMI_INFO("BOB answered the call [%s]", bobData.sipCall_->getCallId().c_str());
-
- {
- // TODO. Must check against scenario data.
-
- // Validate Alice's SDP
- auto aliceLocalMedia = aliceData.sipCall_->getSDP().getActiveMediaDescription(false);
- CPPUNIT_ASSERT_EQUAL(scenario.offer_.size(), aliceLocalMedia.size());
- CPPUNIT_ASSERT_EQUAL(MediaDirection::SENDRECV, aliceLocalMedia[1].direction_);
-
- // Validate Bob's SDP
- auto bobLocalMedia = bobData.sipCall_->getSDP().getActiveMediaDescription(false);
- CPPUNIT_ASSERT_EQUAL(scenario.answer_.size(), bobLocalMedia.size());
-
- CPPUNIT_ASSERT(bobLocalMedia[0].enabled);
- CPPUNIT_ASSERT_EQUAL(MediaType::MEDIA_AUDIO, bobLocalMedia[0].type);
- CPPUNIT_ASSERT(not bobLocalMedia[0].onHold);
- CPPUNIT_ASSERT(bobLocalMedia[0].addr);
- CPPUNIT_ASSERT_EQUAL(MediaDirection::SENDRECV, bobLocalMedia[1].direction_);
- }
-
- std::this_thread::sleep_for(std::chrono::seconds(2));
-
- // Update the media
- auto mediaListUpdate = MediaAttribute::mediaAttributesToMediaMaps(scenario.offerUpdate_);
- Manager::instance().requestMediaChange(aliceData.sipCall_->getCallId(), mediaListUpdate);
-
- // Wait for the VideoMute signal.
- JAMI_INFO("Waiting for the video muted signal");
-
- CPPUNIT_ASSERT_EQUAL(true, waitForSignal(aliceData, DRing::CallSignal::VideoMuted::name));
-
- {
- // Validate Alice's SDP
- auto aliceLocalMedia = aliceData.sipCall_->getSDP().getActiveMediaDescription(false);
- CPPUNIT_ASSERT_EQUAL(scenario.offerUpdate_.size(), aliceLocalMedia.size());
- CPPUNIT_ASSERT_EQUAL(MediaDirection::SENDRECV, aliceLocalMedia[0].direction_);
-
- // Validate Bob's SDP
- auto bobLocalMedia = bobData.sipCall_->getSDP().getActiveMediaDescription(false);
- CPPUNIT_ASSERT_EQUAL(scenario.answerUpdate_.size(), bobLocalMedia.size());
- CPPUNIT_ASSERT_EQUAL(MediaDirection::SENDRECV, bobLocalMedia[0].direction_);
- }
-
- std::this_thread::sleep_for(std::chrono::seconds(1));
-
- // Bob hang-up.
- JAMI_INFO("Hang up BOB's call and wait for ALICE to hang up");
- Manager::instance().hangupCall(bobData.sipCall_->getCallId());
-
- CPPUNIT_ASSERT_EQUAL(true,
- waitForSignal(aliceData,
- DRing::CallSignal::StateChange::name,
- StateEvent::HUNGUP));
-
- JAMI_INFO("Call terminated on both sides");
-}
-
-void
-MediaControlTest::testCallWithMediaList()
-{
- std::map<std::string, std::shared_ptr<DRing::CallbackWrapperBase>> signalHandlers;
-
- CallData aliceData;
- aliceData.account_ = Manager::instance().getAccount<JamiAccount>(aliceAccountId_);
- aliceData.userName_ = aliceData.account_->getAccountDetails()[ConfProperties::USERNAME];
- aliceData.alias_ = aliceData.account_->getAccountDetails()[ConfProperties::ALIAS];
-
- CallData bobData;
- bobData.account_ = Manager::instance().getAccount<JamiAccount>(bobAccountId_);
- bobData.userName_ = bobData.account_->getAccountDetails()[ConfProperties::USERNAME];
- bobData.alias_ = bobData.account_->getAccountDetails()[ConfProperties::ALIAS];
-
- // Insert needed signal handlers.
- signalHandlers.insert(DRing::exportable_callback<DRing::CallSignal::IncomingCall>(
- [&](const std::string& accountId, const std::string& callId, const std::string&) {
- onIncomingCall(accountId, callId, bobData);
- }));
-
- signalHandlers.insert(DRing::exportable_callback<DRing::CallSignal::StateChange>(
- [&](const std::string& callId, const std::string& state, signed) {
- onCallStateChange(callId, state, aliceData);
- }));
-
- signalHandlers.insert(DRing::exportable_callback<DRing::CallSignal::VideoMuted>(
- [&](const std::string& callId, bool muted) { onVideoMuted(callId, muted, aliceData); }));
-
- DRing::registerSignalHandlers(signalHandlers);
-
- 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.offer_.emplace_back(video);
- scenario.answer_.emplace_back(audio);
- scenario.answer_.emplace_back(video);
-
- // Updated offer/answer
- video.muted_ = true;
- scenario.offerUpdate_.emplace_back(audio);
- scenario.offerUpdate_.emplace_back(video);
- scenario.answerUpdate_.emplace_back(audio);
- scenario.answerUpdate_.emplace_back(video);
-
- testWithScenario(aliceData, bobData, scenario);
- }
-}
-
-} // namespace test
-} // namespace jami
-
-RING_TEST_RUNNER(jami::test::MediaControlTest::name())
diff --git a/test/unitTest/media_negotiation/media_negotiation.cpp b/test/unitTest/media_negotiation/media_negotiation.cpp
index e90c4b6733..4dce341471 100644
--- a/test/unitTest/media_negotiation/media_negotiation.cpp
+++ b/test/unitTest/media_negotiation/media_negotiation.cpp
@@ -676,7 +676,8 @@ MediaNegotiationTest::testWithScenario(CallData& aliceData,
CPPUNIT_ASSERT_EQUAL(scenario.offerUpdate_[idx].muted_, mediaAttr[idx].muted_);
// Check isCaptureDeviceMuted API
- CPPUNIT_ASSERT_EQUAL(mediaAttr[idx].muted_, aliceCall->isCaptureDeviceMuted(mediaAttr[idx].type_));
+ CPPUNIT_ASSERT_EQUAL(mediaAttr[idx].muted_,
+ aliceCall->isCaptureDeviceMuted(mediaAttr[idx].type_));
}
}
@@ -690,7 +691,8 @@ MediaNegotiationTest::testWithScenario(CallData& aliceData,
CPPUNIT_ASSERT_EQUAL(scenario.answerUpdate_[idx].muted_, mediaAttr[idx].muted_);
// Check isCaptureDeviceMuted API
- CPPUNIT_ASSERT_EQUAL(mediaAttr[idx].muted_, bobCall->isCaptureDeviceMuted(mediaAttr[idx].type_));
+ CPPUNIT_ASSERT_EQUAL(mediaAttr[idx].muted_,
+ bobCall->isCaptureDeviceMuted(mediaAttr[idx].type_));
}
}
}
@@ -717,11 +719,11 @@ MediaNegotiationTest::audio_and_video_then_mute_video()
configureScenario(aliceData_, bobData_);
MediaAttribute defaultAudio(MediaType::MEDIA_AUDIO);
- defaultAudio.label_ = "main audio";
+ defaultAudio.label_ = "audio_0";
defaultAudio.enabled_ = true;
MediaAttribute defaultVideo(MediaType::MEDIA_VIDEO);
- defaultVideo.label_ = "main video";
+ defaultVideo.label_ = "video_0";
defaultVideo.enabled_ = true;
{
@@ -762,11 +764,11 @@ MediaNegotiationTest::audio_only_then_add_video()
configureScenario(aliceData_, bobData_);
MediaAttribute defaultAudio(MediaType::MEDIA_AUDIO);
- defaultAudio.label_ = "main audio";
+ defaultAudio.label_ = "audio_0";
defaultAudio.enabled_ = true;
MediaAttribute defaultVideo(MediaType::MEDIA_VIDEO);
- defaultVideo.label_ = "main video";
+ defaultVideo.label_ = "video_0";
defaultVideo.enabled_ = true;
{
@@ -802,11 +804,11 @@ MediaNegotiationTest::audio_and_video_then_mute_audio()
configureScenario(aliceData_, bobData_);
MediaAttribute defaultAudio(MediaType::MEDIA_AUDIO);
- defaultAudio.label_ = "main audio";
+ defaultAudio.label_ = "audio_0";
defaultAudio.enabled_ = true;
MediaAttribute defaultVideo(MediaType::MEDIA_VIDEO);
- defaultVideo.label_ = "main video";
+ defaultVideo.label_ = "video_0";
defaultVideo.enabled_ = true;
{
@@ -851,11 +853,11 @@ MediaNegotiationTest::audio_only_then_add_video_but_peer_disabled_multistream()
configureScenario(aliceData_, bobData_);
MediaAttribute defaultAudio(MediaType::MEDIA_AUDIO);
- defaultAudio.label_ = "main audio";
+ defaultAudio.label_ = "audio_0";
defaultAudio.enabled_ = true;
MediaAttribute defaultVideo(MediaType::MEDIA_VIDEO);
- defaultVideo.label_ = "main video";
+ defaultVideo.label_ = "video_0";
defaultVideo.enabled_ = true;
{
--
GitLab