Conversation
Changeset ✓This PR includes a changeset covering all affected packages:
|
| } | ||
|
|
||
| #[derive(uniffi::Object)] | ||
| /// New resampler using SoX (much better quality) |
There was a problem hiding this comment.
This duplicates the comment in the .proto file. It gets copied into the generated foreign bindings.
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 3 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| #[derive(uniffi::Enum)] | ||
| // TODO(theomonnom): support other datatypes (shouldn't really be needed) | ||
| pub enum SoxResamplerDataType { | ||
| Interleaved, | ||
| Split, |
There was a problem hiding this comment.
🔴 Split-channel mode dereferences audio bytes
Selecting Split makes push pass a flat sample slice where soxr requires channel pointers. Soxr dereferences sample bytes as addresses, causing memory faults.
Learn more
A split soxr datatype means each I/O argument is an array containing one buffer pointer per channel. The exported push API can supply only a contiguous i16 slice, while the inner call passes that slice directly to soxr. Soxr casts this address to soxr_cbufs_t and dereferences each entry when split input is selected. Split output is equally incompatible with the single Vec<i16> output buffer.
Example: For stereo split input, a caller passes [left0, left1, right0, right1]. Soxr reads the first machine word of those samples as the left-channel pointer and dereferences it, rather than reading left0.
Recommended fix: Remove Split from this API until it accepts and owns per-channel buffers, or add separate split-I/O methods that construct stable arrays of channel pointers for both input and output.
Was this helpful? React with 👍 or 👎 to provide feedback.
c16010b to
bd19d0b
Compare
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 2 new potential issues.
4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| pub enum SoxQualityRecipe { | ||
| Quick = 0, | ||
| Low = 1, | ||
| Medium = 2, | ||
| High = 3, | ||
| VeryHigh = 4, | ||
| } |
There was a problem hiding this comment.
🟡 Quality recipes select lower modes
Selecting High or VeryHigh passes 3 or 4 to SoX. SoX constants define those recipes as 4 and 6.
Learn more
The enum discriminants are cast directly to c_ulong in SoxResamplerInner::new. Therefore, these numbers are SoX recipe constants rather than ordinary ordinal values. Quick, Low, and Medium happen to use 0, 1, and 2, but the remaining SoX constants are non-contiguous.
Example: A caller selecting VeryHigh sends recipe 4. SoX interprets 4 as SOXR_20_BITQ (High), while Very High is SOXR_28_BITQ at recipe 6.
Recommended fix: Define the variants from the corresponding soxr_sys constants. Keep protobuf conversion explicit so each protobuf name maps to the matching semantic variant.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ) | ||
| }; |
There was a problem hiding this comment.
🟨 Unvalidated audio pointer reaches unsafe slice
A malformed push request can supply a null or misaligned data_ptr with nonzero size. slice::from_raw_parts then invokes undefined behavior.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
Exercise SoxResampler both of the ways the migration has to keep working — directly, and through encoded FFI requests — and compare the two against each other, so the same tests can be re-run after it. Nothing outside the tests changes here.
05a42d4 to
adbcff2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…sh_ffi The exported push and flush return their output by value, so the pointer the FFI response hands the client would aim at a buffer freed while the response is built. Give the request surface its own pair of methods that return the lock along with a slice of out_buf, which is what it read before: the samples stay put until the client's next call. They go with the rest of the FFI handle plumbing once the migration is done. Caught by the protobuf migration tests, which abort on the dangling pointer.
adbcff2 to
534093e
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| let handle_id = *self.handle_id.get_or_init(|| crate::FFI_SERVER.next_id()); | ||
| // the FFI side co-owns from here; released by livekit_ffi_drop_handle | ||
| crate::FFI_SERVER.store_handle(handle_id, ::std::sync::Arc::clone(&self)); |
There was a problem hiding this comment.
🟡 Concurrent handle transfer can be undone
When ffi_handle_id overlaps take_ffi_handle_id, its delayed store_handle can reinsert a handle after the transfer succeeds. On first publication, the transfer can also observe the ID before its handle exists and fail spuriously.
Learn more
The handle ID and the FFI server entry jointly represent whether the object has published FFI ownership. OnceLock now publishes the ID before store_handle creates that ownership, so another thread can observe a half-published object. A later publication can also race after take_ffi_handle_id removes the entry and recreate ownership after the transfer reports success. The previous initialization closure made first publication atomic from observers of handle_id, but supporting republication also requires synchronization with removal.
Example: Thread A calls ffi_handle_id, reads the initialized ID, and pauses before store_handle. Thread B calls take_ffi_handle_id, removes the current entry, and receives the object successfully. Thread A resumes and reinserts the object, so the FFI server remains a co-owner despite the successful transfer.
Recommended fix: Serialize publication and transfer for each migrated object. Keep ID initialization and the first insertion indivisible, and use the same synchronization around later store_handle and take_handle operations so a successful take cannot be followed by an older publication.
Was this helpful? React with 👍 or 👎 to provide feedback.
This PR mechanically refactors the
SoxResamplerFFI, to introduce uniffi bindings, and interoperability at the foreign language side to be able to get the same objects from both FFI and uniffi generated code.This allows a gradual migration, going at different speeds in different SDKs.
The commits are well ordered:
SoxResamplerto uniffi (barely touching the FFIrequests.rsfileNewSoxResamplermessage's params.