From 436d036b629f5318bba619cd429ae1aed8f0a94b Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Wed, 8 Aug 2018 14:41:16 +0200 Subject: [PATCH] Limits reported cumulative packets lost to 0. This ensures that we don't break clients that can't handle negative values. Bug: webrtc:9598 Change-Id: I33c3933982577752eceb738d7e0bd2a6825d2249 Reviewed-on: https://webrtc-review.googlesource.com/93020 Commit-Queue: Sebastian Jansson Reviewed-by: Danil Chapovalov Reviewed-by: Magnus Flodman Cr-Commit-Position: refs/heads/master@{#24230} --- modules/rtp_rtcp/source/receive_statistics_impl.cc | 5 ++++- modules/rtp_rtcp/source/receive_statistics_unittest.cc | 8 ++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index d140eb04fe..f6bcddb5c8 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc @@ -240,7 +240,10 @@ RtcpStatistics StreamStatisticianImpl::CalculateRtcpStatistics( // Since cumulative loss is carried in a signed 24-bit field in RTCP, we may // need to clamp it. statistics.packets_lost = std::min(statistics.packets_lost, 0x7fffff); - statistics.packets_lost = std::max(statistics.packets_lost, -0x800000); + // TODO(bugs.webrtc.org/9598): This packets_lost should be signed according to + // RFC3550. However, old WebRTC implementations reads it as unsigned. + // Therefore we limit this to 0. + statistics.packets_lost = std::max(statistics.packets_lost, 0); statistics.extended_highest_sequence_number = extended_seq_max; // Note: internal jitter value is in Q4 and needs to be scaled by 1/16. statistics.jitter = jitter_q4_ >> 4; diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc index 3c7fb6f83c..eedcf46df7 100644 --- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc +++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc @@ -479,7 +479,9 @@ TEST_F(ReceiveStatisticsTest, NegativeLoss) { receive_statistics_->GetStatistician(kSsrc1)->GetStatistics( &statistics, /*update_fraction_lost=*/true); EXPECT_EQ(0u, statistics.fraction_lost); - EXPECT_EQ(-1, statistics.packets_lost); + // TODO(bugs.webrtc.org/9598): Since old WebRTC implementations reads this + // value as unsigned we currently limit it to 0. + EXPECT_EQ(0, statistics.packets_lost); // Lose 2 packets; now cumulative loss should become positive again. header1_.sequenceNumber = 7; @@ -521,7 +523,9 @@ TEST_F(ReceiveStatisticsTest, NegativeCumulativeLossClamped) { RtcpStatistics statistics; receive_statistics_->GetStatistician(kSsrc1)->GetStatistics( &statistics, /*update_fraction_lost=*/false); - EXPECT_EQ(-0x800000, statistics.packets_lost); + // TODO(bugs.webrtc.org/9598): Since old WebRTC implementations reads this + // value as unsigned we currently limit it to 0. + EXPECT_EQ(0, statistics.packets_lost); } // Test that the extended highest sequence number is computed correctly when