From e04984d1499f3816e31e9e4d83f20cc22dba642c Mon Sep 17 00:00:00 2001 From: Pedro Rodriguez Date: Mon, 27 Mar 2017 17:38:15 +0200 Subject: [PATCH] Fix slideshow keyframe detection (#825) --- erizo/src/erizo/rtp/RtpSlideShowHandler.cpp | 129 +++++++++--------- erizo/src/erizo/rtp/RtpSlideShowHandler.h | 14 +- erizo/src/test/log4cxx.properties | 2 +- .../src/test/rtp/RtpSlideShowHandlerTest.cpp | 42 ++++-- erizo/src/test/utils/Tools.h | 27 +++- 5 files changed, 125 insertions(+), 89 deletions(-) diff --git a/erizo/src/erizo/rtp/RtpSlideShowHandler.cpp b/erizo/src/erizo/rtp/RtpSlideShowHandler.cpp index 0b4938403b..8020b31b8a 100644 --- a/erizo/src/erizo/rtp/RtpSlideShowHandler.cpp +++ b/erizo/src/erizo/rtp/RtpSlideShowHandler.cpp @@ -1,15 +1,16 @@ #include "./MediaDefinitions.h" #include "rtp/RtpSlideShowHandler.h" +#include "rtp/RtpUtils.h" namespace erizo { DEFINE_LOGGER(RtpSlideShowHandler, "rtp.RtpSlideShowHandler"); RtpSlideShowHandler::RtpSlideShowHandler() - : connection_{nullptr}, - slideshow_seq_num_{-1}, last_original_seq_num_{-1}, seq_num_offset_{0}, + : connection_{nullptr}, highest_seq_num_initialized_{false}, + highest_seq_num_ {0}, slideshow_is_active_{false}, - sending_keyframe_ {false} {} + current_keyframe_timestamp_{0} {} void RtpSlideShowHandler::enable() { @@ -32,48 +33,51 @@ void RtpSlideShowHandler::read(Context *ctx, std::shared_ptr packet) ctx->fireRead(packet); return; } - if (seq_num_offset_ > 0) { - char* buf = packet->data; - char* report_pointer = buf; - int rtcp_length = 0; - int total_length = 0; - do { - report_pointer += rtcp_length; - chead = reinterpret_cast(report_pointer); - rtcp_length = (ntohs(chead->length) + 1) * 4; - total_length += rtcp_length; - switch (chead->packettype) { - case RTCP_Receiver_PT: - if ((chead->getHighestSeqnum() + seq_num_offset_) < chead->getHighestSeqnum()) { - // The seqNo adjustment causes a wraparound, add to cycles - chead->setSeqnumCycles(chead->getSeqnumCycles() + 1); + RtpUtils::forEachRRBlock(packet, [this](RtcpHeader *chead) { + switch (chead->packettype) { + case RTCP_Receiver_PT: + { + uint16_t incoming_seq_num = chead->getHighestSeqnum(); + ELOG_DEBUG("Received RR, highest %u", incoming_seq_num); + SequenceNumber input_seq_num = translator_.reverse(incoming_seq_num); + if (input_seq_num.type != SequenceNumberType::Valid) { + break; + } + if (RtpUtils::sequenceNumberLessThan(input_seq_num.input, incoming_seq_num)) { + chead->setSeqnumCycles(chead->getSeqnumCycles() - 1); } - chead->setHighestSeqnum(chead->getHighestSeqnum() + seq_num_offset_); + chead->setHighestSeqnum(input_seq_num.input); + ELOG_DEBUG("Setting highest %u, output %u", input_seq_num.input, input_seq_num.output); break; - case RTCP_RTP_Feedback_PT: - chead->setNackPid(chead->getNackPid() + seq_num_offset_); - break; - default: + } + case RTCP_RTP_Feedback_PT: + { + ELOG_DEBUG("Received NACK, PID %u", chead->getNackPid()); + SequenceNumber input_seq_num = translator_.reverse(chead->getNackPid()); + if (input_seq_num.type == SequenceNumberType::Valid) { + chead->setNackPid(input_seq_num.input); + ELOG_DEBUG("Setting PID %u, output %u", input_seq_num.input, input_seq_num.output); + } break; - } - } while (total_length < packet->length); - } + } + default: + break; + } + }); ctx->fireRead(packet); } void RtpSlideShowHandler::write(Context *ctx, std::shared_ptr packet) { RtpHeader *rtp_header = reinterpret_cast(packet->data); RtcpHeader *rtcp_header = reinterpret_cast(packet->data); - if (packet->type != VIDEO_PACKET || rtcp_header->isRtcp()) { ctx->fireWrite(packet); return; } - last_original_seq_num_ = rtp_header->getSeqNumber(); - if (slideshow_seq_num_ == -1) { // We didn't receive any packets before setting up slideshow - slideshow_seq_num_ = last_original_seq_num_; - } + bool should_skip_packet = false; + + uint16_t packet_seq_num = rtp_header->getSeqNumber(); if (slideshow_is_active_) { bool is_keyframe = false; RtpMap *codec = connection_->getRemoteSdpInfo().getCodecByExternalPayloadType(rtp_header->getPayloadType()); @@ -82,43 +86,40 @@ void RtpSlideShowHandler::write(Context *ctx, std::shared_ptr packet } else if (codec && codec->encoding_name == "VP9") { is_keyframe = isVP9Keyframe(packet); } - if (is_keyframe) { - setPacketSeqNumber(packet, slideshow_seq_num_++); - ctx->fireWrite(packet); - } - } else { - if (seq_num_offset_ > 0) { - setPacketSeqNumber(packet, (last_original_seq_num_ - seq_num_offset_)); - } + should_skip_packet = !is_keyframe; + } + maybeUpdateHighestSeqNum(rtp_header->getSeqNumber()); + SequenceNumber sequence_number_info = translator_.get(packet_seq_num, should_skip_packet); + if (!should_skip_packet && sequence_number_info.type == SequenceNumberType::Valid) { + rtp_header->setSeqNumber(sequence_number_info.output); + ELOG_DEBUG("Writing packet incoming seq %u, output %u", packet_seq_num, sequence_number_info.output); ctx->fireWrite(packet); + } else { + ELOG_DEBUG("Skipped packet %u", packet_seq_num); } } bool RtpSlideShowHandler::isVP8Keyframe(std::shared_ptr packet) { bool is_keyframe = false; RtpHeader *rtp_header = reinterpret_cast(packet->data); - unsigned char* start_buffer = reinterpret_cast (packet->data); - start_buffer = start_buffer + rtp_header->getHeaderLength(); - RTPPayloadVP8* payload = vp8_parser_.parseVP8( - start_buffer, packet->length - rtp_header->getHeaderLength()); - if (!payload->frameType) { // Its a keyframe first packet - sending_keyframe_ = true; - } - delete payload; - is_keyframe = sending_keyframe_; - if (sending_keyframe_ && rtp_header->getMarker()) { - sending_keyframe_ = false; + uint16_t seq_num = rtp_header->getSeqNumber(); + uint32_t timestamp = rtp_header->getTimestamp(); + + if (packet->is_keyframe && + (RtpUtils::sequenceNumberLessThan(highest_seq_num_, seq_num) || !highest_seq_num_initialized_)) { + is_keyframe = true; + current_keyframe_timestamp_ = timestamp; + } else if (timestamp == current_keyframe_timestamp_) { + is_keyframe = true; } + + ELOG_DEBUG("packet is_keyframe %d, packet timestamp %u, current_keyframe timestamp %u, result %d", + packet->is_keyframe, rtp_header->getTimestamp(), current_keyframe_timestamp_, is_keyframe); return is_keyframe; } bool RtpSlideShowHandler::isVP9Keyframe(std::shared_ptr packet) { - RtpHeader *rtp_header = reinterpret_cast(packet->data); - unsigned char* start_buffer = reinterpret_cast (packet->data); - start_buffer = start_buffer + rtp_header->getHeaderLength(); - RTPPayloadVP9* payload = vp9_parser_.parseVP9( - start_buffer, packet->length - rtp_header->getHeaderLength()); - return !payload->frameType; + return packet->is_keyframe; } void RtpSlideShowHandler::setSlideShowMode(bool active) { @@ -127,27 +128,19 @@ void RtpSlideShowHandler::setSlideShowMode(bool active) { } if (active) { - slideshow_seq_num_ = last_original_seq_num_ - seq_num_offset_; - sending_keyframe_ = false; slideshow_is_active_ = true; connection_->setFeedbackReports(false, 0); - ELOG_DEBUG("%s message: Setting seqNo, seqNo: %u", connection_->toLog(), slideshow_seq_num_); } else { - seq_num_offset_ = last_original_seq_num_ - slideshow_seq_num_ + 1; - ELOG_DEBUG("%s message: Changing offset manually, original_seq_num: %u, slideshow_seq_num: %u, offset: %u", - connection_->toLog(), last_original_seq_num_, slideshow_seq_num_, seq_num_offset_); slideshow_is_active_ = false; connection_->setFeedbackReports(true, 0); } } -inline void RtpSlideShowHandler::setPacketSeqNumber(std::shared_ptr packet, uint16_t seq_number) { - RtpHeader *head = reinterpret_cast (packet->data); - RtcpHeader *chead = reinterpret_cast (packet->data); - if (chead->isRtcp()) { - return; +void RtpSlideShowHandler::maybeUpdateHighestSeqNum(uint16_t seq_num) { + if (RtpUtils::sequenceNumberLessThan(highest_seq_num_, seq_num) || !highest_seq_num_initialized_) { + highest_seq_num_ = seq_num; + highest_seq_num_initialized_ = true; } - head->setSeqNumber(seq_number); } } // namespace erizo diff --git a/erizo/src/erizo/rtp/RtpSlideShowHandler.h b/erizo/src/erizo/rtp/RtpSlideShowHandler.h index 0182432ef2..a16d58db53 100644 --- a/erizo/src/erizo/rtp/RtpSlideShowHandler.h +++ b/erizo/src/erizo/rtp/RtpSlideShowHandler.h @@ -6,6 +6,7 @@ #include "pipeline/Handler.h" #include "./logger.h" #include "./WebRtcConnection.h" +#include "rtp/SequenceNumberTranslator.h" #include "rtp/RtpVP8Parser.h" #include "rtp/RtpVP9Parser.h" @@ -32,17 +33,16 @@ class RtpSlideShowHandler : public Handler { private: bool isVP8Keyframe(std::shared_ptr packet); bool isVP9Keyframe(std::shared_ptr packet); + void maybeUpdateHighestSeqNum(uint16_t seq_num); private: WebRtcConnection* connection_; - int32_t slideshow_seq_num_, last_original_seq_num_; - uint16_t seq_num_offset_; + SequenceNumberTranslator translator_; + bool highest_seq_num_initialized_; + uint16_t highest_seq_num_; - bool slideshow_is_active_, sending_keyframe_; - RtpVP8Parser vp8_parser_; - RtpVP9Parser vp9_parser_; - - inline void setPacketSeqNumber(std::shared_ptr packet, uint16_t seq_number); + bool slideshow_is_active_; + uint32_t current_keyframe_timestamp_; }; } // namespace erizo diff --git a/erizo/src/test/log4cxx.properties b/erizo/src/test/log4cxx.properties index 3a79cb37c4..84e793bba0 100644 --- a/erizo/src/test/log4cxx.properties +++ b/erizo/src/test/log4cxx.properties @@ -47,7 +47,7 @@ log4j.logger.rtp.RtpPacketQueue=ERROR log4j.logger.rtp.RtpRetransmissionHandler=ERROR log4j.logger.rtp.RtpVP8Fragmenter=ERROR log4j.logger.rtp.RtpVP8Parser=ERROR -log4j.logger.rtp.RtpVP8SlideShowHandler=ERROR +log4j.logger.rtp.RtpSlideShowHandler=ERROR log4j.logger.rtp.RtpAudioMuteHandler=ERROR log4j.logger.rtp.RtpSink=ERROR log4j.logger.rtp.RtpSource=ERROR diff --git a/erizo/src/test/rtp/RtpSlideShowHandlerTest.cpp b/erizo/src/test/rtp/RtpSlideShowHandlerTest.cpp index aaf40c79f2..06bf9af1c7 100644 --- a/erizo/src/test/rtp/RtpSlideShowHandlerTest.cpp +++ b/erizo/src/test/rtp/RtpSlideShowHandlerTest.cpp @@ -33,6 +33,8 @@ using erizo::OutboundHandler; using erizo::Worker; using std::queue; +const uint32_t kArbitraryTimestamp = 1000; + class RtpSlideShowHandlerTest : public erizo::HandlerTest { public: RtpSlideShowHandlerTest() { @@ -95,13 +97,13 @@ TEST_F(RtpSlideShowHandlerTest, shouldTransmitJustKeyframesInVP9) { packet_queue.pop(); } } -TEST_F(RtpSlideShowHandlerTest, shouldTransmitFromBeginningOfKFrameToMarkerPacketsWhenActive) { +TEST_F(RtpSlideShowHandlerTest, shouldTransmitFromBeginningOfKFrameToTimestampChange) { uint16_t seq_number = erizo::kArbitrarySeqNumber; - packet_queue.push(erizo::PacketTools::createVP8Packet(seq_number, false, false)); - packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, true, false)); - packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, false, false)); - packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, false, true)); - packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, false, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(seq_number, kArbitraryTimestamp - 1, false, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp, true, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp, false, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp, false, true)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp + 1, false, false)); slideshow_handler->setSlideShowMode(true); EXPECT_CALL(*writer.get(), write(_, _)).Times(3); @@ -111,6 +113,22 @@ TEST_F(RtpSlideShowHandlerTest, shouldTransmitFromBeginningOfKFrameToMarkerPacke } } +TEST_F(RtpSlideShowHandlerTest, shouldIgnoreKeyframeIfIsKeyframeIsNotFirstPacket) { + uint16_t seq_number = erizo::kArbitrarySeqNumber; + packet_queue.push(erizo::PacketTools::createVP8Packet(seq_number, kArbitraryTimestamp - 1, false, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(seq_number + 2, kArbitraryTimestamp, false, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(seq_number + 1, kArbitraryTimestamp, true, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp, false, true)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp + 1, false, false)); + slideshow_handler->setSlideShowMode(true); + + EXPECT_CALL(*writer.get(), write(_, _)).Times(0); + while (!packet_queue.empty()) { + pipeline->write(packet_queue.front()); + packet_queue.pop(); + } +} + TEST_F(RtpSlideShowHandlerTest, shouldMantainSequenceNumberInSlideShow) { uint16_t seq_number = erizo::kArbitrarySeqNumber; packet_queue.push(erizo::PacketTools::createVP8Packet(seq_number, true, false)); @@ -157,17 +175,17 @@ TEST_F(RtpSlideShowHandlerTest, shouldAdjustSequenceNumberAfterSlideShow) { uint16_t seq_number = erizo::kArbitrarySeqNumber; uint16_t packets_after_handler = 0; - packet_queue.push(erizo::PacketTools::createVP8Packet(seq_number, true, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(seq_number, kArbitraryTimestamp, true, false)); packets_after_handler++; - packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, false, true)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp, false, true)); packets_after_handler++; - packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, false, false)); - packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, false, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp + 1, false, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp + 2, false, false)); - packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, true, false)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp + 3, true, false)); packets_after_handler++; - packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, false, true)); + packet_queue.push(erizo::PacketTools::createVP8Packet(++seq_number, kArbitraryTimestamp + 3, false, true)); packets_after_handler++; slideshow_handler->setSlideShowMode(true); diff --git a/erizo/src/test/utils/Tools.h b/erizo/src/test/utils/Tools.h index a8beb4d08c..02dea3d81e 100644 --- a/erizo/src/test/utils/Tools.h +++ b/erizo/src/test/utils/Tools.h @@ -102,6 +102,31 @@ class PacketTools { return packet; } + static std::shared_ptr createVP8Packet(uint16_t seq_number, uint32_t timestamp, + bool is_keyframe, bool is_marker) { + erizo::RtpHeader *header = new erizo::RtpHeader(); + header->setPayloadType(96); + header->setSeqNumber(seq_number); + header->setSSRC(kVideoSsrc); + header->setTimestamp(timestamp); + header->setMarker(is_marker); + char packet_buffer[200]; + memset(packet_buffer, 0, 200); + char* data_pointer; + char* parsing_pointer; + memcpy(packet_buffer, reinterpret_cast(header), header->getHeaderLength()); + data_pointer = packet_buffer + header->getHeaderLength(); + parsing_pointer = data_pointer; + + *parsing_pointer = 0x10; + parsing_pointer++; + *parsing_pointer = is_keyframe? 0x00: 0x01; + + auto packet = std::make_shared(0, packet_buffer, 200, VIDEO_PACKET); + packet->is_keyframe = is_keyframe; + return packet; + } + static std::shared_ptr createVP9Packet(uint16_t seq_number, bool is_keyframe, bool is_marker) { erizo::RtpHeader *header = new erizo::RtpHeader(); header->setPayloadType(98); @@ -186,7 +211,7 @@ class BaseHandlerTest { EXPECT_CALL(*reader, notifyUpdate()).Times(testing::AtLeast(1)); EXPECT_CALL(*writer, notifyUpdate()).Times(testing::AtLeast(1)); - EXPECT_CALL(*reader, read(_,_)).Times(testing::AtLeast(0)); + EXPECT_CALL(*reader, read(_, _)).Times(testing::AtLeast(0)); EXPECT_CALL(*writer, write(_, _)).Times(testing::AtLeast(0)); std::shared_ptr connection_ptr = std::dynamic_pointer_cast(connection);