From 30e8931ea7700e831cb2e4e373c5368604503e46 Mon Sep 17 00:00:00 2001 From: nisse Date: Mon, 29 May 2017 08:16:37 -0700 Subject: [PATCH] Delete RtpData::OnRecoveredPacket, use RecoveredPacketReceiver instead. BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/2886813002 Cr-Commit-Position: refs/heads/master@{#18305} --- .../rtp_rtcp/include/flexfec_receiver.h | 10 ---- .../rtp_rtcp/include/rtp_rtcp_defines.h | 16 ++++-- .../rtp_rtcp/include/ulpfec_receiver.h | 2 +- .../rtp_rtcp/source/ulpfec_receiver_impl.cc | 16 ++---- .../rtp_rtcp/source/ulpfec_receiver_impl.h | 4 +- .../source/ulpfec_receiver_unittest.cc | 57 +++++++++---------- webrtc/video/rtp_stream_receiver.cc | 42 +++++++------- webrtc/video/rtp_stream_receiver.h | 8 ++- webrtc/voice_engine/channel.h | 3 +- 9 files changed, 74 insertions(+), 84 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/include/flexfec_receiver.h b/webrtc/modules/rtp_rtcp/include/flexfec_receiver.h index a31a285968..437a365f60 100644 --- a/webrtc/modules/rtp_rtcp/include/flexfec_receiver.h +++ b/webrtc/modules/rtp_rtcp/include/flexfec_receiver.h @@ -23,16 +23,6 @@ namespace webrtc { -// Callback interface for packets recovered by FlexFEC. The implementation -// should be able to demultiplex the recovered RTP packets based on SSRC. -class RecoveredPacketReceiver { - public: - virtual void OnRecoveredPacket(const uint8_t* packet, size_t length) = 0; - - protected: - virtual ~RecoveredPacketReceiver() = default; -}; - class FlexfecReceiver { public: FlexfecReceiver(uint32_t ssrc, diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 11bc9433a4..81b46f8abf 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -195,9 +195,17 @@ class RtpData { virtual int32_t OnReceivedPayloadData(const uint8_t* payload_data, size_t payload_size, const WebRtcRTPHeader* rtp_header) = 0; +}; - virtual bool OnRecoveredPacket(const uint8_t* packet, - size_t packet_length) = 0; +// Callback interface for packets recovered by FlexFEC or ULPFEC. In +// the FlexFEC case, the implementation should be able to demultiplex +// the recovered RTP packets based on SSRC. +class RecoveredPacketReceiver { + public: + virtual void OnRecoveredPacket(const uint8_t* packet, size_t length) = 0; + + protected: + virtual ~RecoveredPacketReceiver() = default; }; class RtpFeedback { @@ -400,10 +408,6 @@ class NullRtpData : public RtpData { const WebRtcRTPHeader* rtp_header) override { return 0; } - - bool OnRecoveredPacket(const uint8_t* packet, size_t packet_length) override { - return true; - } }; // Statistics about packet loss for a single directional connection. All values diff --git a/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h b/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h index af416f9469..de4c4f1b80 100644 --- a/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h +++ b/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h @@ -31,7 +31,7 @@ struct FecPacketCounter { class UlpfecReceiver { public: - static UlpfecReceiver* Create(RtpData* callback); + static UlpfecReceiver* Create(RecoveredPacketReceiver* callback); virtual ~UlpfecReceiver() {} diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc index 2752a378c9..dad445dab0 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc @@ -21,11 +21,11 @@ namespace webrtc { -UlpfecReceiver* UlpfecReceiver::Create(RtpData* callback) { +UlpfecReceiver* UlpfecReceiver::Create(RecoveredPacketReceiver* callback) { return new UlpfecReceiverImpl(callback); } -UlpfecReceiverImpl::UlpfecReceiverImpl(RtpData* callback) +UlpfecReceiverImpl::UlpfecReceiverImpl(RecoveredPacketReceiver* callback) : recovered_packet_callback_(callback), fec_(ForwardErrorCorrection::CreateUlpfec()) {} @@ -212,10 +212,8 @@ int32_t UlpfecReceiverImpl::ProcessReceivedFec() { if (!received_packets_.front()->is_fec) { ForwardErrorCorrection::Packet* packet = received_packets_.front()->pkt; crit_sect_.Leave(); - if (!recovered_packet_callback_->OnRecoveredPacket(packet->data, - packet->length)) { - return -1; - } + recovered_packet_callback_->OnRecoveredPacket(packet->data, + packet->length); crit_sect_.Enter(); } if (fec_->DecodeFec(&received_packets_, &recovered_packets_) != 0) { @@ -233,10 +231,8 @@ int32_t UlpfecReceiverImpl::ProcessReceivedFec() { ForwardErrorCorrection::Packet* packet = recovered_packet->pkt; ++packet_counter_.num_recovered_packets; crit_sect_.Leave(); - if (!recovered_packet_callback_->OnRecoveredPacket(packet->data, - packet->length)) { - return -1; - } + recovered_packet_callback_->OnRecoveredPacket(packet->data, + packet->length); crit_sect_.Enter(); recovered_packet->returned = true; } diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h index 8dbc8af687..e24570c1cd 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.h @@ -23,7 +23,7 @@ namespace webrtc { class UlpfecReceiverImpl : public UlpfecReceiver { public: - explicit UlpfecReceiverImpl(RtpData* callback); + explicit UlpfecReceiverImpl(RecoveredPacketReceiver* callback); virtual ~UlpfecReceiverImpl(); int32_t AddReceivedRedPacket(const RTPHeader& rtp_header, @@ -37,7 +37,7 @@ class UlpfecReceiverImpl : public UlpfecReceiver { private: rtc::CriticalSection crit_sect_; - RtpData* recovered_packet_callback_; + RecoveredPacketReceiver* recovered_packet_callback_; std::unique_ptr fec_; // TODO(holmer): In the current version |received_packets_| is never more // than one packet, since we process FEC every time a new packet diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc index ddca65530f..6f9243429d 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc @@ -16,6 +16,7 @@ #include "webrtc/modules/rtp_rtcp/include/rtp_header_parser.h" #include "webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h" #include "webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h" +#include "webrtc/modules/rtp_rtcp/mocks/mock_recovered_packet_receiver.h" #include "webrtc/modules/rtp_rtcp/source/byte_io.h" #include "webrtc/modules/rtp_rtcp/source/fec_test_helper.h" #include "webrtc/modules/rtp_rtcp/source/forward_error_correction.h" @@ -36,13 +37,19 @@ using test::fec::UlpfecPacketGenerator; constexpr int kFecPayloadType = 96; constexpr uint32_t kMediaSsrc = 835424; + +class NullRecoveredPacketReceiver : public RecoveredPacketReceiver { + public: + void OnRecoveredPacket(const uint8_t* packet, size_t length) override {} +}; + } // namespace class UlpfecReceiverTest : public ::testing::Test { protected: UlpfecReceiverTest() : fec_(ForwardErrorCorrection::CreateUlpfec()), - receiver_fec_(UlpfecReceiver::Create(&rtp_data_callback_)), + receiver_fec_(UlpfecReceiver::Create(&recovered_packet_receiver_)), packet_generator_(kMediaSsrc) {} // Generates |num_fec_packets| FEC packets, given |media_packets|. @@ -64,7 +71,7 @@ class UlpfecReceiverTest : public ::testing::Test { // to the receiver. void BuildAndAddRedFecPacket(Packet* packet); - // Ensure that |rtp_data_callback_| will be called correctly + // Ensure that |recovered_packet_receiver_| will be called correctly // and that the recovered packet will be identical to the lost packet. void VerifyReconstructedMediaPacket(const AugmentedPacket& packet, size_t times); @@ -75,7 +82,7 @@ class UlpfecReceiverTest : public ::testing::Test { size_t length, uint8_t ulpfec_payload_type); - MockRtpData rtp_data_callback_; + MockRecoveredPacketReceiver recovered_packet_receiver_; std::unique_ptr fec_; std::unique_ptr receiver_fec_; UlpfecPacketGenerator packet_generator_; @@ -134,15 +141,13 @@ void UlpfecReceiverTest::VerifyReconstructedMediaPacket( // Verify that the content of the reconstructed packet is equal to the // content of |packet|, and that the same content is received |times| number // of times in a row. - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, packet.length)) + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, packet.length)) .With(Args<0, 1>(ElementsAreArray(packet.data, packet.length))) - .Times(times) - .WillRepeatedly(Return(true)); + .Times(times); } void UlpfecReceiverTest::InjectGarbagePacketLength(size_t fec_garbage_offset) { - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _)) - .WillRepeatedly(Return(true)); + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, _)); const size_t kNumFecPackets = 1; std::list augmented_media_packets; @@ -172,7 +177,7 @@ void UlpfecReceiverTest::SurvivesMaliciousPacket(const uint8_t* data, std::unique_ptr parser(RtpHeaderParser::Create()); ASSERT_TRUE(parser->Parse(data, length, &header)); - NullRtpData null_callback; + NullRecoveredPacketReceiver null_callback; std::unique_ptr receiver_fec( UlpfecReceiver::Create(&null_callback)); @@ -345,9 +350,8 @@ TEST_F(UlpfecReceiverTest, PacketNotDroppedTooEarly) { EncodeFec(media_packets_batch1, kNumFecPacketsBatch1, &fec_packets); BuildAndAddRedMediaPacket(augmented_media_packets_batch1.front()); - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _)) - .Times(1) - .WillRepeatedly(Return(true)); + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, _)) + .Times(1); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); delayed_fec = fec_packets.front(); @@ -362,17 +366,15 @@ TEST_F(UlpfecReceiverTest, PacketNotDroppedTooEarly) { for (auto it = augmented_media_packets_batch2.begin(); it != augmented_media_packets_batch2.end(); ++it) { BuildAndAddRedMediaPacket(*it); - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _)) - .Times(1) - .WillRepeatedly(Return(true)); + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, _)) + .Times(1); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); } // Add the delayed FEC packet. One packet should be reconstructed. BuildAndAddRedFecPacket(delayed_fec); - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _)) - .Times(1) - .WillRepeatedly(Return(true)); + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, _)) + .Times(1); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); } @@ -390,9 +392,8 @@ TEST_F(UlpfecReceiverTest, PacketDroppedWhenTooOld) { EncodeFec(media_packets_batch1, kNumFecPacketsBatch1, &fec_packets); BuildAndAddRedMediaPacket(augmented_media_packets_batch1.front()); - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _)) - .Times(1) - .WillRepeatedly(Return(true)); + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, _)) + .Times(1); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); delayed_fec = fec_packets.front(); @@ -407,16 +408,15 @@ TEST_F(UlpfecReceiverTest, PacketDroppedWhenTooOld) { for (auto it = augmented_media_packets_batch2.begin(); it != augmented_media_packets_batch2.end(); ++it) { BuildAndAddRedMediaPacket(*it); - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _)) - .Times(1) - .WillRepeatedly(Return(true)); + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, _)) + .Times(1); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); } // Add the delayed FEC packet. No packet should be reconstructed since the // first media packet of that frame has been dropped due to being too old. BuildAndAddRedFecPacket(delayed_fec); - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _)).Times(0); + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, _)).Times(0); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); } @@ -435,7 +435,7 @@ TEST_F(UlpfecReceiverTest, OldFecPacketDropped) { for (auto it = fec_packets.begin(); it != fec_packets.end(); ++it) { // Only FEC packets inserted. No packets recoverable at this time. BuildAndAddRedFecPacket(*it); - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _)).Times(0); + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, _)).Times(0); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); } // Move unique_ptr's to media_packets for lifetime management. @@ -450,9 +450,8 @@ TEST_F(UlpfecReceiverTest, OldFecPacketDropped) { // and should have been dropped. Only the media packet we inserted will be // returned. BuildAndAddRedMediaPacket(augmented_media_packets.front()); - EXPECT_CALL(rtp_data_callback_, OnRecoveredPacket(_, _)) - .Times(1) - .WillRepeatedly(Return(true)); + EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, _)) + .Times(1); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); } diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index 99d0cc893f..78057771d6 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -277,17 +277,17 @@ int32_t RtpStreamReceiver::OnReceivedPayloadData( } // TODO(nisse): Try to delete this method. Obstacles: It is used by -// ParseAndHandleEncapsulatingHeader, for handling Rtx packets. And -// it's part of the RtpData interface which we implement. -bool RtpStreamReceiver::OnRecoveredPacket(const uint8_t* rtp_packet, +// ParseAndHandleEncapsulatingHeader, for handling Rtx packets, and +// for callbacks from |ulpfec_receiver_|. +void RtpStreamReceiver::OnRecoveredPacket(const uint8_t* rtp_packet, size_t rtp_packet_length) { RTPHeader header; if (!rtp_header_parser_->Parse(rtp_packet, rtp_packet_length, &header)) { - return false; + return; } header.payload_type_frequency = kVideoPayloadTypeFrequency; bool in_order = IsPacketInOrder(header); - return ReceivePacket(rtp_packet, rtp_packet_length, header, in_order); + ReceivePacket(rtp_packet, rtp_packet_length, header, in_order); } // TODO(pbos): Remove as soon as audio can handle a changing payload type @@ -428,13 +428,13 @@ rtc::Optional RtpStreamReceiver::LastReceivedKeyframePacketMs() const { return packet_buffer_->LastReceivedKeyframePacketMs(); } -// TODO(nisse): Drop return value. -bool RtpStreamReceiver::ReceivePacket(const uint8_t* packet, +void RtpStreamReceiver::ReceivePacket(const uint8_t* packet, size_t packet_length, const RTPHeader& header, bool in_order) { if (rtp_payload_registry_.IsEncapsulated(header)) { - return ParseAndHandleEncapsulatingHeader(packet, packet_length, header); + ParseAndHandleEncapsulatingHeader(packet, packet_length, header); + return; } const uint8_t* payload = packet + header.headerLength; assert(packet_length >= header.headerLength); @@ -442,13 +442,13 @@ bool RtpStreamReceiver::ReceivePacket(const uint8_t* packet, PayloadUnion payload_specific; if (!rtp_payload_registry_.GetPayloadSpecifics(header.payloadType, &payload_specific)) { - return false; + return; } - return rtp_receiver_->IncomingRtpPacket(header, payload, payload_length, - payload_specific, in_order); + rtp_receiver_->IncomingRtpPacket(header, payload, payload_length, + payload_specific, in_order); } -bool RtpStreamReceiver::ParseAndHandleEncapsulatingHeader( +void RtpStreamReceiver::ParseAndHandleEncapsulatingHeader( const uint8_t* packet, size_t packet_length, const RTPHeader& header) { if (rtp_payload_registry_.IsRed(header)) { int8_t ulpfec_pt = rtp_payload_registry_.ulpfec_payload_type(); @@ -460,24 +460,24 @@ bool RtpStreamReceiver::ParseAndHandleEncapsulatingHeader( } if (ulpfec_receiver_->AddReceivedRedPacket(header, packet, packet_length, ulpfec_pt) != 0) { - return false; + return; } - return ulpfec_receiver_->ProcessReceivedFec() == 0; + ulpfec_receiver_->ProcessReceivedFec(); } else if (rtp_payload_registry_.IsRtx(header)) { if (header.headerLength + header.paddingLength == packet_length) { // This is an empty packet and should be silently dropped before trying to // parse the RTX header. - return true; + return; } // Remove the RTX header and parse the original RTP header. if (packet_length < header.headerLength) - return false; + return; if (packet_length > sizeof(restored_packet_)) - return false; + return; rtc::CritScope lock(&receive_cs_); if (restored_packet_in_use_) { LOG(LS_WARNING) << "Multiple RTX headers detected, dropping packet."; - return false; + return; } if (!rtp_payload_registry_.RestoreOriginalPacket( restored_packet_, packet, &packet_length, rtp_receiver_->SSRC(), @@ -485,14 +485,12 @@ bool RtpStreamReceiver::ParseAndHandleEncapsulatingHeader( LOG(LS_WARNING) << "Incoming RTX packet: Invalid RTP header ssrc: " << header.ssrc << " payload type: " << static_cast(header.payloadType); - return false; + return; } restored_packet_in_use_ = true; - bool ret = OnRecoveredPacket(restored_packet_, packet_length); + OnRecoveredPacket(restored_packet_, packet_length); restored_packet_in_use_ = false; - return ret; } - return false; } void RtpStreamReceiver::NotifyReceiverOfFecPacket(const RTPHeader& header) { diff --git a/webrtc/video/rtp_stream_receiver.h b/webrtc/video/rtp_stream_receiver.h index 14edb61388..f6b36faeea 100644 --- a/webrtc/video/rtp_stream_receiver.h +++ b/webrtc/video/rtp_stream_receiver.h @@ -56,6 +56,7 @@ class VideoReceiver; } // namespace vcm class RtpStreamReceiver : public RtpData, + public RecoveredPacketReceiver, public RtpFeedback, public VCMFrameTypeCallback, public VCMPacketRequestCallback, @@ -102,7 +103,8 @@ class RtpStreamReceiver : public RtpData, int32_t OnReceivedPayloadData(const uint8_t* payload_data, size_t payload_size, const WebRtcRTPHeader* rtp_header) override; - bool OnRecoveredPacket(const uint8_t* packet, size_t packet_length) override; + // Implements RecoveredPacketReceiver. + void OnRecoveredPacket(const uint8_t* packet, size_t packet_length) override; // Implements RtpFeedback. int32_t OnInitializeDecoder(int8_t payload_type, @@ -140,13 +142,13 @@ class RtpStreamReceiver : public RtpData, private: bool AddReceiveCodec(const VideoCodec& video_codec); - bool ReceivePacket(const uint8_t* packet, + void ReceivePacket(const uint8_t* packet, size_t packet_length, const RTPHeader& header, bool in_order); // Parses and handles for instance RTX and RED headers. // This function assumes that it's being called from only one thread. - bool ParseAndHandleEncapsulatingHeader(const uint8_t* packet, + void ParseAndHandleEncapsulatingHeader(const uint8_t* packet, size_t packet_length, const RTPHeader& header); void NotifyReceiverOfFecPacket(const RTPHeader& header); diff --git a/webrtc/voice_engine/channel.h b/webrtc/voice_engine/channel.h index b7b59a447c..8c9d623b19 100644 --- a/webrtc/voice_engine/channel.h +++ b/webrtc/voice_engine/channel.h @@ -316,7 +316,6 @@ class Channel int32_t OnReceivedPayloadData(const uint8_t* payloadData, size_t payloadSize, const WebRtcRTPHeader* rtpHeader) override; - bool OnRecoveredPacket(const uint8_t* packet, size_t packet_length) override; // From RtpFeedback in the RTP/RTCP module int32_t OnInitializeDecoder(int8_t payloadType, @@ -417,6 +416,8 @@ class Channel bool OnRtpPacketWithHeader(const uint8_t* received_packet, size_t length, RTPHeader *header); + bool OnRecoveredPacket(const uint8_t* packet, size_t packet_length); + bool ReceivePacket(const uint8_t* packet, size_t packet_length, const RTPHeader& header,