From c14fdcb6463edd8a64ff723b52829bea91e0b1ca Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Wed, 8 Apr 2020 11:44:19 -0400
Subject: [PATCH] sipcall: fix audio binding on offHold
offHold can return false even if the action is pushed in the
requests. This means, that the audio will not be binded to the
call if offHold is waiting for ICE.
Change-Id: I0fbdf4682a613919cee1ae5b2242077d1bd57fd7
---
src/call.h | 11 ++++++----
src/manager.cpp | 49 +++++++++++++++++++++++--------------------
src/sip/sipcall.cpp | 51 ++++++++++++++++++++++++++++++++++++++-------
src/sip/sipcall.h | 11 ++++++++--
4 files changed, 86 insertions(+), 36 deletions(-)
diff --git a/src/call.h b/src/call.h
index 02b2c8c4f6..658a61b055 100644
--- a/src/call.h
+++ b/src/call.h
@@ -59,6 +59,7 @@ class Call : public Recordable, public std::enable_shared_from_this<Call> {
public:
using SubcallSet = std::set<std::shared_ptr<Call>, std::owner_less<std::shared_ptr<Call>>>;
using OnNeedFallbackCb = std::function<void()>;
+ using OnReadyCb = std::function<void(bool)>;
static const char * const DEFAULT_ID;
@@ -229,15 +230,17 @@ class Call : public Recordable, public std::enable_shared_from_this<Call> {
/**
* Put a call on hold
- * @return bool True on success
+ * @param cb On hold can be queued if waiting for ICE. This callback will be called when ready
+ * @return bool True on success, False if failed or pending
*/
- virtual bool onhold() = 0;
+ virtual bool onhold(OnReadyCb&& cb) = 0;
/**
* Resume a call from hold state
- * @return bool True on success
+ * @param cb On hold can be queued if waiting for ICE. This callback will be called when ready
+ * @return bool True on success, False if failed or pending
*/
- virtual bool offhold() = 0;
+ virtual bool offhold(OnReadyCb&& cb) = 0;
virtual void sendKeyframe() = 0;
diff --git a/src/manager.cpp b/src/manager.cpp
index 0e976aa755..dcace10ef5 100644
--- a/src/manager.cpp
+++ b/src/manager.cpp
@@ -1064,29 +1064,29 @@ Manager::onHoldCall(const std::string& callId)
if (auto call = getCallFromCallID(callId)) {
try {
- result = call->onhold();
- if (result)
+ result = call->onhold([=](bool ok) {
+ if (!ok) {
+ JAMI_ERR("hold failed for call %s", callId.c_str());
+ return;
+ }
removeAudio(*call); // Unbind calls in main buffer
+ // Remove call from the queue if it was still there
+ pimpl_->removeWaitingCall(callId);
+
+ // keeps current call id if the action is not holding this call
+ // or a new outgoing call. This could happen in case of a conference
+ if (current_call_id == callId)
+ pimpl_->unsetCurrentCall();
+ });
} catch (const VoipLinkException &e) {
JAMI_ERR("%s", e.what());
result = false;
}
-
} else {
JAMI_DBG("CallID %s doesn't exist in call onHold", callId.c_str());
return false;
}
- if (result) {
- // Remove call from the queue if it was still there
- pimpl_->removeWaitingCall(callId);
-
- // keeps current call id if the action is not holding this call
- // or a new outgoing call. This could happen in case of a conference
- if (current_call_id == callId)
- pimpl_->unsetCurrentCall();
- }
-
return result;
}
@@ -1103,21 +1103,24 @@ Manager::offHoldCall(const std::string& callId)
return false;
try {
- result = call->offhold();
+ result = call->offhold([=](bool ok) {
+ if (!ok) {
+ JAMI_ERR("off hold failed for call %s", callId.c_str());
+ return;
+ }
+
+ if (isConferenceParticipant(callId))
+ pimpl_->switchCall(call->getConfId());
+ else
+ pimpl_->switchCall(call);
+
+ addAudio(*call);
+ });
} catch (const VoipLinkException &e) {
JAMI_ERR("%s", e.what());
return false;
}
- if (result) {
- if (isConferenceParticipant(callId))
- pimpl_->switchCall(call->getConfId());
- else
- pimpl_->switchCall(call);
-
- addAudio(*call);
- }
-
return result;
}
diff --git a/src/sip/sipcall.cpp b/src/sip/sipcall.cpp
index f90d5488b9..238189e718 100644
--- a/src/sip/sipcall.cpp
+++ b/src/sip/sipcall.cpp
@@ -582,13 +582,26 @@ SIPCall::attendedTransfer(const std::string& to)
}
bool
-SIPCall::onhold()
+SIPCall::onhold(OnReadyCb&& cb)
{
// If ICE is currently negotiating, we must wait before hold the call
if (isWaitingForIceAndMedia_) {
+ holdCb_ = std::move(cb);
remainingRequest_ = Request::HoldingOn;
return false;
}
+
+ auto result = hold();
+
+ if (cb)
+ cb(result);
+
+ return result;
+}
+
+bool
+SIPCall::hold()
+{
if (not setState(CallState::HOLD))
return false;
@@ -602,22 +615,33 @@ SIPCall::onhold()
}
isWaitingForIceAndMedia_ = true;
-
return true;
}
bool
-SIPCall::offhold()
+SIPCall::offhold(OnReadyCb&& cb)
{
- bool success = false;
// If ICE is currently negotiating, we must wait before unhold the call
if (isWaitingForIceAndMedia_) {
+ offHoldCb_ = std::move(cb);
remainingRequest_ = Request::HoldingOff;
return false;
}
+ auto result = unhold();
+
+ if (cb)
+ cb(result);
+
+ return result;
+}
+
+bool
+SIPCall::unhold()
+{
auto& account = getSIPAccount();
+ bool success = false;
try {
if (account.isStunEnabled())
success = internalOffHold([&] { updateSDPFromSTUN(); });
@@ -645,7 +669,11 @@ SIPCall::internalOffHold(const std::function<void()>& sdp_cb)
if (getConnectionState() == ConnectionState::CONNECTED) {
if (SIPSessionReinvite() != PJ_SUCCESS) {
JAMI_WARN("[call:%s] resuming hold", getCallId().c_str());
- onhold();
+ if (isWaitingForIceAndMedia_) {
+ remainingRequest_ = Request::HoldingOn;
+ } else {
+ hold();
+ }
return false;
}
}
@@ -1023,12 +1051,21 @@ SIPCall::startAllMedia()
// Media is restarted, we can process the last holding request.
isWaitingForIceAndMedia_ = false;
if (remainingRequest_ != Request::NoRequest) {
+ bool result = true;
switch (remainingRequest_) {
case Request::HoldingOn:
- onhold();
+ result = hold();
+ if (holdCb_) {
+ holdCb_(result);
+ holdCb_ = nullptr;
+ }
break;
case Request::HoldingOff:
- offhold();
+ result = unhold();
+ if (offHoldCb_) {
+ offHoldCb_(result);
+ offHoldCb_ = nullptr;
+ }
break;
case Request::SwitchInput:
SIPSessionReinvite();
diff --git a/src/sip/sipcall.h b/src/sip/sipcall.h
index 8fb180d9b8..c84440637b 100644
--- a/src/sip/sipcall.h
+++ b/src/sip/sipcall.h
@@ -94,8 +94,8 @@ public: // overridden
void refuse() override;
void transfer(const std::string& to) override;
bool attendedTransfer(const std::string& to) override;
- bool onhold() override;
- bool offhold() override;
+ bool onhold(OnReadyCb&& cb) override;
+ bool offhold(OnReadyCb&& cb) override;
void switchInput(const std::string& resource) override;
void peerHungup() override;
void carryingDTMFdigits(char code) override;
@@ -264,6 +264,10 @@ private:
bool internalOffHold(const std::function<void()> &SDPUpdateFunc);
+ bool hold();
+
+ bool unhold();
+
int SIPSessionReinvite();
std::vector<IceCandidate> getAllRemoteCandidates();
@@ -343,6 +347,9 @@ private:
bool pendingRecord_ {false};
time_point lastKeyFrameReq_ {time_point::min()};
+
+ OnReadyCb holdCb_ {};
+ OnReadyCb offHoldCb_ {};
};
// Helpers
--
GitLab