From 0166be8208752f4bab3cd4e6e7d50c63711646c1 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 25 Aug 2022 11:31:01 +0000 Subject: [PATCH] Let SDP operations always look at all simulcast layers This simplifies the logic of what simulcast layers to signal, and avoids situations where the upper layers get confused about which layers exist. Bug: chromium:1350245 Change-Id: I9edeb93cbb30e872c4d3f3429a85a1fccf17996a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/272902 Commit-Queue: Harald Alvestrand Reviewed-by: Florent Castelli Cr-Commit-Position: refs/heads/main@{#37905} --- pc/rtp_sender.cc | 42 ++++++++++++++++++++++++++++-- pc/rtp_sender.h | 10 +++++++ pc/sdp_offer_answer.cc | 8 +++--- pc/test/mock_rtp_sender_internal.h | 8 ++++++ 4 files changed, 62 insertions(+), 6 deletions(-) diff --git a/pc/rtp_sender.cc b/pc/rtp_sender.cc index bf41f97cf2..b42e439b2e 100644 --- a/pc/rtp_sender.cc +++ b/pc/rtp_sender.cc @@ -169,6 +169,20 @@ RtpParameters RtpSenderBase::GetParametersInternal() const { }); } +RtpParameters RtpSenderBase::GetParametersInternalWithAllLayers() const { + RTC_DCHECK_RUN_ON(signaling_thread_); + if (stopped_) { + return RtpParameters(); + } + if (!media_channel_ || !ssrc_) { + return init_parameters_; + } + return worker_thread_->Invoke(RTC_FROM_HERE, [&] { + RtpParameters result = media_channel_->GetRtpSendParameters(ssrc_); + return result; + }); +} + RtpParameters RtpSenderBase::GetParameters() const { RTC_DCHECK_RUN_ON(signaling_thread_); RtpParameters result = GetParametersInternal(); @@ -207,6 +221,30 @@ RTCError RtpSenderBase::SetParametersInternal(const RtpParameters& parameters) { }); } +RTCError RtpSenderBase::SetParametersInternalWithAllLayers( + const RtpParameters& parameters) { + RTC_DCHECK_RUN_ON(signaling_thread_); + RTC_DCHECK(!stopped_); + + if (UnimplementedRtpParameterHasValue(parameters)) { + LOG_AND_RETURN_ERROR( + RTCErrorType::UNSUPPORTED_PARAMETER, + "Attempted to set an unimplemented parameter of RtpParameters."); + } + if (!media_channel_ || !ssrc_) { + auto result = cricket::CheckRtpParametersInvalidModificationAndValues( + init_parameters_, parameters); + if (result.ok()) { + init_parameters_ = parameters; + } + return result; + } + return worker_thread_->Invoke(RTC_FROM_HERE, [&] { + RtpParameters rtp_parameters = parameters; + return media_channel_->SetRtpSendParameters(ssrc_, rtp_parameters); + }); +} + RTCError RtpSenderBase::SetParameters(const RtpParameters& parameters) { RTC_DCHECK_RUN_ON(signaling_thread_); TRACE_EVENT0("webrtc", "RtpSenderBase::SetParameters"); @@ -377,7 +415,7 @@ RTCError RtpSenderBase::DisableEncodingLayers( } // Check that all the specified layers exist and disable them in the channel. - RtpParameters parameters = GetParametersInternal(); + RtpParameters parameters = GetParametersInternalWithAllLayers(); for (const std::string& rid : rids) { if (absl::c_none_of(parameters.encodings, [&rid](const RtpEncodingParameters& encoding) { @@ -402,7 +440,7 @@ RTCError RtpSenderBase::DisableEncodingLayers( [&encoding](const std::string& rid) { return encoding.rid == rid; }); } - RTCError result = SetParametersInternal(parameters); + RTCError result = SetParametersInternalWithAllLayers(parameters); if (result.ok()) { disabled_rids_.insert(disabled_rids_.end(), rids.begin(), rids.end()); // Invalidate any transaction upon success. diff --git a/pc/rtp_sender.h b/pc/rtp_sender.h index 6e07dde4c5..33d613905b 100644 --- a/pc/rtp_sender.h +++ b/pc/rtp_sender.h @@ -75,6 +75,13 @@ class RtpSenderInternal : public RtpSenderInterface { virtual RtpParameters GetParametersInternal() const = 0; virtual RTCError SetParametersInternal(const RtpParameters& parameters) = 0; + // GetParameters and SetParameters will remove deactivated simulcast layers + // and restore them on SetParameters. This is probably a Bad Idea, but we + // do not know who depends on this behavior + virtual RtpParameters GetParametersInternalWithAllLayers() const = 0; + virtual RTCError SetParametersInternalWithAllLayers( + 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. @@ -118,6 +125,9 @@ class RtpSenderBase : public RtpSenderInternal, public ObserverInterface { // Allow access to get/set parameters without invalidating transaction id. RtpParameters GetParametersInternal() const override; RTCError SetParametersInternal(const RtpParameters& parameters) override; + RtpParameters GetParametersInternalWithAllLayers() const override; + RTCError SetParametersInternalWithAllLayers( + 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 diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 3c8c5062ce..6936029b50 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -479,7 +479,7 @@ RTCError UpdateSimulcastLayerStatusInSender( const std::vector& layers, rtc::scoped_refptr sender) { RTC_DCHECK(sender); - RtpParameters parameters = sender->GetParametersInternal(); + RtpParameters parameters = sender->GetParametersInternalWithAllLayers(); std::vector disabled_layers; // The simulcast envelope cannot be changed, only the status of the streams. @@ -498,7 +498,7 @@ RTCError UpdateSimulcastLayerStatusInSender( encoding.active = !iter->is_paused; } - RTCError result = sender->SetParametersInternal(parameters); + RTCError result = sender->SetParametersInternalWithAllLayers(parameters); if (result.ok()) { result = sender->DisableEncodingLayers(disabled_layers); } @@ -524,7 +524,7 @@ bool SimulcastIsRejected(const ContentInfo* local_content, RTCError DisableSimulcastInSender( rtc::scoped_refptr sender) { RTC_DCHECK(sender); - RtpParameters parameters = sender->GetParametersInternal(); + RtpParameters parameters = sender->GetParametersInternalWithAllLayers(); if (parameters.encodings.size() <= 1) { return RTCError::OK(); } @@ -610,7 +610,7 @@ cricket::MediaDescriptionOptions 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_internal()->GetParametersInternal(); + transceiver->sender_internal()->GetParametersInternalWithAllLayers(); bool has_rids = std::any_of(send_parameters.encodings.begin(), send_parameters.encodings.end(), [](const RtpEncodingParameters& encoding) { diff --git a/pc/test/mock_rtp_sender_internal.h b/pc/test/mock_rtp_sender_internal.h index 5e7670ebf0..5abdc16496 100644 --- a/pc/test/mock_rtp_sender_internal.h +++ b/pc/test/mock_rtp_sender_internal.h @@ -46,11 +46,19 @@ class MockRtpSenderInternal : public RtpSenderInternal { (override)); MOCK_METHOD(RtpParameters, GetParameters, (), (const, override)); MOCK_METHOD(RtpParameters, GetParametersInternal, (), (const, override)); + MOCK_METHOD(RtpParameters, + GetParametersInternalWithAllLayers, + (), + (const, override)); MOCK_METHOD(RTCError, SetParameters, (const RtpParameters&), (override)); MOCK_METHOD(RTCError, SetParametersInternal, (const RtpParameters&), (override)); + MOCK_METHOD(RTCError, + SetParametersInternalWithAllLayers, + (const RtpParameters&), + (override)); MOCK_METHOD(rtc::scoped_refptr, GetDtmfSender, (),