-
Notifications
You must be signed in to change notification settings - Fork 193
Dynacast: keep SVC tracks active while any quality is subscribed #1214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| livekit: patch | ||
| --- | ||
|
|
||
| Dynacast: keep SVC (VP9/AV1) tracks active while any quality is subscribed - #1214 (@MaxHeimbrock) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| livekit: patch | ||
| livekit-ffi: patch | ||
| --- | ||
|
|
||
| Fix for dynacast error - #1213 (@MaxHeimbrock) |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1928,10 +1928,17 @@ impl RoomSession { | |||||||||||
| } | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| let video_codec = publication.publish_options().video_codec; | ||||||||||||
| // SVC codecs carry all spatial layers in one encoded stream. | ||||||||||||
| let is_svc = matches!( | ||||||||||||
| video_codec, | ||||||||||||
| crate::options::VideoCodec::VP9 | crate::options::VideoCodec::AV1 | ||||||||||||
| ); | ||||||||||||
|
Comment on lines
+1933
to
+1936
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Dynacast layer control is broken for VP9/AV1 simulcast tracks because SVC detection relies on codec type alone The SVC flag is set based solely on codec type ( Impact: For VP9/AV1 simulcast tracks, dynacast can never disable individual quality layers — all layers stay active whenever any subscriber is present. Mechanism: SVC override forces all qualities enabled for misidentified simulcast tracksVP9 and AV1 can be published in regular simulcast mode (multiple RTP encodings with RIDs, When The let is_svc = publication.publish_options().scalability_mode.is_some();
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||
|
|
||||||||||||
| let qualities: Vec<proto::SubscribedQuality> = if !update.subscribed_codecs.is_empty() { | ||||||||||||
| // This is the requested codec, which we also advertise in simulcast_codecs and use | ||||||||||||
| // for sender codec preferences, so it should match the SFU's subscribed codec key. | ||||||||||||
| let codec = publication.publish_options().video_codec.as_str().to_lowercase(); | ||||||||||||
| let codec = video_codec.as_str().to_lowercase(); | ||||||||||||
| log::info!( | ||||||||||||
| "dynacast: SFU quality update for {}: subscribed_codecs={:?}, looking for codec '{}'", | ||||||||||||
| track_sid, | ||||||||||||
|
|
@@ -1980,7 +1987,7 @@ impl RoomSession { | |||||||||||
| update.subscribed_qualities.clone() | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| if let Err(e) = video_track.set_publishing_layers(&qualities) { | ||||||||||||
| if let Err(e) = video_track.set_publishing_layers(&qualities, is_svc) { | ||||||||||||
| log::error!("dynacast: failed to set publishing layers for {}: {}", track_sid, e); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Diagnostic log shows wrong quality label for SVC tracks with empty RID The diagnostic log builds its layer summary using the raw encoding RID ( Impact: Developers debugging SVC dynacast issues see incorrect quality labels in logs, making diagnosis harder. Mechanism: raw vs corrected RID in log vs matchAt (Refers to line 407) Was this helpful? React with 👍 or 👎 to provide feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Public API inconsistency: publishing_layers() reports different quality than set_publishing_layers() for SVC tracks The public The test at (Refers to lines 336-350) Was this helpful? React with 👍 or 👎 to provide feedback. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -355,6 +355,7 @@ impl LocalVideoTrack { | |
| pub(crate) fn set_publishing_layers( | ||
| &self, | ||
| qualities: &[proto::SubscribedQuality], | ||
| is_svc: bool, | ||
| ) -> RoomResult<()> { | ||
| let transceiver = self.transceiver().ok_or_else(|| { | ||
| RoomError::Internal("cannot set publishing layers: no transceiver".into()) | ||
|
|
@@ -368,19 +369,34 @@ impl LocalVideoTrack { | |
| return Ok(()); | ||
| } | ||
|
|
||
| // For SVC codecs all spatial layers ride in a single encoded stream | ||
| // and the SFU selects layers server-side, so any enabled quality | ||
| // keeps the whole encoding active. | ||
| let qualities: Vec<proto::SubscribedQuality> = | ||
| if is_svc && qualities.iter().any(|q| q.enabled) { | ||
| qualities | ||
| .iter() | ||
| .map(|q| proto::SubscribedQuality { enabled: true, ..q.clone() }) | ||
| .collect() | ||
| } else { | ||
| qualities.to_vec() | ||
| }; | ||
|
|
||
| let mut changed = false; | ||
| for encoding in &mut params.encodings { | ||
| let quality = crate::options::video_quality_for_rid_or_default(&encoding.rid); | ||
| // The SFU addresses layers by spatial index (0 = Low), so a | ||
| // single rid-less encoding is addressed as Low, not High. | ||
| let rid = if encoding.rid.is_empty() { "q" } else { encoding.rid.as_str() }; | ||
| let quality = crate::options::video_quality_for_rid_or_default(rid); | ||
|
|
||
| let should_active = qualities | ||
| .iter() | ||
| .find(|q| q.quality == quality as i32) | ||
| .map(|q| q.enabled) | ||
| .unwrap_or(false); | ||
| // A quality missing from the update is left untouched. | ||
| let Some(subscribed) = qualities.iter().find(|q| q.quality == quality as i32) else { | ||
| continue; | ||
| }; | ||
|
Comment on lines
+392
to
+395
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Behavioral change: missing qualities now leave encodings untouched instead of disabling them The old code at This is a meaningful semantic change: previously, an update listing only Was this helpful? React with 👍 or 👎 to provide feedback. |
||
|
|
||
| if encoding.active != should_active { | ||
| if encoding.active != subscribed.enabled { | ||
| changed = true; | ||
| encoding.active = should_active; | ||
| encoding.active = subscribed.enabled; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -394,11 +410,10 @@ impl LocalVideoTrack { | |
| }) | ||
| .collect(); | ||
|
|
||
| sender | ||
| .set_parameters(params) | ||
| .map_err(|e| RoomError::Internal(format!("failed to set sender parameters: {}", e)))?; | ||
|
|
||
| if changed { | ||
| sender.set_parameters(params).map_err(|e| { | ||
| RoomError::Internal(format!("failed to set sender parameters: {}", e)) | ||
| })?; | ||
| log::debug!("dynacast: layers changed -> [{}]", layers.join(", ")); | ||
| } else { | ||
| log::debug!("dynacast: layers unchanged [{}]", layers.join(", ")); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ use { | |
| TestRoomOptions, | ||
| }, | ||
| livekit::{ | ||
| options::VideoCodec, | ||
| options::{TrackPublishOptions, VideoCodec}, | ||
| prelude::*, | ||
| track::{PublishingLayerQuality, VideoQuality}, | ||
| }, | ||
|
|
@@ -428,3 +428,110 @@ async fn test_dynacast_multiple_subscribers_only_publish_requested_tracks() -> R | |
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Verifies dynacast behavior for an SVC track (VP9, L3T3_KEY). | ||
| /// | ||
| /// SVC tracks carry all spatial layers in a single encoded stream and the SFU | ||
| /// selects layers server-side, so: | ||
| /// 1. Subscriber quality requests must never deactivate the encoding while at | ||
| /// least one quality is subscribed (any-enabled rule). | ||
| /// 2. Unsubscribing the last subscriber deactivates the encoding. | ||
| /// 3. Resubscribing reactivates it. | ||
| #[cfg(feature = "__lk-e2e-test")] | ||
| #[test_log::test(tokio::test)] | ||
| async fn test_dynacast_svc() -> Result<()> { | ||
| let mut pub_room_opts = RoomOptions::default(); | ||
| pub_room_opts.dynacast = true; | ||
| let pub_options = TestRoomOptions { room: pub_room_opts, ..Default::default() }; | ||
| let sub_options = TestRoomOptions::default(); | ||
|
|
||
| let mut rooms = test_rooms_with_options([pub_options, sub_options]).await?; | ||
| let (pub_room, _pub_events) = rooms.remove(0); | ||
|
Comment on lines
+446
to
+449
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: You can use |
||
| let (_sub_room, mut sub_events) = rooms.remove(0); | ||
|
|
||
| let pub_room = Arc::new(pub_room); | ||
| let solid_params = SolidColorParams { width: 1280, height: 720, luma: 128 }; | ||
| let mut solid_track = SolidColorTrack::new(pub_room.clone(), solid_params); | ||
| solid_track | ||
| .publish_with_options(TrackPublishOptions { | ||
| video_codec: VideoCodec::VP9, | ||
| simulcast: false, | ||
| scalability_mode: Some("L3T3_KEY".to_string()), | ||
| ..Default::default() | ||
| }) | ||
| .await?; | ||
|
|
||
| let sub_publication: RemoteTrackPublication = timeout(Duration::from_secs(15), async { | ||
| loop { | ||
| let Some(event) = sub_events.recv().await else { | ||
| return Err(anyhow!("Event channel closed before TrackSubscribed")); | ||
| }; | ||
| if let RoomEvent::TrackSubscribed { publication, .. } = event { | ||
| return Ok(publication); | ||
| } | ||
| } | ||
| }) | ||
| .await??; | ||
|
|
||
| let pub_video_track = publisher_video_track(&pub_room)?; | ||
|
|
||
| // --- Baseline: the single SVC encoding is active --- | ||
| let layers = | ||
| wait_for_layers(&pub_video_track, "svc baseline", Duration::from_secs(15), |layers| { | ||
| layers.len() == 1 && layers[0].active | ||
| }) | ||
| .await?; | ||
| log::info!("dynacast svc baseline layers: {:?}", layers); | ||
|
|
||
| // --- Request LOW quality: the SVC encoding must stay active --- | ||
| log::info!("dynacast svc test: requesting LOW quality"); | ||
| sub_publication.set_video_quality(VideoQuality::Low); | ||
|
|
||
| // The resulting quality update arrives asynchronously; poll to make sure | ||
| // the encoding never gets deactivated by it. | ||
| let deadline = tokio::time::Instant::now() + Duration::from_secs(10); | ||
| while tokio::time::Instant::now() < deadline { | ||
| let layers = pub_video_track.publishing_layers(); | ||
| assert!( | ||
| !layers.is_empty() && layers.iter().all(|layer| layer.active), | ||
| "SVC encoding must stay active after LOW request, got {:?}", | ||
| layers | ||
| ); | ||
| time::sleep(Duration::from_millis(250)).await; | ||
| } | ||
|
|
||
| // --- Request HIGH quality again: still active --- | ||
| log::info!("dynacast svc test: requesting HIGH quality"); | ||
| sub_publication.set_video_quality(VideoQuality::High); | ||
|
|
||
| let deadline = tokio::time::Instant::now() + Duration::from_secs(10); | ||
| while tokio::time::Instant::now() < deadline { | ||
| let layers = pub_video_track.publishing_layers(); | ||
| assert!( | ||
| !layers.is_empty() && layers.iter().all(|layer| layer.active), | ||
| "SVC encoding must stay active after HIGH request, got {:?}", | ||
| layers | ||
| ); | ||
| time::sleep(Duration::from_millis(250)).await; | ||
| } | ||
|
|
||
| // --- Unsubscribe: with no subscribers left the encoding is deactivated --- | ||
| log::info!("dynacast svc test: unsubscribing"); | ||
| sub_publication.set_subscribed(false); | ||
|
|
||
| wait_for_layers(&pub_video_track, "svc unsubscribed", Duration::from_secs(30), |layers| { | ||
| !layers.is_empty() && layers.iter().all(|layer| !layer.active) | ||
| }) | ||
| .await?; | ||
|
|
||
| // --- Resubscribe: the encoding comes back --- | ||
| log::info!("dynacast svc test: resubscribing"); | ||
| sub_publication.set_subscribed(true); | ||
|
|
||
| wait_for_layers(&pub_video_track, "svc resubscribed", Duration::from_secs(30), |layers| { | ||
| !layers.is_empty() && layers.iter().all(|layer| layer.active) | ||
| }) | ||
| .await?; | ||
|
|
||
| Ok(()) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion(non-blocking): Consider breaking this out into a
consthelper function.