RtpSender's RtpParameters were invalidated in a call to SLD/SRD.
RtpSender uses a transactional model when getting and setting RtpParameters.
One must call GetParameters() and then can use the returned object in a
subsequent call to SetParameters().
PeerConnection was calling GetParameters() and SetParameters() during
negotiation in SetLocalDescription and SetRemoteDescription effectively
invalidating any parameters that the client previously held.
This change introduces an internal way for the platform to modify
parameters without invalidating the transactional model, provided that
the modification is not severe.
Ex. removing simulcast layers is a severe modification and will
invalidate any outstanding parameters.
Bug: webrtc:10339
Change-Id: I362e8ca4d9556e04a1aa7a3e74e2c275f8d16fbc
Reviewed-on: https://webrtc-review.googlesource.com/c/124504
Reviewed-by: Seth Hampson <shampson@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Amit Hilbuch <amithi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26864}diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 0288afc..d3b9258 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -3015,7 +3015,7 @@
const std::vector<SimulcastLayer>& layers,
rtc::scoped_refptr<RtpSenderInternal> sender) {
RTC_DCHECK(sender);
- RtpParameters parameters = sender->GetParameters();
+ RtpParameters parameters = sender->GetParametersInternal();
std::vector<std::string> disabled_layers;
// The simulcast envelope cannot be changed, only the status of the streams.
@@ -3034,7 +3034,7 @@
encoding.active = !iter->is_paused;
}
- RTCError result = sender->SetParameters(parameters);
+ RTCError result = sender->SetParametersInternal(parameters);
if (result.ok()) {
result = sender->DisableEncodingLayers(disabled_layers);
}
@@ -3045,7 +3045,7 @@
static RTCError DisableSimulcastInSender(
rtc::scoped_refptr<RtpSenderInternal> sender) {
RTC_DCHECK(sender);
- RtpParameters parameters = sender->GetParameters();
+ RtpParameters parameters = sender->GetParametersInternal();
if (parameters.encodings.size() <= 1) {
return RTCError::OK();
}
@@ -4243,7 +4243,8 @@
// The following sets up RIDs and Simulcast.
// RIDs are included if Simulcast is requested or if any RID was specified.
- RtpParameters send_parameters = transceiver->sender()->GetParameters();
+ RtpParameters send_parameters =
+ transceiver->internal()->sender_internal()->GetParametersInternal();
bool has_rids = std::any_of(send_parameters.encodings.begin(),
send_parameters.encodings.end(),
[](const RtpEncodingParameters& encoding) {
diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc
index 695d249..07e079a 100644
--- a/pc/peer_connection_simulcast_unittest.cc
+++ b/pc/peer_connection_simulcast_unittest.cc
@@ -108,6 +108,24 @@
std::move(observer));
}
+ void ExchangeOfferAnswer(PeerConnectionWrapper* local,
+ PeerConnectionWrapper* remote,
+ const std::vector<SimulcastLayer>& answer_layers) {
+ auto offer = local->CreateOfferAndSetAsLocal();
+ // Remove simulcast as the second peer connection won't support it.
+ auto removed_simulcast = RemoveSimulcast(offer.get());
+ std::string err;
+ EXPECT_TRUE(remote->SetRemoteDescription(std::move(offer), &err)) << err;
+ auto answer = remote->CreateAnswerAndSetAsLocal();
+ // Setup the answer to look like a server response.
+ auto mcd_answer = answer->description()->contents()[0].media_description();
+ auto& receive_layers = mcd_answer->simulcast_description().receive_layers();
+ for (const SimulcastLayer& layer : answer_layers) {
+ receive_layers.AddLayer(layer);
+ }
+ EXPECT_TRUE(local->SetRemoteDescription(std::move(answer), &err)) << err;
+ }
+
RtpTransceiverInit CreateTransceiverInit(
const std::vector<SimulcastLayer>& layers) {
RtpTransceiverInit init;
@@ -327,18 +345,8 @@
auto local = CreatePeerConnectionWrapper();
auto remote = CreatePeerConnectionWrapper();
auto layers = CreateLayers({"1", "2", "3", "4"}, true);
- // The number of layers does not change.
- auto expected_layers = CreateLayers({"1", "2", "3", "4"}, false);
- RTC_DCHECK_EQ(layers.size(), expected_layers.size());
- expected_layers[0].is_paused = false;
auto transceiver = AddTransceiver(local.get(), layers);
- auto offer = local->CreateOfferAndSetAsLocal();
- // Remove simulcast as the second peer connection won't support it.
- RemoveSimulcast(offer.get());
- std::string error;
- EXPECT_TRUE(remote->SetRemoteDescription(std::move(offer), &error)) << error;
- auto answer = remote->CreateAnswerAndSetAsLocal();
- EXPECT_TRUE(local->SetRemoteDescription(std::move(answer), &error)) << error;
+ ExchangeOfferAnswer(local.get(), remote.get(), {});
auto parameters = transceiver->sender()->GetParameters();
// Should only have the first layer.
EXPECT_THAT(parameters.encodings,
@@ -416,4 +424,41 @@
EXPECT_TRUE(ValidateTransceiverParameters(transceiver, layers));
}
+// Checks that if the number of layers changes during negotiation, then any
+// outstanding get/set parameters transaction is invalidated.
+TEST_F(PeerConnectionSimulcastTests, ParametersAreInvalidatedWhenLayersChange) {
+ auto local = CreatePeerConnectionWrapper();
+ auto remote = CreatePeerConnectionWrapper();
+ auto layers = CreateLayers({"1", "2", "3"}, true);
+ auto transceiver = AddTransceiver(local.get(), layers);
+ auto parameters = transceiver->sender()->GetParameters();
+ ASSERT_EQ(3u, parameters.encodings.size());
+ // Response will reject simulcast altogether.
+ ExchangeOfferAnswer(local.get(), remote.get(), {});
+ auto result = transceiver->sender()->SetParameters(parameters);
+ EXPECT_EQ(RTCErrorType::INVALID_STATE, result.type());
+}
+
+// Checks that even though negotiation modifies the sender's parameters, an
+// outstanding get/set parameters transaction is not invalidated.
+// This test negotiates twice because initial parameters before negotiation
+// is missing critical information and cannot be set on the sender.
+TEST_F(PeerConnectionSimulcastTests,
+ NegotiationDoesNotInvalidateParameterTransactions) {
+ auto local = CreatePeerConnectionWrapper();
+ auto remote = CreatePeerConnectionWrapper();
+ auto layers = CreateLayers({"1", "2", "3"}, true);
+ auto expected_layers = CreateLayers({"1", "2", "3"}, false);
+ auto transceiver = AddTransceiver(local.get(), layers);
+ ExchangeOfferAnswer(local.get(), remote.get(), expected_layers);
+
+ // Verify that negotiation does not invalidate the parameters.
+ auto parameters = transceiver->sender()->GetParameters();
+ ExchangeOfferAnswer(local.get(), remote.get(), expected_layers);
+
+ auto result = transceiver->sender()->SetParameters(parameters);
+ EXPECT_TRUE(result.ok());
+ EXPECT_TRUE(ValidateTransceiverParameters(transceiver, expected_layers));
+}
+
} // namespace webrtc
diff --git a/pc/rtp_sender.cc b/pc/rtp_sender.cc
index 908d938..22957d3 100644
--- a/pc/rtp_sender.cc
+++ b/pc/rtp_sender.cc
@@ -141,42 +141,29 @@
media_channel_ = media_channel;
}
-RtpParameters RtpSenderBase::GetParameters() const {
+RtpParameters RtpSenderBase::GetParametersInternal() const {
if (stopped_) {
return RtpParameters();
}
if (!media_channel_ || !ssrc_) {
- RtpParameters result = init_parameters_;
- last_transaction_id_ = rtc::CreateRandomUuid();
- result.transaction_id = last_transaction_id_.value();
- return result;
+ return init_parameters_;
}
return worker_thread_->Invoke<RtpParameters>(RTC_FROM_HERE, [&] {
RtpParameters result = media_channel_->GetRtpSendParameters(ssrc_);
RemoveEncodingLayers(disabled_rids_, &result.encodings);
- last_transaction_id_ = rtc::CreateRandomUuid();
- result.transaction_id = last_transaction_id_.value();
return result;
});
}
-RTCError RtpSenderBase::SetParameters(const RtpParameters& parameters) {
- TRACE_EVENT0("webrtc", "RtpSenderBase::SetParameters");
- if (stopped_) {
- return RTCError(RTCErrorType::INVALID_STATE);
- }
- if (!last_transaction_id_) {
- LOG_AND_RETURN_ERROR(
- RTCErrorType::INVALID_STATE,
- "Failed to set parameters since getParameters() has never been called"
- " on this sender");
- }
- if (last_transaction_id_ != parameters.transaction_id) {
- LOG_AND_RETURN_ERROR(
- RTCErrorType::INVALID_MODIFICATION,
- "Failed to set parameters since the transaction_id doesn't match"
- " the last value returned from getParameters()");
- }
+RtpParameters RtpSenderBase::GetParameters() const {
+ RtpParameters result = GetParametersInternal();
+ last_transaction_id_ = rtc::CreateRandomUuid();
+ result.transaction_id = last_transaction_id_.value();
+ return result;
+}
+
+RTCError RtpSenderBase::SetParametersInternal(const RtpParameters& parameters) {
+ RTC_DCHECK(!stopped_);
if (UnimplementedRtpParameterHasValue(parameters)) {
LOG_AND_RETURN_ERROR(
@@ -200,13 +187,34 @@
rtp_parameters = RestoreEncodingLayers(parameters, disabled_rids_,
old_parameters.encodings);
}
- RTCError result =
- media_channel_->SetRtpSendParameters(ssrc_, rtp_parameters);
- last_transaction_id_.reset();
- return result;
+ return media_channel_->SetRtpSendParameters(ssrc_, rtp_parameters);
});
}
+RTCError RtpSenderBase::SetParameters(const RtpParameters& parameters) {
+ TRACE_EVENT0("webrtc", "RtpSenderBase::SetParameters");
+ if (stopped_) {
+ LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE,
+ "Cannot set parameters on a stopped sender.");
+ }
+ if (!last_transaction_id_) {
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::INVALID_STATE,
+ "Failed to set parameters since getParameters() has never been called"
+ " on this sender");
+ }
+ if (last_transaction_id_ != parameters.transaction_id) {
+ LOG_AND_RETURN_ERROR(
+ RTCErrorType::INVALID_MODIFICATION,
+ "Failed to set parameters since the transaction_id doesn't match"
+ " the last value returned from getParameters()");
+ }
+
+ RTCError result = SetParametersInternal(parameters);
+ last_transaction_id_.reset();
+ return result;
+}
+
bool RtpSenderBase::SetTrack(MediaStreamTrackInterface* track) {
TRACE_EVENT0("webrtc", "RtpSenderBase::SetTrack");
if (stopped_) {
@@ -315,31 +323,45 @@
RTCError RtpSenderBase::DisableEncodingLayers(
const std::vector<std::string>& rids) {
if (stopped_) {
- return RTCError(RTCErrorType::INVALID_STATE);
+ LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE,
+ "Cannot disable encodings on a stopped sender.");
}
- if (!media_channel_ || !ssrc_) {
- RemoveEncodingLayers(rids, &init_parameters_.encodings);
+ if (rids.empty()) {
return RTCError::OK();
}
// Check that all the specified layers exist and disable them in the channel.
- RtpParameters parameters = GetParameters();
+ RtpParameters parameters = GetParametersInternal();
for (const std::string& rid : rids) {
- auto iter = absl::c_find_if(parameters.encodings,
- [&rid](const RtpEncodingParameters& encoding) {
- return encoding.rid == rid;
- });
- if (iter == parameters.encodings.end()) {
- return RTCError(RTCErrorType::INVALID_PARAMETER,
- "RID: " + rid + " does not refer to a valid layer.");
+ if (absl::c_none_of(parameters.encodings,
+ [&rid](const RtpEncodingParameters& encoding) {
+ return encoding.rid == rid;
+ })) {
+ LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
+ "RID: " + rid + " does not refer to a valid layer.");
}
- iter->active = false;
}
- RTCError result = SetParameters(parameters);
+ if (!media_channel_ || !ssrc_) {
+ RemoveEncodingLayers(rids, &init_parameters_.encodings);
+ // Invalidate any transaction upon success.
+ last_transaction_id_.reset();
+ return RTCError::OK();
+ }
+
+ for (RtpEncodingParameters& encoding : parameters.encodings) {
+ // Remain active if not in the disable list.
+ encoding.active &= absl::c_none_of(
+ rids,
+ [&encoding](const std::string& rid) { return encoding.rid == rid; });
+ }
+
+ RTCError result = SetParametersInternal(parameters);
if (result.ok()) {
disabled_rids_.insert(disabled_rids_.end(), rids.begin(), rids.end());
+ // Invalidate any transaction upon success.
+ last_transaction_id_.reset();
}
return result;
}
diff --git a/pc/rtp_sender.h b/pc/rtp_sender.h
index 42a877e..c786d90 100644
--- a/pc/rtp_sender.h
+++ b/pc/rtp_sender.h
@@ -55,6 +55,11 @@
virtual void Stop() = 0;
+ // |GetParameters| and |SetParameters| operate with a transactional model.
+ // Allow access to get/set parameters without invalidating transaction id.
+ virtual RtpParameters GetParametersInternal() const = 0;
+ virtual RTCError SetParametersInternal(const RtpParameters& parameters) = 0;
+
// Returns an ID that changes every time SetTrack() is called, but
// otherwise remains constant. Used to generate IDs for stats.
// The special value zero means that no track is attached.
@@ -83,6 +88,11 @@
RtpParameters GetParameters() const override;
RTCError SetParameters(const RtpParameters& parameters) override;
+ // |GetParameters| and |SetParameters| operate with a transactional model.
+ // Allow access to get/set parameters without invalidating transaction id.
+ RtpParameters GetParametersInternal() const override;
+ RTCError SetParametersInternal(const RtpParameters& parameters) override;
+
// Used to set the SSRC of the sender, once a local description has been set.
// If |ssrc| is 0, this indiates that the sender should disconnect from the
// underlying transport (this occurs if the sender isn't seen in a local
diff --git a/pc/rtp_sender_receiver_unittest.cc b/pc/rtp_sender_receiver_unittest.cc
index c10ac87..d55e4f4 100644
--- a/pc/rtp_sender_receiver_unittest.cc
+++ b/pc/rtp_sender_receiver_unittest.cc
@@ -1388,6 +1388,7 @@
params.encodings[i].dependency_rids.push_back("dummy_rid");
EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER,
video_rtp_sender_->SetParameters(params).type());
+ params = video_rtp_sender_->GetParameters();
}
DestroyVideoRtpSender();
@@ -1773,6 +1774,21 @@
// TODO(webrtc:9926) - Validate media channel not set once fakes updated.
}
+// Checks that calling the internal methods for get/set parameters do not
+// invalidate any parameters retreived by clients.
+TEST_F(RtpSenderReceiverTest,
+ InternalParameterMethodsDoNotInvalidateTransaction) {
+ CreateVideoRtpSender();
+ RtpParameters parameters = video_rtp_sender_->GetParameters();
+ RtpParameters new_parameters = video_rtp_sender_->GetParametersInternal();
+ new_parameters.encodings[0].active = false;
+ video_rtp_sender_->SetParametersInternal(new_parameters);
+ new_parameters.encodings[0].active = true;
+ video_rtp_sender_->SetParametersInternal(new_parameters);
+ parameters.encodings[0].active = false;
+ EXPECT_TRUE(video_rtp_sender_->SetParameters(parameters).ok());
+}
+
// Helper method for syntactic sugar for accepting a vector with '{}' notation.
std::pair<RidList, RidList> CreatePairOfRidVectors(
const std::vector<std::string>& first,
diff --git a/pc/test/mock_rtp_sender_internal.h b/pc/test/mock_rtp_sender_internal.h
index a497cb5..fa16220 100644
--- a/pc/test/mock_rtp_sender_internal.h
+++ b/pc/test/mock_rtp_sender_internal.h
@@ -34,7 +34,9 @@
MOCK_CONST_METHOD0(init_send_encodings, std::vector<RtpEncodingParameters>());
MOCK_METHOD1(set_transport, void(rtc::scoped_refptr<DtlsTransportInterface>));
MOCK_CONST_METHOD0(GetParameters, RtpParameters());
+ MOCK_CONST_METHOD0(GetParametersInternal, RtpParameters());
MOCK_METHOD1(SetParameters, RTCError(const RtpParameters&));
+ MOCK_METHOD1(SetParametersInternal, RTCError(const RtpParameters&));
MOCK_CONST_METHOD0(GetDtmfSender, rtc::scoped_refptr<DtmfSenderInterface>());
MOCK_METHOD1(SetFrameEncryptor,
void(rtc::scoped_refptr<FrameEncryptorInterface>));