From db2f52ba88cf9f98211df2dabb3f8aca9251c4a2 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Fri, 23 Feb 2024 09:14:34 +0100 Subject: [PATCH] Reland "Make setCodecPreferences only look at receive codecs" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of commit 1cce1d7ddcbde3a3648007b5a131bd0c2638724b after updating the WPT that broke on Mac. Original change's description: > Make setCodecPreferences only look at receive codecs > > which is what is noted in JSEP: > https://www.rfc-editor.org/rfc/rfc8829.html#name-setcodecpreferences > > Some W3C spec modifications are required since the W3C specification > currently takes into account send codecs as well. > > Spec issue: > https://github.com/w3c/webrtc-pc/issues/2888 > Spec PR: > https://github.com/w3c/webrtc-pc/pull/2926 > > setCodecPreferences continues to modify the codecs in an offer. > > Also rename RtpSender::SetCodecPreferences to RtpSender::SetSendCodecs for consistent semantics. > > BUG=webrtc:15396 > > Change-Id: I1e8fbe77cb2670575578a777ed1336567a1e4031 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328780 > Reviewed-by: Henrik Boström > Commit-Queue: Philipp Hancke > Reviewed-by: Harald Alvestrand > Cr-Commit-Position: refs/heads/main@{#41719} Bug: webrtc:15396 Change-Id: I0c7b17f00de02286f176b500460e17980b83b35b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/339541 Commit-Queue: Philipp Hancke Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#41807} --- media/base/media_engine.cc | 18 +- media/base/media_engine.h | 6 +- ...er_connection_encodings_integrationtest.cc | 166 +++++++++--------- pc/peer_connection_media_unittest.cc | 23 --- pc/peer_connection_svc_integrationtest.cc | 27 +-- pc/rtc_stats_collector_unittest.cc | 20 +-- pc/rtp_sender.cc | 14 +- pc/rtp_sender.h | 10 +- pc/rtp_transceiver.cc | 69 ++------ pc/test/mock_rtp_sender_internal.h | 2 +- 10 files changed, 127 insertions(+), 228 deletions(-) diff --git a/media/base/media_engine.cc b/media/base/media_engine.cc index 7304ab03d7..c551a58cf9 100644 --- a/media/base/media_engine.cc +++ b/media/base/media_engine.cc @@ -67,11 +67,11 @@ std::vector GetDefaultEnabledRtpHeaderExtensions( webrtc::RTCError CheckScalabilityModeValues( const webrtc::RtpParameters& rtp_parameters, - rtc::ArrayView codec_preferences, + rtc::ArrayView send_codecs, absl::optional send_codec) { using webrtc::RTCErrorType; - if (codec_preferences.empty()) { + if (send_codecs.empty()) { // This is an audio sender or an extra check in the stack where the codec // list is not available and we can't check the scalability_mode values. return webrtc::RTCError::OK(); @@ -80,7 +80,7 @@ webrtc::RTCError CheckScalabilityModeValues( for (size_t i = 0; i < rtp_parameters.encodings.size(); ++i) { if (rtp_parameters.encodings[i].codec) { bool codecFound = false; - for (const cricket::VideoCodec& codec : codec_preferences) { + for (const cricket::Codec& codec : send_codecs) { if (codec.MatchesRtpCodec(*rtp_parameters.encodings[i].codec)) { codecFound = true; send_codec = codec; @@ -97,7 +97,7 @@ webrtc::RTCError CheckScalabilityModeValues( if (rtp_parameters.encodings[i].scalability_mode) { if (!send_codec) { bool scalabilityModeFound = false; - for (const cricket::VideoCodec& codec : codec_preferences) { + for (const cricket::Codec& codec : send_codecs) { for (const auto& scalability_mode : codec.scalability_modes) { if (ScalabilityModeToString(scalability_mode) == *rtp_parameters.encodings[i].scalability_mode) { @@ -139,7 +139,7 @@ webrtc::RTCError CheckScalabilityModeValues( webrtc::RTCError CheckRtpParametersValues( const webrtc::RtpParameters& rtp_parameters, - rtc::ArrayView codec_preferences, + rtc::ArrayView send_codecs, absl::optional send_codec) { using webrtc::RTCErrorType; @@ -196,8 +196,7 @@ webrtc::RTCError CheckRtpParametersValues( } } - return CheckScalabilityModeValues(rtp_parameters, codec_preferences, - send_codec); + return CheckScalabilityModeValues(rtp_parameters, send_codecs, send_codec); } webrtc::RTCError CheckRtpParametersInvalidModificationAndValues( @@ -210,7 +209,7 @@ webrtc::RTCError CheckRtpParametersInvalidModificationAndValues( webrtc::RTCError CheckRtpParametersInvalidModificationAndValues( const webrtc::RtpParameters& old_rtp_parameters, const webrtc::RtpParameters& rtp_parameters, - rtc::ArrayView codec_preferences, + rtc::ArrayView send_codecs, absl::optional send_codec) { using webrtc::RTCErrorType; if (rtp_parameters.encodings.size() != old_rtp_parameters.encodings.size()) { @@ -246,8 +245,7 @@ webrtc::RTCError CheckRtpParametersInvalidModificationAndValues( "Attempted to set RtpParameters with modified SSRC"); } - return CheckRtpParametersValues(rtp_parameters, codec_preferences, - send_codec); + return CheckRtpParametersValues(rtp_parameters, send_codecs, send_codec); } CompositeMediaEngine::CompositeMediaEngine( diff --git a/media/base/media_engine.h b/media/base/media_engine.h index 428123516f..b054893163 100644 --- a/media/base/media_engine.h +++ b/media/base/media_engine.h @@ -42,14 +42,14 @@ namespace cricket { // least one video codec of the list. If the list is empty, no check is done. webrtc::RTCError CheckScalabilityModeValues( const webrtc::RtpParameters& new_parameters, - rtc::ArrayView codec_preferences, + rtc::ArrayView send_codecs, absl::optional send_codec); // Checks the parameters have valid and supported values, and checks parameters // with CheckScalabilityModeValues(). webrtc::RTCError CheckRtpParametersValues( const webrtc::RtpParameters& new_parameters, - rtc::ArrayView codec_preferences, + rtc::ArrayView send_codecs, absl::optional send_codec); // Checks that the immutable values have not changed in new_parameters and @@ -57,7 +57,7 @@ webrtc::RTCError CheckRtpParametersValues( webrtc::RTCError CheckRtpParametersInvalidModificationAndValues( const webrtc::RtpParameters& old_parameters, const webrtc::RtpParameters& new_parameters, - rtc::ArrayView codec_preferences, + rtc::ArrayView send_codecs, absl::optional send_codec); // Checks that the immutable values have not changed in new_parameters and diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc index 06299d7029..d6c4499210 100644 --- a/pc/peer_connection_encodings_integrationtest.cc +++ b/pc/peer_connection_encodings_integrationtest.cc @@ -134,12 +134,12 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test { return transceiver_or_error.value(); } - bool HasSenderVideoCodecCapability( + bool HasReceiverVideoCodecCapability( rtc::scoped_refptr pc_wrapper, absl::string_view codec_name) { std::vector codecs = pc_wrapper->pc_factory() - ->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_VIDEO) + ->GetRtpReceiverCapabilities(cricket::MEDIA_TYPE_VIDEO) .codecs; return std::find_if(codecs.begin(), codecs.end(), [&codec_name](const RtpCodecCapability& codec) { @@ -152,7 +152,7 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test { absl::string_view codec_name) { std::vector codecs = pc_wrapper->pc_factory() - ->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_VIDEO) + ->GetRtpReceiverCapabilities(cricket::MEDIA_TYPE_VIDEO) .codecs; codecs.erase(std::remove_if(codecs.begin(), codecs.end(), [&codec_name](const RtpCodecCapability& codec) { @@ -431,7 +431,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP8"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP8"); transceiver->SetCodecPreferences(codecs); NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); @@ -464,10 +464,14 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, rtc::scoped_refptr transceiver = AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); - // Restricting codecs restricts what SetParameters() will accept or reject. + // Restricting the local receive codecs will restrict what we offer and + // hence the answer if it is a subset of our offer. std::vector codecs = GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP8"); transceiver->SetCodecPreferences(codecs); + + NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); + // Attempt SVC (L3T3_KEY). This is not possible because only VP8 is up for // negotiation and VP8 does not support it. rtc::scoped_refptr sender = transceiver->sender(); @@ -481,7 +485,6 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, ASSERT_EQ(parameters.encodings.size(), 1u); EXPECT_THAT(parameters.encodings[0].scalability_mode, Eq(absl::nullopt)); - NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); local_pc_wrapper->WaitForConnection(); remote_pc_wrapper->WaitForConnection(); @@ -502,6 +505,60 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, EXPECT_THAT(parameters.encodings[0].scalability_mode, Eq(absl::nullopt)); } +TEST_F(PeerConnectionEncodingsIntegrationTest, + SetParametersWithScalabilityModeNotSupportedBySubsequentNegotiation) { + rtc::scoped_refptr local_pc_wrapper = CreatePc(); + rtc::scoped_refptr remote_pc_wrapper = CreatePc(); + ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); + + std::vector layers = + CreateLayers({"f"}, /*active=*/true); + rtc::scoped_refptr transceiver = + AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, + layers); + // Restricting the local receive codecs will restrict what we offer and + // hence the answer if it is a subset of our offer. + std::vector codecs = + GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP8"); + transceiver->SetCodecPreferences(codecs); + + // Attempt SVC (L3T3_KEY). This is still possible because VP9 might be + // available from the remote end. + rtc::scoped_refptr sender = transceiver->sender(); + RtpParameters parameters = sender->GetParameters(); + ASSERT_EQ(parameters.encodings.size(), 1u); + parameters.encodings[0].scalability_mode = "L3T3_KEY"; + parameters.encodings[0].scale_resolution_down_by = 1; + EXPECT_TRUE(sender->SetParameters(parameters).ok()); + + NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); + + // `scalability_mode` is set to the VP8 default since that is what was + // negotiated. + parameters = sender->GetParameters(); + ASSERT_EQ(parameters.encodings.size(), 1u); + EXPECT_THAT(parameters.encodings[0].scalability_mode, Eq("L1T2")); + + local_pc_wrapper->WaitForConnection(); + remote_pc_wrapper->WaitForConnection(); + + // Wait until media is flowing. + ASSERT_TRUE_WAIT(HasOutboundRtpBytesSent(local_pc_wrapper, 1u), + kDefaultTimeout.ms()); + // When `scalability_mode` is not set, VP8 defaults to L1T1. + rtc::scoped_refptr report = GetStats(local_pc_wrapper); + std::vector outbound_rtps = + report->GetStatsOfType(); + ASSERT_THAT(outbound_rtps, SizeIs(1u)); + EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[0]), + StrCaseEq("video/VP8")); + EXPECT_THAT(*outbound_rtps[0]->scalability_mode, StrEq("L1T2")); + // GetParameters() confirms `scalability_mode` is still not set. + parameters = sender->GetParameters(); + ASSERT_EQ(parameters.encodings.size(), 1u); + EXPECT_THAT(parameters.encodings[0].scalability_mode, Eq("L1T2")); +} + TEST_F(PeerConnectionEncodingsIntegrationTest, VP8_FallbackFromSvcResultsInL1T2) { rtc::scoped_refptr local_pc_wrapper = CreatePc(); @@ -558,6 +615,13 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[0]), StrCaseEq("video/VP8")); EXPECT_THAT(*outbound_rtps[0]->scalability_mode, StrEq("L1T2")); + + // Now that we know VP8 is used, try setting L3T3 which should fail. + parameters = sender->GetParameters(); + ASSERT_EQ(parameters.encodings.size(), 1u); + parameters.encodings[0].scalability_mode = "L3T3_KEY"; + parameters.encodings[0].scale_resolution_down_by = 1; + EXPECT_FALSE(sender->SetParameters(parameters).ok()); } // The legacy SVC path is triggered when VP9 us used, but `scalability_mode` has @@ -577,7 +641,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP9"); transceiver->SetCodecPreferences(codecs); NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); @@ -622,7 +686,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP9"); transceiver->SetCodecPreferences(codecs); // Configure SVC, a.k.a. "L3T3_KEY". rtc::scoped_refptr sender = transceiver->sender(); @@ -675,7 +739,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP9"); transceiver->SetCodecPreferences(codecs); // Configure SVC, a.k.a. "L3T3_KEY". rtc::scoped_refptr sender = transceiver->sender(); @@ -725,7 +789,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP9"); transceiver->SetCodecPreferences(codecs); // The original negotiation triggers legacy SVC because we didn't specify @@ -780,7 +844,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP9"); transceiver->SetCodecPreferences(codecs); // Legacy SVC mode and all layers inactive. @@ -817,7 +881,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP9"); transceiver->SetCodecPreferences(codecs); // Standard mode and all layers inactive. @@ -857,7 +921,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, VP9_TargetBitrate_LegacyL1T3) { AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP9"); transceiver->SetCodecPreferences(codecs); // In legacy SVC, disabling the bottom two layers encodings is interpreted as @@ -904,7 +968,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, VP9_TargetBitrate_StandardL1T3) { AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP9"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP9"); transceiver->SetCodecPreferences(codecs); // With standard APIs, L1T3 is explicitly specified and the encodings refers @@ -955,7 +1019,7 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "VP8"); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, "VP8"); transceiver->SetCodecPreferences(codecs); NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); @@ -1364,72 +1428,6 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, EXPECT_EQ(error.type(), RTCErrorType::INVALID_MODIFICATION); } -TEST_F(PeerConnectionEncodingsIntegrationTest, - SetParametersRejectsNonPreferredCodecParameterAudio) { - rtc::scoped_refptr local_pc_wrapper = CreatePc(); - - absl::optional opus = - local_pc_wrapper->FindFirstSendCodecWithName(cricket::MEDIA_TYPE_AUDIO, - "opus"); - ASSERT_TRUE(opus); - - std::vector not_opus_codecs = - local_pc_wrapper->pc_factory() - ->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_AUDIO) - .codecs; - not_opus_codecs.erase( - std::remove_if(not_opus_codecs.begin(), not_opus_codecs.end(), - [&](const auto& codec) { - return absl::EqualsIgnoreCase(codec.name, opus->name); - }), - not_opus_codecs.end()); - - auto transceiver_or_error = - local_pc_wrapper->pc()->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - ASSERT_TRUE(transceiver_or_error.ok()); - rtc::scoped_refptr audio_transceiver = - transceiver_or_error.MoveValue(); - ASSERT_TRUE(audio_transceiver->SetCodecPreferences(not_opus_codecs).ok()); - - RtpParameters parameters = audio_transceiver->sender()->GetParameters(); - parameters.encodings[0].codec = opus; - RTCError error = audio_transceiver->sender()->SetParameters(parameters); - EXPECT_EQ(error.type(), RTCErrorType::INVALID_MODIFICATION); -} - -TEST_F(PeerConnectionEncodingsIntegrationTest, - SetParametersRejectsNonPreferredCodecParameterVideo) { - rtc::scoped_refptr local_pc_wrapper = CreatePc(); - - absl::optional vp8 = - local_pc_wrapper->FindFirstSendCodecWithName(cricket::MEDIA_TYPE_VIDEO, - "vp8"); - ASSERT_TRUE(vp8); - - std::vector not_vp8_codecs = - local_pc_wrapper->pc_factory() - ->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_VIDEO) - .codecs; - not_vp8_codecs.erase( - std::remove_if(not_vp8_codecs.begin(), not_vp8_codecs.end(), - [&](const auto& codec) { - return absl::EqualsIgnoreCase(codec.name, vp8->name); - }), - not_vp8_codecs.end()); - - auto transceiver_or_error = - local_pc_wrapper->pc()->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); - ASSERT_TRUE(transceiver_or_error.ok()); - rtc::scoped_refptr video_transceiver = - transceiver_or_error.MoveValue(); - ASSERT_TRUE(video_transceiver->SetCodecPreferences(not_vp8_codecs).ok()); - - RtpParameters parameters = video_transceiver->sender()->GetParameters(); - parameters.encodings[0].codec = vp8; - RTCError error = video_transceiver->sender()->SetParameters(parameters); - EXPECT_EQ(error.type(), RTCErrorType::INVALID_MODIFICATION); -} - TEST_F(PeerConnectionEncodingsIntegrationTest, SetParametersRejectsNonNegotiatedCodecParameterAudio) { rtc::scoped_refptr local_pc_wrapper = CreatePc(); @@ -1875,7 +1873,7 @@ class PeerConnectionEncodingsIntegrationParameterizedTest bool SkipTestDueToAv1Missing( rtc::scoped_refptr local_pc_wrapper) { if (codec_name_ == "AV1" && - !HasSenderVideoCodecCapability(local_pc_wrapper, "AV1")) { + !HasReceiverVideoCodecCapability(local_pc_wrapper, "AV1")) { RTC_LOG(LS_WARNING) << "\n***\nAV1 is not available, skipping test.\n***"; return true; } @@ -1901,7 +1899,7 @@ TEST_P(PeerConnectionEncodingsIntegrationParameterizedTest, AllLayersInactive) { AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, codec_name_); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, codec_name_); transceiver->SetCodecPreferences(codecs); // Standard mode and all layers inactive. @@ -1944,7 +1942,7 @@ TEST_P(PeerConnectionEncodingsIntegrationParameterizedTest, Simulcast) { AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, layers); std::vector codecs = - GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, codec_name_); + GetCapabilitiesAndRestrictToCodec(remote_pc_wrapper, codec_name_); transceiver->SetCodecPreferences(codecs); rtc::scoped_refptr sender = transceiver->sender(); diff --git a/pc/peer_connection_media_unittest.cc b/pc/peer_connection_media_unittest.cc index b892eacb78..04fb9c9e26 100644 --- a/pc/peer_connection_media_unittest.cc +++ b/pc/peer_connection_media_unittest.cc @@ -1547,29 +1547,6 @@ TEST_F(PeerConnectionMediaTestUnifiedPlan, EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, result.type()); } -TEST_F(PeerConnectionMediaTestUnifiedPlan, - SetCodecPreferencesAudioMissingSendCodec) { - auto fake_engine = std::make_unique(); - auto recv_codecs = fake_engine->voice().recv_codecs(); - recv_codecs.push_back(cricket::CreateAudioCodec(recv_codecs.back().id + 1, - "recv_only_codec", 0, 1)); - fake_engine->SetAudioRecvCodecs(recv_codecs); - auto caller = CreatePeerConnectionWithAudio(std::move(fake_engine)); - - auto transceiver = caller->pc()->GetTransceivers().front(); - auto capabilities = caller->pc_factory()->GetRtpReceiverCapabilities( - cricket::MediaType::MEDIA_TYPE_AUDIO); - - std::vector codecs; - absl::c_copy_if(capabilities.codecs, std::back_inserter(codecs), - [](const RtpCodecCapability& codec) { - return codec.name.find("_only_") != std::string::npos; - }); - - auto result = transceiver->SetCodecPreferences(codecs); - EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, result.type()); -} - TEST_F(PeerConnectionMediaTestUnifiedPlan, SetCodecPreferencesAudioRejectsVideoCodec) { auto caller = CreatePeerConnectionWithAudio(); diff --git a/pc/peer_connection_svc_integrationtest.cc b/pc/peer_connection_svc_integrationtest.cc index 32ca451866..7deafbf428 100644 --- a/pc/peer_connection_svc_integrationtest.cc +++ b/pc/peer_connection_svc_integrationtest.cc @@ -40,7 +40,7 @@ class PeerConnectionSVCIntegrationTest rtc::scoped_refptr transceiver, absl::string_view codec_name) { RtpCapabilities capabilities = - caller()->pc_factory()->GetRtpSenderCapabilities( + caller()->pc_factory()->GetRtpReceiverCapabilities( cricket::MEDIA_TYPE_VIDEO); std::vector codecs; for (const RtpCodecCapability& codec_capability : capabilities.codecs) { @@ -95,7 +95,7 @@ TEST_F(PeerConnectionSVCIntegrationTest, SetParametersAcceptsL1T3WithVP8) { ConnectFakeSignaling(); RtpCapabilities capabilities = - caller()->pc_factory()->GetRtpSenderCapabilities( + caller()->pc_factory()->GetRtpReceiverCapabilities( cricket::MEDIA_TYPE_VIDEO); std::vector vp8_codec; for (const RtpCodecCapability& codec_capability : capabilities.codecs) { @@ -119,27 +119,6 @@ TEST_F(PeerConnectionSVCIntegrationTest, SetParametersAcceptsL1T3WithVP8) { EXPECT_TRUE(result.ok()); } -TEST_F(PeerConnectionSVCIntegrationTest, SetParametersRejectsL3T3WithVP8) { - ASSERT_TRUE(CreatePeerConnectionWrappers()); - ConnectFakeSignaling(); - - RtpTransceiverInit init; - RtpEncodingParameters encoding_parameters; - init.send_encodings.push_back(encoding_parameters); - auto transceiver_or_error = - caller()->pc()->AddTransceiver(caller()->CreateLocalVideoTrack(), init); - ASSERT_TRUE(transceiver_or_error.ok()); - auto transceiver = transceiver_or_error.MoveValue(); - EXPECT_TRUE(SetCodecPreferences(transceiver, cricket::kVp8CodecName).ok()); - - RtpParameters parameters = transceiver->sender()->GetParameters(); - ASSERT_EQ(parameters.encodings.size(), 1u); - parameters.encodings[0].scalability_mode = "L3T3"; - auto result = transceiver->sender()->SetParameters(parameters); - EXPECT_FALSE(result.ok()); - EXPECT_EQ(result.type(), RTCErrorType::INVALID_MODIFICATION); -} - TEST_F(PeerConnectionSVCIntegrationTest, SetParametersAcceptsL1T3WithVP8AfterNegotiation) { ASSERT_TRUE(CreatePeerConnectionWrappers()); @@ -251,7 +230,7 @@ TEST_F(PeerConnectionSVCIntegrationTest, FallbackToL1Tx) { auto caller_transceiver = transceiver_or_error.MoveValue(); RtpCapabilities capabilities = - caller()->pc_factory()->GetRtpSenderCapabilities( + caller()->pc_factory()->GetRtpReceiverCapabilities( cricket::MEDIA_TYPE_VIDEO); std::vector send_codecs = capabilities.codecs; // Only keep VP9 in the caller diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index 351ab6de64..71542ca4bf 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -495,7 +495,7 @@ class RTCStatsCollectorWrapper { CreateMockSender(media_type, track, ssrc, attachment_id, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); - EXPECT_CALL(*sender, SetCodecPreferences(_)); + EXPECT_CALL(*sender, SetSendCodecs(_)); pc_->AddSender(sender); return sender; } @@ -572,7 +572,7 @@ class RTCStatsCollectorWrapper { voice_sender_info.local_stats[0].ssrc + 10, local_stream_ids); EXPECT_CALL(*rtp_sender, SetMediaChannel(_)).WillRepeatedly(Return()); EXPECT_CALL(*rtp_sender, Stop()); - EXPECT_CALL(*rtp_sender, SetCodecPreferences(_)); + EXPECT_CALL(*rtp_sender, SetSendCodecs(_)); pc_->AddSender(rtp_sender); } @@ -611,7 +611,7 @@ class RTCStatsCollectorWrapper { video_sender_info.local_stats[0].ssrc + 10, local_stream_ids); EXPECT_CALL(*rtp_sender, SetMediaChannel(_)).WillRepeatedly(Return()); EXPECT_CALL(*rtp_sender, Stop()); - EXPECT_CALL(*rtp_sender, SetCodecPreferences(_)); + EXPECT_CALL(*rtp_sender, SetSendCodecs(_)); pc_->AddSender(rtp_sender); } @@ -3092,7 +3092,7 @@ TEST_F(RTCStatsCollectorTest, RTCVideoSourceStatsCollectedForSenderWithTrack) { cricket::MEDIA_TYPE_VIDEO, video_track, kSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); - EXPECT_CALL(*sender, SetCodecPreferences(_)); + EXPECT_CALL(*sender, SetSendCodecs(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3137,7 +3137,7 @@ TEST_F(RTCStatsCollectorTest, cricket::MEDIA_TYPE_VIDEO, video_track, kNoSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); - EXPECT_CALL(*sender, SetCodecPreferences(_)); + EXPECT_CALL(*sender, SetSendCodecs(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3168,7 +3168,7 @@ TEST_F(RTCStatsCollectorTest, cricket::MEDIA_TYPE_VIDEO, video_track, kSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); - EXPECT_CALL(*sender, SetCodecPreferences(_)); + EXPECT_CALL(*sender, SetSendCodecs(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3192,7 +3192,7 @@ TEST_F(RTCStatsCollectorTest, cricket::MEDIA_TYPE_AUDIO, /*track=*/nullptr, kSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); - EXPECT_CALL(*sender, SetCodecPreferences(_)); + EXPECT_CALL(*sender, SetSendCodecs(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3545,7 +3545,7 @@ TEST_F(RTCStatsCollectorTest, cricket::MEDIA_TYPE_VIDEO, /*track=*/nullptr, kSsrc, kAttachmentId, {}); EXPECT_CALL(*sender, Stop()); EXPECT_CALL(*sender, SetMediaChannel(_)); - EXPECT_CALL(*sender, SetCodecPreferences(_)); + EXPECT_CALL(*sender, SetSendCodecs(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3667,7 +3667,7 @@ TEST_F(RTCStatsCollectorTest, RtpIsMissingWhileSsrcIsZero) { rtc::scoped_refptr sender = CreateMockSender(cricket::MEDIA_TYPE_AUDIO, track, 0, 49, {}); EXPECT_CALL(*sender, Stop()); - EXPECT_CALL(*sender, SetCodecPreferences(_)); + EXPECT_CALL(*sender, SetSendCodecs(_)); pc_->AddSender(sender); rtc::scoped_refptr report = stats_->GetStatsReport(); @@ -3685,7 +3685,7 @@ TEST_F(RTCStatsCollectorTest, DoNotCrashIfSsrcIsKnownButInfosAreStillMissing) { rtc::scoped_refptr sender = CreateMockSender(cricket::MEDIA_TYPE_AUDIO, track, 4711, 49, {}); EXPECT_CALL(*sender, Stop()); - EXPECT_CALL(*sender, SetCodecPreferences(_)); + EXPECT_CALL(*sender, SetSendCodecs(_)); pc_->AddSender(sender); // We do not generate any matching voice_sender_info stats. diff --git a/pc/rtp_sender.cc b/pc/rtp_sender.cc index b0c32eff85..be0975778f 100644 --- a/pc/rtp_sender.cc +++ b/pc/rtp_sender.cc @@ -248,7 +248,7 @@ void RtpSenderBase::SetParametersInternal(const RtpParameters& parameters, } if (!media_channel_ || !ssrc_) { auto result = cricket::CheckRtpParametersInvalidModificationAndValues( - init_parameters_, parameters, codec_preferences_, absl::nullopt); + init_parameters_, parameters, send_codecs_, absl::nullopt); if (result.ok()) { init_parameters_ = parameters; } @@ -299,7 +299,7 @@ RTCError RtpSenderBase::SetParametersInternalWithAllLayers( } if (!media_channel_ || !ssrc_) { auto result = cricket::CheckRtpParametersInvalidModificationAndValues( - init_parameters_, parameters, codec_preferences_, absl::nullopt); + init_parameters_, parameters, send_codecs_, absl::nullopt); if (result.ok()) { init_parameters_ = parameters; } @@ -345,16 +345,14 @@ RTCError RtpSenderBase::CheckCodecParameters(const RtpParameters& parameters) { // the SVC capabilities. absl::optional send_codec_with_svc_info; if (send_codec && send_codec->type == cricket::Codec::Type::kVideo) { - auto codec_match = - absl::c_find_if(codec_preferences_, [&](auto& codec_preference) { - return send_codec->Matches(codec_preference); - }); - if (codec_match != codec_preferences_.end()) { + auto codec_match = absl::c_find_if( + send_codecs_, [&](auto& codec) { return send_codec->Matches(codec); }); + if (codec_match != send_codecs_.end()) { send_codec_with_svc_info = *codec_match; } } - return cricket::CheckScalabilityModeValues(parameters, codec_preferences_, + return cricket::CheckScalabilityModeValues(parameters, send_codecs_, send_codec_with_svc_info); } diff --git a/pc/rtp_sender.h b/pc/rtp_sender.h index 8925230636..90d33aaffc 100644 --- a/pc/rtp_sender.h +++ b/pc/rtp_sender.h @@ -102,8 +102,7 @@ class RtpSenderInternal : public RtpSenderInterface { // Used by the owning transceiver to inform the sender on the currently // selected codecs. - virtual void SetCodecPreferences( - std::vector codec_preferences) = 0; + virtual void SetSendCodecs(std::vector send_codecs) = 0; }; // Shared implementation for RtpSenderInternal interface. @@ -221,9 +220,8 @@ class RtpSenderBase : public RtpSenderInternal, public ObserverInterface { is_transceiver_stopped_ = true; } - void SetCodecPreferences( - std::vector codec_preferences) override { - codec_preferences_ = codec_preferences; + void SetSendCodecs(std::vector send_codecs) override { + send_codecs_ = send_codecs; } protected: @@ -261,7 +259,7 @@ class RtpSenderBase : public RtpSenderInternal, public ObserverInterface { std::vector stream_ids_; RtpParameters init_parameters_; - std::vector codec_preferences_; + std::vector send_codecs_; // TODO(tommi): `media_channel_` and several other member variables in this // class (ssrc_, stopped_, etc) are accessed from more than one thread without diff --git a/pc/rtp_transceiver.cc b/pc/rtp_transceiver.cc index de31b2e36a..55a0f7d584 100644 --- a/pc/rtp_transceiver.cc +++ b/pc/rtp_transceiver.cc @@ -41,12 +41,12 @@ namespace { RTCError VerifyCodecPreferences( const std::vector& codecs, - const std::vector& send_codecs, const std::vector& recv_codecs) { // If the intersection between codecs and // RTCRtpSender.getCapabilities(kind).codecs or the intersection between - // codecs and RTCRtpReceiver.getCapabilities(kind).codecs only contains RTX, - // RED or FEC codecs or is an empty set, throw InvalidModificationError. + // codecs and RTCRtpReceiver.getCapabilities(kind).codecs contains only + // contains RTX, RED, FEC or CN codecs or is an empty set, throw + // InvalidModificationError. // This ensures that we always have something to offer, regardless of // transceiver.direction. @@ -62,34 +62,15 @@ RTCError VerifyCodecPreferences( "codec capabilities."); } - if (!absl::c_any_of(codecs, [&send_codecs](const RtpCodecCapability& codec) { - return codec.IsMediaCodec() && - absl::c_any_of(send_codecs, - [&codec](const cricket::Codec& send_codec) { - return send_codec.MatchesRtpCodec(codec); - }); - })) { - LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION, - "Invalid codec preferences: Missing codec from send " - "codec capabilities."); - } - - // Let codecCapabilities be the union of - // RTCRtpSender.getCapabilities(kind).codecs and - // RTCRtpReceiver.getCapabilities(kind).codecs. For each codec in codecs, If + // Let codecCapabilities RTCRtpReceiver.getCapabilities(kind).codecs. + // For each codec in codecs, If // codec is not in codecCapabilities, throw InvalidModificationError. for (const auto& codec_preference : codecs) { bool is_recv_codec = absl::c_any_of( recv_codecs, [&codec_preference](const cricket::Codec& codec) { return codec.MatchesRtpCodec(codec_preference); }); - - bool is_send_codec = absl::c_any_of( - send_codecs, [&codec_preference](const cricket::Codec& codec) { - return codec.MatchesRtpCodec(codec_preference); - }); - - if (!is_recv_codec && !is_send_codec) { + if (!is_recv_codec) { LOG_AND_RETURN_ERROR( RTCErrorType::INVALID_MODIFICATION, std::string("Invalid codec preferences: invalid codec with name \"") + @@ -110,25 +91,6 @@ RTCError VerifyCodecPreferences( return RTCError::OK(); } -// Matches the list of codecs as capabilities (potentially without SVC related -// information) to the list of send codecs and returns the list of codecs with -// all the SVC related information. -std::vector MatchCodecPreferences( - const std::vector& codecs, - const std::vector& send_codecs) { - std::vector result; - - for (const auto& codec_preference : codecs) { - for (const cricket::VideoCodec& send_codec : send_codecs) { - if (send_codec.MatchesRtpCodec(codec_preference)) { - result.push_back(send_codec); - } - } - } - - return result; -} - TaskQueueBase* GetCurrentTaskQueueOrThread() { TaskQueueBase* current = TaskQueueBase::Current(); if (!current) @@ -165,7 +127,7 @@ RtpTransceiver::RtpTransceiver( RTC_DCHECK(media_type_ == cricket::MEDIA_TYPE_AUDIO || media_type_ == cricket::MEDIA_TYPE_VIDEO); RTC_DCHECK_EQ(sender->media_type(), receiver->media_type()); - sender->internal()->SetCodecPreferences( + sender->internal()->SetSendCodecs( sender->media_type() == cricket::MEDIA_TYPE_VIDEO ? media_engine()->video().send_codecs(false) : media_engine()->voice().send_codecs()); @@ -441,10 +403,7 @@ void RtpTransceiver::AddSender( media_type() == cricket::MEDIA_TYPE_VIDEO ? media_engine()->video().send_codecs(false) : media_engine()->voice().send_codecs(); - sender->internal()->SetCodecPreferences( - codec_preferences_.empty() - ? send_codecs - : MatchCodecPreferences(codec_preferences_, send_codecs)); + sender->internal()->SetSendCodecs(send_codecs); senders_.push_back(sender); } @@ -693,10 +652,6 @@ RTCError RtpTransceiver::SetCodecPreferences( // to codecs and abort these steps. if (codec_capabilities.empty()) { codec_preferences_.clear(); - senders_.front()->internal()->SetCodecPreferences( - media_type() == cricket::MEDIA_TYPE_VIDEO - ? media_engine()->video().send_codecs(false) - : media_engine()->voice().send_codecs()); return RTCError::OK(); } @@ -709,19 +664,15 @@ RTCError RtpTransceiver::SetCodecPreferences( // 6. to 8. RTCError result; - std::vector recv_codecs, send_codecs; + std::vector recv_codecs; if (media_type_ == cricket::MEDIA_TYPE_AUDIO) { - send_codecs = media_engine()->voice().send_codecs(); recv_codecs = media_engine()->voice().recv_codecs(); } else if (media_type_ == cricket::MEDIA_TYPE_VIDEO) { - send_codecs = media_engine()->video().send_codecs(context()->use_rtx()); recv_codecs = media_engine()->video().recv_codecs(context()->use_rtx()); } - result = VerifyCodecPreferences(codecs, send_codecs, recv_codecs); + result = VerifyCodecPreferences(codecs, recv_codecs); if (result.ok()) { - senders_.front()->internal()->SetCodecPreferences( - MatchCodecPreferences(codecs, send_codecs)); codec_preferences_ = codecs; } diff --git a/pc/test/mock_rtp_sender_internal.h b/pc/test/mock_rtp_sender_internal.h index 4cfb2cfeaf..9a01aecabb 100644 --- a/pc/test/mock_rtp_sender_internal.h +++ b/pc/test/mock_rtp_sender_internal.h @@ -69,7 +69,7 @@ class MockRtpSenderInternal : public RtpSenderInternal { (const RtpParameters&), (override)); MOCK_METHOD(void, - SetCodecPreferences, + SetSendCodecs, (std::vector), (override)); MOCK_METHOD(rtc::scoped_refptr,