Fix first encoding's maxBitrate being ignored when scalability is set.
EncoderStreamFactory has two code paths for creating a stream: the
"simulcast path" and the "default path". Only the former cares about
encoding paramter's maxBitrate. The latter assumes that
`encoder_config.max_bitrate_bps` already encompasses the maxBitrate of
the first encoding, but this is not always the case.
As of M113, when scalability mode is specified, {active,inactive} does
not count as simulcast stream but as a default stream represented by
encoding[0].
The problem is that `encoder_config.max_bitrate_bps` only includes
`encodings[0].max_bitrate_bps` when `encodings.size() == 1` which isn't
the case here.
This CL fixes the problem by making the "create default stream" code
path look at the first encoding's maxBitrate and remove existing
assumptions that `encoder_config.max_bitrate_bps` encompasses
`encodings[0].max_bitrate_bps`. This is a step in the right direction
since we're trying to remove all special cases and have encodings map
1:1 with SSRCs, so the "max bps of entire stream" should indeed be a
separate limit than the per-encoding limits and it was confusing that
sometimes it included and sometimes it excluded encoding[0]'s limit.
This issue did not happen in {inactive,active} since that code path
counts as "simulcast stream", so "default stream" is only ever
applicable for index 0.
TESTED=Simulcast Playground, see https://crbug.com/1455962.
Bug: chromium:1455962
Change-Id: I7c44925b780623b5979751e8959e972293648a3d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/313282
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40482}
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index 35a5eb4..95668de 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -352,18 +352,6 @@
return false;
}
-// Returns its smallest positive argument. If neither argument is positive,
-// returns an arbitrary nonpositive value.
-int MinPositive(int a, int b) {
- if (a <= 0) {
- return b;
- }
- if (b <= 0) {
- return a;
- }
- return std::min(a, b);
-}
-
bool IsLayerActive(const webrtc::RtpEncodingParameters& layer) {
return layer.active &&
(!layer.max_bitrate_bps || *layer.max_bitrate_bps > 0) &&
@@ -2042,29 +2030,20 @@
}
// parameters_.max_bitrate comes from the max bitrate set at the SDP
- // (m-section) level with the attribute "b=AS." Note that we override this
- // value below if the RtpParameters max bitrate set with
- // RtpSender::SetParameters has a lower value.
+ // (m-section) level with the attribute "b=AS." Note that stream max bitrate
+ // is the RtpSender's max bitrate, but each individual encoding may also have
+ // its own max bitrate specified by SetParameters.
int stream_max_bitrate = parameters_.max_bitrate_bps;
- // When simulcast is enabled (when there are multiple encodings),
- // encodings[i].max_bitrate_bps will be enforced by
- // encoder_config.simulcast_layers[i].max_bitrate_bps. Otherwise, it's
- // enforced by stream_max_bitrate, taking the minimum of the two maximums
- // (one coming from SDP, the other coming from RtpParameters).
- if (rtp_parameters_.encodings[0].max_bitrate_bps &&
- rtp_parameters_.encodings.size() == 1) {
- stream_max_bitrate =
- MinPositive(*(rtp_parameters_.encodings[0].max_bitrate_bps),
- parameters_.max_bitrate_bps);
- }
-
// The codec max bitrate comes from the "x-google-max-bitrate" parameter
- // attribute set in the SDP for a specific codec. As done in
- // WebRtcVideoSendChannel::SetSendParameters, this value does not override the
- // stream max_bitrate set above.
+ // attribute set in the SDP for a specific codec. It only has an effect if
+ // max bitrate is not specified via "b=AS" and doesn't apply in singlecast
+ // if the encoding has a max bitrate.
int codec_max_bitrate_kbps;
+ bool is_single_encoding_with_max_bitrate =
+ rtp_parameters_.encodings.size() == 1 &&
+ rtp_parameters_.encodings[0].max_bitrate_bps.value_or(0) > 0;
if (codec.GetParam(kCodecParamMaxBitrate, &codec_max_bitrate_kbps) &&
- stream_max_bitrate == -1) {
+ stream_max_bitrate == -1 && !is_single_encoding_with_max_bitrate) {
stream_max_bitrate = codec_max_bitrate_kbps * 1000;
}
encoder_config.max_bitrate_bps = stream_max_bitrate;
diff --git a/video/config/encoder_stream_factory.cc b/video/config/encoder_stream_factory.cc
index de475b9..c955602 100644
--- a/video/config/encoder_stream_factory.cc
+++ b/video/config/encoder_stream_factory.cc
@@ -174,10 +174,27 @@
const absl::optional<webrtc::DataRate>& experimental_min_bitrate) const {
std::vector<webrtc::VideoStream> layers;
+ // The max bitrate specified by the API.
+ // - `encoder_config.simulcast_layers[0].max_bitrate_bps` comes from the first
+ // RtpEncodingParamters, which is the encoding of this stream.
+ // - `encoder_config.max_bitrate_bps` comes from SDP; "b=AS" or conditionally
+ // "x-google-max-bitrate".
+ // If `api_max_bitrate_bps` has a value then it is positive.
+ absl::optional<int> api_max_bitrate_bps;
+ if (encoder_config.simulcast_layers[0].max_bitrate_bps > 0) {
+ api_max_bitrate_bps = encoder_config.simulcast_layers[0].max_bitrate_bps;
+ }
+ if (encoder_config.max_bitrate_bps > 0) {
+ api_max_bitrate_bps =
+ api_max_bitrate_bps.has_value()
+ ? std::min(encoder_config.max_bitrate_bps, *api_max_bitrate_bps)
+ : encoder_config.max_bitrate_bps;
+ }
+
// For unset max bitrates set default bitrate for non-simulcast.
int max_bitrate_bps =
- (encoder_config.max_bitrate_bps > 0)
- ? encoder_config.max_bitrate_bps
+ api_max_bitrate_bps.has_value()
+ ? *api_max_bitrate_bps
: GetMaxDefaultVideoBitrateKbps(width, height, is_screenshare_) *
1000;
@@ -189,7 +206,7 @@
// Use set min bitrate.
min_bitrate_bps = encoder_config.simulcast_layers[0].min_bitrate_bps;
// If only min bitrate is configured, make sure max is above min.
- if (encoder_config.max_bitrate_bps <= 0)
+ if (!api_max_bitrate_bps.has_value())
max_bitrate_bps = std::max(min_bitrate_bps, max_bitrate_bps);
}
int max_framerate = (encoder_config.simulcast_layers[0].max_framerate > 0)
@@ -253,7 +270,7 @@
sum_max_bitrates_kbps += spatial_layer.maxBitrate;
}
RTC_DCHECK_GE(sum_max_bitrates_kbps, 0);
- if (encoder_config.max_bitrate_bps <= 0) {
+ if (!api_max_bitrate_bps.has_value()) {
max_bitrate_bps = sum_max_bitrates_kbps * 1000;
} else {
max_bitrate_bps =
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index c63cf01..1e24093 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -1106,8 +1106,7 @@
// or/and can be provided by encoder. In presence of both set of
// limits, the final set is derived as their intersection.
int min_bitrate_bps;
- if (encoder_config_.simulcast_layers.empty() ||
- encoder_config_.simulcast_layers[0].min_bitrate_bps <= 0) {
+ if (encoder_config_.simulcast_layers[0].min_bitrate_bps <= 0) {
min_bitrate_bps = encoder_bitrate_limits->min_bitrate_bps;
} else {
min_bitrate_bps = std::max(encoder_bitrate_limits->min_bitrate_bps,
@@ -1115,10 +1114,20 @@
}
int max_bitrate_bps;
- // We don't check encoder_config_.simulcast_layers[0].max_bitrate_bps
- // here since encoder_config_.max_bitrate_bps is derived from it (as
- // well as from other inputs).
- if (encoder_config_.max_bitrate_bps <= 0) {
+ // The API max bitrate comes from both `encoder_config_.max_bitrate_bps`
+ // and `encoder_config_.simulcast_layers[0].max_bitrate_bps`.
+ absl::optional<int> api_max_bitrate_bps;
+ if (encoder_config_.simulcast_layers[0].max_bitrate_bps > 0) {
+ api_max_bitrate_bps =
+ encoder_config_.simulcast_layers[0].max_bitrate_bps;
+ }
+ if (encoder_config_.max_bitrate_bps > 0) {
+ api_max_bitrate_bps = api_max_bitrate_bps.has_value()
+ ? std::min(encoder_config_.max_bitrate_bps,
+ *api_max_bitrate_bps)
+ : encoder_config_.max_bitrate_bps;
+ }
+ if (!api_max_bitrate_bps.has_value()) {
max_bitrate_bps = encoder_bitrate_limits->max_bitrate_bps;
} else {
max_bitrate_bps = std::min(encoder_bitrate_limits->max_bitrate_bps,
@@ -1138,7 +1147,7 @@
<< ", max=" << encoder_bitrate_limits->max_bitrate_bps
<< ") do not intersect with limits set by app"
<< " (min=" << streams.back().min_bitrate_bps
- << ", max=" << encoder_config_.max_bitrate_bps
+ << ", max=" << api_max_bitrate_bps.value_or(-1)
<< "). The app bitrate limits will be used.";
}
}
