zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "Fix bug where we assume new m= sections will always be bundled."

Browse Source 
This reverts commit d2b885fd91909f1b17fb11292a8c989d5d883b22.

Reason for revert: Speculative revert for Chromium importer

Original change's description:
> Fix bug where we assume new m= sections will always be bundled.
>
> A recent change [1] assumes that all new m= sections will share the
> first BUNDLE group (if one already exists), which avoids generating
> ICE candidates that are ultimately unnecessary. This is fine for JSEP
> endpoints, but it breaks the following scenarios for non-JSEP endpoints:
>
> * Remote offer adding a new m= section that's not part of any BUNDLE
>   group.
> * Remote offer adding an m= section to the second BUNDLE group.
>
> The latter is specifically problematic for any application that wants
> to bundle all audio streams in one group and all video streams in
> another group when using Unified Plan SDP, to replicate the behavior of
> using Plan B without bundling. It may try to add a video stream only
> for WebRTC to bundle it with audio.
>
> This is fixed by doing some minor re-factoring, having BundleManager
> update the bundle groups at offer time.
>
> Also:
> * Added some additional validation for multiple bundle groups in a
>   subsequent offer, since that now becomes relevant.
> * Improved rollback support, because now rolling back an offer may need
>   to not only remove mid->transport mappings but alter them.
>
> [1]: https://webrtc-review.googlesource.com/c/src/+/221601
>
> Bug: webrtc:12906, webrtc:12999
> Change-Id: I4c6e7020c0be33a782d3608dee88e4e2fceb1be1
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/225642
> Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> Reviewed-by: Henrik Boström <hbos@webrtc.org>
> Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#34544}

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: webrtc:12906, webrtc:12999
Change-Id: I00179d7573f322ad539ff16cad1f85320cfb2270
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/227081
Reviewed-by: Björn Terelius <terelius@google.com>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34578}
This commit is contained in:
Björn Terelius
2021-07-27 22:03:35 +00:00
committed by WebRTC LUCI CQ
parent fd05d6f504
commit dc364e5bc2
7 changed files with 102 additions and 830 deletions
Download Patch File Download Diff File Expand all files Collapse all files

145
pc/jsep_transport_collection.cc
View File

