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 <hta@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31828}
diff --git a/api/video/video_bitrate_allocator.cc b/api/video/video_bitrate_allocator.cc
index 8ad5f75..f4e843b 100644
--- a/api/video/video_bitrate_allocator.cc
+++ b/api/video/video_bitrate_allocator.cc
@@ -49,4 +49,6 @@
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 04de04c..fdc86db 100644
--- a/api/video/video_bitrate_allocator.h
+++ b/api/video/video_bitrate_allocator.h
@@ -40,6 +40,10 @@
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 d03082b..65c7243 100644
--- a/api/video_codecs/video_codec.cc
+++ b/api/video_codecs/video_codec.cc
@@ -83,6 +83,7 @@
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 c07fae9..dd3d344 100644
--- a/api/video_codecs/video_codec.h
+++ b/api/video_codecs/video_codec.h
@@ -146,6 +146,9 @@
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 6efcbf2..19b0135 100644
--- a/api/video_codecs/video_encoder_config.cc
+++ b/api/video_codecs/video_encoder_config.cc
@@ -55,7 +55,8 @@
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 ef8db10..7a59dac 100644
--- a/api/video_codecs/video_encoder_config.h
+++ b/api/video_codecs/video_encoder_config.h
@@ -176,6 +176,9 @@
// 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 863ccc7..60baed9 100644
--- a/media/engine/simulcast_encoder_adapter.cc
+++ b/media/engine/simulcast_encoder_adapter.cc
@@ -619,6 +619,10 @@
// 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 26fa335..a075f7b 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -2399,6 +2399,8 @@
}
}
+ 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 @@
((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 @@
}
std::vector<webrtc::VideoStream>
-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 21033ff..9737dd8 100644
--- a/media/engine/webrtc_video_engine.h
+++ b/media/engine/webrtc_video_engine.h
@@ -661,7 +661,7 @@
const absl::optional<webrtc::DataRate>& experimental_min_bitrate) const;
std::vector<webrtc::VideoStream>
- 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 ac04bc3..9b64d16 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 @@
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 5159526..d3e4a17 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 @@
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 @@
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 fef74cd..13de875 100644
--- a/modules/video_coding/utility/simulcast_rate_allocator.cc
+++ b/modules/video_coding/utility/simulcast_rate_allocator.cc
@@ -61,7 +61,8 @@
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 @@
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 @@
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 @@
: 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 d9d9627..9b2f969 100644
--- a/modules/video_coding/utility/simulcast_rate_allocator.h
+++ b/modules/video_coding/utility/simulcast_rate_allocator.h
@@ -38,6 +38,8 @@
int temporal_id,
bool base_heavy_tl3_alloc);
+ void SetLegacyConferenceMode(bool mode) override;
+
private:
void DistributeAllocationToSimulcastLayers(
DataRate total_bitrate,
@@ -58,6 +60,7 @@
const StableTargetRateExperiment stable_rate_settings_;
const RateControlSettings rate_control_settings_;
std::vector<bool> 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 871e5a1..69c14ff 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 @@
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<bool>& active_streams) {
@@ -669,9 +670,9 @@
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 @@
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 @@
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 58cb991..043be3d 100644
--- a/modules/video_coding/utility/simulcast_utility.cc
+++ b/modules/video_coding/utility/simulcast_utility.cc
@@ -84,16 +84,7 @@
}
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 7f36f99..ccc435a 100644
--- a/modules/video_coding/video_codec_initializer.cc
+++ b/modules/video_coding/video_codec_initializer.cc
@@ -274,6 +274,8 @@
}
}
+ 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 d5a18f0..686396c 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 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 @@
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 @@
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 80b7963..f38d2a2 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -780,6 +780,8 @@
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.
