Example of exposing a livekit-ffi feature over uniffi - sox resampler - #1397
Example of exposing a livekit-ffi feature over uniffi - sox resampler#13971egoman wants to merge 2 commits into
Conversation
More about why this is being done in the AGENTS.md, or here: 6e31b06
This exposes the sox resampler over uniffi to provide an idea to others what a potential livekit-ffi interface over uniffi could look like as part of the migration.
No changeset foundThis PR modifies versioned packages but doesn't include a changeset. The following packages require a version bump:
A package must be bumped when its own files change, and whenever a package it depends on is bumped (so downstream consumers get a matching release). Click here to create a changeset for the missing packages The link pre-populates a changeset file with If this change doesn't require a version bump, add the |
| /// A wrapper over an [`FfiHandleId`] whose state lives in the [`FFI_SERVER`] | ||
| /// handle map rather than in the wrapper itself. | ||
| /// | ||
| /// This is what lets a UniFFI object and the legacy C ABI operate on one shared | ||
| /// value: the UniFFI object stores only the id, and resolves [`Self::Inner`] out | ||
| /// of the handle store on every call, exactly as the protobuf request handlers | ||
| /// in [`crate::server::requests`] do. | ||
| /// | ||
| /// # Ownership | ||
| /// | ||
| /// Ownership is RAII. [`Self::from_ffi_handle_id`] *adopts* a handle, and | ||
| /// implementors are expected to `impl Drop` and call | ||
| /// `FFI_SERVER.drop_handle(self.ffi_handle_id())` there. Rust cannot make `Drop` | ||
| /// part of a trait contract, so that is a convention this trait documents rather | ||
| /// than enforces. | ||
| /// | ||
| /// Two escape hatches exist, and the distinction between them matters: | ||
| /// | ||
| /// * [`Self::ffi_handle_id`] borrows the id. The caller may use it to address the | ||
| /// same state over the legacy C ABI, but must **not** free it — this value is | ||
| /// still the owner. | ||
| /// * [`Self::leak_ffi_handle_id`] consumes `self` and hands ownership out, so the | ||
| /// handle survives and the caller becomes responsible for freeing it (via | ||
| /// `livekit_ffi_drop_handle`). | ||
| pub trait BackedByFfiHandle: Sized { | ||
| /// The type actually stored in the handle map. | ||
| /// | ||
| /// For the two surfaces to share state this must be the *identical* type the | ||
| /// legacy handlers pass to [`crate::server::FfiServer::retrieve_handle`] — | ||
| /// that lookup is a downcast, so a merely structurally-similar type resolves | ||
| /// to `FfiError::InvalidRequest("handle is not a ...")`. | ||
| type Inner: FfiHandle + Clone; | ||
|
|
||
| /// Adopt an existing handle. | ||
| /// | ||
| /// This *takes ownership*: dropping the returned value drops the handle. | ||
| fn from_ffi_handle_id(ffi_handle_id: FfiHandleId) -> Self; | ||
|
|
||
| /// Borrow the handle id. Does **not** transfer ownership. | ||
| fn ffi_handle_id(&self) -> FfiHandleId; | ||
|
|
||
| /// Consume `self` and return the handle id *without* dropping the handle. | ||
| /// | ||
| /// Use this to move ownership back to the legacy C ABI. The caller must | ||
| /// eventually free the handle. | ||
| fn leak_ffi_handle_id(self) -> FfiHandleId { | ||
| // Suppress the implementor's `Drop` (and so its `drop_handle` call) | ||
| // while still reading the id out. | ||
| let this = ManuallyDrop::new(self); | ||
| this.ffi_handle_id() | ||
| } | ||
|
|
||
| /// Given an instance of [Self::Inner], stores the object into the handle map | ||
| /// and returns a new instance of the [BackedByFfiHandle] wrapper type. | ||
| fn from_inner(inner: Self::Inner) -> Self { | ||
| let handle_id = FFI_SERVER.next_id(); | ||
| FFI_SERVER.store_handle(handle_id, inner); | ||
| Self::from_ffi_handle_id(handle_id) | ||
| } | ||
|
|
||
| /// Resolve the backing value out of the handle store. | ||
| /// | ||
| /// The clone releases the dashmap shard's read guard before returning, so a | ||
| /// caller that then locks an inner mutex is not holding two locks at once. | ||
| /// This mirrors the `retrieve_handle(..)?.clone()` idiom in | ||
| /// [`crate::server::requests`]. | ||
| fn inner(&self) -> FfiResult<Self::Inner> { | ||
| FFI_SERVER | ||
| .retrieve_handle::<Self::Inner>(self.ffi_handle_id()) | ||
| .map(|handle| Self::Inner::clone(&handle)) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is worth reading - I am proposing all new uniffi livekit-ffi objects would implement this trait. It's a in essence a lot like the existing FfiHandle type.
| impl BackedByFfiHandle for SoxResampler { | ||
| /// Identical to the type `on_push_sox_resampler` / `on_flush_sox_resampler` | ||
| /// look up, which is what makes the two surfaces share one resampler. | ||
| type Inner = Arc<Mutex<resampler::SoxResampler>>; | ||
|
|
||
| fn from_ffi_handle_id(ffi_handle_id: FfiHandleId) -> Self { | ||
| Self { handle_id: ffi_handle_id } | ||
| } | ||
|
|
||
| fn ffi_handle_id(&self) -> FfiHandleId { | ||
| self.handle_id | ||
| } | ||
| } | ||
|
|
||
| impl Drop for SoxResampler { | ||
| fn drop(&mut self) { | ||
| FFI_SERVER.drop_handle(self.handle_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
Here's what that trait implementation looks like. Note that Inner must EXACTLY match the type in the FfiServer handle map.
This trait then provides self.inner() which all downstream methods can use to get the inner object state. In the future in a later migration step, we can change how BackedByFfiHandle works to move away from FFI_SERVER and there should be largely no changes needed in the code of the implementers.
One unfortunate thing - I couldn't figure out a good way to automatically derive Drop, and it's the one place that FFI_SERVER leaks through into this implementation. It's because rust doesn't allow impl<T> Drop for T or trait A: Drop, the Drop trait has some special semantics that seem to make this not possible. I can try to dig into this more if nobody else has any ideas but IMO I think the current state is probably fine.
There was a problem hiding this comment.
Devin Review found 4 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| /// Channels split across separate buffers (`SOXR_INT16_S`). | ||
| Int16Split, |
There was a problem hiding this comment.
🔴 Split sample layout crashes resampling
Selecting Int16Split passes one flat buffer where libsoxr requires channel pointers. Valid split input or output can crash or corrupt audio.
Prompt for agents
The UniFFI SoxResamplerDataType exposes Int16Split, but SoxResampler::push always constructs one flat &[i16], and server::resampler::SoxResampler always supplies one flat Vec<i16> as output. Libsoxr interprets SOXR_INT16_S input and output as arrays of per-channel pointers. Either remove split mode from this API until it is supported, or redesign push and output storage to marshal separate channel buffers correctly. Cover split input and split output with multi-channel tests.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ) | ||
| .map_err(|reason| SoxResamplerError::Create { reason })?; | ||
|
|
||
| Ok(Self::from_inner(Arc::new(Mutex::new(inner)))) |
There was a problem hiding this comment.
🔴 Concurrent resamplers can crash
Concurrent push calls on separate objects bypass each per-object Mutex and race in libsoxr's global FFT cache. First use can crash the process.
Prompt for agents
The UniFFI objects synchronize only each individual resampler, while this repository's libsoxr build omits _OPENMP and therefore leaves its process-global FFT cache locks as no-ops. The test comments document that concurrent use of separate resamplers can segfault. Add process-wide synchronization around every libsoxr operation that can touch shared state, including calls through both the legacy request handlers and the new UniFFI methods, so the two surfaces cannot race each other.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Yes, this is a known issue unrelated to this PR. The existing c abi ffi suffers from this problem too.
| #[uniffi::constructor] | ||
| pub fn from_handle(handle_id: u64) -> Self { | ||
| Self::from_ffi_handle_id(handle_id) | ||
| } |
There was a problem hiding this comment.
🟡 Wrong handles delete unrelated resources
from_handle accepts another resource's ID without validation. Dropping the wrapper deletes that resource, even after push reports InvalidHandle.
Prompt for agents
SoxResampler::from_handle currently returns Self for every u64, making the wrapper an unconditional owner. Validate the handle against Arc<Mutex<server::resampler::SoxResampler>> before adopting it and return Result<Self, SoxResamplerError> on missing or mistyped IDs. Ensure failed adoption never calls drop_handle for the supplied ID, and update the mistyped-handle test to verify the original resource remains stored.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Yes, also a "known issue" - the existing ffi handle map approach is pretty loose and there's no way to really validate this with how it currently works.
| pub fn push(&self, data_ptr: u64, size: u32) -> Result<SoxResamplerOutput, SoxResamplerError> { | ||
| let data = unsafe { | ||
| slice::from_raw_parts(data_ptr as *const i16, size as usize / size_of::<i16>()) | ||
| }; |
There was a problem hiding this comment.
Yes, also a "known issue", the goal was to keep the interface as similar as possible to the old c abi ffi.
I've discussed with a few folks that in order to move along with the livekit-ffi migration, we need an example of how potentially we could migrate livekit-ffi related features to uniffi. To date this has not been super clear - we need to figure out how to do this in a backwards compatible way so that we can start this migration, work on it slowly in the background, and not force a "hard cutover" until everything upstream has migrated.
This pull request is roughly what I have in mind. At a 10k foot view, the idea is to start by adding uniffi objects which read and write the same
FfiServermanaged state from the handle map. Because the state is in the handle map, it still works with all legacy c abi ffi methods. Any uniffi object can be converted on the fly to a c abi ffi handle with::leak_ffi_handle_id, or a new uniffi object can be constructed from a c abi handle via::from_ffi_handle_id.Through these mechanisms, an existing sdk can migrate slowly, ie:
I ported the
SoxResamplerhere because it is a very small, isolated piece which can be tested in isolation. The goal next would be to try this with a small piece ofRoomfunctionality to be 100% sure all these patterns transfer.