admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix bug where transport sequence numbers are allocated for packets without the header extension registered.

Browse Source 
This is an issue if the sequence numbers are to be used to compute packet loss statistics since it introduces gaps which are not related to loss.

Also making sure that the header extensions are properly guarded by the send crit sect.

Review-Url: https://codereview.webrtc.org/2190913002
Cr-Commit-Position: refs/heads/master@{#13557}
This commit is contained in:
stefan 2016-07-28 07:56:38 -07:00 committed by Commit bot
parent 5deaaaa0b4
commit a23fc626a2
4 changed files with 80 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
webrtc/modules/rtp_rtcp/source/rtp_sender.cc
View File

@ -591,9 +591,9 @@ size_t RTPSender::SendPadData(size_t bytes,
size_t padding_bytes_in_packet =
std::min(MaxDataPayloadLength(), kMaxPaddingLength);
size_t bytes_sent = 0;
bool using_transport_seq = rtp_header_extension_map_.IsRegistered(
kRtpExtensionTransportSequenceNumber) &&
transport_sequence_number_allocator_;
bool using_transport_seq =
IsRtpHeaderExtensionRegistered(kRtpExtensionTransportSequenceNumber) &&
transport_sequence_number_allocator_;
for (; bytes > 0; bytes -= padding_bytes_in_packet) {
if (bytes < padding_bytes_in_packet)
bytes = padding_bytes_in_packet;
@ -647,9 +647,13 @@ size_t RTPSender::SendPadData(size_t bytes,
}
uint8_t padding_packet[IP_PACKET_SIZE];
size_t header_length =
CreateRtpHeader(padding_packet, payload_type, ssrc, false, timestamp,
sequence_number, std::vector<uint32_t>());
size_t header_length = 0;
{
rtc::CritScope lock(&send_critsect_);
header_length =
CreateRtpHeader(padding_packet, payload_type, ssrc, false, timestamp,
sequence_number, std::vector<uint32_t>());
}
BuildPaddingPacket(padding_packet, header_length, padding_bytes_in_packet);
size_t length = padding_bytes_in_packet + header_length;
int64_t now_ms = clock_->TimeInMilliseconds();
@ -666,13 +670,11 @@ size_t RTPSender::SendPadData(size_t bytes,
UpdateAbsoluteSendTime(padding_packet, length, rtp_header, now_ms);
PacketOptions options;
if (AllocateTransportSequenceNumber(&options.packet_id)) {
if (UpdateTransportSequenceNumber(options.packet_id, padding_packet,
length, rtp_header)) {
if (transport_feedback_observer_)
transport_feedback_observer_->AddPacket(options.packet_id, length,
probe_cluster_id);
}
if (UpdateTransportSequenceNumber(padding_packet, length, rtp_header,
&options.packet_id)) {
if (transport_feedback_observer_)
transport_feedback_observer_->AddPacket(options.packet_id, length,
probe_cluster_id);
}
if (!SendPacketToNetwork(padding_packet, length, options))
@ -859,13 +861,11 @@ bool RTPSender::PrepareAndSendPacket(uint8_t* buffer,
UpdateAbsoluteSendTime(buffer_to_send_ptr, length, rtp_header, now_ms);
PacketOptions options;
if (AllocateTransportSequenceNumber(&options.packet_id)) {
if (UpdateTransportSequenceNumber(options.packet_id, buffer_to_send_ptr,
length, rtp_header)) {
if (transport_feedback_observer_)
transport_feedback_observer_->AddPacket(options.packet_id, length,
probe_cluster_id);
}
if (UpdateTransportSequenceNumber(buffer_to_send_ptr, length, rtp_header,
&options.packet_id)) {
if (transport_feedback_observer_)
transport_feedback_observer_->AddPacket(options.packet_id, length,
probe_cluster_id);
}
if (!is_retransmit && !send_over_rtx) {
@ -992,13 +992,11 @@ int32_t RTPSender::SendToNetwork(uint8_t* buffer,
}
PacketOptions options;
if (AllocateTransportSequenceNumber(&options.packet_id)) {
if (UpdateTransportSequenceNumber(options.packet_id, buffer, length,
rtp_header)) {
if (transport_feedback_observer_)
transport_feedback_observer_->AddPacket(options.packet_id, length,
PacketInfo::kNotAProbe);
}
if (UpdateTransportSequenceNumber(buffer, length, rtp_header,
&options.packet_id)) {
if (transport_feedback_observer_)
transport_feedback_observer_->AddPacket(options.packet_id, length,
PacketInfo::kNotAProbe);
}
UpdateDelayStatistics(capture_time_ms, now_ms);
UpdateOnSendPacket(options.packet_id, capture_time_ms, rtp_header.ssrc);
@ -1587,11 +1585,11 @@ void RTPSender::UpdateAbsoluteSendTime(uint8_t* rtp_packet,
ConvertMsTo24Bits(now_ms));
}
bool RTPSender::UpdateTransportSequenceNumber(
uint16_t sequence_number,
uint8_t* rtp_packet,
size_t rtp_packet_length,
const RTPHeader& rtp_header) const {
bool RTPSender::UpdateTransportSequenceNumber(uint8_t* rtp_packet,
size_t rtp_packet_length,
const RTPHeader& rtp_header,
int* sequence_number) const {
RTC_DCHECK(sequence_number);
size_t offset;
rtc::CritScope lock(&send_critsect_);
@ -1609,7 +1607,10 @@ bool RTPSender::UpdateTransportSequenceNumber(
RTC_NOTREACHED();
}
BuildTransportSequenceNumberExtension(rtp_packet + offset, sequence_number);
if (!AllocateTransportSequenceNumber(sequence_number))
return false;
BuildTransportSequenceNumberExtension(rtp_packet + offset, *sequence_number);
return true;
}

35
webrtc/modules/rtp_rtcp/source/rtp_sender.h
View File

@ -166,17 +166,24 @@ class RTPSender : public RTPSenderInterface {
size_t RtpHeaderExtensionLength() const;
uint16_t BuildRTPHeaderExtension(uint8_t* data_buffer, bool marker_bit) const;
uint16_t BuildRTPHeaderExtension(uint8_t* data_buffer, bool marker_bit) const
EXCLUSIVE_LOCKS_REQUIRED(send_critsect_);
uint8_t BuildTransmissionTimeOffsetExtension(uint8_t *data_buffer) const;
uint8_t BuildAudioLevelExtension(uint8_t* data_buffer) const;
uint8_t BuildAbsoluteSendTimeExtension(uint8_t* data_buffer) const;
uint8_t BuildVideoRotationExtension(uint8_t* data_buffer) const;
uint8_t BuildTransmissionTimeOffsetExtension(uint8_t* data_buffer) const
EXCLUSIVE_LOCKS_REQUIRED(send_critsect_);
uint8_t BuildAudioLevelExtension(uint8_t* data_buffer) const
EXCLUSIVE_LOCKS_REQUIRED(send_critsect_);
uint8_t BuildAbsoluteSendTimeExtension(uint8_t* data_buffer) const
EXCLUSIVE_LOCKS_REQUIRED(send_critsect_);
uint8_t BuildVideoRotationExtension(uint8_t* data_buffer) const
EXCLUSIVE_LOCKS_REQUIRED(send_critsect_);
uint8_t BuildTransportSequenceNumberExtension(uint8_t* data_buffer,
uint16_t sequence_number) const;
uint16_t sequence_number) const
EXCLUSIVE_LOCKS_REQUIRED(send_critsect_);
uint8_t BuildPlayoutDelayExtension(uint8_t* data_buffer,
uint16_t min_playout_delay_ms,
uint16_t max_playout_delay_ms) const;
uint16_t max_playout_delay_ms) const
EXCLUSIVE_LOCKS_REQUIRED(send_critsect_);
// Verifies that the specified extension is registered, and that it is
// present in rtp packet. If extension is not registered kNotRegistered is
@ -335,7 +342,8 @@ class RTPSender : public RTPSenderInterface {
bool marker_bit,
uint32_t timestamp,
uint16_t sequence_number,
const std::vector<uint32_t>& csrcs) const;
const std::vector<uint32_t>& csrcs) const
EXCLUSIVE_LOCKS_REQUIRED(send_critsect_);
bool PrepareAndSendPacket(uint8_t* buffer,
size_t length,
@ -370,7 +378,8 @@ class RTPSender : public RTPSenderInterface {
const uint8_t* rtp_packet,
size_t rtp_packet_length,
const RTPHeader& rtp_header,
size_t* position) const;
size_t* position) const
EXCLUSIVE_LOCKS_REQUIRED(send_critsect_);
void UpdateTransmissionTimeOffset(uint8_t* rtp_packet,
size_t rtp_packet_length,
@ -381,10 +390,10 @@ class RTPSender : public RTPSenderInterface {
const RTPHeader& rtp_header,
int64_t now_ms) const;
bool UpdateTransportSequenceNumber(uint16_t sequence_number,
uint8_t* rtp_packet,
bool UpdateTransportSequenceNumber(uint8_t* rtp_packet,
size_t rtp_packet_length,
const RTPHeader& rtp_header) const;
const RTPHeader& rtp_header,
int* sequence_number) const;
void UpdatePlayoutDelayLimits(uint8_t* rtp_packet,
size_t rtp_packet_length,
@ -423,7 +432,7 @@ class RTPSender : public RTPSenderInterface {
int8_t payload_type_ GUARDED_BY(send_critsect_);
std::map<int8_t, RtpUtility::Payload*> payload_type_map_;
RtpHeaderExtensionMap rtp_header_extension_map_;
RtpHeaderExtensionMap rtp_header_extension_map_ GUARDED_BY(send_critsect_);
int32_t transmission_time_offset_;
uint32_t absolute_send_time_;
VideoRotation rotation_;

33
webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc
View File

@ -152,10 +152,10 @@ class RtpSenderTest : public ::testing::Test {
}
SimulatedClock fake_clock_;
MockRtcEventLog mock_rtc_event_log_;
testing::NiceMock<MockRtcEventLog> mock_rtc_event_log_;
MockRtpPacketSender mock_paced_sender_;
MockTransportSequenceNumberAllocator seq_num_allocator_;
MockSendPacketObserver send_packet_observer_;
testing::StrictMock<MockTransportSequenceNumberAllocator> seq_num_allocator_;
testing::StrictMock<MockSendPacketObserver> send_packet_observer_;
RateLimiter retransmission_rate_limiter_;
std::unique_ptr<RTPSender> rtp_sender_;
int payload_;
@ -491,10 +491,6 @@ TEST_F(RtpSenderTestWithoutPacer, BuildRTPPacketWithAbsoluteSendTimeExtension) {
}
TEST_F(RtpSenderTestWithoutPacer, SendsPacketsWithTransportSequenceNumber) {
// Ignore rtc event calls.
EXPECT_CALL(mock_rtc_event_log_,
LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _));
EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
kRtpExtensionTransportSequenceNumber,
kTransportSequenceNumberExtensionId));
@ -521,10 +517,14 @@ TEST_F(RtpSenderTestWithoutPacer, SendsPacketsWithTransportSequenceNumber) {
rtp_header.extension.transportSequenceNumber);
}
TEST_F(RtpSenderTestWithoutPacer, OnSendPacketUpdated) {
EXPECT_CALL(mock_rtc_event_log_, // Ignore rtc event calls.
LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _));
TEST_F(RtpSenderTestWithoutPacer, NoAllocationIfNotRegistered) {
SendGenericPayload();
}
TEST_F(RtpSenderTestWithoutPacer, OnSendPacketUpdated) {
EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
kRtpExtensionTransportSequenceNumber,
kTransportSequenceNumberExtensionId));
EXPECT_CALL(seq_num_allocator_, AllocateSequenceNumber())
.WillOnce(testing::Return(kTransportSequenceNumber));
EXPECT_CALL(send_packet_observer_,
@ -972,8 +972,9 @@ TEST_F(RtpSenderTest, SendPadding) {
}
TEST_F(RtpSenderTest, OnSendPacketUpdated) {
EXPECT_CALL(mock_rtc_event_log_, // Ignore rtc event calls.
LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _));
EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
kRtpExtensionTransportSequenceNumber,
kTransportSequenceNumberExtensionId));
rtp_sender_->SetStorePacketsStatus(true, 10);
EXPECT_CALL(send_packet_observer_,
@ -991,8 +992,9 @@ TEST_F(RtpSenderTest, OnSendPacketUpdated) {
}
TEST_F(RtpSenderTest, OnSendPacketNotUpdatedForRetransmits) {
EXPECT_CALL(mock_rtc_event_log_, // Ignore rtc event calls.
LogRtpHeader(PacketDirection::kOutgoingPacket, _, _, _));
EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
kRtpExtensionTransportSequenceNumber,
kTransportSequenceNumberExtensionId));
rtp_sender_->SetStorePacketsStatus(true, 10);
EXPECT_CALL(send_packet_observer_, OnSendPacket(_, _, _)).Times(0);
@ -1012,6 +1014,9 @@ TEST_F(RtpSenderTest, OnSendPacketNotUpdatedWithoutSeqNumAllocator) {
false, &fake_clock_, &transport_, &mock_paced_sender_,
nullptr /* TransportSequenceNumberAllocator */, nullptr, nullptr, nullptr,
nullptr, nullptr, &send_packet_observer_, nullptr));
EXPECT_EQ(0, rtp_sender_->RegisterRtpHeaderExtension(
kRtpExtensionTransportSequenceNumber,
kTransportSequenceNumberExtensionId));
rtp_sender_->SetSequenceNumber(kSeqNum);
rtp_sender_->SetStorePacketsStatus(true, 10);

5
webrtc/video/end_to_end_tests.cc
View File

@ -2077,6 +2077,11 @@ void EndToEndTest::VerifyHistogramStats(bool use_rtx,
VideoSendStream::Config* send_config,
std::vector<VideoReceiveStream::Config>* receive_configs,
VideoEncoderConfig* encoder_config) override {
static const int kExtensionId = 8;
send_config->rtp.extensions.push_back(RtpExtension(
RtpExtension::kTransportSequenceNumberUri, kExtensionId));
(*receive_configs)[0].rtp.extensions.push_back(RtpExtension(
RtpExtension::kTransportSequenceNumberUri, kExtensionId));
// NACK
send_config->rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
(*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;