[Enhancement]Extend SimulcastEncoderAdapter to support adaptive streaming#75
Conversation
b181ae4 to
5008872
Compare
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…oder info forwarding
b1a3ce4 to
bbbb3bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
video/video_stream_encoder_unittest.cc (1)
6830-6844: Consider extracting a small helper for 3-layer VP8 config construction.These repeated setup blocks differ mostly by active-layer flags; a helper would reduce duplication and keep future updates safer.
Also applies to: 6860-6873, 6900-6913, 6930-6943
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@video/video_stream_encoder_unittest.cc` around lines 6830 - 6844, Extract a small helper (e.g., Build3LayerVp8Config or MakeVp8SimulcastConfig) that wraps creating a VideoEncoderConfig via test::FillEncoderConfiguration(PayloadStringToCodecType("VP8"), 3, &config), sets video_stream_factory=nullptr, sets each simulcast_layers[*].num_temporal_layers=1 and .max_framerate=kDefaultFramerate, assigns max_bitrate_bps=kSimulcastTargetBitrate.bps() and content_type=VideoEncoderConfig::ContentType::kRealtimeVideo, and accepts a parameter to mark which of the three simulcast_layers is active (or a three-bool vector); then replace the repeated blocks that manually toggle config_1_active.simulcast_layers[i].active (occurrences around the references shown) with calls to this helper to reduce duplication and centralize future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@media/engine/simulcast_encoder_adapter_unittest.cc`:
- Around line 1761-1768: The test's fps_allocation expectations are inconsistent
with the current adapter/mock contract: MockVideoEncoder::SetRates() does not
clear fps_allocation_ for paused layers and
SimulcastEncoderAdapter::GetEncoderInfo() copies
encoder_impl_info.fps_allocation[0] into each stream context, so update the
assertions to match that behavior (i.e., expect slot 0 to remain populated with
the low encoder's allocation rather than IsEmpty()). Locate the failing
assertions around info.fps_allocation in the test and change them to assert the
populated values (mirroring low_encoder->set_fps_allocation(...) ) and apply the
same change to the similar block at lines referenced (around 1825-1831), instead
of modifying production copying or mock SetRates behavior.
In `@video/adaptation/video_stream_encoder_resource_manager.cc`:
- Around line 562-598: ResetAdaptationsForSimulcastChange clears
bandwidth_quality_scaler_resource_ but the subsequent reconfigure path
(ConfigureBandwidthQualityScaler) can immediately re-add it for encoders with
is_qp_trusted == false because that function doesn't consider simulcast_active_.
Modify ConfigureBandwidthQualityScaler (or the place that adds
bandwidth_quality_scaler_resource_) to check simulcast_active_ (and/or
is_qp_trusted) and skip creating/adding bandwidth_quality_scaler_resource_ when
we're handling a simulcast active-layer transition (i.e., simulcast_active_
indicates the change) for untrusted-QP encoders; ensure the check uses the
existing members bandwidth_quality_scaler_resource_, simulcast_active_, and
is_qp_trusted to avoid re-adding the resource during this reset flow.
In `@video/video_stream_encoder.cc`:
- Around line 1062-1127: The code currently determines simulcast state from
encoder_config_.simulcast_layers which only updates on reconfigure; instead,
derive the active-layer count from the runtime EncoderInfo/state (the info
passed into ConfigureQualityScaler/EncodeVideoFrame or the current per-layer
sending state exposed by SetRates) so transitions reflect actual sending layers.
Change the logic that computes num_active_layers (and the subsequent
is_simulcast/was_simulcast decisions and calls to
stream_resource_manager_.SetSimulcastActive(...) and
ResetAdaptationsForSimulcastChange()) to use the runtime active-layer count from
the EncoderInfo/runtime layer state, and update
prev_num_active_simulcast_layers_ from that runtime count so the quality scaler
is enabled/disabled based on actual sending layers rather than only
encoder_config_.simulcast_layers.
---
Nitpick comments:
In `@video/video_stream_encoder_unittest.cc`:
- Around line 6830-6844: Extract a small helper (e.g., Build3LayerVp8Config or
MakeVp8SimulcastConfig) that wraps creating a VideoEncoderConfig via
test::FillEncoderConfiguration(PayloadStringToCodecType("VP8"), 3, &config),
sets video_stream_factory=nullptr, sets each
simulcast_layers[*].num_temporal_layers=1 and .max_framerate=kDefaultFramerate,
assigns max_bitrate_bps=kSimulcastTargetBitrate.bps() and
content_type=VideoEncoderConfig::ContentType::kRealtimeVideo, and accepts a
parameter to mark which of the three simulcast_layers is active (or a three-bool
vector); then replace the repeated blocks that manually toggle
config_1_active.simulcast_layers[i].active (occurrences around the references
shown) with calls to this helper to reduce duplication and centralize future
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33f64430-927d-4742-9af5-29b0e9aed35d
📒 Files selected for processing (10)
media/engine/simulcast_encoder_adapter.ccmedia/engine/simulcast_encoder_adapter_unittest.ccsdk/objc/native/src/objc_video_encoder_factory.mmvideo/adaptation/video_stream_encoder_resource_manager.ccvideo/adaptation/video_stream_encoder_resource_manager.hvideo/config/encoder_stream_factory.ccvideo/config/encoder_stream_factory_unittest.ccvideo/video_stream_encoder.ccvideo/video_stream_encoder.hvideo/video_stream_encoder_unittest.cc
|
Pushed I also folded in the review-summary nit by extracting Verified locally with:
|
Summary
This fixes quality scaler interference during simulcast transitions and ensures
the SimulcastEncoderAdapter (SEA) correctly reports the active encoder's runtime
adaptation hints when only one spatial layer is active.
Without this fix, the quality scaler remains active during simulcast and
continuously degrades the input resolution, causing all simulcast layer
dimensions to be derived from scaled-down frames (e.g. f=268x480 instead of
f=720x1280 for 720p capture). Additionally, the SEA was reporting aggregated
encoder info with
scaling_settings = kOffeven when only one layer was active,preventing the quality scaler from starting in single-active-layer mode (1:1 calls).
Problem
Three interrelated issues combined to produce broken quality adaptation behavior
during simulcast transitions:
Quality scaler active during simulcast. When transitioning from a 1:1
call to a group call (simulcast), the quality scaler was not disabled. It
continued to scale down the input resolution, and since simulcast layer
dimensions are computed from the input frame size, all layers (q, h, f) were
recomputed from the degraded input. This produced cascading downgrades
(quality:1 → quality:2 → quality:3 → quality:4) and eventually triggered
simulcast layer count reduction from 3 to 2.
SEA reporting aggregated info in single-active-layer mode. The
SimulcastEncoderAdapter only forwarded the active encoder's info when
stream_contexts_.size() == 1. At runtime, SEA can have multiple streamcontexts while only one layer is unpaused. In that state, SEA reported
scaling_settings = kOff, so the quality scaler could never start — even in1:1 mode where it's needed.
Conservative min_pixels_per_frame floor for iOS encoders. The ObjC
encoder bridge used the 2-argument
ScalingSettingsconstructor, inheritingkDefaultMinPixelsPerFrame = 320*180 = 57600. This prevented the qualityscaler from reducing resolution below ~180x320 (the quarter-resolution
simulcast layer for 720p), limiting adaptation range on constrained networks.
Root Cause
Quality scaler during simulcast:
ConfigureQualityScaler()inVideoStreamEncoderResourceManagerhad no awareness of whether simulcast wasactive. It would start/keep the quality scaler running whenever the encoder
reported QP thresholds or
is_quality_scaling_allowedwas set, regardless ofhow many simulcast layers were active.
SEA encoder info: The bug is in
SimulcastEncoderAdapter::GetEncoderInfo().The old logic treated "single stream context" as equivalent to "single active
layer", but those are not the same thing at runtime. Once SEA enters
multi-encoder mode, the number of stream contexts stays greater than one even if
only one layer is currently unpaused.
FindRequiredActiveLayers:
FindRequiredActiveLayers()inencoder_stream_factory.ccreturned the position of the first active layerinstead of the highest, which could discard upper active layers when lower ones
were inactive.
min_pixels_per_frame: The ObjC bridge in
objc_video_encoder_factory.mmused
ScalingSettings(low, high)which defaultsmin_pixels_per_frameto57600 — too conservative for iOS VideoToolbox encoders that can encode well
below that threshold.
Fix
1. Quality scaler suppression during simulcast
simulcast_active_flag toVideoStreamEncoderResourceManager.SetSimulcastActive(bool)is called fromReconfigureEncoder()based on thenumber of active simulcast layers (not configured streams — the config may
always specify 3 streams while the SFU controls activation).
ConfigureQualityScaler()now checks!simulcast_active_as an additionalcondition for
quality_scaling_allowed. When simulcast is active, the qualityscaler is stopped and removed.
2. Simulcast transition handling in VideoStreamEncoder
ReconfigureEncoder()now detects transitions between single-stream andsimulcast based on
num_active_layers > 1.scaler and calls
ResetAdaptationsForSimulcastChange()to clear accumulatedresolution restrictions. The video source then delivers full-resolution frames
for correct layer dimension computation.
quality scaler from a clean state (zero downgrades).
3. SEA single-active-layer encoder info forwarding
runtime-sensitive fields:
scaling_settings,supports_native_handle,has_trusted_rate_controller,is_hardware_accelerated,is_qp_trusted,resolution_bitrate_limits,min_qp,preferred_pixel_formats.implementation_name,fps_allocation,requested_resolution_alignment,apply_alignment_to_all_simulcast_layers.4. FindRequiredActiveLayers fix
FindRequiredActiveLayers()to return the position after the highestactive layer (not the first), ensuring all active layers are preserved in the
stream count.
5. Lower min_pixels_per_frame for iOS encoders
ScalingSettings(low, high, 90*160)instead of
ScalingSettings(low, high). This lowers the quality scaler floorfrom 57600 to 14400 pixels, allowing adaptation below the quarter-resolution
simulcast layer on constrained networks (e.g. 3G).
Why This Is Safe
state. Single-active-layer behavior (1:1 calls) is unchanged.
first frame after clearing is still degraded, the source delivers
full-resolution frames on the next frame, triggering a reconfigure with
correct dimensions.
pre-init
GetEncoderInfo(), bitrate allocation, or true multi-layeraggregation.
min_pixels_per_framechange only affects ObjC-bridged encoders (iOSVideoToolbox). The global
kDefaultMinPixelsPerFrameremains unchanged forsoftware and Android encoders.
Tests
Added regression coverage in:
video_stream_encoder_unittest.cc: quality scaler restrictions are clearedwhen active layers increase from 1 to multiple, and from 2 to 3.
simulcast_encoder_adapter_unittest.cc: forwarding active encoder info whenonly one layer is unpaused; restoring aggregated SEA behavior when a second
layer becomes active.
encoder_stream_factory_unittest.cc: stream count preservation for sparseactive patterns, highest-only active, lowest-only active.
Validation
Validated with on-device iOS testing:
dimensions match configured ratios from full capture resolution.
Summary by CodeRabbit
Bug Fixes
New Features
Tests