From 254bd32188e654b2c03b7d9fc29851d5197200b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Wed, 11 Sep 2024 10:32:08 +0200 Subject: [PATCH] Update when/how `requested_resolution` throws for invalid parameters. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This CL makes `requested_resolution`, which is the C++ name for what the spec calls scaleResolutionDownTo, align with the latest PR[1]. The PR says to ignore scaleResolutionDownBy when scaleResolutionDownTo is specified as to be backwards compatible with scaleResolutionDownBy's default scaling factors (e.g. 4:2:1). Ignoring is different than what the code does today which is to throw an InvalidModificationError. We don't want to throw or else get+setParameters() would throw by default due to 4:2:1 defaults so the app would have to remember to delete these attributes every time even though it never specified them (Chrome has a bug here but fixing that would expose this problem, see https://crbug.com/344943229). [1] https://github.com/w3c/webrtc-extensions/pull/221 Bug: none Change-Id: I21165c9b9f9ee7259d88b89f9ae58b862ea4521e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/362260 Commit-Queue: Henrik Boström Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/main@{#43002} --- media/base/media_engine.cc | 18 +++- ...er_connection_encodings_integrationtest.cc | 98 +++++++++++++++++++ 2 files changed, 111 insertions(+), 5 deletions(-) diff --git a/media/base/media_engine.cc b/media/base/media_engine.cc index acb95d11c0..dff4de0b14 100644 --- a/media/base/media_engine.cc +++ b/media/base/media_engine.cc @@ -145,6 +145,7 @@ webrtc::RTCError CheckRtpParametersValues( const webrtc::FieldTrialsView& field_trials) { using webrtc::RTCErrorType; + bool has_requested_resolution = false; for (size_t i = 0; i < rtp_parameters.encodings.size(); ++i) { if (rtp_parameters.encodings[i].bitrate_priority <= 0) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_RANGE, @@ -183,11 +184,8 @@ webrtc::RTCError CheckRtpParametersValues( } } - if (rtp_parameters.encodings[i].requested_resolution && - rtp_parameters.encodings[i].scale_resolution_down_by) { - LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_RANGE, - "Attempted to set scale_resolution_down_by and " - "requested_resolution simultaniously."); + if (rtp_parameters.encodings[i].requested_resolution.has_value()) { + has_requested_resolution = true; } if (!field_trials.IsEnabled("WebRTC-MixedCodecSimulcast")) { @@ -200,6 +198,16 @@ webrtc::RTCError CheckRtpParametersValues( } } + if (has_requested_resolution && + absl::c_any_of(rtp_parameters.encodings, + [](const webrtc::RtpEncodingParameters& encoding) { + return !encoding.requested_resolution.has_value(); + })) { + LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_MODIFICATION, + "If a resolution is specified on any encoding then " + "it must be specified on all encodings."); + } + return CheckScalabilityModeValues(rtp_parameters, send_codecs, send_codec); } diff --git a/pc/peer_connection_encodings_integrationtest.cc b/pc/peer_connection_encodings_integrationtest.cc index bce27fcfb5..552df132a1 100644 --- a/pc/peer_connection_encodings_integrationtest.cc +++ b/pc/peer_connection_encodings_integrationtest.cc @@ -187,6 +187,24 @@ class PeerConnectionEncodingsIntegrationTest : public ::testing::Test { local_pc_wrapper.get(), &PeerConnectionTestWrapper::AddIceCandidate); } + // Negotiate without any tweaks (does not work for simulcast loopback). + void Negotiate( + rtc::scoped_refptr local_pc_wrapper, + rtc::scoped_refptr remote_pc_wrapper) { + std::unique_ptr offer = + CreateOffer(local_pc_wrapper); + rtc::scoped_refptr p1 = + SetLocalDescription(local_pc_wrapper, offer.get()); + rtc::scoped_refptr p2 = + SetRemoteDescription(remote_pc_wrapper, offer.get()); + EXPECT_TRUE(Await({p1, p2})); + std::unique_ptr answer = + CreateAnswer(remote_pc_wrapper); + p1 = SetLocalDescription(remote_pc_wrapper, answer.get()); + p2 = SetRemoteDescription(local_pc_wrapper, answer.get()); + EXPECT_TRUE(Await({p1, p2})); + } + void NegotiateWithSimulcastTweaks( rtc::scoped_refptr local_pc_wrapper, rtc::scoped_refptr remote_pc_wrapper) { @@ -2098,6 +2116,86 @@ TEST_F(PeerConnectionEncodingsIntegrationTest, ASSERT_TRUE(transceiver_or_error.ok()); } +TEST_F(PeerConnectionEncodingsIntegrationTest, + RequestedResolutionParameterChecking) { + rtc::scoped_refptr pc_wrapper = CreatePc(); + + // AddTransceiver: If `requested_resolution` is specified on any encoding it + // must be specified on all encodings. + RtpTransceiverInit init; + RtpEncodingParameters encoding; + encoding.requested_resolution = std::nullopt; + init.send_encodings.push_back(encoding); + encoding.requested_resolution = {.width = 1280, .height = 720}; + init.send_encodings.push_back(encoding); + auto transceiver_or_error = + pc_wrapper->pc()->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, init); + EXPECT_FALSE(transceiver_or_error.ok()); + EXPECT_EQ(transceiver_or_error.error().type(), + RTCErrorType::UNSUPPORTED_OPERATION); + + // AddTransceiver: Specifying both `requested_resolution` and + // `scale_resolution_down_by` is allowed (the latter is ignored). + init.send_encodings[0].requested_resolution = {.width = 640, .height = 480}; + init.send_encodings[0].scale_resolution_down_by = 1.0; + init.send_encodings[1].requested_resolution = {.width = 1280, .height = 720}; + init.send_encodings[1].scale_resolution_down_by = 2.0; + transceiver_or_error = + pc_wrapper->pc()->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, init); + ASSERT_TRUE(transceiver_or_error.ok()); + + // SetParameters: If `requested_resolution` is specified on any encoding it + // must be specified on all encodings. + auto sender = transceiver_or_error.value()->sender(); + auto parameters = sender->GetParameters(); + parameters.encodings[0].requested_resolution = {.width = 640, .height = 480}; + parameters.encodings[1].requested_resolution = std::nullopt; + auto error = sender->SetParameters(parameters); + EXPECT_FALSE(error.ok()); + EXPECT_EQ(error.type(), RTCErrorType::INVALID_MODIFICATION); + + // SetParameters: Specifying both `requested_resolution` and + // `scale_resolution_down_by` is allowed (the latter is ignored). + parameters = sender->GetParameters(); + parameters.encodings[0].requested_resolution = {.width = 640, .height = 480}; + parameters.encodings[0].scale_resolution_down_by = 2.0; + parameters.encodings[1].requested_resolution = {.width = 1280, .height = 720}; + parameters.encodings[1].scale_resolution_down_by = 1.0; + error = sender->SetParameters(parameters); + EXPECT_TRUE(error.ok()); +} + +TEST_F(PeerConnectionEncodingsIntegrationTest, + ScaleResolutionDownByIsIgnoredWhenRequestedResolutionIsSpecified) { + rtc::scoped_refptr local_pc_wrapper = CreatePc(); + rtc::scoped_refptr remote_pc_wrapper = CreatePc(); + + rtc::scoped_refptr stream = + local_pc_wrapper->GetUserMedia( + /*audio=*/false, {}, /*video=*/true, {.width = 640, .height = 360}); + rtc::scoped_refptr track = stream->GetVideoTracks()[0]; + + // Configure contradicting scaling factors (180p vs 360p). + RtpTransceiverInit init; + RtpEncodingParameters encoding; + encoding.scale_resolution_down_by = 2.0; + encoding.requested_resolution = {.width = 640, .height = 360}; + init.send_encodings.push_back(encoding); + auto transceiver_or_error = + local_pc_wrapper->pc()->AddTransceiver(track, init); + + // Negotiate singlecast. + ExchangeIceCandidates(local_pc_wrapper, remote_pc_wrapper); + Negotiate(local_pc_wrapper, remote_pc_wrapper); + + // Confirm 640x360 is sent. + // If `scale_resolution_down_by` was not ignored we would never ramp up to + // full resolution. + ASSERT_TRUE_WAIT(GetEncodingResolution(local_pc_wrapper) == + (Resolution{.width = 640, .height = 360}), + kLongTimeoutForRampingUp.ms()); +} + // Tests that use the standard path (specifying both `scalability_mode` and // `scale_resolution_down_by` or `requested_resolution`) should pass for all // codecs.