diff --git a/pc/jsep_transport_collection.cc b/pc/jsep_transport_collection.cc index 42d374ce51..96cbd9212c 100644 --- a/pc/jsep_transport_collection.cc +++ b/pc/jsep_transport_collection.cc @@ -55,7 +55,9 @@ void JsepTransportCollection::RegisterTransport( const std::string& mid, std::unique_ptr transport) { RTC_DCHECK_RUN_ON(&sequence_checker_); + SetTransportForMid(mid, transport.get()); jsep_transports_by_name_[mid] = std::move(transport); + RTC_DCHECK(IsConsistent()); } std::vector 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 diff --git a/pc/jsep_transport_collection.h b/pc/jsep_transport_collection.h index 307865490a..726ba348b2 100644 --- a/pc/jsep_transport_collection.h +++ b/pc/jsep_transport_collection.h @@ -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> diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index f193f02750..4d06d494fa 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -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(); } diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index a9470757e6..ef2c7ce089 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -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_);