admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Make JsepTransportCollection self-managing for transports

Browse Source 
This entails instantly deleting the transport when it is no longer
referenced by any MID.

Also adds consistency checks to JsepTransportCollection.

Bug: webrtc:12837
Change-Id: I85775aeb676aac3a9aee74280cc72ac87a0f49b5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221982
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34273}
This commit is contained in:
Harald Alvestrand 2021-06-11 13:04:59 +00:00 committed by WebRTC LUCI CQ
parent 63c96ce3c7
commit 4bb81aca75
4 changed files with 60 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

61
pc/jsep_transport_collection.cc
View File

@ -55,7 +55,9 @@ void JsepTransportCollection::RegisterTransport(
const std::string& mid,
std::unique_ptr<cricket::JsepTransport> transport) {
RTC_DCHECK_RUN_ON(&sequence_checker_);
SetTransportForMid(mid, transport.get());
jsep_transports_by_name_[mid] = std::move(transport);
RTC_DCHECK(IsConsistent());
}
std::vector<cricket::JsepTransport*> JsepTransportCollection::Transports() {
@ -73,6 +75,7 @@ void JsepTransportCollection::DestroyAllTransports() {
map_change_callback_(jsep_transport.first, nullptr);
}
jsep_transports_by_name_.clear();
RTC_DCHECK(IsConsistent());
}
const cricket::JsepTransport* JsepTransportCollection::GetTransportByName(
@ -115,23 +118,36 @@ bool JsepTransportCollection::SetTransportForMid(
pending_mids_.push_back(mid);
// The map_change_callback must be called before destroying the
// transport, because it removes references to the transport
// in the RTP demuxer.
bool result = map_change_callback_(mid, jsep_transport);
if (it == mid_to_transport_.end()) {
mid_to_transport_.insert(std::make_pair(mid, jsep_transport));
} else {
auto old_transport = it->second;
it->second = jsep_transport;
MaybeDestroyJsepTransport(old_transport);
}
return map_change_callback_(mid, jsep_transport);
RTC_DCHECK(IsConsistent());
return result;
}
void JsepTransportCollection::RemoveTransportForMid(const std::string& mid) {
RTC_DCHECK_RUN_ON(&sequence_checker_);
RTC_DCHECK(IsConsistent());
bool ret = map_change_callback_(mid, nullptr);
// Calling OnTransportChanged with nullptr should always succeed, since it is
// only expected to fail when adding media to a transport (not removing).
RTC_DCHECK(ret);
mid_to_transport_.erase(mid);
auto old_transport = GetTransportForMid(mid);
if (old_transport) {
mid_to_transport_.erase(mid);
MaybeDestroyJsepTransport(old_transport);
}
RTC_DCHECK(IsConsistent());
}
void JsepTransportCollection::RollbackTransports() {
@ -139,9 +155,6 @@ void JsepTransportCollection::RollbackTransports() {
for (auto&& mid : pending_mids_) {
RemoveTransportForMid(mid);
}
for (auto&& mid : pending_mids_) {
MaybeDestroyJsepTransport(mid);
}
pending_mids_.clear();
}
@ -162,21 +175,39 @@ bool JsepTransportCollection::TransportInUse(
}
void JsepTransportCollection::MaybeDestroyJsepTransport(
const std::string& mid) {
cricket::JsepTransport* transport) {
RTC_DCHECK_RUN_ON(&sequence_checker_);
auto it = jsep_transports_by_name_.find(mid);
if (it == jsep_transports_by_name_.end()) {
return;
}
// Don't destroy the JsepTransport if there are still media sections referring
// to it.
if (TransportInUse(it->second.get())) {
if (TransportInUse(transport)) {
return;
}
for (const auto& it : jsep_transports_by_name_) {
if (it.second.get() == transport) {
jsep_transports_by_name_.erase(it.first);
state_change_callback_();
break;
}
}
RTC_DCHECK(IsConsistent());
}
jsep_transports_by_name_.erase(mid);
state_change_callback_();
bool JsepTransportCollection::IsConsistent() {
RTC_DCHECK_RUN_ON(&sequence_checker_);
for (const auto& it : jsep_transports_by_name_) {
if (!TransportInUse(it.second.get())) {
RTC_LOG(LS_ERROR) << "Transport registered with mid " << it.first
<< " is not in use, transport " << it.second;
return false;
}
const auto& lookup = mid_to_transport_.find(it.first);
if (lookup->second != it.second.get()) {
RTC_LOG(LS_ERROR) << "Note: Mid " << it.first << " was registered to "
<< it.second.get() << " but currently maps to "
<< lookup->second;
}
}
return true;
}
} // namespace webrtc

11
pc/jsep_transport_collection.h
View File

@ -88,8 +88,12 @@ class JsepTransportCollection {
cricket::JsepTransport* GetTransportForMid(const std::string& mid);
const cricket::JsepTransport* GetTransportForMid(
const std::string& mid) const;
// Set transport for a MID. This may destroy a transport if it is no
// longer in use.
bool SetTransportForMid(const std::string& mid,
cricket::JsepTransport* jsep_transport);
// Remove a transport for a MID. This may destroy a transport if it is
// no longer in use.
void RemoveTransportForMid(const std::string& mid);
// Roll back pending mid-to-transport mappings.
void RollbackTransports();
@ -97,10 +101,13 @@ class JsepTransportCollection {
void CommitTransports();
// Returns true if any mid currently maps to this transport.
bool TransportInUse(cricket::JsepTransport* jsep_transport) const;
// Destroy a transport if it's no longer in use.
void MaybeDestroyJsepTransport(const std::string& mid);
private:
// Destroy a transport if it's no longer in use.
void MaybeDestroyJsepTransport(cricket::JsepTransport* transport);
bool IsConsistent(); // For testing only: Verify internal structure.
RTC_NO_UNIQUE_ADDRESS SequenceChecker sequence_checker_;
// This member owns the JSEP transports.
std::map<std::string, std::unique_ptr<cricket::JsepTransport>>

34
pc/jsep_transport_controller.cc
View File

@ -855,7 +855,6 @@ void JsepTransportController::HandleRejectedContent(
bundles_.DeleteMid(bundle_group, content_info.name);
}
}
MaybeDestroyJsepTransport(content_info.name);
}
bool JsepTransportController::HandleBundledContent(
@ -869,15 +868,11 @@ bool JsepTransportController::HandleBundledContent(
// If the content is bundled, let the
// BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first,
// then destroy the cricket::JsepTransport.
if (transports_.SetTransportForMid(content_info.name, jsep_transport)) {
// TODO(bugs.webrtc.org/9719) For media transport this is far from ideal,
// because it means that we first create media transport and start
// connecting it, and then we destroy it. We will need to address it before
// video path is enabled.
MaybeDestroyJsepTransport(content_info.name);
return true;
}
return false;
// TODO(bugs.webrtc.org/9719) For media transport this is far from ideal,
// because it means that we first create media transport and start
// connecting it, and then we destroy it. We will need to address it before
// video path is enabled.
return transports_.SetTransportForMid(content_info.name, jsep_transport);
}
cricket::JsepTransportDescription
@ -1078,30 +1073,11 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
jsep_transport->SignalRtcpMuxActive.connect(
this, &JsepTransportController::UpdateAggregateStates_n);
transports_.SetTransportForMid(content_info.name, jsep_transport.get());
transports_.RegisterTransport(content_info.name, std::move(jsep_transport));
UpdateAggregateStates_n();
return RTCError::OK();
}
void JsepTransportController::MaybeDestroyJsepTransport(
const std::string& mid) {
TRACE_EVENT0("webrtc", "JsepTransportController::MaybeDestroyJsepTransport");
auto jsep_transport = GetJsepTransportByName(mid);
if (!jsep_transport) {
return;
}
// Don't destroy the JsepTransport if there are still media sections referring
// to it.
if (transports_.TransportInUse(jsep_transport)) {
return;
}
transports_.MaybeDestroyJsepTransport(mid);
UpdateAggregateStates_n();
}
void JsepTransportController::DestroyAllJsepTransports_n() {
transports_.DestroyAllTransports();
}

2
pc/jsep_transport_controller.h
View File

@ -381,8 +381,6 @@ class JsepTransportController : public sigslot::has_slots<> {
const cricket::SessionDescription& description)
RTC_RUN_ON(network_thread_);
void MaybeDestroyJsepTransport(const std::string& mid)
RTC_RUN_ON(network_thread_);
void DestroyAllJsepTransports_n() RTC_RUN_ON(network_thread_);
void SetIceRole_n(cricket::IceRole ice_role) RTC_RUN_ON(network_thread_);