From cf41eb1ce13a3a751eec60d79be47f6277190b41 Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Mon, 10 Jun 2019 11:30:59 +0200 Subject: [PATCH] Reland "Cleanup of video packet overhead calculation." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of 890bc3069cbababa19b40ec02684253d60e051b2 Zero bitrate caused division by zero in DCHECK for max bitrate. Added unit tests to ensure that setting zero bitrate does not crash. > Original change's description: > > Cleanup of video packet overhead calculation. > > > > This CL updates the video packet overhead calculation to make it more > > clear. This prepares for future work on improving the accuracy of the > > calculation. > > > > Bug: webrtc:9883 > > Change-Id: I1d623a3e0de45be7b6e4a1f9e3cbe54fd2b8a45a > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/138077 > > Commit-Queue: Sebastian Jansson > > Reviewed-by: Erik Språng > > Cr-Commit-Position: refs/heads/master@{#28040} Bug: webrtc:10674 Change-Id: I156d1ee5546ede7e43ae1d9a298dcaba6071230f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/140890 Reviewed-by: Niels Moller Reviewed-by: Erik Språng Commit-Queue: Sebastian Jansson Cr-Commit-Position: refs/heads/master@{#28212} --- api/units/data_rate.h | 10 ++--- call/rtp_video_sender.cc | 63 +++++++++++++++---------------- call/rtp_video_sender_unittest.cc | 19 ++++++++++ 3 files changed, 54 insertions(+), 38 deletions(-) diff --git a/api/units/data_rate.h b/api/units/data_rate.h index 3ecdce657c..b04ee38bbf 100644 --- a/api/units/data_rate.h +++ b/api/units/data_rate.h @@ -126,11 +126,11 @@ inline Frequency operator/(const DataRate rate, const DataSize size) { size.bytes()); } inline DataRate operator*(const DataSize size, const Frequency frequency) { - int64_t millihertz = frequency.millihertz(); - int64_t kMaxBeforeConversion = - std::numeric_limits::max() / 8 / millihertz; - RTC_DCHECK_LE(size.bytes(), kMaxBeforeConversion); - int64_t millibits_per_second = size.bytes() * 8 * millihertz; + RTC_DCHECK(frequency.IsZero() || + size.bytes() <= std::numeric_limits::max() / 8 / + frequency.millihertz()); + int64_t millibits_per_second = + size.bytes() * 8 * frequency.millihertz(); return DataRate::bps((millibits_per_second + 500) / 1000); } inline DataRate operator*(const Frequency frequency, const DataSize size) { diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 424c32df36..eb00c480a5 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -189,19 +189,13 @@ std::unique_ptr MaybeCreateFlexfecSender( RTPSender::FecExtensionSizes(), rtp_state, clock); } -uint32_t CalculateOverheadRateBps(int packets_per_second, - size_t overhead_bytes_per_packet, - uint32_t max_overhead_bps) { - uint32_t overhead_bps = - static_cast(8 * overhead_bytes_per_packet * packets_per_second); - return std::min(overhead_bps, max_overhead_bps); -} - -int CalculatePacketRate(uint32_t bitrate_bps, size_t packet_size_bytes) { - size_t packet_size_bits = 8 * packet_size_bytes; - // Ceil for int value of bitrate_bps / packet_size_bits. - return static_cast((bitrate_bps + packet_size_bits - 1) / - packet_size_bits); +DataRate CalculateOverheadRate(DataRate data_rate, + DataSize packet_size, + DataSize overhead_per_packet) { + Frequency packet_rate = data_rate / packet_size; + // TOSO(srte): We should not need to round to nearest whole packet per second + // rate here. + return packet_rate.RoundUpTo(Frequency::hertz(1)) * overhead_per_packet; } } // namespace @@ -694,16 +688,17 @@ void RtpVideoSender::OnBitrateUpdated(uint32_t bitrate_bps, int framerate) { // Substract overhead from bitrate. rtc::CritScope lock(&crit_); + DataSize packet_overhead = DataSize::bytes( + overhead_bytes_per_packet_ + transport_overhead_bytes_per_packet_); + DataSize max_total_packet_size = DataSize::bytes( + rtp_config_.max_packet_size + transport_overhead_bytes_per_packet_); uint32_t payload_bitrate_bps = bitrate_bps; if (send_side_bwe_with_overhead_) { - uint32_t overhead_bps = CalculateOverheadRateBps( - CalculatePacketRate( - bitrate_bps, - rtp_config_.max_packet_size + transport_overhead_bytes_per_packet_), - overhead_bytes_per_packet_ + transport_overhead_bytes_per_packet_, - bitrate_bps); - RTC_DCHECK_LE(overhead_bps, bitrate_bps); - payload_bitrate_bps = bitrate_bps - overhead_bps; + DataRate overhead_rate = CalculateOverheadRate( + DataRate::bps(bitrate_bps), max_total_packet_size, packet_overhead); + // TODO(srte): We probably should not accept 0 payload bitrate here. + payload_bitrate_bps = + rtc::saturated_cast(bitrate_bps - overhead_rate.bps()); } // Get the encoder target rate. It is the estimated network rate - @@ -724,18 +719,20 @@ void RtpVideoSender::OnBitrateUpdated(uint32_t bitrate_bps, loss_mask_vector_.clear(); - uint32_t encoder_overhead_rate_bps = - send_side_bwe_with_overhead_ - ? CalculateOverheadRateBps( - CalculatePacketRate(encoder_target_rate_bps_, - rtp_config_.max_packet_size + - transport_overhead_bytes_per_packet_ - - overhead_bytes_per_packet_), - overhead_bytes_per_packet_ + - transport_overhead_bytes_per_packet_, - bitrate_bps - encoder_target_rate_bps_) - : 0; - + uint32_t encoder_overhead_rate_bps = 0; + if (send_side_bwe_with_overhead_) { + // TODO(srte): The packet size should probably be the same as in the + // CalculateOverheadRate call above (just max_total_packet_size), it doesn't + // make sense to use different packet rates for different overhead + // calculations. + DataRate encoder_overhead_rate = CalculateOverheadRate( + DataRate::bps(encoder_target_rate_bps_), + max_total_packet_size - DataSize::bytes(overhead_bytes_per_packet_), + packet_overhead); + encoder_overhead_rate_bps = + std::min(encoder_overhead_rate.bps(), + bitrate_bps - encoder_target_rate_bps_); + } // When the field trial "WebRTC-SendSideBwe-WithOverhead" is enabled // protection_bitrate includes overhead. const uint32_t media_rate = encoder_target_rate_bps_ + diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index 16eb8a35c9..016c259151 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -631,4 +631,23 @@ TEST(RtpVideoSenderTest, EarlyRetransmits) { test.clock().AdvanceTimeMilliseconds(33); ASSERT_TRUE(event.Wait(kTimeoutMs)); } + +TEST(RtpVideoSenderTest, CanSetZeroBitrateWithOverhead) { + test::ScopedFieldTrials trials("WebRTC-SendSideBwe-WithOverhead/Enabled/"); + RtpVideoSenderTestFixture test({kSsrc1}, {kRtxSsrc1}, kPayloadType, {}); + + test.router()->OnBitrateUpdated(/*bitrate_bps*/ 0, + /*fraction_loss*/ 0, + /*rtt*/ 0, + /*framerate*/ 0); +} + +TEST(RtpVideoSenderTest, CanSetZeroBitrateWithoutOverhead) { + RtpVideoSenderTestFixture test({kSsrc1}, {kRtxSsrc1}, kPayloadType, {}); + + test.router()->OnBitrateUpdated(/*bitrate_bps*/ 0, + /*fraction_loss*/ 0, + /*rtt*/ 0, + /*framerate*/ 0); +} } // namespace webrtc