From 32ca95145c4636374266f5b5d4d1ac43658bc758 Mon Sep 17 00:00:00 2001 From: Florent Castelli Date: Fri, 24 Jul 2020 17:29:40 +0200 Subject: [PATCH] Only enable conference mode simulcast allocations with flag enabled Non-conference mode simulcast screenshares were mistakenly using the conference mode semantics in the simulcast rate allocator, which broke spec compliant usage in some situation. This behavior should only be used when explicitly using the SDP entry "a=x-google-flag:conference" in both offer and answer. Bug: webrtc:11310, chromium:1093819 Change-Id: Ibcba75c88a8405d60467546b33977a782e04e469 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/179081 Reviewed-by: Harald Alvestrand Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Florent Castelli Cr-Commit-Position: refs/heads/master@{#31828} --- api/video/video_bitrate_allocator.cc | 2 ++ api/video/video_bitrate_allocator.h | 4 ++++ api/video_codecs/video_codec.cc | 1 + api/video_codecs/video_codec.h | 3 +++ api/video_codecs/video_encoder_config.cc | 3 ++- api/video_codecs/video_encoder_config.h | 3 +++ media/engine/simulcast_encoder_adapter.cc | 4 ++++ media/engine/webrtc_video_engine.cc | 6 +++-- media/engine/webrtc_video_engine.h | 2 +- .../codecs/vp8/libvpx_vp8_encoder.cc | 2 +- .../codecs/vp8/test/vp8_impl_unittest.cc | 2 ++ .../utility/simulcast_rate_allocator.cc | 16 +++++++------- .../utility/simulcast_rate_allocator.h | 3 +++ .../simulcast_rate_allocator_unittest.cc | 15 +++++++------ .../video_coding/utility/simulcast_utility.cc | 11 +--------- .../video_coding/video_codec_initializer.cc | 2 ++ .../video_codec_initializer_unittest.cc | 22 +++++++++++++++++-- video/video_stream_encoder.cc | 2 ++ 18 files changed, 71 insertions(+), 32 deletions(-) diff --git a/api/video/video_bitrate_allocator.cc b/api/video/video_bitrate_allocator.cc index 8ad5f75244..f4e843b348 100644 --- a/api/video/video_bitrate_allocator.cc +++ b/api/video/video_bitrate_allocator.cc @@ -49,4 +49,6 @@ VideoBitrateAllocation VideoBitrateAllocator::Allocate( return GetAllocation(parameters.total_bitrate.bps(), parameters.framerate); } +void VideoBitrateAllocator::SetLegacyConferenceMode(bool enabled) {} + } // namespace webrtc diff --git a/api/video/video_bitrate_allocator.h b/api/video/video_bitrate_allocator.h index 04de04c1b0..fdc86dbc57 100644 --- a/api/video/video_bitrate_allocator.h +++ b/api/video/video_bitrate_allocator.h @@ -40,6 +40,10 @@ class VideoBitrateAllocator { virtual VideoBitrateAllocation Allocate( VideoBitrateAllocationParameters parameters); + + // Deprecated: Only used to work around issues with the legacy conference + // screenshare mode and shouldn't be needed by any subclasses. + virtual void SetLegacyConferenceMode(bool enabled); }; class VideoBitrateAllocationObserver { diff --git a/api/video_codecs/video_codec.cc b/api/video_codecs/video_codec.cc index d03082b91e..65c7243c00 100644 --- a/api/video_codecs/video_codec.cc +++ b/api/video_codecs/video_codec.cc @@ -83,6 +83,7 @@ VideoCodec::VideoCodec() mode(VideoCodecMode::kRealtimeVideo), expect_encode_from_texture(false), timing_frame_thresholds({0, 0}), + legacy_conference_mode(false), codec_specific_() {} VideoCodecVP8* VideoCodec::VP8() { diff --git a/api/video_codecs/video_codec.h b/api/video_codecs/video_codec.h index c07fae9b8b..dd3d344084 100644 --- a/api/video_codecs/video_codec.h +++ b/api/video_codecs/video_codec.h @@ -146,6 +146,9 @@ class RTC_EXPORT VideoCodec { uint16_t outlier_ratio_percent; } timing_frame_thresholds; + // Legacy Google conference mode flag for simulcast screenshare + bool legacy_conference_mode; + bool operator==(const VideoCodec& other) const = delete; bool operator!=(const VideoCodec& other) const = delete; diff --git a/api/video_codecs/video_encoder_config.cc b/api/video_codecs/video_encoder_config.cc index 6efcbf2bdd..19b0135641 100644 --- a/api/video_codecs/video_encoder_config.cc +++ b/api/video_codecs/video_encoder_config.cc @@ -55,7 +55,8 @@ VideoEncoderConfig::VideoEncoderConfig() min_transmit_bitrate_bps(0), max_bitrate_bps(0), bitrate_priority(1.0), - number_of_streams(0) {} + number_of_streams(0), + legacy_conference_mode(false) {} VideoEncoderConfig::VideoEncoderConfig(VideoEncoderConfig&&) = default; diff --git a/api/video_codecs/video_encoder_config.h b/api/video_codecs/video_encoder_config.h index ef8db100a3..7a59dacd1b 100644 --- a/api/video_codecs/video_encoder_config.h +++ b/api/video_codecs/video_encoder_config.h @@ -176,6 +176,9 @@ class VideoEncoderConfig { // Max number of encoded VideoStreams to produce. size_t number_of_streams; + // Legacy Google conference mode flag for simulcast screenshare + bool legacy_conference_mode; + private: // Access to the copy constructor is private to force use of the Copy() // method for those exceptional cases where we do use it. diff --git a/media/engine/simulcast_encoder_adapter.cc b/media/engine/simulcast_encoder_adapter.cc index 863ccc756e..60baed9daa 100644 --- a/media/engine/simulcast_encoder_adapter.cc +++ b/media/engine/simulcast_encoder_adapter.cc @@ -619,6 +619,10 @@ void SimulcastEncoderAdapter::PopulateStreamCodec( // TODO(ronghuawu): what to do with targetBitrate. stream_codec->startBitrate = start_bitrate_kbps; + + // Legacy screenshare mode is only enabled for the first simulcast layer + stream_codec->legacy_conference_mode = + inst.legacy_conference_mode && stream_index == 0; } bool SimulcastEncoderAdapter::Initialized() const { diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 26fa335cf6..a075f7b58a 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -2399,6 +2399,8 @@ WebRtcVideoChannel::WebRtcVideoSendStream::CreateVideoEncoderConfig( } } + encoder_config.legacy_conference_mode = parameters_.conference_mode; + int max_qp = kDefaultQpMax; codec.GetParam(kCodecParamMaxQuantization, &max_qp); encoder_config.video_stream_factory = @@ -3394,7 +3396,7 @@ std::vector EncoderStreamFactory::CreateEncoderStreams( ((absl::EqualsIgnoreCase(codec_name_, kVp8CodecName) || absl::EqualsIgnoreCase(codec_name_, kH264CodecName)) && is_screenshare_ && conference_mode_)) { - return CreateSimulcastOrConfereceModeScreenshareStreams( + return CreateSimulcastOrConferenceModeScreenshareStreams( width, height, encoder_config, experimental_min_bitrate); } @@ -3483,7 +3485,7 @@ EncoderStreamFactory::CreateDefaultVideoStreams( } std::vector -EncoderStreamFactory::CreateSimulcastOrConfereceModeScreenshareStreams( +EncoderStreamFactory::CreateSimulcastOrConferenceModeScreenshareStreams( int width, int height, const webrtc::VideoEncoderConfig& encoder_config, diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index 21033ffae7..9737dd8294 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -661,7 +661,7 @@ class EncoderStreamFactory const absl::optional& experimental_min_bitrate) const; std::vector - CreateSimulcastOrConfereceModeScreenshareStreams( + CreateSimulcastOrConferenceModeScreenshareStreams( int width, int height, const webrtc::VideoEncoderConfig& encoder_config, diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc index ac04bc3e50..9b64d1654f 100644 --- a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc +++ b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc @@ -879,7 +879,7 @@ size_t LibvpxVp8Encoder::SteadyStateSize(int sid, int tid) { const int encoder_id = encoders_.size() - 1 - sid; size_t bitrate_bps; float fps; - if (SimulcastUtility::IsConferenceModeScreenshare(codec_) || + if ((SimulcastUtility::IsConferenceModeScreenshare(codec_) && sid == 0) || vpx_configs_[encoder_id].ts_number_layers <= 1) { // In conference screenshare there's no defined per temporal layer bitrate // and framerate. diff --git a/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc b/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc index 51595260dd..d3e4a1739e 100644 --- a/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc +++ b/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc @@ -523,6 +523,7 @@ TEST_F(TestVp8Impl, KeepsTimestampOnReencode) { codec_settings_.maxBitrate = 1000; codec_settings_.mode = VideoCodecMode::kScreensharing; codec_settings_.VP8()->numberOfTemporalLayers = 2; + codec_settings_.legacy_conference_mode = true; EXPECT_CALL(*vpx, img_wrap(_, _, _, _, _, _)) .WillOnce(Invoke([](vpx_image_t* img, vpx_img_fmt_t fmt, unsigned int d_w, @@ -653,6 +654,7 @@ TEST_F(TestVp8Impl, GetEncoderInfoFpsAllocationScreenshareLayers) { codec_settings_.simulcastStream[0].maxBitrate = kLegacyScreenshareTl1BitrateKbps; codec_settings_.simulcastStream[0].numberOfTemporalLayers = 2; + codec_settings_.legacy_conference_mode = true; EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK, encoder_->InitEncode(&codec_settings_, kSettings)); diff --git a/modules/video_coding/utility/simulcast_rate_allocator.cc b/modules/video_coding/utility/simulcast_rate_allocator.cc index fef74cdb45..13de8755d0 100644 --- a/modules/video_coding/utility/simulcast_rate_allocator.cc +++ b/modules/video_coding/utility/simulcast_rate_allocator.cc @@ -61,7 +61,8 @@ float SimulcastRateAllocator::GetTemporalRateAllocation( SimulcastRateAllocator::SimulcastRateAllocator(const VideoCodec& codec) : codec_(codec), stable_rate_settings_(StableTargetRateExperiment::ParseFromFieldTrials()), - rate_control_settings_(RateControlSettings::ParseFromFieldTrials()) {} + rate_control_settings_(RateControlSettings::ParseFromFieldTrials()), + legacy_conference_mode_(false) {} SimulcastRateAllocator::~SimulcastRateAllocator() = default; @@ -228,12 +229,7 @@ void SimulcastRateAllocator::DistributeAllocationToTemporalLayers( uint32_t max_bitrate_kbps; // Legacy temporal-layered only screenshare, or simulcast screenshare // with legacy mode for simulcast stream 0. - const bool conference_screenshare_mode = - codec_.mode == VideoCodecMode::kScreensharing && - ((num_spatial_streams == 1 && num_temporal_streams == 2) || // Legacy. - (num_spatial_streams > 1 && simulcast_id == 0 && - num_temporal_streams == 2)); // Simulcast. - if (conference_screenshare_mode) { + if (legacy_conference_mode_ && simulcast_id == 0) { // TODO(holmer): This is a "temporary" hack for screensharing, where we // interpret the startBitrate as the encoder target bitrate. This is // to allow for a different max bitrate, so if the codec can't meet @@ -253,7 +249,7 @@ void SimulcastRateAllocator::DistributeAllocationToTemporalLayers( if (num_temporal_streams == 1) { tl_allocation.push_back(target_bitrate_kbps); } else { - if (conference_screenshare_mode) { + if (legacy_conference_mode_ && simulcast_id == 0) { tl_allocation = ScreenshareTemporalLayerAllocation( target_bitrate_kbps, max_bitrate_kbps, simulcast_id); } else { @@ -338,4 +334,8 @@ int SimulcastRateAllocator::NumTemporalStreams(size_t simulcast_id) const { : codec_.simulcastStream[simulcast_id].numberOfTemporalLayers); } +void SimulcastRateAllocator::SetLegacyConferenceMode(bool enabled) { + legacy_conference_mode_ = enabled; +} + } // namespace webrtc diff --git a/modules/video_coding/utility/simulcast_rate_allocator.h b/modules/video_coding/utility/simulcast_rate_allocator.h index d9d9627352..9b2f9696e6 100644 --- a/modules/video_coding/utility/simulcast_rate_allocator.h +++ b/modules/video_coding/utility/simulcast_rate_allocator.h @@ -38,6 +38,8 @@ class SimulcastRateAllocator : public VideoBitrateAllocator { int temporal_id, bool base_heavy_tl3_alloc); + void SetLegacyConferenceMode(bool mode) override; + private: void DistributeAllocationToSimulcastLayers( DataRate total_bitrate, @@ -58,6 +60,7 @@ class SimulcastRateAllocator : public VideoBitrateAllocator { const StableTargetRateExperiment stable_rate_settings_; const RateControlSettings rate_control_settings_; std::vector stream_enabled_; + bool legacy_conference_mode_; RTC_DISALLOW_COPY_AND_ASSIGN(SimulcastRateAllocator); }; diff --git a/modules/video_coding/utility/simulcast_rate_allocator_unittest.cc b/modules/video_coding/utility/simulcast_rate_allocator_unittest.cc index 871e5a1692..69c14ff78e 100644 --- a/modules/video_coding/utility/simulcast_rate_allocator_unittest.cc +++ b/modules/video_coding/utility/simulcast_rate_allocator_unittest.cc @@ -88,8 +88,9 @@ class SimulcastRateAllocatorTest : public ::testing::TestWithParam { EXPECT_EQ(sum, actual.get_sum_bps()); } - void CreateAllocator() { + void CreateAllocator(bool legacy_conference_mode = false) { allocator_.reset(new SimulcastRateAllocator(codec_)); + allocator_->SetLegacyConferenceMode(legacy_conference_mode); } void SetupCodec3SL3TL(const std::vector& active_streams) { @@ -669,9 +670,9 @@ INSTANTIATE_TEST_SUITE_P(ScreenshareTest, ScreenshareRateAllocationTest, ::testing::Bool()); -TEST_P(ScreenshareRateAllocationTest, BitrateBelowTl0) { +TEST_P(ScreenshareRateAllocationTest, ConferenceBitrateBelowTl0) { SetupConferenceScreenshare(GetParam()); - CreateAllocator(); + CreateAllocator(true); VideoBitrateAllocation allocation = allocator_->Allocate(VideoBitrateAllocationParameters( @@ -684,9 +685,9 @@ TEST_P(ScreenshareRateAllocationTest, BitrateBelowTl0) { EXPECT_EQ(allocation.is_bw_limited(), GetParam()); } -TEST_P(ScreenshareRateAllocationTest, BitrateAboveTl0) { +TEST_P(ScreenshareRateAllocationTest, ConferenceBitrateAboveTl0) { SetupConferenceScreenshare(GetParam()); - CreateAllocator(); + CreateAllocator(true); uint32_t target_bitrate_kbps = (kLegacyScreenshareTargetBitrateKbps + kLegacyScreenshareMaxBitrateKbps) / @@ -704,10 +705,10 @@ TEST_P(ScreenshareRateAllocationTest, BitrateAboveTl0) { EXPECT_EQ(allocation.is_bw_limited(), GetParam()); } -TEST_F(ScreenshareRateAllocationTest, BitrateAboveTl1) { +TEST_F(ScreenshareRateAllocationTest, ConferenceBitrateAboveTl1) { // This test is only for the non-simulcast case. SetupConferenceScreenshare(false); - CreateAllocator(); + CreateAllocator(true); VideoBitrateAllocation allocation = allocator_->Allocate(VideoBitrateAllocationParameters( diff --git a/modules/video_coding/utility/simulcast_utility.cc b/modules/video_coding/utility/simulcast_utility.cc index 58cb991155..043be3d5c3 100644 --- a/modules/video_coding/utility/simulcast_utility.cc +++ b/modules/video_coding/utility/simulcast_utility.cc @@ -84,16 +84,7 @@ bool SimulcastUtility::ValidSimulcastParameters(const VideoCodec& codec, } bool SimulcastUtility::IsConferenceModeScreenshare(const VideoCodec& codec) { - if (codec.mode != VideoCodecMode::kScreensharing || - NumberOfTemporalLayers(codec, 0) != 2) { - return false; - } - - // Fixed default bitrates for legacy screenshare layers mode. - return (codec.numberOfSimulcastStreams == 0 && codec.maxBitrate == 1000) || - (codec.numberOfSimulcastStreams >= 1 && - codec.simulcastStream[0].maxBitrate == 1000 && - codec.simulcastStream[0].targetBitrate == 200); + return codec.legacy_conference_mode; } int SimulcastUtility::NumberOfTemporalLayers(const VideoCodec& codec, diff --git a/modules/video_coding/video_codec_initializer.cc b/modules/video_coding/video_codec_initializer.cc index 7f36f99f89..ccc435a7c4 100644 --- a/modules/video_coding/video_codec_initializer.cc +++ b/modules/video_coding/video_codec_initializer.cc @@ -274,6 +274,8 @@ VideoCodec VideoCodecInitializer::VideoEncoderConfigToVideoCodec( } } + video_codec.legacy_conference_mode = config.legacy_conference_mode; + return video_codec; } diff --git a/modules/video_coding/video_codec_initializer_unittest.cc b/modules/video_coding/video_codec_initializer_unittest.cc index d5a18f0413..686396c51a 100644 --- a/modules/video_coding/video_codec_initializer_unittest.cc +++ b/modules/video_coding/video_codec_initializer_unittest.cc @@ -42,7 +42,8 @@ static const uint32_t kDefaultTargetBitrateBps = 2000000; static const uint32_t kDefaultMaxBitrateBps = 2000000; static const uint32_t kDefaultMinTransmitBitrateBps = 400000; static const int kDefaultMaxQp = 48; -static const uint32_t kScreenshareTl0BitrateBps = 200000; +static const uint32_t kScreenshareTl0BitrateBps = 120000; +static const uint32_t kScreenshareConferenceTl0BitrateBps = 200000; static const uint32_t kScreenshareCodecTargetBitrateBps = 200000; static const uint32_t kScreenshareDefaultFramerate = 5; // Bitrates for the temporal layers of the higher screenshare simulcast stream. @@ -126,7 +127,7 @@ class VideoCodecInitializerTest : public ::testing::Test { VideoStream DefaultScreenshareStream() { VideoStream stream = DefaultStream(); stream.min_bitrate_bps = 30000; - stream.target_bitrate_bps = kScreenshareTl0BitrateBps; + stream.target_bitrate_bps = kScreenshareCodecTargetBitrateBps; stream.max_bitrate_bps = 1000000; stream.max_framerate = kScreenshareDefaultFramerate; stream.num_temporal_layers = 2; @@ -174,6 +175,23 @@ TEST_F(VideoCodecInitializerTest, SingleStreamVp8ScreenshareInactive) { EXPECT_EQ(0U, bitrate_allocation.get_sum_bps()); } +TEST_F(VideoCodecInitializerTest, TemporalLayeredVp8ScreenshareConference) { + SetUpFor(VideoCodecType::kVideoCodecVP8, 1, 2, true); + streams_.push_back(DefaultScreenshareStream()); + EXPECT_TRUE(InitializeCodec()); + bitrate_allocator_->SetLegacyConferenceMode(true); + + EXPECT_EQ(1u, codec_out_.numberOfSimulcastStreams); + EXPECT_EQ(2u, codec_out_.VP8()->numberOfTemporalLayers); + VideoBitrateAllocation bitrate_allocation = + bitrate_allocator_->Allocate(VideoBitrateAllocationParameters( + kScreenshareCodecTargetBitrateBps, kScreenshareDefaultFramerate)); + EXPECT_EQ(kScreenshareCodecTargetBitrateBps, + bitrate_allocation.get_sum_bps()); + EXPECT_EQ(kScreenshareConferenceTl0BitrateBps, + bitrate_allocation.GetBitrate(0, 0)); +} + TEST_F(VideoCodecInitializerTest, TemporalLayeredVp8Screenshare) { SetUpFor(VideoCodecType::kVideoCodecVP8, 1, 2, true); streams_.push_back(DefaultScreenshareStream()); diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index 80b79635f1..f38d2a2990 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -780,6 +780,8 @@ void VideoStreamEncoder::ReconfigureEncoder() { rate_allocator_ = settings_.bitrate_allocator_factory->CreateVideoBitrateAllocator(codec); + rate_allocator_->SetLegacyConferenceMode( + encoder_config_.legacy_conference_mode); // Reset (release existing encoder) if one exists and anything except // start bitrate or max framerate has changed.