diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index e594dcc6ca..d178406b29 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -137,7 +137,7 @@ class Call : public webrtc::Call, const PacketTime& packet_time) override; // Implements RecoveredPacketReceiver. - bool OnRecoveredPacket(const uint8_t* packet, size_t length) override; + void OnRecoveredPacket(const uint8_t* packet, size_t length) override; void SetBitrateConfig( const webrtc::Call::Config::BitrateConfig& bitrate_config) override; @@ -179,7 +179,7 @@ class Call : public webrtc::Call, rtc::Optional ParseRtpPacket(const uint8_t* packet, size_t length, - const PacketTime& packet_time) + const PacketTime* packet_time) SHARED_LOCKS_REQUIRED(receive_crit_); void UpdateSendHistograms(int64_t first_sent_packet_ms) @@ -409,7 +409,7 @@ Call::~Call() { rtc::Optional Call::ParseRtpPacket( const uint8_t* packet, size_t length, - const PacketTime& packet_time) { + const PacketTime* packet_time) { RtpPacketReceived parsed_packet; if (!parsed_packet.Parse(packet, length)) return rtc::Optional(); @@ -419,8 +419,8 @@ rtc::Optional Call::ParseRtpPacket( parsed_packet.IdentifyExtensions(it->second.extensions); int64_t arrival_time_ms; - if (packet_time.timestamp != -1) { - arrival_time_ms = (packet_time.timestamp + 500) / 1000; + if (packet_time && packet_time->timestamp != -1) { + arrival_time_ms = (packet_time->timestamp + 500) / 1000; } else { arrival_time_ms = clock_->TimeInMilliseconds(); } @@ -1189,7 +1189,7 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type, // TODO(nisse): We should parse the RTP header only here, and pass // on parsed_packet to the receive streams. rtc::Optional parsed_packet = - ParseRtpPacket(packet, length, packet_time); + ParseRtpPacket(packet, length, &packet_time); if (!parsed_packet) return DELIVERY_PACKET_ERROR; @@ -1255,13 +1255,20 @@ PacketReceiver::DeliveryStatus Call::DeliverPacket( // TODO(brandtr): Update this member function when we support protecting // audio packets with FlexFEC. -bool Call::OnRecoveredPacket(const uint8_t* packet, size_t length) { - uint32_t ssrc = ByteReader::ReadBigEndian(&packet[8]); +void Call::OnRecoveredPacket(const uint8_t* packet, size_t length) { ReadLockScoped read_lock(*receive_crit_); - auto it = video_receive_ssrcs_.find(ssrc); + rtc::Optional parsed_packet = + ParseRtpPacket(packet, length, nullptr); + if (!parsed_packet) + return; + + parsed_packet->set_recovered(true); + + auto it = video_receive_ssrcs_.find(parsed_packet->Ssrc()); if (it == video_receive_ssrcs_.end()) - return false; - return it->second->OnRecoveredPacket(packet, length); + return; + + it->second->OnRtpPacket(*parsed_packet); } void Call::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet, diff --git a/webrtc/modules/rtp_rtcp/include/flexfec_receiver.h b/webrtc/modules/rtp_rtcp/include/flexfec_receiver.h index 67dc813bc4..a31a285968 100644 --- a/webrtc/modules/rtp_rtcp/include/flexfec_receiver.h +++ b/webrtc/modules/rtp_rtcp/include/flexfec_receiver.h @@ -27,7 +27,7 @@ namespace webrtc { // should be able to demultiplex the recovered RTP packets based on SSRC. class RecoveredPacketReceiver { public: - virtual bool OnRecoveredPacket(const uint8_t* packet, size_t length) = 0; + virtual void OnRecoveredPacket(const uint8_t* packet, size_t length) = 0; protected: virtual ~RecoveredPacketReceiver() = default; diff --git a/webrtc/modules/rtp_rtcp/mocks/mock_recovered_packet_receiver.h b/webrtc/modules/rtp_rtcp/mocks/mock_recovered_packet_receiver.h index fcc637b044..d79034480e 100644 --- a/webrtc/modules/rtp_rtcp/mocks/mock_recovered_packet_receiver.h +++ b/webrtc/modules/rtp_rtcp/mocks/mock_recovered_packet_receiver.h @@ -19,7 +19,7 @@ namespace webrtc { class MockRecoveredPacketReceiver : public RecoveredPacketReceiver { public: - MOCK_METHOD2(OnRecoveredPacket, bool(const uint8_t* packet, size_t length)); + MOCK_METHOD2(OnRecoveredPacket, void(const uint8_t* packet, size_t length)); }; } // namespace webrtc diff --git a/webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc b/webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc index 81c57850a1..47534e480e 100644 --- a/webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc +++ b/webrtc/modules/rtp_rtcp/source/flexfec_receiver.cc @@ -132,10 +132,8 @@ bool FlexfecReceiver::ProcessReceivedPackets() { continue; } ++packet_counter_.num_recovered_packets; - if (!recovered_packet_receiver_->OnRecoveredPacket( - recovered_packet->pkt->data, recovered_packet->pkt->length)) { - return false; - } + recovered_packet_receiver_->OnRecoveredPacket( + recovered_packet->pkt->data, recovered_packet->pkt->length); recovered_packet->returned = true; // Periodically log the incoming packets. int64_t now_ms = clock_->TimeInMilliseconds(); diff --git a/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc index 01d9a59cee..508521da5d 100644 --- a/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/flexfec_receiver_unittest.cc @@ -227,8 +227,7 @@ TEST_F(FlexfecReceiverTest, RecoversFromSingleMediaLoss) { EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, (*media_it)->length)) .With( - Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))) - .WillOnce(Return(true)); + Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))); receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header)); } @@ -250,8 +249,7 @@ TEST_F(FlexfecReceiverTest, RecoversFromDoubleMediaLoss) { EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, (*media_it)->length)) .With( - Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))) - .WillOnce(Return(true)); + Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))); receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header)); // Receive second FEC packet and recover second lost media packet. @@ -261,8 +259,7 @@ TEST_F(FlexfecReceiverTest, RecoversFromDoubleMediaLoss) { EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, (*media_it)->length)) .With( - Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))) - .WillOnce(Return(true)); + Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))); receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header)); } @@ -301,8 +298,7 @@ TEST_F(FlexfecReceiverTest, DoesNotCallbackTwice) { EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, (*media_it)->length)) .With( - Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))) - .WillOnce(Return(true)); + Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))); receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header)); // Receive FEC packet again. @@ -348,8 +344,7 @@ TEST_F(FlexfecReceiverTest, RecoversFrom50PercentLoss) { EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, (*media_it)->length)) .With(Args<0, 1>( - ElementsAreArray((*media_it)->data, (*media_it)->length))) - .WillOnce(Return(true)); + ElementsAreArray((*media_it)->data, (*media_it)->length))); receiver_.OnRtpPacket(ParsePacket(*fec_packet_with_rtp_header)); ++media_it; } @@ -389,8 +384,7 @@ TEST_F(FlexfecReceiverTest, DelayedFecPacketDoesHelp) { EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, (*media_it)->length)) .With( - Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))) - .WillOnce(Return(true)); + Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))); receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header)); } @@ -454,13 +448,11 @@ TEST_F(FlexfecReceiverTest, RecoversWithMediaPacketsOutOfOrder) { EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, (*media_packet1)->length)) .With(Args<0, 1>( - ElementsAreArray((*media_packet1)->data, (*media_packet1)->length))) - .WillOnce(Return(true)); + ElementsAreArray((*media_packet1)->data, (*media_packet1)->length))); EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, (*media_packet4)->length)) .With(Args<0, 1>( - ElementsAreArray((*media_packet4)->data, (*media_packet4)->length))) - .WillOnce(Return(true)); + ElementsAreArray((*media_packet4)->data, (*media_packet4)->length))); // Add FEC packets. auto fec_it = fec_packets.begin(); @@ -492,8 +484,7 @@ TEST_F(FlexfecReceiverTest, CalculatesNumberOfPackets) { EXPECT_CALL(recovered_packet_receiver_, OnRecoveredPacket(_, (*media_it)->length)) .With( - Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))) - .WillOnce(Return(true)); + Args<0, 1>(ElementsAreArray((*media_it)->data, (*media_it)->length))); receiver_.OnRtpPacket(ParsePacket(*packet_with_rtp_header)); // Check stats calculations. diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_received.h b/webrtc/modules/rtp_rtcp/source/rtp_packet_received.h index 95674cf863..68c33e9387 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_packet_received.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_received.h @@ -36,9 +36,9 @@ class RtpPacketReceived : public rtp::Packet { NtpTime capture_ntp_time() const { return capture_time_; } void set_capture_ntp_time(NtpTime time) { capture_time_ = time; } - // Flag if packet arrived via rtx. - bool retransmit() const { return retransmit_; } - void set_retransmit(bool value) { retransmit_ = value; } + // Flag if packet was recovered via RTX or FEC. + bool recovered() const { return recovered_; } + void set_recovered(bool value) { recovered_ = value; } int payload_type_frequency() const { return payload_type_frequency_; } void set_payload_type_frequency(int value) { @@ -49,7 +49,7 @@ class RtpPacketReceived : public rtp::Packet { NtpTime capture_time_; int64_t arrival_time_ms_ = 0; int payload_type_frequency_ = 0; - bool retransmit_ = false; + bool recovered_ = false; }; } // namespace webrtc diff --git a/webrtc/test/fuzzers/flexfec_receiver_fuzzer.cc b/webrtc/test/fuzzers/flexfec_receiver_fuzzer.cc index 58c1a8d8d8..d38b8a2007 100644 --- a/webrtc/test/fuzzers/flexfec_receiver_fuzzer.cc +++ b/webrtc/test/fuzzers/flexfec_receiver_fuzzer.cc @@ -20,7 +20,7 @@ namespace webrtc { namespace { class DummyCallback : public RecoveredPacketReceiver { - bool OnRecoveredPacket(const uint8_t* packet, size_t length) { return true; } + void OnRecoveredPacket(const uint8_t* packet, size_t length) override {} }; } // namespace diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index 88d86ef986..53aa1ef3c1 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -275,6 +275,9 @@ int32_t RtpStreamReceiver::OnReceivedPayloadData( return 0; } +// 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, size_t rtp_packet_length) { RTPHeader header; @@ -302,36 +305,37 @@ void RtpStreamReceiver::OnIncomingSSRCChanged(const uint32_t ssrc) { rtp_rtcp_->SetRemoteSSRC(ssrc); } +// This method handles both regular RTP packets and packets recovered +// via FlexFEC. void RtpStreamReceiver::OnRtpPacket(const RtpPacketReceived& packet) { { rtc::CritScope lock(&receive_cs_); if (!receiving_) { return; } - } - int64_t now_ms = clock_->TimeInMilliseconds(); + if (!packet.recovered()) { + int64_t now_ms = clock_->TimeInMilliseconds(); - { - // Periodically log the RTP header of incoming packets. - rtc::CritScope lock(&receive_cs_); - if (now_ms - last_packet_log_ms_ > kPacketLogIntervalMs) { - std::stringstream ss; - ss << "Packet received on SSRC: " << packet.Ssrc() - << " with payload type: " << static_cast(packet.PayloadType()) - << ", timestamp: " << packet.Timestamp() - << ", sequence number: " << packet.SequenceNumber() - << ", arrival time: " << packet.arrival_time_ms(); - int32_t time_offset; - if (packet.GetExtension(&time_offset)) { - ss << ", toffset: " << time_offset; + // Periodically log the RTP header of incoming packets. + if (now_ms - last_packet_log_ms_ > kPacketLogIntervalMs) { + std::stringstream ss; + ss << "Packet received on SSRC: " << packet.Ssrc() + << " with payload type: " << static_cast(packet.PayloadType()) + << ", timestamp: " << packet.Timestamp() + << ", sequence number: " << packet.SequenceNumber() + << ", arrival time: " << packet.arrival_time_ms(); + int32_t time_offset; + if (packet.GetExtension(&time_offset)) { + ss << ", toffset: " << time_offset; + } + uint32_t send_time; + if (packet.GetExtension(&send_time)) { + ss << ", abs send time: " << send_time; + } + LOG(LS_INFO) << ss.str(); + last_packet_log_ms_ = now_ms; } - uint32_t send_time; - if (packet.GetExtension(&send_time)) { - ss << ", abs send time: " << send_time; - } - LOG(LS_INFO) << ss.str(); - last_packet_log_ms_ = now_ms; } } @@ -343,13 +347,20 @@ void RtpStreamReceiver::OnRtpPacket(const RtpPacketReceived& packet) { header.payload_type_frequency = kVideoPayloadTypeFrequency; bool in_order = IsPacketInOrder(header); - rtp_payload_registry_.SetIncomingPayloadType(header); + if (!packet.recovered()) { + // TODO(nisse): Why isn't this done for recovered packets? + rtp_payload_registry_.SetIncomingPayloadType(header); + } ReceivePacket(packet.data(), packet.size(), header, in_order); // Update receive statistics after ReceivePacket. // Receive statistics will be reset if the payload type changes (make sure // that the first packet is included in the stats). - rtp_receive_statistics_->IncomingPacket( - header, packet.size(), IsPacketRetransmitted(header, in_order)); + if (!packet.recovered()) { + // TODO(nisse): We should pass a recovered flag to stats, to aid + // fixing bug bugs.webrtc.org/6339. + rtp_receive_statistics_->IncomingPacket( + header, packet.size(), IsPacketRetransmitted(header, in_order)); + } } int32_t RtpStreamReceiver::RequestKeyFrame() { diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc index cee62d297c..0ce3794eb5 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/webrtc/video/video_receive_stream.cc @@ -246,11 +246,6 @@ void VideoReceiveStream::OnRtpPacket(const RtpPacketReceived& packet) { rtp_stream_receiver_.OnRtpPacket(packet); } -bool VideoReceiveStream::OnRecoveredPacket(const uint8_t* packet, - size_t length) { - return rtp_stream_receiver_.OnRecoveredPacket(packet, length); -} - void VideoReceiveStream::SetSync(Syncable* audio_syncable) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); rtp_stream_sync_.ConfigureSync(audio_syncable); diff --git a/webrtc/video/video_receive_stream.h b/webrtc/video/video_receive_stream.h index 73bacdd520..ca797e2a3e 100644 --- a/webrtc/video/video_receive_stream.h +++ b/webrtc/video/video_receive_stream.h @@ -60,8 +60,6 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, void SignalNetworkState(NetworkState state); bool DeliverRtcp(const uint8_t* packet, size_t length); - bool OnRecoveredPacket(const uint8_t* packet, size_t length); - void SetSync(Syncable* audio_syncable); // Implements webrtc::VideoReceiveStream. diff --git a/webrtc/video/video_receive_stream_unittest.cc b/webrtc/video/video_receive_stream_unittest.cc index d536bf1dc6..a6a3ab28a0 100644 --- a/webrtc/video/video_receive_stream_unittest.cc +++ b/webrtc/video/video_receive_stream_unittest.cc @@ -129,9 +129,9 @@ TEST_F(VideoReceiveStreamTest, CreateFrameFromH264FmtpSpropAndIdr) { EXPECT_CALL(mock_h264_video_decoder_, RegisterDecodeCompleteCallback(_)); video_receive_stream_->Start(); EXPECT_CALL(mock_h264_video_decoder_, Decode(_, false, _, _, _)); - EXPECT_EQ(true, - video_receive_stream_->OnRecoveredPacket(rtppacket.data(), - rtppacket.size())); + RtpPacketReceived parsed_packet; + ASSERT_TRUE(parsed_packet.Parse(rtppacket.data(), rtppacket.size())); + video_receive_stream_->OnRtpPacket(parsed_packet); EXPECT_CALL(mock_h264_video_decoder_, Release()); // Make sure the decoder thread had a chance to run. init_decode_event_.Wait(kDefaultTimeOutMs);