-
Notifications
You must be signed in to change notification settings - Fork 213
add option to add simuclcast layers in trackpublishoptions #1322
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 | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,7 +20,7 @@ | |||||||||||||||||||||
| }, | ||||||||||||||||||||||
| options::{ | ||||||||||||||||||||||
| AudioEncoding, DegradationPreference, FrameMetadataFeatures, TrackPublishOptions, | ||||||||||||||||||||||
| VideoEncoderBackend, VideoEncoding, | ||||||||||||||||||||||
| VideoEncoderBackend, VideoEncoding, VideoPreset, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| prelude::*, | ||||||||||||||||||||||
| webrtc::{ | ||||||||||||||||||||||
|
|
@@ -354,7 +354,11 @@ | |||||||||||||||||||||
| red: opts.red.unwrap_or(default_publish_options.red), | ||||||||||||||||||||||
| simulcast: opts.simulcast.unwrap_or(default_publish_options.simulcast), | ||||||||||||||||||||||
| stream: opts.stream.unwrap_or(default_publish_options.stream), | ||||||||||||||||||||||
| simulcast_layers: default_publish_options.simulcast_layers, | ||||||||||||||||||||||
| simulcast_layers: if opts.simulcast_layers.is_empty() { | ||||||||||||||||||||||
|
Check failure on line 357 in livekit-ffi/src/conversion/room.rs
|
||||||||||||||||||||||
| opts.default_publish_options.simulcast_layers | ||||||||||||||||||||||
|
Check failure on line 358 in livekit-ffi/src/conversion/room.rs
|
||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| Some(opts.simulcast_layers.into_iter().map(Into::into).collect()) | ||||||||||||||||||||||
|
Check failure on line 360 in livekit-ffi/src/conversion/room.rs
|
||||||||||||||||||||||
| }, | ||||||||||||||||||||||
|
Comment on lines
+357
to
+361
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. 🔴 Fallback to default simulcast layers reads from the wrong value When no simulcast layers are provided, the fallback reads the default layers off the incoming request instead of the defaults object ( Wrong receiver on the default lookup
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+357
to
+361
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. 🟡 Pull request is missing the required change documentation entry The change adds a new option without adding the required documentation entry for the release tooling, which the repository mandates for every pull request. AGENTS.md changeset requirementAGENTS.md states "Every PR needs a changeset" and that it must list crates to bump. This PR only touches Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||
| preconnect_buffer: opts | ||||||||||||||||||||||
| .preconnect_buffer | ||||||||||||||||||||||
| .unwrap_or(default_publish_options.preconnect_buffer), | ||||||||||||||||||||||
|
|
@@ -381,6 +385,12 @@ | |||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| impl From<proto::VideoPreset> for VideoPreset { | ||||||||||||||||||||||
| fn from(preset: proto::VideoPreset) -> Self { | ||||||||||||||||||||||
| Self { width: preset.width, height: preset.height, encoding: preset.encoding.into() } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| #[cfg(test)] | ||||||||||||||||||||||
| mod tests { | ||||||||||||||||||||||
| use livekit::options::{TrackPublishOptions, VideoEncoderBackend}; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
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.
🔴 Simulcast layers can never be sent because the option field was never added to the message definition
The new simulcast layer presets are read from the publish options message (
opts.simulcast_layersatlivekit-ffi/src/conversion/room.rs:357) even though no such field was added to that message definition, so clients have no way to actually supply simulcast layers.Impact: The advertised ability to choose simulcast layers when publishing a track does not work at all.
Missing repeated VideoPreset field in TrackPublishOptions
The proto change only adds the
VideoPresetmessage (livekit-ffi/protocol/room.proto:308-313);TrackPublishOptions(livekit-ffi/protocol/room.proto:315-336) still ends at field 13 with norepeated VideoPreset simulcast_layers = 14;. Since the Rust structs are generated from this proto at build time (livekit-ffi/src/proto.rs:18),opts.simulcast_layersdoes not exist and the FFI cannot receive layer presets.Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.