From c30e9e230065ddde4cc439d9ba430273413e70d7 Mon Sep 17 00:00:00 2001 From: "sprang@webrtc.org" Date: Mon, 8 Sep 2014 08:20:18 +0000 Subject: [PATCH] Ignore FEC packet in stats, if it is first packet on ssrc. BUG=chrome:410456 R=mflodman@webrtc.org, tommi@webrtc.org Review URL: https://webrtc-codereview.appspot.com/22309004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7096 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../source/receive_statistics_impl.cc | 37 ++++---- .../rtp_rtcp/source/receive_statistics_impl.h | 2 - .../source/receive_statistics_unittest.cc | 88 ++++++++++++------- 3 files changed, 70 insertions(+), 57 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc b/webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc index e3bc95f79..f063ce38e 100644 --- a/webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/receive_statistics_impl.cc @@ -408,36 +408,31 @@ ReceiveStatisticsImpl::~ReceiveStatisticsImpl() { void ReceiveStatisticsImpl::IncomingPacket(const RTPHeader& header, size_t bytes, bool retransmitted) { - StatisticianImplMap::iterator it; + StreamStatisticianImpl* impl; { CriticalSectionScoped cs(receive_statistics_lock_.get()); - it = statisticians_.find(header.ssrc); - if (it == statisticians_.end()) { - std::pair insert_result = - statisticians_.insert(std::make_pair( - header.ssrc, new StreamStatisticianImpl(clock_, this, this))); - it = insert_result.first; + StatisticianImplMap::iterator it = statisticians_.find(header.ssrc); + if (it != statisticians_.end()) { + impl = it->second; + } else { + impl = new StreamStatisticianImpl(clock_, this, this); + statisticians_[header.ssrc] = impl; } } - it->second->IncomingPacket(header, bytes, retransmitted); + // 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, bytes, retransmitted); } void ReceiveStatisticsImpl::FecPacketReceived(uint32_t ssrc) { CriticalSectionScoped cs(receive_statistics_lock_.get()); StatisticianImplMap::iterator it = statisticians_.find(ssrc); - assert(it != statisticians_.end()); - it->second->FecPacketReceived(); -} - -void ReceiveStatisticsImpl::ChangeSsrc(uint32_t from_ssrc, uint32_t to_ssrc) { - CriticalSectionScoped cs(receive_statistics_lock_.get()); - StatisticianImplMap::iterator from_it = statisticians_.find(from_ssrc); - if (from_it == statisticians_.end()) - return; - if (statisticians_.find(to_ssrc) != statisticians_.end()) - return; - statisticians_[to_ssrc] = from_it->second; - statisticians_.erase(from_it); + // Ignore FEC if it is the first packet. + if (it != statisticians_.end()) { + it->second->FecPacketReceived(); + } } StatisticianMap ReceiveStatisticsImpl::GetActiveStatisticians() const { diff --git a/webrtc/modules/rtp_rtcp/source/receive_statistics_impl.h b/webrtc/modules/rtp_rtcp/source/receive_statistics_impl.h index 4aa41f349..40ca28574 100644 --- a/webrtc/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/webrtc/modules/rtp_rtcp/source/receive_statistics_impl.h @@ -114,8 +114,6 @@ class ReceiveStatisticsImpl : public ReceiveStatistics, virtual int32_t Process() OVERRIDE; virtual int32_t TimeUntilNextProcess() OVERRIDE; - void ChangeSsrc(uint32_t from_ssrc, uint32_t to_ssrc); - virtual void RegisterRtcpStatisticsCallback(RtcpStatisticsCallback* callback) OVERRIDE; diff --git a/webrtc/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/webrtc/modules/rtp_rtcp/source/receive_statistics_unittest.cc index f0b9dedde..5b4d0ddd2 100644 --- a/webrtc/modules/rtp_rtcp/source/receive_statistics_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/receive_statistics_unittest.cc @@ -219,41 +219,42 @@ TEST_F(ReceiveStatisticsTest, RtcpCallbacks) { EXPECT_EQ(1u, callback.num_calls_); } +class RtpTestCallback : public StreamDataCountersCallback { + public: + RtpTestCallback() + : StreamDataCountersCallback(), num_calls_(0), ssrc_(0), stats_() {} + virtual ~RtpTestCallback() {} + + virtual void DataCountersUpdated(const StreamDataCounters& counters, + uint32_t ssrc) { + ssrc_ = ssrc; + stats_ = counters; + ++num_calls_; + } + + void ExpectMatches(uint32_t num_calls, + uint32_t ssrc, + uint32_t bytes, + uint32_t padding, + uint32_t packets, + uint32_t retransmits, + uint32_t fec) { + EXPECT_EQ(num_calls, num_calls_); + EXPECT_EQ(ssrc, ssrc_); + EXPECT_EQ(bytes, stats_.bytes); + EXPECT_EQ(padding, stats_.padding_bytes); + EXPECT_EQ(packets, stats_.packets); + EXPECT_EQ(retransmits, stats_.retransmitted_packets); + EXPECT_EQ(fec, stats_.fec_packets); + } + + uint32_t num_calls_; + uint32_t ssrc_; + StreamDataCounters stats_; +}; + TEST_F(ReceiveStatisticsTest, RtpCallbacks) { - class TestCallback : public StreamDataCountersCallback { - public: - TestCallback() - : StreamDataCountersCallback(), num_calls_(0), ssrc_(0), stats_() {} - virtual ~TestCallback() {} - - virtual void DataCountersUpdated(const StreamDataCounters& counters, - uint32_t ssrc) { - ssrc_ = ssrc; - stats_ = counters; - ++num_calls_; - } - - void ExpectMatches(uint32_t num_calls, - uint32_t ssrc, - uint32_t bytes, - uint32_t padding, - uint32_t packets, - uint32_t retransmits, - uint32_t fec) { - EXPECT_EQ(num_calls, num_calls_); - EXPECT_EQ(ssrc, ssrc_); - EXPECT_EQ(bytes, stats_.bytes); - EXPECT_EQ(padding, stats_.padding_bytes); - EXPECT_EQ(packets, stats_.packets); - EXPECT_EQ(retransmits, stats_.retransmitted_packets); - EXPECT_EQ(fec, stats_.fec_packets); - } - - uint32_t num_calls_; - uint32_t ssrc_; - StreamDataCounters stats_; - } callback; - + RtpTestCallback callback; receive_statistics_->RegisterRtpStatisticsCallback(&callback); const uint32_t kHeaderLength = 20; @@ -300,4 +301,23 @@ TEST_F(ReceiveStatisticsTest, RtpCallbacks) { callback.ExpectMatches( 5, kSsrc1, 4 * kPacketSize1, kPaddingLength * 2, 4, 1, 1); } + +TEST_F(ReceiveStatisticsTest, RtpCallbacksFecFirst) { + RtpTestCallback callback; + receive_statistics_->RegisterRtpStatisticsCallback(&callback); + + const uint32_t kHeaderLength = 20; + + // If first packet is FEC, ignore it. + receive_statistics_->FecPacketReceived(kSsrc1); + EXPECT_EQ(0u, callback.num_calls_); + + header1_.headerLength = kHeaderLength; + receive_statistics_->IncomingPacket( + header1_, kPacketSize1 + kHeaderLength, false); + callback.ExpectMatches(1, kSsrc1, kPacketSize1, 0, 1, 0, 0); + + receive_statistics_->FecPacketReceived(kSsrc1); + callback.ExpectMatches(2, kSsrc1, kPacketSize1, 0, 1, 0, 1); +} } // namespace webrtc