From b3179c75ed409fffbaa64ecbee90db243b11efe9 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Thu, 22 Mar 2018 10:13:07 +0100 Subject: [PATCH] Remove RTPSender::SetSendPayloadType Bug: None Change-Id: Id99c9eda5e377de68c8bff053511534c66bd60a0 Reviewed-on: https://webrtc-review.googlesource.com/63801 Commit-Queue: Danil Chapovalov Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#22559} --- modules/rtp_rtcp/source/rtp_sender.cc | 17 ++--- modules/rtp_rtcp/source/rtp_sender.h | 4 +- .../rtp_rtcp/source/rtp_sender_unittest.cc | 69 +++++++++---------- 3 files changed, 40 insertions(+), 50 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index e88305a41e..b0006359d0 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -123,7 +123,7 @@ RTPSender::RTPSender( transport_(transport), sending_media_(true), // Default to sending media. max_packet_size_(IP_PACKET_SIZE - 28), // Default is IP-v4/UDP. - payload_type_(-1), + last_payload_type_(-1), payload_type_map_(), rtp_header_extension_map_(), packet_history_(clock), @@ -304,12 +304,6 @@ int32_t RTPSender::DeRegisterSendPayload(int8_t payload_type) { return 0; } -// TODO(nisse): Delete this method, only used internally and by test code. -void RTPSender::SetSendPayloadType(int8_t payload_type) { - rtc::CritScope lock(&send_critsect_); - payload_type_ = payload_type; -} - void RTPSender::SetMaxRtpPacketSize(size_t max_packet_size) { RTC_DCHECK_GE(max_packet_size, 100); RTC_DCHECK_LE(max_packet_size, IP_PACKET_SIZE); @@ -363,7 +357,7 @@ int32_t RTPSender::CheckPayloadType(int8_t payload_type, RTC_LOG(LS_ERROR) << "Invalid payload_type " << payload_type << "."; return -1; } - if (payload_type_ == payload_type) { + if (last_payload_type_ == payload_type) { if (!audio_configured_) { *video_type = video_->VideoCodecType(); } @@ -376,7 +370,6 @@ int32_t RTPSender::CheckPayloadType(int8_t payload_type, << " not registered."; return -1; } - SetSendPayloadType(payload_type); RtpUtility::Payload* payload = it->second; RTC_DCHECK(payload); if (payload->typeSpecific.is_video() && !audio_configured_) { @@ -531,7 +524,7 @@ size_t RTPSender::SendPadData(size_t bytes, timestamp = last_rtp_timestamp_; capture_time_ms = capture_time_ms_; if (rtx_ == kRtxOff) { - if (payload_type_ == -1) + if (last_payload_type_ == -1) break; // Without RTX we can't send padding in the middle of frames. // For audio marker bits doesn't mark the end of a frame and frames @@ -550,7 +543,7 @@ size_t RTPSender::SendPadData(size_t bytes, sequence_number = sequence_number_; ++sequence_number_; - payload_type = payload_type_; + payload_type = last_payload_type_; over_rtx = false; } else { // Without abs-send-time or transport sequence number a media packet @@ -1093,6 +1086,8 @@ bool RTPSender::AssignSequenceNumber(RtpPacketToSend* packet) { // Remember marker bit to determine if padding can be inserted with // sequence number following |packet|. last_packet_marker_bit_ = packet->Marker(); + // Remember payload type to use in the padding packet if rtx is disabled. + last_payload_type_ = packet->PayloadType(); // Save timestamps to generate timestamp field and extensions for the padding. last_rtp_timestamp_ = packet->Timestamp(); last_timestamp_time_ms_ = clock_->TimeInMilliseconds(); diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 50ecf1a3e1..a471c9dd69 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -81,8 +81,6 @@ class RTPSender { int32_t DeRegisterSendPayload(const int8_t payload_type); - void SetSendPayloadType(int8_t payload_type); - void SetSendingMediaStatus(bool enabled); bool SendingMedia() const; @@ -277,7 +275,7 @@ class RTPSender { size_t max_packet_size_; - int8_t payload_type_ RTC_GUARDED_BY(send_critsect_); + int8_t last_payload_type_ RTC_GUARDED_BY(send_critsect_); std::map payload_type_map_; RtpHeaderExtensionMap rtp_header_extension_map_ diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index c659662f3d..6d8de07a46 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -172,7 +172,6 @@ class RtpSenderTest : public ::testing::TestWithParam { nullptr, &seq_num_allocator_, nullptr, nullptr, nullptr, nullptr, &mock_rtc_event_log_, &send_packet_observer_, &retransmission_rate_limiter_, nullptr, populate_network2)); - rtp_sender_->SetSendPayloadType(kPayload); rtp_sender_->SetSequenceNumber(kSeqNum); rtp_sender_->SetTimestampOffset(0); rtp_sender_->SetSSRC(kSsrc); @@ -346,7 +345,7 @@ TEST_P(RtpSenderTestWithoutPacer, AssignSequenceNumberFailsOnNotSending) { EXPECT_FALSE(rtp_sender_->AssignSequenceNumber(packet.get())); } -TEST_P(RtpSenderTestWithoutPacer, AssignSequenceNumberMayAllowPadding) { +TEST_P(RtpSenderTestWithoutPacer, AssignSequenceNumberMayAllowPaddingOnVideo) { constexpr size_t kPaddingSize = 100; auto packet = rtp_sender_->AllocatePacket(); ASSERT_TRUE(packet); @@ -354,7 +353,7 @@ TEST_P(RtpSenderTestWithoutPacer, AssignSequenceNumberMayAllowPadding) { ASSERT_FALSE(rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); packet->SetMarker(false); ASSERT_TRUE(rtp_sender_->AssignSequenceNumber(packet.get())); - // Packet without marker bit doesn't allow padding. + // Packet without marker bit doesn't allow padding on video stream. EXPECT_FALSE(rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); packet->SetMarker(true); @@ -363,6 +362,37 @@ TEST_P(RtpSenderTestWithoutPacer, AssignSequenceNumberMayAllowPadding) { EXPECT_TRUE(rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); } +TEST_P(RtpSenderTest, AssignSequenceNumberAllowsPaddingOnAudio) { + MockTransport transport; + const bool kEnableAudio = true; + rtp_sender_.reset(new RTPSender( + kEnableAudio, &fake_clock_, &transport, &mock_paced_sender_, nullptr, + nullptr, nullptr, nullptr, nullptr, nullptr, &mock_rtc_event_log_, + nullptr, &retransmission_rate_limiter_, nullptr, false)); + rtp_sender_->SetTimestampOffset(0); + rtp_sender_->SetSSRC(kSsrc); + + std::unique_ptr audio_packet = rtp_sender_->AllocatePacket(); + // Padding on audio stream allowed regardless of marker in the last packet. + audio_packet->SetMarker(false); + audio_packet->SetPayloadType(kPayload); + rtp_sender_->AssignSequenceNumber(audio_packet.get()); + + const size_t kPaddingSize = 59; + EXPECT_CALL(transport, SendRtp(_, kPaddingSize + kRtpHeaderSize, _)) + .WillOnce(testing::Return(true)); + EXPECT_EQ(kPaddingSize, + rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); + + // Requested padding size is too small, will send a larger one. + const size_t kMinPaddingSize = 50; + EXPECT_CALL(transport, SendRtp(_, kMinPaddingSize + kRtpHeaderSize, _)) + .WillOnce(testing::Return(true)); + EXPECT_EQ( + kMinPaddingSize, + rtp_sender_->TimeToSendPadding(kMinPaddingSize - 5, PacedPacketInfo())); +} + TEST_P(RtpSenderTestWithoutPacer, AssignSequenceNumberSetPaddingTimestamps) { constexpr size_t kPaddingSize = 100; auto packet = rtp_sender_->AllocatePacket(); @@ -962,7 +992,6 @@ TEST_P(RtpSenderTest, SendFlexfecPackets) { &retransmission_rate_limiter_, nullptr, false)); rtp_sender_->SetSSRC(kMediaSsrc); rtp_sender_->SetSequenceNumber(kSeqNum); - rtp_sender_->SetSendPayloadType(kMediaPayloadType); rtp_sender_->SetStorePacketsStatus(true, 10); // Parameters selected to generate a single FEC packet per media packet. @@ -1022,7 +1051,6 @@ TEST_P(RtpSenderTest, NoFlexfecForTimingFrames) { &retransmission_rate_limiter_, nullptr, false)); rtp_sender_->SetSSRC(kMediaSsrc); rtp_sender_->SetSequenceNumber(kSeqNum); - rtp_sender_->SetSendPayloadType(kMediaPayloadType); rtp_sender_->SetStorePacketsStatus(true, 10); // Need extension to be registered for timing frames to be sent. @@ -1122,7 +1150,6 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { &retransmission_rate_limiter_, nullptr, false)); rtp_sender_->SetSSRC(kMediaSsrc); rtp_sender_->SetSequenceNumber(kSeqNum); - rtp_sender_->SetSendPayloadType(kMediaPayloadType); // Parameters selected to generate a single FEC packet per media packet. FecProtectionParams params; @@ -1145,7 +1172,6 @@ TEST_P(RtpSenderTestWithoutPacer, SendFlexfecPackets) { } TEST_P(RtpSenderTest, FecOverheadRate) { - constexpr int kMediaPayloadType = 127; constexpr int kFlexfecPayloadType = 118; constexpr uint32_t kMediaSsrc = 1234; constexpr uint32_t kFlexfecSsrc = 5678; @@ -1163,7 +1189,6 @@ TEST_P(RtpSenderTest, FecOverheadRate) { &retransmission_rate_limiter_, nullptr, false)); rtp_sender_->SetSSRC(kMediaSsrc); rtp_sender_->SetSequenceNumber(kSeqNum); - rtp_sender_->SetSendPayloadType(kMediaPayloadType); // Parameters selected to generate a single FEC packet per media packet. FecProtectionParams params; @@ -1963,40 +1988,12 @@ TEST_P(RtpSenderTest, DoesNotUpdateOverheadOnEqualSize) { SendGenericPayload(); } -TEST_P(RtpSenderTest, SendAudioPadding) { - MockTransport transport; - const bool kEnableAudio = true; - rtp_sender_.reset(new RTPSender( - kEnableAudio, &fake_clock_, &transport, &mock_paced_sender_, nullptr, - nullptr, nullptr, nullptr, nullptr, nullptr, &mock_rtc_event_log_, - nullptr, &retransmission_rate_limiter_, nullptr, false)); - rtp_sender_->SetSendPayloadType(kPayload); - rtp_sender_->SetSequenceNumber(kSeqNum); - rtp_sender_->SetTimestampOffset(0); - rtp_sender_->SetSSRC(kSsrc); - - const size_t kPaddingSize = 59; - EXPECT_CALL(transport, SendRtp(_, kPaddingSize + kRtpHeaderSize, _)) - .WillOnce(testing::Return(true)); - EXPECT_EQ(kPaddingSize, - rtp_sender_->TimeToSendPadding(kPaddingSize, PacedPacketInfo())); - - // Requested padding size is too small, will send a larger one. - const size_t kMinPaddingSize = 50; - EXPECT_CALL(transport, SendRtp(_, kMinPaddingSize + kRtpHeaderSize, _)) - .WillOnce(testing::Return(true)); - EXPECT_EQ( - kMinPaddingSize, - rtp_sender_->TimeToSendPadding(kMinPaddingSize - 5, PacedPacketInfo())); -} - TEST_P(RtpSenderTest, SendsKeepAlive) { MockTransport transport; rtp_sender_.reset( new RTPSender(false, &fake_clock_, &transport, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, &mock_rtc_event_log_, nullptr, &retransmission_rate_limiter_, nullptr, false)); - rtp_sender_->SetSendPayloadType(kPayload); rtp_sender_->SetSequenceNumber(kSeqNum); rtp_sender_->SetTimestampOffset(0); rtp_sender_->SetSSRC(kSsrc);