From f48c3736e0078a990c4532f7d64a59b2528ab1c0 Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Thu, 15 Jul 2021 14:39:43 +0200 Subject: [PATCH] Add ability to configure video codecs at peer level (PC level framework) Bug: b/192821182 Change-Id: Ic1b55028102fbd331f0fb6a3a8c758c311267cbc Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/226220 Reviewed-by: Andrey Logvin Commit-Queue: Mirko Bonadei Cr-Commit-Position: refs/heads/master@{#34477} --- .../peerconnection_quality_test_fixture.h | 55 ++++++++++++------- test/pc/e2e/peer_configurer.cc | 26 ++++++--- test/pc/e2e/peer_configurer.h | 6 ++ test/pc/e2e/peer_connection_quality_test.cc | 19 ++++--- .../e2e/peer_connection_quality_test_params.h | 2 + test/pc/e2e/sdp/sdp_changer.cc | 24 ++++---- test/pc/e2e/sdp/sdp_changer.h | 17 +++--- 7 files changed, 91 insertions(+), 58 deletions(-) diff --git a/api/test/peerconnection_quality_test_fixture.h b/api/test/peerconnection_quality_test_fixture.h index 8717e8f73d..20d9c74efa 100644 --- a/api/test/peerconnection_quality_test_fixture.h +++ b/api/test/peerconnection_quality_test_fixture.h @@ -268,6 +268,27 @@ class PeerConnectionE2EQualityTestFixture { absl::optional sync_group; }; + struct VideoCodecConfig { + explicit VideoCodecConfig(std::string name) + : name(std::move(name)), required_params() {} + VideoCodecConfig(std::string name, + std::map required_params) + : name(std::move(name)), required_params(std::move(required_params)) {} + // Next two fields are used to specify concrete video codec, that should be + // used in the test. Video code will be negotiated in SDP during offer/ + // answer exchange. + // Video codec name. You can find valid names in + // media/base/media_constants.h + std::string name = cricket::kVp8CodecName; + // Map of parameters, that have to be specified on SDP codec. Each parameter + // is described by key and value. Codec parameters will match the specified + // map if and only if for each key from |required_params| there will be + // a parameter with name equal to this key and parameter value will be equal + // to the value from |required_params| for this key. + // If empty then only name will be used to match the codec. + std::map required_params; + }; + // This class is used to fully configure one peer inside the call. class PeerConfigurer { public: @@ -343,6 +364,14 @@ class PeerConnectionE2EQualityTestFixture { // applied to all summed RTP streams for this peer. virtual PeerConfigurer* SetBitrateSettings( BitrateSettings bitrate_settings) = 0; + // Set the list of video codecs used by the peer during the test. These + // codecs will be negotiated in SDP during offer/answer exchange. The order + // of these codecs during negotiation will be the same as in |video_codecs|. + // Codecs have to be available in codecs list provided by peer connection to + // be negotiated. If some of specified codecs won't be found, the test will + // crash. + virtual PeerConfigurer* SetVideoCodecs( + std::vector video_codecs) = 0; }; // Contains configuration for echo emulator. @@ -352,27 +381,6 @@ class PeerConnectionE2EQualityTestFixture { TimeDelta echo_delay = TimeDelta::Millis(50); }; - struct VideoCodecConfig { - explicit VideoCodecConfig(std::string name) - : name(std::move(name)), required_params() {} - VideoCodecConfig(std::string name, - std::map required_params) - : name(std::move(name)), required_params(std::move(required_params)) {} - // Next two fields are used to specify concrete video codec, that should be - // used in the test. Video code will be negotiated in SDP during offer/ - // answer exchange. - // Video codec name. You can find valid names in - // media/base/media_constants.h - std::string name = cricket::kVp8CodecName; - // Map of parameters, that have to be specified on SDP codec. Each parameter - // is described by key and value. Codec parameters will match the specified - // map if and only if for each key from |required_params| there will be - // a parameter with name equal to this key and parameter value will be equal - // to the value from |required_params| for this key. - // If empty then only name will be used to match the codec. - std::map required_params; - }; - // Contains parameters, that describe how long framework should run quality // test. struct RunParams { @@ -383,6 +391,11 @@ class PeerConnectionE2EQualityTestFixture { // it will be shut downed. TimeDelta run_duration; + // DEPRECATED: Instead of setting the codecs in RunParams (which apply to + // all the participants in the call, please set them with + // PeerConfigurer, this will allow more flexibility and let + // different Peers support different codecs. + // // List of video codecs to use during the test. These codecs will be // negotiated in SDP during offer/answer exchange. The order of these codecs // during negotiation will be the same as in |video_codecs|. Codecs have diff --git a/test/pc/e2e/peer_configurer.cc b/test/pc/e2e/peer_configurer.cc index 18570c2c6b..caf29eb992 100644 --- a/test/pc/e2e/peer_configurer.cc +++ b/test/pc/e2e/peer_configurer.cc @@ -90,11 +90,22 @@ void SetDefaultValuesForMissingParams( *p->name + "_auto_audio_stream_label_"); audio_stream_names_provider.MaybeSetName(&p->audio_config->stream_label); } - } - if (run_params->video_codecs.empty()) { - run_params->video_codecs.push_back( - VideoCodecConfig(cricket::kVp8CodecName)); + if (p->video_codecs.empty()) { + // TODO(mbonadei): Remove the usage of RunParams to set codecs, this is + // only needed for backwards compatibility. + if (!run_params->video_codecs.empty()) { + p->video_codecs = run_params->video_codecs; + } else { + p->video_codecs.push_back( + PeerConnectionE2EQualityTestFixture::VideoCodecConfig( + cricket::kVp8CodecName)); + } + } else { + RTC_CHECK(run_params->video_codecs.empty()) + << "Setting video_codecs in both PeerConfigurer and RunParams is not " + "supported."; + } } } @@ -102,7 +113,6 @@ void ValidateParams( const RunParams& run_params, const std::vector>& peers) { RTC_CHECK_GT(run_params.video_encoder_bitrate_multiplier, 0.0); - RTC_CHECK_GE(run_params.video_codecs.size(), 1); std::set peer_names; std::set video_labels; @@ -113,6 +123,8 @@ void ValidateParams( for (size_t i = 0; i < peers.size(); ++i) { Params* p = peers[i]->params(); + // Each peer should at least support 1 video codec. + RTC_CHECK_GE(p->video_codecs.size(), 1); { RTC_CHECK(p->name); @@ -160,14 +172,14 @@ void ValidateParams( RTC_CHECK_LT(*video_config.simulcast_config->target_spatial_index, video_config.simulcast_config->simulcast_streams_count); } - RTC_CHECK_EQ(run_params.video_codecs.size(), 1) + RTC_CHECK_EQ(p->video_codecs.size(), 1) << "Only 1 video codec is supported when simulcast is enabled in " << "at least 1 video config"; RTC_CHECK(!video_config.max_encode_bitrate_bps) << "Setting max encode bitrate is not implemented for simulcast."; RTC_CHECK(!video_config.min_encode_bitrate_bps) << "Setting min encode bitrate is not implemented for simulcast."; - if (run_params.video_codecs[0].name == cricket::kVp8CodecName && + if (p->video_codecs[0].name == cricket::kVp8CodecName && !video_config.simulcast_config->encoding_params.empty()) { RTC_CHECK_EQ(video_config.simulcast_config->simulcast_streams_count, video_config.simulcast_config->encoding_params.size()) diff --git a/test/pc/e2e/peer_configurer.h b/test/pc/e2e/peer_configurer.h index 422d3d7341..bacfb7a295 100644 --- a/test/pc/e2e/peer_configurer.h +++ b/test/pc/e2e/peer_configurer.h @@ -168,6 +168,12 @@ class PeerConfigurerImpl final params_->bitrate_settings = bitrate_settings; return this; } + PeerConfigurer* SetVideoCodecs( + std::vector + video_codecs) override { + params_->video_codecs = std::move(video_codecs); + return this; + } PeerConfigurer* SetIceTransportFactory( std::unique_ptr factory) override { diff --git a/test/pc/e2e/peer_connection_quality_test.cc b/test/pc/e2e/peer_connection_quality_test.cc index 38a9ebf801..6fbfd37eac 100644 --- a/test/pc/e2e/peer_connection_quality_test.cc +++ b/test/pc/e2e/peer_connection_quality_test.cc @@ -444,8 +444,9 @@ void PeerConnectionE2EQualityTest::SetupCallOnSignalingThread( RtpTransceiverInit transceiver_params; if (video_config.simulcast_config) { transceiver_params.direction = RtpTransceiverDirection::kSendOnly; - // Because simulcast enabled |run_params.video_codecs| has only 1 element. - if (run_params.video_codecs[0].name == cricket::kVp8CodecName) { + // Because simulcast enabled |alice_->params()->video_codecs| has only 1 + // element. + if (alice_->params()->video_codecs[0].name == cricket::kVp8CodecName) { // For Vp8 simulcast we need to add as many RtpEncodingParameters to the // track as many simulcast streams requested. If they specified in // |video_config.simulcast_config| it should be copied from there. @@ -508,14 +509,14 @@ void PeerConnectionE2EQualityTest::SetPeerCodecPreferences( const RunParams& run_params) { std::vector with_rtx_video_capabilities = FilterVideoCodecCapabilities( - run_params.video_codecs, true, run_params.use_ulp_fec, + peer->params()->video_codecs, true, run_params.use_ulp_fec, run_params.use_flex_fec, peer->pc_factory() ->GetRtpSenderCapabilities(cricket::MediaType::MEDIA_TYPE_VIDEO) .codecs); std::vector without_rtx_video_capabilities = FilterVideoCodecCapabilities( - run_params.video_codecs, false, run_params.use_ulp_fec, + peer->params()->video_codecs, false, run_params.use_ulp_fec, run_params.use_flex_fec, peer->pc_factory() ->GetRtpSenderCapabilities(cricket::MediaType::MEDIA_TYPE_VIDEO) @@ -552,8 +553,7 @@ PeerConnectionE2EQualityTest::CreateSignalingInterceptor( video_config.simulcast_config->simulcast_streams_count}); } } - PatchingParams patching_params(run_params.video_codecs, - run_params.use_conference_mode, + PatchingParams patching_params(run_params.use_conference_mode, stream_label_to_simulcast_streams_count); return std::make_unique(patching_params); } @@ -594,8 +594,8 @@ void PeerConnectionE2EQualityTest::ExchangeOfferAnswer( RTC_CHECK(offer); offer->ToString(&log_output); RTC_LOG(INFO) << "Original offer: " << log_output; - LocalAndRemoteSdp patch_result = - signaling_interceptor->PatchOffer(std::move(offer)); + LocalAndRemoteSdp patch_result = signaling_interceptor->PatchOffer( + std::move(offer), alice_->params()->video_codecs[0]); patch_result.local_sdp->ToString(&log_output); RTC_LOG(INFO) << "Offer to set as local description: " << log_output; patch_result.remote_sdp->ToString(&log_output); @@ -611,7 +611,8 @@ void PeerConnectionE2EQualityTest::ExchangeOfferAnswer( RTC_CHECK(answer); answer->ToString(&log_output); RTC_LOG(INFO) << "Original answer: " << log_output; - patch_result = signaling_interceptor->PatchAnswer(std::move(answer)); + patch_result = signaling_interceptor->PatchAnswer( + std::move(answer), bob_->params()->video_codecs[0]); patch_result.local_sdp->ToString(&log_output); RTC_LOG(INFO) << "Answer to set as local description: " << log_output; patch_result.remote_sdp->ToString(&log_output); diff --git a/test/pc/e2e/peer_connection_quality_test_params.h b/test/pc/e2e/peer_connection_quality_test_params.h index e1c0232cb2..31938f77f5 100644 --- a/test/pc/e2e/peer_connection_quality_test_params.h +++ b/test/pc/e2e/peer_connection_quality_test_params.h @@ -116,6 +116,8 @@ struct Params { PeerConnectionInterface::RTCConfiguration rtc_configuration; BitrateSettings bitrate_settings; + std::vector + video_codecs; }; } // namespace webrtc_pc_e2e diff --git a/test/pc/e2e/sdp/sdp_changer.cc b/test/pc/e2e/sdp/sdp_changer.cc index b46aea1c5f..aa067f0c40 100644 --- a/test/pc/e2e/sdp/sdp_changer.cc +++ b/test/pc/e2e/sdp/sdp_changer.cc @@ -166,14 +166,15 @@ void SignalingInterceptor::FillSimulcastContext( } LocalAndRemoteSdp SignalingInterceptor::PatchOffer( - std::unique_ptr offer) { + std::unique_ptr offer, + const PeerConnectionE2EQualityTestFixture::VideoCodecConfig& first_codec) { for (auto& content : offer->description()->contents()) { context_.mids_order.push_back(content.mid()); cricket::MediaContentDescription* media_desc = content.media_description(); if (media_desc->type() != cricket::MediaType::MEDIA_TYPE_VIDEO) { continue; } - if (content.media_description()->streams().size() == 0) { + if (content.media_description()->streams().empty()) { // It means that this media section describes receive only media section // in SDP. RTC_CHECK_EQ(content.media_description()->direction(), @@ -183,13 +184,13 @@ LocalAndRemoteSdp SignalingInterceptor::PatchOffer( media_desc->set_conference_mode(params_.use_conference_mode); } - if (params_.stream_label_to_simulcast_streams_count.size() > 0) { + if (!params_.stream_label_to_simulcast_streams_count.empty()) { // Because simulcast enabled |params_.video_codecs| has only 1 element. - if (params_.video_codecs[0].name == cricket::kVp8CodecName) { + if (first_codec.name == cricket::kVp8CodecName) { return PatchVp8Offer(std::move(offer)); } - if (params_.video_codecs[0].name == cricket::kVp9CodecName) { + if (first_codec.name == cricket::kVp9CodecName) { return PatchVp9Offer(std::move(offer)); } } @@ -362,7 +363,8 @@ LocalAndRemoteSdp SignalingInterceptor::PatchVp9Offer( } LocalAndRemoteSdp SignalingInterceptor::PatchAnswer( - std::unique_ptr answer) { + std::unique_ptr answer, + const PeerConnectionE2EQualityTestFixture::VideoCodecConfig& first_codec) { for (auto& content : answer->description()->contents()) { cricket::MediaContentDescription* media_desc = content.media_description(); if (media_desc->type() != cricket::MediaType::MEDIA_TYPE_VIDEO) { @@ -375,13 +377,13 @@ LocalAndRemoteSdp SignalingInterceptor::PatchAnswer( media_desc->set_conference_mode(params_.use_conference_mode); } - if (params_.stream_label_to_simulcast_streams_count.size() > 0) { + if (!params_.stream_label_to_simulcast_streams_count.empty()) { // Because simulcast enabled |params_.video_codecs| has only 1 element. - if (params_.video_codecs[0].name == cricket::kVp8CodecName) { + if (first_codec.name == cricket::kVp8CodecName) { return PatchVp8Answer(std::move(answer)); } - if (params_.video_codecs[0].name == cricket::kVp9CodecName) { + if (first_codec.name == cricket::kVp9CodecName) { return PatchVp9Answer(std::move(answer)); } } @@ -534,7 +536,7 @@ SignalingInterceptor::PatchOffererIceCandidates( // This is candidate for simulcast section, so it should be transformed // into candidates for replicated sections. The sdpMLineIndex is set to // -1 and ignored if the rid is present. - for (auto rid : simulcast_info_it->second->rids) { + for (const std::string& rid : simulcast_info_it->second->rids) { out.push_back(CreateIceCandidate(rid, -1, candidate->candidate())); } } else { @@ -560,7 +562,7 @@ SignalingInterceptor::PatchAnswererIceCandidates( // section. out.push_back(CreateIceCandidate(simulcast_info_it->second->mid, 0, candidate->candidate())); - } else if (context_.simulcast_infos_by_rid.size()) { + } else if (!context_.simulcast_infos_by_rid.empty()) { // When using simulcast and bundle, put everything on the first m-line. out.push_back(CreateIceCandidate("", 0, candidate->candidate())); } else { diff --git a/test/pc/e2e/sdp/sdp_changer.h b/test/pc/e2e/sdp/sdp_changer.h index 11e3d421d3..943d332945 100644 --- a/test/pc/e2e/sdp/sdp_changer.h +++ b/test/pc/e2e/sdp/sdp_changer.h @@ -61,17 +61,12 @@ struct LocalAndRemoteSdp { struct PatchingParams { PatchingParams( - std::vector - video_codecs, bool use_conference_mode, std::map stream_label_to_simulcast_streams_count) - : video_codecs(std::move(video_codecs)), - use_conference_mode(use_conference_mode), + : use_conference_mode(use_conference_mode), stream_label_to_simulcast_streams_count( stream_label_to_simulcast_streams_count) {} - std::vector - video_codecs; bool use_conference_mode; std::map stream_label_to_simulcast_streams_count; }; @@ -81,9 +76,11 @@ class SignalingInterceptor { explicit SignalingInterceptor(PatchingParams params) : params_(params) {} LocalAndRemoteSdp PatchOffer( - std::unique_ptr offer); + std::unique_ptr offer, + const PeerConnectionE2EQualityTestFixture::VideoCodecConfig& first_codec); LocalAndRemoteSdp PatchAnswer( - std::unique_ptr offer); + std::unique_ptr answer, + const PeerConnectionE2EQualityTestFixture::VideoCodecConfig& first_codec); std::vector> PatchOffererIceCandidates( rtc::ArrayView candidates); @@ -132,9 +129,9 @@ class SignalingInterceptor { LocalAndRemoteSdp PatchVp9Offer( std::unique_ptr offer); LocalAndRemoteSdp PatchVp8Answer( - std::unique_ptr offer); + std::unique_ptr answer); LocalAndRemoteSdp PatchVp9Answer( - std::unique_ptr offer); + std::unique_ptr answer); void FillSimulcastContext(SessionDescriptionInterface* offer); std::unique_ptr RestoreMediaSectionsOrder(