@ -20,53 +20,21 @@
namespace webrtc {
void BundleManager::Update(const cricket::SessionDescription* description,
SdpType type) {
void BundleManager::Update(const cricket::SessionDescription* description) {
RTC_DCHECK_RUN_ON(&sequence_checker_);
// Rollbacks should call Rollback, not Update.
RTC_DCHECK(type != SdpType::kRollback);
bool bundle_groups_changed = false;
// TODO(bugs.webrtc.org/3349): Do this for kPrAnswer as well. To make this
// work, we also need to make sure PRANSWERs don't call
// MaybeDestroyJsepTransport, because the final answer may need the destroyed
// transport if it changes the BUNDLE group.
if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle ||
type == SdpType::kAnswer) {
// If our policy is "max-bundle" or this is an answer, update all bundle
// groups.
bundle_groups_changed = true;
bundle_groups_.clear();
for (const cricket::ContentGroup* new_bundle_group :
description->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)) {
bundle_groups_.push_back(
std::make_unique<cricket::ContentGroup>(*new_bundle_group));
RTC_DLOG(LS_VERBOSE) << "Establishing bundle group "
<< new_bundle_group->ToString();
}
} else if (type == SdpType::kOffer) {
// If this is an offer, update existing bundle groups.
// We do this because as per RFC 8843, section 7.3.2, the answerer cannot
// remove an m= section from an existing BUNDLE group without rejecting it.
// Thus any m= sections added to a BUNDLE group in this offer can
// preemptively start using the bundled transport, as there is no possible
// non-bundled fallback.
for (const cricket::ContentGroup* new_bundle_group :
description->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)) {
// Attempt to find a matching existing group.
for (const std::string& mid : new_bundle_group->content_names()) {
auto it = established_bundle_groups_by_mid_.find(mid);
if (it != established_bundle_groups_by_mid_.end()) {
*it->second = *new_bundle_group;
bundle_groups_changed = true;
RTC_DLOG(LS_VERBOSE)
<< "Establishing bundle group " << new_bundle_group->ToString();
break;
}
}
}
bundle_groups_.clear();
for (const cricket::ContentGroup* new_bundle_group :
description->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)) {
bundle_groups_.push_back(
std::make_unique<cricket::ContentGroup>(*new_bundle_group));
RTC_DLOG(LS_VERBOSE) << "Establishing bundle group "
<< new_bundle_group->ToString();
}
if (bundle_groups_changed) {
RefreshEstablishedBundleGroupsByMid();
established_bundle_groups_by_mid_.clear();
for (const auto& bundle_group : bundle_groups_) {
for (const std::string& content_name : bundle_group->content_names()) {
established_bundle_groups_by_mid_[content_name] = bundle_group.get();
}
}
}
@ -124,34 +92,6 @@ void BundleManager::DeleteGroup(const cricket::ContentGroup* bundle_group) {
bundle_groups_.erase(bundle_group_it);
}
void BundleManager::Rollback() {
RTC_DCHECK_RUN_ON(&sequence_checker_);
bundle_groups_.clear();
for (const auto& bundle_group : stable_bundle_groups_) {
bundle_groups_.push_back(
std::make_unique<cricket::ContentGroup>(*bundle_group));
}
RefreshEstablishedBundleGroupsByMid();
}
void BundleManager::Commit() {
RTC_DCHECK_RUN_ON(&sequence_checker_);
stable_bundle_groups_.clear();
for (const auto& bundle_group : bundle_groups_) {
stable_bundle_groups_.push_back(
std::make_unique<cricket::ContentGroup>(*bundle_group));
}
}
void BundleManager::RefreshEstablishedBundleGroupsByMid() {
established_bundle_groups_by_mid_.clear();
for (const auto& bundle_group : bundle_groups_) {
for (const std::string& content_name : bundle_group->content_names()) {
established_bundle_groups_by_mid_[content_name] = bundle_group.get();
}
}
}
void JsepTransportCollection::RegisterTransport(
const std::string& mid,
std::unique_ptr<cricket::JsepTransport> transport) {
@ -217,6 +157,8 @@ bool JsepTransportCollection::SetTransportForMid(
if (it != mid_to_transport_.end() && it->second == jsep_transport)
return true;
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.
@ -249,33 +191,17 @@ void JsepTransportCollection::RemoveTransportForMid(const std::string& mid) {
RTC_DCHECK(IsConsistent());
}
bool JsepTransportCollection::RollbackTransports() {
void JsepTransportCollection::RollbackTransports() {
RTC_DCHECK_RUN_ON(&sequence_checker_);
bool ret = true;
// First, remove any new mid->transport mappings.
for (const auto& kv : mid_to_transport_) {
if (stable_mid_to_transport_.count(kv.first) == 0) {
ret = ret && map_change_callback_(kv.first, nullptr);
}
for (auto&& mid : pending_mids_) {
RemoveTransportForMid(mid);
}
// Next, restore old mappings.
for (const auto& kv : stable_mid_to_transport_) {
auto it = mid_to_transport_.find(kv.first);
if (it == mid_to_transport_.end() || it->second != kv.second) {
ret = ret && map_change_callback_(kv.first, kv.second);
}
}
mid_to_transport_ = stable_mid_to_transport_;
DestroyUnusedTransports();
RTC_DCHECK(IsConsistent());
return ret;
pending_mids_.clear();
}
void JsepTransportCollection::CommitTransports() {
RTC_DCHECK_RUN_ON(&sequence_checker_);
stable_mid_to_transport_ = mid_to_transport_;
DestroyUnusedTransports();
RTC_DCHECK(IsConsistent());
pending_mids_.clear();
}
bool JsepTransportCollection::TransportInUse(
@ -286,11 +212,6 @@ bool JsepTransportCollection::TransportInUse(
return true;
}
}
for (const auto& kv : stable_mid_to_transport_) {
if (kv.second == jsep_transport) {
return true;
}
}
return false;
}
@ -298,7 +219,7 @@ void JsepTransportCollection::MaybeDestroyJsepTransport(
cricket::JsepTransport* transport) {
RTC_DCHECK_RUN_ON(&sequence_checker_);
// Don't destroy the JsepTransport if there are still media sections referring
// to it, or if it will be needed in case of rollback.
// to it.
if (TransportInUse(transport)) {
return;
}
@ -312,23 +233,6 @@ void JsepTransportCollection::MaybeDestroyJsepTransport(
RTC_DCHECK(IsConsistent());
}
void JsepTransportCollection::DestroyUnusedTransports() {
RTC_DCHECK_RUN_ON(&sequence_checker_);
bool need_state_change_callback = false;
auto it = jsep_transports_by_name_.begin();
while (it != jsep_transports_by_name_.end()) {
if (TransportInUse(it->second.get())) {
++it;
} else {
it = jsep_transports_by_name_.erase(it);
need_state_change_callback = true;
}
}
if (need_state_change_callback) {
state_change_callback_();
}
}
bool JsepTransportCollection::IsConsistent() {
RTC_DCHECK_RUN_ON(&sequence_checker_);
for (const auto& it : jsep_transports_by_name_) {
@ -337,6 +241,13 @@ bool JsepTransportCollection::IsConsistent() {
<< " is not in use, transport " << it.second.get();
return false;
}
const auto& lookup = mid_to_transport_.find(it.first);
if (lookup->second != it.second.get()) {
// Not an error, but unusual.
RTC_DLOG(LS_INFO) << "Note: Mid " << it.first << " was registered to "
<< it.second.get() << " but currently maps to "
<< lookup->second;
}
}
return true;
}