From a6a699a13078bc8ccc4ed38128c90e6fedc882dc Mon Sep 17 00:00:00 2001 From: asapersson Date: Fri, 25 Nov 2016 03:52:46 -0800 Subject: [PATCH] Sent bitrate stats are incorrect if FlexFEC is configured: WebRTC.Video.BitrateSentInKbps WebRTC.Video.MediaBitrateSentInKbps WebRTC.Video.PaddingBitrateSentInKbps WebRTC.Video.RetransmittedBitrateSentInKbps WebRTC.Video.FecBitrateSentInKbps RtpSender has two StreamDataCounters: for the non-RTX and the RTX stream. The same counter (for the non-RTX stream) is reported for both the media SSRC and the FlexFEC SSRC. Bitrate stats are summed for all SSRCs, thus the counter for the non-RTX stream is counted twice. Do not store the counter for the FlexFEC SSRC. Do not include info from FlexFEC substreams in VideoSendStream::Stats::ToString (periodically logged during a call). BUG=webrtc:6774 Review-Url: https://codereview.webrtc.org/2525293002 Cr-Commit-Position: refs/heads/master@{#15238} --- webrtc/video/send_statistics_proxy.cc | 7 + .../video/send_statistics_proxy_unittest.cc | 129 ++++++++++++++++-- webrtc/video/video_send_stream.cc | 2 +- webrtc/video_send_stream.h | 1 + 4 files changed, 130 insertions(+), 9 deletions(-) diff --git a/webrtc/video/send_statistics_proxy.cc b/webrtc/video/send_statistics_proxy.cc index 1f0ee2cb7a..b29884e585 100644 --- a/webrtc/video/send_statistics_proxy.cc +++ b/webrtc/video/send_statistics_proxy.cc @@ -432,6 +432,7 @@ VideoSendStream::StreamStats* SendStatisticsProxy::GetStatsEntry( // Insert new entry and return ptr. VideoSendStream::StreamStats* entry = &stats_.substreams[ssrc]; entry->is_rtx = is_rtx; + entry->is_flexfec = is_flexfec; return entry; } @@ -617,6 +618,12 @@ void SendStatisticsProxy::DataCountersUpdated( RTC_DCHECK(stats) << "DataCountersUpdated reported for unknown ssrc: " << ssrc; + if (stats->is_flexfec) { + // The same counters are reported for both the media ssrc and flexfec ssrc. + // Bitrate stats are summed for all SSRCs. Use fec stats from media update. + return; + } + stats->rtp_stats = counters; if (uma_container_->first_rtp_stats_time_ms_ == -1) uma_container_->first_rtp_stats_time_ms_ = clock_->TimeInMilliseconds(); diff --git a/webrtc/video/send_statistics_proxy_unittest.cc b/webrtc/video/send_statistics_proxy_unittest.cc index a467f6c5d1..f88d026516 100644 --- a/webrtc/video/send_statistics_proxy_unittest.cc +++ b/webrtc/video/send_statistics_proxy_unittest.cc @@ -25,6 +25,7 @@ const uint32_t kFirstSsrc = 17; const uint32_t kSecondSsrc = 42; const uint32_t kFirstRtxSsrc = 18; const uint32_t kSecondRtxSsrc = 43; +const uint32_t kFlexFecSsrc = 55; const int kQpIdx0 = 21; const int kQpIdx1 = 39; @@ -60,6 +61,25 @@ class SendStatisticsProxyTest : public ::testing::Test { return config; } + VideoSendStream::Config GetTestConfigWithFlexFec() { + VideoSendStream::Config config(nullptr); + config.rtp.ssrcs.push_back(kFirstSsrc); + config.rtp.ssrcs.push_back(kSecondSsrc); + config.rtp.rtx.ssrcs.push_back(kFirstRtxSsrc); + config.rtp.rtx.ssrcs.push_back(kSecondRtxSsrc); + config.rtp.flexfec.flexfec_payload_type = 50; + config.rtp.flexfec.flexfec_ssrc = kFlexFecSsrc; + return config; + } + + VideoSendStream::StreamStats GetStreamStats(uint32_t ssrc) { + VideoSendStream::Stats stats = statistics_proxy_->GetStats(); + std::map::iterator it = + stats.substreams.find(ssrc); + EXPECT_NE(it, stats.substreams.end()); + return it->second; + } + void ExpectEqual(VideoSendStream::Stats one, VideoSendStream::Stats other) { EXPECT_EQ(one.input_frame_rate, other.input_frame_rate); EXPECT_EQ(one.encode_frame_rate, other.encode_frame_rate); @@ -824,6 +844,99 @@ TEST_F(SendStatisticsProxyTest, ResetsRtcpCountersOnContentChange) { 4 * 100 / 5)); } +TEST_F(SendStatisticsProxyTest, GetStatsReportsIsFlexFec) { + statistics_proxy_.reset( + new SendStatisticsProxy(&fake_clock_, GetTestConfigWithFlexFec(), + VideoEncoderConfig::ContentType::kRealtimeVideo)); + + StreamDataCountersCallback* proxy = + static_cast(statistics_proxy_.get()); + StreamDataCounters counters; + proxy->DataCountersUpdated(counters, kFirstSsrc); + proxy->DataCountersUpdated(counters, kFlexFecSsrc); + + EXPECT_FALSE(GetStreamStats(kFirstSsrc).is_flexfec); + EXPECT_TRUE(GetStreamStats(kFlexFecSsrc).is_flexfec); +} + +TEST_F(SendStatisticsProxyTest, SendBitratesAreReportedWithFlexFecEnabled) { + statistics_proxy_.reset( + new SendStatisticsProxy(&fake_clock_, GetTestConfigWithFlexFec(), + VideoEncoderConfig::ContentType::kRealtimeVideo)); + + StreamDataCountersCallback* proxy = + static_cast(statistics_proxy_.get()); + + StreamDataCounters counters; + StreamDataCounters rtx_counters; + proxy->DataCountersUpdated(counters, kFirstSsrc); + proxy->DataCountersUpdated(counters, kSecondSsrc); + proxy->DataCountersUpdated(rtx_counters, kFirstRtxSsrc); + proxy->DataCountersUpdated(rtx_counters, kSecondRtxSsrc); + proxy->DataCountersUpdated(counters, kFlexFecSsrc); + + counters.transmitted.header_bytes = 5000; + counters.transmitted.packets = 20; + counters.transmitted.padding_bytes = 10000; + counters.transmitted.payload_bytes = 20000; + counters.retransmitted.header_bytes = 400; + counters.retransmitted.packets = 2; + counters.retransmitted.padding_bytes = 1000; + counters.retransmitted.payload_bytes = 2000; + counters.fec = counters.retransmitted; + rtx_counters.transmitted = counters.transmitted; + + fake_clock_.AdvanceTimeMilliseconds(1000 * metrics::kMinRunTimeInSeconds); + proxy->DataCountersUpdated(counters, kFirstSsrc); + proxy->DataCountersUpdated(counters, kSecondSsrc); + proxy->DataCountersUpdated(rtx_counters, kFirstRtxSsrc); + proxy->DataCountersUpdated(rtx_counters, kSecondRtxSsrc); + proxy->DataCountersUpdated(counters, kFlexFecSsrc); + + // Reset stats proxy causes histograms to be reported. + statistics_proxy_.reset(); + EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.BitrateSentInKbps")); + EXPECT_EQ(1, + metrics::NumEvents( + "WebRTC.Video.BitrateSentInKbps", + static_cast((counters.transmitted.TotalBytes() * 4 * 8) / + metrics::kMinRunTimeInSeconds / 1000))); + + EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.MediaBitrateSentInKbps")); + EXPECT_EQ(1, metrics::NumEvents( + "WebRTC.Video.MediaBitrateSentInKbps", + static_cast((counters.MediaPayloadBytes() * 2 * 8) / + metrics::kMinRunTimeInSeconds / 1000))); + + EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.PaddingBitrateSentInKbps")); + EXPECT_EQ(1, + metrics::NumEvents( + "WebRTC.Video.PaddingBitrateSentInKbps", + static_cast((counters.transmitted.padding_bytes * 4 * 8) / + metrics::kMinRunTimeInSeconds / 1000))); + + EXPECT_EQ(1, + metrics::NumSamples("WebRTC.Video.RetransmittedBitrateSentInKbps")); + EXPECT_EQ(1, + metrics::NumEvents( + "WebRTC.Video.RetransmittedBitrateSentInKbps", + static_cast((counters.retransmitted.TotalBytes() * 2 * 8) / + metrics::kMinRunTimeInSeconds / 1000))); + + EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.RtxBitrateSentInKbps")); + EXPECT_EQ( + 1, metrics::NumEvents( + "WebRTC.Video.RtxBitrateSentInKbps", + static_cast((rtx_counters.transmitted.TotalBytes() * 2 * 8) / + metrics::kMinRunTimeInSeconds / 1000))); + + EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.FecBitrateSentInKbps")); + EXPECT_EQ(1, metrics::NumEvents( + "WebRTC.Video.FecBitrateSentInKbps", + static_cast((counters.fec.TotalBytes() * 2 * 8) / + metrics::kMinRunTimeInSeconds / 1000))); +} + TEST_F(SendStatisticsProxyTest, ResetsRtpCountersOnContentChange) { StreamDataCountersCallback* proxy = static_cast(statistics_proxy_.get()); @@ -835,15 +948,15 @@ TEST_F(SendStatisticsProxyTest, ResetsRtpCountersOnContentChange) { proxy->DataCountersUpdated(rtx_counters, kFirstRtxSsrc); proxy->DataCountersUpdated(rtx_counters, kSecondRtxSsrc); - counters.transmitted.header_bytes = 400; + counters.transmitted.header_bytes = 5000; counters.transmitted.packets = 20; - counters.transmitted.padding_bytes = 1000; - counters.transmitted.payload_bytes = 2000; + counters.transmitted.padding_bytes = 10000; + counters.transmitted.payload_bytes = 20000; - counters.retransmitted.header_bytes = 40; + counters.retransmitted.header_bytes = 400; counters.retransmitted.packets = 2; - counters.retransmitted.padding_bytes = 100; - counters.retransmitted.payload_bytes = 200; + counters.retransmitted.padding_bytes = 1000; + counters.retransmitted.payload_bytes = 2000; counters.fec = counters.retransmitted; @@ -898,7 +1011,7 @@ TEST_F(SendStatisticsProxyTest, ResetsRtpCountersOnContentChange) { EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.FecBitrateSentInKbps")); EXPECT_EQ(1, metrics::NumEvents( "WebRTC.Video.FecBitrateSentInKbps", - static_cast((rtx_counters.fec.TotalBytes() * 2 * 8) / + static_cast((counters.fec.TotalBytes() * 2 * 8) / metrics::kMinRunTimeInSeconds / 1000))); // New start time but same counter values. @@ -965,7 +1078,7 @@ TEST_F(SendStatisticsProxyTest, ResetsRtpCountersOnContentChange) { 1, metrics::NumSamples("WebRTC.Video.Screenshare.FecBitrateSentInKbps")); EXPECT_EQ(1, metrics::NumEvents( "WebRTC.Video.Screenshare.FecBitrateSentInKbps", - static_cast((rtx_counters.fec.TotalBytes() * 2 * 8) / + static_cast((counters.fec.TotalBytes() * 2 * 8) / metrics::kMinRunTimeInSeconds / 1000))); } diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index 51c9dad131..8eb8cba3d1 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -214,7 +214,7 @@ std::string VideoSendStream::Stats::ToString(int64_t time_ms) const { ss << "bw_adapted: " << (bw_limited_resolution ? "true" : "false"); ss << '}'; for (const auto& substream : substreams) { - if (!substream.second.is_rtx) { + if (!substream.second.is_rtx && !substream.second.is_flexfec) { ss << " {ssrc: " << substream.first << ", "; ss << substream.second.ToString(); ss << '}'; diff --git a/webrtc/video_send_stream.h b/webrtc/video_send_stream.h index 74e9d5a559..92181a7878 100644 --- a/webrtc/video_send_stream.h +++ b/webrtc/video_send_stream.h @@ -36,6 +36,7 @@ class VideoSendStream { FrameCounts frame_counts; bool is_rtx = false; + bool is_flexfec = false; int width = 0; int height = 0; // TODO(holmer): Move bitrate_bps out to the webrtc::Call layer.