From 53d901332c2eb43cad0da5768c6f7a8c4aeb9590 Mon Sep 17 00:00:00 2001 From: Jonathan Yu Date: Tue, 19 Dec 2017 22:27:50 +0000 Subject: [PATCH] Revert "Add ProtectionBitrateCalculator as an abstract class. ProtectionBitrateCalculatorDefault implements ProtectionBitrateCalculator. Register VideoSendStream to packet feedback" This reverts commit e58e91b6d143ef847f8df24b19de4ba98cdb6f72. Reason for revert: Breaks downstream project b/70848177 Original change's description: > Add ProtectionBitrateCalculator as an abstract class. ProtectionBitrateCalculatorDefault implements ProtectionBitrateCalculator. Register VideoSendStream to packet feedback > > Bug: webrtc:8656 > Change-Id: Iab4f6ab8997cb082762218afc8580e9985ac2522 > Reviewed-on: https://webrtc-review.googlesource.com/33010 > Commit-Queue: Ying Wang > Reviewed-by: Stefan Holmer > Cr-Commit-Position: refs/heads/master@{#21348} TBR=stefan@webrtc.org,philipel@webrtc.org,yinwa@webrtc.org Change-Id: Ic186ba78be429bd1046ceac15051a3382b6ffc4f No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:8656 Reviewed-on: https://webrtc-review.googlesource.com/35080 Commit-Queue: Lu Liu Reviewed-by: Lu Liu Cr-Commit-Position: refs/heads/master@{#21374} --- modules/video_coding/BUILD.gn | 3 +- ...lt.cc => protection_bitrate_calculator.cc} | 23 +++-- .../protection_bitrate_calculator.h | 58 ++++++++----- .../protection_bitrate_calculator_default.h | 87 ------------------- .../protection_bitrate_calculator_unittest.cc | 4 +- video/video_send_stream.cc | 69 +++------------ video/video_send_stream.h | 1 + 7 files changed, 64 insertions(+), 181 deletions(-) rename modules/video_coding/{protection_bitrate_calculator_default.cc => protection_bitrate_calculator.cc} (89%) delete mode 100644 modules/video_coding/protection_bitrate_calculator_default.h diff --git a/modules/video_coding/BUILD.gn b/modules/video_coding/BUILD.gn index 71e9333bde..59e8eb9221 100644 --- a/modules/video_coding/BUILD.gn +++ b/modules/video_coding/BUILD.gn @@ -57,9 +57,8 @@ rtc_static_library("video_coding") { "packet.h", "packet_buffer.cc", "packet_buffer.h", + "protection_bitrate_calculator.cc", "protection_bitrate_calculator.h", - "protection_bitrate_calculator_default.cc", - "protection_bitrate_calculator_default.h", "qp_parser.cc", "qp_parser.h", "receiver.cc", diff --git a/modules/video_coding/protection_bitrate_calculator_default.cc b/modules/video_coding/protection_bitrate_calculator.cc similarity index 89% rename from modules/video_coding/protection_bitrate_calculator_default.cc rename to modules/video_coding/protection_bitrate_calculator.cc index f0db4c2957..3c1da234d9 100644 --- a/modules/video_coding/protection_bitrate_calculator_default.cc +++ b/modules/video_coding/protection_bitrate_calculator.cc @@ -8,13 +8,13 @@ * be found in the AUTHORS file in the root of the source tree. */ -#include "modules/video_coding/protection_bitrate_calculator_default.h" +#include "modules/video_coding/protection_bitrate_calculator.h" namespace webrtc { using rtc::CritScope; -ProtectionBitrateCalculatorDefault::ProtectionBitrateCalculatorDefault( +ProtectionBitrateCalculator::ProtectionBitrateCalculator( Clock* clock, VCMProtectionCallback* protection_callback) : clock_(clock), @@ -23,22 +23,21 @@ ProtectionBitrateCalculatorDefault::ProtectionBitrateCalculatorDefault( clock_->TimeInMilliseconds())), max_payload_size_(1460) {} -ProtectionBitrateCalculatorDefault::~ProtectionBitrateCalculatorDefault(void) { +ProtectionBitrateCalculator::~ProtectionBitrateCalculator(void) { loss_prot_logic_->Release(); } -void ProtectionBitrateCalculatorDefault::SetEncodingData( - size_t width, - size_t height, - size_t num_temporal_layers, - size_t max_payload_size) { +void ProtectionBitrateCalculator::SetEncodingData(size_t width, + size_t height, + size_t num_temporal_layers, + size_t max_payload_size) { CritScope lock(&crit_sect_); loss_prot_logic_->UpdateFrameSize(width, height); loss_prot_logic_->UpdateNumLayers(num_temporal_layers); max_payload_size_ = max_payload_size; } -uint32_t ProtectionBitrateCalculatorDefault::SetTargetRates( +uint32_t ProtectionBitrateCalculator::SetTargetRates( uint32_t estimated_bitrate_bps, int actual_framerate_fps, uint8_t fraction_lost, @@ -139,8 +138,8 @@ uint32_t ProtectionBitrateCalculatorDefault::SetTargetRates( return estimated_bitrate_bps * (1.0 - protection_overhead_rate); } -void ProtectionBitrateCalculatorDefault::SetProtectionMethod(bool enable_fec, - bool enable_nack) { +void ProtectionBitrateCalculator::SetProtectionMethod(bool enable_fec, + bool enable_nack) { media_optimization::VCMProtectionMethodEnum method(media_optimization::kNone); if (enable_fec && enable_nack) { method = media_optimization::kNackFec; @@ -153,7 +152,7 @@ void ProtectionBitrateCalculatorDefault::SetProtectionMethod(bool enable_fec, loss_prot_logic_->SetMethod(method); } -void ProtectionBitrateCalculatorDefault::UpdateWithEncodedData( +void ProtectionBitrateCalculator::UpdateWithEncodedData( const EncodedImage& encoded_image) { const size_t encoded_length = encoded_image._length; CritScope lock(&crit_sect_); diff --git a/modules/video_coding/protection_bitrate_calculator.h b/modules/video_coding/protection_bitrate_calculator.h index a309ec4bcf..bf50589fe8 100644 --- a/modules/video_coding/protection_bitrate_calculator.h +++ b/modules/video_coding/protection_bitrate_calculator.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017 The WebRTC project authors. All Rights Reserved. + * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. * * Use of this source code is governed by a BSD-style license * that can be found in the LICENSE file in the root of the source @@ -13,7 +13,6 @@ #include #include -#include #include "modules/include/module_common_types.h" #include "modules/video_coding/include/video_coding.h" @@ -23,38 +22,57 @@ namespace webrtc { +// ProtectionBitrateCalculator calculates how much of the allocated network +// capacity that can be used by an encoder and how much that +// is needed for redundant packets such as FEC and NACK. It uses an +// implementation of |VCMProtectionCallback| to set new FEC parameters and get +// the bitrate currently used for FEC and NACK. +// Usage: +// Setup by calling SetProtectionMethod and SetEncodingData. +// For each encoded image, call UpdateWithEncodedData. +// Each time the bandwidth estimate change, call SetTargetRates. SetTargetRates +// will return the bitrate that can be used by an encoder. +// A lock is used to protect internal states, so methods can be called on an +// arbitrary thread. class ProtectionBitrateCalculator { public: - virtual ~ProtectionBitrateCalculator() {} + ProtectionBitrateCalculator(Clock* clock, + VCMProtectionCallback* protection_callback); + ~ProtectionBitrateCalculator(); - virtual void SetProtectionMethod(bool enable_fec, bool enable_nack) = 0; + void SetProtectionMethod(bool enable_fec, bool enable_nack); // Informs media optimization of initial encoding state. - virtual void SetEncodingData(size_t width, - size_t height, - size_t num_temporal_layers, - size_t max_payload_size) = 0; + void SetEncodingData(size_t width, + size_t height, + size_t num_temporal_layers, + size_t max_payload_size); // Returns target rate for the encoder given the channel parameters. // Inputs: estimated_bitrate_bps - the estimated network bitrate in bits/s. // actual_framerate - encoder frame rate. // fraction_lost - packet loss rate in % in the network. // round_trip_time_ms - round trip time in milliseconds. - virtual uint32_t SetTargetRates(uint32_t estimated_bitrate_bps, - int actual_framerate, - uint8_t fraction_lost, - int64_t round_trip_time_ms) = 0; - - virtual uint32_t SetTargetRates(uint32_t estimated_bitrate_bps, - std::vector loss_mask_vector, - int64_t round_trip_time_ms) = 0; - + uint32_t SetTargetRates(uint32_t estimated_bitrate_bps, + int actual_framerate, + uint8_t fraction_lost, + int64_t round_trip_time_ms); // Informs of encoded output. - virtual void UpdateWithEncodedData(const EncodedImage& encoded_image) = 0; + void UpdateWithEncodedData(const EncodedImage& encoded_image); - virtual void OnLossMaskVector(const std::vector loss_mask_vector) = 0; + private: + enum { kBitrateAverageWinMs = 1000 }; + + Clock* const clock_; + VCMProtectionCallback* const protection_callback_; + + rtc::CriticalSection crit_sect_; + std::unique_ptr loss_prot_logic_ + RTC_GUARDED_BY(crit_sect_); + size_t max_payload_size_ RTC_GUARDED_BY(crit_sect_); + + RTC_DISALLOW_COPY_AND_ASSIGN(ProtectionBitrateCalculator); }; } // namespace webrtc - #endif // MODULES_VIDEO_CODING_PROTECTION_BITRATE_CALCULATOR_H_ diff --git a/modules/video_coding/protection_bitrate_calculator_default.h b/modules/video_coding/protection_bitrate_calculator_default.h deleted file mode 100644 index 19fa7e1532..0000000000 --- a/modules/video_coding/protection_bitrate_calculator_default.h +++ /dev/null @@ -1,87 +0,0 @@ -/* - * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#ifndef MODULES_VIDEO_CODING_PROTECTION_BITRATE_CALCULATOR_DEFAULT_H_ -#define MODULES_VIDEO_CODING_PROTECTION_BITRATE_CALCULATOR_DEFAULT_H_ - -#include -#include -#include - -#include "modules/include/module_common_types.h" -#include "modules/video_coding/include/video_coding.h" -#include "modules/video_coding/media_opt_util.h" -#include "modules/video_coding/protection_bitrate_calculator.h" -#include "rtc_base/criticalsection.h" -#include "system_wrappers/include/clock.h" - -namespace webrtc { - -// ProtectionBitrateCalculator calculates how much of the allocated network -// capacity that can be used by an encoder and how much that -// is needed for redundant packets such as FEC and NACK. It uses an -// implementation of |VCMProtectionCallback| to set new FEC parameters and get -// the bitrate currently used for FEC and NACK. -// Usage: -// Setup by calling SetProtectionMethod and SetEncodingData. -// For each encoded image, call UpdateWithEncodedData. -// Each time the bandwidth estimate change, call SetTargetRates. SetTargetRates -// will return the bitrate that can be used by an encoder. -// A lock is used to protect internal states, so methods can be called on an -// arbitrary thread. -class ProtectionBitrateCalculatorDefault : public ProtectionBitrateCalculator { - public: - ProtectionBitrateCalculatorDefault( - Clock* clock, - VCMProtectionCallback* protection_callback); - ~ProtectionBitrateCalculatorDefault(); - - void SetProtectionMethod(bool enable_fec, bool enable_nack) override; - - // Informs media optimization of initial encoding state. - void SetEncodingData(size_t width, - size_t height, - size_t num_temporal_layers, - size_t max_payload_size) override; - - uint32_t SetTargetRates(uint32_t estimated_bitrate_bps, - std::vector loss_mask_vector, - int64_t round_trip_time_ms) override { - return 0; - } - - void OnLossMaskVector(const std::vector loss_mask_vector) override {} - - // Returns target rate for the encoder given the channel parameters. - // Inputs: estimated_bitrate_bps - the estimated network bitrate in bits/s. - // actual_framerate - encoder frame rate. - // fraction_lost - packet loss rate in % in the network. - // round_trip_time_ms - round trip time in milliseconds. - uint32_t SetTargetRates(uint32_t estimated_bitrate_bps, - int actual_framerate, - uint8_t fraction_lost, - int64_t round_trip_time_ms) override; - // Informs of encoded output. - void UpdateWithEncodedData(const EncodedImage& encoded_image) override; - - private: - enum { kBitrateAverageWinMs = 1000 }; - Clock* const clock_; - VCMProtectionCallback* const protection_callback_; - rtc::CriticalSection crit_sect_; - std::unique_ptr loss_prot_logic_ - RTC_GUARDED_BY(crit_sect_); - size_t max_payload_size_ RTC_GUARDED_BY(crit_sect_); - - RTC_DISALLOW_COPY_AND_ASSIGN(ProtectionBitrateCalculatorDefault); -}; - -} // namespace webrtc -#endif // MODULES_VIDEO_CODING_PROTECTION_BITRATE_CALCULATOR_DEFAULT_H_ diff --git a/modules/video_coding/protection_bitrate_calculator_unittest.cc b/modules/video_coding/protection_bitrate_calculator_unittest.cc index dd58bc2676..66e2fa6f84 100644 --- a/modules/video_coding/protection_bitrate_calculator_unittest.cc +++ b/modules/video_coding/protection_bitrate_calculator_unittest.cc @@ -8,7 +8,7 @@ * be found in the AUTHORS file in the root of the source tree. */ -#include "modules/video_coding/protection_bitrate_calculator_default.h" +#include "modules/video_coding/protection_bitrate_calculator.h" #include "system_wrappers/include/clock.h" #include "test/gtest.h" @@ -46,7 +46,7 @@ class ProtectionBitrateCalculatorTest : public ::testing::Test { SimulatedClock clock_; ProtectionCallback protection_callback_; - ProtectionBitrateCalculatorDefault media_opt_; + ProtectionBitrateCalculator media_opt_; }; TEST_F(ProtectionBitrateCalculatorTest, ProtectsUsingFecBitrate) { diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc index 38da14c962..6b406a0577 100644 --- a/video/video_send_stream.cc +++ b/video/video_send_stream.cc @@ -11,14 +11,12 @@ #include #include -#include #include #include #include #include #include "call/rtp_transport_controller_send_interface.h" -#include "call/video_send_stream.h" #include "common_types.h" // NOLINT(build/include) #include "common_video/include/video_bitrate_allocator.h" #include "modules/bitrate_controller/include/bitrate_controller.h" @@ -28,7 +26,6 @@ #include "modules/rtp_rtcp/include/rtp_rtcp.h" #include "modules/rtp_rtcp/source/rtp_sender.h" #include "modules/utility/include/process_thread.h" -#include "modules/video_coding/protection_bitrate_calculator_default.h" #include "modules/video_coding/utility/ivf_file_writer.h" #include "rtc_base/checks.h" #include "rtc_base/file.h" @@ -39,12 +36,11 @@ #include "system_wrappers/include/field_trial.h" #include "video/call_stats.h" #include "video/payload_router.h" +#include "call/video_send_stream.h" namespace webrtc { static const int kMinSendSidePacketHistorySize = 600; -static const int kSendSideSeqNumSetMaxSize = 15000; - namespace { // We don't do MTU discovery, so assume that we have the standard ethernet MTU. @@ -248,8 +244,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, public webrtc::OverheadObserver, public webrtc::VCMProtectionCallback, public VideoStreamEncoder::EncoderSink, - public VideoBitrateAllocationObserver, - public webrtc::PacketFeedbackObserver { + public VideoBitrateAllocationObserver { public: VideoSendStreamImpl( SendStatisticsProxy* stats_proxy, @@ -288,11 +283,6 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, void SetTransportOverhead(size_t transport_overhead_per_packet); - // From PacketFeedbackObserver. - void OnPacketAdded(uint32_t ssrc, uint16_t seq_num) override; - void OnPacketFeedbackVector( - const std::vector& packet_feedback_vector) override; - private: class CheckEncoderActivityTask; class EncoderReconfiguredTask; @@ -333,6 +323,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, void SignalEncoderActive(); const bool send_side_bwe_with_overhead_; + SendStatisticsProxy* const stats_proxy_; const VideoSendStream::Config* const config_; std::map suspended_ssrcs_; @@ -363,8 +354,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, VideoStreamEncoder* const video_stream_encoder_; EncoderRtcpFeedback encoder_feedback_; - const std::unique_ptr - protection_bitrate_calculator_; + ProtectionBitrateCalculator protection_bitrate_calculator_; RtcpBandwidthObserver* const bandwidth_observer_; // RtpRtcp modules, declared here as they use other members on construction. @@ -383,10 +373,6 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, size_t overhead_bytes_per_packet_ RTC_GUARDED_BY(overhead_bytes_per_packet_crit_); size_t transport_overhead_bytes_per_packet_; - - rtc::CriticalSection feedback_packet_seq_num_set_cs_; - std::set feedback_packet_seq_num_set_ - RTC_GUARDED_BY(&feedback_packet_seq_num_set_cs_); }; // TODO(tommi): See if there's a more elegant way to create a task that creates @@ -599,6 +585,7 @@ VideoSendStream::VideoSendStream( // Only signal target bitrate for screenshare streams, for now. video_stream_encoder_->SetBitrateObserver(send_stream_.get()); } + ReconfigureVideoEncoder(std::move(encoder_config)); } @@ -727,10 +714,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( encoder_feedback_(Clock::GetRealTimeClock(), config_->rtp.ssrcs, video_stream_encoder), - protection_bitrate_calculator_( - rtc::MakeUnique( - Clock::GetRealTimeClock(), - this)), + protection_bitrate_calculator_(Clock::GetRealTimeClock(), this), bandwidth_observer_(transport->send_side_cc()->GetBandwidthObserver()), rtp_rtcp_modules_(CreateRtpRtcpModules( config_->send_transport, @@ -827,8 +811,6 @@ VideoSendStreamImpl::VideoSendStreamImpl( config_->encoder_settings.payload_type, config_->encoder_settings.payload_name.c_str()); } - // Signal congestion controller this object is ready for OnPacket* callbacks. - transport_->send_side_cc()->RegisterPacketFeedbackObserver(this); RTC_DCHECK(config_->encoder_settings.encoder); RTC_DCHECK_GE(config_->encoder_settings.payload_type, 0); @@ -872,7 +854,7 @@ VideoSendStreamImpl::~VideoSendStreamImpl() { RTC_DCHECK(!payload_router_.IsActive()) << "VideoSendStreamImpl::Stop not called"; RTC_LOG(LS_INFO) << "~VideoSendStreamInternal: " << config_->ToString(); - transport_->send_side_cc()->DeRegisterPacketFeedbackObserver(this); + for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) { transport_->packet_router()->RemoveSendRtpModule(rtp_rtcp); delete rtp_rtcp; @@ -984,7 +966,7 @@ void VideoSendStreamImpl::OnEncoderConfigurationChanged( size_t number_of_temporal_layers = streams.back().temporal_layer_thresholds_bps.size() + 1; - protection_bitrate_calculator_->SetEncodingData( + protection_bitrate_calculator_.SetEncodingData( streams[0].width, streams[0].height, number_of_temporal_layers, config_->rtp.max_packet_size); @@ -1020,7 +1002,7 @@ EncodedImageCallback::Result VideoSendStreamImpl::OnEncodedImage( check_encoder_activity_task_->UpdateEncoderActivity(); } - protection_bitrate_calculator_->UpdateWithEncodedData(encoded_image); + protection_bitrate_calculator_.UpdateWithEncodedData(encoded_image); EncodedImageCallback::Result result = payload_router_.OnEncodedImage( encoded_image, codec_specific_info, fragmentation); @@ -1126,7 +1108,7 @@ void VideoSendStreamImpl::ConfigureProtection() { // Currently, both ULPFEC and FlexFEC use the same FEC rate calculation logic, // so enable that logic if either of those FEC schemes are enabled. - protection_bitrate_calculator_->SetProtectionMethod( + protection_bitrate_calculator_.SetProtectionMethod( flexfec_enabled || IsUlpfecEnabled(), nack_enabled); } @@ -1234,7 +1216,7 @@ uint32_t VideoSendStreamImpl::OnBitrateUpdated(uint32_t bitrate_bps, // Get the encoder target rate. It is the estimated network rate - // protection overhead. - encoder_target_rate_bps_ = protection_bitrate_calculator_->SetTargetRates( + encoder_target_rate_bps_ = protection_bitrate_calculator_.SetTargetRates( payload_bitrate_bps, stats_proxy_->GetSendFrameRate(), fraction_loss, rtt); @@ -1335,34 +1317,5 @@ void VideoSendStreamImpl::SetTransportOverhead( } } -void VideoSendStreamImpl::OnPacketAdded(uint32_t ssrc, uint16_t seq_num) { - const auto& ssrcs = config_->rtp.ssrcs; - if (std::find(ssrcs.begin(), ssrcs.end(), ssrc) != ssrcs.end()) { - rtc::CritScope lock(&feedback_packet_seq_num_set_cs_); - feedback_packet_seq_num_set_.insert(seq_num); - if (feedback_packet_seq_num_set_.size() > kSendSideSeqNumSetMaxSize) { - feedback_packet_seq_num_set_.erase(feedback_packet_seq_num_set_.begin()); - } - } -} - -void VideoSendStreamImpl::OnPacketFeedbackVector( - const std::vector& packet_feedback_vector) { - // Lost feedbacks are not considered to be lost packets. - std::vector loss_mask_vector; - rtc::CritScope lock(&feedback_packet_seq_num_set_cs_); - for (const PacketFeedback& packet : packet_feedback_vector) { - if (auto it = feedback_packet_seq_num_set_.find(packet.sequence_number) != - feedback_packet_seq_num_set_.end()) { - const bool lost = packet.arrival_time_ms == PacketFeedback::kNotReceived; - loss_mask_vector.push_back(lost); - feedback_packet_seq_num_set_.erase(it); - } - } - if (loss_mask_vector.empty()) - return; - protection_bitrate_calculator_->OnLossMaskVector(loss_mask_vector); -} - } // namespace internal } // namespace webrtc diff --git a/video/video_send_stream.h b/video/video_send_stream.h index d3ba62bda9..998250cd6e 100644 --- a/video/video_send_stream.h +++ b/video/video_send_stream.h @@ -19,6 +19,7 @@ #include "call/video_receive_stream.h" #include "call/video_send_stream.h" #include "common_video/libyuv/include/webrtc_libyuv.h" +#include "modules/video_coding/protection_bitrate_calculator.h" #include "rtc_base/criticalsection.h" #include "rtc_base/event.h" #include "rtc_base/task_queue.h"