From f5b2e519b4067e209d2cebbd4b1ef485d2a5c289 Mon Sep 17 00:00:00 2001 From: perkj Date: Tue, 5 Jul 2016 08:34:04 -0700 Subject: [PATCH] Fix stats for encoder target bitrate when target rate is zero. When the target bitrate is zero, currently VideoSendStream.Stats.target_media_bitrate_bps show the last set rate before the target was set to zero. BUG=webrtc::5687 b/29574845 Review-Url: https://codereview.webrtc.org/2122743003 Cr-Commit-Position: refs/heads/master@{#13386} --- webrtc/modules/video_coding/codec_database.cc | 7 ++---- webrtc/modules/video_coding/codec_database.h | 4 +--- .../modules/video_coding/generic_encoder.cc | 6 ----- webrtc/modules/video_coding/generic_encoder.h | 2 -- .../video_coding/include/video_coding.h | 8 +------ .../include/video_coding_defines.h | 6 ----- .../modules/video_coding/video_coding_impl.cc | 23 ++++--------------- .../modules/video_coding/video_coding_impl.h | 1 - webrtc/modules/video_coding/video_receiver.cc | 2 +- webrtc/modules/video_coding/video_sender.cc | 3 +-- .../video_coding/video_sender_unittest.cc | 3 +-- webrtc/video/end_to_end_tests.cc | 19 ++++++++++++++- webrtc/video/send_statistics_proxy.cc | 2 +- webrtc/video/send_statistics_proxy.h | 5 ++-- webrtc/video/video_send_stream.cc | 16 +++++++++++-- webrtc/video/video_send_stream.h | 1 + webrtc/video/vie_encoder.cc | 7 +----- webrtc/video/vie_encoder.h | 9 +------- 18 files changed, 49 insertions(+), 75 deletions(-) diff --git a/webrtc/modules/video_coding/codec_database.cc b/webrtc/modules/video_coding/codec_database.cc index 1baa414bce..c8c0b9cfdd 100644 --- a/webrtc/modules/video_coding/codec_database.cc +++ b/webrtc/modules/video_coding/codec_database.cc @@ -90,7 +90,6 @@ VCMExtDecoderMapItem::VCMExtDecoderMapItem( external_decoder_instance(external_decoder_instance) {} VCMCodecDataBase::VCMCodecDataBase( - VideoEncoderRateObserver* encoder_rate_observer, VCMEncodedFrameCallback* encoded_frame_callback) : number_of_cores_(0), max_payload_size_(kDefaultPayloadSize), @@ -101,7 +100,6 @@ VCMCodecDataBase::VCMCodecDataBase( encoder_payload_type_(0), external_encoder_(nullptr), internal_source_(false), - encoder_rate_observer_(encoder_rate_observer), encoded_frame_callback_(encoded_frame_callback), ptr_decoder_(nullptr), dec_map_(), @@ -245,9 +243,8 @@ bool VCMCodecDataBase::SetSendCodec(const VideoCodec* send_codec, DeleteEncoder(); RTC_DCHECK_EQ(encoder_payload_type_, send_codec_.plType) << "Encoder not registered for payload type " << send_codec_.plType; - ptr_encoder_.reset( - new VCMGenericEncoder(external_encoder_, encoder_rate_observer_, - encoded_frame_callback_, internal_source_)); + ptr_encoder_.reset(new VCMGenericEncoder( + external_encoder_, encoded_frame_callback_, internal_source_)); encoded_frame_callback_->SetInternalSource(internal_source_); if (ptr_encoder_->InitEncode(&send_codec_, number_of_cores_, max_payload_size_) < 0) { diff --git a/webrtc/modules/video_coding/codec_database.h b/webrtc/modules/video_coding/codec_database.h index 4ba72f7b36..5a7e875644 100644 --- a/webrtc/modules/video_coding/codec_database.h +++ b/webrtc/modules/video_coding/codec_database.h @@ -44,8 +44,7 @@ struct VCMExtDecoderMapItem { class VCMCodecDataBase { public: - VCMCodecDataBase(VideoEncoderRateObserver* encoder_rate_observer, - VCMEncodedFrameCallback* encoded_frame_callback); + explicit VCMCodecDataBase(VCMEncodedFrameCallback* encoded_frame_callback); ~VCMCodecDataBase(); // Sender Side @@ -154,7 +153,6 @@ class VCMCodecDataBase { uint8_t encoder_payload_type_; VideoEncoder* external_encoder_; bool internal_source_; - VideoEncoderRateObserver* const encoder_rate_observer_; VCMEncodedFrameCallback* const encoded_frame_callback_; std::unique_ptr ptr_encoder_; VCMGenericDecoder* ptr_decoder_; diff --git a/webrtc/modules/video_coding/generic_encoder.cc b/webrtc/modules/video_coding/generic_encoder.cc index 0cc3d89a85..e63da026fe 100644 --- a/webrtc/modules/video_coding/generic_encoder.cc +++ b/webrtc/modules/video_coding/generic_encoder.cc @@ -23,11 +23,9 @@ namespace webrtc { VCMGenericEncoder::VCMGenericEncoder( VideoEncoder* encoder, - VideoEncoderRateObserver* rate_observer, VCMEncodedFrameCallback* encoded_frame_callback, bool internal_source) : encoder_(encoder), - rate_observer_(rate_observer), vcm_encoded_frame_callback_(encoded_frame_callback), internal_source_(internal_source), encoder_params_({0, 0, 0, 0}), @@ -102,10 +100,6 @@ void VCMGenericEncoder::SetEncoderParameters(const EncoderParameters& params) { if (rates_have_changed) { uint32_t target_bitrate_kbps = (params.target_bitrate + 500) / 1000; encoder_->SetRates(target_bitrate_kbps, params.input_frame_rate); - if (rate_observer_) { - rate_observer_->OnSetRates(params.target_bitrate, - params.input_frame_rate); - } } } diff --git a/webrtc/modules/video_coding/generic_encoder.h b/webrtc/modules/video_coding/generic_encoder.h index 0493d319aa..9f73f36e5d 100644 --- a/webrtc/modules/video_coding/generic_encoder.h +++ b/webrtc/modules/video_coding/generic_encoder.h @@ -59,7 +59,6 @@ class VCMGenericEncoder { public: VCMGenericEncoder(VideoEncoder* encoder, - VideoEncoderRateObserver* rate_observer, VCMEncodedFrameCallback* encoded_frame_callback, bool internal_source); ~VCMGenericEncoder(); @@ -86,7 +85,6 @@ class VCMGenericEncoder { rtc::RaceChecker race_checker_; VideoEncoder* const encoder_ GUARDED_BY(race_checker_); - VideoEncoderRateObserver* const rate_observer_; VCMEncodedFrameCallback* const vcm_encoded_frame_callback_; const bool internal_source_; rtc::CriticalSection params_lock_; diff --git a/webrtc/modules/video_coding/include/video_coding.h b/webrtc/modules/video_coding/include/video_coding.h index 062dda7080..eeda8a2e34 100644 --- a/webrtc/modules/video_coding/include/video_coding.h +++ b/webrtc/modules/video_coding/include/video_coding.h @@ -72,21 +72,15 @@ class VideoCodingModule : public Module { enum ReceiverRobustness { kNone, kHardNack, kSoftNack, kReferenceSelection }; - static VideoCodingModule* Create( - Clock* clock, - VideoEncoderRateObserver* encoder_rate_observer, - VCMQMSettingsCallback* qm_settings_callback); + static VideoCodingModule* Create(Clock* clock, EventFactory* event_factory); static VideoCodingModule* Create( Clock* clock, - VideoEncoderRateObserver* encoder_rate_observer, VCMQMSettingsCallback* qm_settings_callback, NackSender* nack_sender, KeyFrameRequestSender* keyframe_request_sender, EncodedImageCallback* pre_decode_image_callback); - static VideoCodingModule* Create(Clock* clock, EventFactory* event_factory); - static VideoCodingModule* Create( Clock* clock, EventFactory* event_factory, diff --git a/webrtc/modules/video_coding/include/video_coding_defines.h b/webrtc/modules/video_coding/include/video_coding_defines.h index ba71803c7c..e27de47ee0 100644 --- a/webrtc/modules/video_coding/include/video_coding_defines.h +++ b/webrtc/modules/video_coding/include/video_coding_defines.h @@ -126,12 +126,6 @@ class VCMProtectionCallback { virtual ~VCMProtectionCallback() {} }; -class VideoEncoderRateObserver { - public: - virtual ~VideoEncoderRateObserver() {} - virtual void OnSetRates(uint32_t bitrate_bps, int framerate) = 0; -}; - // Callback class used for telling the user about what frame type needed to // continue decoding. // Typically a key frame when the stream has been corrupted in some way. diff --git a/webrtc/modules/video_coding/video_coding_impl.cc b/webrtc/modules/video_coding/video_coding_impl.cc index 8955e54db7..077f3368a3 100644 --- a/webrtc/modules/video_coding/video_coding_impl.cc +++ b/webrtc/modules/video_coding/video_coding_impl.cc @@ -73,12 +73,11 @@ class VideoCodingModuleImpl : public VideoCodingModule { public: VideoCodingModuleImpl(Clock* clock, EventFactory* event_factory, - VideoEncoderRateObserver* encoder_rate_observer, NackSender* nack_sender, KeyFrameRequestSender* keyframe_request_sender, EncodedImageCallback* pre_decode_image_callback) : VideoCodingModule(), - sender_(clock, &post_encode_callback_, encoder_rate_observer, nullptr), + sender_(clock, &post_encode_callback_, nullptr), receiver_(clock, event_factory, pre_decode_image_callback, @@ -263,29 +262,15 @@ void VideoCodingModule::Codec(VideoCodecType codecType, VideoCodec* codec) { VCMCodecDataBase::Codec(codecType, codec); } -// Create method for current interface, will be removed when the -// new jitter buffer is in place. -VideoCodingModule* VideoCodingModule::Create( - Clock* clock, - VideoEncoderRateObserver* encoder_rate_observer, - VCMQMSettingsCallback* qm_settings_callback) { - return VideoCodingModule::Create(clock, encoder_rate_observer, - qm_settings_callback, - nullptr, // NackSender - nullptr, // KeyframeRequestSender - nullptr); // Pre-decode image callback -} - // Create method for the new jitter buffer. VideoCodingModule* VideoCodingModule::Create( Clock* clock, - VideoEncoderRateObserver* encoder_rate_observer, VCMQMSettingsCallback* qm_settings_callback, NackSender* nack_sender, KeyFrameRequestSender* keyframe_request_sender, EncodedImageCallback* pre_decode_image_callback) { - return new VideoCodingModuleImpl(clock, nullptr, encoder_rate_observer, - nack_sender, keyframe_request_sender, + return new VideoCodingModuleImpl(clock, nullptr, nack_sender, + keyframe_request_sender, pre_decode_image_callback); } @@ -306,7 +291,7 @@ VideoCodingModule* VideoCodingModule::Create( KeyFrameRequestSender* keyframe_request_sender) { assert(clock); assert(event_factory); - return new VideoCodingModuleImpl(clock, event_factory, nullptr, nack_sender, + return new VideoCodingModuleImpl(clock, event_factory, nack_sender, keyframe_request_sender, nullptr); } diff --git a/webrtc/modules/video_coding/video_coding_impl.h b/webrtc/modules/video_coding/video_coding_impl.h index 00bce1b99c..6bafd5ab0f 100644 --- a/webrtc/modules/video_coding/video_coding_impl.h +++ b/webrtc/modules/video_coding/video_coding_impl.h @@ -58,7 +58,6 @@ class VideoSender : public Module { VideoSender(Clock* clock, EncodedImageCallback* post_encode_callback, - VideoEncoderRateObserver* encoder_rate_observer, VCMSendStatisticsCallback* send_stats_callback); ~VideoSender(); diff --git a/webrtc/modules/video_coding/video_receiver.cc b/webrtc/modules/video_coding/video_receiver.cc index ae59cf105c..478a798f0e 100644 --- a/webrtc/modules/video_coding/video_receiver.cc +++ b/webrtc/modules/video_coding/video_receiver.cc @@ -45,7 +45,7 @@ VideoReceiver::VideoReceiver(Clock* clock, _scheduleKeyRequest(false), drop_frames_until_keyframe_(false), max_nack_list_size_(0), - _codecDataBase(nullptr, nullptr), + _codecDataBase(nullptr), pre_decode_image_callback_(pre_decode_image_callback), _receiveStatsTimer(1000, clock_), _retransmissionTimer(10, clock_), diff --git a/webrtc/modules/video_coding/video_sender.cc b/webrtc/modules/video_coding/video_sender.cc index f26be34776..aecc60cde8 100644 --- a/webrtc/modules/video_coding/video_sender.cc +++ b/webrtc/modules/video_coding/video_sender.cc @@ -26,14 +26,13 @@ namespace vcm { VideoSender::VideoSender(Clock* clock, EncodedImageCallback* post_encode_callback, - VideoEncoderRateObserver* encoder_rate_observer, VCMSendStatisticsCallback* send_stats_callback) : clock_(clock), _encoder(nullptr), _mediaOpt(clock_), _encodedFrameCallback(post_encode_callback, &_mediaOpt), send_stats_callback_(send_stats_callback), - _codecDataBase(encoder_rate_observer, &_encodedFrameCallback), + _codecDataBase(&_encodedFrameCallback), frame_dropper_enabled_(true), _sendStatsTimer(1000, clock_), current_codec_(), diff --git a/webrtc/modules/video_coding/video_sender_unittest.cc b/webrtc/modules/video_coding/video_sender_unittest.cc index 5324ceeb0b..923144e3d8 100644 --- a/webrtc/modules/video_coding/video_sender_unittest.cc +++ b/webrtc/modules/video_coding/video_sender_unittest.cc @@ -180,8 +180,7 @@ class TestVideoSender : public ::testing::Test { TestVideoSender() : clock_(1000), encoded_frame_callback_(&clock_) {} void SetUp() override { - sender_.reset( - new VideoSender(&clock_, &encoded_frame_callback_, nullptr, nullptr)); + sender_.reset(new VideoSender(&clock_, &encoded_frame_callback_, nullptr)); } void AddFrame() { diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index e5b12ce0b1..8e3105f741 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -2473,7 +2473,14 @@ TEST_F(EndToEndTest, ReportsSetEncoderRates) { void PerformTest() override { ASSERT_TRUE(Wait()) << "Timed out while waiting for encoder SetRates() call."; - // Wait for GetStats to report a corresponding bitrate. + WaitForEncoderTargetBitrateMatchStats(); + send_stream_->Stop(); + WaitForStatsReportZeroTargetBitrate(); + send_stream_->Start(); + WaitForEncoderTargetBitrateMatchStats(); + } + + void WaitForEncoderTargetBitrateMatchStats() { for (int i = 0; i < kDefaultTimeoutMs; ++i) { VideoSendStream::Stats stats = send_stream_->GetStats(); { @@ -2489,6 +2496,16 @@ TEST_F(EndToEndTest, ReportsSetEncoderRates) { << "Timed out waiting for stats reporting the currently set bitrate."; } + void WaitForStatsReportZeroTargetBitrate() { + for (int i = 0; i < kDefaultTimeoutMs; ++i) { + if (send_stream_->GetStats().target_media_bitrate_bps == 0) { + return; + } + SleepMs(1); + } + FAIL() << "Timed out waiting for stats reporting zero bitrate."; + } + private: rtc::CriticalSection crit_; VideoSendStream* send_stream_; diff --git a/webrtc/video/send_statistics_proxy.cc b/webrtc/video/send_statistics_proxy.cc index abaa438005..6815eb3d71 100644 --- a/webrtc/video/send_statistics_proxy.cc +++ b/webrtc/video/send_statistics_proxy.cc @@ -418,7 +418,7 @@ void SendStatisticsProxy::OnInactiveSsrc(uint32_t ssrc) { stats->width = 0; } -void SendStatisticsProxy::OnSetRates(uint32_t bitrate_bps, int framerate) { +void SendStatisticsProxy::OnSetEncoderTargetRate(uint32_t bitrate_bps) { rtc::CritScope lock(&crit_); stats_.target_media_bitrate_bps = bitrate_bps; } diff --git a/webrtc/video/send_statistics_proxy.h b/webrtc/video/send_statistics_proxy.h index 60d50ec6f0..fa8b3ec5bb 100644 --- a/webrtc/video/send_statistics_proxy.h +++ b/webrtc/video/send_statistics_proxy.h @@ -36,7 +36,6 @@ class SendStatisticsProxy : public CpuOveruseMetricsObserver, public StreamDataCountersCallback, public BitrateStatisticsObserver, public FrameCountObserver, - public VideoEncoderRateObserver, public SendSideDelayObserver { public: static const int kStatsTimeoutMs; @@ -63,8 +62,8 @@ class SendStatisticsProxy : public CpuOveruseMetricsObserver, // how stats are collected. void SetContentType(VideoEncoderConfig::ContentType content_type); - // Implements VideoEncoderRateObserver. - void OnSetRates(uint32_t bitrate_bps, int framerate) override; + // Used to update the encoder target rate. + void OnSetEncoderTargetRate(uint32_t bitrate_bps); // Implements CpuOveruseMetricsObserver. void OnEncodedFrameTimeMeasured(int encode_time_ms, diff --git a/webrtc/video/video_send_stream.cc b/webrtc/video/video_send_stream.cc index eeb9580b6b..971abebff2 100644 --- a/webrtc/video/video_send_stream.cc +++ b/webrtc/video/video_send_stream.cc @@ -396,6 +396,7 @@ VideoSendStream::VideoSendStream( encoder_thread_(EncoderThreadFunction, this, "EncoderThread"), encoder_wakeup_event_(false, false), stop_encoder_thread_(0), + encoder_max_bitrate_bps_(0), state_(State::kStopped), overuse_detector_( Clock::GetRealTimeClock(), @@ -631,6 +632,7 @@ void VideoSendStream::EncoderProcess() { } else if (*pending_state_change == State::kStopped) { bitrate_allocator_->RemoveObserver(this); vie_encoder_.OnBitrateUpdated(0, 0, 0); + stats_proxy_.OnSetEncoderTargetRate(0); state_ = State::kStopped; LOG_F(LS_INFO) << "Encoder stopped."; } @@ -684,6 +686,7 @@ void VideoSendStream::ReconfigureVideoEncoder( config_.encoder_settings.payload_type); { rtc::CritScope lock(&encoder_settings_crit_); + encoder_max_bitrate_bps_ = video_codec.maxBitrate * 1000; pending_encoder_settings_.reset(new EncoderSettings({video_codec, config})); } encoder_wakeup_event_.Set(); @@ -875,9 +878,18 @@ uint32_t VideoSendStream::OnBitrateUpdated(uint32_t bitrate_bps, uint32_t encoder_target_rate_bps = protection_bitrate_calculator_.SetTargetRates( bitrate_bps, stats_proxy_.GetSendFrameRate(), fraction_loss, rtt); - vie_encoder_.OnBitrateUpdated(encoder_target_rate_bps, fraction_loss, rtt); - return bitrate_bps - encoder_target_rate_bps; + uint32_t protection_bitrate = bitrate_bps - encoder_target_rate_bps; + { + // Limit the target bitrate to the configured max bitrate. + rtc::CritScope lock(&encoder_settings_crit_); + encoder_target_rate_bps = + std::min(encoder_max_bitrate_bps_, encoder_target_rate_bps); + } + vie_encoder_.OnBitrateUpdated(encoder_target_rate_bps, fraction_loss, rtt); + stats_proxy_.OnSetEncoderTargetRate(encoder_target_rate_bps); + + return protection_bitrate; } int VideoSendStream::ProtectionRequest(const FecProtectionParams* delta_params, diff --git a/webrtc/video/video_send_stream.h b/webrtc/video/video_send_stream.h index f9c6cdc0d6..6e2d41c237 100644 --- a/webrtc/video/video_send_stream.h +++ b/webrtc/video/video_send_stream.h @@ -138,6 +138,7 @@ class VideoSendStream : public webrtc::VideoSendStream, rtc::CriticalSection encoder_settings_crit_; std::unique_ptr pending_encoder_settings_ GUARDED_BY(encoder_settings_crit_); + uint32_t encoder_max_bitrate_bps_ GUARDED_BY(encoder_settings_crit_); enum class State { kStopped, // VideoSendStream::Start has not yet been called. diff --git a/webrtc/video/vie_encoder.cc b/webrtc/video/vie_encoder.cc index e85a05fd42..30322c298f 100644 --- a/webrtc/video/vie_encoder.cc +++ b/webrtc/video/vie_encoder.cc @@ -35,7 +35,7 @@ ViEEncoder::ViEEncoder(uint32_t number_of_cores, : number_of_cores_(number_of_cores), sink_(sink), vp_(VideoProcessing::Create()), - video_sender_(Clock::GetRealTimeClock(), this, this, this), + video_sender_(Clock::GetRealTimeClock(), this, this), stats_proxy_(stats_proxy), overuse_detector_(overuse_detector), time_of_last_frame_activity_ms_(std::numeric_limits::max()), @@ -193,11 +193,6 @@ int64_t ViEEncoder::time_of_last_frame_activity_ms() { return time_of_last_frame_activity_ms_; } -void ViEEncoder::OnSetRates(uint32_t bitrate_bps, int framerate) { - if (stats_proxy_) - stats_proxy_->OnSetRates(bitrate_bps, framerate); -} - int32_t ViEEncoder::Encoded(const EncodedImage& encoded_image, const CodecSpecificInfo* codec_specific_info, const RTPFragmentationHeader* fragmentation) { diff --git a/webrtc/video/vie_encoder.h b/webrtc/video/vie_encoder.h index 098d67f359..bf090fe75c 100644 --- a/webrtc/video/vie_encoder.h +++ b/webrtc/video/vie_encoder.h @@ -48,8 +48,7 @@ class VideoEncoder; // 4. Call SetEncoder with the codec settings and the object that shall receive // the encoded bit stream. // 5. For each available raw video frame call EncodeVideoFrame. -class ViEEncoder : public VideoEncoderRateObserver, - public EncodedImageCallback, +class ViEEncoder : public EncodedImageCallback, public VCMSendStatisticsCallback { public: friend class ViEBitrateObserver; @@ -81,12 +80,6 @@ class ViEEncoder : public VideoEncoderRateObserver, // an encoded frame. int64_t time_of_last_frame_activity_ms(); - // Implements VideoEncoderRateObserver. - // TODO(perkj): Refactor VideoEncoderRateObserver. This is only used for - // stats. The stats should be set in VideoSendStream instead. - // |bitrate_bps| is the target bitrate and |framerate| is the input frame - // rate so it has nothing to do with the actual encoder. - void OnSetRates(uint32_t bitrate_bps, int framerate) override; // Implements EncodedImageCallback. int32_t Encoded(const EncodedImage& encoded_image,