From da4f09315f6daa2bc9f66150e4a508bac53cd19d Mon Sep 17 00:00:00 2001 From: Bjorn A Mellem Date: Tue, 30 Jul 2019 08:34:03 -0700 Subject: [PATCH] Reland "Only include payload in bytes sent/received." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of 74a1b4b1321b426392d4c32e4a02361226ad5358 Original change's description: > Only include payload in bytes sent/received. > > According to https://www.w3.org/TR/webrtc-stats/#sentrtpstats-dict* and > https://tools.ietf.org/html/rfc3550#section-6.4.1, the bytes sent > statistic should not include headers or padding. > > Similarly, according to > https://www.w3.org/TR/webrtc-stats/#inboundrtpstats-dict*, bytes > received are calculated the same way as bytes sent (eg. not including > padding or headers). > > This change stops adding padding and headers to these statistics. > > Bug: webrtc:8516,webrtc:10525 > Change-Id: I891ad5a11a493cc3212afe93e13f62795bf4031f > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/146180 > Reviewed-by: Stefan Holmer > Reviewed-by: Erik Språng > Reviewed-by: Steve Anton > Reviewed-by: Henrik Boström > Reviewed-by: Ilya Nikolaevskiy > Reviewed-by: Oskar Sundbom > Commit-Queue: Bjorn Mellem > Cr-Commit-Position: refs/heads/master@{#28647} Bug: webrtc:8516, webrtc:10525 Change-Id: Iaa1613e5becdfaa0af0f6b9f00e5b871937a719c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147520 Reviewed-by: Steve Anton Reviewed-by: Fredrik Solenberg Commit-Queue: Bjorn Mellem Cr-Commit-Position: refs/heads/master@{#28731} --- audio/channel_receive.cc | 24 ++++++++++---- audio/channel_send.cc | 26 +++++++++++---- media/engine/webrtc_video_engine.cc | 35 ++++++++++++++------ media/engine/webrtc_video_engine.h | 4 +++ media/engine/webrtc_video_engine_unittest.cc | 29 ++++++++++++---- 5 files changed, 88 insertions(+), 30 deletions(-) diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index f248c99c6d..0f92cfb5bd 100644 --- a/audio/channel_receive.cc +++ b/audio/channel_receive.cc @@ -44,6 +44,7 @@ #include "rtc_base/race_checker.h" #include "rtc_base/thread_checker.h" #include "rtc_base/time_utils.h" +#include "system_wrappers/include/field_trial.h" #include "system_wrappers/include/metrics.h" namespace webrtc { @@ -57,6 +58,11 @@ constexpr double kAudioSampleDurationSeconds = 0.01; constexpr int kVoiceEngineMinMinPlayoutDelayMs = 0; constexpr int kVoiceEngineMaxMinPlayoutDelayMs = 10000; +// Field trial which controls whether to report standard-compliant bytes +// sent/received per stream. If enabled, padding and headers are not included +// in bytes sent or received. +constexpr char kUseStandardBytesStats[] = "WebRTC-UseStandardBytesStats"; + RTPHeader CreateRTPHeaderForMediaTransportFrame( const MediaTransportEncodedAudioFrame& frame, uint64_t channel_id) { @@ -265,6 +271,8 @@ class ChannelReceive : public ChannelReceiveInterface, // E2EE Audio Frame Decryption rtc::scoped_refptr frame_decryptor_; webrtc::CryptoOptions crypto_options_; + + const bool use_standard_bytes_stats_; }; int32_t ChannelReceive::OnReceivedPayloadData(const uint8_t* payloadData, @@ -466,7 +474,9 @@ ChannelReceive::ChannelReceive( associated_send_channel_(nullptr), media_transport_config_(media_transport_config), frame_decryptor_(frame_decryptor), - crypto_options_(crypto_options) { + crypto_options_(crypto_options), + use_standard_bytes_stats_( + webrtc::field_trial::IsEnabled(kUseStandardBytesStats)) { // TODO(nisse): Use _moduleProcessThreadPtr instead? module_process_thread_checker_.Detach(); @@ -767,11 +777,13 @@ CallReceiveStatistics ChannelReceive::GetRTCPStatistics() const { if (statistician) { StreamDataCounters data_counters; statistician->GetReceiveStreamDataCounters(&data_counters); - // TODO(http://crbug.com/webrtc/10525): Bytes received should only include - // payload bytes, not header and padding bytes. - stats.bytesReceived = data_counters.transmitted.payload_bytes + - data_counters.transmitted.header_bytes + - data_counters.transmitted.padding_bytes; + if (use_standard_bytes_stats_) { + stats.bytesReceived = data_counters.transmitted.payload_bytes; + } else { + stats.bytesReceived = data_counters.transmitted.payload_bytes + + data_counters.transmitted.header_bytes + + data_counters.transmitted.padding_bytes; + } stats.packetsReceived = data_counters.transmitted.packets; stats.last_packet_received_timestamp_ms = data_counters.last_packet_received_timestamp_ms; diff --git a/audio/channel_send.cc b/audio/channel_send.cc index 8ce33a46c1..4df06f3a2e 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -53,6 +53,11 @@ namespace { constexpr int64_t kMaxRetransmissionWindowMs = 1000; constexpr int64_t kMinRetransmissionWindowMs = 30; +// Field trial which controls whether to report standard-compliant bytes +// sent/received per stream. If enabled, padding and headers are not included +// in bytes sent or received. +constexpr char kUseStandardBytesStats[] = "WebRTC-UseStandardBytesStats"; + MediaTransportEncodedAudioFrame::FrameType MediaTransportFrameTypeForWebrtcFrameType(webrtc::AudioFrameType frame_type) { switch (frame_type) { @@ -266,6 +271,7 @@ class ChannelSend : public ChannelSendInterface, rtc::ThreadChecker construction_thread_; const bool use_twcc_plr_for_ana_; + const bool use_standard_bytes_stats_; bool encoder_queue_is_active_ RTC_GUARDED_BY(encoder_queue_) = false; @@ -654,6 +660,8 @@ ChannelSend::ChannelSend(Clock* clock, new RateLimiter(clock, kMaxRetransmissionWindowMs)), use_twcc_plr_for_ana_( webrtc::field_trial::FindFullName("UseTwccPlrForAna") == "Enabled"), + use_standard_bytes_stats_( + webrtc::field_trial::IsEnabled(kUseStandardBytesStats)), media_transport_config_(media_transport_config), frame_encryptor_(frame_encryptor), crypto_options_(crypto_options), @@ -1078,13 +1086,17 @@ CallSendStatistics ChannelSend::GetRTCPStatistics() const { StreamDataCounters rtp_stats; StreamDataCounters rtx_stats; _rtpRtcpModule->GetSendStreamDataCounters(&rtp_stats, &rtx_stats); - // TODO(https://crbug.com/webrtc/10525): Bytes sent should only include - // payload bytes, not header and padding bytes. - stats.bytesSent = - rtp_stats.transmitted.payload_bytes + - rtp_stats.transmitted.padding_bytes + rtp_stats.transmitted.header_bytes + - rtx_stats.transmitted.payload_bytes + - rtx_stats.transmitted.padding_bytes + rtx_stats.transmitted.header_bytes; + if (use_standard_bytes_stats_) { + stats.bytesSent = rtp_stats.transmitted.payload_bytes + + rtx_stats.transmitted.payload_bytes; + } else { + stats.bytesSent = rtp_stats.transmitted.payload_bytes + + rtp_stats.transmitted.padding_bytes + + rtp_stats.transmitted.header_bytes + + rtx_stats.transmitted.payload_bytes + + rtx_stats.transmitted.padding_bytes + + rtx_stats.transmitted.header_bytes; + } // TODO(https://crbug.com/webrtc/10555): RTX retransmissions should show up in // separate outbound-rtp stream objects. stats.retransmitted_bytes_sent = rtp_stats.retransmitted.payload_bytes; diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 9658ade62d..a0f6db4cad 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -45,6 +45,11 @@ namespace { const int kMinLayerSize = 16; +// Field trial which controls whether to report standard-compliant bytes +// sent/received per stream. If enabled, padding and headers are not included +// in bytes sent or received. +constexpr char kUseStandardBytesStats[] = "WebRTC-UseStandardBytesStats"; + // If this field trial is enabled, we will enable sending FlexFEC and disable // sending ULPFEC whenever the former has been negotiated in the SDPs. bool IsFlexfecFieldTrialEnabled() { @@ -1795,7 +1800,9 @@ WebRtcVideoChannel::WebRtcVideoSendStream::WebRtcVideoSendStream( encoder_sink_(nullptr), parameters_(std::move(config), options, max_bitrate_bps, codec_settings), rtp_parameters_(CreateRtpParametersWithEncodings(sp)), - sending_(false) { + sending_(false), + use_standard_bytes_stats_( + webrtc::field_trial::IsEnabled(kUseStandardBytesStats)) { // Maximum packet size may come in RtpConfig from external transport, for // example from QuicTransportInterface implementation, so do not exceed // given max_packet_size. @@ -2362,11 +2369,13 @@ VideoSenderInfo WebRtcVideoChannel::WebRtcVideoSendStream::GetVideoSenderInfo( it != stats.substreams.end(); ++it) { // TODO(pbos): Wire up additional stats, such as padding bytes. webrtc::VideoSendStream::StreamStats stream_stats = it->second; - // TODO(http://crbug.com/webrtc/10525): Bytes sent should only include - // payload bytes, not header and padding bytes. - info.bytes_sent += stream_stats.rtp_stats.transmitted.payload_bytes + - stream_stats.rtp_stats.transmitted.header_bytes + - stream_stats.rtp_stats.transmitted.padding_bytes; + if (use_standard_bytes_stats_) { + info.bytes_sent += stream_stats.rtp_stats.transmitted.payload_bytes; + } else { + info.bytes_sent += stream_stats.rtp_stats.transmitted.payload_bytes + + stream_stats.rtp_stats.transmitted.header_bytes + + stream_stats.rtp_stats.transmitted.padding_bytes; + } info.packets_sent += stream_stats.rtp_stats.transmitted.packets; info.total_packet_send_delay_ms += stream_stats.total_packet_send_delay_ms; // TODO(https://crbug.com/webrtc/10555): RTX retransmissions should show up @@ -2482,7 +2491,9 @@ WebRtcVideoChannel::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream( decoder_factory_(decoder_factory), sink_(NULL), first_frame_timestamp_(-1), - estimated_remote_start_ntp_time_ms_(0) { + estimated_remote_start_ntp_time_ms_(0), + use_standard_bytes_stats_( + webrtc::field_trial::IsEnabled(kUseStandardBytesStats)) { config_.renderer = this; ConfigureCodecs(recv_codecs); ConfigureFlexfecCodec(flexfec_config.payload_type); @@ -2783,9 +2794,13 @@ WebRtcVideoChannel::WebRtcVideoReceiveStream::GetVideoReceiverInfo( if (stats.current_payload_type != -1) { info.codec_payload_type = stats.current_payload_type; } - info.bytes_rcvd = stats.rtp_stats.transmitted.payload_bytes + - stats.rtp_stats.transmitted.header_bytes + - stats.rtp_stats.transmitted.padding_bytes; + if (use_standard_bytes_stats_) { + info.bytes_rcvd = stats.rtp_stats.transmitted.payload_bytes; + } else { + info.bytes_rcvd = stats.rtp_stats.transmitted.payload_bytes + + stats.rtp_stats.transmitted.header_bytes + + stats.rtp_stats.transmitted.padding_bytes; + } info.packets_rcvd = stats.rtp_stats.transmitted.packets; info.packets_lost = stats.rtcp_stats.packets_lost; diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index f0e86d895a..c2c137ccaa 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -378,6 +378,8 @@ class WebRtcVideoChannel : public VideoMediaChannel, bool sending_ RTC_GUARDED_BY(&thread_checker_); + const bool use_standard_bytes_stats_; + // In order for the |invoker_| to protect other members from being // destructed as they are used in asynchronous tasks it has to be destructed // first. @@ -468,6 +470,8 @@ class WebRtcVideoChannel : public VideoMediaChannel, // Start NTP time is estimated as current remote NTP time (estimated from // RTCP) minus the elapsed time, as soon as remote NTP time is available. int64_t estimated_remote_start_ntp_time_ms_ RTC_GUARDED_BY(sink_lock_); + + const bool use_standard_bytes_stats_; }; void Construct(webrtc::Call* call, WebRtcVideoEngine* engine); diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 4874cf6200..9b51b1724e 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -87,6 +87,8 @@ static const uint32_t kFlexfecSsrc = 5; static const uint32_t kIncomingUnsignalledSsrc = 0xC0FFEE; static const uint32_t kDefaultRecvSsrc = 0; +constexpr uint32_t kRtpHeaderSize = 12; + static const char kUnsupportedExtensionName[] = "urn:ietf:params:rtp-hdrext:unsupported"; @@ -1593,6 +1595,10 @@ TEST_F(WebRtcVideoChannelBaseTest, InvalidRecvBufferSize) { // Test that stats work properly for a 1-1 call. TEST_F(WebRtcVideoChannelBaseTest, GetStats) { + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-UseStandardBytesStats/Enabled/"); + SetUp(); + const int kDurationSec = 3; const int kFps = 10; SendReceiveManyAndGetStats(DefaultCodec(), kDurationSec, kFps); @@ -1603,7 +1609,8 @@ TEST_F(WebRtcVideoChannelBaseTest, GetStats) { ASSERT_EQ(1U, info.senders.size()); // TODO(whyuan): bytes_sent and bytes_rcvd are different. Are both payload? // For webrtc, bytes_sent does not include the RTP header length. - EXPECT_GT(info.senders[0].bytes_sent, 0); + EXPECT_EQ(info.senders[0].bytes_sent, + NumRtpBytes() - kRtpHeaderSize * NumRtpPackets()); EXPECT_EQ(NumRtpPackets(), info.senders[0].packets_sent); EXPECT_EQ(0.0, info.senders[0].fraction_lost); ASSERT_TRUE(info.senders[0].codec_payload_type); @@ -1626,7 +1633,8 @@ TEST_F(WebRtcVideoChannelBaseTest, GetStats) { EXPECT_EQ(info.senders[0].ssrcs()[0], info.receivers[0].ssrcs()[0]); ASSERT_TRUE(info.receivers[0].codec_payload_type); EXPECT_EQ(DefaultCodec().id, *info.receivers[0].codec_payload_type); - EXPECT_EQ(NumRtpBytes(), info.receivers[0].bytes_rcvd); + EXPECT_EQ(NumRtpBytes() - kRtpHeaderSize * NumRtpPackets(), + info.receivers[0].bytes_rcvd); EXPECT_EQ(NumRtpPackets(), info.receivers[0].packets_rcvd); EXPECT_EQ(0, info.receivers[0].packets_lost); // TODO(asapersson): Not set for webrtc. Handle missing stats. @@ -1647,6 +1655,10 @@ TEST_F(WebRtcVideoChannelBaseTest, GetStats) { // Test that stats work properly for a conf call with multiple recv streams. TEST_F(WebRtcVideoChannelBaseTest, GetStatsMultipleRecvStreams) { + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-UseStandardBytesStats/Enabled/"); + SetUp(); + cricket::FakeVideoRenderer renderer1, renderer2; EXPECT_TRUE(SetOneCodec(DefaultCodec())); cricket::VideoSendParameters parameters; @@ -1677,7 +1689,8 @@ TEST_F(WebRtcVideoChannelBaseTest, GetStatsMultipleRecvStreams) { ASSERT_EQ(1U, info.senders.size()); // TODO(whyuan): bytes_sent and bytes_rcvd are different. Are both payload? // For webrtc, bytes_sent does not include the RTP header length. - EXPECT_GT(GetSenderStats(0).bytes_sent, 0); + EXPECT_EQ_WAIT(NumRtpBytes() - kRtpHeaderSize * NumRtpPackets(), + GetSenderStats(0).bytes_sent, kTimeout); EXPECT_EQ_WAIT(NumRtpPackets(), GetSenderStats(0).packets_sent, kTimeout); EXPECT_EQ(kVideoWidth, GetSenderStats(0).send_frame_width); EXPECT_EQ(kVideoHeight, GetSenderStats(0).send_frame_height); @@ -1686,7 +1699,8 @@ TEST_F(WebRtcVideoChannelBaseTest, GetStatsMultipleRecvStreams) { for (size_t i = 0; i < info.receivers.size(); ++i) { EXPECT_EQ(1U, GetReceiverStats(i).ssrcs().size()); EXPECT_EQ(i + 1, GetReceiverStats(i).ssrcs()[0]); - EXPECT_EQ_WAIT(NumRtpBytes(), GetReceiverStats(i).bytes_rcvd, kTimeout); + EXPECT_EQ_WAIT(NumRtpBytes() - kRtpHeaderSize * NumRtpPackets(), + GetReceiverStats(i).bytes_rcvd, kTimeout); EXPECT_EQ_WAIT(NumRtpPackets(), GetReceiverStats(i).packets_rcvd, kTimeout); EXPECT_EQ_WAIT(kVideoWidth, GetReceiverStats(i).frame_width, kTimeout); EXPECT_EQ_WAIT(kVideoHeight, GetReceiverStats(i).frame_height, kTimeout); @@ -5158,6 +5172,9 @@ TEST_F(WebRtcVideoChannelTest, GetStatsTranslatesDecodeStatsCorrectly) { } TEST_F(WebRtcVideoChannelTest, GetStatsTranslatesReceivePacketStatsCorrectly) { + webrtc::test::ScopedFieldTrials field_trials( + "WebRTC-UseStandardBytesStats/Enabled/"); + FakeVideoReceiveStream* stream = AddRecvStream(); webrtc::VideoReceiveStream::Stats stats; stats.rtp_stats.transmitted.payload_bytes = 2; @@ -5170,9 +5187,7 @@ TEST_F(WebRtcVideoChannelTest, GetStatsTranslatesReceivePacketStatsCorrectly) { cricket::VideoMediaInfo info; ASSERT_TRUE(channel_->GetStats(&info)); - EXPECT_EQ(stats.rtp_stats.transmitted.payload_bytes + - stats.rtp_stats.transmitted.header_bytes + - stats.rtp_stats.transmitted.padding_bytes, + EXPECT_EQ(stats.rtp_stats.transmitted.payload_bytes, rtc::checked_cast(info.receivers[0].bytes_rcvd)); EXPECT_EQ(stats.rtp_stats.transmitted.packets, rtc::checked_cast(info.receivers[0].packets_rcvd));