From 2d2c888293fe5d1a4204d5026d6d630b47f6044a Mon Sep 17 00:00:00 2001 From: Seth Hampson Date: Wed, 16 May 2018 16:02:32 -0700 Subject: [PATCH] Returns RTCError for setting unimplemented RtpParameters. We have a number of RtpParameters that aren't implemented. If a client is setting these values it creates unexpected results when the value doesn't do anything for them. This change incorporates returning the correct error if the parameter is unimplemented. It also changes the scale_resolution_down_by and scale_framerate_down_by RtpEncodingParameters to rtc::Optionals because they aren't implemented. This change is part of the effort to ship get/setParameters in Chrome. Bug: webrtc:8772 Change-Id: I9797695e5116e6aeb3c02afddbf460b2a0d7d5ab Reviewed-on: https://webrtc-review.googlesource.com/75421 Commit-Queue: Seth Hampson Reviewed-by: Taylor Brandstetter Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#23314} --- api/rtpparameters.h | 4 +- media/base/fakemediaengine.h | 2 +- pc/rtpsender.cc | 66 ++++++++++ pc/rtpsenderreceiver_unittest.cc | 200 ++++++++++++++++++++++++++++++- 4 files changed, 266 insertions(+), 6 deletions(-) diff --git a/api/rtpparameters.h b/api/rtpparameters.h index a026c5ae0f..ba02fd24d6 100644 --- a/api/rtpparameters.h +++ b/api/rtpparameters.h @@ -426,11 +426,11 @@ struct RtpEncodingParameters { // For video, scale the resolution down by this factor. // TODO(deadbeef): Not implemented. - double scale_resolution_down_by = 1.0; + rtc::Optional scale_resolution_down_by; // Scale the framerate down by this factor. // TODO(deadbeef): Not implemented. - double scale_framerate_down_by = 1.0; + rtc::Optional scale_framerate_down_by; // For an RtpSender, set to true to cause this encoding to be encoded and // sent, and false for it not to be encoded and sent. This allows control diff --git a/media/base/fakemediaengine.h b/media/base/fakemediaengine.h index e9a6bd6f40..50ef0a3502 100644 --- a/media/base/fakemediaengine.h +++ b/media/base/fakemediaengine.h @@ -110,7 +110,7 @@ template class RtpHelper : public Base { } send_streams_.push_back(sp); rtp_send_parameters_[sp.first_ssrc()] = - CreateRtpParametersWithOneEncoding(); + CreateRtpParametersWithEncodings(sp); return true; } virtual bool RemoveSendStream(uint32_t ssrc) { diff --git a/pc/rtpsender.cc b/pc/rtpsender.cc index 3b0bbf886a..b7e48cd33b 100644 --- a/pc/rtpsender.cc +++ b/pc/rtpsender.cc @@ -30,6 +30,62 @@ int GenerateUniqueId() { return ++g_unique_id; } +// Returns an true if any RtpEncodingParameters member that isn't implemented +// contains a value. +bool UnimplementedRtpEncodingParameterHasValue( + const RtpEncodingParameters& encoding_params) { + if (encoding_params.codec_payload_type.has_value() || + encoding_params.fec.has_value() || encoding_params.rtx.has_value() || + encoding_params.dtx.has_value() || encoding_params.ptime.has_value() || + encoding_params.max_framerate.has_value() || + !encoding_params.rid.empty() || + encoding_params.scale_resolution_down_by.has_value() || + encoding_params.scale_framerate_down_by.has_value() || + !encoding_params.dependency_rids.empty()) { + return true; + } + return false; +} + +// Returns true if a "per-sender" encoding parameter contains a value that isn't +// its default. Currently max_bitrate_bps and bitrate_priority both are +// implemented "per-sender," meaning that these encoding parameters +// are used for the RtpSender as a whole, not for a specific encoding layer. +// This is done by setting these encoding parameters at index 0 of +// RtpParameters.encodings. This function can be used to check if these +// parameters are set at any index other than 0 of RtpParameters.encodings, +// because they are currently unimplemented to be used for a specific encoding +// layer. +bool PerSenderRtpEncodingParameterHasValue( + const RtpEncodingParameters& encoding_params) { + if (encoding_params.max_bitrate_bps.has_value() || + encoding_params.bitrate_priority != kDefaultBitratePriority) { + return true; + } + return false; +} + +// Returns true if any RtpParameters member that isn't implemented contains a +// value. +bool UnimplementedRtpParameterHasValue(const RtpParameters& parameters) { + if (!parameters.mid.empty() || !parameters.header_extensions.empty() || + parameters.degradation_preference != DegradationPreference::BALANCED) { + return true; + } + for (size_t i = 0; i < parameters.encodings.size(); ++i) { + if (UnimplementedRtpEncodingParameterHasValue(parameters.encodings[i])) { + return true; + } + // Encoding parameters that are per-sender should only contain value at + // index 0. + if (i != 0 && + PerSenderRtpEncodingParameterHasValue(parameters.encodings[i])) { + return true; + } + } + return false; +} + } // namespace LocalAudioSinkAdapter::LocalAudioSinkAdapter() : sink_(nullptr) {} @@ -217,6 +273,11 @@ RTCError AudioRtpSender::SetParameters(const RtpParameters& parameters) { " the last value returned from getParameters()"); } + if (UnimplementedRtpParameterHasValue(parameters)) { + LOG_AND_RETURN_ERROR( + RTCErrorType::UNSUPPORTED_PARAMETER, + "Attempted to set an unimplemented parameter of RtpParameters."); + } return worker_thread_->Invoke(RTC_FROM_HERE, [&] { RTCError result = media_channel_->SetRtpSendParameters(ssrc_, parameters); last_transaction_id_.reset(); @@ -421,6 +482,11 @@ RTCError VideoRtpSender::SetParameters(const RtpParameters& parameters) { " the last value returned from getParameters()"); } + if (UnimplementedRtpParameterHasValue(parameters)) { + LOG_AND_RETURN_ERROR( + RTCErrorType::UNSUPPORTED_PARAMETER, + "Attempted to set an unimplemented parameter of RtpParameters."); + } return worker_thread_->Invoke(RTC_FROM_HERE, [&] { RTCError result = media_channel_->SetRtpSendParameters(ssrc_, parameters); last_transaction_id_.reset(); diff --git a/pc/rtpsenderreceiver_unittest.cc b/pc/rtpsenderreceiver_unittest.cc index b084b01e1a..be0e975cce 100644 --- a/pc/rtpsenderreceiver_unittest.cc +++ b/pc/rtpsenderreceiver_unittest.cc @@ -15,6 +15,7 @@ #include "api/rtpparameters.h" #include "media/base/fakemediaengine.h" #include "media/base/rtpdataengine.h" +#include "media/base/testutils.h" #include "media/engine/fakewebrtccall.h" #include "p2p/base/fakedtlstransport.h" #include "pc/audiotrack.h" @@ -162,16 +163,20 @@ class RtpSenderReceiverTest : public testing::Test, void OnAudioSenderDestroyed() { audio_sender_destroyed_signal_fired_ = true; } + void CreateVideoRtpSender(uint32_t ssrc) { + CreateVideoRtpSender(false, ssrc); + } + void CreateVideoRtpSender() { CreateVideoRtpSender(false); } - void CreateVideoRtpSender(bool is_screencast) { + void CreateVideoRtpSender(bool is_screencast, uint32_t ssrc = kVideoSsrc) { AddVideoTrack(is_screencast); video_rtp_sender_ = new VideoRtpSender(worker_thread_, local_stream_->GetVideoTracks()[0], {local_stream_->id()}); video_rtp_sender_->SetVideoMediaChannel(video_media_channel_); - video_rtp_sender_->SetSsrc(kVideoSsrc); - VerifyVideoChannelInput(); + video_rtp_sender_->SetSsrc(ssrc); + VerifyVideoChannelInput(ssrc); } void CreateVideoRtpSenderWithNoTrack() { @@ -661,6 +666,86 @@ TEST_F(RtpSenderReceiverTest, AudioSenderSetParametersOldValueFail) { RTCError result = audio_rtp_sender_->SetParameters(params); EXPECT_EQ(RTCErrorType::INVALID_MODIFICATION, result.type()); + DestroyAudioRtpSender(); +} + +TEST_F(RtpSenderReceiverTest, AudioSenderCantSetUnimplementedRtpParameters) { + CreateAudioRtpSender(); + RtpParameters params = audio_rtp_sender_->GetParameters(); + EXPECT_EQ(1u, params.encodings.size()); + + // Unimplemented RtpParameters: mid, header_extensions, + // degredation_preference. + params.mid = "dummy_mid"; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + params.header_extensions.emplace_back(); + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + ASSERT_EQ(DegradationPreference::BALANCED, params.degradation_preference); + params.degradation_preference = DegradationPreference::MAINTAIN_FRAMERATE; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + + DestroyAudioRtpSender(); +} + +TEST_F(RtpSenderReceiverTest, + AudioSenderCantSetUnimplementedRtpEncodingParameters) { + CreateAudioRtpSender(); + RtpParameters params = audio_rtp_sender_->GetParameters(); + EXPECT_EQ(1u, params.encodings.size()); + + // Unimplemented RtpParameters: codec_payload_type, fec, rtx, dtx, ptime, + // max_framerate, scale_resolution_down_by, scale_framerate_down_by, rid, + // dependency_rids. + params.encodings[0].codec_payload_type = 1; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + params.encodings[0].fec = RtpFecParameters(); + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + params.encodings[0].rtx = RtpRtxParameters(); + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + params.encodings[0].dtx = DtxStatus::ENABLED; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + params.encodings[0].ptime = 1; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + params.encodings[0].max_framerate = 1; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + params.encodings[0].scale_resolution_down_by = 2.0; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + params.encodings[0].rid = "dummy_rid"; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); + params = audio_rtp_sender_->GetParameters(); + + params.encodings[0].dependency_rids.push_back("dummy_rid"); + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + audio_rtp_sender_->SetParameters(params).type()); DestroyAudioRtpSender(); } @@ -782,6 +867,115 @@ TEST_F(RtpSenderReceiverTest, VideoSenderSetParametersOldValueFail) { DestroyVideoRtpSender(); } +TEST_F(RtpSenderReceiverTest, VideoSenderCantSetUnimplementedRtpParameters) { + CreateVideoRtpSender(); + RtpParameters params = video_rtp_sender_->GetParameters(); + EXPECT_EQ(1u, params.encodings.size()); + + // Unimplemented RtpParameters: mid, header_extensions, + // degredation_preference. + params.mid = "dummy_mid"; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.header_extensions.emplace_back(); + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + ASSERT_EQ(DegradationPreference::BALANCED, params.degradation_preference); + params.degradation_preference = DegradationPreference::MAINTAIN_FRAMERATE; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + + DestroyVideoRtpSender(); +} + +TEST_F(RtpSenderReceiverTest, + VideoSenderCantSetUnimplementedEncodingParameters) { + CreateVideoRtpSender(); + RtpParameters params = video_rtp_sender_->GetParameters(); + EXPECT_EQ(1u, params.encodings.size()); + + // Unimplemented RtpParameters: codec_payload_type, fec, rtx, dtx, ptime, + // max_framerate, scale_resolution_down_by, scale_framerate_down_by, rid, + // dependency_rids. + params.encodings[0].codec_payload_type = 1; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.encodings[0].fec = RtpFecParameters(); + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.encodings[0].rtx = RtpRtxParameters(); + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.encodings[0].dtx = DtxStatus::ENABLED; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.encodings[0].ptime = 1; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.encodings[0].max_framerate = 1; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.encodings[0].scale_resolution_down_by = 2.0; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.encodings[0].rid = "dummy_rid"; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.encodings[0].dependency_rids.push_back("dummy_rid"); + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + + DestroyVideoRtpSender(); +} + +// A video sender can have multiple simulcast layers, in which case it will +// contain multiple RtpEncodingParameters. This tests that if this is the case +// (simulcast), then we can't set the bitrate_priority, or max_bitrate_bps +// for any encodings besides at index 0, because these are both implemented +// "per-sender." +TEST_F(RtpSenderReceiverTest, VideoSenderCantSetPerSenderEncodingParameters) { + // Add a simulcast specific send stream that contains 2 encoding parameters. + std::vector ssrcs({1, 2}); + cricket::StreamParams stream_params = + cricket::CreateSimStreamParams("cname", ssrcs); + video_media_channel_->AddSendStream(stream_params); + uint32_t primary_ssrc = stream_params.first_ssrc(); + CreateVideoRtpSender(primary_ssrc); + RtpParameters params = video_rtp_sender_->GetParameters(); + EXPECT_EQ(ssrcs.size(), params.encodings.size()); + + params.encodings[1].bitrate_priority = 2.0; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + params = video_rtp_sender_->GetParameters(); + + params.encodings[1].max_bitrate_bps = 200000; + EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, + video_rtp_sender_->SetParameters(params).type()); + + DestroyVideoRtpSender(); +} + TEST_F(RtpSenderReceiverTest, SetVideoMaxSendBitrate) { CreateVideoRtpSender();