From 6f6ba6edee65d0cd5174e5a58cc864902fa441b6 Mon Sep 17 00:00:00 2001 From: "henrik.lundin@webrtc.org" Date: Thu, 21 Nov 2013 13:17:29 +0000 Subject: [PATCH] Fix issues with sequence number wrap-around in jitter statistics Wrap-arounds in sequence numbers (and in timestamps) were not always treated correctly. This is fixed by introducing two helper functions for correct comparisons, and by casting to the right word size. Also added a new member variable to AutomodeInst_t. The new member keeps track of when the first packet has been registered in the automode code. This was previously done implicitly (and not very good) using the fact that the lastSeqNo and lastTimestamp members were initialized to zero. Two new unit tests were added as part of this CL. NetEqDecodingTest.SequenceNumberWrap was failing before the fixes were made; now it is ok. BUG=2654 R=tina.legrand@webrtc.org Review URL: https://webrtc-codereview.appspot.com/4139004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@5150 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/modules/audio_coding/neteq/automode.c | 26 ++++-- webrtc/modules/audio_coding/neteq/automode.h | 2 + .../neteq/webrtc_neteq_unittest.cc | 79 ++++++++++++++++++- 3 files changed, 99 insertions(+), 8 deletions(-) diff --git a/webrtc/modules/audio_coding/neteq/automode.c b/webrtc/modules/audio_coding/neteq/automode.c index a922448591..4dbd81ed66 100644 --- a/webrtc/modules/audio_coding/neteq/automode.c +++ b/webrtc/modules/audio_coding/neteq/automode.c @@ -28,6 +28,17 @@ extern FILE *delay_fid2; /* file pointer to delay log file */ #endif /* NETEQ_DELAY_LOGGING */ +// These two functions are copied from module_common_types.h, but adapted for C. +int WebRtcNetEQ_IsNewerSequenceNumber(uint16_t sequence_number, + uint16_t prev_sequence_number) { + return sequence_number != prev_sequence_number && + ((uint16_t) (sequence_number - prev_sequence_number)) < 0x8000; +} + +int WebRtcNetEQ_IsNewerTimestamp(uint32_t timestamp, uint32_t prev_timestamp) { + return timestamp != prev_timestamp && + ((uint32_t) (timestamp - prev_timestamp)) < 0x80000000; +} int WebRtcNetEQ_UpdateIatStatistics(AutomodeInst_t *inst, int maxBufLen, uint16_t seqNumber, uint32_t timeStamp, @@ -55,7 +66,8 @@ int WebRtcNetEQ_UpdateIatStatistics(AutomodeInst_t *inst, int maxBufLen, /****************************/ /* Try calculating packet length from current and previous timestamps */ - if ((timeStamp <= inst->lastTimeStamp) || (seqNumber <= inst->lastSeqNo)) + if (!WebRtcNetEQ_IsNewerTimestamp(timeStamp, inst->lastTimeStamp) || + !WebRtcNetEQ_IsNewerSequenceNumber(seqNumber, inst->lastSeqNo)) { /* Wrong timestamp or sequence order; revert to backup plan */ packetLenSamp = inst->packetSpeechLenSamp; /* use stored value */ @@ -68,7 +80,7 @@ int WebRtcNetEQ_UpdateIatStatistics(AutomodeInst_t *inst, int maxBufLen, } /* Check that the packet size is positive; if not, the statistics cannot be updated. */ - if (packetLenSamp > 0) + if (inst->firstPacketReceived && packetLenSamp > 0) { /* packet size ok */ /* calculate inter-arrival time in integer packets (rounding down) */ @@ -113,19 +125,19 @@ int WebRtcNetEQ_UpdateIatStatistics(AutomodeInst_t *inst, int maxBufLen, } /* end of streaming mode */ /* check for discontinuous packet sequence and re-ordering */ - if (seqNumber > inst->lastSeqNo + 1) + if (WebRtcNetEQ_IsNewerSequenceNumber(seqNumber, inst->lastSeqNo + 1)) { /* Compensate for gap in the sequence numbers. * Reduce IAT with expected extra time due to lost packets, but ensure that * the IAT is not negative. */ timeIat -= WEBRTC_SPL_MIN(timeIat, - (uint32_t) (seqNumber - inst->lastSeqNo - 1)); + (uint16_t) (seqNumber - (uint16_t) (inst->lastSeqNo + 1))); } - else if (seqNumber < inst->lastSeqNo) + else if (!WebRtcNetEQ_IsNewerSequenceNumber(seqNumber, inst->lastSeqNo)) { /* compensate for re-ordering */ - timeIat += (uint32_t) (inst->lastSeqNo + 1 - seqNumber); + timeIat += (uint16_t) (inst->lastSeqNo + 1 - seqNumber); } /* saturate IAT at maximum value */ @@ -316,6 +328,8 @@ int WebRtcNetEQ_UpdateIatStatistics(AutomodeInst_t *inst, int maxBufLen, inst->lastTimeStamp = timeStamp; /* remember current timestamp */ + inst->firstPacketReceived = 1; + return retval; } diff --git a/webrtc/modules/audio_coding/neteq/automode.h b/webrtc/modules/audio_coding/neteq/automode.h index 16d72e8d56..c5dd829b83 100644 --- a/webrtc/modules/audio_coding/neteq/automode.h +++ b/webrtc/modules/audio_coding/neteq/automode.h @@ -80,6 +80,8 @@ typedef struct contained special information */ uint16_t lastSeqNo; /* sequence number for last packet received */ uint32_t lastTimeStamp; /* timestamp for the last packet received */ + int firstPacketReceived; /* set to zero implicitly when the instance is + filled with zeros */ int32_t sampleMemory; /* memory position for keeping track of how many samples we cut during expand */ int16_t prevTimeScale; /* indicates that the last mode was an accelerate diff --git a/webrtc/modules/audio_coding/neteq/webrtc_neteq_unittest.cc b/webrtc/modules/audio_coding/neteq/webrtc_neteq_unittest.cc index 8f1c6baaba..c37f8990a8 100644 --- a/webrtc/modules/audio_coding/neteq/webrtc_neteq_unittest.cc +++ b/webrtc/modules/audio_coding/neteq/webrtc_neteq_unittest.cc @@ -17,6 +17,7 @@ #include #include // memset +#include #include #include #include @@ -193,6 +194,8 @@ class NetEqDecodingTest : public ::testing::Test { WebRtcNetEQ_RTPInfo* rtp_info, uint8_t* payload, int* payload_len); + void WrapTest(uint16_t start_seq_no, uint32_t start_timestamp, + const std::set& drop_seq_numbers); NETEQTEST_NetEQClass* neteq_inst_; std::vector dec_; @@ -505,7 +508,7 @@ TEST_F(NetEqDecodingTest, TestAverageInterArrivalTimeNegative) { WebRtcNetEQ_NetworkStatistics network_stats; ASSERT_EQ(0, WebRtcNetEQ_GetNetworkStatistics(neteq_inst_->instance(), &network_stats)); - EXPECT_EQ(-106911, network_stats.clockDriftPPM); + EXPECT_EQ(-103196, network_stats.clockDriftPPM); } TEST_F(NetEqDecodingTest, TestAverageInterArrivalTimePositive) { @@ -536,7 +539,7 @@ TEST_F(NetEqDecodingTest, TestAverageInterArrivalTimePositive) { WebRtcNetEQ_NetworkStatistics network_stats; ASSERT_EQ(0, WebRtcNetEQ_GetNetworkStatistics(neteq_inst_->instance(), &network_stats)); - EXPECT_EQ(108352, network_stats.clockDriftPPM); + EXPECT_EQ(110946, network_stats.clockDriftPPM); } TEST_F(NetEqDecodingTest, LongCngWithClockDrift) { @@ -700,4 +703,76 @@ TEST_F(NetEqDecodingTest, TestExtraDelay) { } } +void NetEqDecodingTest::WrapTest(uint16_t start_seq_no, + uint32_t start_timestamp, + const std::set& drop_seq_numbers) { + uint16_t seq_no = start_seq_no; + uint32_t timestamp = start_timestamp; + const int kFrameSizeMs = 30; + const int kSamples = kFrameSizeMs * 16; + const int kPayloadBytes = kSamples * 2; + double next_input_time_ms = 0.0; + + // Insert speech for 1 second. + const int kSpeechDurationMs = 1000; + for (double t_ms = 0; t_ms < kSpeechDurationMs; t_ms += 10) { + // Each turn in this for loop is 10 ms. + while (next_input_time_ms <= t_ms) { + // Insert one 30 ms speech frame. + uint8_t payload[kPayloadBytes] = {0}; + WebRtcNetEQ_RTPInfo rtp_info; + PopulateRtpInfo(seq_no, timestamp, &rtp_info); + if (drop_seq_numbers.find(seq_no) == drop_seq_numbers.end()) { + // This sequence number was not in the set to drop. Insert it. + ASSERT_EQ(0, + WebRtcNetEQ_RecInRTPStruct(neteq_inst_->instance(), + &rtp_info, + payload, + kPayloadBytes, 0)); + } + ++seq_no; + timestamp += kSamples; + next_input_time_ms += static_cast(kFrameSizeMs); + WebRtcNetEQ_NetworkStatistics network_stats; + ASSERT_EQ(0, WebRtcNetEQ_GetNetworkStatistics(neteq_inst_->instance(), + &network_stats)); + // Expect preferred and actual buffer size to be no more than 2 frames. + EXPECT_LE(network_stats.preferredBufferSize, kFrameSizeMs * 2); + EXPECT_LE(network_stats.currentBufferSize, kFrameSizeMs * 2); + } + // Pull out data once. + ASSERT_TRUE(kBlockSize16kHz == neteq_inst_->recOut(out_data_)); + // Expect delay (in samples) to be less than 2 packets. + EXPECT_LE(timestamp - neteq_inst_->getSpeechTimeStamp(), + static_cast(kSamples * 2)); + } +} + +TEST_F(NetEqDecodingTest, SequenceNumberWrap) { + // Start with a sequence number that will soon wrap. + std::set drop_seq_numbers; // Don't drop any packets. + WrapTest(0xFFFF - 5, 0, drop_seq_numbers); +} + +TEST_F(NetEqDecodingTest, SequenceNumberWrapAndDrop) { + // Start with a sequence number that will soon wrap. + std::set drop_seq_numbers; + drop_seq_numbers.insert(0xFFFF); + drop_seq_numbers.insert(0x0); + WrapTest(0xFFFF - 5, 0, drop_seq_numbers); +} + +TEST_F(NetEqDecodingTest, TimestampWrap) { + // Start with a timestamp that will soon wrap. + std::set drop_seq_numbers; + WrapTest(0, 0xFFFFFFFF - 1000, drop_seq_numbers); +} + +TEST_F(NetEqDecodingTest, TimestampAndSequenceNumberWrap) { + // Start with a timestamp and a sequence number that will wrap at the same + // time. + std::set drop_seq_numbers; + WrapTest(0xFFFF - 2, 0xFFFFFFFF - 1000, drop_seq_numbers); +} + } // namespace