From 9f68535e68bbb4505fe2897952202ff20f682b9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Fri, 17 Jan 2025 12:06:53 +0100 Subject: [PATCH] Fix setParameters() throwing when level-id does not match. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In order to align with this PR[1], setParameters() should not throw if the H265 level ID we're trying to send does not match what was negotiated. This was believed to be fixed by [2] but we were still throwing due to a check on a different layer (media_engine.cc). In order to reproduce the issue despite WebRTC lacking SW encoder/decoder for H265, peer_connection_encodings_integrationtest.cc gets a new test with real stack but fake encoder/decoder factory. This allows negotiating H265 and doing SetParameters() even though the codec is not processing any frames. - Basic test coverage is added for singlecast and simulcast H265. - Test coverage for the bug being fixed added. - In Chrome the equivalent WPTs exists for when real HW is available here[3]. Those tests PASS with this CL (currently FAIL). [1] https://github.com/w3c/webrtc-pc/pull/3023 [2] https://webrtc-review.googlesource.com/c/src/+/368781 [3] https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/webrtc/protocol/h265-level-id.https.html Bug: chromium:381407888 Change-Id: I3619a124586b8b26d3695cfad8890cf40bd475db Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/374164 Reviewed-by: Harald Alvestrand Commit-Queue: Henrik Boström Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Jianlin Qiu Cr-Commit-Position: refs/heads/main@{#43759} --- media/base/media_engine.cc | 3 +- media/engine/webrtc_video_engine.cc | 3 +- ...er_connection_encodings_integrationtest.cc | 166 ++++++++++++++++++ pc/test/peer_connection_test_wrapper.cc | 25 ++- pc/test/peer_connection_test_wrapper.h | 9 + 5 files changed, 198 insertions(+), 8 deletions(-) diff --git a/media/base/media_engine.cc b/media/base/media_engine.cc index 0ce26ff9b6..eb7446623c 100644 --- a/media/base/media_engine.cc +++ b/media/base/media_engine.cc @@ -96,7 +96,8 @@ webrtc::RTCError CheckScalabilityModeValues( if (rtp_parameters.encodings[i].codec) { bool codecFound = false; for (const cricket::Codec& codec : send_codecs) { - if (IsSameRtpCodec(codec, *rtp_parameters.encodings[i].codec) && + if (IsSameRtpCodecIgnoringLevel(codec, + *rtp_parameters.encodings[i].codec) && SupportsMode(codec, rtp_parameters.encodings[i].scalability_mode)) { codecFound = true; send_codec = codec; diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 560fb4c16a..ac0042ebc4 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -1432,7 +1432,8 @@ webrtc::RTCError WebRtcVideoSendChannel::SetRtpSendParameters( // the first layer. // TODO: https://issues.webrtc.org/362277533 - Support mixed-codec simulcast if (parameters.encodings[0].codec && send_codec_ && - !IsSameRtpCodec(send_codec_->codec, *parameters.encodings[0].codec)) { + !IsSameRtpCodecIgnoringLevel(send_codec_->codec, + *parameters.encodings[0].codec)) { RTC_LOG(LS_VERBOSE) << "Trying to change codec to " << parameters.encodings[0].codec->name; // Ignore level when matching negotiated codecs against the requested diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc index f2fb6d82d8..1b6eeaf52e 100644 --- a/pc/peer_connection_encodings_integrationtest.cc +++ b/pc/peer_connection_encodings_integrationtest.cc @@ -41,6 +41,7 @@ #include "api/test/rtc_error_matchers.h" #include "api/units/data_rate.h" #include "api/units/time_delta.h" +#include "media/engine/fake_webrtc_video_engine.h" #include "pc/sdp_utils.h" #include "pc/session_description.h" #include "pc/simulcast_description.h" @@ -2994,4 +2995,169 @@ INSTANTIATE_TEST_SUITE_P(StandardPath, "AV1"), StringParamToString()); +#ifdef RTC_ENABLE_H265 +// These tests use fake encoders and decoders, allowing testing of codec +// preferences, SDP negotiation and get/setParamaters(). But because the codecs +// implementations are fake, these tests do not encode or decode any frames. +class PeerConnectionEncodingsFakeCodecsIntegrationTest + : public PeerConnectionEncodingsIntegrationTest { + public: + scoped_refptr CreatePcWithFakeH265( + std::unique_ptr field_trials = nullptr) { + std::unique_ptr + video_encoder_factory = + std::make_unique(); + video_encoder_factory->AddSupportedVideoCodec( + SdpVideoFormat("H265", + {{"profile-id", "1"}, + {"tier-flag", "0"}, + {"level-id", "156"}, + {"tx-mode", "SRST"}}, + {ScalabilityMode::kL1T1})); + std::unique_ptr + video_decoder_factory = + std::make_unique(); + video_decoder_factory->AddSupportedVideoCodecType("H265"); + auto pc_wrapper = make_ref_counted( + "pc", &pss_, background_thread_.get(), background_thread_.get()); + pc_wrapper->CreatePc( + {}, CreateBuiltinAudioEncoderFactory(), + CreateBuiltinAudioDecoderFactory(), std::move(video_encoder_factory), + std::move(video_decoder_factory), std::move(field_trials)); + return pc_wrapper; + } +}; + +TEST_F(PeerConnectionEncodingsFakeCodecsIntegrationTest, H265Singlecast) { + rtc::scoped_refptr local_pc_wrapper = + CreatePcWithFakeH265(); + rtc::scoped_refptr remote_pc_wrapper = + CreatePcWithFakeH265(); + ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); + + rtc::scoped_refptr transceiver = + local_pc_wrapper->pc() + ->AddTransceiver(cricket::MEDIA_TYPE_VIDEO) + .MoveValue(); + std::vector preferred_codecs = + GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "H265"); + transceiver->SetCodecPreferences(preferred_codecs); + + Negotiate(local_pc_wrapper, remote_pc_wrapper); + local_pc_wrapper->WaitForConnection(); + remote_pc_wrapper->WaitForConnection(); + + // Verify codec. + 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/H265")); +} + +TEST_F(PeerConnectionEncodingsFakeCodecsIntegrationTest, H265Simulcast) { + rtc::scoped_refptr local_pc_wrapper = + CreatePcWithFakeH265(); + rtc::scoped_refptr remote_pc_wrapper = + CreatePcWithFakeH265(); + ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); + + std::vector layers = + CreateLayers({"q", "h", "f"}, /*active=*/true); + + rtc::scoped_refptr transceiver = + AddTransceiverWithSimulcastLayers(local_pc_wrapper, remote_pc_wrapper, + layers); + std::vector preferred_codecs = + GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "H265"); + transceiver->SetCodecPreferences(preferred_codecs); + + NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); + local_pc_wrapper->WaitForConnection(); + remote_pc_wrapper->WaitForConnection(); + + // Wait until all outbound RTPs exist. + EXPECT_THAT( + GetStatsUntil(local_pc_wrapper, OutboundRtpStatsAre(UnorderedElementsAre( + AllOf(RidIs("q")), AllOf(RidIs("h")), + AllOf(RidIs("f"))))), + IsRtcOk()); + + // Verify codec. + rtc::scoped_refptr report = GetStats(local_pc_wrapper); + std::vector outbound_rtps = + report->GetStatsOfType(); + ASSERT_THAT(outbound_rtps, SizeIs(3u)); + EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[0]), + StrCaseEq("video/H265")); + EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[1]), + StrCaseEq("video/H265")); + EXPECT_THAT(GetCurrentCodecMimeType(report, *outbound_rtps[2]), + StrCaseEq("video/H265")); +} + +TEST_F(PeerConnectionEncodingsFakeCodecsIntegrationTest, + H265SetParametersIgnoresLevelId) { + rtc::scoped_refptr local_pc_wrapper = + CreatePcWithFakeH265(); + rtc::scoped_refptr remote_pc_wrapper = + CreatePcWithFakeH265(); + 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); + std::vector preferred_codecs = + GetCapabilitiesAndRestrictToCodec(local_pc_wrapper, "H265"); + transceiver->SetCodecPreferences(preferred_codecs); + rtc::scoped_refptr sender = transceiver->sender(); + + NegotiateWithSimulcastTweaks(local_pc_wrapper, remote_pc_wrapper); + local_pc_wrapper->WaitForConnection(); + remote_pc_wrapper->WaitForConnection(); + + // This includes non-codecs like rtx, red and flexfec too so we need to find + // H265. + std::vector sender_codecs = + local_pc_wrapper->pc_factory() + ->GetRtpSenderCapabilities(cricket::MEDIA_TYPE_VIDEO) + .codecs; + auto it = std::find_if(sender_codecs.begin(), sender_codecs.end(), + [](const RtpCodecCapability codec_capability) { + return codec_capability.name == "H265"; + }); + ASSERT_NE(it, sender_codecs.end()); + RtpCodecCapability& h265_codec = *it; + + // SetParameters() without changing level-id. + EXPECT_EQ(h265_codec.parameters["level-id"], "156"); + { + RtpParameters parameters = sender->GetParameters(); + ASSERT_THAT(parameters.encodings, SizeIs(1)); + parameters.encodings[0].codec = h265_codec; + ASSERT_THAT(sender->SetParameters(parameters), IsRtcOk()); + } + // SetParameters() with a lower level-id. + h265_codec.parameters["level-id"] = "30"; + { + RtpParameters parameters = sender->GetParameters(); + ASSERT_THAT(parameters.encodings, SizeIs(1)); + parameters.encodings[0].codec = h265_codec; + ASSERT_THAT(sender->SetParameters(parameters), IsRtcOk()); + } + // SetParameters() with a higher level-id. + h265_codec.parameters["level-id"] = "180"; + { + RtpParameters parameters = sender->GetParameters(); + ASSERT_THAT(parameters.encodings, SizeIs(1)); + parameters.encodings[0].codec = h265_codec; + ASSERT_THAT(sender->SetParameters(parameters), IsRtcOk()); + } +} +#endif // RTC_ENABLE_H265 + } // namespace webrtc diff --git a/pc/test/peer_connection_test_wrapper.cc b/pc/test/peer_connection_test_wrapper.cc index 1774f1bf08..e458794717 100644 --- a/pc/test/peer_connection_test_wrapper.cc +++ b/pc/test/peer_connection_test_wrapper.cc @@ -172,6 +172,8 @@ bool PeerConnectionTestWrapper::CreatePc( const webrtc::PeerConnectionInterface::RTCConfiguration& config, rtc::scoped_refptr audio_encoder_factory, rtc::scoped_refptr audio_decoder_factory, + std::unique_ptr video_encoder_factory, + std::unique_ptr video_decoder_factory, std::unique_ptr field_trials) { std::unique_ptr port_allocator( new cricket::FakePortAllocator( @@ -190,12 +192,7 @@ bool PeerConnectionTestWrapper::CreatePc( network_thread_, worker_thread_, rtc::Thread::Current(), rtc::scoped_refptr(fake_audio_capture_module_), audio_encoder_factory, audio_decoder_factory, - std::make_unique(), - std::make_unique>(), + std::move(video_encoder_factory), std::move(video_decoder_factory), nullptr /* audio_mixer */, nullptr /* audio_processing */, nullptr, std::move(field_trials)); if (!peer_connection_factory_) { @@ -217,6 +214,22 @@ bool PeerConnectionTestWrapper::CreatePc( } } +bool PeerConnectionTestWrapper::CreatePc( + const webrtc::PeerConnectionInterface::RTCConfiguration& config, + rtc::scoped_refptr audio_encoder_factory, + rtc::scoped_refptr audio_decoder_factory, + std::unique_ptr field_trials) { + return CreatePc(config, std::move(audio_encoder_factory), + std::move(audio_decoder_factory), + std::make_unique(), + std::make_unique>(), + std::move(field_trials)); +} + rtc::scoped_refptr PeerConnectionTestWrapper::CreateDataChannel( const std::string& label, diff --git a/pc/test/peer_connection_test_wrapper.h b/pc/test/peer_connection_test_wrapper.h index 0a3a689e94..5633ea8937 100644 --- a/pc/test/peer_connection_test_wrapper.h +++ b/pc/test/peer_connection_test_wrapper.h @@ -29,6 +29,8 @@ #include "api/scoped_refptr.h" #include "api/sequence_checker.h" #include "api/video/resolution.h" +#include "api/video_codecs/video_decoder_factory.h" +#include "api/video_codecs/video_encoder_factory.h" #include "pc/test/fake_audio_capture_module.h" #include "pc/test/fake_periodic_video_source.h" #include "pc/test/fake_periodic_video_track_source.h" @@ -55,6 +57,13 @@ class PeerConnectionTestWrapper rtc::scoped_refptr audio_encoder_factory, rtc::scoped_refptr audio_decoder_factory, std::unique_ptr field_trials = nullptr); + bool CreatePc( + const webrtc::PeerConnectionInterface::RTCConfiguration& config, + rtc::scoped_refptr audio_encoder_factory, + rtc::scoped_refptr audio_decoder_factory, + std::unique_ptr video_encoder_factory, + std::unique_ptr video_decoder_factory, + std::unique_ptr field_trials = nullptr); rtc::scoped_refptr pc_factory() const {