Enforce VideoEncoderConfig.num_temporal_layers >= 1.
This change clarifies the semantics of this field:
unset: Depends on context.
== 0: Invalid.
== 1: No temporal layering.
>= 2: Temporal layering.
We should try to remove the wrapping optional later.
Bug: webrtc:11297
Change-Id: Id765f2dc1d31a4ba3cd424978ac6054cd60152ea
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166528
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30336}
diff --git a/api/video_codecs/video_encoder_config.cc b/api/video_codecs/video_encoder_config.cc
index 2b1adc0..6efcbf2 100644
--- a/api/video_codecs/video_encoder_config.cc
+++ b/api/video_codecs/video_encoder_config.cc
@@ -24,6 +24,7 @@
max_bitrate_bps(-1),
scale_resolution_down_by(-1.),
max_qp(-1),
+ num_temporal_layers(absl::nullopt),
active(true) {}
VideoStream::VideoStream(const VideoStream& other) = default;
@@ -39,7 +40,7 @@
ss << ", target_bitrate_bps:" << target_bitrate_bps;
ss << ", max_bitrate_bps:" << max_bitrate_bps;
ss << ", max_qp: " << max_qp;
- ss << ", num_temporal_layers: " << num_temporal_layers.value_or(0);
+ ss << ", num_temporal_layers: " << num_temporal_layers.value_or(1);
ss << ", bitrate_priority: " << bitrate_priority.value_or(0);
ss << ", active: " << active;
diff --git a/api/video_codecs/video_encoder_config.h b/api/video_codecs/video_encoder_config.h
index 399b3cc..ef8db10 100644
--- a/api/video_codecs/video_encoder_config.h
+++ b/api/video_codecs/video_encoder_config.h
@@ -24,28 +24,47 @@
namespace webrtc {
+// The |VideoStream| struct describes a simulcast layer, or "stream".
struct VideoStream {
VideoStream();
~VideoStream();
VideoStream(const VideoStream& other);
std::string ToString() const;
+ // Width in pixels.
size_t width;
+
+ // Height in pixels.
size_t height;
+
+ // Frame rate in fps.
int max_framerate;
+ // Bitrate, in bps, for the stream.
int min_bitrate_bps;
int target_bitrate_bps;
int max_bitrate_bps;
+
// Scaling factor applied to the stream size.
// |width| and |height| values are already scaled down.
double scale_resolution_down_by;
+
+ // Maximum Quantization Parameter to use when encoding the stream.
int max_qp;
+ // Determines the number of temporal layers that the stream should be
+ // encoded with. This value should be greater than zero.
+ // TODO(brandtr): This class is used both for configuring the encoder
+ // (meaning that this field _must_ be set), and for signaling the app-level
+ // encoder settings (meaning that the field _may_ be set). We should separate
+ // this and remove this optional instead.
absl::optional<size_t> num_temporal_layers;
+ // The priority of this stream, to be used when allocating resources
+ // between multiple streams.
absl::optional<double> bitrate_priority;
+ // If this stream is enabled by the user, or not.
bool active;
};
diff --git a/api/video_codecs/vp8_temporal_layers_factory.cc b/api/video_codecs/vp8_temporal_layers_factory.cc
index 1de925d..193494d 100644
--- a/api/video_codecs/vp8_temporal_layers_factory.cc
+++ b/api/video_codecs/vp8_temporal_layers_factory.cc
@@ -35,6 +35,7 @@
for (int i = 0; i < num_streams; ++i) {
int num_temporal_layers =
SimulcastUtility::NumberOfTemporalLayers(codec, i);
+ RTC_DCHECK_GE(num_temporal_layers, 1);
if (SimulcastUtility::IsConferenceModeScreenshare(codec) && i == 0) {
// Legacy screenshare layers supports max 2 layers.
num_temporal_layers = std::max(2, num_temporal_layers);
diff --git a/media/engine/simulcast.cc b/media/engine/simulcast.cc
index f9c2d13..79ff6f5 100644
--- a/media/engine/simulcast.cc
+++ b/media/engine/simulcast.cc
@@ -272,7 +272,7 @@
// TODO(pbos): Fill actual temporal-layer bitrate thresholds.
layers[s].max_qp = max_qp;
layers[s].num_temporal_layers =
- temporal_layers_supported ? DefaultNumberOfTemporalLayers(s, false) : 0;
+ temporal_layers_supported ? DefaultNumberOfTemporalLayers(s, false) : 1;
layers[s].max_bitrate_bps = FindSimulcastMaxBitrateBps(width, height);
layers[s].target_bitrate_bps = FindSimulcastTargetBitrateBps(width, height);
int num_temporal_layers = DefaultNumberOfTemporalLayers(s, false);
@@ -343,7 +343,7 @@
layers[0].min_bitrate_bps = webrtc::kDefaultMinVideoBitrateBps;
layers[0].target_bitrate_bps = kScreenshareDefaultTl0BitrateKbps * 1000;
layers[0].max_bitrate_bps = kScreenshareDefaultTl1BitrateKbps * 1000;
- layers[0].num_temporal_layers = temporal_layers_supported ? 2 : 0;
+ layers[0].num_temporal_layers = temporal_layers_supported ? 2 : 1;
// With simulcast enabled, add another spatial layer. This one will have a
// more normal layout, with the regular 3 temporal layer pattern and no fps
@@ -388,7 +388,7 @@
layers[1].max_qp = max_qp;
layers[1].max_framerate = kDefaultVideoMaxFramerate;
layers[1].num_temporal_layers =
- temporal_layers_supported ? DefaultNumberOfTemporalLayers(1, true) : 0;
+ temporal_layers_supported ? DefaultNumberOfTemporalLayers(1, true) : 1;
layers[1].min_bitrate_bps = using_boosted_bitrate
? kScreenshareHighStreamMinBitrateBps
: layers[0].target_bitrate_bps * 2;
diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc
index fa11918..cf38b3b 100644
--- a/video/video_stream_encoder.cc
+++ b/video/video_stream_encoder.cc
@@ -471,6 +471,16 @@
encoder_config_.video_stream_factory->CreateEncoderStreams(
last_frame_info_->width, last_frame_info_->height, encoder_config_);
+ // Check that the higher layers do not try to set number of temporal layers
+ // to less than 1.
+ // TODO(brandtr): Get rid of the wrapping optional as it serves no purpose
+ // at this layer.
+#if RTC_DCHECK_IS_ON
+ for (const auto& stream : streams) {
+ RTC_DCHECK_GE(stream.num_temporal_layers.value_or(1), 1);
+ }
+#endif
+
// TODO(ilnik): If configured resolution is significantly less than provided,
// e.g. because there are not enough SSRCs for all simulcast streams,
// signal new resolutions via SinkWants to video source.
