diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 0288afca3b..d3b9258f38 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -3015,7 +3015,7 @@ static RTCError UpdateSimulcastLayerStatusInSender( const std::vector& layers, rtc::scoped_refptr sender) { RTC_DCHECK(sender); - RtpParameters parameters = sender->GetParameters(); + RtpParameters parameters = sender->GetParametersInternal(); std::vector disabled_layers; // The simulcast envelope cannot be changed, only the status of the streams. @@ -3034,7 +3034,7 @@ static RTCError UpdateSimulcastLayerStatusInSender( 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 UpdateSimulcastLayerStatusInSender( static RTCError DisableSimulcastInSender( rtc::scoped_refptr sender) { RTC_DCHECK(sender); - RtpParameters parameters = sender->GetParameters(); + RtpParameters parameters = sender->GetParametersInternal(); if (parameters.encodings.size() <= 1) { return RTCError::OK(); } @@ -4243,7 +4243,8 @@ GetMediaDescriptionOptionsForTransceiver( // 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 695d249cd9..07e079a2a3 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc @@ -108,6 +108,24 @@ class PeerConnectionSimulcastTests : public testing::Test { std::move(observer)); } + void ExchangeOfferAnswer(PeerConnectionWrapper* local, + PeerConnectionWrapper* remote, + const std::vector& 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& layers) { RtpTransceiverInit init; @@ -327,18 +345,8 @@ TEST_F(PeerConnectionSimulcastTests, SimulcastRejectedRemovesExtraLayers) { 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 @@ TEST_F(PeerConnectionSimulcastTests, TransceiverIsNotRecycledWithSimulcast) { 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 908d938b18..22957d3ae1 100644 --- a/pc/rtp_sender.cc +++ b/pc/rtp_sender.cc @@ -141,42 +141,29 @@ void RtpSenderBase::SetMediaChannel(cricket::MediaChannel* media_channel) { 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(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 @@ RTCError RtpSenderBase::SetParameters(const RtpParameters& parameters) { 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 @@ void RtpSenderBase::Stop() { RTCError RtpSenderBase::DisableEncodingLayers( const std::vector& 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 42a877ecc2..c786d906d8 100644 --- a/pc/rtp_sender.h +++ b/pc/rtp_sender.h @@ -55,6 +55,11 @@ class RtpSenderInternal : public RtpSenderInterface { 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 @@ class RtpSenderBase : public RtpSenderInternal, public ObserverInterface { 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 c10ac87cef..d55e4f46f6 100644 --- a/pc/rtp_sender_receiver_unittest.cc +++ b/pc/rtp_sender_receiver_unittest.cc @@ -1388,6 +1388,7 @@ TEST_F(RtpSenderReceiverTest, 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 @@ TEST_F(RtpSenderReceiverTest, VideoReceiverCannotSetFrameDecryptorAfterStop) { // 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 CreatePairOfRidVectors( const std::vector& first, diff --git a/pc/test/mock_rtp_sender_internal.h b/pc/test/mock_rtp_sender_internal.h index a497cb5c3c..fa16220e9a 100644 --- a/pc/test/mock_rtp_sender_internal.h +++ b/pc/test/mock_rtp_sender_internal.h @@ -34,7 +34,9 @@ class MockRtpSenderInternal : public RtpSenderInternal { MOCK_CONST_METHOD0(init_send_encodings, std::vector()); MOCK_METHOD1(set_transport, void(rtc::scoped_refptr)); 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()); MOCK_METHOD1(SetFrameEncryptor, void(rtc::scoped_refptr));