From 4df20baff171e66e0de5604d20ac8fbc162a7083 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Mon, 9 Jan 2023 13:57:15 +0100 Subject: [PATCH] Implement GetParameters/GetSources support for unsignaled SSRCs. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unsignaled SSRCs are only applicable for the receiver case (not sender). This CL updates the receievr's GetParameters() and GetSources() methods to lookup parameters/sources by the current SSRC (whether or not it was signaled) instead of only looking at the signaled SSRC. To clarify that the `ssrc_` variable inside the [Audio/Video]RtpReceiver is the signaled ssrc (and not set if the current ssrc is unsignaled), we rename this variable to `signaled_ssrc_`. By the looks of it, other APIs like setting volume or packetizers also have a dependency on the assumptions that the SSRC is signaled. We will not address that in this CL, but this CL makes that more clear. Bug: webrtc:14811 Change-Id: I32c93d264ab441ade23a4078639744d25b791742 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/290573 Reviewed-by: Per Kjellander Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#39051} --- pc/audio_rtp_receiver.cc | 51 ++++++++++-------- pc/audio_rtp_receiver.h | 2 +- pc/peer_connection_integrationtest.cc | 78 +++++++++++++++++++++++++++ pc/video_rtp_receiver.cc | 54 ++++++++++--------- pc/video_rtp_receiver.h | 2 +- 5 files changed, 138 insertions(+), 49 deletions(-) diff --git a/pc/audio_rtp_receiver.cc b/pc/audio_rtp_receiver.cc index 9e687b9b55..804d31352d 100644 --- a/pc/audio_rtp_receiver.cc +++ b/pc/audio_rtp_receiver.cc @@ -90,8 +90,8 @@ void AudioRtpReceiver::SetOutputVolume_w(double volume) { if (!media_channel_) return; - ssrc_ ? media_channel_->SetOutputVolume(*ssrc_, volume) - : media_channel_->SetDefaultOutputVolume(volume); + signaled_ssrc_ ? media_channel_->SetOutputVolume(*signaled_ssrc_, volume) + : media_channel_->SetDefaultOutputVolume(volume); } void AudioRtpReceiver::OnSetVolume(double volume) { @@ -137,8 +137,10 @@ RtpParameters AudioRtpReceiver::GetParameters() const { RTC_DCHECK_RUN_ON(worker_thread_); if (!media_channel_) return RtpParameters(); - return ssrc_ ? media_channel_->GetRtpReceiveParameters(*ssrc_) - : media_channel_->GetDefaultRtpReceiveParameters(); + auto current_ssrc = ssrc(); + return current_ssrc.has_value() + ? media_channel_->GetRtpReceiveParameters(current_ssrc.value()) + : media_channel_->GetDefaultRtpReceiveParameters(); } void AudioRtpReceiver::SetFrameDecryptor( @@ -146,8 +148,8 @@ void AudioRtpReceiver::SetFrameDecryptor( RTC_DCHECK_RUN_ON(worker_thread_); frame_decryptor_ = std::move(frame_decryptor); // Special Case: Set the frame decryptor to any value on any existing channel. - if (media_channel_ && ssrc_) { - media_channel_->SetFrameDecryptor(*ssrc_, frame_decryptor_); + if (media_channel_ && signaled_ssrc_) { + media_channel_->SetFrameDecryptor(*signaled_ssrc_, frame_decryptor_); } } @@ -188,15 +190,16 @@ void AudioRtpReceiver::RestartMediaChannel_w( worker_thread_safety_->SetAlive(); if (state != MediaSourceInterface::kInitializing) { - if (ssrc_ == ssrc) + if (signaled_ssrc_ == ssrc) return; - source_->Stop(media_channel_, ssrc_); + source_->Stop(media_channel_, signaled_ssrc_); } - ssrc_ = std::move(ssrc); - source_->Start(media_channel_, ssrc_); - if (ssrc_) { - media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs()); + signaled_ssrc_ = std::move(ssrc); + source_->Start(media_channel_, signaled_ssrc_); + if (signaled_ssrc_) { + media_channel_->SetBaseMinimumPlayoutDelayMs(*signaled_ssrc_, + delay_.GetMs()); } Reconfigure(track_enabled); @@ -214,10 +217,10 @@ void AudioRtpReceiver::SetupUnsignaledMediaChannel() { absl::optional AudioRtpReceiver::ssrc() const { RTC_DCHECK_RUN_ON(worker_thread_); - if (!ssrc_.has_value() && media_channel_) { + if (!signaled_ssrc_.has_value() && media_channel_) { return media_channel_->GetUnsignaledSsrc(); } - return ssrc_; + return signaled_ssrc_; } void AudioRtpReceiver::set_stream_ids(std::vector stream_ids) { @@ -267,18 +270,19 @@ void AudioRtpReceiver::SetStreams( std::vector AudioRtpReceiver::GetSources() const { RTC_DCHECK_RUN_ON(worker_thread_); - if (!media_channel_ || !ssrc_) { + auto current_ssrc = ssrc(); + if (!media_channel_ || !current_ssrc.has_value()) { return {}; } - return media_channel_->GetSources(*ssrc_); + return media_channel_->GetSources(current_ssrc.value()); } void AudioRtpReceiver::SetDepacketizerToDecoderFrameTransformer( rtc::scoped_refptr frame_transformer) { RTC_DCHECK_RUN_ON(worker_thread_); if (media_channel_) { - media_channel_->SetDepacketizerToDecoderFrameTransformer(ssrc_.value_or(0), - frame_transformer); + media_channel_->SetDepacketizerToDecoderFrameTransformer( + signaled_ssrc_.value_or(0), frame_transformer); } frame_transformer_ = std::move(frame_transformer); } @@ -289,14 +293,14 @@ void AudioRtpReceiver::Reconfigure(bool track_enabled) { SetOutputVolume_w(track_enabled ? cached_volume_ : 0); - if (ssrc_ && frame_decryptor_) { + if (signaled_ssrc_ && frame_decryptor_) { // Reattach the frame decryptor if we were reconfigured. - media_channel_->SetFrameDecryptor(*ssrc_, frame_decryptor_); + media_channel_->SetFrameDecryptor(*signaled_ssrc_, frame_decryptor_); } if (frame_transformer_) { media_channel_->SetDepacketizerToDecoderFrameTransformer( - ssrc_.value_or(0), frame_transformer_); + signaled_ssrc_.value_or(0), frame_transformer_); } } @@ -313,8 +317,9 @@ void AudioRtpReceiver::SetJitterBufferMinimumDelay( absl::optional delay_seconds) { RTC_DCHECK_RUN_ON(worker_thread_); delay_.Set(delay_seconds); - if (media_channel_ && ssrc_) - media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs()); + if (media_channel_ && signaled_ssrc_) + media_channel_->SetBaseMinimumPlayoutDelayMs(*signaled_ssrc_, + delay_.GetMs()); } void AudioRtpReceiver::SetMediaChannel( diff --git a/pc/audio_rtp_receiver.h b/pc/audio_rtp_receiver.h index 6ef8f61efd..86c42d532a 100644 --- a/pc/audio_rtp_receiver.h +++ b/pc/audio_rtp_receiver.h @@ -138,7 +138,7 @@ class AudioRtpReceiver : public ObserverInterface, const rtc::scoped_refptr> track_; cricket::VoiceMediaReceiveChannelInterface* media_channel_ RTC_GUARDED_BY(worker_thread_) = nullptr; - absl::optional ssrc_ RTC_GUARDED_BY(worker_thread_); + absl::optional signaled_ssrc_ RTC_GUARDED_BY(worker_thread_); std::vector> streams_ RTC_GUARDED_BY(&signaling_thread_checker_); bool cached_track_enabled_ RTC_GUARDED_BY(&signaling_thread_checker_); diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index bf2d77078f..f46ef2ebdf 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -2756,6 +2756,84 @@ TEST_P(PeerConnectionIntegrationTest, GetSourcesVideo) { EXPECT_EQ(webrtc::RtpSourceType::SSRC, sources[0].source_type()); } +TEST_P(PeerConnectionIntegrationTest, UnsignaledSsrcGetSourcesAudio) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->AddAudioTrack(); + callee()->SetReceivedSdpMunger(RemoveSsrcsAndMsids); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_EQ(callee()->pc()->GetReceivers().size(), 1u); + auto receiver = callee()->pc()->GetReceivers()[0]; + std::vector sources; + EXPECT_TRUE_WAIT(([&receiver, &sources]() { + sources = receiver->GetSources(); + return !sources.empty(); + })(), + kDefaultTimeout); + ASSERT_GT(sources.size(), 0u); + EXPECT_EQ(webrtc::RtpSourceType::SSRC, sources[0].source_type()); +} + +TEST_P(PeerConnectionIntegrationTest, UnsignaledSsrcGetSourcesVideo) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->AddVideoTrack(); + callee()->SetReceivedSdpMunger(RemoveSsrcsAndMsids); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_EQ(callee()->pc()->GetReceivers().size(), 1u); + auto receiver = callee()->pc()->GetReceivers()[0]; + std::vector sources; + EXPECT_TRUE_WAIT(([&receiver, &sources]() { + sources = receiver->GetSources(); + return !sources.empty(); + })(), + kDefaultTimeout); + ASSERT_GT(sources.size(), 0u); + EXPECT_EQ(webrtc::RtpSourceType::SSRC, sources[0].source_type()); +} + +TEST_P(PeerConnectionIntegrationTest, UnsignaledSsrcGetParametersAudio) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->AddAudioTrack(); + callee()->SetReceivedSdpMunger(RemoveSsrcsAndMsids); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_EQ(callee()->pc()->GetReceivers().size(), 1u); + auto receiver = callee()->pc()->GetReceivers()[0]; + RtpParameters parameters; + EXPECT_TRUE_WAIT(([&receiver, ¶meters]() { + parameters = receiver->GetParameters(); + return !parameters.encodings.empty() && + parameters.encodings[0].ssrc.has_value(); + })(), + kDefaultTimeout); + ASSERT_EQ(parameters.encodings.size(), 1u); + EXPECT_TRUE(parameters.encodings[0].ssrc.has_value()); +} + +TEST_P(PeerConnectionIntegrationTest, UnsignaledSsrcGetParametersVideo) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + caller()->AddVideoTrack(); + callee()->SetReceivedSdpMunger(RemoveSsrcsAndMsids); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + ASSERT_EQ(callee()->pc()->GetReceivers().size(), 1u); + auto receiver = callee()->pc()->GetReceivers()[0]; + RtpParameters parameters; + EXPECT_TRUE_WAIT(([&receiver, ¶meters]() { + parameters = receiver->GetParameters(); + return !parameters.encodings.empty() && + parameters.encodings[0].ssrc.has_value(); + })(), + kDefaultTimeout); + ASSERT_EQ(parameters.encodings.size(), 1u); + EXPECT_TRUE(parameters.encodings[0].ssrc.has_value()); +} + // Test that if a track is removed and added again with a different stream ID, // the new stream ID is successfully communicated in SDP and media continues to // flow end-to-end. diff --git a/pc/video_rtp_receiver.cc b/pc/video_rtp_receiver.cc index e7e7726ab0..8a2e65c162 100644 --- a/pc/video_rtp_receiver.cc +++ b/pc/video_rtp_receiver.cc @@ -76,8 +76,10 @@ RtpParameters VideoRtpReceiver::GetParameters() const { RTC_DCHECK_RUN_ON(worker_thread_); if (!media_channel_) return RtpParameters(); - return ssrc_ ? media_channel_->GetRtpReceiveParameters(*ssrc_) - : media_channel_->GetDefaultRtpReceiveParameters(); + auto current_ssrc = ssrc(); + return current_ssrc.has_value() + ? media_channel_->GetRtpReceiveParameters(current_ssrc.value()) + : media_channel_->GetDefaultRtpReceiveParameters(); } void VideoRtpReceiver::SetFrameDecryptor( @@ -85,8 +87,8 @@ void VideoRtpReceiver::SetFrameDecryptor( RTC_DCHECK_RUN_ON(worker_thread_); frame_decryptor_ = std::move(frame_decryptor); // Special Case: Set the frame decryptor to any value on any existing channel. - if (media_channel_ && ssrc_) { - media_channel_->SetFrameDecryptor(*ssrc_, frame_decryptor_); + if (media_channel_ && signaled_ssrc_) { + media_channel_->SetFrameDecryptor(*signaled_ssrc_, frame_decryptor_); } } @@ -102,7 +104,7 @@ void VideoRtpReceiver::SetDepacketizerToDecoderFrameTransformer( frame_transformer_ = std::move(frame_transformer); if (media_channel_) { media_channel_->SetDepacketizerToDecoderFrameTransformer( - ssrc_.value_or(0), frame_transformer_); + signaled_ssrc_.value_or(0), frame_transformer_); } } @@ -134,7 +136,7 @@ void VideoRtpReceiver::RestartMediaChannel_w( const bool encoded_sink_enabled = saved_encoded_sink_enabled_; if (state != MediaSourceInterface::kInitializing) { - if (ssrc == ssrc_) + if (ssrc == signaled_ssrc_) return; // Disconnect from a previous ssrc. @@ -145,7 +147,7 @@ void VideoRtpReceiver::RestartMediaChannel_w( } // Set up the new ssrc. - ssrc_ = std::move(ssrc); + signaled_ssrc_ = std::move(ssrc); SetSink(source_->sink()); if (encoded_sink_enabled) { SetEncodedSinkEnabled(true); @@ -153,22 +155,23 @@ void VideoRtpReceiver::RestartMediaChannel_w( if (frame_transformer_ && media_channel_) { media_channel_->SetDepacketizerToDecoderFrameTransformer( - ssrc_.value_or(0), frame_transformer_); + signaled_ssrc_.value_or(0), frame_transformer_); } - if (media_channel_ && ssrc_) { + if (media_channel_ && signaled_ssrc_) { if (frame_decryptor_) { - media_channel_->SetFrameDecryptor(*ssrc_, frame_decryptor_); + media_channel_->SetFrameDecryptor(*signaled_ssrc_, frame_decryptor_); } - media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs()); + media_channel_->SetBaseMinimumPlayoutDelayMs(*signaled_ssrc_, + delay_.GetMs()); } } void VideoRtpReceiver::SetSink(rtc::VideoSinkInterface* sink) { RTC_DCHECK_RUN_ON(worker_thread_); - if (ssrc_) { - media_channel_->SetSink(*ssrc_, sink); + if (signaled_ssrc_) { + media_channel_->SetSink(*signaled_ssrc_, sink); } else { media_channel_->SetDefaultSink(sink); } @@ -186,10 +189,10 @@ void VideoRtpReceiver::SetupUnsignaledMediaChannel() { absl::optional VideoRtpReceiver::ssrc() const { RTC_DCHECK_RUN_ON(worker_thread_); - if (!ssrc_.has_value() && media_channel_) { + if (!signaled_ssrc_.has_value() && media_channel_) { return media_channel_->GetUnsignaledSsrc(); } - return ssrc_; + return signaled_ssrc_; } void VideoRtpReceiver::set_stream_ids(std::vector stream_ids) { @@ -250,8 +253,9 @@ void VideoRtpReceiver::SetJitterBufferMinimumDelay( absl::optional delay_seconds) { RTC_DCHECK_RUN_ON(worker_thread_); delay_.Set(delay_seconds); - if (media_channel_ && ssrc_) - media_channel_->SetBaseMinimumPlayoutDelayMs(*ssrc_, delay_.GetMs()); + if (media_channel_ && signaled_ssrc_) + media_channel_->SetBaseMinimumPlayoutDelayMs(*signaled_ssrc_, + delay_.GetMs()); } void VideoRtpReceiver::SetMediaChannel( @@ -288,7 +292,7 @@ void VideoRtpReceiver::SetMediaChannel_w( if (media_channel_) { if (saved_generate_keyframe_) { // TODO(bugs.webrtc.org/8694): Stop using 0 to mean unsignalled SSRC - media_channel_->RequestRecvKeyFrame(ssrc_.value_or(0)); + media_channel_->RequestRecvKeyFrame(signaled_ssrc_.value_or(0)); saved_generate_keyframe_ = false; } if (encoded_sink_enabled) { @@ -296,7 +300,7 @@ void VideoRtpReceiver::SetMediaChannel_w( } if (frame_transformer_) { media_channel_->SetDepacketizerToDecoderFrameTransformer( - ssrc_.value_or(0), frame_transformer_); + signaled_ssrc_.value_or(0), frame_transformer_); } } @@ -314,9 +318,11 @@ void VideoRtpReceiver::NotifyFirstPacketReceived() { std::vector VideoRtpReceiver::GetSources() const { RTC_DCHECK_RUN_ON(worker_thread_); - if (!ssrc_ || !media_channel_) - return std::vector(); - return media_channel_->GetSources(*ssrc_); + auto current_ssrc = ssrc(); + if (!media_channel_ || !current_ssrc.has_value()) { + return {}; + } + return media_channel_->GetSources(current_ssrc.value()); } void VideoRtpReceiver::SetupMediaChannel( @@ -341,7 +347,7 @@ void VideoRtpReceiver::OnGenerateKeyFrame() { return; } // TODO(bugs.webrtc.org/8694): Stop using 0 to mean unsignalled SSRC - media_channel_->RequestRecvKeyFrame(ssrc_.value_or(0)); + media_channel_->RequestRecvKeyFrame(signaled_ssrc_.value_or(0)); // We need to remember to request generation of a new key frame if the media // channel changes, because there's no feedback whether the keyframe // generation has completed on the channel. @@ -362,7 +368,7 @@ void VideoRtpReceiver::SetEncodedSinkEnabled(bool enable) { return; // TODO(bugs.webrtc.org/8694): Stop using 0 to mean unsignalled SSRC - const auto ssrc = ssrc_.value_or(0); + const auto ssrc = signaled_ssrc_.value_or(0); if (enable) { media_channel_->SetRecordableEncodedFrameCallback( diff --git a/pc/video_rtp_receiver.h b/pc/video_rtp_receiver.h index caf035e36f..ef88016052 100644 --- a/pc/video_rtp_receiver.h +++ b/pc/video_rtp_receiver.h @@ -151,7 +151,7 @@ class VideoRtpReceiver : public RtpReceiverInternal { const std::string id_; cricket::VideoMediaReceiveChannelInterface* media_channel_ RTC_GUARDED_BY(worker_thread_) = nullptr; - absl::optional ssrc_ RTC_GUARDED_BY(worker_thread_); + absl::optional signaled_ssrc_ RTC_GUARDED_BY(worker_thread_); // `source_` is held here to be able to change the state of the source when // the VideoRtpReceiver is stopped. const rtc::scoped_refptr source_;