From 8dad9b414a6b179cdced1557f182c696c34fbf04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Wed, 22 Aug 2018 10:36:35 +0200 Subject: [PATCH] Eliminate NackModule dependency on VCMPacket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: None Change-Id: I1d4ecac123c888f2315aeb2f717ee756a472036e Reviewed-on: https://webrtc-review.googlesource.com/95420 Reviewed-by: Philip Eliasson Reviewed-by: Erik Språng Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#24387} --- modules/video_coding/nack_module.cc | 6 - modules/video_coding/nack_module.h | 2 - modules/video_coding/nack_module_unittest.cc | 124 ++++++------------- video/rtp_video_stream_receiver.cc | 13 +- 4 files changed, 47 insertions(+), 98 deletions(-) diff --git a/modules/video_coding/nack_module.cc b/modules/video_coding/nack_module.cc index ab66b2b562..2399606d79 100644 --- a/modules/video_coding/nack_module.cc +++ b/modules/video_coding/nack_module.cc @@ -108,12 +108,6 @@ int NackModule::OnReceivedPacket(uint16_t seq_num, bool is_keyframe) { return 0; } -int NackModule::OnReceivedPacket(const VCMPacket& packet) { - return OnReceivedPacket( - packet.seqNum, - packet.is_first_packet_in_frame && packet.frameType == kVideoFrameKey); -} - void NackModule::ClearUpTo(uint16_t seq_num) { rtc::CritScope lock(&crit_); nack_list_.erase(nack_list_.begin(), nack_list_.lower_bound(seq_num)); diff --git a/modules/video_coding/nack_module.h b/modules/video_coding/nack_module.h index fc2f2a7977..44340e9d4c 100644 --- a/modules/video_coding/nack_module.h +++ b/modules/video_coding/nack_module.h @@ -18,7 +18,6 @@ #include "modules/include/module.h" #include "modules/include/module_common_types.h" #include "modules/video_coding/histogram.h" -#include "modules/video_coding/packet.h" #include "rtc_base/criticalsection.h" #include "rtc_base/numerics/sequence_number_util.h" #include "rtc_base/thread_annotations.h" @@ -33,7 +32,6 @@ class NackModule : public Module { KeyFrameRequestSender* keyframe_request_sender); int OnReceivedPacket(uint16_t seq_num, bool is_keyframe); - int OnReceivedPacket(const VCMPacket& packet); void ClearUpTo(uint16_t seq_num); void UpdateRtt(int64_t rtt_ms); void Clear(); diff --git a/modules/video_coding/nack_module_unittest.cc b/modules/video_coding/nack_module_unittest.cc index b8b1355756..bcca351d63 100644 --- a/modules/video_coding/nack_module_unittest.cc +++ b/modules/video_coding/nack_module_unittest.cc @@ -40,53 +40,38 @@ class TestNackModule : public ::testing::Test, }; TEST_F(TestNackModule, NackOnePacket) { - VCMPacket packet; - packet.seqNum = 1; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 3; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1, false); + nack_module_.OnReceivedPacket(3, false); EXPECT_EQ(1u, sent_nacks_.size()); EXPECT_EQ(2, sent_nacks_[0]); } TEST_F(TestNackModule, WrappingSeqNum) { - VCMPacket packet; - packet.seqNum = 0xfffe; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 1; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(0xfffe, false); + nack_module_.OnReceivedPacket(1, false); EXPECT_EQ(2u, sent_nacks_.size()); EXPECT_EQ(0xffff, sent_nacks_[0]); EXPECT_EQ(0, sent_nacks_[1]); } TEST_F(TestNackModule, WrappingSeqNumClearToKeyframe) { - VCMPacket packet; - packet.seqNum = 0xfffe; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 1; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(0xfffe, false); + nack_module_.OnReceivedPacket(1, false); EXPECT_EQ(2u, sent_nacks_.size()); EXPECT_EQ(0xffff, sent_nacks_[0]); EXPECT_EQ(0, sent_nacks_[1]); sent_nacks_.clear(); - packet.frameType = kVideoFrameKey; - packet.is_first_packet_in_frame = true; - packet.seqNum = 2; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(2, true); EXPECT_EQ(0u, sent_nacks_.size()); - packet.seqNum = 501; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(501, true); EXPECT_EQ(498u, sent_nacks_.size()); for (int seq_num = 3; seq_num < 501; ++seq_num) EXPECT_EQ(seq_num, sent_nacks_[seq_num - 3]); sent_nacks_.clear(); - packet.frameType = kVideoFrameDelta; - packet.seqNum = 1001; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1001, false); EXPECT_EQ(499u, sent_nacks_.size()); for (int seq_num = 502; seq_num < 1001; ++seq_num) EXPECT_EQ(seq_num, sent_nacks_[seq_num - 502]); @@ -106,8 +91,7 @@ TEST_F(TestNackModule, WrappingSeqNumClearToKeyframe) { // It will then clear all nacks up to the next keyframe (seq num 2), // thus removing 0xffff and 0 from the nack list. sent_nacks_.clear(); - packet.seqNum = 1004; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1004, false); EXPECT_EQ(2u, sent_nacks_.size()); EXPECT_EQ(1002, sent_nacks_[0]); EXPECT_EQ(1003, sent_nacks_[1]); @@ -123,8 +107,7 @@ TEST_F(TestNackModule, WrappingSeqNumClearToKeyframe) { // Adding packet 1007 will cause the nack module to overflow again, thus // clearing everything up to 501 which is the next keyframe. - packet.seqNum = 1007; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1007, false); sent_nacks_.clear(); clock_->AdvanceTimeMilliseconds(100); nack_module_.Process(); @@ -163,11 +146,8 @@ TEST_F(TestNackModule, DontBurstOnTimeSkip) { } TEST_F(TestNackModule, ResendNack) { - VCMPacket packet; - packet.seqNum = 1; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 3; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1, false); + nack_module_.OnReceivedPacket(3, false); EXPECT_EQ(1u, sent_nacks_.size()); EXPECT_EQ(2, sent_nacks_[0]); @@ -189,19 +169,15 @@ TEST_F(TestNackModule, ResendNack) { nack_module_.Process(); EXPECT_EQ(4u, sent_nacks_.size()); - packet.seqNum = 2; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(2, false); clock_->AdvanceTimeMilliseconds(50); nack_module_.Process(); EXPECT_EQ(4u, sent_nacks_.size()); } TEST_F(TestNackModule, ResendPacketMaxRetries) { - VCMPacket packet; - packet.seqNum = 1; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 3; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1, false); + nack_module_.OnReceivedPacket(3, false); EXPECT_EQ(1u, sent_nacks_.size()); EXPECT_EQ(2, sent_nacks_[0]); @@ -217,53 +193,35 @@ TEST_F(TestNackModule, ResendPacketMaxRetries) { } TEST_F(TestNackModule, TooLargeNackList) { - VCMPacket packet; - packet.seqNum = 0; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 1001; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(0, false); + nack_module_.OnReceivedPacket(1001, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(0, keyframes_requested_); - packet.seqNum = 1003; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1003, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(1, keyframes_requested_); - packet.seqNum = 1004; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1004, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(1, keyframes_requested_); } TEST_F(TestNackModule, TooLargeNackListWithKeyFrame) { - VCMPacket packet; - packet.seqNum = 0; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 1; - packet.is_first_packet_in_frame = true; - packet.frameType = kVideoFrameKey; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 1001; - packet.is_first_packet_in_frame = false; - packet.frameType = kVideoFrameKey; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(0, false); + nack_module_.OnReceivedPacket(1, true); + nack_module_.OnReceivedPacket(1001, false); EXPECT_EQ(999u, sent_nacks_.size()); EXPECT_EQ(0, keyframes_requested_); - packet.seqNum = 1003; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1003, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(0, keyframes_requested_); - packet.seqNum = 1005; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(1005, false); EXPECT_EQ(1000u, sent_nacks_.size()); EXPECT_EQ(1, keyframes_requested_); } TEST_F(TestNackModule, ClearUpTo) { - VCMPacket packet; - packet.seqNum = 0; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 100; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(0, false); + nack_module_.OnReceivedPacket(100, false); EXPECT_EQ(99u, sent_nacks_.size()); sent_nacks_.clear(); @@ -275,11 +233,8 @@ TEST_F(TestNackModule, ClearUpTo) { } TEST_F(TestNackModule, ClearUpToWrap) { - VCMPacket packet; - packet.seqNum = 0xfff0; - nack_module_.OnReceivedPacket(packet); - packet.seqNum = 0xf; - nack_module_.OnReceivedPacket(packet); + nack_module_.OnReceivedPacket(0xfff0, false); + nack_module_.OnReceivedPacket(0xf, false); EXPECT_EQ(30u, sent_nacks_.size()); sent_nacks_.clear(); @@ -291,27 +246,20 @@ TEST_F(TestNackModule, ClearUpToWrap) { } TEST_F(TestNackModule, PacketNackCount) { - VCMPacket packet; - packet.seqNum = 0; - EXPECT_EQ(0, nack_module_.OnReceivedPacket(packet)); - packet.seqNum = 2; - EXPECT_EQ(0, nack_module_.OnReceivedPacket(packet)); - packet.seqNum = 1; - EXPECT_EQ(1, nack_module_.OnReceivedPacket(packet)); + EXPECT_EQ(0, nack_module_.OnReceivedPacket(0, false)); + EXPECT_EQ(0, nack_module_.OnReceivedPacket(2, false)); + EXPECT_EQ(1, nack_module_.OnReceivedPacket(1, false)); sent_nacks_.clear(); nack_module_.UpdateRtt(100); - packet.seqNum = 5; - EXPECT_EQ(0, nack_module_.OnReceivedPacket(packet)); + EXPECT_EQ(0, nack_module_.OnReceivedPacket(5, false)); clock_->AdvanceTimeMilliseconds(100); nack_module_.Process(); clock_->AdvanceTimeMilliseconds(100); nack_module_.Process(); - packet.seqNum = 3; - EXPECT_EQ(3, nack_module_.OnReceivedPacket(packet)); - packet.seqNum = 4; - EXPECT_EQ(3, nack_module_.OnReceivedPacket(packet)); - EXPECT_EQ(0, nack_module_.OnReceivedPacket(packet)); + EXPECT_EQ(3, nack_module_.OnReceivedPacket(3, false)); + EXPECT_EQ(3, nack_module_.OnReceivedPacket(4, false)); + EXPECT_EQ(0, nack_module_.OnReceivedPacket(4, false)); } } // namespace webrtc diff --git a/video/rtp_video_stream_receiver.cc b/video/rtp_video_stream_receiver.cc index 37d95e3fd9..b2ec0c701e 100644 --- a/video/rtp_video_stream_receiver.cc +++ b/video/rtp_video_stream_receiver.cc @@ -199,9 +199,18 @@ int32_t RtpVideoStreamReceiver::OnReceivedPayloadData( WebRtcRTPHeader rtp_header_with_ntp = *rtp_header; rtp_header_with_ntp.ntp_time_ms = ntp_estimator_.Estimate(rtp_header->header.timestamp); + VCMPacket packet(payload_data, payload_size, rtp_header_with_ntp); - packet.timesNacked = - nack_module_ ? nack_module_->OnReceivedPacket(packet) : -1; + if (nack_module_) { + const bool is_keyframe = + rtp_header->video_header().is_first_packet_in_frame && + rtp_header->frameType == kVideoFrameKey; + + packet.timesNacked = nack_module_->OnReceivedPacket( + rtp_header->header.sequenceNumber, is_keyframe); + } else { + packet.timesNacked = -1; + } packet.receive_time_ms = clock_->TimeInMilliseconds(); if (packet.sizeBytes == 0) {