From dbb988b016c3037d883f17542fc8c5ac474e524c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Thu, 15 Nov 2018 08:05:16 +0100 Subject: [PATCH] Change ReceiveStatistics to implement RtpPacketSinkInterface, part 2. Delete the deprecated IncomingPacket method, and convert implementation to use RtpPacketReceived rather than RTPHeader. Part 1 was https://webrtc-review.googlesource.com/c/src/+/100104 Bug: webrtc:7135, webrtc:8016 Change-Id: Ib4840d947870403deea2f9067f847e4b0f182479 Reviewed-on: https://webrtc-review.googlesource.com/c/6762 Commit-Queue: Niels Moller Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#25648} --- modules/rtp_rtcp/include/receive_statistics.h | 7 -- modules/rtp_rtcp/include/rtp_rtcp_defines.cc | 8 ++ modules/rtp_rtcp/include/rtp_rtcp_defines.h | 10 +-- .../source/receive_statistics_impl.cc | 77 ++++++++----------- .../rtp_rtcp/source/receive_statistics_impl.h | 23 +++--- modules/rtp_rtcp/source/rtp_sender.cc | 14 +--- 6 files changed, 59 insertions(+), 80 deletions(-) diff --git a/modules/rtp_rtcp/include/receive_statistics.h b/modules/rtp_rtcp/include/receive_statistics.h index 24d6f81237..ab68ee0f69 100644 --- a/modules/rtp_rtcp/include/receive_statistics.h +++ b/modules/rtp_rtcp/include/receive_statistics.h @@ -56,13 +56,6 @@ class ReceiveStatistics : public ReceiveStatisticsProvider, static ReceiveStatistics* Create(Clock* clock); - // Updates the receive statistics with this packet. - // TODO(bugs.webrtc.org/8016): Deprecated. Delete as soon as - // downstream code is updated to use OnRtpPacket. - RTC_DEPRECATED - virtual void IncomingPacket(const RTPHeader& rtp_header, - size_t packet_length) = 0; - // Increment counter for number of FEC packets received. virtual void FecPacketReceived(const RtpPacketReceived& packet) = 0; diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.cc b/modules/rtp_rtcp/include/rtp_rtcp_defines.cc index 1da8ade774..d743f52f7e 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.cc +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.cc @@ -9,6 +9,7 @@ */ #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "modules/rtp_rtcp/source/rtp_packet.h" #include #include @@ -130,4 +131,11 @@ bool PacketFeedback::operator==(const PacketFeedback& rhs) const { payload_size == rhs.payload_size && pacing_info == rhs.pacing_info; } +void RtpPacketCounter::AddPacket(const RtpPacket& packet) { + ++packets; + header_bytes += packet.headers_size(); + padding_bytes += packet.padding_size(); + payload_bytes += packet.payload_size(); +} + } // namespace webrtc diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index b0ff13d123..54f7eb861e 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -27,6 +27,7 @@ #define IP_PACKET_SIZE 1500 // we assume ethernet namespace webrtc { +class RtpPacket; namespace rtcp { class TransportFeedback; } @@ -449,13 +450,8 @@ struct RtpPacketCounter { packets -= other.packets; } - void AddPacket(size_t packet_length, const RTPHeader& header) { - ++packets; - header_bytes += header.headerLength; - padding_bytes += header.paddingLength; - payload_bytes += - packet_length - (header.headerLength + header.paddingLength); - } + // Not inlined, since use of RtpPacket would result in circular includes. + void AddPacket(const RtpPacket& packet); size_t TotalBytes() const { return header_bytes + payload_bytes + padding_bytes; diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index abe028d670..876e11e3f8 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc @@ -57,33 +57,31 @@ StreamStatisticianImpl::StreamStatisticianImpl( StreamStatisticianImpl::~StreamStatisticianImpl() = default; -void StreamStatisticianImpl::IncomingPacket(const RTPHeader& header, - size_t packet_length) { +void StreamStatisticianImpl::OnRtpPacket(const RtpPacketReceived& packet) { StreamDataCounters counters; { rtc::CritScope cs(&stream_lock_); bool retransmitted = - enable_retransmit_detection_ && IsRetransmitOfOldPacket(header); - counters = UpdateCounters(header, packet_length, retransmitted); + enable_retransmit_detection_ && IsRetransmitOfOldPacket(packet); + counters = UpdateCounters(packet, retransmitted); } rtp_callback_->DataCountersUpdated(counters, ssrc_); } StreamDataCounters StreamStatisticianImpl::UpdateCounters( - const RTPHeader& header, - size_t packet_length, + const RtpPacketReceived& packet, bool retransmitted) { - bool in_order = InOrderPacketInternal(header.sequenceNumber); - RTC_DCHECK_EQ(ssrc_, header.ssrc); - incoming_bitrate_.Update(packet_length, clock_->TimeInMilliseconds()); - receive_counters_.transmitted.AddPacket(packet_length, header); + bool in_order = InOrderPacketInternal(packet.SequenceNumber()); + RTC_DCHECK_EQ(ssrc_, packet.Ssrc()); + incoming_bitrate_.Update(packet.size(), clock_->TimeInMilliseconds()); + receive_counters_.transmitted.AddPacket(packet); if (!in_order && retransmitted) { - receive_counters_.retransmitted.AddPacket(packet_length, header); + receive_counters_.retransmitted.AddPacket(packet); } if (receive_counters_.transmitted.packets == 1) { - received_seq_first_ = header.sequenceNumber; + received_seq_first_ = packet.SequenceNumber(); receive_counters_.first_packet_time_ms = clock_->TimeInMilliseconds(); } @@ -95,26 +93,26 @@ StreamDataCounters StreamStatisticianImpl::UpdateCounters( // Wrong if we use RetransmitOfOldPacket. if (receive_counters_.transmitted.packets > 1 && - received_seq_max_ > header.sequenceNumber) { + received_seq_max_ > packet.SequenceNumber()) { // Wrap around detected. received_seq_wraps_++; } // New max. - received_seq_max_ = header.sequenceNumber; + received_seq_max_ = packet.SequenceNumber(); // If new time stamp and more than one in-order packet received, calculate // new jitter statistics. - if (header.timestamp != last_received_timestamp_ && + if (packet.Timestamp() != last_received_timestamp_ && (receive_counters_.transmitted.packets - receive_counters_.retransmitted.packets) > 1) { - UpdateJitter(header, receive_time); + UpdateJitter(packet, receive_time); } - last_received_timestamp_ = header.timestamp; + last_received_timestamp_ = packet.Timestamp(); last_receive_time_ntp_ = receive_time; last_receive_time_ms_ = clock_->TimeInMilliseconds(); } - size_t packet_oh = header.headerLength + header.paddingLength; + size_t packet_oh = packet.headers_size() + packet.padding_size(); // Our measured overhead. Filter from RFC 5104 4.2.1.2: // avg_OH (new) = 15/16*avg_OH (old) + 1/16*pckt_OH, @@ -122,14 +120,14 @@ StreamDataCounters StreamStatisticianImpl::UpdateCounters( return receive_counters_; } -void StreamStatisticianImpl::UpdateJitter(const RTPHeader& header, +void StreamStatisticianImpl::UpdateJitter(const RtpPacketReceived& packet, NtpTime receive_time) { uint32_t receive_time_rtp = - NtpToRtp(receive_time, header.payload_type_frequency); + NtpToRtp(receive_time, packet.payload_type_frequency()); uint32_t last_receive_time_rtp = - NtpToRtp(last_receive_time_ntp_, header.payload_type_frequency); + NtpToRtp(last_receive_time_ntp_, packet.payload_type_frequency()); int32_t time_diff_samples = (receive_time_rtp - last_receive_time_rtp) - - (header.timestamp - last_received_timestamp_); + (packet.Timestamp() - last_received_timestamp_); time_diff_samples = std::abs(time_diff_samples); @@ -143,12 +141,12 @@ void StreamStatisticianImpl::UpdateJitter(const RTPHeader& header, } } -void StreamStatisticianImpl::FecPacketReceived(const RTPHeader& header, - size_t packet_length) { +void StreamStatisticianImpl::FecPacketReceived( + const RtpPacketReceived& packet) { StreamDataCounters counters; { rtc::CritScope cs(&stream_lock_); - receive_counters_.fec.AddPacket(packet_length, header); + receive_counters_.fec.AddPacket(packet); counters = receive_counters_; } rtp_callback_->DataCountersUpdated(counters, ssrc_); @@ -312,17 +310,17 @@ uint32_t StreamStatisticianImpl::BitrateReceived() const { } bool StreamStatisticianImpl::IsRetransmitOfOldPacket( - const RTPHeader& header) const { - if (InOrderPacketInternal(header.sequenceNumber)) { + const RtpPacketReceived& packet) const { + if (InOrderPacketInternal(packet.SequenceNumber())) { return false; } - uint32_t frequency_khz = header.payload_type_frequency / 1000; + uint32_t frequency_khz = packet.payload_type_frequency() / 1000; assert(frequency_khz > 0); int64_t time_diff_ms = clock_->TimeInMilliseconds() - last_receive_time_ms_; // Diff in time stamp since last received in order. - uint32_t timestamp_diff = header.timestamp - last_received_timestamp_; + uint32_t timestamp_diff = packet.Timestamp() - last_received_timestamp_; uint32_t rtp_time_stamp_diff_ms = timestamp_diff / frequency_khz; int64_t max_delay_ms = 0; @@ -374,31 +372,24 @@ ReceiveStatisticsImpl::~ReceiveStatisticsImpl() { } void ReceiveStatisticsImpl::OnRtpPacket(const RtpPacketReceived& packet) { - RTPHeader header; - packet.GetHeader(&header); - IncomingPacket(header, packet.size()); -} - -void ReceiveStatisticsImpl::IncomingPacket(const RTPHeader& header, - size_t packet_length) { StreamStatisticianImpl* impl; { rtc::CritScope cs(&receive_statistics_lock_); - auto it = statisticians_.find(header.ssrc); + auto it = statisticians_.find(packet.Ssrc()); if (it != statisticians_.end()) { impl = it->second; } else { impl = new StreamStatisticianImpl( - header.ssrc, clock_, /* enable_retransmit_detection = */ false, this, - this); - statisticians_[header.ssrc] = impl; + packet.Ssrc(), clock_, /* enable_retransmit_detection = */ false, + this, this); + statisticians_[packet.Ssrc()] = impl; } } // StreamStatisticianImpl instance is created once and only destroyed when // this whole ReceiveStatisticsImpl is destroyed. StreamStatisticianImpl has // it's own locking so don't hold receive_statistics_lock_ (potential // deadlock). - impl->IncomingPacket(header, packet_length); + impl->OnRtpPacket(packet); } void ReceiveStatisticsImpl::FecPacketReceived(const RtpPacketReceived& packet) { @@ -411,9 +402,7 @@ void ReceiveStatisticsImpl::FecPacketReceived(const RtpPacketReceived& packet) { return; impl = it->second; } - RTPHeader header; - packet.GetHeader(&header); - impl->FecPacketReceived(header, packet.size()); + impl->FecPacketReceived(packet); } StreamStatistician* ReceiveStatisticsImpl::GetStatistician( diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h index 56bfd2b4a4..2380d52b3d 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/modules/rtp_rtcp/source/receive_statistics_impl.h @@ -23,7 +23,8 @@ namespace webrtc { -class StreamStatisticianImpl : public StreamStatistician { +class StreamStatisticianImpl : public StreamStatistician, + public RtpPacketSinkInterface { public: StreamStatisticianImpl(uint32_t ssrc, Clock* clock, @@ -41,22 +42,23 @@ class StreamStatisticianImpl : public StreamStatistician { StreamDataCounters* data_counters) const override; uint32_t BitrateReceived() const override; - void IncomingPacket(const RTPHeader& rtp_header, size_t packet_length); - void FecPacketReceived(const RTPHeader& header, size_t packet_length); + // Implements RtpPacketSinkInterface + void OnRtpPacket(const RtpPacketReceived& packet) override; + + void FecPacketReceived(const RtpPacketReceived& packet); void SetMaxReorderingThreshold(int max_reordering_threshold); void EnableRetransmitDetection(bool enable); private: - bool IsRetransmitOfOldPacket(const RTPHeader& header) const + bool IsRetransmitOfOldPacket(const RtpPacketReceived& packet) const RTC_EXCLUSIVE_LOCKS_REQUIRED(stream_lock_); bool InOrderPacketInternal(uint16_t sequence_number) const RTC_EXCLUSIVE_LOCKS_REQUIRED(stream_lock_); RtcpStatistics CalculateRtcpStatistics() RTC_EXCLUSIVE_LOCKS_REQUIRED(stream_lock_); - void UpdateJitter(const RTPHeader& header, NtpTime receive_time) + void UpdateJitter(const RtpPacketReceived& packet, NtpTime receive_time) RTC_EXCLUSIVE_LOCKS_REQUIRED(stream_lock_); - StreamDataCounters UpdateCounters(const RTPHeader& rtp_header, - size_t packet_length, + StreamDataCounters UpdateCounters(const RtpPacketReceived& packet, bool retransmitted) RTC_EXCLUSIVE_LOCKS_REQUIRED(stream_lock_); @@ -102,14 +104,13 @@ class ReceiveStatisticsImpl : public ReceiveStatistics, ~ReceiveStatisticsImpl() override; - // Implement ReceiveStatisticsProvider. + // Implements ReceiveStatisticsProvider. std::vector RtcpReportBlocks(size_t max_blocks) override; - // Implement RtpPacketSinkInterface + // Implements RtpPacketSinkInterface void OnRtpPacket(const RtpPacketReceived& packet) override; - // Implement ReceiveStatistics. - void IncomingPacket(const RTPHeader& header, size_t packet_length) override; + // Implements ReceiveStatistics. void FecPacketReceived(const RtpPacketReceived& packet) override; StreamStatistician* GetStatistician(uint32_t ssrc) const override; void SetMaxReorderingThreshold(int max_reordering_threshold) override; diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 38d603092f..e80538bba6 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -95,14 +95,6 @@ const char* FrameTypeToString(FrameType frame_type) { } return ""; } - -void CountPacket(RtpPacketCounter* counter, const RtpPacketToSend& packet) { - ++counter->packets; - counter->header_bytes += packet.headers_size(); - counter->padding_bytes += packet.padding_size(); - counter->payload_bytes += packet.payload_size(); -} - } // namespace RTPSender::RTPSender( @@ -883,13 +875,13 @@ void RTPSender::UpdateRtpStats(const RtpPacketToSend& packet, counters->first_packet_time_ms = now_ms; if (IsFecPacket(packet)) - CountPacket(&counters->fec, packet); + counters->fec.AddPacket(packet); if (is_retransmit) { - CountPacket(&counters->retransmitted, packet); + counters->retransmitted.AddPacket(packet); nack_bitrate_sent_.Update(packet.size(), now_ms); } - CountPacket(&counters->transmitted, packet); + counters->transmitted.AddPacket(packet); if (rtp_stats_callback_) rtp_stats_callback_->DataCountersUpdated(*counters, packet.Ssrc());