From 7dae8c5a46db68fb950132b3a6ba49e2e029a33e Mon Sep 17 00:00:00 2001 From: Andrea Date: Mon, 18 Dec 2017 12:27:22 +0100 Subject: [PATCH 1/2] added new nack generator --- .../rtp/RtcpFeedbackGenerationHandler.cpp | 22 +- .../erizo/rtp/RtcpFeedbackGenerationHandler.h | 10 +- erizo/src/erizo/rtp/RtcpNewNackGenerator.cpp | 189 +++++++++++++++ erizo/src/erizo/rtp/RtcpNewNackGenerator.h | 93 +++++++ .../rtp/RtcpFeedbackGenerationHandlerTest.cpp | 2 +- .../src/test/rtp/RtcpNewNackGeneratorTest.cpp | 229 ++++++++++++++++++ .../erizoAgent/log4cxx.properties | 1 + spine/log4cxx.properties | 1 + 8 files changed, 539 insertions(+), 8 deletions(-) create mode 100644 erizo/src/erizo/rtp/RtcpNewNackGenerator.cpp create mode 100644 erizo/src/erizo/rtp/RtcpNewNackGenerator.h create mode 100644 erizo/src/test/rtp/RtcpNewNackGeneratorTest.cpp diff --git a/erizo/src/erizo/rtp/RtcpFeedbackGenerationHandler.cpp b/erizo/src/erizo/rtp/RtcpFeedbackGenerationHandler.cpp index 23d695e569..98bf0aadfc 100644 --- a/erizo/src/erizo/rtp/RtcpFeedbackGenerationHandler.cpp +++ b/erizo/src/erizo/rtp/RtcpFeedbackGenerationHandler.cpp @@ -1,13 +1,15 @@ #include "rtp/RtcpFeedbackGenerationHandler.h" #include "./MediaStream.h" +#include "rtp/RtpUtils.h" namespace erizo { DEFINE_LOGGER(RtcpFeedbackGenerationHandler, "rtp.RtcpFeedbackGenerationHandler"); -RtcpFeedbackGenerationHandler::RtcpFeedbackGenerationHandler(bool nacks_enabled, +RtcpFeedbackGenerationHandler::RtcpFeedbackGenerationHandler(bool nacks_enabled, bool pli_enabled, std::shared_ptr the_clock) - : stream_{nullptr}, enabled_{true}, initialized_{false}, nacks_enabled_{nacks_enabled}, clock_{the_clock} {} + : stream_{nullptr}, enabled_{true}, initialized_{false}, nacks_enabled_{nacks_enabled}, + pli_enabled_{pli_enabled}, clock_{the_clock} {} void RtcpFeedbackGenerationHandler::enable() { enabled_ = true; @@ -39,6 +41,7 @@ void RtcpFeedbackGenerationHandler::read(Context *ctx, std::shared_ptrisRtcp()) { RtpHeader *head = reinterpret_cast(packet->data); @@ -47,7 +50,14 @@ void RtcpFeedbackGenerationHandler::read(Context *ctx, std::shared_ptrsecond->rr_generator->handleRtpPacket(packet); if (nacks_enabled_) { - should_send_nack = generator_it->second->nack_generator->handleRtpPacket(packet); + const auto ret = generator_it->second->nack_generator->handleRtpPacket(packet); + should_send_nack = ret.first; + should_send_pli = ret.second; + if (pli_enabled_ && should_send_pli) { + ELOG_DEBUG("message: should send PLI, ssrc: %u", ssrc); + auto pli = RtpUtils::createPLI(ssrc, packet->type == VIDEO_PACKET ? video_sink_ssrc_ : audio_sink_ssrc_); + ctx->fireWrite(std::move(pli)); + } } } else { ELOG_DEBUG("message: no Generator found, ssrc: %u", ssrc); @@ -83,6 +93,10 @@ void RtcpFeedbackGenerationHandler::notifyUpdate() { if (!stream_) { return; } + + video_sink_ssrc_ = stream_->getVideoSinkSSRC(); + audio_sink_ssrc_ = stream_->getAudioSinkSSRC(); + // TODO(pedro) detect if nacks are enabled here with the negotiated SDP scanning the rtp_mappings std::vector video_ssrc_list = stream_->getVideoSourceSSRCList(); std::for_each(video_ssrc_list.begin(), video_ssrc_list.end(), [this] (uint32_t video_ssrc) { @@ -94,7 +108,7 @@ void RtcpFeedbackGenerationHandler::notifyUpdate() { ELOG_DEBUG("%s, message: Initialized video rrGenerator, ssrc: %u", stream_->toLog(), video_ssrc); if (nacks_enabled_) { ELOG_DEBUG("%s, message: Initialized video nack generator, ssrc %u", stream_->toLog(), video_ssrc); - auto video_nack = std::make_shared(video_ssrc, clock_); + auto video_nack = std::make_shared(video_ssrc, clock_); video_generator->nack_generator = video_nack; } } diff --git a/erizo/src/erizo/rtp/RtcpFeedbackGenerationHandler.h b/erizo/src/erizo/rtp/RtcpFeedbackGenerationHandler.h index a65138a68e..1cdf3d5e0a 100644 --- a/erizo/src/erizo/rtp/RtcpFeedbackGenerationHandler.h +++ b/erizo/src/erizo/rtp/RtcpFeedbackGenerationHandler.h @@ -8,7 +8,7 @@ #include "./logger.h" #include "pipeline/Handler.h" #include "rtp/RtcpRrGenerator.h" -#include "rtp/RtcpNackGenerator.h" +#include "rtp/RtcpNewNackGenerator.h" #include "lib/ClockUtils.h" #define MAX_DELAY 450000 @@ -20,7 +20,7 @@ class MediaStream; class RtcpGeneratorPair { public: std::shared_ptr rr_generator; - std::shared_ptr nack_generator; + std::shared_ptr nack_generator; }; @@ -29,7 +29,7 @@ class RtcpFeedbackGenerationHandler: public Handler { public: - explicit RtcpFeedbackGenerationHandler(bool nacks_enabled = true, + explicit RtcpFeedbackGenerationHandler(bool nacks_enabled = true, bool pli_enabled = true, std::shared_ptr the_clock = std::make_shared()); @@ -50,7 +50,11 @@ class RtcpFeedbackGenerationHandler: public Handler { bool enabled_, initialized_; bool nacks_enabled_; + bool pli_enabled_; std::shared_ptr clock_; + + uint32_t video_sink_ssrc_ = 0; + uint32_t audio_sink_ssrc_ = 0; }; } // namespace erizo diff --git a/erizo/src/erizo/rtp/RtcpNewNackGenerator.cpp b/erizo/src/erizo/rtp/RtcpNewNackGenerator.cpp new file mode 100644 index 0000000000..f04c787150 --- /dev/null +++ b/erizo/src/erizo/rtp/RtcpNewNackGenerator.cpp @@ -0,0 +1,189 @@ +#include "rtp/RtcpNewNackGenerator.h" + +#include +#include "rtp/RtpUtils.h" + +namespace erizo { +namespace { +constexpr int max_nack_blocks = 10; +constexpr int kNackCommonHeaderLengthRtcp = kNackCommonHeaderLengthBytes / 4 - 1; +constexpr int nack_interval = 50; + +/** + * Drops from the set all packets older than the given sequence_number + */ +void drop_packets_from_set(Seq_number_set& set, uint16_t sequence_number) { // NOLINT + set.erase(set.begin(), set.upper_bound(sequence_number)); +} + +void attach_nack_to_rr(uint32_t ssrc, + std::shared_ptr& rr_packet, // NOLINT + const std::vector& nack_blocks); + +std::vector build_nack_blocks(const Seq_number_set& set); +} // namespace + +DEFINE_LOGGER(RtcpNewNackGenerator, "rtp.RtcpNewNackGenerator"); + +std::pair RtcpNewNackGenerator::handleRtpPacket(const std::shared_ptr& packet) { + if (packet->type != VIDEO_PACKET) { + return {false, false}; + } + RtpHeader *head = reinterpret_cast(packet->data); + if (head->getSSRC() != ssrc_) { + ELOG_DEBUG("message: handleRtpPacket Unknown SSRC, ssrc: %u", head->getSSRC()); + return {false, false}; + } + const uint16_t seq_num = head->getSeqNumber(); + // Save when we got a key frame + if (packet->is_keyframe) { + keyframe_sequence_numbers_.insert(keyframe_sequence_numbers_.cend(), seq_num); + } + if (!latest_received_sequence_number_) { + latest_received_sequence_number_ = seq_num; + return {false, false}; + } + // Purge old keyframes + const auto newest_received_sequence_number = latest_sequence_number(*latest_received_sequence_number_, seq_num); + drop_packets_from_set(keyframe_sequence_numbers_, + newest_received_sequence_number - constants::max_packet_age_to_nack); + const auto should_ask_pli = !update_nack_list(seq_num); + latest_received_sequence_number_ = newest_received_sequence_number; + return {!missing_sequence_numbers_.empty() && is_time_to_send(clock_->now()), should_ask_pli}; +} + +bool RtcpNewNackGenerator::missing_too_old_packet(const uint16_t latest_sequence_number) { + if (missing_sequence_numbers_.empty()) { + return false; + } + const uint16_t age_of_oldest_missing_packet = latest_sequence_number - *missing_sequence_numbers_.cbegin(); + return age_of_oldest_missing_packet > constants::max_packet_age_to_nack; +} + +bool RtcpNewNackGenerator::handle_too_large_nack_list() { + ELOG_DEBUG("NACK list has grown too large"); + bool key_frame_found = false; + while (too_large_nack_list()) { + key_frame_found = recycle_frames_until_key_frame(); + } + return key_frame_found; +} + +bool RtcpNewNackGenerator::handle_too_old_packets(const uint16_t latest_sequence_number) { + ELOG_DEBUG("NACK list contains too old sequence numbers"); + bool key_frame_found = false; + while (missing_too_old_packet(latest_sequence_number)) { + key_frame_found = recycle_frames_until_key_frame(); + } + return key_frame_found; +} + +bool RtcpNewNackGenerator::recycle_frames_until_key_frame() { + // should use a frame buffer strategy, for now approximate by seeking the starting packet of the next key frame + std::pair key_frame = extract_oldest_keyframe(); + if (key_frame.first) { + drop_packets_from_set(missing_sequence_numbers_, key_frame.second); + ELOG_DEBUG("recycling frames... found keyframe at seq: %u", key_frame.second); + } else { + ELOG_DEBUG("recycling frames... dropping all"); + missing_sequence_numbers_.clear(); + } + return key_frame.first; +} + +bool RtcpNewNackGenerator::update_nack_list(const uint16_t seq) { + if (is_newer_sequence_number(seq, *latest_received_sequence_number_)) { + for (uint16_t i = *latest_received_sequence_number_ + 1; is_newer_sequence_number(seq, i); ++i) { + missing_sequence_numbers_.insert(missing_sequence_numbers_.cend(), i); + ELOG_DEBUG("added sequence number to NACK list: %u", i); + } + if (too_large_nack_list() && !handle_too_large_nack_list()) { + return false; + } + if (missing_too_old_packet(seq) && !handle_too_old_packets(seq)) { + return false; + } + } else { + missing_sequence_numbers_.erase(seq); + ELOG_DEBUG("recovered packet: %u", seq); + } + return true; +} + +std::pair RtcpNewNackGenerator::extract_oldest_keyframe() { + if (keyframe_sequence_numbers_.empty()) { + return {false, 0}; + } + auto seq = *keyframe_sequence_numbers_.cbegin(); + keyframe_sequence_numbers_.erase(keyframe_sequence_numbers_.cbegin()); + return {true, seq}; +} + +bool RtcpNewNackGenerator::addNackPacketToRr(std::shared_ptr& rr_packet) { + const auto now = clock_->now(); + if (!is_time_to_send(now)) { + return false; + } + rtcp_send_time_ = now; + const auto nack_blocks = build_nack_blocks(missing_sequence_numbers_); + attach_nack_to_rr(ssrc_, rr_packet, nack_blocks); + return true; +} + +std::chrono::milliseconds RtcpNewNackGenerator::rtcp_min_interval() const { + return std::chrono::milliseconds(nack_interval); +} + +bool RtcpNewNackGenerator::is_time_to_send(clock::time_point tp) const { + return (rtcp_send_time_ + rtcp_min_interval() <= tp); +} + +namespace { +std::vector build_nack_blocks(const Seq_number_set& set) { + std::vector nack_blocks; + for (auto it = set.cbegin(); it != set.cend(); ++it) { + if (nack_blocks.size() >= max_nack_blocks) { + break; + } + const uint16_t pid = *it; + ELOG_TRACE2(RtcpNewNackGenerator::logger, "added NACK PID, seq_num %u", pid); + uint16_t blp = 0; + for (++it; it != set.cend(); ++it) { + uint16_t distance = *it - pid; + if (distance > 16) { + break; + } + ELOG_TRACE2(RtcpNewNackGenerator::logger, "added Nack to BLP, seq_num: %u", *it); + blp |= (1 << (distance - 1)); + } + --it; + NackBlock block; + block.setNackPid(pid); + block.setNackBlp(blp); + nack_blocks.push_back(block); + } + return nack_blocks; +} + +void attach_nack_to_rr(const uint32_t ssrc, + std::shared_ptr& rr_packet, // NOLINT + const std::vector& nack_blocks) { + char* buffer = rr_packet->data; + buffer += rr_packet->length; + + RtcpHeader nack_packet; + nack_packet.setPacketType(RTCP_RTP_Feedback_PT); + nack_packet.setBlockCount(1); + nack_packet.setSSRC(ssrc); + nack_packet.setSourceSSRC(ssrc); + nack_packet.setLength(kNackCommonHeaderLengthRtcp + nack_blocks.size()); + memcpy(buffer, reinterpret_cast(&nack_packet), kNackCommonHeaderLengthBytes); + buffer += kNackCommonHeaderLengthBytes; + + memcpy(buffer, nack_blocks.data(), nack_blocks.size()*4); + int nack_length = (nack_packet.getLength()+1)*4; + + rr_packet->length += nack_length; +} +} // namespace +} // namespace erizo diff --git a/erizo/src/erizo/rtp/RtcpNewNackGenerator.h b/erizo/src/erizo/rtp/RtcpNewNackGenerator.h new file mode 100644 index 0000000000..fd78a2657c --- /dev/null +++ b/erizo/src/erizo/rtp/RtcpNewNackGenerator.h @@ -0,0 +1,93 @@ +#ifndef ERIZO_SRC_ERIZO_RTP_RTCPNEWNACKGENERATOR_H_ +#define ERIZO_SRC_ERIZO_RTP_RTCPNEWNACKGENERATOR_H_ + +#include "./logger.h" +#include "MediaDefinitions.h" +#include +#include +#include +#include +#include // NOLINT + +namespace erizo { +namespace constants { + constexpr int max_nack_list_size = 200; // chrome default 250 + constexpr int max_packet_age_to_nack = 300; // chrome default 450 +} + +inline bool is_newer_sequence_number(uint16_t sequence_number, uint16_t prev_sequence_number) { + return sequence_number != prev_sequence_number && + static_cast(sequence_number - prev_sequence_number) < 0x8000; +} + +inline uint16_t latest_sequence_number(uint16_t sequence_number1, uint16_t sequence_number2) { + return is_newer_sequence_number(sequence_number1, sequence_number2) + ? sequence_number1 + : sequence_number2; +} + +/// Comparator for seq_number_set +struct Seq_number_less_than { + bool operator()(uint16_t lhs, uint16_t rhs) const { return is_newer_sequence_number(rhs, lhs); } +}; + +/// Set storing sequence numbers in ascending order (iff close numbers get inserted) +using Seq_number_set = std::set; + +class RtcpNewNackGenerator { + public: + DECLARE_LOGGER(); + + explicit RtcpNewNackGenerator(uint32_t ssrc, const std::shared_ptr& clock = std::make_shared()) + : ssrc_(ssrc), clock_(clock) {} + + /** + * Handles an incoming rtp packet. Compatible with RtcpFeedbackGenerationHandler. + * + * @return A pair where first indicates if there's a NACK request and second if there's a PLI request + */ + std::pair handleRtpPacket(const std::shared_ptr& packet); + + /** + * Builds a nack and attaches it to an rtcp rr. Compatible with RtcpFeedbackGenerationHandler. + */ + bool addNackPacketToRr(std::shared_ptr& rr_packet); // NOLINT + + private: + inline bool too_large_nack_list() const; + + bool missing_too_old_packet(const uint16_t latest_sequence_number); + + bool handle_too_large_nack_list(); + + bool handle_too_old_packets(const uint16_t latest_sequence_number); + + bool recycle_frames_until_key_frame(); + + bool update_nack_list(const uint16_t seq); + + bool is_time_to_send(clock::time_point) const; + + std::chrono::milliseconds rtcp_min_interval() const; + + /** + * Find and remove the oldest key frame from the set + * @returns {true, sequence_number} if one key frame has been found, otherwise {false, undefined} + */ + std::pair extract_oldest_keyframe(); + + uint32_t ssrc_; + boost::optional latest_received_sequence_number_; + Seq_number_set missing_sequence_numbers_; + Seq_number_set keyframe_sequence_numbers_; + + std::shared_ptr clock_; + clock::time_point rtcp_send_time_; +}; + +bool RtcpNewNackGenerator::too_large_nack_list() const { + return missing_sequence_numbers_.size() > constants::max_nack_list_size; +} +} // namespace erizo + +#endif // ERIZO_SRC_ERIZO_RTP_RTCPNEWNACKGENERATOR_H_ diff --git a/erizo/src/test/rtp/RtcpFeedbackGenerationHandlerTest.cpp b/erizo/src/test/rtp/RtcpFeedbackGenerationHandlerTest.cpp index 651e40913a..34638ca259 100644 --- a/erizo/src/test/rtp/RtcpFeedbackGenerationHandlerTest.cpp +++ b/erizo/src/test/rtp/RtcpFeedbackGenerationHandlerTest.cpp @@ -40,7 +40,7 @@ class RtcpFeedbackRrGenerationTest : public erizo::HandlerTest { protected: void setHandler() { - rr_handler = std::make_shared(false, clock); + rr_handler = std::make_shared(false, false, clock); pipeline->addBack(rr_handler); } diff --git a/erizo/src/test/rtp/RtcpNewNackGeneratorTest.cpp b/erizo/src/test/rtp/RtcpNewNackGeneratorTest.cpp new file mode 100644 index 0000000000..7c6c9ff385 --- /dev/null +++ b/erizo/src/test/rtp/RtcpNewNackGeneratorTest.cpp @@ -0,0 +1,229 @@ +#include +#include + +#include +#include +#include +#include +#include +#include + +#include +#include + +#include +#include + +#include "../utils/Mocks.h" +#include "../utils/Tools.h" +#include "../utils/Matchers.h" + +using ::testing::_; +using ::testing::IsNull; +using ::testing::Args; +using ::testing::Return; +using ::testing::AllOf; +using erizo::DataPacket; +using erizo::packetType; +using erizo::AUDIO_PACKET; +using erizo::VIDEO_PACKET; +using erizo::RtcpNewNackGenerator; +using erizo::RtcpHeader; +using erizo::WebRtcConnection; +using erizo::SimulatedClock; + +class RtcpNewNackGeneratorTest :public ::testing::Test { + public: + RtcpNewNackGeneratorTest(): clock{std::make_shared()}, + nack_generator{erizo::kVideoSsrc, clock} {} + + protected: + void advanceClockMs(int time_ms) { + clock->advanceTime(std::chrono::milliseconds(time_ms)); + } + + std::shared_ptr generateRrWithNack() { + std::shared_ptr new_receiver_report = erizo::PacketTools::createReceiverReport(erizo::kVideoSsrc, + erizo::kVideoSsrc, 0, VIDEO_PACKET); + nack_generator.addNackPacketToRr(new_receiver_report); + return new_receiver_report; + } + + bool RtcpPacketContainsNackSeqNum(std::shared_ptr rtcp_packet, uint16_t lost_seq_num) { + char* moving_buf = rtcp_packet->data; + int rtcp_length = 0; + int total_length = 0; + bool found_nack = false; + do { + moving_buf += rtcp_length; + RtcpHeader* chead = reinterpret_cast(moving_buf); + rtcp_length = (ntohs(chead->length) + 1) * 4; + total_length += rtcp_length; + + if (chead->packettype == RTCP_RTP_Feedback_PT) { + erizo::RtpUtils::forEachNack(chead, [chead, lost_seq_num, &found_nack](uint16_t seq_num, + uint16_t plb, RtcpHeader* nack_head) { + uint16_t initial_seq_num = seq_num; + if (initial_seq_num == lost_seq_num) { + found_nack = true; + return; + } + uint16_t kNackBlpSize = 16; + for (int i = -1; i <= kNackBlpSize; i++) { + if ((plb >> i) & 0x0001) { + uint16_t seq_num = initial_seq_num + i + 1; + if (seq_num == lost_seq_num) { + found_nack = true; + return; + } + } + } + }); + } + } while (total_length < rtcp_packet->length); + return found_nack; + } + + std::shared_ptr clock; + std::shared_ptr receiver_report; + RtcpNewNackGenerator nack_generator; + const uint16_t kMaxSeqnum = 65534; +}; + +TEST_F(RtcpNewNackGeneratorTest, shouldNotGenerateNackForConsecutivePackets) { + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + 1, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + EXPECT_FALSE(nack_generator.handleRtpPacket(second_packet).first); +} + +TEST_F(RtcpNewNackGeneratorTest, shouldNotGenerateNackForConsecutivePacketsWithRollOver) { + auto first_packet = erizo::PacketTools::createDataPacket(kMaxSeqnum, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(kMaxSeqnum + 1, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + EXPECT_FALSE(nack_generator.handleRtpPacket(second_packet).first); +} + +TEST_F(RtcpNewNackGeneratorTest, shouldGenerateNackOnLostPacket) { + uint16_t kArbitraryNumberOfPackets = 4; + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).first); +} + +TEST_F(RtcpNewNackGeneratorTest, shouldGenerateNackOnLostPacketWithRollOver) { + uint16_t kArbitraryNumberOfPackets = 4; + auto first_packet = erizo::PacketTools::createDataPacket(kMaxSeqnum, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(kMaxSeqnum + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).first); +} + +TEST_F(RtcpNewNackGeneratorTest, nackShouldContainLostPackets) { + uint16_t kArbitraryNumberOfPackets = 4; + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).first); + + receiver_report = generateRrWithNack(); + EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); +} + +TEST_F(RtcpNewNackGeneratorTest, nackShouldContainLostPacketsInMoreThanOneBlock) { + uint16_t kArbitraryNumberOfPackets = 30; // Bigger than 16 - one block + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).first); + + receiver_report = generateRrWithNack(); + + for (int i = 13; i < 42; ++i) { + EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, i)) << "i = " << i; + } +} + +TEST_F(RtcpNewNackGeneratorTest, shouldNotRetransmitNacksInmediately) { + uint16_t kArbitraryNumberOfPackets = 4; + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + nack_generator.handleRtpPacket(second_packet); + + receiver_report = generateRrWithNack(); + + EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); + + receiver_report = generateRrWithNack(); + + EXPECT_FALSE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); +} + +TEST_F(RtcpNewNackGeneratorTest, shouldRetransmitNacksAfterTime) { + uint16_t kArbitraryNumberOfPackets = 4; + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + nack_generator.handleRtpPacket(second_packet); + + receiver_report = generateRrWithNack(); + + EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); + + advanceClockMs(50); + receiver_report = generateRrWithNack(); + + EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); +} + +TEST_F(RtcpNewNackGeneratorTest, cornerCase) { + auto first_packet = erizo::PacketTools::createDataPacket(65518, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(65520, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).first); + receiver_report = generateRrWithNack(); + EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, 65519)); +} + +TEST_F(RtcpNewNackGeneratorTest, shouldRetransmitNacksMoreThanTwice) { + uint16_t kArbitraryNumberOfPackets = 4; + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + nack_generator.handleRtpPacket(second_packet); + + receiver_report = generateRrWithNack(); + + EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); + + advanceClockMs(50); + receiver_report = generateRrWithNack(); + EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); + + advanceClockMs(50); + receiver_report = generateRrWithNack(); + EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); +} + +TEST_F(RtcpNewNackGeneratorTest, shouldAskPLI) { + uint16_t kArbitraryNumberOfPackets = 800; + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).second); +} diff --git a/erizo_controller/erizoAgent/log4cxx.properties b/erizo_controller/erizoAgent/log4cxx.properties index 162aa38649..c966796db4 100644 --- a/erizo_controller/erizoAgent/log4cxx.properties +++ b/erizo_controller/erizoAgent/log4cxx.properties @@ -59,6 +59,7 @@ log4j.logger.rtp.RtcpFeedbackGenerationHandler=WARN log4j.logger.rtp.RtpPaddingRemovalHandler=WARN log4j.logger.rtp.RtcpRrGenerator=WARN log4j.logger.rtp.RtcpNackGenerator=WARN +log4j.logger.rtp.RtcpNewNackGenerator=WARN log4j.logger.rtp.SRPacketHandler=WARN log4j.logger.rtp.BandwidthEstimationHandler=WARN log4j.logger.rtp.SenderBandwidthEstimationHandler=WARN diff --git a/spine/log4cxx.properties b/spine/log4cxx.properties index 9fdd04ea3f..0f2d6818e2 100644 --- a/spine/log4cxx.properties +++ b/spine/log4cxx.properties @@ -58,6 +58,7 @@ log4j.logger.rtp.RtcpFeedbackGenerationHandler=WARN log4j.logger.rtp.RtpPaddingRemovalHandler=WARN log4j.logger.rtp.RtcpRrGenerator=WARN log4j.logger.rtp.RtcpNackGenerator=WARN +log4j.logger.rtp.RtcpNewNackGenerator=WARN log4j.logger.rtp.SRPacketHandler=WARN log4j.logger.rtp.BandwidthEstimationHandler=WARN log4j.logger.rtp.SenderBandwidthEstimationHandler=WARN From fbabbb77c546e42a63b9d719ed23626a2d09fd06 Mon Sep 17 00:00:00 2001 From: Andrea Date: Wed, 20 Dec 2017 12:24:52 +0100 Subject: [PATCH 2/2] new interface for RtcpNackGenerator and tests to help development --- erizo/src/erizo/rtp/RtcpNackGenerator.cpp | 14 ++++---- erizo/src/erizo/rtp/RtcpNackGenerator.h | 5 ++- erizo/src/test/rtp/RtcpNackGeneratorTest.cpp | 34 +++++++++++++++---- .../src/test/rtp/RtcpNewNackGeneratorTest.cpp | 13 ++++++- 4 files changed, 50 insertions(+), 16 deletions(-) diff --git a/erizo/src/erizo/rtp/RtcpNackGenerator.cpp b/erizo/src/erizo/rtp/RtcpNackGenerator.cpp index 510abf589b..e8279dd2a6 100644 --- a/erizo/src/erizo/rtp/RtcpNackGenerator.cpp +++ b/erizo/src/erizo/rtp/RtcpNackGenerator.cpp @@ -14,23 +14,23 @@ static const int kNackCommonHeaderLengthRtcp = kNackCommonHeaderLengthBytes/4 - RtcpNackGenerator::RtcpNackGenerator(uint32_t ssrc, std::shared_ptr the_clock) : initialized_{false}, highest_seq_num_{0}, ssrc_{ssrc}, clock_{the_clock} {} -bool RtcpNackGenerator::handleRtpPacket(std::shared_ptr packet) { +std::pair RtcpNackGenerator::handleRtpPacket(std::shared_ptr packet) { if (packet->type != VIDEO_PACKET) { - return false; + return {false, false}; } RtpHeader *head = reinterpret_cast(packet->data); uint16_t seq_num = head->getSeqNumber(); if (head->getSSRC() != ssrc_) { ELOG_DEBUG("message: handleRtpPacket Unknown SSRC, ssrc: %u", head->getSSRC()); - return false; + return {false, false}; } if (!initialized_) { highest_seq_num_ = seq_num; initialized_ = true; - return 0; + return {false, false}; } if (seq_num == highest_seq_num_) { - return false; + return {false, false}; } // TODO(pedro) Consider clearing the nack list if this is a keyframe if (RtpUtils::sequenceNumberLessThan(seq_num, highest_seq_num_)) { @@ -45,11 +45,11 @@ bool RtcpNackGenerator::handleRtpPacket(std::shared_ptr packet) { ELOG_DEBUG("message: Recovered Packet %u", seq_num); nack_info_list_.erase(nack_info); } - return false; + return {false, false}; } bool available_nacks = addNacks(seq_num); highest_seq_num_ = seq_num; - return available_nacks; + return {available_nacks, false}; } bool RtcpNackGenerator::addNacks(uint16_t seq_num) { diff --git a/erizo/src/erizo/rtp/RtcpNackGenerator.h b/erizo/src/erizo/rtp/RtcpNackGenerator.h index 4ecc71efd5..12b7732741 100644 --- a/erizo/src/erizo/rtp/RtcpNackGenerator.h +++ b/erizo/src/erizo/rtp/RtcpNackGenerator.h @@ -28,7 +28,10 @@ class RtcpNackGenerator{ public: explicit RtcpNackGenerator(uint32_t ssrc_, std::shared_ptr the_clock = std::make_shared()); - bool handleRtpPacket(std::shared_ptr packet); + /** + * @return A pair where first indicates if there's a NACK request and second if there's a PLI request + */ + std::pair handleRtpPacket(std::shared_ptr packet); bool addNackPacketToRr(std::shared_ptr rr_packet); private: diff --git a/erizo/src/test/rtp/RtcpNackGeneratorTest.cpp b/erizo/src/test/rtp/RtcpNackGeneratorTest.cpp index c6cfac3198..639b95b368 100644 --- a/erizo/src/test/rtp/RtcpNackGeneratorTest.cpp +++ b/erizo/src/test/rtp/RtcpNackGeneratorTest.cpp @@ -29,7 +29,7 @@ using erizo::RtcpHeader; using erizo::WebRtcConnection; using erizo::SimulatedClock; -class RtcpNackGeneratorTest :public ::testing::Test { +class RtcpNackGeneratorTest :public ::testing::TestWithParam { public: RtcpNackGeneratorTest(): clock{std::make_shared()}, nack_generator{erizo::kVideoSsrc, clock} {} @@ -92,7 +92,7 @@ TEST_F(RtcpNackGeneratorTest, shouldNotGenerateNackForConsecutivePackets) { auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + 1, VIDEO_PACKET); nack_generator.handleRtpPacket(first_packet); - EXPECT_FALSE(nack_generator.handleRtpPacket(second_packet)); + EXPECT_FALSE(nack_generator.handleRtpPacket(second_packet).first); } TEST_F(RtcpNackGeneratorTest, shouldNotGenerateNackForConsecutivePacketsWithRollOver) { @@ -100,7 +100,7 @@ TEST_F(RtcpNackGeneratorTest, shouldNotGenerateNackForConsecutivePacketsWithRoll auto second_packet = erizo::PacketTools::createDataPacket(kMaxSeqnum + 1, VIDEO_PACKET); nack_generator.handleRtpPacket(first_packet); - EXPECT_FALSE(nack_generator.handleRtpPacket(second_packet)); + EXPECT_FALSE(nack_generator.handleRtpPacket(second_packet).first); } TEST_F(RtcpNackGeneratorTest, shouldGenerateNackOnLostPacket) { @@ -109,7 +109,7 @@ TEST_F(RtcpNackGeneratorTest, shouldGenerateNackOnLostPacket) { auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, VIDEO_PACKET); nack_generator.handleRtpPacket(first_packet); - EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet)); + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).first); } TEST_F(RtcpNackGeneratorTest, shouldGenerateNackOnLostPacketWithRollOver) { @@ -118,7 +118,7 @@ TEST_F(RtcpNackGeneratorTest, shouldGenerateNackOnLostPacketWithRollOver) { auto second_packet = erizo::PacketTools::createDataPacket(kMaxSeqnum + kArbitraryNumberOfPackets, VIDEO_PACKET); nack_generator.handleRtpPacket(first_packet); - EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet)); + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).first); } TEST_F(RtcpNackGeneratorTest, nackShouldContainLostPackets) { @@ -128,7 +128,7 @@ TEST_F(RtcpNackGeneratorTest, nackShouldContainLostPackets) { VIDEO_PACKET); nack_generator.handleRtpPacket(first_packet); - EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet)); + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).first); receiver_report = generateRrWithNack(); EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); @@ -141,7 +141,7 @@ TEST_F(RtcpNackGeneratorTest, nackShouldContainLostPacketsInMoreThanOneBlock) { VIDEO_PACKET); nack_generator.handleRtpPacket(first_packet); - EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet)); + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).first); receiver_report = generateRrWithNack(); EXPECT_TRUE(RtcpPacketContainsNackSeqNum(receiver_report, 35)); @@ -202,3 +202,23 @@ TEST_F(RtcpNackGeneratorTest, shouldNotRetransmitNacksMoreThanTwice) { receiver_report = generateRrWithNack(); EXPECT_FALSE(RtcpPacketContainsNackSeqNum(receiver_report, erizo::kArbitrarySeqNumber + 1)); } + +TEST_F(RtcpNackGeneratorTest, shouldAskPLI) { + uint16_t kArbitraryNumberOfPackets = 800; + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).second); +} + +TEST_P(RtcpNackGeneratorTest, shouldNotAskPLI) { + uint16_t kArbitraryNumberOfPackets = GetParam(); + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + EXPECT_FALSE(nack_generator.handleRtpPacket(second_packet).second); +} + +INSTANTIATE_TEST_CASE_P(RtcpNackGeneratorTest, RtcpNackGeneratorTest, ::testing::Range(0, 50)); diff --git a/erizo/src/test/rtp/RtcpNewNackGeneratorTest.cpp b/erizo/src/test/rtp/RtcpNewNackGeneratorTest.cpp index 7c6c9ff385..62bef39be9 100644 --- a/erizo/src/test/rtp/RtcpNewNackGeneratorTest.cpp +++ b/erizo/src/test/rtp/RtcpNewNackGeneratorTest.cpp @@ -32,7 +32,7 @@ using erizo::RtcpHeader; using erizo::WebRtcConnection; using erizo::SimulatedClock; -class RtcpNewNackGeneratorTest :public ::testing::Test { +class RtcpNewNackGeneratorTest :public ::testing::TestWithParam { public: RtcpNewNackGeneratorTest(): clock{std::make_shared()}, nack_generator{erizo::kVideoSsrc, clock} {} @@ -227,3 +227,14 @@ TEST_F(RtcpNewNackGeneratorTest, shouldAskPLI) { nack_generator.handleRtpPacket(first_packet); EXPECT_TRUE(nack_generator.handleRtpPacket(second_packet).second); } + +TEST_P(RtcpNewNackGeneratorTest, shouldNotAskPLI) { + uint16_t kArbitraryNumberOfPackets = GetParam(); + auto first_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber, VIDEO_PACKET); + auto second_packet = erizo::PacketTools::createDataPacket(erizo::kArbitrarySeqNumber + kArbitraryNumberOfPackets, + VIDEO_PACKET); + nack_generator.handleRtpPacket(first_packet); + EXPECT_FALSE(nack_generator.handleRtpPacket(second_packet).second); +} + +INSTANTIATE_TEST_CASE_P(RtcpNewNackGeneratorTest, RtcpNewNackGeneratorTest, ::testing::Range(0, 50));