From b8023690d9f0e150cfe698cd68b477903ac66178 Mon Sep 17 00:00:00 2001 From: Florent Castelli Date: Tue, 17 Jan 2023 18:08:26 +0100 Subject: [PATCH] Ensure RTCRtpSenders are always created with one encoding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It is possible to have AddTransceiver calls with an empty array of encodings or AddTrack calls. In both cases, before negotiation, the sender's encodings array would be empty and it was not possible to update any value. Now, a default entry should be created in those cases, allowing to update the configuration before negotiation. Bug: webrtc:10567 Change-Id: I1271e2965e1a97c1e472451e0ab8867fc24f6c2b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/290994 Auto-Submit: Florent Castelli Reviewed-by: Henrik Boström Commit-Queue: Florent Castelli Cr-Commit-Position: refs/heads/main@{#39126} --- pc/peer_connection.cc | 5 +++++ pc/peer_connection_integrationtest.cc | 30 +++++++++++++++++++++++++++ pc/rtp_transmission_manager.cc | 22 +++++++++++--------- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 5633be352e..36ffd1122f 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -1096,6 +1096,11 @@ PeerConnection::AddTransceiver( } } + // If no encoding parameters were provided, a default entry is created. + if (parameters.encodings.empty()) { + parameters.encodings.push_back({}); + } + if (UnimplementedRtpParameterHasValue(parameters)) { LOG_AND_RETURN_ERROR( RTCErrorType::UNSUPPORTED_PARAMETER, diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index f46ef2ebdf..220415baf2 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -2834,6 +2834,14 @@ TEST_P(PeerConnectionIntegrationTest, UnsignaledSsrcGetParametersVideo) { EXPECT_TRUE(parameters.encodings[0].ssrc.has_value()); } +TEST_P(PeerConnectionIntegrationTest, + GetParametersHasEncodingsBeforeNegotiation) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + auto sender = caller()->AddTrack(caller()->CreateLocalVideoTrack()); + auto parameters = sender->GetParameters(); + EXPECT_EQ(parameters.encodings.size(), 1u); +} + // 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. @@ -3410,6 +3418,28 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan, } } +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, + GetParametersHasEncodingsBeforeNegotiation) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + auto result = caller()->pc()->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + auto transceiver = result.MoveValue(); + auto parameters = transceiver->sender()->GetParameters(); + EXPECT_EQ(parameters.encodings.size(), 1u); +} + +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, + GetParametersHasInitEncodingsBeforeNegotiation) { + ASSERT_TRUE(CreatePeerConnectionWrappers()); + RtpTransceiverInit init; + init.send_encodings.push_back({}); + init.send_encodings[0].max_bitrate_bps = 12345; + auto result = caller()->pc()->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, init); + auto transceiver = result.MoveValue(); + auto parameters = transceiver->sender()->GetParameters(); + ASSERT_EQ(parameters.encodings.size(), 1u); + EXPECT_EQ(parameters.encodings[0].max_bitrate_bps, 12345); +} + INSTANTIATE_TEST_SUITE_P( PeerConnectionIntegrationTest, PeerConnectionIntegrationTest, diff --git a/pc/rtp_transmission_manager.cc b/pc/rtp_transmission_manager.cc index 96b748b4b4..96b308842b 100644 --- a/pc/rtp_transmission_manager.cc +++ b/pc/rtp_transmission_manager.cc @@ -150,10 +150,11 @@ RtpTransmissionManager::AddTrackPlanB( (track->kind() == MediaStreamTrackInterface::kAudioKind ? cricket::MEDIA_TYPE_AUDIO : cricket::MEDIA_TYPE_VIDEO); - auto new_sender = - CreateSender(media_type, track->id(), track, adjusted_stream_ids, - init_send_encodings ? *init_send_encodings - : std::vector()); + auto new_sender = CreateSender( + media_type, track->id(), track, adjusted_stream_ids, + init_send_encodings + ? *init_send_encodings + : std::vector(1, RtpEncodingParameters{})); if (track->kind() == MediaStreamTrackInterface::kAudioKind) { new_sender->internal()->SetMediaChannel(voice_media_send_channel()); GetAudioTransceiver()->internal()->AddSender(new_sender); @@ -217,10 +218,11 @@ RtpTransmissionManager::AddTrackUnifiedPlan( if (FindSenderById(sender_id)) { sender_id = rtc::CreateRandomUuid(); } - auto sender = CreateSender(media_type, sender_id, track, stream_ids, - init_send_encodings - ? *init_send_encodings - : std::vector()); + auto sender = CreateSender( + media_type, sender_id, track, stream_ids, + init_send_encodings + ? *init_send_encodings + : std::vector(1, RtpEncodingParameters{})); auto receiver = CreateReceiver(media_type, rtc::CreateRandomUuid()); transceiver = CreateAndAddTransceiver(sender, receiver); transceiver->internal()->set_created_by_addtrack(true); @@ -411,7 +413,7 @@ void RtpTransmissionManager::AddAudioTrack(AudioTrackInterface* track, // Normal case; we've never seen this track before. auto new_sender = CreateSender(cricket::MEDIA_TYPE_AUDIO, track->id(), rtc::scoped_refptr(track), - {stream->id()}, {}); + {stream->id()}, {{}}); new_sender->internal()->SetMediaChannel(voice_media_send_channel()); GetAudioTransceiver()->internal()->AddSender(new_sender); // If the sender has already been configured in SDP, we call SetSsrc, @@ -458,7 +460,7 @@ void RtpTransmissionManager::AddVideoTrack(VideoTrackInterface* track, // Normal case; we've never seen this track before. auto new_sender = CreateSender(cricket::MEDIA_TYPE_VIDEO, track->id(), rtc::scoped_refptr(track), - {stream->id()}, {}); + {stream->id()}, {{}}); new_sender->internal()->SetMediaChannel(video_media_send_channel()); GetVideoTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info =