From 43bfe0b8a668ea770fef0238440d3898f3ae5264 Mon Sep 17 00:00:00 2001 From: Rasmus Brandt Date: Tue, 21 Jan 2020 13:54:11 +0100 Subject: [PATCH] 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 Reviewed-by: Ilya Nikolaevskiy Cr-Commit-Position: refs/heads/master@{#30336} --- api/video_codecs/video_encoder_config.cc | 3 ++- api/video_codecs/video_encoder_config.h | 19 +++++++++++++++++++ .../vp8_temporal_layers_factory.cc | 1 + media/engine/simulcast.cc | 6 +++--- video/video_stream_encoder.cc | 10 ++++++++++ 5 files changed, 35 insertions(+), 4 deletions(-) diff --git a/api/video_codecs/video_encoder_config.cc b/api/video_codecs/video_encoder_config.cc index 2b1adc021b..6efcbf2bdd 100644 --- a/api/video_codecs/video_encoder_config.cc +++ b/api/video_codecs/video_encoder_config.cc @@ -24,6 +24,7 @@ VideoStream::VideoStream() 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 @@ std::string VideoStream::ToString() const { 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 399b3ccd38..ef8db100a3 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 num_temporal_layers; + // The priority of this stream, to be used when allocating resources + // between multiple streams. absl::optional 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 1de925dbff..193494d71d 100644 --- a/api/video_codecs/vp8_temporal_layers_factory.cc +++ b/api/video_codecs/vp8_temporal_layers_factory.cc @@ -35,6 +35,7 @@ std::unique_ptr Vp8TemporalLayersFactory::Create( 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 f9c2d13ed9..79ff6f5e49 100644 --- a/media/engine/simulcast.cc +++ b/media/engine/simulcast.cc @@ -272,7 +272,7 @@ std::vector GetNormalSimulcastLayers( // 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 @@ std::vector GetScreenshareLayers( 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 @@ std::vector GetScreenshareLayers( 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 fa1191830a..cf38b3b1a8 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -471,6 +471,16 @@ void VideoStreamEncoder::ReconfigureEncoder() { 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.