add option to add simuclcast layers in trackpublishoptions - #1322
add option to add simuclcast layers in trackpublishoptions#1322nitch193 wants to merge 1 commit into
Conversation
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // A video preset describing a simulcast layer. | ||
| message VideoPreset { | ||
| required uint32 width = 1; | ||
| required uint32 height = 2; | ||
| required VideoEncoding encoding = 3; | ||
| } |
There was a problem hiding this comment.
🔴 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_layers at livekit-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 VideoPreset message (livekit-ffi/protocol/room.proto:308-313); TrackPublishOptions (livekit-ffi/protocol/room.proto:315-336) still ends at field 13 with no repeated VideoPreset simulcast_layers = 14;. Since the Rust structs are generated from this proto at build time (livekit-ffi/src/proto.rs:18), opts.simulcast_layers does not exist and the FFI cannot receive layer presets.
Prompt for agents
The PR adds a VideoPreset proto message and consumes opts.simulcast_layers in livekit-ffi/src/conversion/room.rs, but TrackPublishOptions in livekit-ffi/protocol/room.proto was never given a corresponding field. Add a new repeated VideoPreset simulcast_layers field with the next unused field number (14) to TrackPublishOptions so the conversion has something to read, and verify prost-generated types line up with the conversion code (note prost models proto2 required message fields as Option, so proto::VideoPreset::encoding will likely need handling for the None case).
Was this helpful? React with 👍 or 👎 to provide feedback.
| simulcast_layers: if opts.simulcast_layers.is_empty() { | ||
| opts.default_publish_options.simulcast_layers | ||
| } else { | ||
| Some(opts.simulcast_layers.into_iter().map(Into::into).collect()) | ||
| }, |
There was a problem hiding this comment.
🔴 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 (opts.default_publish_options.simulcast_layers at livekit-ffi/src/conversion/room.rs:358), so the default simulcast configuration cannot be resolved.
Impact: Publishing without explicit simulcast layers cannot fall back to the SDK defaults.
Wrong receiver on the default lookup
default_publish_options is a local binding created at livekit-ffi/src/conversion/room.rs:338 (TrackPublishOptions::default()); it is not a field of the proto request opts. Every other branch in this From impl uses the local (e.g. livekit-ffi/src/conversion/room.rs:353-356).
| simulcast_layers: if opts.simulcast_layers.is_empty() { | |
| opts.default_publish_options.simulcast_layers | |
| } else { | |
| Some(opts.simulcast_layers.into_iter().map(Into::into).collect()) | |
| }, | |
| simulcast_layers: if opts.simulcast_layers.is_empty() { | |
| default_publish_options.simulcast_layers | |
| } else { | |
| Some(opts.simulcast_layers.into_iter().map(Into::into).collect()) | |
| }, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| simulcast_layers: if opts.simulcast_layers.is_empty() { | ||
| opts.default_publish_options.simulcast_layers | ||
| } else { | ||
| Some(opts.simulcast_layers.into_iter().map(Into::into).collect()) | ||
| }, |
There was a problem hiding this comment.
🟡 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.
Impact: The release notes and crate version bumps will not reflect this change.
AGENTS.md changeset requirement
AGENTS.md states "Every PR needs a changeset" and that it must list crates to bump. This PR only touches livekit-ffi/protocol/room.proto and livekit-ffi/src/conversion/room.rs; no file was added under /.changeset.
Prompt for agents
Per AGENTS.md, every PR requires a changeset under /.changeset documenting the change and listing crates that need version bumps (here livekit-ffi, and livekit if public API changes). Create one with `knope document-change` or manually.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Before you submit your PR
Make sure the following is true before submitting your PR:
PR description
Helps to add simulcast layers in livkit-ffi in trackpublishoptions during publishing of track, Fixes livekit/client-sdk-unity#176
Breaking changes
no breaking change
MSRV
no