Update when/how `requested_resolution` throws for invalid parameters.
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 <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#43002}
diff --git a/media/base/media_engine.cc b/media/base/media_engine.cc
index acb95d1..dff4de0 100644
--- a/media/base/media_engine.cc
+++ b/media/base/media_engine.cc
@@ -145,6 +145,7 @@
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 @@
}
}
- 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 @@
}
}
+ 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 bce27fc..552df13 100644
--- a/pc/peer_connection_encodings_integrationtest.cc
+++ b/pc/peer_connection_encodings_integrationtest.cc
@@ -187,6 +187,24 @@
local_pc_wrapper.get(), &PeerConnectionTestWrapper::AddIceCandidate);
}
+ // Negotiate without any tweaks (does not work for simulcast loopback).
+ void Negotiate(
+ rtc::scoped_refptr<PeerConnectionTestWrapper> local_pc_wrapper,
+ rtc::scoped_refptr<PeerConnectionTestWrapper> remote_pc_wrapper) {
+ std::unique_ptr<SessionDescriptionInterface> offer =
+ CreateOffer(local_pc_wrapper);
+ rtc::scoped_refptr<MockSetSessionDescriptionObserver> p1 =
+ SetLocalDescription(local_pc_wrapper, offer.get());
+ rtc::scoped_refptr<MockSetSessionDescriptionObserver> p2 =
+ SetRemoteDescription(remote_pc_wrapper, offer.get());
+ EXPECT_TRUE(Await({p1, p2}));
+ std::unique_ptr<SessionDescriptionInterface> 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<PeerConnectionTestWrapper> local_pc_wrapper,
rtc::scoped_refptr<PeerConnectionTestWrapper> remote_pc_wrapper) {
@@ -2098,6 +2116,86 @@
ASSERT_TRUE(transceiver_or_error.ok());
}
+TEST_F(PeerConnectionEncodingsIntegrationTest,
+ RequestedResolutionParameterChecking) {
+ rtc::scoped_refptr<PeerConnectionTestWrapper> 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<PeerConnectionTestWrapper> local_pc_wrapper = CreatePc();
+ rtc::scoped_refptr<PeerConnectionTestWrapper> remote_pc_wrapper = CreatePc();
+
+ rtc::scoped_refptr<MediaStreamInterface> stream =
+ local_pc_wrapper->GetUserMedia(
+ /*audio=*/false, {}, /*video=*/true, {.width = 640, .height = 360});
+ rtc::scoped_refptr<VideoTrackInterface> 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.
