From 3d622d6e5c427b835888d17bf3d3ddd416bc3f39 Mon Sep 17 00:00:00 2001 From: Elad Alon Date: Thu, 9 May 2019 22:31:08 +0200 Subject: [PATCH] Revert "Refactor handling of configuration overrides from Vp8FrameBufferController" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 4d6795f828bc4f2b050405b0ff73d4020b2a2963. Reason for revert: chromium:961253 Original change's description: > Refactor handling of configuration overrides from Vp8FrameBufferController > > Make Vp8FrameBufferController::UpdateConfiguration return a set > of desired overrides. These overrides are cumulative with > previously returned override sets. > > Bug: webrtc:10382 > Change-Id: I1aa9544ae0cf6c57115e80963b3bbcdc3101db5e > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/134649 > Reviewed-by: Rasmus Brandt > Reviewed-by: Erik Språng > Commit-Queue: Elad Alon > Cr-Commit-Position: refs/heads/master@{#27835} TBR=brandtr@webrtc.org,eladalon@webrtc.org,sprang@webrtc.org Bug: chromium:961253 Change-Id: I06f0eafd4f38c441ddbdfeebae8055b02465eb9b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/135940 Commit-Queue: Elad Alon Reviewed-by: Elad Alon Cr-Commit-Position: refs/heads/master@{#27900} --- .../vp8_frame_buffer_controller.h | 74 +++----- api/video_codecs/vp8_temporal_layers.cc | 12 +- api/video_codecs/vp8_temporal_layers.h | 4 +- .../codecs/vp8/default_temporal_layers.cc | 34 ++-- .../codecs/vp8/default_temporal_layers.h | 4 +- .../vp8/default_temporal_layers_unittest.cc | 35 ++-- .../codecs/vp8/libvpx_vp8_encoder.cc | 179 +++++++----------- .../codecs/vp8/libvpx_vp8_encoder.h | 3 - .../codecs/vp8/screenshare_layers.cc | 67 +++---- .../codecs/vp8/screenshare_layers.h | 17 +- .../codecs/vp8/screenshare_layers_unittest.cc | 60 +++--- .../simulcast_rate_allocator_unittest.cc | 3 +- video/video_stream_encoder.cc | 4 +- 13 files changed, 203 insertions(+), 293 deletions(-) diff --git a/api/video_codecs/vp8_frame_buffer_controller.h b/api/video_codecs/vp8_frame_buffer_controller.h index bcfbd9763f..f71740e848 100644 --- a/api/video_codecs/vp8_frame_buffer_controller.h +++ b/api/video_codecs/vp8_frame_buffer_controller.h @@ -11,7 +11,6 @@ #ifndef API_VIDEO_CODECS_VP8_FRAME_BUFFER_CONTROLLER_H_ #define API_VIDEO_CODECS_VP8_FRAME_BUFFER_CONTROLLER_H_ -#include #include #include @@ -47,48 +46,34 @@ namespace webrtc { struct CodecSpecificInfo; -// Each member represents an override of the VPX configuration if the optional -// value is set. +// TODO(eladalon): This configuration is temporal-layers specific; refactor. struct Vp8EncoderConfig { - struct TemporalLayerConfig { - bool operator!=(const TemporalLayerConfig& other) const { - return ts_number_layers != other.ts_number_layers || - ts_target_bitrate != other.ts_target_bitrate || - ts_rate_decimator != other.ts_rate_decimator || - ts_periodicity != other.ts_periodicity || - ts_layer_id != other.ts_layer_id; - } + static constexpr size_t kMaxPeriodicity = 16; + static constexpr size_t kMaxLayers = 5; - static constexpr size_t kMaxPeriodicity = 16; - static constexpr size_t kMaxLayers = 5; + // Number of active temporal layers. Set to 0 if not used. + uint32_t ts_number_layers; + // Arrays of length |ts_number_layers|, indicating (cumulative) target bitrate + // and rate decimator (e.g. 4 if every 4th frame is in the given layer) for + // each active temporal layer, starting with temporal id 0. + uint32_t ts_target_bitrate[kMaxLayers]; + uint32_t ts_rate_decimator[kMaxLayers]; - // Number of active temporal layers. Set to 0 if not used. - uint32_t ts_number_layers; - - // Arrays of length |ts_number_layers|, indicating (cumulative) target - // bitrate and rate decimator (e.g. 4 if every 4th frame is in the given - // layer) for each active temporal layer, starting with temporal id 0. - std::array ts_target_bitrate; - std::array ts_rate_decimator; - - // The periodicity of the temporal pattern. Set to 0 if not used. - uint32_t ts_periodicity; - - // Array of length |ts_periodicity| indicating the sequence of temporal id's - // to assign to incoming frames. - std::array ts_layer_id; - }; - - absl::optional temporal_layer_config; + // The periodicity of the temporal pattern. Set to 0 if not used. + uint32_t ts_periodicity; + // Array of length |ts_periodicity| indicating the sequence of temporal id's + // to assign to incoming frames. + uint32_t ts_layer_id[kMaxPeriodicity]; // Target bitrate, in bps. - absl::optional rc_target_bitrate; + uint32_t rc_target_bitrate; - // Clamp QP to max. Use 0 to disable clamping. - absl::optional rc_max_quantizer; + // Clamp QP to min/max. Use 0 to disable clamping. + uint32_t rc_min_quantizer; + uint32_t rc_max_quantizer; - // Error resilience mode. - absl::optional g_error_resilient; + // If has_value(), override error resilience to value(). + absl::optional error_resilient; }; // This interface defines a way of delegating the logic of buffer management. @@ -98,10 +83,6 @@ class Vp8FrameBufferController { public: virtual ~Vp8FrameBufferController() = default; - // Set limits on QP. - // The limits are suggestion-only; the controller is allowed to exceed them. - virtual void SetQpLimits(size_t stream_index, int min_qp, int max_qp) = 0; - // Number of streamed controlled by |this|. virtual size_t StreamCount() const = 0; @@ -122,13 +103,12 @@ class Vp8FrameBufferController { const std::vector& bitrates_bps, int framerate_fps) = 0; - // Called by the encoder before encoding a frame. Returns a set of overrides - // the controller wishes to enact in the encoder's configuration. - // If a value is not overridden, previous overrides are still in effect. - // (It is therefore not possible to go from a specific override to - // no-override. Once the controller takes responsibility over a value, it - // must maintain responsibility for it.) - virtual Vp8EncoderConfig UpdateConfiguration(size_t stream_index) = 0; + // Called by the encoder before encoding a frame. |cfg| contains the current + // configuration. If the encoder wishes any part of that to be changed before + // the encode step, |cfg| should be changed and then return true. If false is + // returned, the encoder will proceed without updating the configuration. + virtual bool UpdateConfiguration(size_t stream_index, + Vp8EncoderConfig* cfg) = 0; // Returns the recommended VP8 encode flags needed. // The timestamp may be used as both a time and a unique identifier, and so diff --git a/api/video_codecs/vp8_temporal_layers.cc b/api/video_codecs/vp8_temporal_layers.cc index 22916a7dd5..7f7d8ad3df 100644 --- a/api/video_codecs/vp8_temporal_layers.cc +++ b/api/video_codecs/vp8_temporal_layers.cc @@ -28,13 +28,6 @@ Vp8TemporalLayers::Vp8TemporalLayers( })); } -void Vp8TemporalLayers::SetQpLimits(size_t stream_index, - int min_qp, - int max_qp) { - RTC_DCHECK_LT(stream_index, controllers_.size()); - return controllers_[stream_index]->SetQpLimits(0, min_qp, max_qp); -} - size_t Vp8TemporalLayers::StreamCount() const { return controllers_.size(); } @@ -54,9 +47,10 @@ void Vp8TemporalLayers::OnRatesUpdated( framerate_fps); } -Vp8EncoderConfig Vp8TemporalLayers::UpdateConfiguration(size_t stream_index) { +bool Vp8TemporalLayers::UpdateConfiguration(size_t stream_index, + Vp8EncoderConfig* cfg) { RTC_DCHECK_LT(stream_index, controllers_.size()); - return controllers_[stream_index]->UpdateConfiguration(0); + return controllers_[stream_index]->UpdateConfiguration(0, cfg); } Vp8FrameConfig Vp8TemporalLayers::NextFrameConfig(size_t stream_index, diff --git a/api/video_codecs/vp8_temporal_layers.h b/api/video_codecs/vp8_temporal_layers.h index 4194352ded..3864705c98 100644 --- a/api/video_codecs/vp8_temporal_layers.h +++ b/api/video_codecs/vp8_temporal_layers.h @@ -35,8 +35,6 @@ class Vp8TemporalLayers final : public Vp8FrameBufferController { std::vector>&& controllers); ~Vp8TemporalLayers() override = default; - void SetQpLimits(size_t stream_index, int min_qp, int max_qp) override; - size_t StreamCount() const override; bool SupportsEncoderFrameDropping(size_t stream_index) const override; @@ -45,7 +43,7 @@ class Vp8TemporalLayers final : public Vp8FrameBufferController { const std::vector& bitrates_bps, int framerate_fps) override; - Vp8EncoderConfig UpdateConfiguration(size_t stream_index) override; + bool UpdateConfiguration(size_t stream_index, Vp8EncoderConfig* cfg) override; Vp8FrameConfig NextFrameConfig(size_t stream_index, uint32_t rtp_timestamp) override; diff --git a/modules/video_coding/codecs/vp8/default_temporal_layers.cc b/modules/video_coding/codecs/vp8/default_temporal_layers.cc index 339e81dd16..8fa8f0d774 100644 --- a/modules/video_coding/codecs/vp8/default_temporal_layers.cc +++ b/modules/video_coding/codecs/vp8/default_temporal_layers.cc @@ -10,6 +10,7 @@ #include "modules/video_coding/codecs/vp8/default_temporal_layers.h" #include +#include #include #include @@ -250,13 +251,6 @@ DefaultTemporalLayers::DefaultTemporalLayers(int number_of_temporal_layers) DefaultTemporalLayers::~DefaultTemporalLayers() = default; -void DefaultTemporalLayers::SetQpLimits(size_t stream_index, - int min_qp, - int max_qp) { - RTC_DCHECK_LT(stream_index, StreamCount()); - // Ignore. -} - size_t DefaultTemporalLayers::StreamCount() const { return 1; } @@ -284,34 +278,28 @@ void DefaultTemporalLayers::OnRatesUpdated( } } -Vp8EncoderConfig DefaultTemporalLayers::UpdateConfiguration( - size_t stream_index) { +bool DefaultTemporalLayers::UpdateConfiguration(size_t stream_index, + Vp8EncoderConfig* cfg) { RTC_DCHECK_LT(stream_index, StreamCount()); - Vp8EncoderConfig config; - if (!new_bitrates_bps_) { - return config; + return false; } - config.temporal_layer_config.emplace(); - Vp8EncoderConfig::TemporalLayerConfig& ts_config = - config.temporal_layer_config.value(); - for (size_t i = 0; i < num_layers_; ++i) { - ts_config.ts_target_bitrate[i] = (*new_bitrates_bps_)[i] / 1000; + cfg->ts_target_bitrate[i] = (*new_bitrates_bps_)[i] / 1000; // ..., 4, 2, 1 - ts_config.ts_rate_decimator[i] = 1 << (num_layers_ - i - 1); + cfg->ts_rate_decimator[i] = 1 << (num_layers_ - i - 1); } - ts_config.ts_number_layers = num_layers_; - ts_config.ts_periodicity = temporal_ids_.size(); - std::copy(temporal_ids_.begin(), temporal_ids_.end(), - ts_config.ts_layer_id.begin()); + cfg->ts_number_layers = num_layers_; + cfg->ts_periodicity = temporal_ids_.size(); + memcpy(cfg->ts_layer_id, &temporal_ids_[0], + sizeof(unsigned int) * temporal_ids_.size()); new_bitrates_bps_.reset(); - return config; + return true; } bool DefaultTemporalLayers::IsSyncFrame(const Vp8FrameConfig& config) const { diff --git a/modules/video_coding/codecs/vp8/default_temporal_layers.h b/modules/video_coding/codecs/vp8/default_temporal_layers.h index cb1cbbeef3..34ed711c87 100644 --- a/modules/video_coding/codecs/vp8/default_temporal_layers.h +++ b/modules/video_coding/codecs/vp8/default_temporal_layers.h @@ -34,8 +34,6 @@ class DefaultTemporalLayers final : public Vp8FrameBufferController { explicit DefaultTemporalLayers(int number_of_temporal_layers); ~DefaultTemporalLayers() override; - void SetQpLimits(size_t stream_index, int min_qp, int max_qp) override; - size_t StreamCount() const override; bool SupportsEncoderFrameDropping(size_t stream_index) const override; @@ -50,7 +48,7 @@ class DefaultTemporalLayers final : public Vp8FrameBufferController { const std::vector& bitrates_bps, int framerate_fps) override; - Vp8EncoderConfig UpdateConfiguration(size_t stream_index) override; + bool UpdateConfiguration(size_t stream_index, Vp8EncoderConfig* cfg) override; void OnEncodeDone(size_t stream_index, uint32_t rtp_timestamp, diff --git a/modules/video_coding/codecs/vp8/default_temporal_layers_unittest.cc b/modules/video_coding/codecs/vp8/default_temporal_layers_unittest.cc index 2c419dabe2..8f01e597f6 100644 --- a/modules/video_coding/codecs/vp8/default_temporal_layers_unittest.cc +++ b/modules/video_coding/codecs/vp8/default_temporal_layers_unittest.cc @@ -23,8 +23,6 @@ #include "test/gtest.h" #include "vpx/vp8cx.h" -// TODO(bugs.webrtc.org/10582): Test the behavior of UpdateConfiguration(). - namespace webrtc { namespace test { namespace { @@ -121,11 +119,12 @@ TEST_F(TemporalLayersTest, 2Layers) { constexpr int kNumLayers = 2; DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); constexpr size_t kPatternSize = 4; constexpr size_t kRepetitions = 4; @@ -163,11 +162,12 @@ TEST_F(TemporalLayersTest, 3Layers) { constexpr int kNumLayers = 3; DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); int expected_flags[16] = { kTemporalUpdateLast, @@ -217,11 +217,12 @@ TEST_F(TemporalLayersTest, Alternative3Layers) { ScopedFieldTrials field_trial("WebRTC-UseShortVP8TL3Pattern/Enabled/"); DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); int expected_flags[8] = {kTemporalUpdateLast, kTemporalUpdateAltrefWithoutDependency, @@ -259,11 +260,12 @@ TEST_F(TemporalLayersTest, SearchOrder) { ScopedFieldTrials field_trial("WebRTC-UseShortVP8TL3Pattern/Enabled/"); DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); // Use a repeating pattern of tl 0, 2, 1, 2. // Tl 0, 1, 2 update last, golden, altref respectively. @@ -302,11 +304,12 @@ TEST_F(TemporalLayersTest, SearchOrderWithDrop) { ScopedFieldTrials field_trial("WebRTC-UseShortVP8TL3Pattern/Enabled/"); DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); // Use a repeating pattern of tl 0, 2, 1, 2. // Tl 0, 1, 2 update last, golden, altref respectively. @@ -341,11 +344,12 @@ TEST_F(TemporalLayersTest, 4Layers) { constexpr int kNumLayers = 4; DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); int expected_flags[16] = { kTemporalUpdateLast, kTemporalUpdateNoneNoRefGoldenAltRef, @@ -396,11 +400,12 @@ TEST_F(TemporalLayersTest, DoesNotReferenceDroppedFrames) { ScopedFieldTrials field_trial("WebRTC-UseShortVP8TL3Pattern/Enabled/"); DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); // Start with a keyframe. uint32_t timestamp = 0; @@ -482,11 +487,12 @@ TEST_F(TemporalLayersTest, DoesNotReferenceUnlessGuaranteedToExist) { // Tl 0, 1 updates last, golden respectively. Altref is always last keyframe. DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); // Start with a keyframe. uint32_t timestamp = 0; @@ -551,11 +557,12 @@ TEST_F(TemporalLayersTest, DoesNotReferenceUnlessGuaranteedToExistLongDelay) { ScopedFieldTrials field_trial("WebRTC-UseShortVP8TL3Pattern/Enabled/"); DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); // Start with a keyframe. uint32_t timestamp = 0; @@ -611,11 +618,12 @@ TEST_F(TemporalLayersTest, KeyFrame) { constexpr int kNumLayers = 3; DefaultTemporalLayers tl(kNumLayers); DefaultTemporalLayersChecker checker(kNumLayers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated(0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, kNumLayers), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); int expected_flags[8] = { kTemporalUpdateLastRefAltRef, @@ -727,10 +735,11 @@ INSTANTIATE_TEST_SUITE_P(DefaultTemporalLayersTest, TEST_P(TemporalLayersReferenceTest, ValidFrameConfigs) { const int num_layers = GetParam(); DefaultTemporalLayers tl(num_layers); + Vp8EncoderConfig cfg; tl.OnRatesUpdated( 0, GetTemporalLayerRates(kDefaultBytesPerFrame, kDefaultFramerate, 1), kDefaultFramerate); - tl.UpdateConfiguration(0); + tl.UpdateConfiguration(0, &cfg); // Run through the pattern and store the frame dependencies, plus keep track // of the buffer state; which buffers references which temporal layers (if diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc index d35ec29785..86da3a712d 100644 --- a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc +++ b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc @@ -15,7 +15,6 @@ #include #include -#include #include #include #include @@ -146,74 +145,58 @@ void UpdateRateSettings(vpx_codec_enc_cfg_t* config, config->rc_dropframe_thresh = new_settings.rc_dropframe_thresh; } -static_assert(Vp8EncoderConfig::TemporalLayerConfig::kMaxPeriodicity == - VPX_TS_MAX_PERIODICITY, +static_assert(Vp8EncoderConfig::kMaxPeriodicity == VPX_TS_MAX_PERIODICITY, "Vp8EncoderConfig::kMaxPeriodicity must be kept in sync with the " "constant in libvpx."); -static_assert(Vp8EncoderConfig::TemporalLayerConfig::kMaxLayers == - VPX_TS_MAX_LAYERS, +static_assert(Vp8EncoderConfig::kMaxLayers == VPX_TS_MAX_LAYERS, "Vp8EncoderConfig::kMaxLayers must be kept in sync with the " "constant in libvpx."); -// Allow a newer value to override a current value only if the new value -// is set. -template -bool MaybeSetNewValue(const absl::optional& new_value, - absl::optional* base_value) { - if (new_value.has_value() && new_value != *base_value) { - *base_value = new_value; - return true; - } else { - return false; +static Vp8EncoderConfig GetEncoderConfig(vpx_codec_enc_cfg* vpx_config) { + Vp8EncoderConfig config; + + config.ts_number_layers = vpx_config->ts_number_layers; + memcpy(config.ts_target_bitrate, vpx_config->ts_target_bitrate, + sizeof(unsigned int) * Vp8EncoderConfig::kMaxLayers); + memcpy(config.ts_rate_decimator, vpx_config->ts_rate_decimator, + sizeof(unsigned int) * Vp8EncoderConfig::kMaxLayers); + config.ts_periodicity = vpx_config->ts_periodicity; + memcpy(config.ts_layer_id, vpx_config->ts_layer_id, + sizeof(unsigned int) * Vp8EncoderConfig::kMaxPeriodicity); + config.rc_target_bitrate = vpx_config->rc_target_bitrate; + config.rc_min_quantizer = vpx_config->rc_min_quantizer; + config.rc_max_quantizer = vpx_config->rc_max_quantizer; + + return config; +} + +static void FillInEncoderConfig(vpx_codec_enc_cfg* vpx_config, + const Vp8EncoderConfig& config) { + vpx_config->ts_number_layers = config.ts_number_layers; + memcpy(vpx_config->ts_target_bitrate, config.ts_target_bitrate, + sizeof(unsigned int) * Vp8EncoderConfig::kMaxLayers); + memcpy(vpx_config->ts_rate_decimator, config.ts_rate_decimator, + sizeof(unsigned int) * Vp8EncoderConfig::kMaxLayers); + vpx_config->ts_periodicity = config.ts_periodicity; + memcpy(vpx_config->ts_layer_id, config.ts_layer_id, + sizeof(unsigned int) * Vp8EncoderConfig::kMaxPeriodicity); + vpx_config->rc_target_bitrate = config.rc_target_bitrate; + vpx_config->rc_min_quantizer = config.rc_min_quantizer; + vpx_config->rc_max_quantizer = config.rc_max_quantizer; + if (config.error_resilient.has_value()) { + vpx_config->g_error_resilient = config.error_resilient.value(); } } -// Adds configuration from |new_config| to |base_config|. Both configs consist -// of optionals, and only optionals which are set in |new_config| can have -// an effect. (That is, set values in |base_config| cannot be unset.) -// Returns |true| iff any changes were made to |base_config|. -bool MaybeExtendVp8EncoderConfig(const Vp8EncoderConfig& new_config, - Vp8EncoderConfig* base_config) { - bool changes_made = false; - changes_made |= MaybeSetNewValue(new_config.temporal_layer_config, - &base_config->temporal_layer_config); - changes_made |= MaybeSetNewValue(new_config.rc_target_bitrate, - &base_config->rc_target_bitrate); - changes_made |= MaybeSetNewValue(new_config.rc_max_quantizer, - &base_config->rc_max_quantizer); - changes_made |= MaybeSetNewValue(new_config.g_error_resilient, - &base_config->g_error_resilient); - return changes_made; -} - -void ApplyVp8EncoderConfigToVpxConfig(const Vp8EncoderConfig& encoder_config, - vpx_codec_enc_cfg_t* vpx_config) { - if (encoder_config.temporal_layer_config.has_value()) { - const Vp8EncoderConfig::TemporalLayerConfig& ts_config = - encoder_config.temporal_layer_config.value(); - vpx_config->ts_number_layers = ts_config.ts_number_layers; - std::copy(ts_config.ts_target_bitrate.begin(), - ts_config.ts_target_bitrate.end(), - std::begin(vpx_config->ts_target_bitrate)); - std::copy(ts_config.ts_rate_decimator.begin(), - ts_config.ts_rate_decimator.end(), - std::begin(vpx_config->ts_rate_decimator)); - vpx_config->ts_periodicity = ts_config.ts_periodicity; - std::copy(ts_config.ts_layer_id.begin(), ts_config.ts_layer_id.end(), - std::begin(vpx_config->ts_layer_id)); - } - - if (encoder_config.rc_target_bitrate.has_value()) { - vpx_config->rc_target_bitrate = encoder_config.rc_target_bitrate.value(); - } - - if (encoder_config.rc_max_quantizer.has_value()) { - vpx_config->rc_max_quantizer = encoder_config.rc_max_quantizer.value(); - } - - if (encoder_config.g_error_resilient.has_value()) { - vpx_config->g_error_resilient = encoder_config.g_error_resilient.value(); - } +bool UpdateVpxConfiguration(size_t stream_index, + Vp8FrameBufferController* frame_buffer_controller, + vpx_codec_enc_cfg_t* cfg) { + Vp8EncoderConfig config = GetEncoderConfig(cfg); + const bool res = + frame_buffer_controller->UpdateConfiguration(stream_index, &config); + if (res) + FillInEncoderConfig(cfg, config); + return res; } } // namespace @@ -299,7 +282,6 @@ LibvpxVp8Encoder::LibvpxVp8Encoder( cpu_speed_.assign(kMaxSimulcastStreams, cpu_speed_default_); encoders_.reserve(kMaxSimulcastStreams); configurations_.reserve(kMaxSimulcastStreams); - config_overrides_.reserve(kMaxSimulcastStreams); downsampling_factors_.reserve(kMaxSimulcastStreams); } @@ -322,7 +304,6 @@ int LibvpxVp8Encoder::Release() { encoders_.clear(); configurations_.clear(); - config_overrides_.clear(); send_stream_.clear(); cpu_speed_.clear(); @@ -386,9 +367,8 @@ void LibvpxVp8Encoder::SetRates(const RateControlParameters& parameters) { } } - for (size_t i = 0; i < encoders_.size(); ++i) { - const size_t stream_idx = encoders_.size() - 1 - i; - + size_t stream_idx = encoders_.size() - 1; + for (size_t i = 0; i < encoders_.size(); ++i, --stream_idx) { unsigned int target_bitrate_kbps = parameters.bitrate.GetSpatialLayerSum(stream_idx) / 1000; @@ -403,7 +383,8 @@ void LibvpxVp8Encoder::SetRates(const RateControlParameters& parameters) { static_cast(parameters.framerate_fps + 0.5)); } - UpdateVpxConfiguration(stream_idx); + UpdateVpxConfiguration(stream_idx, frame_buffer_controller_.get(), + &configurations_[i]); if (rate_control_settings_.Vp8DynamicRateSettings()) { // Tweak rate control settings based on available network headroom. @@ -505,7 +486,6 @@ int LibvpxVp8Encoder::InitEncode(const VideoCodec* inst, encoded_images_.resize(number_of_streams); encoders_.resize(number_of_streams); configurations_.resize(number_of_streams); - config_overrides_.resize(number_of_streams); downsampling_factors_.resize(number_of_streams); raw_images_.resize(number_of_streams); send_stream_.resize(number_of_streams); @@ -620,7 +600,7 @@ int LibvpxVp8Encoder::InitEncode(const VideoCodec* inst, // Note the order we use is different from webm, we have lowest resolution // at position 0 and they have highest resolution at position 0. - const size_t stream_idx_cfg_0 = encoders_.size() - 1; + int stream_idx = encoders_.size() - 1; SimulcastRateAllocator init_allocator(codec_); VideoBitrateAllocation allocation = init_allocator.GetAllocation( inst->startBitrate * 1000, inst->maxFramerate); @@ -630,21 +610,18 @@ int LibvpxVp8Encoder::InitEncode(const VideoCodec* inst, stream_bitrates.push_back(bitrate); } - configurations_[0].rc_target_bitrate = stream_bitrates[stream_idx_cfg_0]; - if (stream_bitrates[stream_idx_cfg_0] > 0) { + configurations_[0].rc_target_bitrate = stream_bitrates[stream_idx]; + if (stream_bitrates[stream_idx] > 0) { frame_buffer_controller_->OnRatesUpdated( - stream_idx_cfg_0, - allocation.GetTemporalLayerAllocation(stream_idx_cfg_0), + stream_idx, allocation.GetTemporalLayerAllocation(stream_idx), inst->maxFramerate); } - frame_buffer_controller_->SetQpLimits(stream_idx_cfg_0, - configurations_[0].rc_min_quantizer, - configurations_[0].rc_max_quantizer); - UpdateVpxConfiguration(stream_idx_cfg_0); - configurations_[0].rc_dropframe_thresh = FrameDropThreshold(stream_idx_cfg_0); + UpdateVpxConfiguration(stream_idx, frame_buffer_controller_.get(), + &configurations_[0]); + configurations_[0].rc_dropframe_thresh = FrameDropThreshold(stream_idx); - for (size_t i = 1; i < encoders_.size(); ++i) { - const size_t stream_idx = encoders_.size() - 1 - i; + --stream_idx; + for (size_t i = 1; i < encoders_.size(); ++i, --stream_idx) { memcpy(&configurations_[i], &configurations_[0], sizeof(configurations_[0])); @@ -670,10 +647,8 @@ int LibvpxVp8Encoder::InitEncode(const VideoCodec* inst, stream_idx, allocation.GetTemporalLayerAllocation(stream_idx), inst->maxFramerate); } - frame_buffer_controller_->SetQpLimits(stream_idx, - configurations_[i].rc_min_quantizer, - configurations_[i].rc_max_quantizer); - UpdateVpxConfiguration(stream_idx); + UpdateVpxConfiguration(stream_idx, frame_buffer_controller_.get(), + &configurations_[i]); } return InitAndSetControlSettings(); @@ -881,27 +856,6 @@ size_t LibvpxVp8Encoder::SteadyStateSize(int sid, int tid) { 0.5); } -bool LibvpxVp8Encoder::UpdateVpxConfiguration(size_t stream_index) { - RTC_DCHECK(frame_buffer_controller_); - - const size_t config_index = configurations_.size() - 1 - stream_index; - - RTC_DCHECK_LT(config_index, config_overrides_.size()); - Vp8EncoderConfig* config = &config_overrides_[config_index]; - - const Vp8EncoderConfig new_config = - frame_buffer_controller_->UpdateConfiguration(stream_index); - - const bool changes_made = MaybeExtendVp8EncoderConfig(new_config, config); - - // Note that overrides must be applied even if they haven't changed. - RTC_DCHECK_LT(config_index, configurations_.size()); - vpx_codec_enc_cfg_t* vpx_config = &configurations_[config_index]; - ApplyVp8EncoderConfigToVpxConfig(*config, vpx_config); - - return changes_made; -} - int LibvpxVp8Encoder::Encode(const VideoFrame& frame, const std::vector* frame_types) { RTC_DCHECK_EQ(frame.width(), codec_.width); @@ -1023,11 +977,16 @@ int LibvpxVp8Encoder::Encode(const VideoFrame& frame, // Note that streams are defined starting from lowest resolution at // position 0 to highest resolution at position |encoders_.size() - 1|, // whereas |encoder_| is from highest to lowest resolution. - for (size_t i = 0; i < encoders_.size(); ++i) { - const size_t stream_idx = encoders_.size() - 1 - i; - - if (UpdateVpxConfiguration(stream_idx)) { - if (libvpx_->codec_enc_config_set(&encoders_[i], &configurations_[i])) + size_t stream_idx = encoders_.size() - 1; + for (size_t i = 0; i < encoders_.size(); ++i, --stream_idx) { + // Allow the layers adapter to temporarily modify the configuration. This + // change isn't stored in configurations_ so change will be discarded at + // the next update. + vpx_codec_enc_cfg_t temp_config; + memcpy(&temp_config, &configurations_[i], sizeof(vpx_codec_enc_cfg_t)); + if (UpdateVpxConfiguration(stream_idx, frame_buffer_controller_.get(), + &temp_config)) { + if (libvpx_->codec_enc_config_set(&encoders_[i], &temp_config)) return WEBRTC_VIDEO_CODEC_ERROR; } diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h index 1e17096a87..0913f5bf48 100644 --- a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h +++ b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.h @@ -93,8 +93,6 @@ class LibvpxVp8Encoder : public VideoEncoder { size_t SteadyStateSize(int sid, int tid); - bool UpdateVpxConfiguration(size_t stream_index); - const std::unique_ptr libvpx_; const absl::optional> @@ -119,7 +117,6 @@ class LibvpxVp8Encoder : public VideoEncoder { std::vector encoded_images_; std::vector encoders_; std::vector configurations_; - std::vector config_overrides_; std::vector downsampling_factors_; // Variable frame-rate screencast related fields and methods. diff --git a/modules/video_coding/codecs/vp8/screenshare_layers.cc b/modules/video_coding/codecs/vp8/screenshare_layers.cc index c8ea6ca1c4..2f4b482033 100644 --- a/modules/video_coding/codecs/vp8/screenshare_layers.cc +++ b/modules/video_coding/codecs/vp8/screenshare_layers.cc @@ -55,6 +55,8 @@ ScreenshareLayers::ScreenshareLayers(int num_temporal_layers) last_sync_timestamp_(-1), last_emitted_tl0_timestamp_(-1), last_frame_time_ms_(-1), + min_qp_(-1), + max_qp_(-1), max_debt_bytes_(0), encode_framerate_(1000.0f, 1000.0f), // 1 second window, second scale. bitrate_updated_(false), @@ -69,24 +71,6 @@ ScreenshareLayers::~ScreenshareLayers() { UpdateHistograms(); } -void ScreenshareLayers::SetQpLimits(size_t stream_index, - int min_qp, - int max_qp) { - RTC_DCHECK_LT(stream_index, StreamCount()); - // 0 < min_qp <= max_qp - RTC_DCHECK_LT(0, min_qp); - RTC_DCHECK_LE(min_qp, max_qp); - - RTC_DCHECK_EQ(min_qp_.has_value(), max_qp_.has_value()); - if (!min_qp_.has_value()) { - min_qp_ = min_qp; - max_qp_ = max_qp; - } else { - RTC_DCHECK_EQ(min_qp, min_qp_.value()); - RTC_DCHECK_EQ(max_qp, max_qp_.value()); - } -} - size_t ScreenshareLayers::StreamCount() const { return 1; } @@ -494,12 +478,19 @@ uint32_t ScreenshareLayers::GetCodecTargetBitrateKbps() const { return std::max(layers_[0].target_rate_kbps_, target_bitrate_kbps); } -Vp8EncoderConfig ScreenshareLayers::UpdateConfiguration(size_t stream_index) { +bool ScreenshareLayers::UpdateConfiguration(size_t stream_index, + Vp8EncoderConfig* cfg) { RTC_DCHECK_LT(stream_index, StreamCount()); - RTC_DCHECK(min_qp_.has_value()); - RTC_DCHECK(max_qp_.has_value()); - const uint32_t target_bitrate_kbps = GetCodecTargetBitrateKbps(); + if (min_qp_ == -1 || max_qp_ == -1) { + // Store the valid qp range. This must not change during the lifetime of + // this class. + min_qp_ = cfg->rc_min_quantizer; + max_qp_ = cfg->rc_max_quantizer; + } + + bool cfg_updated = false; + uint32_t target_bitrate_kbps = GetCodecTargetBitrateKbps(); // TODO(sprang): We _really_ need to make an overhaul of this class. :( // If we're dropping frames in order to meet a target framerate, adjust the @@ -512,16 +503,12 @@ Vp8EncoderConfig ScreenshareLayers::UpdateConfiguration(size_t stream_index) { } if (bitrate_updated_ || - encoder_config_.rc_target_bitrate != - absl::make_optional(encoder_config_bitrate_kbps)) { - encoder_config_.rc_target_bitrate = encoder_config_bitrate_kbps; + cfg->rc_target_bitrate != encoder_config_bitrate_kbps) { + cfg->rc_target_bitrate = encoder_config_bitrate_kbps; // Don't reconfigure qp limits during quality boost frames. if (active_layer_ == -1 || layers_[active_layer_].state != TemporalLayer::State::kQualityBoost) { - const int min_qp = min_qp_.value(); - const int max_qp = max_qp_.value(); - // After a dropped frame, a frame with max qp will be encoded and the // quality will then ramp up from there. To boost the speed of recovery, // encode the next frame with lower max qp, if there is sufficient @@ -530,8 +517,10 @@ Vp8EncoderConfig ScreenshareLayers::UpdateConfiguration(size_t stream_index) { // will propagate to TL1. // Currently, reduce max qp by 20% for TL0 and 15% for TL1. if (layers_[1].target_rate_kbps_ >= kMinBitrateKbpsForQpBoost) { - layers_[0].enhanced_max_qp = min_qp + (((max_qp - min_qp) * 80) / 100); - layers_[1].enhanced_max_qp = min_qp + (((max_qp - min_qp) * 85) / 100); + layers_[0].enhanced_max_qp = + min_qp_ + (((max_qp_ - min_qp_) * 80) / 100); + layers_[1].enhanced_max_qp = + min_qp_ + (((max_qp_ - min_qp_) * 85) / 100); } else { layers_[0].enhanced_max_qp = -1; layers_[1].enhanced_max_qp = -1; @@ -549,19 +538,20 @@ Vp8EncoderConfig ScreenshareLayers::UpdateConfiguration(size_t stream_index) { } bitrate_updated_ = false; + cfg_updated = true; } // Don't try to update boosts state if not active yet. if (active_layer_ == -1) - return encoder_config_; + return cfg_updated; - if (number_of_temporal_layers_ <= 1) - return encoder_config_; + if (max_qp_ == -1 || number_of_temporal_layers_ <= 1) + return cfg_updated; // If layer is in the quality boost state (following a dropped frame), update // the configuration with the adjusted (lower) qp and set the state back to // normal. - unsigned int adjusted_max_qp = max_qp_.value(); // Set the normal max qp. + unsigned int adjusted_max_qp = max_qp_; // Set the normal max qp. if (layers_[active_layer_].state == TemporalLayer::State::kQualityBoost) { if (layers_[active_layer_].enhanced_max_qp != -1) { // Bitrate is high enough for quality boost, update max qp. @@ -570,9 +560,14 @@ Vp8EncoderConfig ScreenshareLayers::UpdateConfiguration(size_t stream_index) { // Regardless of qp, reset the boost state for the next frame. layers_[active_layer_].state = TemporalLayer::State::kNormal; } - encoder_config_.rc_max_quantizer = adjusted_max_qp; - return encoder_config_; + if (adjusted_max_qp == cfg->rc_max_quantizer) + return cfg_updated; + + cfg->rc_max_quantizer = adjusted_max_qp; + cfg_updated = true; + + return cfg_updated; } void ScreenshareLayers::TemporalLayer::UpdateDebt(int64_t delta_ms) { diff --git a/modules/video_coding/codecs/vp8/screenshare_layers.h b/modules/video_coding/codecs/vp8/screenshare_layers.h index d7de52711c..06d45534fe 100644 --- a/modules/video_coding/codecs/vp8/screenshare_layers.h +++ b/modules/video_coding/codecs/vp8/screenshare_layers.h @@ -36,8 +36,6 @@ class ScreenshareLayers final : public Vp8FrameBufferController { explicit ScreenshareLayers(int num_temporal_layers); ~ScreenshareLayers() override; - void SetQpLimits(size_t stream_index, int min_qp, int max_qp) override; - size_t StreamCount() const override; bool SupportsEncoderFrameDropping(size_t stream_index) const override; @@ -52,7 +50,9 @@ class ScreenshareLayers final : public Vp8FrameBufferController { const std::vector& bitrates_bps, int framerate_fps) override; - Vp8EncoderConfig UpdateConfiguration(size_t stream_index) override; + // Update the encoder configuration with target bitrates or other parameters. + // Returns true iff the configuration was actually modified. + bool UpdateConfiguration(size_t stream_index, Vp8EncoderConfig* cfg) override; void OnEncodeDone(size_t stream_index, uint32_t rtp_timestamp, @@ -89,18 +89,15 @@ class ScreenshareLayers final : public Vp8FrameBufferController { bool TimeToSync(int64_t timestamp) const; uint32_t GetCodecTargetBitrateKbps() const; - const int number_of_temporal_layers_; - - // TODO(eladalon/sprang): These should be made into const-int set in the ctor. - absl::optional min_qp_; - absl::optional max_qp_; - + int number_of_temporal_layers_; int active_layer_; int64_t last_timestamp_; int64_t last_sync_timestamp_; int64_t last_emitted_tl0_timestamp_; int64_t last_frame_time_ms_; rtc::TimestampWrapAroundHandler time_wrap_handler_; + int min_qp_; + int max_qp_; uint32_t max_debt_bytes_; std::map pending_frame_configs_; @@ -155,8 +152,6 @@ class ScreenshareLayers final : public Vp8FrameBufferController { int64_t tl1_target_bitrate_sum_ = 0; } stats_; - Vp8EncoderConfig encoder_config_; - // Optional utility used to verify reference validity. std::unique_ptr checker_; }; diff --git a/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc b/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc index 1f85cb6ec2..032c5280bd 100644 --- a/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc +++ b/modules/video_coding/codecs/vp8/screenshare_layers_unittest.cc @@ -30,6 +30,7 @@ using ::testing::_; using ::testing::ElementsAre; using ::testing::NiceMock; +using ::testing::Return; namespace webrtc { namespace { @@ -92,19 +93,7 @@ class ScreenshareLayerTest : public ::testing::Test { if (tl_config_.drop_frame) { return -1; } - const uint32_t prev_rc_target_bitrate = cfg_.rc_target_bitrate.value_or(-1); - const uint32_t prev_rc_max_quantizer = cfg_.rc_max_quantizer.value_or(-1); - - cfg_ = layers_->UpdateConfiguration(0); - - config_updated_ = - cfg_.temporal_layer_config.has_value() || - (cfg_.rc_target_bitrate.has_value() && - cfg_.rc_target_bitrate.value() != prev_rc_target_bitrate) || - (cfg_.rc_max_quantizer.has_value() && - cfg_.rc_max_quantizer.value() != prev_rc_max_quantizer) || - cfg_.g_error_resilient.has_value(); - + config_updated_ = layers_->UpdateConfiguration(0, &cfg_); int flags = LibvpxVp8Encoder::EncodeFlags(tl_config_); EXPECT_NE(-1, frame_size_); return flags; @@ -121,11 +110,13 @@ class ScreenshareLayerTest : public ::testing::Test { } Vp8EncoderConfig ConfigureBitrates() { - layers_->SetQpLimits(0, min_qp_, max_qp_); + Vp8EncoderConfig vp8_cfg; + memset(&vp8_cfg, 0, sizeof(Vp8EncoderConfig)); + vp8_cfg.rc_min_quantizer = min_qp_; + vp8_cfg.rc_max_quantizer = max_qp_; layers_->OnRatesUpdated(0, kDefault2TlBitratesBps, kFrameRate); - const Vp8EncoderConfig vp8_cfg = layers_->UpdateConfiguration(0); - EXPECT_TRUE(vp8_cfg.rc_target_bitrate.has_value()); - frame_size_ = FrameSizeForBitrate(vp8_cfg.rc_target_bitrate.value()); + EXPECT_TRUE(layers_->UpdateConfiguration(0, &vp8_cfg)); + frame_size_ = FrameSizeForBitrate(vp8_cfg.rc_target_bitrate); return vp8_cfg; } @@ -255,7 +246,7 @@ TEST_F(ScreenshareLayerTest, 2LayersSyncAfterTimeout) { CodecSpecificInfo info; tl_config_ = NextFrameConfig(0, timestamp_); - cfg_ = layers_->UpdateConfiguration(0); + config_updated_ = layers_->UpdateConfiguration(0, &cfg_); // Simulate TL1 being at least 8 qp steps better. if (tl_config_.packetizer_temporal_idx == 0) { @@ -403,7 +394,7 @@ TEST_F(ScreenshareLayerTest, TargetBitrateCappedByTL0) { const std::vector layer_rates = {kTl0_kbps * 1000, (kTl1_kbps - kTl0_kbps) * 1000}; layers_->OnRatesUpdated(0, layer_rates, kFrameRate); - cfg_ = layers_->UpdateConfiguration(0); + EXPECT_TRUE(layers_->UpdateConfiguration(0, &cfg_)); EXPECT_EQ(static_cast( ScreenshareLayers::kMaxTL0FpsReduction * kTl0_kbps + 0.5), @@ -416,7 +407,7 @@ TEST_F(ScreenshareLayerTest, TargetBitrateCappedByTL1) { const std::vector layer_rates = {kTl0_kbps * 1000, (kTl1_kbps - kTl0_kbps) * 1000}; layers_->OnRatesUpdated(0, layer_rates, kFrameRate); - cfg_ = layers_->UpdateConfiguration(0); + EXPECT_TRUE(layers_->UpdateConfiguration(0, &cfg_)); EXPECT_EQ(static_cast( kTl1_kbps / ScreenshareLayers::kAcceptableTargetOvershoot), @@ -427,7 +418,7 @@ TEST_F(ScreenshareLayerTest, TargetBitrateBelowTL0) { const int kTl0_kbps = 100; const std::vector layer_rates = {kTl0_kbps * 1000}; layers_->OnRatesUpdated(0, layer_rates, kFrameRate); - cfg_ = layers_->UpdateConfiguration(0); + EXPECT_TRUE(layers_->UpdateConfiguration(0, &cfg_)); EXPECT_EQ(static_cast(kTl0_kbps), cfg_.rc_target_bitrate); } @@ -493,7 +484,7 @@ TEST_F(ScreenshareLayerTest, RespectsMaxIntervalBetweenFrames) { const std::vector layer_rates = {kLowBitrateKbps * 1000}; layers_->OnRatesUpdated(0, layer_rates, kFrameRate); - cfg_ = layers_->UpdateConfiguration(0); + layers_->UpdateConfiguration(0, &cfg_); EXPECT_EQ(kTl0Flags, LibvpxVp8Encoder::EncodeFlags(NextFrameConfig(0, kStartTimestamp))); @@ -536,7 +527,7 @@ TEST_F(ScreenshareLayerTest, UpdatesHistograms) { } int flags = LibvpxVp8Encoder::EncodeFlags(tl_config_); if (flags != -1) - cfg_ = layers_->UpdateConfiguration(0); + layers_->UpdateConfiguration(0, &cfg_); if (timestamp >= kTimestampDelta5Fps * 5 && !overshoot && flags != -1) { // Simulate one overshoot. @@ -601,6 +592,13 @@ TEST_F(ScreenshareLayerTest, UpdatesHistograms) { kDefaultTl1BitrateKbps)); } +TEST_F(ScreenshareLayerTest, AllowsUpdateConfigBeforeSetRates) { + layers_.reset(new ScreenshareLayers(2)); + // New layer instance, OnRatesUpdated() never called. + // UpdateConfiguration() call should not cause crash. + layers_->UpdateConfiguration(0, &cfg_); +} + TEST_F(ScreenshareLayerTest, RespectsConfiguredFramerate) { int64_t kTestSpanMs = 2000; int64_t kFrameIntervalsMs = 1000 / kFrameRate; @@ -656,7 +654,7 @@ TEST_F(ScreenshareLayerTest, 2LayersSyncAtOvershootDrop) { // Simulate overshoot of this frame. layers_->OnEncodeDone(0, timestamp_, 0, false, 0, nullptr); - cfg_ = layers_->UpdateConfiguration(0); + config_updated_ = layers_->UpdateConfiguration(0, &cfg_); EXPECT_EQ(kTl1SyncFlags, LibvpxVp8Encoder::EncodeFlags(tl_config_)); CodecSpecificInfo new_info; @@ -689,8 +687,7 @@ TEST_F(ScreenshareLayerTest, DropOnTooShortFrameInterval) { TEST_F(ScreenshareLayerTest, AdjustsBitrateWhenDroppingFrames) { const uint32_t kTimestampDelta10Fps = kTimestampDelta5Fps / 2; const int kNumFrames = 30; - ASSERT_TRUE(cfg_.rc_target_bitrate.has_value()); - const uint32_t default_bitrate = cfg_.rc_target_bitrate.value(); + uint32_t default_bitrate = cfg_.rc_target_bitrate; layers_->OnRatesUpdated(0, kDefault2TlBitratesBps, 10); int num_dropped_frames = 0; @@ -699,7 +696,7 @@ TEST_F(ScreenshareLayerTest, AdjustsBitrateWhenDroppingFrames) { ++num_dropped_frames; timestamp_ += kTimestampDelta10Fps; } - cfg_ = layers_->UpdateConfiguration(0); + layers_->UpdateConfiguration(0, &cfg_); EXPECT_EQ(num_dropped_frames, kNumFrames / 2); EXPECT_EQ(cfg_.rc_target_bitrate, default_bitrate * 2); @@ -708,20 +705,20 @@ TEST_F(ScreenshareLayerTest, AdjustsBitrateWhenDroppingFrames) { TEST_F(ScreenshareLayerTest, UpdatesConfigurationAfterRateChange) { // Set inital rate again, no need to update configuration. layers_->OnRatesUpdated(0, kDefault2TlBitratesBps, kFrameRate); - cfg_ = layers_->UpdateConfiguration(0); + EXPECT_FALSE(layers_->UpdateConfiguration(0, &cfg_)); // Rate changed, now update config. std::vector bitrates = kDefault2TlBitratesBps; bitrates[1] -= 100000; layers_->OnRatesUpdated(0, bitrates, 5); - cfg_ = layers_->UpdateConfiguration(0); + EXPECT_TRUE(layers_->UpdateConfiguration(0, &cfg_)); // Changed rate, but then set changed rate again before trying to update // configuration, update should still apply. bitrates[1] -= 100000; layers_->OnRatesUpdated(0, bitrates, 5); layers_->OnRatesUpdated(0, bitrates, 5); - cfg_ = layers_->UpdateConfiguration(0); + EXPECT_TRUE(layers_->UpdateConfiguration(0, &cfg_)); } TEST_F(ScreenshareLayerTest, MaxQpRestoredAfterDoubleDrop) { @@ -747,8 +744,7 @@ TEST_F(ScreenshareLayerTest, MaxQpRestoredAfterDoubleDrop) { EXPECT_EQ(kTl1Flags, SkipUntilTlAndSync(1, false)); EXPECT_TRUE(config_updated_); EXPECT_LT(cfg_.rc_max_quantizer, max_qp_); - ASSERT_TRUE(cfg_.rc_max_quantizer.has_value()); - const uint32_t adjusted_qp = cfg_.rc_max_quantizer.value(); + uint32_t adjusted_qp = cfg_.rc_max_quantizer; // Simulate overshoot of this frame. layers_->OnEncodeDone(0, timestamp_, 0, false, -1, nullptr); diff --git a/modules/video_coding/utility/simulcast_rate_allocator_unittest.cc b/modules/video_coding/utility/simulcast_rate_allocator_unittest.cc index 471fcd0d60..eefb743d63 100644 --- a/modules/video_coding/utility/simulcast_rate_allocator_unittest.cc +++ b/modules/video_coding/utility/simulcast_rate_allocator_unittest.cc @@ -15,7 +15,6 @@ #include #include -#include "api/video_codecs/vp8_frame_buffer_controller.h" #include "api/video_codecs/vp8_frame_config.h" #include "api/video_codecs/vp8_temporal_layers.h" #include "rtc_base/checks.h" @@ -40,7 +39,7 @@ class MockTemporalLayers : public Vp8FrameBufferController { public: MOCK_METHOD2(NextFrameConfig, Vp8FrameConfig(size_t, uint32_t)); MOCK_METHOD3(OnRatesUpdated, void(size_t, const std::vector&, int)); - MOCK_METHOD1(UpdateConfiguration, Vp8EncoderConfig(size_t)); + MOCK_METHOD2(UpdateConfiguration, bool(size_t, Vp8EncoderConfig*)); MOCK_METHOD6(OnEncodeDone, void(size_t, uint32_t, size_t, bool, int, CodecSpecificInfo*)); MOCK_METHOD4(FrameEncoded, void(size_t, uint32_t, size_t, int)); diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index d935ea0c73..491fee1493 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -711,7 +711,9 @@ void VideoStreamEncoder::ReconfigureEncoder() { } // Reset (release existing encoder) if one exists and anything except - // start bitrate or max framerate has changed. + // start bitrate or max framerate has changed. Don't call Release() if + // |pending_encoder_creation_| as that means this is a new encoder + // that has not yet been initialized. const bool reset_required = RequiresEncoderReset(codec, send_codec_); send_codec_ = codec;