From a8c2f5180f1d61f99fee6112d1271b179980b302 Mon Sep 17 00:00:00 2001 From: Florent Castelli Date: Thu, 28 Nov 2019 15:48:24 +0100 Subject: [PATCH] Remove unused non-standard RtpEncodingParameters members Bug: webrtc:7580 Change-Id: Ic1a6e52f25eb35c797e669bffe8040ec84fec386 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/160415 Reviewed-by: Steve Anton Commit-Queue: Florent Castelli Cr-Commit-Position: refs/heads/master@{#29983} --- api/rtp_parameters.h | 52 +--------- pc/peer_connection_rtp_unittest.cc | 47 --------- pc/rtp_parameters_conversion.cc | 13 --- pc/rtp_parameters_conversion_unittest.cc | 31 ------ pc/rtp_sender.cc | 17 ---- pc/rtp_sender_receiver_unittest.cc | 123 ----------------------- 6 files changed, 3 insertions(+), 280 deletions(-) diff --git a/api/rtp_parameters.h b/api/rtp_parameters.h index 77db960c4c..124abc9685 100644 --- a/api/rtp_parameters.h +++ b/api/rtp_parameters.h @@ -380,30 +380,6 @@ struct RTC_EXPORT RtpEncodingParameters { // unset SSRC acts as a "wildcard" SSRC. absl::optional ssrc; - // Can be used to reference a codec in the |codecs| member of the - // RtpParameters that contains this RtpEncodingParameters. If unset, the - // implementation will choose the first possible codec (if a sender), or - // prepare to receive any codec (for a receiver). - // TODO(deadbeef): Not implemented. Implementation of RtpSender will always - // choose the first codec from the list. - absl::optional codec_payload_type; - - // Specifies the FEC mechanism, if set. - // TODO(deadbeef): Not implemented. Current implementation will use whatever - // FEC codecs are available, including red+ulpfec. - absl::optional fec; - - // Specifies the RTX parameters, if set. - // TODO(deadbeef): Not implemented with PeerConnection senders/receivers. - absl::optional rtx; - - // Only used for audio. If set, determines whether or not discontinuous - // transmission will be used, if an available codec supports it. If not - // set, the implementation default setting will be used. - // TODO(deadbeef): Not implemented. Current implementation will use a CN - // codec as long as it's present. - absl::optional dtx; - // The relative bitrate priority of this encoding. Currently this is // implemented for the entire rtp sender by using the value of the first // encoding parameter. @@ -421,14 +397,6 @@ struct RTC_EXPORT RtpEncodingParameters { // TODO(http://crbug.com/webrtc/8630): Implement this per encoding parameter. double network_priority = kDefaultBitratePriority; - // Indicates the preferred duration of media represented by a packet in - // milliseconds for this encoding. If set, this will take precedence over the - // ptime set in the RtpCodecParameters. This could happen if SDP negotiation - // creates a ptime for a specific codec, which is later changed in the - // RtpEncodingParameters by the application. - // TODO(bugs.webrtc.org/8819): Not implemented. - absl::optional ptime; - // If set, this represents the Transport Independent Application Specific // maximum bandwidth defined in RFC3890. If unset, there is no maximum // bitrate. Currently this is implemented for the entire rtp sender by using @@ -443,7 +411,6 @@ struct RTC_EXPORT RtpEncodingParameters { absl::optional max_bitrate_bps; // Specifies the minimum bitrate in bps for video. - // TODO(asapersson): Not implemented for ORTC API. absl::optional min_bitrate_bps; // Specifies the maximum framerate in fps for video. @@ -462,10 +429,6 @@ struct RTC_EXPORT RtpEncodingParameters { // For video, scale the resolution down by this factor. absl::optional scale_resolution_down_by; - // Scale the framerate down by this factor. - // TODO(deadbeef): Not implemented. - absl::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 // across multiple encodings of a sender for turning simulcast layers on and @@ -478,24 +441,15 @@ struct RTC_EXPORT RtpEncodingParameters { // Called "encodingId" in ORTC. std::string rid; - // RIDs of encodings on which this layer depends. - // Called "dependencyEncodingIds" in ORTC spec. - // TODO(deadbeef): Not implemented. - std::vector dependency_rids; - bool operator==(const RtpEncodingParameters& o) const { - return ssrc == o.ssrc && codec_payload_type == o.codec_payload_type && - fec == o.fec && rtx == o.rtx && dtx == o.dtx && - bitrate_priority == o.bitrate_priority && - network_priority == o.network_priority && ptime == o.ptime && + return ssrc == o.ssrc && bitrate_priority == o.bitrate_priority && + network_priority == o.network_priority && max_bitrate_bps == o.max_bitrate_bps && min_bitrate_bps == o.min_bitrate_bps && max_framerate == o.max_framerate && num_temporal_layers == o.num_temporal_layers && scale_resolution_down_by == o.scale_resolution_down_by && - scale_framerate_down_by == o.scale_framerate_down_by && - active == o.active && rid == o.rid && - dependency_rids == o.dependency_rids; + active == o.active && rid == o.rid; } bool operator!=(const RtpEncodingParameters& o) const { return !(*this == o); diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc index b70999289f..a1f50c51a2 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc @@ -1460,53 +1460,6 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, .error() .type()); init.send_encodings = default_send_encodings; - - init.send_encodings[0].codec_payload_type = 1; - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - caller->pc() - ->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init) - .error() - .type()); - init.send_encodings = default_send_encodings; - - init.send_encodings[0].fec = RtpFecParameters(); - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - caller->pc() - ->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init) - .error() - .type()); - init.send_encodings = default_send_encodings; - - init.send_encodings[0].rtx = RtpRtxParameters(); - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - caller->pc() - ->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init) - .error() - .type()); - init.send_encodings = default_send_encodings; - - init.send_encodings[0].dtx = DtxStatus::ENABLED; - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - caller->pc() - ->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init) - .error() - .type()); - init.send_encodings = default_send_encodings; - - init.send_encodings[0].ptime = 1; - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - caller->pc() - ->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init) - .error() - .type()); - init.send_encodings = default_send_encodings; - - init.send_encodings[0].dependency_rids.push_back("dummy_rid"); - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - caller->pc() - ->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init) - .error() - .type()); } // Test that AddTransceiver fails if trying to use invalid RTP encoding diff --git a/pc/rtp_parameters_conversion.cc b/pc/rtp_parameters_conversion.cc index b7fb69175c..363fa06006 100644 --- a/pc/rtp_parameters_conversion.cc +++ b/pc/rtp_parameters_conversion.cc @@ -234,17 +234,9 @@ RTCErrorOr ToCricketStreamParamsVec( } cricket::StreamParamsVec cricket_streams; const RtpEncodingParameters& encoding = encodings[0]; - if (encoding.rtx && encoding.rtx->ssrc && !encoding.ssrc) { - LOG_AND_RETURN_ERROR(RTCErrorType::UNSUPPORTED_PARAMETER, - "Setting an RTX SSRC explicitly while leaving the " - "primary SSRC unset is not currently supported."); - } if (encoding.ssrc) { cricket::StreamParams stream_params; stream_params.add_ssrc(*encoding.ssrc); - if (encoding.rtx && encoding.rtx->ssrc) { - stream_params.AddFidSsrc(*encoding.ssrc, *encoding.rtx->ssrc); - } cricket_streams.push_back(std::move(stream_params)); } return std::move(cricket_streams); @@ -308,11 +300,6 @@ std::vector ToRtpEncodings( for (const cricket::StreamParams& stream_param : stream_params) { RtpEncodingParameters rtp_encoding; rtp_encoding.ssrc.emplace(stream_param.first_ssrc()); - uint32_t rtx_ssrc = 0; - if (stream_param.GetFidSsrc(stream_param.first_ssrc(), &rtx_ssrc)) { - RtpRtxParameters rtx_param(rtx_ssrc); - rtp_encoding.rtx.emplace(rtx_param); - } rtp_encodings.push_back(std::move(rtp_encoding)); } return rtp_encodings; diff --git a/pc/rtp_parameters_conversion_unittest.cc b/pc/rtp_parameters_conversion_unittest.cc index 3d64d62d78..44dc0df18e 100644 --- a/pc/rtp_parameters_conversion_unittest.cc +++ b/pc/rtp_parameters_conversion_unittest.cc @@ -346,23 +346,6 @@ TEST(RtpParametersConversionTest, ToCricketStreamParamsVecSimple) { EXPECT_EQ(0xbaadf00d, result.value()[0].first_ssrc()); } -TEST(RtpParametersConversionTest, ToCricketStreamParamsVecWithRtx) { - std::vector encodings; - RtpEncodingParameters encoding; - // Test a corner case SSRC of 0. - encoding.ssrc.emplace(0u); - encoding.rtx.emplace(0xdeadbeef); - encodings.push_back(encoding); - auto result = ToCricketStreamParamsVec(encodings); - ASSERT_TRUE(result.ok()); - ASSERT_EQ(1u, result.value().size()); - EXPECT_EQ(2u, result.value()[0].ssrcs.size()); - EXPECT_EQ(0u, result.value()[0].first_ssrc()); - uint32_t rtx_ssrc = 0; - EXPECT_TRUE(result.value()[0].GetFidSsrc(0u, &rtx_ssrc)); - EXPECT_EQ(0xdeadbeef, rtx_ssrc); -} - // No encodings should be accepted; an endpoint may want to prepare a // decoder/encoder without having something to receive/send yet. TEST(RtpParametersConversionTest, ToCricketStreamParamsVecNoEncodings) { @@ -377,21 +360,11 @@ TEST(RtpParametersConversionTest, ToCricketStreamParamsVecNoEncodings) { TEST(RtpParametersConversionTest, ToCricketStreamParamsVecMissingSsrcs) { std::vector encodings = {{}}; // Creates RtxParameters with empty SSRC. - encodings[0].rtx.emplace(); auto result = ToCricketStreamParamsVec(encodings); ASSERT_TRUE(result.ok()); EXPECT_EQ(0u, result.value().size()); } -// The media engine doesn't have a way of receiving an RTX SSRC that's known -// with a primary SSRC that's unknown, so this should produce an error. -TEST(RtpParametersConversionTest, ToStreamParamsWithPrimarySsrcSetAndRtxUnset) { - std::vector encodings = {{}}; - encodings[0].rtx.emplace(0xdeadbeef); - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - ToCricketStreamParamsVec(encodings).error().type()); -} - // TODO(deadbeef): Update this test when we support multiple encodings. TEST(RtpParametersConversionTest, ToCricketStreamParamsVecMultipleEncodings) { std::vector encodings = {{}, {}}; @@ -511,11 +484,9 @@ TEST(RtpParametersConversionTest, ToRtpEncodingsWithMultipleStreamParams) { cricket::StreamParamsVec streams; cricket::StreamParams stream1; stream1.ssrcs.push_back(1111u); - stream1.AddFidSsrc(1111u, 0xaaaaaaaa); cricket::StreamParams stream2; stream2.ssrcs.push_back(2222u); - stream2.AddFidSsrc(2222u, 0xaaaaaaab); streams.push_back(stream1); streams.push_back(stream2); @@ -523,9 +494,7 @@ TEST(RtpParametersConversionTest, ToRtpEncodingsWithMultipleStreamParams) { auto rtp_encodings = ToRtpEncodings(streams); ASSERT_EQ(2u, rtp_encodings.size()); EXPECT_EQ(1111u, rtp_encodings[0].ssrc); - EXPECT_EQ(0xaaaaaaaa, rtp_encodings[0].rtx->ssrc); EXPECT_EQ(2222u, rtp_encodings[1].ssrc); - EXPECT_EQ(0xaaaaaaab, rtp_encodings[1].rtx->ssrc); } TEST(RtpParametersConversionTest, ToAudioRtpCodecParameters) { diff --git a/pc/rtp_sender.cc b/pc/rtp_sender.cc index 9eaed311a7..402ad97920 100644 --- a/pc/rtp_sender.cc +++ b/pc/rtp_sender.cc @@ -38,20 +38,6 @@ 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.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 @@ -109,9 +95,6 @@ bool UnimplementedRtpParameterHasValue(const RtpParameters& parameters) { 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 && diff --git a/pc/rtp_sender_receiver_unittest.cc b/pc/rtp_sender_receiver_unittest.cc index 9026cfc201..b9c07ef651 100644 --- a/pc/rtp_sender_receiver_unittest.cc +++ b/pc/rtp_sender_receiver_unittest.cc @@ -968,46 +968,6 @@ TEST_F(RtpSenderReceiverTest, AudioSenderCantSetUnimplementedRtpParameters) { 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, - // scale_framerate_down_by, 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].dependency_rids.push_back("dummy_rid"); - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - audio_rtp_sender_->SetParameters(params).type()); - - DestroyAudioRtpSender(); -} - TEST_F(RtpSenderReceiverTest, SetAudioMaxSendBitrate) { CreateAudioRtpSender(); @@ -1245,46 +1205,6 @@ TEST_F(RtpSenderReceiverTest, VideoSenderCantSetUnimplementedRtpParameters) { 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, - // scale_framerate_down_by, 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].dependency_rids.push_back("dummy_rid"); - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - video_rtp_sender_->SetParameters(params).type()); - - DestroyVideoRtpSender(); -} - TEST_F(RtpSenderReceiverTest, VideoSenderCanSetScaleResolutionDownBy) { CreateVideoRtpSender(); @@ -1309,49 +1229,6 @@ TEST_F(RtpSenderReceiverTest, VideoSenderDetectInvalidScaleResolutionDownBy) { DestroyVideoRtpSender(); } -TEST_F(RtpSenderReceiverTest, - VideoSenderCantSetUnimplementedEncodingParametersWithSimulcast) { - CreateVideoRtpSenderWithSimulcast(); - RtpParameters params = video_rtp_sender_->GetParameters(); - EXPECT_EQ(kVideoSimulcastLayerCount, params.encodings.size()); - - // Unimplemented RtpParameters: codec_payload_type, fec, rtx, dtx, ptime, - // scale_framerate_down_by, dependency_rids. - for (size_t i = 0; i < params.encodings.size(); i++) { - params.encodings[i].codec_payload_type = 1; - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - video_rtp_sender_->SetParameters(params).type()); - params = video_rtp_sender_->GetParameters(); - - params.encodings[i].fec = RtpFecParameters(); - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - video_rtp_sender_->SetParameters(params).type()); - params = video_rtp_sender_->GetParameters(); - - params.encodings[i].rtx = RtpRtxParameters(); - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - video_rtp_sender_->SetParameters(params).type()); - params = video_rtp_sender_->GetParameters(); - - params.encodings[i].dtx = DtxStatus::ENABLED; - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - video_rtp_sender_->SetParameters(params).type()); - params = video_rtp_sender_->GetParameters(); - - params.encodings[i].ptime = 1; - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - video_rtp_sender_->SetParameters(params).type()); - params = video_rtp_sender_->GetParameters(); - - params.encodings[i].dependency_rids.push_back("dummy_rid"); - EXPECT_EQ(RTCErrorType::UNSUPPORTED_PARAMETER, - video_rtp_sender_->SetParameters(params).type()); - params = video_rtp_sender_->GetParameters(); - } - - 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