diff --git a/crates/oakaudio/src/lib.rs b/crates/oakaudio/src/lib.rs index a7d781126..f92ae7cc6 100644 --- a/crates/oakaudio/src/lib.rs +++ b/crates/oakaudio/src/lib.rs @@ -20,12 +20,12 @@ //! (`include/audio/*.h`). See README.md for the architectural mapping //! (singleton manager, stateless sync helpers, local value types). //! -//! ## FFI discipline +//! ## Structure //! -//! Identical to the oaknode crate: every export goes through -//! [`handle::guard*`], handles are opaque refcounted boxes (or, for the -//! singleton `AudioManager`, borrow-only no-ops), shared state behind -//! `Mutex`. +//! Single-lib unification: the module crates are called directly from the +//! oakengine facade (`crates/oakengine`), which owns the C-ABI shims and +//! boxes the singleton [`manager::ManagerInner`] behind its own `CHandle`. +//! Shared state lives behind `Mutex`. #![deny(unsafe_op_in_unsafe_fn)] #![warn(missing_docs)] diff --git a/crates/oakcodec/README.md b/crates/oakcodec/README.md index a61c3af6d..15f7d8926 100644 --- a/crates/oakcodec/README.md +++ b/crates/oakcodec/README.md @@ -1,13 +1,17 @@ # oakcodec Rust crate -> Status: **implemented**. Implements `include/codec/*.h` verbatim -> (`src/ffi/`); every export has success + failure-path tests -> (`cargo test`: unit tests in `src/ffi/*.rs`, the contract tests in -> `tests/`, and real-media tests in `src/realmedia_tests.rs`). The FFmpeg -> engine is fully implemented through the [`ffmpeg-next`] crate (decode, -> probe, audio conform, encode); the OIIO engine remains a stub in this -> build. This crate mirrors the `crates/oaknode/` template (same FFI -> discipline, same testing layers). +> Status: **implemented**. Implements the `include/codec/*.h` contract; +> every function has success + failure-path tests (`cargo test`: unit +> tests in `src/`, the contract tests in `tests/`, and real-media tests +> in `src/realmedia_tests.rs`). The FFmpeg engine is fully implemented +> through the [`ffmpeg-next`] crate (decode, probe, audio conform, +> encode); the OIIO engine remains a stub in this build. +> +> Single-lib unification (M14, `docs/zh/plans/riir/single-lib.md`): the +> C-ABI export layer (`src/ffi.rs`) and the module-crossing bridge +> (`src/bridge/`) are gone. Other module crates and the oakengine facade +> call this crate's modules directly; the facade (`crates/oakengine`) +> serves the frozen `include/codec/*.h` functions. ## Scope @@ -18,20 +22,18 @@ conform and proxy generation managers, export format/codec tables, encoding parameters, and the background-task submit hook. Public contract: `include/codec/*.h` (7 headers: frame.h, decoder.h, -encoder.h, conform.h, proxy.h, task.h, error.h) — frozen, implemented -verbatim by `src/ffi.rs`. Interim state (pre-M8) is documented in +encoder.h, conform.h, proxy.h, task.h, error.h) — frozen, served by the +oakengine facade. Interim state (pre-M8) is documented in `src/codec/NOTES.md`: conform/proxy work is delegated to the global task submit callback and otherwise reports unavailable, never crashes and never blocks. ## Key architectural decisions (C++ → Rust mapping) -1. **`shared_ptr` → refcounted `RefBox` handle.** The C++ `Frame`/ - `Decoder`/`Encoder` objects are heap boxes behind the neutral - by-value handle struct `{ctx, addref, release, abi_version}` (see - `handle.rs`), exactly as oaknode/oakplugin do. Handles are - deliberately duplicated per module: the function pointers always - point into the DLL that created the object. +1. **`shared_ptr` → `Arc`.** The C++ `Frame`/`Decoder`/`Encoder` + objects are handed around as plain Rust values (`Frame`) or + `Arc` / `Arc`. No refcounted C-handle + scaffolding remains (the former `handle.rs` was deleted in M14 R5). 2. **Inheritance → traits.** The C++ `Decoder`/`Encoder` abstract bases plus their FFmpeg/OIIO subclasses become a Rust trait with two implementors. The probe/dispatch (decide which implementation @@ -39,15 +41,14 @@ and never blocks. `PixelFormat`/`SampleFormat` support is a trait query, not a virtual chain. 3. **`Frame` owns its params by value.** `olive::Frame` wraps an - `OakVideoParams` handle (an oakcommon by-value handle, NOT owned by - codec) plus a `Vec` pixel buffer. In Rust the params are held as - the oakcommon handle (refcounted through `bridge::common`) so the - byte-level ABI stays unchanged; the buffer is a plain `Vec`. -4. **No adapter layer.** Codec calls other modules' C ABIs directly - (`bridge/common.rs`, `bridge/render.rs`), keeping the 2026-08 + `OakVideoParams` handle plus a `Vec` pixel buffer. In Rust the + params are held as an `oakcommon::videoparams::VideoParams` value + (single-lib unification dropped the refcounted oakcommon handle); + the buffer is a plain `Vec`. +4. **No adapter layer.** Codec calls the other module crates directly + (`oakcommon`, `oakcore-rs`, `oakffmpeg-link`), keeping the 2026-08 decision recorded in NOTES.md §6. Only genuinely repeated - conversions survive as small module-local helpers (e.g. - `fill_render_params`, `cancel_atom_is_cancelled`). + conversions survive as small module-local helpers. 5. **XML stays on the C++ side.** `EncodingParams::load/save` use oakcommon's C++ `XmlStreamWriter/Reader` classes (`src/common/src/xmlutils.h`), exactly as oaknode/oakrender do — @@ -58,8 +59,8 @@ and never blocks. 7. **Enum values are the C contract.** `ExportFormat::Format`, `ExportCodec::Codec`, `Interlacing`, `VideoScalingMethod`, `SampleFormat::Format` all stay as the raw int values the C ABI - documents (oakengine/encoding.h), so `ffi.rs` marshals them without - translation. + documents (oakengine/encoding.h), so the facade marshals them + without translation. ## Layout @@ -67,8 +68,7 @@ and never blocks. src/ lib.rs crate doc + module map error.rs error codes (mirrors include/codec/error.h) - handle.rs refcounted-handle scaffolding (same pattern as node) - frame.rs Frame (CPU pixel buffer + OakVideoParams handle) + frame.rs Frame (CPU pixel buffer + VideoParams value) framemanager.rs FrameManager (buffer pool + background GC thread) decoder.rs Decoder trait + CodecStream + RenderMode + probe ffmpeg.rs FFmpegDecoder / FFmpegEncoder (ffmpeg-next) @@ -85,17 +85,16 @@ src/ footagedescription.rs FootageDescription (codec-internal stream desc) planarfiledevice.rs PlanarFileDevice (stdio plane-channel I/O) realmedia_tests.rs real-media tests (demo.mp4, H.264 round-trip) - bridge/ C ABI imports: common.rs, render.rs - ffi.rs include/codec/*.h export layer tests/ contract + golden tests (see test section below) ``` ## Hard rules for the implementer -1. Every `extern "C"` body goes through `handle::guard*`; no panic - crosses FFI. -2. The handle is the only way out of the crate; the public API never - hands out raw `&Frame`/`&Decoder` references. +1. No panics cross a module boundary: the facade wraps every call in + its panic-catching shims, and callback types stay `unsafe extern + "C"` with panic-free bodies. +2. Objects leave the crate only as Rust types (`Arc`, values, `&` + refs); raw handles exist solely inside the facade. 3. Behavior parity with C++ is proven by the unchanged C ABI test suite (`src/codec/tests`) plus the contract tests in `tests/`. 4. Where C++ behavior is genuinely load-bearing but ugly, port the diff --git a/crates/oakcodec/src/decoder.rs b/crates/oakcodec/src/decoder.rs index 9c19f477e..52251289a 100644 --- a/crates/oakcodec/src/decoder.rs +++ b/crates/oakcodec/src/decoder.rs @@ -32,15 +32,20 @@ use oakcore_rs::{Rational, TimeRange}; use crate::footagedescription::FootageDescription; use crate::frame::Frame; -/// `OakRenderTexture` — refcounted GPU texture handle (an oakrender type, -/// opaque to oakcodec). The codec crate cannot depend on oakrender (the -/// dependency cycle), so it only ever produces an empty handle — the -/// shared [`crate::handle::CHandle`] carries that value unchanged. -pub type OakRenderTexture = crate::handle::CHandle; +/// `OakRenderTexture` — GPU texture token (an oakrender type, opaque to +/// oakcodec). The codec crate cannot depend on oakrender (dependency +/// cycle), so it never constructs a real texture: [`Decoder::retrieve_video`] +/// only reports whether the decode succeeded and returns this unit token +/// (the former empty `CHandle`). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct OakRenderTexture; -/// `OakNodeBlock` — opaque node-block handle owned elsewhere; the codec -/// only stores and forwards it (borrowed, never dereferenced). -pub type OakNodeBlock = crate::handle::CHandle; +/// `OakNodeBlock` — opaque timeline-block token owned elsewhere; the codec +/// only stores and forwards it (borrowed, never dereferenced). No module +/// ever constructs one (the former `CHandle` was only ever `None`), so the +/// type is an empty marker kept for API compatibility. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct OakNodeBlock; /// `oakcodec_video_stream_info` — POD probe output describing one video /// stream; see `include/codec/decoder.h`. @@ -162,8 +167,8 @@ pub enum RetrieveState { /// `Decoder::CodecStream` — identifies one (filename, stream) pair plus an /// optional associated timeline block. /// -/// The block is an opaque `OakNodeBlock` handle that codec only stores and -/// compares, never dereferences or retains (borrowed pointer). +/// The block is an opaque [`OakNodeBlock`] token that codec only stores and +/// compares, never dereferences or retains (borrowed). #[derive(Clone, Debug, PartialEq, Eq)] pub struct CodecStream { filename: String, @@ -231,8 +236,7 @@ impl CodecStream { /// /// Implementations are [`crate::ffmpeg::FFmpegDecoder`] and /// [`crate::oiio::OIIODecoder`]. The trait surface mirrors the C++ -/// abstract base; the refcounted handle that backs the public API wraps an -/// `Arc`. +/// abstract base; the public API hands out `Arc` values. pub trait Decoder: Send + Sync { /// Unique decoder id ("ffmpeg"/"oiio"). fn id(&self) -> String; diff --git a/crates/oakcodec/src/encoder.rs b/crates/oakcodec/src/encoder.rs index dd2e36008..0ace7ac1e 100644 --- a/crates/oakcodec/src/encoder.rs +++ b/crates/oakcodec/src/encoder.rs @@ -29,7 +29,8 @@ use oakcore_rs::{PixelFormat, SampleFormat}; use crate::encodingparams::EncodingParams; use crate::frame::Frame; -/// `olive::Encoder` — encoder trait. Backs the refcounted encoder handle. +/// `olive::Encoder` — encoder trait. Instances are handed out as +/// `Arc`. pub trait Encoder: Send + Sync { /// Unique encoder id. fn id(&self) -> String; @@ -129,8 +130,8 @@ pub fn set_test_encoders(list: Vec>) { /// WebM, SRT → FFmpeg; OpenEXR, PNG, TIFF → OIIO; anything else → `None`). /// A non-empty test-injected list (see [`set_test_encoders`]) wins over the /// built-in mapping. The concrete implementations are dylib stubs whose -/// `open()` fails with a clear message, so an initialized encoder handle is -/// always constructible for a recognized format. +/// `open()` fails with a clear message, so an initialized encoder is always +/// constructible for a recognized format. pub fn create_from_params(params: &EncodingParams) -> Option> { if let Some(store) = TEST_ENCODERS.get() { let injected = store.lock().unwrap(); diff --git a/crates/oakcodec/src/error.rs b/crates/oakcodec/src/error.rs index 47abb4f57..b41aea58c 100644 --- a/crates/oakcodec/src/error.rs +++ b/crates/oakcodec/src/error.rs @@ -32,7 +32,8 @@ pub const OAKCODEC_E_NOMEM: i32 = -50005; /// The operation was cancelled. pub const OAKCODEC_E_CANCELLED: i32 = -50006; -/// Current ABI version stamped into every oakcodec handle. +/// The frozen C-ABI `abi_version` tag (`include/codec/decoder.h`); kept for +/// parity with the handle structs, no longer stamped at runtime. pub const OAKCODEC_ABI_VERSION: u32 = 1; /// Crate-internal result type; the FFI layer maps it to the codes. diff --git a/crates/oakcodec/src/ffmpeg.rs b/crates/oakcodec/src/ffmpeg.rs index eaff1268c..d30e2c8d9 100644 --- a/crates/oakcodec/src/ffmpeg.rs +++ b/crates/oakcodec/src/ffmpeg.rs @@ -38,8 +38,8 @@ //! path or loop mode). //! * [`RetrieveVideoParams`] drops `renderer`, `divider` and //! `maximum_format` (the Rust trait surface), so [`Decoder::retrieve_video`] -//! can only produce textures through an empty renderer handle and there -//! is no preview-divider scaling. +//! only reports decode success — it returns a unit [`OakRenderTexture`] +//! token, never a real texture — and there is no preview-divider scaling. //! * Probing uses stream parameters (no second decode pass), so `is_still` //! is always false and interlacing always progressive. //! * Subtitle streams are counted but not added as subtitle entries. @@ -325,12 +325,7 @@ impl Decoder for FFmpegDecoder { return Err(fail("decoder is not open on a video stream")); } let _ = state.retrieve_frame(&p.time, p.time == crate::decoder::k_any_timecode(), None)?; - Ok(OakRenderTexture { - ctx: std::ptr::null_mut(), - addref: None, - release: None, - abi_version: crate::handle::OAKCODEC_ABI_VERSION, - }) + Ok(OakRenderTexture) } fn retrieve_audio( diff --git a/crates/oakcodec/src/handle.rs b/crates/oakcodec/src/handle.rs deleted file mode 100644 index 945f05225..000000000 --- a/crates/oakcodec/src/handle.rs +++ /dev/null @@ -1,281 +0,0 @@ -// Oak Video Editor - Non-Linear Video Editor -// Copyright (C) 2026 Oak Team -// -// This program is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. -// -// This program is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. -// -// You should have received a copy of the GNU General Public License -// along with this program. If not, see . - -//! Refcounted-handle scaffolding. Same pattern as the oaknode/oakplugin -//! crates (`src/node/rust/src/handle.rs`); intentionally duplicated rather -//! than shared — each module DLL must run its own addref/release code -//! (the function pointers in a handle always point into the DLL that -//! created the object). - -use std::panic::{catch_unwind, AssertUnwindSafe}; -use std::sync::atomic::{AtomicI32, AtomicU32, Ordering}; -use crate::error::{self, OAKCODEC_E_FAILED}; - -/// Number of boxed handle objects currently alive (leak/debug checking). -/// -/// Mirrors `oakcodec::g_alive_count` in `src/codec/c_api/frame.cpp`: every -/// `make_owned` box increments it and `box_release` decrements it when the -/// last reference drops. `oakcodec_debug_alive_count` reports it. -static ALIVE: AtomicI32 = AtomicI32::new(0); - -/// ABI version stamped into every handle. -pub const OAKCODEC_ABI_VERSION: u32 = 1; - -/// Heap box behind a handle's `ctx`. -pub struct RefBox { - /// Atomic reference count. - pub refs: AtomicU32, - /// Boxed value. - pub value: T, -} - -/// `#[repr(C)]` mirror of the public handle structs -/// (`{ctx, addref, release, abi_version}`). -/// The shared ABI value-handle type (single-lib unification, see -/// `docs/zh/plans/riir/single-lib.md`): one canonical -/// `{ctx, addref, release, abi_version}` type in `oakcore-rs`, re-exported -/// here so the crate's `ffi.rs` signatures and handle scaffolding stay -/// source-compatible. -pub use oakcore_rs::handle::CHandle; - -/// Increment the reference count of a boxed `RefBox`. -/// -/// # Safety -/// `ptr` must point to a live `RefBox` previously created by this module. -unsafe extern "C" fn box_addref(ptr: *mut std::ffi::c_void) { - if ptr.is_null() { - return; - } - let boxed = unsafe { &*(ptr as *const RefBox) }; - boxed.refs.fetch_add(1, Ordering::SeqCst); -} - -/// Decrement the reference count; destroys the box at zero. -/// -/// # Safety -/// `ptr` must point to a live `RefBox` previously created by this module. -unsafe extern "C" fn box_release(ptr: *mut std::ffi::c_void) { - if ptr.is_null() { - return; - } - let boxed = unsafe { &*(ptr as *const RefBox) }; - if boxed.refs.fetch_sub(1, Ordering::SeqCst) == 1 { - // The last reference: the box is destroyed and the alive count - // drops with it (mirrors `alive_dec` in c_api/frame.cpp). - ALIVE.fetch_sub(1, Ordering::SeqCst); - unsafe { drop(Box::from_raw(ptr as *mut RefBox)) }; - } -} - -/// Owned handle with count 1; empty on allocation failure. -pub fn make_owned(value: T) -> CHandle { - let boxed = Box::new(RefBox { - refs: AtomicU32::new(1), - value, - }); - let ctx = Box::into_raw(boxed) as *mut std::ffi::c_void; - // Every boxed handle counts toward `oakcodec_debug_alive_count` - // (mirrors `alive_inc` in c_api/frame.cpp). - ALIVE.fetch_add(1, Ordering::SeqCst); - CHandle { - ctx, - addref: Some(box_addref::), - release: Some(box_release::), - abi_version: OAKCODEC_ABI_VERSION, - } -} - -/// Borrowed handle for an object owned elsewhere. -/// -/// Takes ownership of the boxed `T` already allocated at `ptr` (e.g. one -/// passed in from C++). The resulting handle's release drops that box. -/// -/// # Safety -/// Caller guarantees `ptr` was allocated with `Box::new` and is not used -/// after this call. -pub unsafe fn make_borrowed(ptr: *mut T) -> CHandle { - if ptr.is_null() { - return CHandle::null(); - } - // Move ownership into a RefBox so addref/release and get() behave - // uniformly with owned handles. - let value = unsafe { *Box::from_raw(ptr) }; - make_owned(value) -} - -/// Typed view into a handle; `None` for empty handles. -/// -/// # Safety -/// `T` must be the boxed type. -pub unsafe fn get(h: &CHandle) -> Option<&T> { - if h.is_null() { - return None; - } - let boxed = unsafe { &*(h.ctx as *const RefBox) }; - Some(&boxed.value) -} - -/// Panic-catching FFI wrapper for i32-returning exports. -pub fn guard error::Result<()>>(f: F) -> i32 { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(())) => error::OAKCODEC_OK, - Ok(Err(e)) => e.code(), - Err(_) => OAKCODEC_E_FAILED, - } -} - -/// Panic-catching FFI wrapper for handle-returning exports. -pub fn guard_handle error::Result>(f: F) -> CHandle { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(h)) => h, - Ok(Err(_)) | Err(_) => CHandle::null(), - } -} - -/// Panic-catching FFI wrapper for void exports. -pub fn guard_void(f: F) { - let _ = catch_unwind(AssertUnwindSafe(f)); -} - -/// Panic-catching FFI wrapper for exports that return a raw `i32` code -/// directly (neither `Result` nor a handle). On panic, `OAKCODEC_E_FAILED`. -pub fn guard_raw i32>(f: F) -> i32 { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(code) => code, - Err(_) => OAKCODEC_E_FAILED, - } -} - -/// Panic-catching FFI wrapper for exports that return a raw `i64` directly -/// (e.g. `oakcodec_decoder_get_image_sequence_index`). On panic, -/// `OAKCODEC_E_FAILED`. -pub fn guard_i64 i64>(f: F) -> i64 { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(v) => v, - Err(_) => OAKCODEC_E_FAILED as i64, - } -} - -/// Number of live boxed handle objects (see [`ALIVE`]). -pub fn alive_count() -> i32 { - ALIVE.load(Ordering::SeqCst) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::error::OAKCODEC_E_INVALID; - - #[test] - fn make_owned_lifecycle_tracks_alive_count() { - // The shared test lock serializes the crate's `alive_count` - // assertions against every other test that creates handles. - let _g = crate::lock_tests(); - let before = alive_count(); - let h = make_owned(42u32); - assert!(!h.is_null()); - assert_eq!(alive_count(), before + 1); - - // addref/release cycle keeps the box alive. - let addref = h.addref.unwrap(); - let release = h.release.unwrap(); - // SAFETY: `h.ctx` is a live RefBox. - unsafe { addref(h.ctx) }; - // SAFETY: second reference released; box stays (refs 2 -> 1). - unsafe { release(h.ctx) }; - assert_eq!(alive_count(), before + 1); - - // Release the owned reference: box destroyed. - // SAFETY: last reference. - unsafe { release(h.ctx) }; - assert_eq!(alive_count(), before); - } - - #[test] - fn make_borrowed_null_is_null_handle() { - let h = unsafe { make_borrowed::(std::ptr::null_mut()) }; - assert!(h.is_null()); - } - - #[test] - fn make_borrowed_takes_ownership() { - let _g = crate::lock_tests(); - let before = alive_count(); - let raw = Box::into_raw(Box::new(7u32)); - let h = unsafe { make_borrowed(raw) }; - assert!(!h.is_null()); - assert_eq!(alive_count(), before + 1); - assert_eq!(unsafe { *get::(&h).unwrap() }, 7); - unsafe { h.release.unwrap()(h.ctx) }; - assert_eq!(alive_count(), before); - } - - #[test] - fn addref_on_null_ctx_is_noop() { - // A handle with function pointers but a null ctx: both thunks no-op. - let h = CHandle { - ctx: std::ptr::null_mut(), - addref: Some(box_addref::), - release: Some(box_release::), - abi_version: OAKCODEC_ABI_VERSION, - }; - // SAFETY: ctx is null; the thunks guard on it. - unsafe { h.addref.unwrap()(h.ctx) }; - // SAFETY: ctx is null; the thunks guard on it. - unsafe { h.release.unwrap()(h.ctx) }; - } - - #[test] - fn guard_maps_results_and_panics() { - assert_eq!(guard(|| Ok(())), crate::error::OAKCODEC_OK); - assert_eq!( - guard(|| Err(crate::error::Error::Invalid)), - OAKCODEC_E_INVALID - ); - assert_eq!(guard(|| panic!("boom")), crate::error::OAKCODEC_E_FAILED); - - let ok = guard_handle(|| Ok(make_owned(1u32))); - assert!(!ok.is_null()); - assert!(guard_handle(|| Err::(crate::error::Error::Invalid)).is_null()); - assert!(guard_handle(|| panic!("boom")).is_null()); - - assert_eq!(guard_raw(|| 5), 5); - assert_eq!( - guard_raw(|| panic!("boom")), - crate::error::OAKCODEC_E_FAILED - ); - - assert_eq!(guard_i64(|| 5), 5); - assert_eq!( - guard_i64(|| panic!("boom")), - crate::error::OAKCODEC_E_FAILED as i64 - ); - - let mut called = false; - guard_void(|| called = true); - assert!(called); - guard_void(|| panic!("boom")); - } - - #[test] - fn null_handle_helpers() { - let h = CHandle::null(); - assert!(h.is_null()); - // The shared `null()` stamps no ABI version (single-lib - // unification; `make_owned` stamps the crate version). - assert_eq!(h.abi_version, 0); - } -} diff --git a/crates/oakcodec/src/lib.rs b/crates/oakcodec/src/lib.rs index ad753885d..ac8548942 100644 --- a/crates/oakcodec/src/lib.rs +++ b/crates/oakcodec/src/lib.rs @@ -18,13 +18,15 @@ //! //! Reimplements the C++ oakcodec module behind its frozen C ABI //! (`include/codec/*.h`). See README.md for the architectural mapping -//! (inheritance → traits, shared_ptr → refcounted handles, etc.). +//! (inheritance → traits, shared_ptr → `Arc`, etc.). //! -//! ## FFI discipline +//! ## Structure //! -//! Identical to the oaknode/oakplugin crates: every export goes through -//! [`handle::guard*`], handles are opaque refcounted boxes, shared -//! state behind `Mutex`. +//! Single-lib unification: the module crates are called directly from +//! other module crates and the oakengine facade (`crates/oakengine`), so +//! no C-ABI export layer remains here (the `include/codec/*.h` contracts +//! are served by the facade). Shared state lives behind `Mutex`, decoder +//! instances behind `Arc`. #![deny(unsafe_op_in_unsafe_fn)] #![warn(missing_docs)] @@ -44,7 +46,6 @@ pub mod ffmpeg; pub mod footagedescription; pub mod frame; pub mod framemanager; -pub mod handle; pub mod oiio; pub mod oiioframebridge; pub mod planarfiledevice; @@ -57,9 +58,9 @@ pub mod timecodemetadata; mod realmedia_tests; /// Process-wide test lock: serializes every test that reads or mutates -/// crate-global state (the injected decoder registry, the handle alive -/// count). One lock for the whole crate — tests race only with each -/// other, never with production code. +/// crate-global state (the injected decoder/encoder registries). One lock +/// for the whole crate — tests race only with each other, never with +/// production code. #[cfg(test)] static TEST_LOCK: Mutex<()> = Mutex::new(()); diff --git a/crates/oakcodec/tests/ffi_contract_test.rs b/crates/oakcodec/tests/ffi_contract_test.rs index 7f456af42..adf5aa5fd 100644 --- a/crates/oakcodec/tests/ffi_contract_test.rs +++ b/crates/oakcodec/tests/ffi_contract_test.rs @@ -30,8 +30,9 @@ /// stamped). Covers frame/decoder/encoder/conform/proxy `init` /// families. /// -/// Covered in `src/ffi/frame.rs` / `decoder.rs` / `encoder.rs` unit -/// tests (`handle::alive_count` tracks the boxed-object count). +/// Covered in the (deleted) `src/ffi/*.rs` unit tests; the C-ABI entry +/// points now live in the oakengine facade, whose own tests assert the +/// same contract. #[test] fn handle_contract_all_exports() { // No-op — see the module doc. diff --git a/crates/oakengine/src/stubs.rs b/crates/oakengine/src/stubs.rs index 41d3cafdd..12865e473 100644 --- a/crates/oakengine/src/stubs.rs +++ b/crates/oakengine/src/stubs.rs @@ -10622,11 +10622,11 @@ pub mod timeline { } // SAFETY: the caller guarantees a valid NUL-terminated string. let name = unsafe { crate::handle::read_cstr(name) }; - let list_mut = unsafe { oaktimeline::handle::get_mut::(&list) }; + let list_mut = unsafe { oaktimeline::handle::get_mut::>>(&list) }; let Some(list_mut) = list_mut else { return oaktimeline::error::OAKTIMELINE_E_INVALID; }; - list_mut.add_marker(oaktimeline::marker::TimelineMarker::with_time( + list_mut.lock().unwrap_or_else(|e| e.into_inner()).add_marker(oaktimeline::marker::TimelineMarker::with_time( color, TimeRange::new( Rational::new(in_num as i64, in_den as i64), @@ -10643,10 +10643,10 @@ pub mod timeline { return oaktimeline::error::OAKTIMELINE_E_INVALID; } // SAFETY: marker-list handles box TimelineMarkerList. - match unsafe { oaktimeline::handle::get::(&list) } { + match unsafe { oaktimeline::handle::get::>>(&list) } { Some(l) => { // SAFETY: valid out pointer. - unsafe { *out_count = l.size() as c_int }; + unsafe { *out_count = l.lock().unwrap_or_else(|e| e.into_inner()).size() as c_int }; oaktimeline::error::OAKTIMELINE_OK } None => oaktimeline::error::OAKTIMELINE_E_INVALID, @@ -10675,10 +10675,11 @@ pub mod timeline { return oaktimeline::error::OAKTIMELINE_E_INVALID; } // SAFETY: marker-list handles box TimelineMarkerList. - let l = match unsafe { oaktimeline::handle::get::(&list) } { + let l = match unsafe { oaktimeline::handle::get::>>(&list) } { Some(l) => l, None => return oaktimeline::error::OAKTIMELINE_E_INVALID, }; + let l = l.lock().unwrap_or_else(|e| e.into_inner()); let Some(m) = l.at(index as usize) else { return oaktimeline::error::OAKTIMELINE_E_NOT_FOUND; }; @@ -10852,9 +10853,9 @@ pub mod timeline { /// `oaktimeline_workarea_set_enabled`. pub fn oaktimeline_workarea_set_enabled(w: CHandle, enabled: c_int) -> c_int { // SAFETY: work-area handles box TimelineWorkArea. - match unsafe { oaktimeline::handle::get_mut::(&w) } { + match unsafe { oaktimeline::handle::get_mut::>>(&w) } { Some(wa) => { - wa.set_enabled(enabled != 0); + wa.lock().unwrap_or_else(|e| e.into_inner()).set_enabled(enabled != 0); oaktimeline::error::OAKTIMELINE_OK } None => oaktimeline::error::OAKTIMELINE_E_INVALID, @@ -10871,11 +10872,11 @@ pub mod timeline { enabled: *mut c_int, ) -> c_int { // SAFETY: work-area handles box TimelineWorkArea. - let wa = match unsafe { oaktimeline::handle::get::(&w) } { + let wa = match unsafe { oaktimeline::handle::get::>>(&w) } { Some(wa) => wa, None => return oaktimeline::error::OAKTIMELINE_E_INVALID, }; - let range = wa.range(); + let range = *wa.lock().unwrap_or_else(|e| e.into_inner()).range(); // Out params may individually be NULL (the header contract); write // each only when the caller supplied a target. unsafe { @@ -10892,7 +10893,7 @@ pub mod timeline { *out_den = range.out().denominator() as c_int; } if !enabled.is_null() { - *enabled = if wa.enabled() { 1 } else { 0 }; + *enabled = if wa.lock().unwrap_or_else(|e| e.into_inner()).enabled() { 1 } else { 0 }; } } oaktimeline::error::OAKTIMELINE_OK @@ -10910,9 +10911,9 @@ pub mod timeline { return oaktimeline::error::OAKTIMELINE_E_INVALID; } // SAFETY: work-area handles box TimelineWorkArea. - match unsafe { oaktimeline::handle::get_mut::(&w) } { + match unsafe { oaktimeline::handle::get_mut::>>(&w) } { Some(wa) => { - wa.set_range(TimeRange::new( + wa.lock().unwrap_or_else(|e| e.into_inner()).set_range(TimeRange::new( Rational::new(in_num as i64, in_den as i64), Rational::new(out_num as i64, out_den as i64), )); diff --git a/crates/oakengine/src/test_support/codec.rs b/crates/oakengine/src/test_support/codec.rs index 55bd4e1c7..969e7c7b3 100644 --- a/crates/oakengine/src/test_support/codec.rs +++ b/crates/oakengine/src/test_support/codec.rs @@ -21,6 +21,7 @@ use super::common; use std::ffi::{c_char, c_int}; +use crate::audio::oakengine_audio_destroy_instance; use crate::codec::{ oakengine_encoding_codec_is_lossless, oakengine_encoding_codec_is_still_image, oakengine_encoding_codec_name, oakengine_encoding_filename_contains_digit_placeholder, @@ -299,9 +300,14 @@ fn params_handle_round_trip() { /// Audio recording without a running audio manager fails with E_STATE. #[test] fn start_audio_recording_no_manager() { - let p = unsafe { oakengine_encoding_params_create() }; - assert!(!p.is_null()); - let rc = unsafe { oakengine_encoding_start_audio_recording(p, std::ptr::null_mut(), 0) }; - assert_eq!(rc, -2); // OAKENGINE_E_STATE - unsafe { oakengine_encoding_params_destroy(p) }; + // The audio manager is a process-wide singleton shared with it_audio; + // serialize and normalize it (no instance) before asserting E_STATE. + common::with_manager(|| unsafe { + let _ = oakengine_audio_destroy_instance(); + let p = oakengine_encoding_params_create(); + assert!(!p.is_null()); + let rc = oakengine_encoding_start_audio_recording(p, std::ptr::null_mut(), 0); + assert_eq!(rc, -2); // OAKENGINE_E_STATE + oakengine_encoding_params_destroy(p); + }); } diff --git a/crates/oaknode/src/handle.rs b/crates/oaknode/src/handle.rs index e62c07da5..0559eab20 100644 --- a/crates/oaknode/src/handle.rs +++ b/crates/oaknode/src/handle.rs @@ -14,18 +14,25 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -//! Refcounted-handle scaffolding. Same pattern as the oakplugin crate -//! (`src/plugin/rust/src/handle.rs`); intentionally duplicated rather -//! than shared — each module DLL must run its own addref/release code -//! (the function pointers in a handle always point into the DLL that -//! created the object). +//! Refcounted-handle scaffolding for the oakengine facade boundary. +//! +//! Single-lib unification made module-to-module calls plain Rust; the +//! facade (oakengine) is the only remaining consumer of `CHandle`s in +//! this crate — it boxes oaknode domain objects (`Project`, +//! `NodeRef`) and small ABI payloads behind [`CHandle`]s so the frozen +//! C API keeps working unchanged, and oakstorage reuses the same boxes +//! for the write-through session. This module is that surface: +//! [`make_owned`]/[`make_owned_with`] create the boxes, [`get`] borrows +//! their payloads, [`RefBox`] is the box layout. +//! +//! The crate's own object references never travel through handles, and +//! the panic-catching `guard*` wrappers from the old FFI era were +//! removed together with the crate's C exports (oakengine has its own +//! guard layer). use std::any::Any; -use std::panic::{catch_unwind, AssertUnwindSafe}; use std::sync::atomic::{AtomicU32, Ordering}; -use crate::error::OAKNODE_E_FAILED; - /// ABI version stamped into every handle. pub const OAKNODE_ABI_VERSION: u32 = 1; @@ -40,45 +47,34 @@ pub struct RefBox { /// The shared ABI value-handle type (single-lib unification, see /// `docs/zh/plans/riir/single-lib.md`): one canonical /// `{ctx, addref, release, abi_version}` type in `oakcore-rs`, re-exported -/// here so the crate's `ffi.rs` signatures and handle scaffolding stay -/// source-compatible. +/// here so the facade's handle scaffolding stays source-compatible. pub use oakcore_rs::handle::CHandle; -/// addref 的实现:原子 +1。拥有型与借用型共用——借用型只延长盒子 -/// 的寿命,不延长被借用对象。 +/// addref implementation: atomic +1. Shared by owned and facade boxes — +/// a borrowed copy only extends the box's lifetime, never the borrowed +/// object's. unsafe extern "C" fn refbox_addref(ctx: *mut std::ffi::c_void) { unsafe { let rb = ctx as *const RefBox; - // 调用方保证句柄在借用期内有效(ctx 非空且未被释放)。 + // Caller guarantees the handle is alive (ctx non-null and not + // released) for the duration of the call. (*rb).refs.fetch_add(1, Ordering::Relaxed); } } -/// release 的实现(拥有型):原子 -1,归零时回收盒子并销毁内含对象。 +/// release implementation (owned): atomic -1, frees the box and destroys +/// the boxed value at zero. unsafe extern "C" fn refbox_release_owned(ctx: *mut std::ffi::c_void) { unsafe { let rb = ctx as *mut RefBox; - // AcqRel:归零这一侧要能看见最后一次引用前的全部写(含对象 - // 析构所需的内部状态)。 + // AcqRel: the zeroing side must observe every write from the last + // reference (including state the destructor needs). if (*rb).refs.fetch_sub(1, Ordering::AcqRel) == 1 { drop(Box::from_raw(rb)); } } } -/// release 的实现(借用型,[`make_borrowed`] 的产物):归零时只回收 -/// 盒子内存,把内含对象原样忘掉——其所有权仍在借用方手里。 -unsafe extern "C" fn refbox_release_borrowed(ctx: *mut std::ffi::c_void) { - unsafe { - let rb = ctx as *mut RefBox; - if (*rb).refs.fetch_sub(1, Ordering::AcqRel) == 1 { - // 部分 move:把 value 移出临时 Box,Box 析构只释放分配; - // value 用 forget 放弃析构(double-free 防线)。 - std::mem::forget((Box::from_raw(rb)).value); - } - } -} - /// Owned handle with count 1; empty on allocation failure. pub fn make_owned(value: T) -> CHandle { let rb = Box::into_raw(Box::new(RefBox { @@ -94,7 +90,7 @@ pub fn make_owned(value: T) -> CHandle { } /// Owned handle with count 1 and a caller-provided release routine -/// (used by the ffi layer's alive-counted node/project boxes, where the +/// (used by the facade for the alive-counted project boxes, where the /// release must also update the debug counter). pub fn make_owned_with( value: T, @@ -112,32 +108,6 @@ pub fn make_owned_with( } } -/// Borrowed handle for an object owned elsewhere (release frees only -/// the box). -/// -/// Semantics: bitwise copy ("borrowed copy"); the borrowed object's -/// destructor is entirely the caller's responsibility — the box never -/// touches it. -/// -/// # Safety -/// Caller guarantees `ptr` outlives every derived handle, and that its -/// value is not moved or destroyed for the borrow's lifetime. -pub unsafe fn make_borrowed(ptr: *mut T) -> CHandle { - if ptr.is_null() { - return CHandle::null(); - } - let rb = Box::into_raw(Box::new(RefBox { - refs: AtomicU32::new(1), - value: unsafe { std::ptr::read(ptr) }, - })); - CHandle { - ctx: rb as *mut std::ffi::c_void, - addref: Some(refbox_addref::), - release: Some(refbox_release_borrowed::), - abi_version: OAKNODE_ABI_VERSION, - } -} - /// Typed view into a handle; `None` for empty handles. /// /// # Safety @@ -148,27 +118,3 @@ pub unsafe fn get(h: &CHandle) -> Option<&T> { } unsafe { Some(&(*(h.ctx as *const RefBox)).value) } } - -/// Panic-catching FFI wrapper for i32-returning exports. -/// -/// Panics map to [`OAKNODE_E_FAILED`]. -pub fn guard crate::error::Result<()>>(f: F) -> i32 { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(())) => crate::error::OAKNODE_OK, - Ok(Err(e)) => e.code(), - Err(_) => OAKNODE_E_FAILED, - } -} - -/// Panic-catching FFI wrapper for handle-returning exports. -pub fn guard_handle crate::error::Result>(f: F) -> CHandle { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(h)) => h, - Ok(Err(_)) | Err(_) => CHandle::null(), - } -} - -/// Panic-catching FFI wrapper for void exports. -pub fn guard_void(f: F) { - let _ = catch_unwind(AssertUnwindSafe(f)); -} diff --git a/crates/oaknode/src/lib.rs b/crates/oaknode/src/lib.rs index c2598a621..df7306dbb 100644 --- a/crates/oaknode/src/lib.rs +++ b/crates/oaknode/src/lib.rs @@ -20,11 +20,16 @@ //! (`include/node/*.h`). See README.md for the architectural mapping //! (inheritance → arena + trait objects, etc.). //! -//! ## FFI discipline +//! ## Handle discipline //! -//! Identical to the oakplugin crate: every export goes through -//! [`handle::guard*`], handles are opaque refcounted boxes, shared -//! state behind `Mutex`. +//! Post-single-lib, this crate has no C exports: module-to-module calls +//! are plain Rust types, and object references never travel through +//! handles. The remaining `CHandle`s are (a) the facade-facing handle +//! scaffolding in [`handle`] (oakengine/oakstorage box `Project` / +//! `NodeRef` behind it) and (b) opaque cross-module payloads the crate +//! cannot name as Rust types — oakrender textures/frames/caches and +//! oaktimeline markers/work areas, whose owning crates depend on +//! oaknode. Shared state lives behind `Mutex`. #![deny(unsafe_op_in_unsafe_fn)] #![warn(missing_docs)] diff --git a/crates/oaknode/src/node.rs b/crates/oaknode/src/node.rs index ed282c53b..58e5b3f4f 100644 --- a/crates/oaknode/src/node.rs +++ b/crates/oaknode/src/node.rs @@ -426,6 +426,12 @@ impl NodeCore { /// The node's oakrender caches (frame/thumbnail/audio/waveform), /// owned handles released with the node. +/// +/// Cross-module payloads: the cache objects live behind opaque +/// oakrender handles created lazily by the facade (oakengine reads +/// `caches.video` directly through the C ABI), and oakrender depends on +/// oaknode, so no Rust type is nameable here — the handle is the +/// boundary representation. #[derive(Clone)] pub struct NodeCaches { /// Video frame hash cache. diff --git a/crates/oaknode/src/nodes/generatorwithmerge.rs b/crates/oaknode/src/nodes/generatorwithmerge.rs index 514329fdb..97d8957ea 100644 --- a/crates/oaknode/src/nodes/generatorwithmerge.rs +++ b/crates/oaknode/src/nodes/generatorwithmerge.rs @@ -90,7 +90,9 @@ impl GeneratorWithMerge { /// The Rust model has no shader-job payload (see /// [`crate::nodes::mathbase`]): the merged case pushes a null /// texture handle marking a renderer-deferred `"mrg"` shader job, - /// and the un-merged case pushes `job` itself. + /// and the un-merged case pushes `job` itself. `job` is an opaque + /// oakrender texture handle (cross-module payload; null in the + /// deferred-job model) — see [`crate::value::NodeValue::Texture`]. pub fn push_mergable_job( inputs: &crate::value::NodeValueRow, job: crate::handle::CHandle, diff --git a/crates/oaknode/src/sequence.rs b/crates/oaknode/src/sequence.rs index e682f8c32..6e23fa0c6 100644 --- a/crates/oaknode/src/sequence.rs +++ b/crates/oaknode/src/sequence.rs @@ -35,9 +35,11 @@ pub const TRACK_INPUT_FORMAT: &str = "track_in_%1"; pub struct SequenceBehavior { /// Track list node ids (video then audio, C++ order). pub track_lists: Vec, - /// Timeline markers handle (oaktimeline, owned). + /// Timeline markers handle (oaktimeline, owned; created lazily by + /// the facade through the C ABI). pub markers: crate::handle::CHandle, - /// Work area handle (oaktimeline, owned). + /// Work area handle (oaktimeline, owned; created lazily by the + /// facade through the C ABI). pub workarea: crate::handle::CHandle, /// Length cache (C++ last_length_). pub last_length: oakcore_rs::Rational, diff --git a/crates/oaknode/src/value.rs b/crates/oaknode/src/value.rs index 6f70748ab..289b63897 100644 --- a/crates/oaknode/src/value.rs +++ b/crates/oaknode/src/value.rs @@ -22,7 +22,9 @@ //! value and releases on drop, which keeps the ownership chain inside //! the refcount discipline instead of the C++ shared_ptr-in-Variant //! model (the one documented exception of the C++ tree; it does not -//! exist here). +//! exist here). Textures specifically are oakrender objects, and +//! oakrender depends on oaknode, so the payload must stay an opaque +//! [`crate::handle::CHandle`] at this boundary. use std::ffi::c_int; diff --git a/crates/oaknode/tests/engine_units_test.rs b/crates/oaknode/tests/engine_units_test.rs index 80cc44641..e36236065 100644 --- a/crates/oaknode/tests/engine_units_test.rs +++ b/crates/oaknode/tests/engine_units_test.rs @@ -21,7 +21,7 @@ use oakcore_rs::{Rational, TimeRange}; -use oaknode::error::{Error, OAKNODE_E_FAILED, OAKNODE_E_INVALID}; +use oaknode::error::{Error, OAKNODE_E_INVALID}; use oaknode::handle::{self, CHandle, RefBox}; use oaknode::id::NodeId; use oaknode::input::{flags, Input, ValueHint}; @@ -851,51 +851,22 @@ fn ops_category_and_copy_inputs() { assert!(ops::copy_inputs(&mut g, src, NodeId::INVALID, false).is_err()); } -/// handle.rs: null/is_null/guards + refcount discipline. +/// handle.rs: null/is_null + owned-box refcount discipline (the +/// facade-facing surface; the guard* wrappers and make_borrowed were +/// removed with the crate's C exports). #[test] -fn handle_helpers_and_guards() { - use oaknode::error::OAKNODE_OK; - +fn handle_boxing_discipline() { let null = CHandle::null(); assert!(null.is_null()); assert!(unsafe { handle::get::(&null) }.is_none()); - // guard: Ok -> OK; Err -> mapped code; panic -> E_FAILED. - assert_eq!(handle::guard(|| Ok(())), OAKNODE_OK); - assert_eq!(handle::guard(|| Err(Error::Invalid)), OAKNODE_E_INVALID); - assert_eq!( - handle::guard(|| -> Result<(), Error> { panic!("boom") }), - OAKNODE_E_FAILED - ); - - // guard_handle: Ok -> handle; Err/panic -> empty. - let h = handle::guard_handle(|| Ok(handle::make_owned(5u32))); - assert!(!h.ctx.is_null()); - assert!( - handle::guard_handle(|| -> Result { Err(Error::NotFound) }) - .ctx - .is_null() - ); - assert!( - handle::guard_handle(|| -> Result { panic!("x") }) - .ctx - .is_null() - ); - - // guard_void swallows panics. - handle::guard_void(|| panic!("swallowed")); - - // make_owned / make_owned_with / make_borrowed round-trip. + // make_owned / make_owned_with round-trip. let owned = handle::make_owned(7u32); let rb = owned.ctx as *const RefBox; unsafe { assert_eq!((*rb).refs.load(std::sync::atomic::Ordering::Relaxed), 1); } - let value = 9u32; - let borrowed = unsafe { handle::make_borrowed(&value as *const u32 as *mut u32) }; - assert!(!borrowed.ctx.is_null()); - assert_eq!(unsafe { handle::get::(&borrowed) }, Some(&9u32)); - assert!(unsafe { handle::make_borrowed::(std::ptr::null_mut()) }.is_null()); + assert_eq!(unsafe { handle::get::(&owned) }, Some(&7u32)); // make_owned_with uses a custom release. unsafe extern "C" fn custom_release(ctx: *mut std::ffi::c_void) { @@ -913,7 +884,7 @@ fn handle_helpers_and_guards() { ); // Release everything (single release each). - for h in [owned, borrowed, custom] { + for h in [owned, custom] { unsafe { (h.release.unwrap())(h.ctx) }; } } diff --git a/crates/oakplugin/README.md b/crates/oakplugin/README.md index 8b3baed41..0132117f4 100644 --- a/crates/oakplugin/README.md +++ b/crates/oakplugin/README.md @@ -12,7 +12,7 @@ src/ lib.rs crate 文档、模块图、FFI 纪律 error.rs 错误码(与 include/plugin/error.h 一一对应) - handle.rs 引用计数句柄脚手架({ctx,addref,release,abi_version}) + handle.rs RefBox 边界容器(Host::create_instance 返回值类型) property.rs PropertySet:OFX 属性集的存储与类型化读写 suites/ 插件调进来的 C 函数表(unsafe trampoline 层) mod.rs fetchSuite 注册表 + 渲染/GL 上下文 TLS @@ -163,9 +163,11 @@ src/ ## 实现纪律(实现方必读) -1. 所有 `extern "C"` 函数体必须包 `crate::handle::guard(..)`( - catch_unwind + 错误码映射),禁止 panic 越过 FFI。 -2. 句柄全部经 `handle.rs` 的 `RefBox`;`ctx` 永不裸指针外露含义。 +1. panic 不得越过插件 FFI:suite 分发表(`suites/mod.rs`)与宿主侧 + 插件调用点各自 `catch_unwind` 兜底并映射错误码。 +2. 实例以 `Arc>` 传(`RefBox` 为 facade 边界类型, + 见 `handle.rs`);跨 FFI 的裸指针只指向堆上稳定对象(props/ + tag 打标),永不外露其地址含义。 3. 共享状态(插件缓存、instance 注册表、线程表)一律 `Mutex`; 插件可能在其自起线程回调任意 suite(MultiThread suite 存活期)。 4. OFX 语义以 HostSupport 的行为为参照系;每个协商/时序实现点 @@ -259,8 +261,8 @@ cargo tarpaulin --out stdout --features test-stubs # 覆盖率门槛 TDD:测试声明与实现声明同步冻结(tests/,函数体 `todo!()`): -- `handle_test.rs` / `property_test.rs` — 基础设施契约(引用计数、 - free 容错、Registry、属性集语义、并发)。 +- `handle_test.rs` / `property_test.rs` — 基础设施契约(RefBox + 容器、属性集语义、并发)。 - `suites_test.rs` — 八张 suite 的 round-trip(经最小测试插件, "插件视角"的 HostSupport 兼容性背书)。 - `ffi_host_test.rs` / `ffi_instance_test.rs` — C ABI 出口契约 diff --git a/crates/oakplugin/src/handle.rs b/crates/oakplugin/src/handle.rs index a61758c64..559805780 100644 --- a/crates/oakplugin/src/handle.rs +++ b/crates/oakplugin/src/handle.rs @@ -14,213 +14,30 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -//! 引用计数句柄脚手架。 +//! 句柄机制的历史遗留:`RefBox` 容器。 //! -//! 对应 C 侧布局(`include/plugin/instance.h`,与 oak 全项目约定一致): +//! 单库化(M14 R5)后 crate 内部不再传 CHandle——原 CHandle 装拆 +//! (`make_owned`/`make_borrowed`/`get`)、panic 兜底(`guard`/ +//! `guard_handle`/`guard_void`)与身份注册表(`Registry`)均已删除: +//! 前者只被测试使用,后者在 src 无读取方(param 桥实际走 +//! [`crate::suites::param`] 的 props 地址映射与 [`crate::node`] 的 +//! 身份注册表)。 //! -//! ```c -//! typedef struct OakPluginInstance { -//! void *ctx; -//! void (*addref)(void *ctx); -//! void (*release)(void *ctx); -//! uint32_t abi_version; -//! } OakPluginInstance; -//! ``` -//! -//! 句柄按值传;`ctx` 指向本 crate 堆上的 [`RefBox`]。`addref`/ -//! `release` 函数指针永远指向本 crate 的代码(所有权不出 DLL)。 +//! 仅 [`RefBox`] 保留:它是 [`crate::host::Host::create_instance`] 的 +//! 返回值容器(`Arc>`),该类型被 oakengine 的 +//! test_support 显式标注消费,是本 crate 在 facade 边界的公开类型。 -use std::any::Any; -use std::collections::HashMap; -use std::panic::{catch_unwind, AssertUnwindSafe}; -use std::sync::atomic::{AtomicU32, Ordering}; -use std::sync::{Arc, Mutex, Weak}; +use std::sync::atomic::AtomicU32; -use crate::error::OAKPLUGIN_E_FAILED; - -/// 当前 ABI 版本,写进每个句柄的 `abi_version` 字段。 -pub const OAKPLUGIN_ABI_VERSION: u32 = 1; - -/// 句柄背后的堆盒子。`owns == false` 的盒子(借用包装)在计数归零时 -/// 只释放盒子本身,不销毁内含对象。 +/// 边界盒:`Arc>` 的承载类型。 +/// +/// 原为 C 句柄(`{ctx, addref, release, abi_version}`)背后的堆盒子, +/// addref/release thunk 经 `refs` 计数;装拆删除后 `refs` 不再被读写, +/// 仅以固定值 1 构造("单个拥有者"语义),生命周期完全由外层 +/// `Arc` 管理。 pub struct RefBox { - /// 引用计数(原子;release 可在任意线程发生)。 + /// 引用计数(句柄时代遗留,现无 thunk 读写)。 pub refs: AtomicU32, /// 内含对象。 pub value: T, } - -/// C 句柄的 Rust 镜像。`#[repr(C)]`,与 C 头文件布局一致。 -/// -/// 生命周期:`*_init`/`*_create` 返回计数 1 的拥有型句柄; -/// The shared ABI value-handle type (single-lib unification, see -/// `docs/zh/plans/riir/single-lib.md`): one canonical -/// `{ctx, addref, release, abi_version}` type in `oakcore-rs`, re-exported -/// here so the crate's handle scaffolding stays source-compatible. -/// `Send + Sync` come from the shared type. -pub use oakcore_rs::handle::CHandle; - -/// addref 的实现:原子 +1。拥有型与借用型共用——借用型只延长盒子 -/// 的寿命,不延长被借用对象。 -unsafe extern "C" fn refbox_addref(ctx: *mut std::ffi::c_void) { - unsafe { - let rb = ctx as *const RefBox; - // 调用方保证句柄在借用期内有效(ctx 非空且未被释放)。 - (*rb).refs.fetch_add(1, Ordering::Relaxed); - } -} - -/// release 的实现(拥有型):原子 -1,归零时回收盒子并销毁内含对象。 -unsafe extern "C" fn refbox_release_owned(ctx: *mut std::ffi::c_void) { - unsafe { - let rb = ctx as *mut RefBox; - // AcqRel:归零这一侧要能看见最后一次引用前的全部写(含对象 - // 析构所需的内部状态)。 - if (*rb).refs.fetch_sub(1, Ordering::AcqRel) == 1 { - drop(Box::from_raw(rb)); - } - } -} - -/// release 的实现(借用型,[`make_borrowed`] 的产物):归零时只回收 -/// 盒子内存,把内含对象原样忘掉——其所有权仍在借用方手里。 -unsafe extern "C" fn refbox_release_borrowed(ctx: *mut std::ffi::c_void) { - unsafe { - let rb = ctx as *mut RefBox; - if (*rb).refs.fetch_sub(1, Ordering::AcqRel) == 1 { - // 部分 move:把 value 移出临时 Box,Box 析构只释放分配; - // value 用 forget 放弃析构(double-free 防线)。 - std::mem::forget((Box::from_raw(rb)).value); - } - } -} - -/// 为 `T` 制作拥有型句柄(计数 1)。分配失败返回空句柄并销毁对象。 -/// -/// 注:Rust 默认分配失败(OOM)直接 abort,不会走到"返回空句柄" -/// 路径;此处语义保留给未来接入自定义分配器的场景。 -pub fn make_owned(value: T) -> CHandle { - let rb = Box::into_raw(Box::new(RefBox { - refs: AtomicU32::new(1), - value, - })); - CHandle { - ctx: rb as *mut std::ffi::c_void, - addref: Some(refbox_addref::), - release: Some(refbox_release_owned::), - abi_version: OAKPLUGIN_ABI_VERSION, - } -} - -/// 为已有对象制作借用句柄(计数归零只释放盒子)。`ptr` 必须在本 -/// 句柄被释放前保持有效。 -/// -/// 语义:按位拷贝("借用拷贝",如纹理句柄的快照);被借用对象 -/// 的析构完全由调用方负责,盒子从不碰它。拷贝即快照——借出后 -/// 修改 `*ptr` 不会反映到句柄内。 -/// -/// # Safety -/// 调用方保证 `ptr` 的生命周期覆盖所有派生句柄,且其值在借用期内 -/// 不被 move/析构。 -pub unsafe fn make_borrowed(ptr: *mut T) -> CHandle { - if ptr.is_null() { - return CHandle::null(); - } - let rb = Box::into_raw(Box::new(RefBox { - refs: AtomicU32::new(1), - value: unsafe { std::ptr::read(ptr) }, - })); - CHandle { - ctx: rb as *mut std::ffi::c_void, - addref: Some(refbox_addref::), - release: Some(refbox_release_borrowed::), - abi_version: OAKPLUGIN_ABI_VERSION, - } -} - -/// 取回盒子内对象的不可变引用;空句柄返回 `None`。 -/// -/// # Safety -/// 调用方必须保证 `T` 与创建句柄时的类型一致。 -pub unsafe fn get(h: &CHandle) -> Option<&T> { - if h.is_null() { - return None; - } - unsafe { Some(&(*(h.ctx as *const RefBox)).value) } -} - -/// FFI 兜底:捕获 panic,把 `Result` 映射为对外错误码 -/// ([`crate::error`])。所有返回 i32 的导出函数必须经它。 -/// -/// panic 路径返回 `OAKPLUGIN_E_FAILED`;panic 详情暂不落日志 -/// (message 桥接入后补 TODO)。 -pub fn guard(f: F) -> i32 -where - F: FnOnce() -> crate::error::Result<()>, -{ - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(())) => crate::error::OAKPLUGIN_OK, - Ok(Err(e)) => e.code(), - Err(_) => OAKPLUGIN_E_FAILED, - } -} - -/// 指针/句柄返回值版本的 [`guard`]:panic 或 Err 时返回空句柄 -/// (指针类返回 NULL)。 -pub fn guard_handle(f: F) -> CHandle -where - F: FnOnce() -> crate::error::Result, -{ - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(h)) => h, - Ok(Err(_)) | Err(_) => CHandle::null(), - } -} - -/// 无返回值版本:panic 被吞(日志回调待 message 出口接入后补)。 -pub fn guard_void(f: F) -where - F: FnOnce(), -{ - let _ = catch_unwind(AssertUnwindSafe(f)); -} - -/// 句柄身份注册表:`usize` 身份 ↔ 弱引用。供 param↔node 等需要 -/// "按身份找回对象"的桥使用(替代 M9 C++ 版的 -/// `oaknode_node_identity()` 注册表)。 -pub struct Registry { - map: Mutex>>>, -} - -impl Registry { - /// 空注册表。 - pub fn new() -> Self { - Self { - map: Mutex::new(HashMap::new()), - } - } - - /// 登记对象,返回其身份(地址语义,进程内唯一)。 - pub fn register(&self, obj: &Arc>) -> usize { - // Arc 分配地址即身份:同一 RefBox 恒稳定,进程内唯一。 - let id = Arc::as_ptr(obj) as *const () as usize; - lock(&self.map).insert(id, Arc::downgrade(obj)); - id - } - - /// 按身份取对象;对象已销毁或身份未知返回 `None`。 - pub fn lookup(&self, id: usize) -> Option>> { - lock(&self.map).get(&id).and_then(|w| w.upgrade()) - } - - /// 摘除身份(对象销毁路径调用)。未知身份是 no-op。 - pub fn unregister(&self, id: usize) { - lock(&self.map).remove(&id); - } -} - -/// 取锁。毒锁(本 crate 代码持锁时 panic)时接管内部状态继续—— -/// 一次 panic 不级联成后续所有 FFI 调用失败。 -fn lock(m: &Mutex) -> std::sync::MutexGuard<'_, T> { - m.lock().unwrap_or_else(|e| e.into_inner()) -} diff --git a/crates/oakplugin/src/host.rs b/crates/oakplugin/src/host.rs index 3b429bc5e..41a773b84 100644 --- a/crates/oakplugin/src/host.rs +++ b/crates/oakplugin/src/host.rs @@ -47,7 +47,7 @@ use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex, OnceLock}; use crate::descriptor::EffectDescriptor; -use crate::handle::{RefBox, Registry}; +use crate::handle::RefBox; use crate::instance::Instance; use crate::property::{PropertySet, Value}; use crate::suites::status; @@ -790,14 +790,6 @@ pub struct Host { pub(crate) instances: Mutex>>>, } -/// 实例身份注册表(param 桥按身份反查;见 [`crate::handle::Registry`])。 -static INSTANCE_REGISTRY: OnceLock> = OnceLock::new(); - -/// 实例身份注册表入口。 -pub(crate) fn instance_registry() -> &'static Registry { - INSTANCE_REGISTRY.get_or_init(Registry::new) -} - impl Host { /// 进程单例。首次调用构建宿主属性集(能力宣告在此写入)。 pub fn global() -> &'static Host { @@ -919,7 +911,7 @@ impl Host { ))); } - // 登记:实例表 + param→instance 回写表。 + // 登记活跃实例表(泄漏断言用)+ param→instance 回写表。 self.instances .lock() .unwrap_or_else(|e| e.into_inner()) @@ -929,7 +921,6 @@ impl Host { let p_addr = &p.props as *const PropertySet as usize; crate::suites::param::register_param_owner(p_addr, inst_props); } - instance_registry().register(&arc); Ok(arc) } diff --git a/crates/oakplugin/src/instance.rs b/crates/oakplugin/src/instance.rs index b9af4b684..58beec023 100644 --- a/crates/oakplugin/src/instance.rs +++ b/crates/oakplugin/src/instance.rs @@ -86,8 +86,9 @@ pub struct RenderScale { pub y: f64, } -/// 插件实例。`Arc>` 管理生命周期;身份注册见 -/// [`crate::handle::Registry`](param 桥按身份反查)。 +/// 插件实例。`Arc>` 管理生命周期(`RefBox` 为 facade +/// 边界类型,见 [`crate::handle`]);param 桥按 props 地址映射反查 +/// ([`crate::suites::param`]),节点绑定经 [`crate::node`] 身份注册表。 /// /// `#[repr(C)]` + props 在偏移 0(句柄约定,见 [`crate::suites::tag`]; /// 实例期 effect/param-set handle 即 `&props`)。 @@ -123,8 +124,9 @@ pub struct Instance { pub render_lock: std::sync::Mutex<()>, } -/// 实例销毁路径:先通知 destroyInstance action,再摘除身份登记。 -/// (RefBox 归零时 Drop 触发——action 通知必须在对象析构前发出。) +/// 实例销毁路径:先通知 destroyInstance action,再摘除 param 回写登记。 +/// (`Arc>` 归零时 Drop 触发——action 通知必须在 +/// 对象析构前发出。) impl Drop for Instance { fn drop(&mut self) { if !self @@ -134,8 +136,6 @@ impl Drop for Instance { self.notify_destroy(); } crate::suites::param::unregister_params_of(&self.props as *const _ as usize); - crate::host::instance_registry() - .unregister(&self.props as *const crate::property::PropertySet as usize); } } @@ -954,8 +954,8 @@ impl Instance { Ok(()) } - /// 销毁(destroyInstance action)。析构由 RefBox 驱动; - /// 此处只做 action 通知,幂等([`Instance::drop`] 的 + /// 销毁(destroyInstance action)。析构由 `Arc>` + /// 归零驱动;此处只做 action 通知,幂等([`Instance::drop`] 的 /// `destroyed` 门保证只发一次)。 pub(crate) fn notify_destroy(&self) { use crate::host::ACTION_DESTROY_INSTANCE; diff --git a/crates/oakplugin/tests/handle_test.rs b/crates/oakplugin/tests/handle_test.rs index ddd337b79..7cc8518c7 100644 --- a/crates/oakplugin/tests/handle_test.rs +++ b/crates/oakplugin/tests/handle_test.rs @@ -14,22 +14,23 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -//! handle.rs 的契约测试:引用计数语义、free 容错、借用盒、Registry。 +//! handle.rs 的契约测试:`RefBox` 容器。 //! -//! 对应实现:crate::handle。每个测试只验一条规则,命名即规约。 +//! 单库化(M14 R5)后 crate 内部不再传 CHandle——原 CHandle 装拆 +//! (`make_owned`/`make_borrowed`/`get`)、panic 兜底(`guard`/ +//! `guard_handle`/`guard_void`)与身份注册表(`Registry`)均已删除。 +//! [`oakplugin::handle::RefBox`] 仅作为 `Host::create_instance` 的 +//! 边界返回类型保留(`Arc>`,oakengine test_support +//! 消费);此处验证其基本契约。每个测试只验一条规则,命名即规约。 mod common; -use std::ptr; use std::sync::atomic::{AtomicU32, AtomicUsize, Ordering}; use std::sync::Arc; -use oakplugin::error::{Error, OAKPLUGIN_E_FAILED, OAKPLUGIN_E_NOT_FOUND, OAKPLUGIN_OK}; -use oakplugin::handle::{ - get, guard, guard_handle, make_borrowed, make_owned, CHandle, RefBox, Registry, -}; +use oakplugin::handle::RefBox; -/// 析构标志:以"被析构次数"断言对象的销毁时机(引用计数语义的 +/// 析构标志:以"被析构次数"断言对象的销毁时机(Arc 生命周期语义的 /// 行为探针)。 struct DropFlag(Arc); @@ -39,207 +40,32 @@ impl Drop for DropFlag { } } -/// 模拟 C 侧 addref(头文件契约:复制句柄时先 addref)。 -fn addref(h: &CHandle) { - unsafe { (h.addref.expect("addref fn 缺失"))(h.ctx) }; -} - -/// 模拟 C 侧 free(`oakplugin_instance_free` 语义:ctx 非空才调 -/// release,随后清空 ctx;空句柄/已清空句柄是 no-op)。 -fn free(h: &mut CHandle) { - if !h.is_null() { - unsafe { (h.release.expect("release fn 缺失"))(h.ctx) }; - } - h.ctx = ptr::null_mut(); -} - -/// 拥有型句柄:创建计数为 1;addref 后 release 一次对象仍活; -/// 再 release 对象销毁(用析构标志位断言)。 +/// 值可经 `.value` 字段取回(create_instance 返回后调用方的访问方式)。 #[test] -fn owned_handle_refcount_lifecycle() { - let drops = Arc::new(AtomicUsize::new(0)); - - // 按值传句柄的位级复制(C 侧 `OakPluginInstance` 结构体拷贝), - // 复制方必须先 addref:refs 1 -> 2。 - let h1 = make_owned(DropFlag(drops.clone())); - let mut h2 = unsafe { ptr::read(&h1) }; - addref(&h2); - - // 释放一份:2 -> 1,对象仍活。 - let mut h1 = h1; - free(&mut h1); - assert_eq!(drops.load(Ordering::Relaxed), 0); - - // 释放最后一份:1 -> 0,对象恰好销毁一次。 - free(&mut h2); - assert_eq!(drops.load(Ordering::Relaxed), 1); -} - -/// free(NULL)/free(空句柄)/重复 free 同一个已清空句柄:全部 no-op, -/// 不崩、不计数变化(alive 计数前后一致)。 -#[test] -fn free_null_and_empty_is_noop() { - let drops = Arc::new(AtomicUsize::new(0)); - - // free(空句柄):no-op,不崩。 - let mut null = CHandle::null(); - free(&mut null); - assert!(null.is_null()); - - // 正常销毁后句柄已清空;重复 free 是 no-op,计数不再变化。 - let mut h = make_owned(DropFlag(drops.clone())); - free(&mut h); - assert_eq!(drops.load(Ordering::Relaxed), 1); - free(&mut h); - free(&mut h); - assert_eq!(drops.load(Ordering::Relaxed), 1); -} - -/// 借用句柄:release 只释放盒子,被借用的对象仍然存活 -/// (用外部栈对象的析构标志断言)。 -#[test] -fn borrowed_handle_never_destroys_object() { - let drops = Arc::new(AtomicUsize::new(0)); - let mut obj = DropFlag(drops.clone()); - - let mut h = unsafe { make_borrowed(&mut obj as *mut DropFlag) }; - assert!(!h.is_null()); - - free(&mut h); - // 盒子释放了,但对象归调用方:析构不在 release 时发生。 - assert_eq!(drops.load(Ordering::Relaxed), 0); - assert!(h.is_null()); - - // 对象最终由调用方析构,恰好一次。 - drop(obj); - assert_eq!(drops.load(Ordering::Relaxed), 1); -} - -/// 空句柄的 get::() 返回 None;类型不符的 get 是调用方责任 -/// (文档约定),此处只验空句柄路径。 -#[test] -fn get_on_empty_handle_is_none() { - assert!(unsafe { get::(&CHandle::null()) }.is_none()); - - // 对照:正常句柄能取回引用。 - let mut h = make_owned(42u32); - assert_eq!(unsafe { get::(&h) }, Some(&42)); - free(&mut h); -} - -/// guard:闭包 panic 被捕获并映射为 OAKPLUGIN_E_FAILED, -/// 不 unwind 出 FFI;Err 映射为对应负码;Ok 映射为 OAKPLUGIN_OK。 -#[test] -fn guard_maps_panic_err_ok() { - // Ok -> OAKPLUGIN_OK。 - assert_eq!(guard(|| Ok(())), OAKPLUGIN_OK); - - // Err -> 对应负码(错误码与 include/plugin/error.h 一致, - // 项目 -MMCCCC 方案:-90001..-90005)。 - assert_eq!(guard(|| Err(Error::NotFound)), OAKPLUGIN_E_NOT_FOUND); - assert_eq!( - guard(|| Err(Error::Invalid)), - oakplugin::error::OAKPLUGIN_E_INVALID - ); - assert_eq!( - guard(|| Err(Error::State)), - oakplugin::error::OAKPLUGIN_E_STATE - ); - assert_eq!( - guard(|| Err(Error::Failed("x".into()))), - oakplugin::error::OAKPLUGIN_E_FAILED - ); - assert_eq!( - guard(|| Err(Error::NoMem)), - oakplugin::error::OAKPLUGIN_E_NOMEM - ); - - // panic -> OAKPLUGIN_E_FAILED,且 panic 不越过 guard 边界 - // (catch_unwind 语义:本测试线程存活即证明未 unwind)。 - assert_eq!(guard(|| panic!("boom")), OAKPLUGIN_E_FAILED); -} - -/// guard_handle:panic/Err 返回空句柄;Ok 透传非空句柄。 -#[test] -fn guard_handle_maps_to_null_on_failure() { - let mut ok = guard_handle(|| Ok(make_owned(7u32))); - assert!(!ok.is_null()); - free(&mut ok); - - let err = guard_handle(|| Err::(Error::State)); - assert!(err.is_null()); - - let panicked: CHandle = guard_handle(|| panic!("boom")); - assert!(panicked.is_null()); -} - -/// Registry:register 返回唯一身份;lookup 命中;对象销毁后 -/// lookup 返回 None(弱引用语义);unregister 未知身份 no-op。 -#[test] -fn registry_register_lookup_unregister() { - let reg: Registry = Registry::new(); - - let arc = Arc::new(RefBox { +fn refbox_exposes_value_by_field() { + let boxed = RefBox { refs: AtomicU32::new(1), value: 7u32, - }); - let id = reg.register(&arc); - assert_eq!(id, Arc::as_ptr(&arc) as *const () as usize); - assert_eq!(reg.lookup(id).unwrap().value, 7); - - // 同一对象重复登记:身份稳定(地址语义),值覆盖。 - let id2 = reg.register(&arc); - assert_eq!(id2, id); - - // 摘除后 lookup 命中失败;再摘除未知身份是 no-op。 - reg.unregister(id); - assert!(reg.lookup(id).is_none()); - reg.unregister(id); - - // 对象销毁后弱引用失效:lookup 返回 None。 - let dying = Arc::new(RefBox { - refs: AtomicU32::new(1), - value: 9u32, - }); - let dying_id = reg.register(&dying); - drop(dying); - assert!(reg.lookup(dying_id).is_none()); - - drop(arc); + }; + assert_eq!(boxed.value, 7); } -/// 并发:64 线程对同一句柄 addref/release 各一千次,最终计数正确、 -/// 对象恰好销毁一次(线程模型是 multithread suite 的直接投影)。 +/// 生命周期完全由外层 Arc 管理:`refs` 只以固定值 1 构造(无 thunk +/// 增减),Arc 归零时内含对象恰好析构一次;弱引用在 Arc 存活期间 +/// 有效、归零后失效(host 的活跃实例表即 Weak 列表)。 #[test] -fn refcount_is_thread_safe() { +fn refbox_value_drops_exactly_once_when_arc_reaches_zero() { let drops = Arc::new(AtomicUsize::new(0)); - let h = make_owned(DropFlag(drops.clone())); + let boxed = Arc::new(RefBox { + refs: AtomicU32::new(1), + value: DropFlag(drops.clone()), + }); - // 每个线程持有 ctx + 函数指针的拷贝(对应 C 侧各线程各持一份 - // 句柄值),对同一对象做 1000 次 addref/release 配对。 - // 裸指针不可 Send,测试侧经 usize 搬运(C 侧本来也是整数传递)。 - let threads: Vec<_> = (0..64) - .map(|_| { - let ctx = h.ctx as usize; - let addref = h.addref.expect("addref fn 缺失"); - let release = h.release.expect("release fn 缺失"); - std::thread::spawn(move || { - for _ in 0..1000 { - unsafe { addref(ctx as *mut std::ffi::c_void) }; - unsafe { release(ctx as *mut std::ffi::c_void) }; - } - }) - }) - .collect(); - for t in threads { - t.join().unwrap(); - } - - // 全部配对完成:计数回到创建值,对象存活。 + let weak = Arc::downgrade(&boxed); + assert!(weak.upgrade().is_some()); assert_eq!(drops.load(Ordering::Relaxed), 0); - // 最终一次 release 恰好销毁。 - let mut h = h; - free(&mut h); + drop(boxed); assert_eq!(drops.load(Ordering::Relaxed), 1); + assert!(weak.upgrade().is_none()); } diff --git a/crates/oakrender/README.md b/crates/oakrender/README.md index f7942fb5f..1dc0bdb42 100644 --- a/crates/oakrender/README.md +++ b/crates/oakrender/README.md @@ -63,7 +63,7 @@ frozen, implemented verbatim by `src/ffi.rs`. src/ lib.rs crate doc + module map error.rs error codes (mirrors include/render/error.h) - handle.rs refcounted-handle scaffolding + live-object accounting + handle.rs refcounted-handle scaffolding (facade entry points only) texture.rs Texture value type (wraps backend textures / CPU frames) frame.rs VideoParamsPod + Frame helpers cache.rs PlaybackCache / FrameHashCache family + C++-parity disk state @@ -83,7 +83,9 @@ tests/ contract + golden tests (common/ has shared helpers) ## Hard rules -1. Every export goes through `handle::guard*`. +1. `CHandle` only appears at the facade boundary: the crate's internal + calls pass Rust types directly; `handle::make_owned`/`get`/`get_mut` + are the facade entry points the oakengine stubs call. 2. No `unsafe` outside `backend.rs` (GPU FFI) and `bridge/`. 3. F32 + ACEScg pipeline invariants are asserted in tests, not in comments (see tests/pipeline_test.rs). diff --git a/crates/oakrender/src/autocacher.rs b/crates/oakrender/src/autocacher.rs index 3ce9dbd97..20f62ca1d 100644 --- a/crates/oakrender/src/autocacher.rs +++ b/crates/oakrender/src/autocacher.rs @@ -337,7 +337,10 @@ mod tests { #[test] fn single_frame_cancels_previous() { - let (mut c, mut pool) = new_cacher(); + // The race this asserts ("the previous frame is cancelled") is only + // deterministic when the first job cannot finish before the + // superseding submit lands — produce frames slowly for this test. + let (mut c, mut pool) = new_cacher_slow(); c.attach(7); let first = c.single_frame(Rational::new(0, 1)); let second = c.single_frame(Rational::new(1, 1)); @@ -349,6 +352,25 @@ mod tests { pool.shutdown(); } + /// A cacher whose frames take ~100ms to produce (see + /// [`single_frame_cancels_previous`]). + fn new_cacher_slow() -> (PreviewAutoCacher, WorkerPool) { + let mut pool = WorkerPool::new(2); + pool.start(); + let producer: crate::ticket::Producer = Arc::new(|_, _| { + std::thread::sleep(std::time::Duration::from_millis(100)); + let mut f = Frame::new(); + let mut p = VideoParamsPod::default(); + p.width = 4; + p.height = 4; + f.set_video_params(p); + f.allocate(); + Ok(crate::ticket::TicketPayload::Video(Texture::wrap_frame(f))) + }); + let arena = Arc::new(TicketArena::new(pool.clone(), producer)); + (PreviewAutoCacher::new(arena), pool) + } + #[test] fn ignore_requests_suppresses_jobs() { let (mut c, mut pool) = new_cacher(); diff --git a/crates/oakrender/src/copier.rs b/crates/oakrender/src/copier.rs index bd7780d57..dbcdb2982 100644 --- a/crates/oakrender/src/copier.rs +++ b/crates/oakrender/src/copier.rs @@ -17,13 +17,44 @@ //! Render-side project copy client (the C++ ProjectCopier, inverted): //! all copying happens inside oaknode. oaknode never implemented the //! deep-copy direction (single-lib plan §4.1 — dead direction), so the -//! copy operations fail explainably and the success-path tests are -//! `#[ignore]`d. +//! copy operations fail explainably; the tests assert those failures. +//! +//! M14 R5: the module is entirely internal to oakrender (no facade entry +//! is involved), so the oaknode project handle was reduced to its numeric +//! identity — a Rust value type instead of a `CHandle`. The live project +//! object stays with oaknode; the render side stores only identity pairs +//! (see `COVERAGE.md`, "render 只存 identity 对"). use crate::error::{Error, Result}; -/// A project handle (oaknode-owned; the shared canonical handle type). -pub type ProjectHandle = crate::handle::CHandle; +/// An opaque identity for an oaknode project (the C++ handle's `ctx` +/// reduced to its numeric value). oaknode owns the live project and +/// maintains the identity map; this module never holds the object, so no +/// lifetime management is required. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct ProjectHandle(u64); + +impl ProjectHandle { + /// New identity; `0` is the empty handle. + pub fn new(identity: u64) -> Self { + ProjectHandle(identity) + } + + /// The empty handle (no project attached). + pub fn null() -> Self { + ProjectHandle(0) + } + + /// True for the empty handle. + pub fn is_null(self) -> bool { + self.0 == 0 + } + + /// The raw identity value. + pub fn as_u64(self) -> u64 { + self.0 + } +} /// One change record (see oaknode `ChangeRecord`). #[repr(C)] @@ -77,15 +108,13 @@ pub fn project_sync_copy( )) } -/// A handle to a render-side project copy. +/// A render-side project copy (identity only — the copy's live object +/// stays with oaknode). pub struct ProjectCopy { /// Identity of the source project. pub source: u64, - /// Identity of the copied project (oaknode-owned). + /// Identity of the copied project (0 = no copy attached). pub copy: u64, - /// Owned oaknode handle to the copy (kept alive for the copier's - /// lifetime; released on drop). - copy_handle: Option, /// Change-generation counter of the last successful sync. pub last_sync_generation: u64, /// True while recorded changes await `sync`. @@ -98,19 +127,18 @@ impl ProjectCopy { Self { source: 0, copy: 0, - copy_handle: None, last_sync_generation: 0, has_pending_updates: false, } } - /// Create a deep copy of `source` through the oaknode C ABI - /// (C++ `ProjectCopier::set_project`). + /// Create a deep copy of `source` through oaknode (C++ + /// `ProjectCopier::set_project`). pub fn set_project(&mut self, source: ProjectHandle) -> Result<()> { if source.is_null() { return Err(Error::Invalid); } - // Release any previous copy. + // Drop any previous copy. self.release_copy(); let copy = crate::copier::project_deep_copy(source); if copy.is_null() { @@ -118,9 +146,8 @@ impl ProjectCopy { "oaknode_project_deep_copy failed (symbol missing or copy error)".into(), )); } - self.source = source.ctx as u64; - self.copy = copy.ctx as u64; - self.copy_handle = Some(copy); + self.source = source.as_u64(); + self.copy = copy.as_u64(); self.last_sync_generation = 0; self.has_pending_updates = false; Ok(()) @@ -129,26 +156,26 @@ impl ProjectCopy { /// Push a recorded change set into the copy (C++ /// ProjectCopier::process_update_queue). pub fn sync(&mut self, changes: &[ChangeRecord]) -> Result<()> { - let source = ProjectHandle { - ctx: self.source as *mut std::ffi::c_void, - addref: None, - release: None, - abi_version: crate::handle::OAKRENDER_ABI_VERSION, - }; - let copy = self.copy_handle.unwrap_or_else(ProjectHandle::null); - if copy.is_null() { + if self.copy == 0 { return Err(Error::State); } - crate::copier::project_sync_copy(source, copy, changes)?; + crate::copier::project_sync_copy( + ProjectHandle::new(self.source), + ProjectHandle::new(self.copy), + changes, + )?; self.last_sync_generation += 1; self.has_pending_updates = false; Ok(()) } - /// The copied project handle (owned by this copier; borrowed for the - /// caller). + /// The copied project's identity (borrowed for the caller). pub fn copied_project(&self) -> Option { - self.copy_handle + if self.copy == 0 { + None + } else { + Some(ProjectHandle::new(self.copy)) + } } /// The copied counterpart of an original node — requires the oaknode @@ -158,19 +185,12 @@ impl ProjectCopy { None } - /// Drop the copy (releases the oaknode handle). + /// Drop the copy (forgets its identity). pub fn destroy(&mut self) { self.release_copy(); } fn release_copy(&mut self) { - if let Some(handle) = self.copy_handle.take() { - if let Some(release) = handle.release { - // SAFETY: the handle came from oaknode_project_deep_copy; - // releasing the last reference destroys the copy. - unsafe { release(handle.ctx) }; - } - } self.copy = 0; } } @@ -181,12 +201,6 @@ impl Default for ProjectCopy { } } -impl Drop for ProjectCopy { - fn drop(&mut self) { - self.release_copy(); - } -} - #[cfg(test)] mod tests { use super::*; @@ -209,6 +223,19 @@ mod tests { ); } + #[test] + fn set_project_with_valid_identity_fails() { + // oaknode never implemented the deep-copy direction; even a valid + // identity cannot be copied, and the copier fails explainably. + let mut pc = ProjectCopy::new(); + assert_eq!( + pc.set_project(ProjectHandle::new(1)).unwrap_err().code(), + Error::Failed(String::new()).code() + ); + assert_eq!(pc.copy, 0); + assert!(pc.copied_project().is_none()); + } + #[test] fn sync_without_project_is_state_error() { let mut pc = ProjectCopy::new(); @@ -225,21 +252,4 @@ mod tests { pc.destroy(); assert_eq!(pc.copy, 0); } - - #[test] - #[ignore = "needs oaknode deep-copy (not implemented)"] - fn deep_copy_roundtrip_with_real_node() { - // oaknode never implemented the deep-copy direction; the copy - // always fails explainably, which is what the live path checks. - let mut pc = ProjectCopy::new(); - let src = ProjectHandle { - ctx: 1 as *mut std::ffi::c_void, - addref: None, - release: None, - abi_version: crate::handle::OAKRENDER_ABI_VERSION, - }; - pc.set_project(src).unwrap(); - assert_ne!(pc.copy, 0); - assert!(pc.copied_project().is_some()); - } } diff --git a/crates/oakrender/src/handle.rs b/crates/oakrender/src/handle.rs index 021fb4692..dafa57727 100644 --- a/crates/oakrender/src/handle.rs +++ b/crates/oakrender/src/handle.rs @@ -14,61 +14,40 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -//! Refcounted-handle scaffolding (same per-module pattern as the -//! oakplugin/oaknode crates; duplicated on purpose — handle function -//! pointers must run code from the creating DLL). +//! Refcounted-handle scaffolding for the oakengine facade entry points. +//! +//! M14 R5: after the single-lib unification, no object reference passes +//! as a `CHandle` inside oakrender anymore — the crate's internal calls +//! use Rust types directly. The remaining surface is only what the +//! facade's stubs.rs calls at the boundary: [`make_owned`] (box a value +//! into an owned handle), [`get`]/[`get_mut`] (typed views back out). //! //! Mirrors `src/render/c_api/internalhandles.h`: every public oakrender //! handle is `{ctx, addref, release, abi_version}`; `ctx` points at a -//! [`RefBox`] on this crate's heap. `owns == false` boxes (borrowed -//! wrappers) only free the box at zero. -//! -//! Live-object accounting mirrors the C++ `alive_inc`/`alive_dec`: -//! [`make_owned`] counts the handle, the owned release un-counts it, so -//! `oakrender_debug_alive_count()` stays meaningful for leak assertions. +//! [`RefBox`] on this crate's heap. use std::any::Any; -use std::panic::{catch_unwind, AssertUnwindSafe}; -use std::sync::atomic::{AtomicU32, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicU32, Ordering}; /// ABI version stamped into every handle. pub const OAKRENDER_ABI_VERSION: u32 = 1; /// Heap box behind a handle's `ctx`. -pub struct RefBox { +struct RefBox { /// Atomic reference count. - pub refs: AtomicU32, + refs: AtomicU32, /// Boxed value. - pub value: T, + value: T, } /// The shared ABI value-handle type (single-lib unification, see /// `docs/zh/plans/riir/single-lib.md`): one canonical /// `{ctx, addref, release, abi_version}` type in `oakcore-rs`, re-exported -/// here so the crate's `ffi.rs` signatures and handle scaffolding stay -/// source-compatible. `Send + Sync` come from the shared type. +/// here so the facade's handle scaffolding stays source-compatible. +/// `Send + Sync` come from the shared type. pub use oakcore_rs::handle::CHandle; -/// Global live-object count (owned handles + cancel-atom boxes). -static ALIVE_COUNT: AtomicUsize = AtomicUsize::new(0); - -/// Increment the live-object count (owned handle creation). -pub fn alive_inc() { - ALIVE_COUNT.fetch_add(1, Ordering::Relaxed); -} - -/// Decrement the live-object count (owned handle destruction). -pub fn alive_dec() { - ALIVE_COUNT.fetch_sub(1, Ordering::Relaxed); -} - -/// Current live-object count (`oakrender_debug_alive_count`). -pub fn alive_count() -> i32 { - ALIVE_COUNT.load(Ordering::Relaxed) as i32 -} - -/// addref implementation: atomic +1. Shared by owned and borrowed boxes — -/// borrowing only extends the box's lifetime, not the borrowed object's. +/// addref implementation: atomic +1 on the box's refcount. unsafe extern "C" fn refbox_addref(ctx: *mut std::ffi::c_void) { unsafe { let rb = ctx as *const RefBox; @@ -77,42 +56,25 @@ unsafe extern "C" fn refbox_addref(ctx: *mut std::ffi::c_void) { } } -/// release implementation (owned): atomic -1; at zero, reclaim the box and -/// destroy the contained object. +/// release implementation: atomic -1; at zero, reclaim the box and destroy +/// the contained object. unsafe extern "C" fn refbox_release_owned(ctx: *mut std::ffi::c_void) { unsafe { let rb = ctx as *mut RefBox; // AcqRel: the thread that drops the last reference must observe all // prior writes (including internal state the destructor needs). if (*rb).refs.fetch_sub(1, Ordering::AcqRel) == 1 { - alive_dec(); drop(Box::from_raw(rb)); } } } -/// release implementation (borrowed, produced by [`make_borrowed`]): at -/// zero only reclaim the box memory, forgetting the contained object — its -/// ownership stays with the borrower. -unsafe extern "C" fn refbox_release_borrowed(ctx: *mut std::ffi::c_void) { - unsafe { - let rb = ctx as *mut RefBox; - if (*rb).refs.fetch_sub(1, Ordering::AcqRel) == 1 { - // Partial move: move the value out of the temporary Box so the - // Box drop only frees the allocation; forget the value so it is - // never dropped (double-free guard). - std::mem::forget((Box::from_raw(rb)).value); - } - } -} - /// Owned handle with count 1; empty on allocation failure. pub fn make_owned(value: T) -> CHandle { let rb = Box::into_raw(Box::new(RefBox { refs: AtomicU32::new(1), value, })); - alive_inc(); CHandle { ctx: rb as *mut std::ffi::c_void, addref: Some(refbox_addref::), @@ -121,26 +83,6 @@ pub fn make_owned(value: T) -> CHandle { } } -/// Borrowed handle for an object owned elsewhere. -/// -/// # Safety -/// Caller guarantees `ptr` outlives every derived handle. -pub unsafe fn make_borrowed(ptr: *mut T) -> CHandle { - if ptr.is_null() { - return CHandle::null(); - } - let rb = Box::into_raw(Box::new(RefBox { - refs: AtomicU32::new(1), - value: unsafe { std::ptr::read(ptr) }, - })); - CHandle { - ctx: rb as *mut std::ffi::c_void, - addref: Some(refbox_addref::), - release: Some(refbox_release_borrowed::), - abi_version: OAKRENDER_ABI_VERSION, - } -} - /// Typed view into a handle; `None` for empty handles. /// /// # Safety @@ -165,44 +107,6 @@ pub unsafe fn get_mut(h: &CHandle) -> Option<&mut T> { unsafe { Some(&mut (*(h.ctx as *mut RefBox)).value) } } -/// A boxed handle that does **not** participate in the live-object count -/// (mirrors the C++ borrowed `make_handle(…, owns=false)` boxes): the -/// release only frees the box and its value, never a foreign object. -pub fn make_borrowed_owned(value: T) -> CHandle { - let rb = Box::into_raw(Box::new(RefBox { - refs: AtomicU32::new(1), - value, - })); - CHandle { - ctx: rb as *mut std::ffi::c_void, - addref: Some(refbox_addref::), - release: Some(refbox_release_borrowed::), - abi_version: OAKRENDER_ABI_VERSION, - } -} - -/// Panic-catching FFI wrapper for i32-returning exports. -pub fn guard crate::error::Result<()>>(f: F) -> i32 { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(())) => crate::error::OAKRENDER_OK, - Ok(Err(e)) => e.code(), - Err(_) => crate::error::OAKRENDER_E_FAILED, - } -} - -/// Panic-catching FFI wrapper for handle-returning exports. -pub fn guard_handle crate::error::Result>(f: F) -> CHandle { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(h)) => h, - Ok(Err(_)) | Err(_) => CHandle::null(), - } -} - -/// Panic-catching FFI wrapper for void exports. -pub fn guard_void(f: F) { - let _ = catch_unwind(AssertUnwindSafe(f)); -} - #[cfg(test)] mod tests { use super::*; @@ -215,12 +119,10 @@ mod tests { let h = make_owned(Obj(7)); assert!(!h.is_null()); assert_eq!(h.abi_version, OAKRENDER_ABI_VERSION); - let before = alive_count(); // addref/release through the stored function pointers. unsafe { h.addref.unwrap()(h.ctx) }; unsafe { h.release.unwrap()(h.ctx) }; unsafe { h.release.unwrap()(h.ctx) }; - assert_eq!(alive_count(), before - 1); } #[test] @@ -232,47 +134,6 @@ mod tests { unsafe { h.release.unwrap()(h.ctx) }; } - #[test] - fn borrowed_release_does_not_count() { - let mut obj = Obj(5); - let before = alive_count(); - let h = unsafe { make_borrowed(&mut obj) }; - assert!(!h.is_null()); - assert_eq!(alive_count(), before, "borrowed boxes are not counted"); - unsafe { h.release.unwrap()(h.ctx) }; - assert_eq!(alive_count(), before); - // The borrowed value is intact (never dropped). - assert_eq!(obj, Obj(5)); - } - - #[test] - fn guard_maps_results() { - assert_eq!(guard(|| Ok(())), 0); - assert_eq!(guard(|| Err(crate::error::Error::Invalid)), -70001); - assert_eq!(guard(|| panic!("boom")), -70003); - } - - #[test] - fn guard_handle_and_void_panic_safety() { - // Panics map to empty handles / are swallowed. - let h = guard_handle(|| panic!("boom")); - assert!(h.is_null()); - let h = guard_handle(|| Err(crate::error::Error::State)); - assert!(h.is_null()); - let h = guard_handle(|| Ok(make_owned(Obj(1)))); - assert!(!h.is_null()); - unsafe { h.release.unwrap()(h.ctx) }; - - guard_void(|| panic!("swallowed")); - guard_void(|| {}); - } - - #[test] - fn make_borrowed_null_yields_empty() { - let h = unsafe { make_borrowed::(std::ptr::null_mut()) }; - assert!(h.is_null()); - } - #[test] fn get_mut_mutates_boxed_value() { let h = make_owned(Obj(3)); @@ -283,18 +144,4 @@ mod tests { } unsafe { h.release.unwrap()(h.ctx) }; } - - #[test] - fn make_borrowed_owned_does_not_count() { - let before = alive_count(); - let h = make_borrowed_owned(Obj(4)); - assert_eq!( - alive_count(), - before, - "borrowed-owned boxes are not counted" - ); - assert!(!h.is_null()); - unsafe { h.release.unwrap()(h.ctx) }; - assert_eq!(alive_count(), before); - } } diff --git a/crates/oakrender/tests/common/mod.rs b/crates/oakrender/tests/common/mod.rs index 1521292b5..a3e2dc05e 100644 --- a/crates/oakrender/tests/common/mod.rs +++ b/crates/oakrender/tests/common/mod.rs @@ -45,17 +45,6 @@ impl Drop for ManagerGuard { } } -/// A non-null fake handle (ctx only — the ABI functions that accept -/// borrowed handles only check `ctx` in this pass). -pub fn fake_handle(seed: usize) -> oakrender::handle::CHandle { - oakrender::handle::CHandle { - ctx: seed as *mut std::ffi::c_void, - addref: None, - release: None, - abi_version: oakrender::handle::OAKRENDER_ABI_VERSION, - } -} - // --------------------------------------------------------------------------- // Host-symbol stand-ins (oakcore_* / fb_find_best_pix_fmt_of_list) // --------------------------------------------------------------------------- diff --git a/crates/oakrender/tests/copier_test.rs b/crates/oakrender/tests/copier_test.rs index f17752e14..2bc64b36a 100644 --- a/crates/oakrender/tests/copier_test.rs +++ b/crates/oakrender/tests/copier_test.rs @@ -15,13 +15,11 @@ // along with this program. If not, see . //! Copier + autocacher contract tests (the former render→node -//! coupling, now C ABI clients). +//! coupling, now direct Rust calls). //! -//! The oaknode C ABI (project deep-copy / sync) is a concurrent -//! dependency; success-path tests are `#[ignore]`d and the error paths -//! run without liboaknode. - -mod common; +//! The oaknode deep-copy direction is unimplemented (single-lib plan +//! §4.1 — dead direction), so the success paths fail explainably and the +//! tests assert those failures. use std::sync::Arc; use std::time::Duration; @@ -53,33 +51,38 @@ fn cacher() -> (oakrender::autocacher::PreviewAutoCacher, WorkerPool) { (oakrender::autocacher::PreviewAutoCacher::new(arena), pool) } -/// deep_copy through the C ABI: the render-side copy evaluates -/// identically to the source project for a fixture graph (comparison -/// via the oaknode evaluation C ABI). +/// Deep-copy through oaknode with a valid (non-empty) project identity: +/// the direction is unimplemented (single-lib plan §4.1 — dead +/// direction), so the copier fails explainably — the live path every +/// caller sees today. #[test] -#[ignore = "needs oaknode C ABI (oaknode_project_deep_copy)"] -fn deep_copy_evaluates_identically() { +fn deep_copy_with_valid_identity_fails_explainably() { let mut copier = oakrender::copier::ProjectCopy::new(); - let src = common::fake_handle(7); - copier.set_project(src).unwrap(); - assert_ne!(copier.copy, 0); - assert!(copier.copied_project().is_some()); + let src = oakrender::copier::ProjectHandle::new(7); + assert_eq!( + copier.set_project(src).unwrap_err().code(), + Error::Failed(String::new()).code() + ); + assert_eq!(copier.copy, 0); + assert!(copier.copied_project().is_none()); } -/// sync applies recorded changes; the copy matches a fresh deep_copy -/// afterwards. +/// sync requires an established copy; with the deep-copy direction dead, +/// no copy can ever be attached, so sync always reports the state error. #[test] -#[ignore = "needs oaknode C ABI (oaknode_project_sync_copy)"] -fn sync_matches_fresh_copy() { +fn sync_without_established_copy_fails() { let mut copier = oakrender::copier::ProjectCopy::new(); - let src = common::fake_handle(7); - copier.set_project(src).unwrap(); + let src = oakrender::copier::ProjectHandle::new(7); + assert!(copier.set_project(src).is_err()); let changes = [oakrender::copier::ChangeRecord { kind: oakrender::copier::change_kind::NODE_ADD, payload: [0u8; 48], }]; - copier.sync(&changes).unwrap(); - assert_eq!(copier.last_sync_generation, 1); + assert_eq!( + copier.sync(&changes).unwrap_err().code(), + Error::State.code() + ); + assert_eq!(copier.last_sync_generation, 0); } /// Autocacher attach/detach: requests on the copied project's caches @@ -153,13 +156,13 @@ fn change_record_marshalling() { } } -/// Copier failure paths without liboaknode. +/// Copier failure paths (the deep-copy direction is dead). #[test] fn copier_error_paths() { let mut copier = oakrender::copier::ProjectCopy::new(); assert_eq!( copier - .set_project(oakrender::handle::CHandle::null()) + .set_project(oakrender::copier::ProjectHandle::null()) .unwrap_err() .code(), Error::Invalid.code() diff --git a/crates/oakstorage/README.md b/crates/oakstorage/README.md index 7fe98ed28..118aa5479 100644 --- a/crates/oakstorage/README.md +++ b/crates/oakstorage/README.md @@ -53,7 +53,7 @@ Consumers never branch on backend. src/ lib.rs crate doc + module map error.rs error/info codes (M10 §2.1, -MMCCCC module 10) - handle.rs refcounted-handle scaffolding (shared oakcore CHandle) + handle.rs shared CHandle re-export (boxes live in oaknode's handle.rs) uri.rs URI parsing/classification session.rs StorageProject session (open/take/uri) registry.rs backend registry (register/unregister/arbitrate) diff --git a/crates/oakstorage/src/backends/database/mod.rs b/crates/oakstorage/src/backends/database/mod.rs index f02ec3a45..da1940e03 100644 --- a/crates/oakstorage/src/backends/database/mod.rs +++ b/crates/oakstorage/src/backends/database/mod.rs @@ -78,6 +78,7 @@ use oaknode::project::Project; use crate::backend::{LoadResult, StorageBackend}; use crate::error::{Error, Result}; use crate::handle::CHandle; +use crate::nodeutil::ProjectArc; use crate::uri::StorageUri; /// Journal kind for redo commands (the D1 save path). @@ -613,15 +614,9 @@ impl DatabaseBackend { if file_uri.scheme != "file" { return Err(Error::Invalid); } - let handle = self.load_handle_by_uuid(uri, uuid)?; + let project = self.load_project_by_uuid(uri, uuid)?; let backend = crate::backends::ove_xml::OveXmlBackend::new(); - let result = backend.save(handle, file_uri, 0); - // The loaded handle is ours (refcount 1); release it regardless - // of the save outcome. - if let Some(release) = handle.release { - unsafe { release(handle.ctx) }; - } - result + backend.save_project(&project, file_uri, 0) } /// Import a `.ove`/`.otio`/`.fcpxml` file as a new library row @@ -634,6 +629,9 @@ impl DatabaseBackend { return Err(Error::Invalid); } let backend = crate::registry::Registry::global().resolve(file_uri)?; + // The file backend's `load` returns its project as a handle (the + // facade-facing trait form); convert it to the boxed project the + // crate uses internally, and release the temporary handle. let result = backend.load(file_uri)?; let handle = result.project; if handle.is_null() { @@ -643,12 +641,15 @@ impl DatabaseBackend { ))); } let uuid = { - let arc = unsafe { crate::nodeutil::project_arc(&handle)? }; let fresh = new_uuid(); - arc.lock().map_err(|_| Error::State)?.uuid = fresh.clone(); + unsafe { crate::nodeutil::project_arc(&handle)? } + .lock() + .map_err(|_| Error::State)? + .uuid = fresh.clone(); fresh }; - let outcome = self.save(handle, uri, 0).map(|()| uuid.clone()); + let arc = unsafe { crate::nodeutil::project_arc(&handle)? }; + let outcome = self.save_project(&arc, uri, 0).map(|()| uuid.clone()); if let Some(release) = handle.release { unsafe { release(handle.ctx) }; } @@ -683,11 +684,14 @@ impl DatabaseBackend { /// persistent undo history (plan §0): a snapshot at or before `seq` /// is replayed forward with the journal rows up to `seq`. `seq` 0 is /// the empty project. E_INVALID when `seq` is out of range. + /// + /// Returns the project as an owned oaknode handle (the facade-facing + /// form — the engine's integration tests consume it this way). pub fn load_at(&self, uri: &StorageUri, uuid: &str, seq: i64) -> Result { let target = parse_target(uri)?; let key = db_key_of(&target); let uuid = uuid.to_string(); - self.run(key, move |conn| async move { + let project = self.run(key, move |conn| async move { let model = project::Entity::find() .filter(project::Column::Uuid.eq(&uuid)) .one(&conn) @@ -698,30 +702,22 @@ impl DatabaseBackend { return Err(Error::Invalid); } let xml = assemble_at(&conn, model.id, &model.uuid, seq).await?; - Ok(crate::nodeutil::make_project_owned( - crate::nodeutil::serializer_load(&xml)?, - )) - }) + crate::nodeutil::serializer_load(&xml) + })?; + Ok(crate::nodeutil::make_project_owned(project)) } /// The manager stats of a library project (plan §4): the head state /// is replayed and the stats derived from the node graph. pub fn project_stats(&self, uri: &StorageUri, uuid: &str) -> Result { - let handle = self.load_handle_by_uuid(uri, uuid)?; - let stats = (|| -> Result { - let arc = unsafe { crate::nodeutil::project_arc(&handle)? }; - let guard = arc.lock().map_err(|_| Error::State)?; - Ok(derive_stats(&guard)) - })(); - if let Some(release) = handle.release { - unsafe { release(handle.ctx) }; - } - stats + let project = self.load_project_by_uuid(uri, uuid)?; + let guard = project.lock().map_err(|_| Error::State)?; + Ok(derive_stats(&guard)) } - /// Load the project payload of the library row `uuid` as an owned - /// handle (the head state). - fn load_handle_by_uuid(&self, uri: &StorageUri, uuid: &str) -> Result { + /// Load the project payload of the library row `uuid` as the boxed + /// project (the head state). + fn load_project_by_uuid(&self, uri: &StorageUri, uuid: &str) -> Result { let target = parse_target(uri)?; let key = db_key_of(&target); let uuid = uuid.to_string(); @@ -733,11 +729,50 @@ impl DatabaseBackend { .map_err(db_err)? .ok_or(Error::NotFound)?; let xml = assemble_at(&conn, model.id, &model.uuid, model.command_seq).await?; - Ok(crate::nodeutil::make_project_owned( - crate::nodeutil::serializer_load(&xml)?, - )) + crate::nodeutil::serializer_load(&xml) }) } + + /// Load the head state of the library's selected project (the + /// `?project=` uuid, or the most recently modified row) as the boxed + /// project. The Rust-typed inner load: the facade-facing trait `load` + /// wraps the result in an owned handle. + pub fn load_project(&self, uri: &StorageUri) -> Result { + let target = parse_target(uri)?; + let key = db_key_of(&target); + let project = target.project().map(str::to_string); + self.run(key, move |conn| async move { + let model = pick_project(&conn, project.as_deref()).await?; + let xml = assemble_at(&conn, model.id, &model.uuid, model.command_seq).await?; + crate::nodeutil::serializer_load(&xml) + }) + } + + /// Save a project (already read out of its handle) to the library, + /// journaling the diff. The Rust-typed inner save: the facade-facing + /// trait `save` converts the project handle and forwards here. + pub fn save_project( + &self, + project: &ProjectArc, + uri: &StorageUri, + _options: u32, + ) -> Result<()> { + let target = parse_target(uri)?; + let key = db_key_of(&target); + // Serialize under the project lock (the same per-node writer the + // `.ove` backend uses — one serialization truth). + let (uuid, name, nodes, settings_xml, settings_map) = { + let guard = project.lock().map_err(|_| Error::State)?; + let uuid = guard.uuid.clone(); + let name = project_display_name(&guard); + let (nodes, settings_xml, settings_map) = serialize_project_state(&guard)?; + (uuid, name, nodes, settings_xml, settings_map) + }; + self.run(key, move |conn| async move { + save_tx(&conn, &uuid, &name, &nodes, &settings_xml, &settings_map).await + })?; + Ok(()) + } } impl Default for DatabaseBackend { @@ -763,35 +798,17 @@ impl StorageBackend for DatabaseBackend { } fn load(&self, uri: &StorageUri) -> Result { - let target = parse_target(uri)?; - let key = db_key_of(&target); - let project = target.project().map(str::to_string); - self.run(key, move |conn| async move { - let model = pick_project(&conn, project.as_deref()).await?; - let xml = assemble_at(&conn, model.id, &model.uuid, model.command_seq).await?; - let handle = - crate::nodeutil::make_project_owned(crate::nodeutil::serializer_load(&xml)?); - Ok(LoadResult::success(handle)) - }) + // Facade boundary: box the loaded project into an owned handle. + Ok(LoadResult::success(crate::nodeutil::make_project_owned( + self.load_project(uri)?, + ))) } - fn save(&self, project: CHandle, uri: &StorageUri, _options: u32) -> Result<()> { - let target = parse_target(uri)?; - let key = db_key_of(&target); - // Serialize under the project lock (the same per-node writer the - // `.ove` backend uses — one serialization truth). + fn save(&self, project: CHandle, uri: &StorageUri, options: u32) -> Result<()> { + // Facade boundary: convert the project handle to the boxed + // project, then run the Rust-typed save. let arc = unsafe { crate::nodeutil::project_arc(&project)? }; - let (uuid, name, nodes, settings_xml, settings_map) = { - let guard = arc.lock().map_err(|_| Error::State)?; - let uuid = guard.uuid.clone(); - let name = project_display_name(&guard); - let (nodes, settings_xml, settings_map) = serialize_project_state(&guard)?; - (uuid, name, nodes, settings_xml, settings_map) - }; - self.run(key, move |conn| async move { - save_tx(&conn, &uuid, &name, &nodes, &settings_xml, &settings_map).await - })?; - Ok(()) + self.save_project(&arc, uri, options) } } diff --git a/crates/oakstorage/src/backends/otio.rs b/crates/oakstorage/src/backends/otio.rs index c10184d09..ef0563b3e 100644 --- a/crates/oakstorage/src/backends/otio.rs +++ b/crates/oakstorage/src/backends/otio.rs @@ -67,6 +67,7 @@ use oaknode::track::{TrackBehavior, TrackListBehavior, TrackType}; use crate::backend::LoadResult; use crate::nodeutil as node; +use crate::nodeutil::ProjectArc; use crate::error::{Error, Result}; use crate::uri::StorageUri; @@ -81,6 +82,46 @@ impl OtioBackend { pub fn new() -> Self { OtioBackend } + + /// Save a project (already read out of its handle) to the URI. The + /// Rust-typed inner path: the facade-facing trait `save` converts the + /// project handle and forwards here. + pub fn save_project( + &self, + project: &ProjectArc, + uri: &StorageUri, + _options: u32, + ) -> Result<()> { + let path = uri.local_path().ok_or(Error::Invalid)?.to_string(); + let ext = uri.extension().ok_or(Error::Invalid)?; + let guard = project.lock().map_err(|_| Error::State)?; + let timelines = project_to_timelines(&guard); + + match ext.as_str() { + "otio" => { + let root = if timelines.len() == 1 { + Serializable::Timeline(timelines.into_iter().next().unwrap()) + } else if timelines.is_empty() { + // Nothing to export; a single empty timeline is the + // friendliest shape for a fresh import. + Serializable::Timeline(Timeline::new("Timeline")) + } else { + let children: Vec = timelines + .into_iter() + .map(Serializable::Timeline) + .collect(); + Serializable::SerializableCollection(SerializableCollection::new( + "oak", + children, + )) + }; + root.to_json_file(&path).map_err(|e| Error::Io(e.to_string())) + } + "fcpxml" => oakotio::to_fcpxml_file(&timelines, &path) + .map_err(|e| Error::Io(e.to_string())), + _ => Err(Error::Invalid), + } + } } impl Default for OtioBackend { @@ -149,38 +190,12 @@ impl crate::backend::StorageBackend for OtioBackend { &self, project: crate::handle::CHandle, uri: &StorageUri, - _options: u32, + options: u32, ) -> Result<()> { - let path = uri.local_path().ok_or(Error::Invalid)?.to_string(); - let ext = uri.extension().ok_or(Error::Invalid)?; + // Facade boundary: convert the project handle to the boxed + // project, then run the Rust-typed save. let arc = unsafe { node::project_arc(&project)? }; - let guard = arc.lock().map_err(|_| Error::State)?; - let timelines = project_to_timelines(&guard); - - match ext.as_str() { - "otio" => { - let root = if timelines.len() == 1 { - Serializable::Timeline(timelines.into_iter().next().unwrap()) - } else if timelines.is_empty() { - // Nothing to export; a single empty timeline is the - // friendliest shape for a fresh import. - Serializable::Timeline(Timeline::new("Timeline")) - } else { - let children: Vec = timelines - .into_iter() - .map(Serializable::Timeline) - .collect(); - Serializable::SerializableCollection(SerializableCollection::new( - "oak", - children, - )) - }; - root.to_json_file(&path).map_err(|e| Error::Io(e.to_string())) - } - "fcpxml" => oakotio::to_fcpxml_file(&timelines, &path) - .map_err(|e| Error::Io(e.to_string())), - _ => Err(Error::Invalid), - } + self.save_project(&arc, uri, options) } } diff --git a/crates/oakstorage/src/backends/ove_xml.rs b/crates/oakstorage/src/backends/ove_xml.rs index 06036493d..5f442af79 100644 --- a/crates/oakstorage/src/backends/ove_xml.rs +++ b/crates/oakstorage/src/backends/ove_xml.rs @@ -32,6 +32,7 @@ use crate::backend::LoadResult; use crate::error::{Error, OAKSTORAGE_OK, OAKSTORAGE_TOO_NEW, OAKSTORAGE_TOO_OLD, OAKSTORAGE_UNKNOWN_VERSION}; +use crate::nodeutil::ProjectArc; use crate::uri::StorageUri; use oaknode::serializer::XmlRead; @@ -82,6 +83,29 @@ impl OveXmlBackend { pub fn new() -> Self { OveXmlBackend } + + /// Save a project (already read out of its handle) to the URI. The + /// Rust-typed inner path: the facade-facing trait `save` converts the + /// project handle and forwards here. + pub fn save_project( + &self, + project: &ProjectArc, + uri: &StorageUri, + _options: u32, + ) -> crate::error::Result<()> { + let path = uri + .local_path() + .ok_or(Error::Invalid)? + .to_string(); + let xml = { + let guard = project + .lock() + .map_err(|_| Error::State)?; + crate::nodeutil::serializer_save(&guard)? + }; + std::fs::write(&path, xml).map_err(|e| Error::Io(e.to_string()))?; + Ok(()) + } } impl Default for OveXmlBackend { @@ -135,20 +159,11 @@ impl crate::backend::StorageBackend for OveXmlBackend { &self, project: crate::handle::CHandle, uri: &StorageUri, - _options: u32, + options: u32, ) -> crate::error::Result<()> { - let path = uri - .local_path() - .ok_or(Error::Invalid)? - .to_string(); + // Facade boundary: convert the project handle to the boxed + // project, then run the Rust-typed save. let arc = unsafe { crate::nodeutil::project_arc(&project)? }; - let xml = { - let guard = arc - .lock() - .map_err(|_| Error::State)?; - crate::nodeutil::serializer_save(&guard)? - }; - std::fs::write(&path, xml).map_err(|e| Error::Io(e.to_string()))?; - Ok(()) + self.save_project(&arc, uri, options) } } diff --git a/crates/oakstorage/src/handle.rs b/crates/oakstorage/src/handle.rs index 3d2eb2cfd..81a1a0b07 100644 --- a/crates/oakstorage/src/handle.rs +++ b/crates/oakstorage/src/handle.rs @@ -14,164 +14,16 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -//! Refcounted-handle scaffolding (same pattern as the other crates; -//! duplicated on purpose — handle function pointers must run code from -//! the creating DLL). +//! The `CHandle` ABI value-handle type (single-lib unification, see +//! `docs/zh/plans/riir/single-lib.md`): one canonical +//! `{ctx, addref, release, abi_version}` type in `oakcore-rs`, re-exported +//! so the crate's signatures stay source-compatible with the shared type. //! -//! `CHandle` is the canonical shared ABI value-handle type from -//! `oakcore-rs` (single-lib unification, see -//! `docs/zh/plans/riir/single-lib.md`), so a handle returned by -//! `oakstorage_*` is structurally interchangeable with one created by -//! `oaknode_*`: `oakstorage_save` consumes an `OakNodeProject*` handle -//! and `oakstorage_project_take_project` hands one back. - -use std::any::Any; -use std::panic::{catch_unwind, AssertUnwindSafe}; -use std::sync::atomic::{AtomicU32, Ordering}; +//! M14 R5: oakstorage never creates its own boxes — every project handle +//! it produces goes through `oaknode::handle` (see [`crate::nodeutil`]), +//! so the per-DLL RefBox machinery is not needed here and was removed; +//! only the shared `CHandle` type remains, for the facade-facing surface +//! (project handles in [`crate::backend::LoadResult`] and the +//! `StorageBackend` trait, the write-through binding entries). pub use oakcore_rs::handle::CHandle; - -use crate::error::OAKSTORAGE_E_FAILED; - -/// ABI version stamped into every handle. -pub const OAKSTORAGE_ABI_VERSION: u32 = 1; - -/// Heap box behind a handle's `ctx`. -pub struct RefBox { - /// Atomic reference count. - pub refs: AtomicU32, - /// Boxed value. - pub value: T, -} - -/// addref implementation: atomic +1 (owned and borrowed handles alike — -/// a borrow only extends the box's life, not the borrowed object's). -unsafe extern "C" fn refbox_addref(ctx: *mut std::ffi::c_void) { - unsafe { - let rb = ctx as *const RefBox; - // The caller guarantees the handle is live for the borrow period. - (*rb).refs.fetch_add(1, Ordering::Relaxed); - } -} - -/// release implementation (owned): atomic -1; at zero, reclaim the box -/// and destroy the contained value. -unsafe extern "C" fn refbox_release_owned(ctx: *mut std::ffi::c_void) { - unsafe { - let rb = ctx as *mut RefBox; - // AcqRel: the zeroing side sees every write that preceded the - // last reference (including the state the destructor needs). - if (*rb).refs.fetch_sub(1, Ordering::AcqRel) == 1 { - drop(Box::from_raw(rb)); - } - } -} - -/// release implementation (borrowed, from [`make_borrowed`]): at zero, -/// free only the box memory and forget the contained value — ownership -/// stays with the borrowing side. -unsafe extern "C" fn refbox_release_borrowed(ctx: *mut std::ffi::c_void) { - unsafe { - let rb = ctx as *mut RefBox; - if (*rb).refs.fetch_sub(1, Ordering::AcqRel) == 1 { - // Partial move: move the value out of the temporary Box (its - // destructor then only frees the allocation); `forget` skips - // the value's destructor (double-free defense). - std::mem::forget((Box::from_raw(rb)).value); - } - } -} - -/// Owned handle with count 1; empty on allocation failure. -pub fn make_owned(value: T) -> CHandle { - let rb = Box::into_raw(Box::new(RefBox { - refs: AtomicU32::new(1), - value, - })); - CHandle { - ctx: rb as *mut std::ffi::c_void, - addref: Some(refbox_addref::), - release: Some(refbox_release_owned::), - abi_version: OAKSTORAGE_ABI_VERSION, - } -} - -/// Owned handle with count 1 and a caller-provided release routine -/// (used by the ffi layer's alive-counted session boxes, whose release -/// must also update the debug counter). -pub fn make_owned_with( - value: T, - release: unsafe extern "C" fn(*mut std::ffi::c_void), -) -> CHandle { - let rb = Box::into_raw(Box::new(RefBox { - refs: AtomicU32::new(1), - value, - })); - CHandle { - ctx: rb as *mut std::ffi::c_void, - addref: Some(refbox_addref::), - release: Some(release), - abi_version: OAKSTORAGE_ABI_VERSION, - } -} - -/// Borrowed handle for an object owned elsewhere (release frees only -/// the box). -/// -/// Semantics: bitwise copy ("borrowed copy"); the borrowed object's -/// destructor is entirely the caller's responsibility — the box never -/// touches it. -/// -/// # Safety -/// Caller guarantees `ptr` outlives every derived handle, and that its -/// value is not moved or destroyed for the borrow's lifetime. -pub unsafe fn make_borrowed(ptr: *mut T) -> CHandle { - if ptr.is_null() { - return CHandle::null(); - } - let rb = Box::into_raw(Box::new(RefBox { - refs: AtomicU32::new(1), - value: unsafe { std::ptr::read(ptr) }, - })); - CHandle { - ctx: rb as *mut std::ffi::c_void, - addref: Some(refbox_addref::), - release: Some(refbox_release_borrowed::), - abi_version: OAKSTORAGE_ABI_VERSION, - } -} - -/// Typed view into a handle; `None` for empty handles. -/// -/// # Safety -/// `T` must be the boxed type. -pub unsafe fn get(h: &CHandle) -> Option<&T> { - if h.ctx.is_null() { - return None; - } - unsafe { Some(&(*(h.ctx as *const RefBox)).value) } -} - -/// Panic-catching FFI wrapper for i32-returning exports. -/// -/// Panics map to [`OAKSTORAGE_E_FAILED`]. -pub fn guard crate::error::Result<()>>(f: F) -> i32 { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(())) => crate::error::OAKSTORAGE_OK, - Ok(Err(e)) => e.code(), - Err(_) => OAKSTORAGE_E_FAILED, - } -} - -/// Panic-catching FFI wrapper for handle-returning exports. -pub fn guard_handle crate::error::Result>(f: F) -> CHandle { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(h)) => h, - Ok(Err(_)) | Err(_) => CHandle::null(), - } -} - -/// Panic-catching FFI wrapper for void exports. -pub fn guard_void(f: F) { - let _ = catch_unwind(AssertUnwindSafe(f)); -} diff --git a/crates/oakstorage/src/nodeutil.rs b/crates/oakstorage/src/nodeutil.rs index f2bb912ed..ed2000c49 100644 --- a/crates/oakstorage/src/nodeutil.rs +++ b/crates/oakstorage/src/nodeutil.rs @@ -19,8 +19,11 @@ //! //! The ove-xml/otio backends need oaknode's project + serializer //! families. Graph (de)serialization stays oaknode's own serializer -//! (`load`/`save`); these helpers box the resulting `Arc>` -//! into the canonical handle form the storage session API moves around. +//! (`load`/`save`); these helpers box the resulting +//! `Arc>` into the canonical handle form only where a +//! project crosses the facade boundary (backends' `load` results and +//! `save` inputs) — inside the crate, projects travel as the plain +//! [`ProjectArc`] alias. use std::sync::{Arc, Mutex}; @@ -31,36 +34,15 @@ use crate::handle::CHandle; pub type ProjectArc = Arc>; /// Box an existing project as an owned handle (refcount 1). +/// +/// This is the facade-boundary conversion: backends hand loaded projects +/// to the caller (through [`crate::backend::LoadResult`] / the database +/// backend's `load_at`) as an oaknode project handle; everything inside +/// the crate works with the boxed [`ProjectArc`] instead. pub fn make_project_owned(project: ProjectArc) -> CHandle { oaknode::handle::make_owned(project) } -/// Release one owned reference of a project handle produced by -/// [`make_project_owned`] (or the database backend's load). The handle is -/// dead afterwards; the write-through binding keeps its own reference, so -/// releasing the caller's copy never tears a bound project down. -pub fn release_project(mut h: CHandle) { - if let Some(release) = h.release { - // SAFETY: `h` is an owned handle from this module; the release runs - // the box's own destructor once per owned reference. - unsafe { release(h.ctx) }; - } - h.ctx = std::ptr::null_mut(); -} - -/// The boxed project of a handle produced by [`make_project_owned`] or the -/// database backend's load (both box a `ProjectArc`). `None` for an empty -/// handle. -/// -/// The handle ABI carries no type tag, so the caller must only pass handles -/// from those two producers; the unsafe downcast is contained here instead -/// of spread across every direct-rlib consumer. -pub fn project_arc_of(h: &CHandle) -> Option { - // SAFETY: the documented precondition — handles from this module's - // producers box a `ProjectArc`. - unsafe { oaknode::handle::get::(h) }.cloned() -} - /// Read the boxed project of a project handle. /// /// # Safety @@ -72,6 +54,19 @@ pub unsafe fn project_arc(h: &CHandle) -> Result { .ok_or(crate::error::Error::Invalid) } +/// Release a project handle created by [`make_project_owned`] (its box's +/// release callback drops one reference). NULL is a no-op. +pub fn release_project(h: CHandle) { + if h.is_null() { + return; + } + if let Some(release) = h.release { + // SAFETY: `h` was produced by `make_project_owned`; the callback + // owns the box and nulls nothing else. + unsafe { release(h.ctx) }; + } +} + /// Load a project from XML text via the oaknode serializer. pub fn serializer_load(xml: &str) -> Result { oaknode::serializer::load(xml).map_err(|e| crate::error::Error::Format(e.to_string())) diff --git a/crates/oakstorage/src/session.rs b/crates/oakstorage/src/session.rs index 73f3aa095..a2e44fcc5 100644 --- a/crates/oakstorage/src/session.rs +++ b/crates/oakstorage/src/session.rs @@ -15,29 +15,31 @@ // along with this program. If not, see . //! The project session (M10 §2.2 `OakStorageProject`): wraps a loaded -//! project plus its source URI. `take` transfers the project handle -//! out, leaving an empty shell that must still be freed. +//! project plus its source URI. `take` transfers the project out, leaving +//! an empty shell that must still be freed. +//! +//! M14 R5: the session holds the boxed project directly +//! ([`crate::nodeutil::ProjectArc`]) instead of a `CHandle` — the handle +//! form is only produced at the backend `load` boundary +//! ([`crate::backend::LoadResult`]); `open` converts it before wrapping. -use crate::handle::CHandle; +use crate::nodeutil::ProjectArc; use crate::uri::StorageUri; /// An open project session. pub struct Session { /// Source URI. uri: StorageUri, - /// The project handle (None after [`Session::take`]). - project: Option, + /// The boxed project (None after [`Session::take`]). + project: Option, } impl Session { - /// Wrap a freshly loaded project. A null handle (the version-info - /// path: TOO_OLD/TOO_NEW/UNKNOWN_VERSION carries no project) maps to - /// `None`, not `Some(null)`. - pub fn new(uri: StorageUri, project: CHandle) -> Self { - Session { - uri, - project: (!project.is_null()).then_some(project), - } + /// Wrap a freshly loaded project. `None` (the version-info path: + /// TOO_OLD/TOO_NEW/UNKNOWN_VERSION carries no project) leaves an empty + /// session. + pub fn new(uri: StorageUri, project: Option) -> Self { + Session { uri, project } } /// Source URI. @@ -45,27 +47,14 @@ impl Session { &self.uri } - /// Borrowed project handle (None after take). - pub fn project(&self) -> Option<&CHandle> { + /// Borrowed project (None after take). + pub fn project(&self) -> Option<&ProjectArc> { self.project.as_ref() } - /// Transfer the project out (C++ take_project semantics); the - /// session becomes an empty shell, and the caller owns the returned - /// handle (release it with `oaknode_project_free`). - pub fn take(&mut self) -> Option { + /// Transfer the project out (C++ take_project semantics); the session + /// becomes an empty shell, and the caller owns the returned reference. + pub fn take(&mut self) -> Option { self.project.take() } } - -impl Drop for Session { - fn drop(&mut self) { - // Release the still-held project handle (the `take` path already - // removed it). - if let Some(h) = self.project.take() { - if let Some(f) = h.release { - unsafe { f(h.ctx) }; - } - } - } -} diff --git a/crates/oakstorage/src/writethrough.rs b/crates/oakstorage/src/writethrough.rs index f12c438c7..ed01b86ce 100644 --- a/crates/oakstorage/src/writethrough.rs +++ b/crates/oakstorage/src/writethrough.rs @@ -27,20 +27,22 @@ //! //! ## Binding model //! -//! The map is keyed by the project handle's `ctx` pointer (the module -//! `RefBox` identity — one per in-memory project instance), so several -//! projects can be bound at once (multi-project, plan §3) without -//! confusing their library rows. Every undo-path operation re-saves ALL -//! bound projects ([`note_command`]): the oakstorage backend diffs each -//! project against its own library head, so untouched projects are -//! no-op touches and only the project the command actually changed -//! advances its journal. (The plan's "current project" phrasing maps to -//! this — the undo stack is cleared on every project switch, so at most -//! one project's graph changes per command; a bound-but-untouched -//! project can gain its import row this way, which reflects its true -//! state.) Closing a project ([`unbind_project`], hooked from the -//! facade's `project_free`) flushes its pending writes and drops the -//! binding. +//! The map is keyed by the project box's identity (`Arc::as_ptr` — one +//! per in-memory project instance), so several projects can be bound at +//! once (multi-project, plan §3) without confusing their library rows. +//! Every undo-path operation re-saves ALL bound projects +//! ([`note_command`]): the oakstorage backend diffs each project against +//! its own library head, so untouched projects are no-op touches and +//! only the project the command actually changed advances its journal. +//! (The plan's "current project" phrasing maps to this — the undo stack +//! is cleared on every project switch, so at most one project's graph +//! changes per command; a bound-but-untouched project can gain its +//! import row this way, which reflects its true state.) Closing a +//! project ([`unbind_project`], hooked from the facade's `project_free`) +//! flushes its pending writes and drops the binding. The binding holds +//! the boxed project ([`ProjectArc`]) directly — the `CHandle` facade +//! entries convert at the boundary — so a bound project outlives the +//! caller's own handle reference. //! //! The library is selected from the `Storage` config group (all defaults //! are config-driven, plan §5): @@ -79,27 +81,22 @@ use std::collections::HashMap; use std::panic::{catch_unwind, AssertUnwindSafe}; -use std::sync::{Condvar, Mutex, OnceLock}; +use std::sync::{Arc, Condvar, Mutex, OnceLock}; use std::time::Duration; -use oaknode::project::Project; use oakundo::global; -use crate::backend::StorageBackend; use crate::backends::database::DatabaseBackend; use crate::handle::CHandle; +use crate::nodeutil::ProjectArc; use crate::uri::StorageUri; -/// The boxed project payload behind an oaknode project handle (the -/// engine's `crate::handle::domain::ProjectArc` equivalent). -type ProjectArc = std::sync::Arc>; - /// One bound project: its session (library uri + row uuid) plus the /// write state. struct Binding { - /// The project handle (addref'd at bind, released at unbind; keeps - /// the project alive past the caller's own handle). - project: CHandle, + /// The boxed project (kept alive here past the caller's own handle + /// reference). + project: ProjectArc, /// Library uri (`oakdb+sqlite:///…`). uri: String, /// Library row uuid. @@ -123,7 +120,7 @@ pub fn backend() -> &'static DatabaseBackend { BACKEND.get_or_init(DatabaseBackend::new) } -/// project identity (handle `ctx` pointer) -> binding. +/// project identity (boxed project allocation) -> binding. fn bindings() -> &'static Mutex> { static BINDINGS: OnceLock>> = OnceLock::new(); BINDINGS.get_or_init(|| Mutex::new(HashMap::new())) @@ -167,12 +164,20 @@ fn ensure_command_observer() { /// backend is disabled by config, the project cannot be addressed (no /// uuid), or it is already bound. No database write happens here — the /// first undo-path operation creates the library row. +/// +/// Facade entry: takes the project as a `CHandle` (the engine's project +/// handle form) and converts it to the boxed project at the boundary; +/// everything from here on works with the [`ProjectArc`]. pub fn bind_project(project: CHandle) { let _ = catch_unwind(AssertUnwindSafe(|| { if project.is_null() { return; } - let key = project.ctx as usize; + // SAFETY: facade project handles box a `ProjectArc`. + let Ok(arc) = (unsafe { crate::nodeutil::project_arc(&project) }) else { + return; + }; + let key = Arc::as_ptr(&arc) as usize; { let g = bindings().lock().unwrap_or_else(|e| e.into_inner()); if g.contains_key(&key) { @@ -186,28 +191,19 @@ pub fn bind_project(project: CHandle) { return; }; let uuid = { - let arc = unsafe { oaknode::handle::get::(&project) }; - match arc { - Some(a) => a.lock().unwrap_or_else(|e| e.into_inner()).uuid.clone(), - None => return, - } + let guard = arc.lock().unwrap_or_else(|e| e.into_inner()); + guard.uuid.clone() }; if uuid.is_empty() { return; } - // Addref the handle so the binding owns a reference independent of - // the caller's. - let owned = project; - if let Some(addref) = owned.addref { - unsafe { addref(owned.ctx) }; - } bindings() .lock() .unwrap_or_else(|e| e.into_inner()) .insert( key, Binding { - project: owned, + project: arc, uri, uuid, dirty: false, @@ -224,14 +220,15 @@ pub fn bind_project(project: CHandle) { /// project). No-op when not bound. pub fn unbind_project(project: CHandle) { let _ = catch_unwind(AssertUnwindSafe(|| { - let key = project.ctx as usize; + // SAFETY: facade project handles box a `ProjectArc`. + let Ok(arc) = (unsafe { crate::nodeutil::project_arc(&project) }) else { + return; + }; + let key = Arc::as_ptr(&arc) as usize; flush_one(key); let mut g = bindings().lock().unwrap_or_else(|e| e.into_inner()); - if let Some(b) = g.remove(&key) { - if let Some(release) = b.project.release { - unsafe { release(b.project.ctx) }; - } - } + // Dropping the binding releases the project reference. + g.remove(&key); })); } @@ -240,10 +237,14 @@ pub fn is_bound(project: CHandle) -> bool { if project.is_null() { return false; } + // SAFETY: facade project handles box a `ProjectArc`. + let Ok(arc) = (unsafe { crate::nodeutil::project_arc(&project) }) else { + return false; + }; bindings() .lock() .unwrap_or_else(|e| e.into_inner()) - .contains_key(&(project.ctx as usize)) + .contains_key(&(Arc::as_ptr(&arc) as usize)) } /// The last write-through / snapshot error of `project` (empty when none @@ -252,10 +253,14 @@ pub fn last_error(project: CHandle) -> Option { if project.is_null() { return None; } + // SAFETY: facade project handles box a `ProjectArc`. + let Ok(arc) = (unsafe { crate::nodeutil::project_arc(&project) }) else { + return None; + }; bindings() .lock() .unwrap_or_else(|e| e.into_inner()) - .get(&(project.ctx as usize)) + .get(&(Arc::as_ptr(&arc) as usize)) .and_then(|b| b.last_error.clone()) } @@ -286,7 +291,7 @@ fn write_through(key: usize) { let (project, uri, _uuid) = { let mut g = bindings().lock().unwrap_or_else(|e| e.into_inner()); match g.get_mut(&key) { - Some(b) => (b.project, b.uri.clone(), b.uuid.clone()), + Some(b) => (b.project.clone(), b.uri.clone(), b.uuid.clone()), None => return, } }; @@ -297,7 +302,7 @@ fn write_through(key: usize) { return; } }; - match backend().save(project, &parsed, 0) { + match backend().save_project(&project, &parsed, 0) { Ok(()) => { let mut g = bindings().lock().unwrap_or_else(|e| e.into_inner()); if let Some(b) = g.get_mut(&key) { diff --git a/crates/oakstorage/tests/storage_test.rs b/crates/oakstorage/tests/storage_test.rs index 636b2b9c7..7ebac30a5 100644 --- a/crates/oakstorage/tests/storage_test.rs +++ b/crates/oakstorage/tests/storage_test.rs @@ -104,7 +104,14 @@ fn open(uri: &str) -> oakstorage::error::Result<(Session, i32)> { let parsed = StorageUri::parse(uri)?; let backend = Registry::global().resolve(&parsed)?; let result = backend.load(&parsed)?; - let session = Session::new(parsed, result.project); + // The backend hands the project back as a handle (the facade-facing + // form); the session stores the boxed project directly. + let project = if result.project.is_null() { + None + } else { + Some(unsafe { project_arc(&result.project) }.unwrap()) + }; + let session = Session::new(parsed, project); Ok((session, result.version_info)) } @@ -275,8 +282,7 @@ fn ove_xml_roundtrip_field_by_field() { assert_eq!(rc, OAKSTORAGE_OK); assert_eq!(session.uri().to_uri_string(), uri); - let proj_handle = session.project().cloned().unwrap(); - let loaded = unsafe { project_arc(&proj_handle) }.unwrap(); + let loaded = session.project().cloned().unwrap(); { let o = project.lock().unwrap(); let l = loaded.lock().unwrap(); @@ -301,8 +307,7 @@ fn ove_xml_compress_flag_still_round_trips() { let (session, rc) = open(&uri).unwrap(); assert!(session.project().is_some(), "open failed rc={rc}"); assert_eq!(rc, OAKSTORAGE_OK); - let proj_handle = session.project().cloned().unwrap(); - let loaded = unsafe { project_arc(&proj_handle) }.unwrap(); + let loaded = session.project().cloned().unwrap(); { let o = project.lock().unwrap(); let l = loaded.lock().unwrap(); @@ -333,8 +338,7 @@ fn ove_xml_timeline_roundtrip() { let (session, rc) = open(&uri).unwrap(); assert!(session.project().is_some(), "open failed rc={rc}"); assert_eq!(rc, OAKSTORAGE_OK); - let proj_handle = session.project().cloned().unwrap(); - let loaded = unsafe { project_arc(&proj_handle) }.unwrap(); + let loaded = session.project().cloned().unwrap(); { let l = loaded.lock().unwrap(); assert_imported_timeline(&l); @@ -842,8 +846,7 @@ fn assert_interchange_roundtrip(ext: &str) { let (session, rc) = open(&uri).unwrap(); assert!(session.project().is_some(), "open failed rc={rc}"); assert_eq!(rc, OAKSTORAGE_OK); - let proj_handle = session.project().cloned().unwrap(); - let loaded = unsafe { project_arc(&proj_handle) }.unwrap(); + let loaded = session.project().cloned().unwrap(); { let l = loaded.lock().unwrap(); assert_imported_timeline(&l); @@ -906,9 +909,8 @@ fn null_and_invalid_handles() { let (mut session, _) = open(&uri).unwrap(); let taken = session.take().expect("take transfers the project"); - assert!(!taken.ctx.is_null()); assert!(session.project().is_none(), "empty shell after take"); - release(taken); + drop(taken); } // --------------------------------------------------------------------------- @@ -931,15 +933,14 @@ fn session_take_transfers_project() { // Take transfers the project; the session shell stays empty. let taken = session.take().unwrap(); - assert!(!taken.ctx.is_null()); assert!(session.project().is_none(), "take empties the shell"); - release(taken); + drop(taken); // Dropping the shell (with the project already taken) is a no-op. drop(session); // A second open/take pairing works the same. let (mut session, _) = open(&uri).unwrap(); let taken = session.take().unwrap(); - assert!(!taken.ctx.is_null()); - release(taken); + assert!(session.project().is_none(), "take empties the shell"); + drop(taken); } diff --git a/crates/oaktask/README.md b/crates/oaktask/README.md index ea2c8a744..28eafb5c1 100644 --- a/crates/oaktask/README.md +++ b/crates/oaktask/README.md @@ -85,7 +85,7 @@ load/save/import/OTIO tasks). The behavior it reproduces is defined by: src/ lib.rs crate doc + module declarations error.rs OAKTASK_* codes + Error enum (module number 08) - handle.rs RefBox / CHandle / guard* FFI scaffolding + handle.rs owned-handle box + get/get_mut views (facade task boxes) task.rs Task base class + TaskEvent / EventListener manager.rs TaskManager singleton codecbridge.rs codec task submitter registration diff --git a/crates/oaktask/src/handle.rs b/crates/oaktask/src/handle.rs index e04f5f661..cf01be0cf 100644 --- a/crates/oaktask/src/handle.rs +++ b/crates/oaktask/src/handle.rs @@ -19,6 +19,12 @@ //! than shared — each module DLL must run its own addref/release code (the //! function pointers in a handle always point into the DLL that created the //! object). +//! +//! M14 R5: only the parts the oakengine facade needs remain (owned +//! box/addref/release plus typed `get`/`get_mut` views — the facade boxes +//! its task payloads through [`make_owned`] and reads them back with +//! `get`/`get_mut`). The borrowed-handle and panic-guard helpers had no +//! in-crate callers and were removed. use std::sync::atomic::AtomicU32; @@ -65,23 +71,6 @@ unsafe extern "C" fn owned_release(ctx: *mut std::ffi::c_void) { } } -/// Release function for borrowed handles: destroys only the box, never the -/// pointee. -/// -/// CPP-PARITY: src/task/c_api/taskhandle.h (wrap_borrowed) -unsafe extern "C" fn borrowed_release(ctx: *mut std::ffi::c_void) { - if ctx.is_null() { - return; - } - let b = ctx as *const RefBox<*mut T>; - let last = unsafe { (*b).refs.fetch_sub(1, std::sync::atomic::Ordering::SeqCst) }; - if last == 1 { - unsafe { - drop(Box::from_raw(ctx as *mut RefBox<*mut T>)); - } - } -} - /// Owned handle with count 1; empty on allocation failure. pub fn make_owned(value: T) -> CHandle { let b = Box::new(RefBox { @@ -96,27 +85,6 @@ pub fn make_owned(value: T) -> CHandle { } } -/// Borrowed handle for an object owned elsewhere (release frees only the -/// box). -/// -/// # Safety -/// Caller guarantees `ptr` outlives every derived handle. -pub unsafe fn make_borrowed(ptr: *mut T) -> CHandle { - if ptr.is_null() { - return CHandle::null(); - } - let b = Box::new(RefBox { - refs: AtomicU32::new(1), - value: ptr, - }); - CHandle { - ctx: Box::into_raw(b) as *mut std::ffi::c_void, - addref: Some(owned_addref::<*mut T>), - release: Some(borrowed_release::), - abi_version: OAKTASK_ABI_VERSION, - } -} - /// Typed view into a handle; `None` for empty handles. /// /// # Safety @@ -139,25 +107,3 @@ pub unsafe fn get_mut(h: &CHandle) -> Option<&mut T> { } unsafe { Some(&mut (*(h.ctx as *mut RefBox)).value) } } - -/// Panic-catching FFI wrapper for i32-returning exports. -pub fn guard crate::error::Result<()>>(f: F) -> i32 { - match std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) { - Ok(Ok(())) => crate::error::OAKTASK_OK, - Ok(Err(e)) => e.code(), - Err(_) => crate::error::OAKTASK_E_FAILED, - } -} - -/// Panic-catching FFI wrapper for handle-returning exports. -pub fn guard_handle crate::error::Result>(f: F) -> CHandle { - match std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)) { - Ok(Ok(h)) => h, - Ok(Err(_)) | Err(_) => CHandle::null(), - } -} - -/// Panic-catching FFI wrapper for void exports. -pub fn guard_void(f: F) { - let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); -} diff --git a/crates/oaktask/src/manager.rs b/crates/oaktask/src/manager.rs index d2b352687..ae7c345a9 100644 --- a/crates/oaktask/src/manager.rs +++ b/crates/oaktask/src/manager.rs @@ -33,7 +33,6 @@ use std::sync::Mutex; use crate::error::{Error, Result}; -use crate::handle::{make_borrowed, CHandle}; use crate::task::Task; /// Process-wide singleton manager. C++ is a lazy singleton; the Rust side @@ -186,14 +185,6 @@ impl TaskManager { self.tasks.len() } - /// Borrowed handle to the task at `index`; `Err(Error::NotFound)` if out - /// of range. - pub fn get_task_at(&self, index: usize) -> Result { - let task = self.tasks.get(index).ok_or(Error::NotFound)?; - let ptr = task.task() as *const Task as usize as *mut Task; - Ok(unsafe { make_borrowed::(ptr) }) - } - /// Raw pointer to the task at `index` (stable while the manager owns /// it). Used by `oaktask_manager_at` to build a borrowed task handle. pub fn task_ptr_at(&self, index: usize) -> Result<*mut Task> { diff --git a/crates/oaktimeline/COVERAGE.md b/crates/oaktimeline/COVERAGE.md index d87471da4..3a183da17 100644 --- a/crates/oaktimeline/COVERAGE.md +++ b/crates/oaktimeline/COVERAGE.md @@ -50,7 +50,7 @@ | `create_remove_command(Node/Block)` | `undocommon::create_remove_command` | | `create_and_run_remove_command(Node/Block)` | `undocommon::create_and_run_remove_command` | | `free_command_handle` | `undocommon::free_command_handle` | -| `CHandleCommandWrapper` | `undocommon::CHandleCommandWrapper`(把 oakundo vtable command 当本 crate 命令暴露的封装) | +| `CHandleCommandWrapper` | 已删除(M14 R5):wrapper 是旧 C ABI 时代的产物;命令现在直接以 `UndoCommand` 值存在,经 `box_command` 装箱 | ## 5. Track 命令(timelineundotrack.h) @@ -120,4 +120,4 @@ |---|---| | `oaknode_c_api::to_native` / `oakundo_capi::make_command_handle`(C++ 内部助手) | 不复制;Rust 侧把 handle 当 opaque,命令经 vtable、裸指针经 bridge | | `Timeline::PLAYHEAD_COLOR` | UI 取色宏,归 facade/app | -| 全部 `*_internal` 私有辅助 / `MemoryManager` 语义 | 实现细节,重组于各模块内部;内存所有权由 `CHandle` addref/release 表达 | +| 全部 `*_internal` 私有辅助 / `MemoryManager` 语义 | 实现细节,重组于各模块内部;共享对象经 `Arc>` 表达,`CHandle` 只剩 facade 边界 | diff --git a/crates/oaktimeline/README.md b/crates/oaktimeline/README.md index 40165aad3..2136e9c40 100644 --- a/crates/oaktimeline/README.md +++ b/crates/oaktimeline/README.md @@ -33,8 +33,8 @@ internal Rust types. exposes `prepare()` / `redo()` / `undo()` (all `todo!()` here) and is surfaced to the world through the oakundo C ABI vtable (`bridge::undo::oakundo_command_init`, Rust callbacks as `userdata`). - Every command struct carries a `to_command() -> CHandle` factory doc - comment describing the wiring. + Every command struct carries a `to_command()` factory that wraps it + into an `oakundo::undocommand::UndoCommand` value for the undo stack. 3. **Value types come from `oakcore-rs`.** Markers and work areas are built on `Rational`/`TimeRange`, so the crate depends on @@ -51,11 +51,13 @@ internal Rust types. role is subsumed by vtable commands and by value handles treated as opaque. -5. **Handles.** `handle.rs` provides the shared `RefBox`/`CHandle` - scaffolding (duplicated per crate, as in oaknode) with - `OAKTIMELINE_ABI_VERSION = 1`. Borrowed handles into node-owned - objects and owning handles created by `*_create` share one box - layout `{ctx, addref, release, abi_version}`. +5. **Handles.** `handle.rs` keeps the `RefBox`/`CHandle` scaffolding + for the oakengine facade boundary (the C ABI export layer is the only + place left that talks handles). Every `make_owned` value handle boxes + an `Arc>` — the same pattern as the oaknode project handles — + so the crate's commands hold the shared marker list / work area as a + plain Rust `Arc>` and the facade entries convert the handle + back to the `Arc` at the boundary. ## Layout @@ -68,7 +70,7 @@ src/ WaveformMode, EditToInfo) — timelinecommon.h marker.rs TimelineMarker/MarkerList + 5 marker commands workarea.rs TimelineWorkArea + 2 workarea commands - undocommon.rs node/block remove helpers + CHandleCommandWrapper + undocommon.rs node/block remove helpers + MultiUndoCommand undotrack.rs track ripple/prepend/insert-after/replace commands undogeneral.rs resize/media-in/add/remove-track/transition/gap/ enable-disable/insert-gaps/default-transition commands diff --git a/crates/oaktimeline/src/handle.rs b/crates/oaktimeline/src/handle.rs index 282224818..53de7ec8a 100644 --- a/crates/oaktimeline/src/handle.rs +++ b/crates/oaktimeline/src/handle.rs @@ -14,18 +14,25 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . -//! Refcounted-handle scaffolding. Same pattern as the oaknode crate -//! (`src/node/rust/src/handle.rs`); intentionally duplicated rather -//! than shared — each module DLL must run its own addref/release code -//! (the function pointers in a handle always point into the DLL that -//! created the object). Value handles (`OakTimelineMarkerList`, -//! `OakTimelineWorkArea`) share this box. +//! Refcounted-handle scaffolding for the oakengine facade boundary. +//! +//! After the single-lib unification the crate's internal object +//! references are plain Rust types: the timeline commands hold +//! [`Arc>`] handles to the shared marker list / work area, and +//! [`CHandle`] appears only in the facade-facing entries that the +//! oakengine C ABI export layer (and the first-party frontends) call. +//! +//! Every [`make_owned`] value handle boxes an [`Arc>`] behind a +//! [`RefBox`] (the same pattern as the oaknode project handles): the +//! facade reads the shared object back through +//! `get::>>`, clones the `Arc`, and the command entries do +//! the same when they convert a handle into the crate's Rust-typed +//! command constructors. The box's addref/release functions only manage +//! the handle shell — the `Arc` keeps the actual value alive. use std::ffi::c_void; -use std::panic::{catch_unwind, AssertUnwindSafe}; use std::sync::atomic::{AtomicU32, Ordering}; - -use crate::error::{Result, OAKTIMELINE_E_FAILED, OAKTIMELINE_OK}; +use std::sync::{Arc, Mutex}; /// ABI version stamped into every oaktimeline handle. pub const OAKTIMELINE_ABI_VERSION: u32 = 1; @@ -74,56 +81,50 @@ unsafe extern "C" fn release_box(ptr: *mut c_void) { } /// Owned handle with count 1; empty on allocation failure. +/// +/// The boxed value is an [`Arc>`] (see the module docs): every +/// handle produced here references the same shared object that the +/// timeline commands hold directly. pub fn make_owned(value: T) -> CHandle { + make_owned_arc(Arc::new(Mutex::new(value))) +} + +/// Owned handle over an already-shared `Arc>`; empty on +/// allocation failure. The `Arc` is moved into the box, so every handle +/// created from the same `Arc` shares one object. +pub fn make_owned_arc(arc: Arc>) -> CHandle { let boxed = Box::new(RefBox { refs: AtomicU32::new(1), - value, + value: arc, }); let ptr = Box::into_raw(boxed) as *mut c_void; CHandle { ctx: ptr, - addref: Some(addref_box::), - release: Some(release_box::), + addref: Some(addref_box::>>), + release: Some(release_box::>>), abi_version: OAKTIMELINE_ABI_VERSION, } } -/// Borrowed handle for an object owned elsewhere (release frees only -/// the box). -/// -/// The box holds a detached copy of `*ptr`, so the underlying object is -/// never touched by the handle's release; `get` returns the copy. The -/// caller retains ownership of `ptr`. -/// -/// # Safety -/// Caller guarantees `ptr` is valid for reading for the duration of the -/// call. -pub unsafe fn make_borrowed(ptr: *mut T) -> CHandle { - let value = unsafe { ptr.read() }; - make_owned(value) -} - /// Typed view into a handle; `None` for empty handles. /// +/// Value handles box an [`Arc>`]; read the shared object with +/// `get::>>(h)` and clone the `Arc` (or lock it in place). +/// /// # Safety /// `T` must be the boxed type. pub unsafe fn get(h: &CHandle) -> Option<&T> { if h.ctx.is_null() { return None; } - if h.addref.is_some() { - // Owned (or borrowed-via-copy) handle: `ctx` points at a `RefBox`. - let rb = unsafe { &*(h.ctx as *const RefBox) }; - Some(&rb.value) - } else { - // Borrowed handle wrapping a raw object pointer (e.g. test-stub - // handles): `ctx` is the object itself, not a `RefBox`. - Some(unsafe { &*(h.ctx as *const T) }) - } + let rb = unsafe { &*(h.ctx as *const RefBox) }; + Some(&rb.value) } -/// Mutable typed view into a handle; `None` for empty handles. Used by -/// undo commands to mutate the boxed value they hold a handle to. +/// Mutable typed view into a handle; `None` for empty handles. With the +/// `Arc`-boxed value handles this yields `&mut Arc>` — use +/// [`get::>>`](get) plus `Mutex::lock` to mutate the shared +/// value instead. /// /// # Safety /// `T` must be the boxed type, and the caller must guarantee exclusive @@ -133,47 +134,6 @@ pub unsafe fn get_mut(h: &CHandle) -> Option<&mut T> { if h.ctx.is_null() { return None; } - if h.addref.is_some() { - // Owned (or borrowed-via-copy) handle: `ctx` points at a `RefBox`. - let rb = unsafe { &mut *(h.ctx as *mut RefBox) }; - Some(&mut rb.value) - } else { - // Borrowed handle wrapping a raw object pointer (e.g. test-stub - // handles): `ctx` is the object itself, not a `RefBox`. - Some(unsafe { &mut *(h.ctx as *mut T) }) - } -} - -/// Panic-catching FFI wrapper for i32-returning exports. -pub fn guard Result<()>>(f: F) -> i32 { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(())) => OAKTIMELINE_OK, - Ok(Err(e)) => e.code(), - Err(_) => OAKTIMELINE_E_FAILED, - } -} - -/// Panic-catching FFI wrapper for handle-returning exports. -pub fn guard_handle Result>(f: F) -> CHandle { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(h)) => h, - _ => CHandle::null(), - } -} - -/// Panic-catching FFI wrapper for void exports. -pub fn guard_void(f: F) { - let _ = catch_unwind(AssertUnwindSafe(f)); -} - -/// Panic-catching FFI wrapper for exports returning an `i32` value that is -/// not an error code (e.g. two-stage string lengths): a successful closure -/// returns its value verbatim, errors map through `Error::code`, and a -/// panic becomes `OAKTIMELINE_E_FAILED`. -pub fn guard_i32 Result>(f: F) -> i32 { - match catch_unwind(AssertUnwindSafe(f)) { - Ok(Ok(v)) => v, - Ok(Err(e)) => e.code(), - Err(_) => OAKTIMELINE_E_FAILED, - } + let rb = unsafe { &mut *(h.ctx as *mut RefBox) }; + Some(&mut rb.value) } diff --git a/crates/oaktimeline/src/marker.rs b/crates/oaktimeline/src/marker.rs index a461bcc59..06eb017fa 100644 --- a/crates/oaktimeline/src/marker.rs +++ b/crates/oaktimeline/src/marker.rs @@ -25,10 +25,12 @@ //! not re-sort (the De-Qt marker has no parent pointer); callers restore //! order via `TimelineMarkerList::resort`/`resort_at`. +use std::sync::{Arc, Mutex}; + use oakcore_rs::{Rational, TimeRange}; use oakundo::undocommand::UndoCommand; -use crate::handle::{get, get_mut}; +use crate::handle::get; use crate::undocommon::{box_command, Command}; /// `TimelineMarker` — a named, colored time range on a timeline @@ -211,6 +213,15 @@ impl TimelineMarkerList { self.markers_.get_mut(i) } + /// Remove the marker at `index`, returning it; `None` out of range. + fn remove_at(&mut self, index: usize) -> Option { + if index < self.markers_.len() { + Some(self.markers_.remove(index)) + } else { + None + } + } + /// Remove the marker at `index` and re-insert it sorted; no-op when /// `index` is out of range. Used by the time-change command after /// mutating a marker in place. @@ -238,8 +249,10 @@ fn rational_abs(r: Rational) -> Rational { /// `MarkerAddCommand` (timelinemarker.h). pub struct MarkerAddCommand { - /// Target list. - marker_list: crate::handle::CHandle, + /// Target list (the shared marker list behind the facade's value + /// handle; `None` for an empty/null handle, which makes the command a + /// no-op). + marker_list: Option>>, /// Marker range. range: TimeRange, /// Marker name. @@ -258,8 +271,28 @@ impl MarkerAddCommand { name: &str, color: i32, ) -> Self { + // SAFETY: marker-list handles box an `Arc>` + // (created by `make_owned`); reading it clones the shared `Arc`. + let list = unsafe { get::>>(&marker_list) }.cloned(); Self { - marker_list, + marker_list: list, + range, + name: name.to_string(), + color, + added: false, + } + } + + /// Construct over an already-shared marker list (the crate's + /// Rust-typed entry; the facade-facing [`Self::new`] forwards here). + pub fn new_arc( + marker_list: Arc>, + range: TimeRange, + name: &str, + color: i32, + ) -> Self { + Self { + marker_list: Some(marker_list), range, name: name.to_string(), color, @@ -272,11 +305,10 @@ impl MarkerAddCommand { if self.added { return; } - let marker = TimelineMarker::with_time(self.color, self.range, &self.name); - // SAFETY: the boxed value is a `TimelineMarkerList` created by - // `make_owned`, and the command holds exclusive access to it. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { - list.add_marker(marker); + if let Some(list) = &self.marker_list { + let marker = TimelineMarker::with_time(self.color, self.range, &self.name); + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); + l.add_marker(marker); self.added = true; } } @@ -290,13 +322,12 @@ impl MarkerAddCommand { if !self.added { return; } - // SAFETY: as `redo`. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { - if let Some(m) = list.get_marker_at_time(self.range.in_()) { - // SAFETY: `m` borrows from `list`; `remove_marker` uses it - // only for identity comparison before detaching. - let mptr = m as *const TimelineMarker; - let _ = list.remove_marker(unsafe { &*mptr }); + if let Some(list) = &self.marker_list { + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(index) = (0..l.size()) + .find(|&i| l.at(i).is_some_and(|m| m.time().in_() == self.range.in_())) + { + l.remove_at(index); } self.added = false; } @@ -322,8 +353,9 @@ impl Command for MarkerAddCommand { /// `MarkerRemoveCommand` (timelinemarker.h). pub struct MarkerRemoveCommand { - /// Target list. - marker_list: crate::handle::CHandle, + /// Target list (shared marker list behind the facade's value handle; + /// `None` for an empty/null handle, which makes the command a no-op). + marker_list: Option>>, /// Index of the marker to remove. index: usize, /// Marker detached on `redo`, re-inserted by `undo`. @@ -333,8 +365,20 @@ pub struct MarkerRemoveCommand { impl MarkerRemoveCommand { /// Construct from list + index of the marker to remove. pub fn new(marker_list: crate::handle::CHandle, index: usize) -> Self { + // SAFETY: as `MarkerAddCommand::new`. + let list = unsafe { get::>>(&marker_list) }.cloned(); Self { - marker_list, + marker_list: list, + index, + removed: None, + } + } + + /// Construct over an already-shared marker list (the crate's + /// Rust-typed entry; the facade-facing [`Self::new`] forwards here). + pub fn new_arc(marker_list: Arc>, index: usize) -> Self { + Self { + marker_list: Some(marker_list), index, removed: None, } @@ -345,26 +389,20 @@ impl MarkerRemoveCommand { if self.removed.is_some() { return; } - // SAFETY: the boxed value is a `TimelineMarkerList` created by - // `make_owned`, and the command holds exclusive access to it. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { - if let Some(m) = list.at(self.index) { - // SAFETY: `m` borrows from `list`; `remove_marker` uses it - // only for identity comparison before detaching. - let mptr = m as *const TimelineMarker; - if let Some(removed) = list.remove_marker(unsafe { &*mptr }) { - self.removed = Some(removed); - } + if let Some(list) = &self.marker_list { + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(removed) = l.remove_at(self.index) { + self.removed = Some(removed); } } } /// `undo`: re-insert the marker, sorted. pub fn undo(&mut self) { - // SAFETY: as `redo`. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { + if let Some(list) = &self.marker_list { + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); if let Some(marker) = self.removed.take() { - list.add_marker(marker); + l.add_marker(marker); } } } @@ -389,8 +427,9 @@ impl Command for MarkerRemoveCommand { /// `MarkerChangeColorCommand` (timelinemarker.h). pub struct MarkerChangeColorCommand { - /// Target list. - marker_list: crate::handle::CHandle, + /// Target list (shared marker list behind the facade's value handle; + /// `None` for an empty/null handle, which makes the command a no-op). + marker_list: Option>>, /// Index of the marker to change. index: usize, /// Color before the change. @@ -403,14 +442,38 @@ impl MarkerChangeColorCommand { /// Construct from list + index + new color, capturing the current color /// as old. pub fn new(marker_list: crate::handle::CHandle, index: usize, new_color: i32) -> Self { - // SAFETY: the boxed value is a `TimelineMarkerList` created by - // `make_owned`; reading it here is the command's own handle. - let old_color = unsafe { get::(&marker_list) } - .and_then(|l| l.at(index)) + // SAFETY: as `MarkerAddCommand::new`. + let list = unsafe { get::>>(&marker_list) }.cloned(); + let old_color = list + .as_ref() + .and_then(|l| { + let l = l.lock().unwrap_or_else(|e| e.into_inner()); + l.at(index).map(|m| m.color()) + }) + .unwrap_or(0); + Self { + marker_list: list, + index, + old_color, + new_color, + } + } + + /// Construct over an already-shared marker list (the crate's + /// Rust-typed entry; the facade-facing [`Self::new`] forwards here). + pub fn new_arc( + marker_list: Arc>, + index: usize, + new_color: i32, + ) -> Self { + let old_color = marker_list + .lock() + .unwrap_or_else(|e| e.into_inner()) + .at(index) .map(|m| m.color()) .unwrap_or(0); Self { - marker_list, + marker_list: Some(marker_list), index, old_color, new_color, @@ -419,9 +482,9 @@ impl MarkerChangeColorCommand { /// `redo`: apply the new color. pub fn redo(&mut self) { - // SAFETY: as `new`. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { - if let Some(m) = list.at_mut(self.index) { + if let Some(list) = &self.marker_list { + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(m) = l.at_mut(self.index) { m.set_color(self.new_color); } } @@ -429,9 +492,9 @@ impl MarkerChangeColorCommand { /// `undo`: restore the old color. pub fn undo(&mut self) { - // SAFETY: as `new`. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { - if let Some(m) = list.at_mut(self.index) { + if let Some(list) = &self.marker_list { + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(m) = l.at_mut(self.index) { m.set_color(self.old_color); } } @@ -457,8 +520,9 @@ impl Command for MarkerChangeColorCommand { /// `MarkerChangeNameCommand` (timelinemarker.h). pub struct MarkerChangeNameCommand { - /// Target list. - marker_list: crate::handle::CHandle, + /// Target list (shared marker list behind the facade's value handle; + /// `None` for an empty/null handle, which makes the command a no-op). + marker_list: Option>>, /// Index of the marker to change. index: usize, /// Name before the change. @@ -471,14 +535,34 @@ impl MarkerChangeNameCommand { /// Construct from list + index + new name, capturing the current name as /// old. pub fn new(marker_list: crate::handle::CHandle, index: usize, name: &str) -> Self { - // SAFETY: the boxed value is a `TimelineMarkerList` created by - // `make_owned`; reading it here is the command's own handle. - let old_name = unsafe { get::(&marker_list) } - .and_then(|l| l.at(index)) + // SAFETY: as `MarkerAddCommand::new`. + let list = unsafe { get::>>(&marker_list) }.cloned(); + let old_name = list + .as_ref() + .and_then(|l| { + let l = l.lock().unwrap_or_else(|e| e.into_inner()); + l.at(index).map(|m| m.name().to_string()) + }) + .unwrap_or_default(); + Self { + marker_list: list, + index, + old_name, + new_name: name.to_string(), + } + } + + /// Construct over an already-shared marker list (the crate's + /// Rust-typed entry; the facade-facing [`Self::new`] forwards here). + pub fn new_arc(marker_list: Arc>, index: usize, name: &str) -> Self { + let old_name = marker_list + .lock() + .unwrap_or_else(|e| e.into_inner()) + .at(index) .map(|m| m.name().to_string()) .unwrap_or_default(); Self { - marker_list, + marker_list: Some(marker_list), index, old_name, new_name: name.to_string(), @@ -487,9 +571,9 @@ impl MarkerChangeNameCommand { /// `redo`: apply the new name. pub fn redo(&mut self) { - // SAFETY: as `new`. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { - if let Some(m) = list.at_mut(self.index) { + if let Some(list) = &self.marker_list { + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(m) = l.at_mut(self.index) { m.set_name(&self.new_name); } } @@ -497,9 +581,9 @@ impl MarkerChangeNameCommand { /// `undo`: restore the old name. pub fn undo(&mut self) { - // SAFETY: as `new`. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { - if let Some(m) = list.at_mut(self.index) { + if let Some(list) = &self.marker_list { + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(m) = l.at_mut(self.index) { m.set_name(&self.old_name); } } @@ -526,8 +610,9 @@ impl Command for MarkerChangeNameCommand { /// `MarkerChangeTimeCommand` (timelinemarker.h). The old range is captured at /// construction when not supplied. pub struct MarkerChangeTimeCommand { - /// Target list. - marker_list: crate::handle::CHandle, + /// Target list (shared marker list behind the facade's value handle; + /// `None` for an empty/null handle, which makes the command a no-op). + marker_list: Option>>, /// Index of the marker to change. index: usize, /// Time range before the change. @@ -540,14 +625,38 @@ impl MarkerChangeTimeCommand { /// Construct from list + index + new time, capturing the current range as /// old. pub fn new(marker_list: crate::handle::CHandle, index: usize, time: TimeRange) -> Self { - // SAFETY: the boxed value is a `TimelineMarkerList` created by - // `make_owned`; reading it here is the command's own handle. - let old_time = unsafe { get::(&marker_list) } - .and_then(|l| l.at(index)) + // SAFETY: as `MarkerAddCommand::new`. + let list = unsafe { get::>>(&marker_list) }.cloned(); + let old_time = list + .as_ref() + .and_then(|l| { + let l = l.lock().unwrap_or_else(|e| e.into_inner()); + l.at(index).map(|m| *m.time()) + }) + .unwrap_or_else(|| TimeRange::new(Rational::new(0, 1), Rational::new(0, 1))); + Self { + marker_list: list, + index, + old_time, + new_time: time, + } + } + + /// Construct over an already-shared marker list (the crate's + /// Rust-typed entry; the facade-facing [`Self::new`] forwards here). + pub fn new_arc( + marker_list: Arc>, + index: usize, + time: TimeRange, + ) -> Self { + let old_time = marker_list + .lock() + .unwrap_or_else(|e| e.into_inner()) + .at(index) .map(|m| *m.time()) .unwrap_or_else(|| TimeRange::new(Rational::new(0, 1), Rational::new(0, 1))); Self { - marker_list, + marker_list: Some(marker_list), index, old_time, new_time: time, @@ -556,23 +665,23 @@ impl MarkerChangeTimeCommand { /// `redo`: apply the new time (resorts). pub fn redo(&mut self) { - // SAFETY: as `new`. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { - if let Some(m) = list.at_mut(self.index) { + if let Some(list) = &self.marker_list { + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(m) = l.at_mut(self.index) { m.set_time(self.new_time); } - list.resort_at(self.index); + l.resort_at(self.index); } } /// `undo`: restore the old time (resorts). pub fn undo(&mut self) { - // SAFETY: as `new`. - if let Some(list) = unsafe { get_mut::(&self.marker_list) } { - if let Some(m) = list.at_mut(self.index) { + if let Some(list) = &self.marker_list { + let mut l = list.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(m) = l.at_mut(self.index) { m.set_time(self.old_time); } - list.resort_at(self.index); + l.resort_at(self.index); } } diff --git a/crates/oaktimeline/src/undocommon.rs b/crates/oaktimeline/src/undocommon.rs index 80a98fc76..d23e37415 100644 --- a/crates/oaktimeline/src/undocommon.rs +++ b/crates/oaktimeline/src/undocommon.rs @@ -22,9 +22,8 @@ //! ([`crate::util::NodeRef`] + the project's graph arena). //! //! `CHandleCommandWrapper` in C++ subclasses `olive::UndoCommand` to wrap a -//! raw `OakUndoCommand`; the Rust equivalent holds an -//! [`oakundo::undocommand::UndoCommand`] and forwards `redo`/`undo` to -//! `redo_now`/`undo_now`. +//! raw `OakUndoCommand`; the Rust equivalent is the crate's own commands +//! boxed through [`box_command`], so the wrapper is gone. use std::ffi::c_void; @@ -188,42 +187,6 @@ pub(crate) fn box_command(cmd: T) -> UndoCommand { UndoCommand::from_vtable(vtable, userdata) } -/// `CHandleCommandWrapper` — an oakundo command exposed as a timeline-level -/// command. `redo`/`undo` forward to `redo_now`/`undo_now`; dropping drops -/// the wrapped command. -pub struct CHandleCommandWrapper { - /// Wrapped command; `None` mirrors the old empty (null) command handle. - command: Option, -} - -impl CHandleCommandWrapper { - /// Construct over an owned command value. - pub fn new(command: UndoCommand) -> Self { - Self { - command: Some(command), - } - } - - /// Whether the wrapper holds a command (the old non-null check). - pub fn is_valid(&self) -> bool { - self.command.is_some() - } - - /// `redo`: forward to `UndoCommand::redo_now`. - pub fn redo(&mut self) { - if let Some(c) = self.command.as_mut() { - c.redo_now(); - } - } - - /// `undo`: forward to `UndoCommand::undo_now`. - pub fn undo(&mut self) { - if let Some(c) = self.command.as_mut() { - c.undo_now(); - } - } -} - /// `MultiUndoCommand` — a command that runs several child commands in order /// on `redo` and in reverse on `undo` (timelineundocommon.h /// `MultiUndoCommand`). Children are boxed `Command`s; the whole group wraps diff --git a/crates/oaktimeline/src/workarea.rs b/crates/oaktimeline/src/workarea.rs index b54f221a0..cbe5e5bd7 100644 --- a/crates/oaktimeline/src/workarea.rs +++ b/crates/oaktimeline/src/workarea.rs @@ -21,9 +21,13 @@ //! De-Qt: no QObject, no signals — change notifications are the facade's //! job. +use std::sync::{Arc, Mutex}; + use oakcore_rs::{Rational, TimeRange}; use oakundo::undocommand::UndoCommand; +use crate::handle::get; + /// The reset sentinel `k_reset_in` (timelineworkarea.h): 0/1. Exposed as a /// function because `Rational::new` is not yet `const` in oakcore-rs. pub fn reset_in() -> Rational { @@ -90,8 +94,9 @@ impl TimelineWorkArea { /// `WorkareaSetEnabledCommand` (timelineundoworkarea.h). pub struct WorkareaSetEnabledCommand { - /// Target work area handle. - points: crate::handle::CHandle, + /// Target work area (shared behind the facade's value handle; `None` + /// for an empty/null handle, which makes the command a no-op). + points: Option>>, /// New enabled flag. new_enabled: bool, /// Enabled flag captured at construction, restored by `undo`. @@ -101,11 +106,29 @@ pub struct WorkareaSetEnabledCommand { impl WorkareaSetEnabledCommand { /// Construct from work area + new enabled value (captures old at ctor). pub fn new(points: crate::handle::CHandle, enabled: bool) -> Self { - let old_enabled = unsafe { crate::handle::get::(&points) } - .map(|wa| wa.enabled()) + // SAFETY: work-area handles box an `Arc>` + // (created by `make_owned`); reading it clones the shared `Arc`. + let wa = unsafe { get::>>(&points) }.cloned(); + let old_enabled = wa + .as_ref() + .and_then(|w| { + let w = w.lock().unwrap_or_else(|e| e.into_inner()); + Some(w.enabled()) + }) .unwrap_or(false); Self { - points, + points: wa, + new_enabled: enabled, + old_enabled, + } + } + + /// Construct over an already-shared work area (the crate's Rust-typed + /// entry; the facade-facing [`Self::new`] forwards here). + pub fn new_arc(points: Arc>, enabled: bool) -> Self { + let old_enabled = points.lock().unwrap_or_else(|e| e.into_inner()).enabled(); + Self { + points: Some(points), new_enabled: enabled, old_enabled, } @@ -113,14 +136,16 @@ impl WorkareaSetEnabledCommand { /// `redo`: set enabled. pub fn redo(&mut self) { - if let Some(wa) = unsafe { crate::handle::get_mut::(&self.points) } { + if let Some(wa) = &self.points { + let mut wa = wa.lock().unwrap_or_else(|e| e.into_inner()); wa.set_enabled(self.new_enabled); } } /// `undo`: restore old enabled. pub fn undo(&mut self) { - if let Some(wa) = unsafe { crate::handle::get_mut::(&self.points) } { + if let Some(wa) = &self.points { + let mut wa = wa.lock().unwrap_or_else(|e| e.into_inner()); wa.set_enabled(self.old_enabled); } } @@ -146,8 +171,9 @@ impl crate::undocommon::Command for WorkareaSetEnabledCommand { /// `WorkareaSetRangeCommand` (timelineundoworkarea.h). The old range is /// captured at construction when not supplied. pub struct WorkareaSetRangeCommand { - /// Target work area handle. - workarea: crate::handle::CHandle, + /// Target work area (shared behind the facade's value handle; `None` + /// for an empty/null handle, which makes the command a no-op). + workarea: Option>>, /// New range. new_range: TimeRange, /// Range captured at construction, restored by `undo`. @@ -157,10 +183,23 @@ pub struct WorkareaSetRangeCommand { impl WorkareaSetRangeCommand { /// Construct from work area + new range (captures current as old). pub fn new(workarea: crate::handle::CHandle, range: TimeRange) -> Self { - let old_range = unsafe { crate::handle::get::(&workarea) } - .map(|wa| *wa.range()) + // SAFETY: as `WorkareaSetEnabledCommand::new`. + let wa = unsafe { get::>>(&workarea) }.cloned(); + let old_range = wa + .as_ref() + .and_then(|w| { + let w = w.lock().unwrap_or_else(|e| e.into_inner()); + Some(*w.range()) + }) .unwrap_or(range); - Self::new_with_old(workarea, range, old_range) + Self::new_with_old_opt(wa, range, old_range) + } + + /// Construct over an already-shared work area (the crate's Rust-typed + /// entry; the facade-facing [`Self::new`] forwards here). + pub fn new_arc(workarea: Arc>, range: TimeRange) -> Self { + let old_range = *workarea.lock().unwrap_or_else(|e| e.into_inner()).range(); + Self::new_with_old_opt(Some(workarea), range, old_range) } /// Construct from work area + new range + explicitly supplied old @@ -170,6 +209,29 @@ impl WorkareaSetRangeCommand { workarea: crate::handle::CHandle, range: TimeRange, old_range: TimeRange, + ) -> Self { + // SAFETY: as `WorkareaSetEnabledCommand::new`. + let wa = unsafe { get::>>(&workarea) }.cloned(); + Self::new_with_old_opt(wa, range, old_range) + } + + /// Construct over an already-shared work area with an explicitly + /// supplied old range (the crate's Rust-typed entry; the facade-facing + /// [`Self::new_with_old`] forwards here). + pub fn new_with_old_arc( + workarea: Arc>, + range: TimeRange, + old_range: TimeRange, + ) -> Self { + Self::new_with_old_opt(Some(workarea), range, old_range) + } + + /// Shared constructor over an optional work area (`None` mirrors the + /// empty/null handle: the command is a no-op). + fn new_with_old_opt( + workarea: Option>>, + range: TimeRange, + old_range: TimeRange, ) -> Self { Self { workarea, @@ -180,14 +242,16 @@ impl WorkareaSetRangeCommand { /// `redo`: set the range. pub fn redo(&mut self) { - if let Some(wa) = unsafe { crate::handle::get_mut::(&self.workarea) } { + if let Some(wa) = &self.workarea { + let mut wa = wa.lock().unwrap_or_else(|e| e.into_inner()); wa.set_range(self.new_range); } } /// `undo`: restore the old range. pub fn undo(&mut self) { - if let Some(wa) = unsafe { crate::handle::get_mut::(&self.workarea) } { + if let Some(wa) = &self.workarea { + let mut wa = wa.lock().unwrap_or_else(|e| e.into_inner()); wa.set_range(self.old_range); } } diff --git a/crates/oaktimeline/tests/handle_test.rs b/crates/oaktimeline/tests/handle_test.rs index 68d999d7e..79aa79467 100644 --- a/crates/oaktimeline/tests/handle_test.rs +++ b/crates/oaktimeline/tests/handle_test.rs @@ -17,16 +17,16 @@ //! Contract tests for the refcounted-handle scaffolding //! (`src/handle.rs`). The C++ gtest suite (`src/timeline/tests`) //! drives the ABI-level refcount behaviour through the exports; these -//! tests pin the Rust-side primitive semantics that every export -//! relies on: null sentinel, `make_owned`/`make_borrowed` ownership, -//! panics escaping through `guard*`, and the ABI version stamp. +//! tests pin the Rust-side primitive semantics every facade-facing +//! entry relies on: the null sentinel, `make_owned`'s shared +//! `Arc>` box, and the ABI version stamp. The old +//! `guard*`/`make_borrowed` helpers left with the deleted C ABI export +//! layer. -use oaktimeline::error::{ - Error, OAKTIMELINE_ABI_VERSION, OAKTIMELINE_E_FAILED, OAKTIMELINE_E_INVALID, OAKTIMELINE_OK, -}; -use oaktimeline::handle::{ - get, guard, guard_handle, guard_void, make_borrowed, make_owned, CHandle, -}; +use std::sync::{Arc, Mutex}; + +use oaktimeline::error::OAKTIMELINE_ABI_VERSION; +use oaktimeline::handle::{get, make_owned, CHandle}; /// `CHandle::null()` is the all-null sentinel with abi_version 0. #[test] @@ -50,87 +50,45 @@ fn make_owned_starts_at_one_and_releases() { // Addref bumps the count so the box is still live afterwards. let addref = h.addref.unwrap(); - // Safety: `h` is an owned handle whose box is `RefBox`. + // Safety: `h` is an owned handle whose box is `RefBox>>`. unsafe { addref(h.ctx) }; - let val = unsafe { get::(&h) }; - assert_eq!(val, Some(&42)); + // SAFETY: `make_owned` boxes an `Arc>`. + let val = unsafe { get::>>(&h) }; + assert_eq!(*val.unwrap().lock().unwrap(), 42); } -/// `make_owned` boxes a `Send + 'static` value and `get` returns the -/// boxed value back out. +/// `make_owned` boxes a `Send + 'static` value behind a shared +/// `Arc>` and `get` hands the shared `Arc` back out. #[test] fn make_owned_round_trips_value() { let h = make_owned(7i32); - let val = unsafe { get::(&h) }; - assert_eq!(val, Some(&7)); + // SAFETY: `make_owned` boxes an `Arc>`. + let val = unsafe { get::>>(&h) }; + assert_eq!(*val.unwrap().lock().unwrap(), 7); } -/// `make_borrowed` wraps a caller-owned pointer without transferring -/// ownership: releasing the borrowed handle frees only the box, never -/// the underlying object. -/// -/// # Safety -/// The borrowed allocation must outlive the handle; the caller frees -/// it afterwards. +/// Every handle produced by `make_owned` shares one object: mutating +/// through a clone of the boxed `Arc` is visible through any other clone +/// of the same handle. #[test] -fn make_borrowed_release_does_not_free_owner() { - let mut v = 5i32; - let h = unsafe { make_borrowed::(&mut v) }; - assert!(!h.is_null()); - // Releasing the borrowed handle destroys only the internal copy. - let release = h.release.unwrap(); - // Safety: `h` is an owned box (see `make_borrowed`), release boxed copy. - unsafe { release(h.ctx) }; - // The caller's object is untouched. - assert_eq!(v, 5); +fn make_owned_shares_one_object() { + let h1 = make_owned(0i32); + let h2 = h1.clone(); + // SAFETY: both handles box the same `Arc>` allocation. + let a = unsafe { get::>>(&h1) }.unwrap().clone(); + let b = unsafe { get::>>(&h2) }.unwrap().clone(); + assert!(Arc::ptr_eq(&a, &b)); + *a.lock().unwrap() = 42; + assert_eq!(*b.lock().unwrap(), 42); } -/// `guard` maps a successful closure to `OAKTIMELINE_OK`. +/// `get` on a null handle yields `None` (the null-handle sentinel maps +/// to "no object" everywhere). #[test] -fn guard_success_returns_ok() { - assert_eq!(guard(|| Ok(())), OAKTIMELINE_OK); -} - -/// `guard` maps an `Err(Error::Invalid)` to `OAKTIMELINE_E_INVALID`. -#[test] -fn guard_error_maps_code() { - assert_eq!(guard(|| Err(Error::Invalid)), OAKTIMELINE_E_INVALID); -} - -/// `guard_handle` returns the inner handle on success. -#[test] -fn guard_handle_success_returns_handle() { - let inner = make_owned(3i32); - let out = guard_handle(|| Ok(inner.clone())); - assert!(!out.is_null()); - assert_eq!(unsafe { get::(&out) }, Some(&3)); -} - -/// `guard_handle` returns a null handle on error so callers never see -/// a partially-built object. -#[test] -fn guard_handle_error_returns_null() { - let out = guard_handle(|| Err(Error::Invalid)); - assert!(out.is_null()); -} - -/// A panicking closure does not unwind across the `guard_void` FFI -/// boundary; it is caught and converted to the failed code. -#[test] -fn guard_catches_panic() { - // `guard` maps the panic to the failed code. - let code = guard(|| -> oaktimeline::error::Result<()> { - panic!("boom"); - }); - assert_eq!(code, OAKTIMELINE_E_FAILED); - - // `guard_void` swallows the panic without unwinding into the caller. - let mut reached = false; - guard_void(|| { - panic!("no unwind"); - }); - reached = true; - assert!(reached); +fn get_null_handle_is_none() { + let h = CHandle::null(); + // SAFETY: null handle -> None without touching any pointer. + assert!(unsafe { get::(&h) }.is_none()); } /// Every handle produced through the crate carries @@ -141,27 +99,3 @@ fn handles_are_stamped_with_abi_version() { assert_eq!(h.abi_version, OAKTIMELINE_ABI_VERSION); assert_ne!(h.abi_version, 0); } - -/// Null and invalid handles are rejected uniformly: every `guard` -/// family returns the error code / null handle without touching the -/// pointer. -#[test] -fn guard_rejects_invalid_handles() { - let null_h = CHandle::null(); - // Reading through a null handle yields None, which maps to invalid. - let code = guard(|| { - if unsafe { get::(&null_h) }.is_none() { - return Err(Error::Invalid); - } - Ok(()) - }); - assert_eq!(code, OAKTIMELINE_E_INVALID); - - let h_out = guard_handle(|| { - if unsafe { get::(&null_h) }.is_none() { - return Err(Error::Invalid); - } - Ok(make_owned(0i32)) - }); - assert!(h_out.is_null()); -} diff --git a/crates/oaktimeline/tests/marker_test.rs b/crates/oaktimeline/tests/marker_test.rs index 201951058..847473f6d 100644 --- a/crates/oaktimeline/tests/marker_test.rs +++ b/crates/oaktimeline/tests/marker_test.rs @@ -21,15 +21,28 @@ //! after out-of-band edits. The XML load/save contract left with the //! deleted C ABI export layer (single-lib unification). +use std::sync::{Arc, Mutex, MutexGuard}; + use oakcore_rs::{Rational, TimeRange}; use oaktimeline::common::EditToInfo; -use oaktimeline::handle::{get, get_mut, make_owned}; +use oaktimeline::handle::{get, make_owned, CHandle}; use oaktimeline::marker::{ MarkerAddCommand, MarkerChangeColorCommand, MarkerChangeNameCommand, MarkerChangeTimeCommand, MarkerRemoveCommand, TimelineMarker, TimelineMarkerList, }; use oaktimeline::undocommon::Command; +/// Lock the shared marker list behind a live handle. Every marker-list +/// handle in these tests comes from `make_owned`, which boxes an +/// `Arc>`. +fn list_of(h: &CHandle) -> MutexGuard<'_, TimelineMarkerList> { + // SAFETY: as above; the handle is live. + unsafe { get::>>(h) } + .expect("live marker-list handle") + .lock() + .unwrap_or_else(|e| e.into_inner()) +} + /// A default `TimelineMarker` starts at the null time with no name. #[test] fn marker_default_is_null_time() { @@ -234,35 +247,22 @@ fn marker_add_command_undo_before_redo() { 0, ); cmd.undo(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 0 - ); + assert_eq!(list_of(&list_h).size(), 0); } /// `MarkerRemoveCommand` redo is idempotent (a second redo is a no-op). #[test] fn marker_remove_command_double_redo() { let list_h = make_owned(TimelineMarkerList::new()); - { - let l = unsafe { get_mut::(&list_h) }.unwrap(); - l.add_marker(TimelineMarker::with_time( - 1, - TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), - "m", - )); - } + list_of(&list_h).add_marker(TimelineMarker::with_time( + 1, + TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), + "m", + )); let mut cmd = MarkerRemoveCommand::new(list_h.clone(), 0); cmd.redo(); cmd.redo(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 0 - ); + assert_eq!(list_of(&list_h).size(), 0); } /// Every marker command dispatches through the `Command` trait, which is @@ -270,14 +270,11 @@ fn marker_remove_command_double_redo() { #[test] fn marker_commands_trait_dispatch() { let list_h = make_owned(TimelineMarkerList::new()); - { - let l = unsafe { get_mut::(&list_h) }.unwrap(); - l.add_marker(TimelineMarker::with_time( - 1, - TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), - "m", - )); - } + list_of(&list_h).add_marker(TimelineMarker::with_time( + 1, + TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), + "m", + )); let mut add = MarkerAddCommand::new( list_h.clone(), @@ -286,75 +283,27 @@ fn marker_commands_trait_dispatch() { 0, ); Command::redo(&mut add); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 2 - ); + assert_eq!(list_of(&list_h).size(), 2); Command::undo(&mut add); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 1 - ); + assert_eq!(list_of(&list_h).size(), 1); let mut remove = MarkerRemoveCommand::new(list_h.clone(), 0); Command::redo(&mut remove); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 0 - ); + assert_eq!(list_of(&list_h).size(), 0); Command::undo(&mut remove); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 1 - ); + assert_eq!(list_of(&list_h).size(), 1); let mut color = MarkerChangeColorCommand::new(list_h.clone(), 0, 7); Command::redo(&mut color); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .color(), - 7 - ); + assert_eq!(list_of(&list_h).at(0).unwrap().color(), 7); Command::undo(&mut color); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .color(), - 1 - ); + assert_eq!(list_of(&list_h).at(0).unwrap().color(), 1); let mut name = MarkerChangeNameCommand::new(list_h.clone(), 0, "z"); Command::redo(&mut name); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .name(), - "z" - ); + assert_eq!(list_of(&list_h).at(0).unwrap().name(), "z"); Command::undo(&mut name); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .name(), - "m" - ); + assert_eq!(list_of(&list_h).at(0).unwrap().name(), "m"); let mut time = MarkerChangeTimeCommand::new( list_h.clone(), @@ -363,22 +312,12 @@ fn marker_commands_trait_dispatch() { ); Command::redo(&mut time); assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .time() - .in_(), + list_of(&list_h).at(0).unwrap().time().in_(), Rational::new(50, 1) ); Command::undo(&mut time); assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .time() - .in_(), + list_of(&list_h).at(0).unwrap().time().in_(), Rational::new(1, 1) ); } @@ -428,27 +367,18 @@ fn marker_add_command_redo_undo() { ); cmd.redo(); - let list = unsafe { get::(&list_h) }.unwrap(); + let list = list_of(&list_h); assert_eq!(list.size(), 1); assert_eq!(list.at(0).unwrap().name(), "m"); assert_eq!(list.at(0).unwrap().color(), 4); + drop(list); // Redo is idempotent. cmd.redo(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 1 - ); + assert_eq!(list_of(&list_h).size(), 1); cmd.undo(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 0 - ); + assert_eq!(list_of(&list_h).size(), 0); } /// A `MarkerRemoveCommand` redo drops the marker; undo re-inserts it @@ -456,26 +386,18 @@ fn marker_add_command_redo_undo() { #[test] fn marker_remove_command_redo_undo() { let list_h = make_owned(TimelineMarkerList::new()); - { - let l = unsafe { get_mut::(&list_h) }.unwrap(); - l.add_marker(TimelineMarker::with_time( - 1, - TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), - "m", - )); - } + list_of(&list_h).add_marker(TimelineMarker::with_time( + 1, + TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), + "m", + )); let mut cmd = MarkerRemoveCommand::new(list_h.clone(), 0); cmd.redo(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 0 - ); + assert_eq!(list_of(&list_h).size(), 0); cmd.undo(); - let list = unsafe { get::(&list_h) }.unwrap(); + let list = list_of(&list_h); assert_eq!(list.size(), 1); assert_eq!(list.at(0).unwrap().name(), "m"); } @@ -485,34 +407,17 @@ fn marker_remove_command_redo_undo() { #[test] fn marker_change_color_command_redo_undo() { let list_h = make_owned(TimelineMarkerList::new()); - { - let l = unsafe { get_mut::(&list_h) }.unwrap(); - l.add_marker(TimelineMarker::with_time( - 1, - TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), - "m", - )); - } + list_of(&list_h).add_marker(TimelineMarker::with_time( + 1, + TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), + "m", + )); let mut cmd = MarkerChangeColorCommand::new(list_h.clone(), 0, 9); cmd.redo(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .color(), - 9 - ); + assert_eq!(list_of(&list_h).at(0).unwrap().color(), 9); cmd.undo(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .color(), - 1 - ); + assert_eq!(list_of(&list_h).at(0).unwrap().color(), 1); } /// `MarkerChangeNameCommand` redo changes the name and undo restores @@ -520,34 +425,17 @@ fn marker_change_color_command_redo_undo() { #[test] fn marker_change_name_command_redo_undo() { let list_h = make_owned(TimelineMarkerList::new()); - { - let l = unsafe { get_mut::(&list_h) }.unwrap(); - l.add_marker(TimelineMarker::with_time( - 1, - TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), - "m", - )); - } + list_of(&list_h).add_marker(TimelineMarker::with_time( + 1, + TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), + "m", + )); let mut cmd = MarkerChangeNameCommand::new(list_h.clone(), 0, "renamed"); cmd.redo(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .name(), - "renamed" - ); + assert_eq!(list_of(&list_h).at(0).unwrap().name(), "renamed"); cmd.undo(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .name(), - "m" - ); + assert_eq!(list_of(&list_h).at(0).unwrap().name(), "m"); } /// `MarkerChangeTimeCommand` redo moves the marker and undo restores @@ -555,35 +443,22 @@ fn marker_change_name_command_redo_undo() { #[test] fn marker_change_time_command_redo_undo() { let list_h = make_owned(TimelineMarkerList::new()); - { - let l = unsafe { get_mut::(&list_h) }.unwrap(); - l.add_marker(TimelineMarker::with_time( - 1, - TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), - "m", - )); - } + list_of(&list_h).add_marker(TimelineMarker::with_time( + 1, + TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), + "m", + )); let new_t = TimeRange::new(Rational::new(50, 1), Rational::new(51, 1)); let mut cmd = MarkerChangeTimeCommand::new(list_h.clone(), 0, new_t); cmd.redo(); assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .time() - .in_(), + list_of(&list_h).at(0).unwrap().time().in_(), Rational::new(50, 1) ); cmd.undo(); assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .at(0) - .unwrap() - .time() - .in_(), + list_of(&list_h).at(0).unwrap().time().in_(), Rational::new(1, 1) ); } @@ -595,14 +470,11 @@ fn marker_change_time_command_redo_undo() { #[test] fn marker_commands_box_to_undo_command() { let list_h = make_owned(TimelineMarkerList::new()); - { - let l = unsafe { get_mut::(&list_h) }.unwrap(); - l.add_marker(TimelineMarker::with_time( - 1, - TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), - "m", - )); - } + list_of(&list_h).add_marker(TimelineMarker::with_time( + 1, + TimeRange::new(Rational::new(1, 1), Rational::new(2, 1)), + "m", + )); let mut cmd = MarkerAddCommand::new( list_h.clone(), @@ -612,19 +484,9 @@ fn marker_commands_box_to_undo_command() { ) .to_command(); cmd.redo_now(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 2 - ); + assert_eq!(list_of(&list_h).size(), 2); cmd.undo_now(); - assert_eq!( - unsafe { get::(&list_h) } - .unwrap() - .size(), - 1 - ); + assert_eq!(list_of(&list_h).size(), 1); } /// Loading a marker with a `color`/`in`/`out` attribute equal to the diff --git a/crates/oaktimeline/tests/workarea_test.rs b/crates/oaktimeline/tests/workarea_test.rs index ce30de310..dda703e42 100644 --- a/crates/oaktimeline/tests/workarea_test.rs +++ b/crates/oaktimeline/tests/workarea_test.rs @@ -19,13 +19,26 @@ //! `reset_in`/`reset_out` sentinels. The XML load/save contract left //! with the deleted C ABI export layer (single-lib unification). +use std::sync::{Arc, Mutex, MutexGuard}; + use oakcore_rs::{Rational, TimeRange}; -use oaktimeline::handle::{get, make_owned}; +use oaktimeline::handle::{get, make_owned, CHandle}; use oaktimeline::undocommon::Command; use oaktimeline::workarea::{ reset_in, reset_out, TimelineWorkArea, WorkareaSetEnabledCommand, WorkareaSetRangeCommand, }; +/// Lock the shared work area behind a live handle. Every work-area handle +/// in these tests comes from `make_owned`, which boxes an +/// `Arc>`. +fn wa_of(h: &CHandle) -> MutexGuard<'_, TimelineWorkArea> { + // SAFETY: as above; the handle is live. + unsafe { get::>>(h) } + .expect("live work-area handle") + .lock() + .unwrap_or_else(|e| e.into_inner()) +} + /// `reset_in` is the null rational (0/1) marking an unset work area /// start. #[test] @@ -96,9 +109,9 @@ fn workarea_set_enabled_command_redo_undo() { let wa_h = make_owned(TimelineWorkArea::new()); let mut cmd = WorkareaSetEnabledCommand::new(wa_h.clone(), true); cmd.redo(); - assert!(unsafe { get::(&wa_h) }.unwrap().enabled()); + assert!(wa_of(&wa_h).enabled()); cmd.undo(); - assert!(!unsafe { get::(&wa_h) }.unwrap().enabled()); + assert!(!wa_of(&wa_h).enabled()); } /// `WorkareaSetRangeCommand` redo stores the new range and undo @@ -109,20 +122,11 @@ fn workarea_set_range_command_redo_undo() { let new_range = TimeRange::new(Rational::new(10, 1), Rational::new(20, 1)); let mut cmd = WorkareaSetRangeCommand::new(wa_h.clone(), new_range); cmd.redo(); - assert_eq!( - *unsafe { get::(&wa_h) }.unwrap().range(), - new_range - ); + assert_eq!(*wa_of(&wa_h).range(), new_range); cmd.undo(); // Undo restores the range captured at construction (the reset range). - assert_eq!( - unsafe { get::(&wa_h) }.unwrap().in_(), - reset_in() - ); - assert_eq!( - unsafe { get::(&wa_h) }.unwrap().out(), - reset_out() - ); + assert_eq!(wa_of(&wa_h).in_(), reset_in()); + assert_eq!(wa_of(&wa_h).out(), reset_out()); } /// `to_command` boxes a work area command into an oakundo `UndoCommand` @@ -133,9 +137,9 @@ fn workarea_commands_box_to_undo_command() { let wa_h = make_owned(TimelineWorkArea::new()); let mut enabled_cmd = WorkareaSetEnabledCommand::new(wa_h.clone(), true).to_command(); enabled_cmd.redo_now(); - assert!(unsafe { get::(&wa_h) }.unwrap().enabled()); + assert!(wa_of(&wa_h).enabled()); enabled_cmd.undo_now(); - assert!(!unsafe { get::(&wa_h) }.unwrap().enabled()); + assert!(!wa_of(&wa_h).enabled()); let mut range_cmd = WorkareaSetRangeCommand::new( wa_h.clone(), @@ -143,15 +147,9 @@ fn workarea_commands_box_to_undo_command() { ) .to_command(); range_cmd.redo_now(); - assert_eq!( - unsafe { get::(&wa_h) }.unwrap().in_(), - Rational::new(1, 1) - ); + assert_eq!(wa_of(&wa_h).in_(), Rational::new(1, 1)); range_cmd.undo_now(); - assert_eq!( - unsafe { get::(&wa_h) }.unwrap().in_(), - reset_in() - ); + assert_eq!(wa_of(&wa_h).in_(), reset_in()); } /// `Command` trait dispatch routes through the same redo/undo bodies as @@ -162,22 +160,16 @@ fn workarea_commands_trait_dispatch() { let mut e = WorkareaSetEnabledCommand::new(wa_h.clone(), true); Command::redo(&mut e); - assert!(unsafe { get::(&wa_h) }.unwrap().enabled()); + assert!(wa_of(&wa_h).enabled()); Command::undo(&mut e); - assert!(!unsafe { get::(&wa_h) }.unwrap().enabled()); + assert!(!wa_of(&wa_h).enabled()); let mut r = WorkareaSetRangeCommand::new( wa_h.clone(), TimeRange::new(Rational::new(3, 1), Rational::new(4, 1)), ); Command::redo(&mut r); - assert_eq!( - unsafe { get::(&wa_h) }.unwrap().in_(), - Rational::new(3, 1) - ); + assert_eq!(wa_of(&wa_h).in_(), Rational::new(3, 1)); Command::undo(&mut r); - assert_eq!( - unsafe { get::(&wa_h) }.unwrap().in_(), - reset_in() - ); + assert_eq!(wa_of(&wa_h).in_(), reset_in()); } diff --git a/crates/oakundo/src/global.rs b/crates/oakundo/src/global.rs index c778e581e..c495e8d0e 100644 --- a/crates/oakundo/src/global.rs +++ b/crates/oakundo/src/global.rs @@ -24,41 +24,43 @@ //! lets downstream modules (the oakstorage write-through session manager) //! subscribe to "a command was recorded" notifications without a facade //! round-trip. +//! +//! M14 R5: everything in this module is Rust-typed — the process-wide +//! stack is a plain `static Mutex` and the open group holds an +//! [`UndoCommand`] value. The only remaining `CHandle` is the +//! [`push_or_run`] facade entry, which converts the incoming module +//! command handle into an [`UndoCommand`] value at the boundary. use std::ffi::{c_char, c_int, c_void}; +use std::panic::{catch_unwind, AssertUnwindSafe}; use std::sync::{Mutex, OnceLock}; -use crate::error::{Error, Result}; +use crate::error::{Error, OAKUNDO_E_FAILED, Result}; use crate::handle::CHandle; use crate::undocommand::{ - command_free, command_from_owned, command_init_multi, command_multi_add_child, - command_multi_child, command_multi_child_count, command_redo_now, command_undo_now, - UndoCommand, -}; -use crate::undostack::{ - undostack_can_redo, undostack_can_undo, undostack_clear, undostack_command_is_done, - undostack_command_text, undostack_count, undostack_index, undostack_init, undostack_jump, - undostack_push, undostack_push_pre_executed, + command_free, command_from_owned, command_redo_now, command_take, UndoCommand, }; +use crate::undostack::UndoStack; -/// The process-wide undo stack handle (`OakUndoStack`), created lazily -/// on first use and kept for the process lifetime. -fn global_stack() -> &'static CHandle { - static STACK: OnceLock = OnceLock::new(); - STACK.get_or_init(|| undostack_init()) +/// The process-wide undo stack, created lazily on first use and kept for +/// the process lifetime. +fn global_stack() -> &'static Mutex { + static STACK: OnceLock> = OnceLock::new(); + STACK.get_or_init(|| Mutex::new(UndoStack::new())) } /// Stable opaque token for the engine's `oakengine_undo_handle` export: -/// the stack handle's `ctx` pointer (never dereferenced by callers; lives -/// for the process). +/// the stack's address (never dereferenced by callers; lives for the +/// process). pub fn stack_token() -> *mut c_void { - global_stack().ctx + global_stack() as *const Mutex as *mut c_void } -/// Borrowed copy of the process-wide stack handle (for the module-level -/// queries below). -fn stack() -> CHandle { - *global_stack() +/// Run `f` on the process-wide stack; a poisoned mutex is recovered (its +/// inner value is still valid). +fn with_stack(f: impl FnOnce(&mut UndoStack) -> Result) -> Result { + let mut guard = global_stack().lock().unwrap_or_else(|e| e.into_inner()); + f(&mut guard) } // --------------------------------------------------------------------------- @@ -99,10 +101,10 @@ fn notify_observers() { // Undo group // --------------------------------------------------------------------------- -/// The currently open undo group (a multi command handle) plus its name. +/// The currently open undo group (a multi command value) plus its name. struct OpenGroup { - /// Multi command handle; owned by this state until end/abort. - multi: CHandle, + /// Multi command value; owned by this state until end/abort. + multi: UndoCommand, /// Group label. #[allow(dead_code)] name: String, @@ -121,12 +123,8 @@ pub fn group_begin(name: &str) -> Result<()> { if g.is_some() { return Err(Error::State); } - let multi = command_init_multi(); - if multi.is_null() { - return Err(Error::NoMem); - } *g = Some(OpenGroup { - multi, + multi: UndoCommand::multi(), name: name.to_string(), }); Ok(()) @@ -141,28 +139,16 @@ pub fn group_end() -> Result<()> { let multi = open.multi; let name = open.name; drop(g); - // Same NULL-for-empty convention as [`push_or_run`]: the module's - // `read_name` treats NULL like an empty label, while an empty String's - // dangling `as_ptr()` (0x1) would be strlen'd -> SIGSEGV. - let name_ptr = if name.is_empty() { - std::ptr::null() - } else { - name.as_ptr() as *const c_char - }; - // push_pre_executed discards an empty multi command. Either way the - // stack took (or destroyed) the command; release our own reference to - // the multi handle. - let rc = undostack_push_pre_executed(stack(), multi, name_ptr); - let mut multi_handle = multi; - command_free(&mut multi_handle); - if rc == 0 { - // The group's children were redo'd eagerly at push time; the whole - // group is one command (commit at group_end). - notify_observers(); + // push_pre_executed discards an empty multi command; either way the + // stack takes (or destroys) the command value. + with_stack(|s| { + s.push_pre_executed(multi, &name); Ok(()) - } else { - Err(Error::from_code(rc)) - } + })?; + // The group's children were redo'd eagerly at push time; the whole + // group is one command (commit at group_end). + notify_observers(); + Ok(()) } /// Undo all executed children and discard the group. [`Error::State`] when @@ -170,40 +156,17 @@ pub fn group_end() -> Result<()> { pub fn group_abort() -> Result<()> { let mut g = group_lock(); let open = g.take().ok_or(Error::State)?; + let mut multi = open.multi; drop(g); // The multi command itself is never marked done (each child was // redo'd eagerly at push time), so `undo_now` on it is a no-op. Undo // the executed children individually instead, in reverse insertion - // order (mirroring the multi's reverse-order undo), each through its - // own borrowed handle. - let mut count: c_int = 0; - let rc = command_multi_child_count(open.multi, &mut count); - if rc != 0 { - let mut multi = open.multi; - command_free(&mut multi); - return Err(Error::from_code(rc)); - } + // order (mirroring the multi's reverse-order undo). + let count = multi.multi_child_count(); for i in (0..count).rev() { - let mut child = CHandle::null(); - let rc = command_multi_child(open.multi, i, &mut child); - if rc != 0 { - let mut multi = open.multi; - command_free(&mut multi); - return Err(Error::from_code(rc)); - } - let rc = command_undo_now(child); - // The child handle is borrowed (owns:false): release only its shell - // — the child value lives on in the multi until the multi itself is - // freed below. - command_free(&mut child); - if rc != 0 { - let mut multi = open.multi; - command_free(&mut multi); - return Err(Error::from_code(rc)); - } + multi.multi_child_mut(i)?.undo_now(); } - let mut multi = open.multi; - command_free(&mut multi); + // The multi command value drops here, freeing the children. Ok(()) } @@ -215,37 +178,42 @@ pub fn group_abort() -> Result<()> { /// the STACK — a group child joins the group, and the group itself /// notifies at [`group_end`]. pub fn push_or_run(command: CHandle, name: &str) -> c_int { - let g = group_lock(); - if let Some(group) = g.as_ref() { - // The module's `command_multi_add_child` consumes the child's - // command value (command_take), so the eager redo must happen on the - // still-owned handle FIRST — the group takes the already-done - // command (C++ semantics: add_child + redo_now, net effect identical - // for the group's reverse-order undo). + let mut g = group_lock(); + if let Some(group) = g.as_mut() { + // The group takes the command value (command_take), so the eager + // redo must happen on the still-owned handle FIRST — the group + // receives an already-done command (C++ semantics: add_child + + // redo_now, net effect identical for the group's reverse-order + // undo). let rc = command_redo_now(command); if rc != 0 { return rc; } - let rc = command_multi_add_child(group.multi, command); - drop(g); - return rc; + // SAFETY: `command` is a live module command handle (facade + // contract); `command_take` moves the value out of its box and + // marks the shell non-owning. + return match unsafe { command_take(command.ctx) } { + Ok(cmd) => { + group.multi.multi_add_child(cmd); + 0 + } + Err(e) => e.code(), + }; } - let stack = stack(); - // The module treats a NULL name like an empty label, but an empty Rust - // String's `as_ptr()` is a DANGLING non-NULL pointer (0x1): the module's - // `read_name` would strlen it and SIGSEGV. Pass a real NULL instead. - let label_ptr = if name.is_empty() { - std::ptr::null() - } else { - name.as_ptr() as *const c_char + // No group open: take the command value and push it onto the + // process-wide stack (redo then record). + // SAFETY: as above, `command` is a live module command handle. + let cmd = match unsafe { command_take(command.ctx) } { + Ok(cmd) => cmd, + Err(e) => return e.code(), }; - let rc = undostack_push(stack, command, label_ptr); - if rc == 0 { - // The stack took a reference; the command's redo already ran (plan - // M13 D2): persist the write-through subscribers. - notify_observers(); - } - rc + let mut guard = global_stack().lock().unwrap_or_else(|e| e.into_inner()); + guard.push(cmd, name); + drop(guard); + // The stack took the command; its redo already ran (plan M13 D2): + // persist the write-through subscribers. + notify_observers(); + 0 } // --------------------------------------------------------------------------- @@ -309,87 +277,102 @@ pub fn redoable() -> bool { /// Total number of history rows. pub fn count() -> Result { - let mut c: i64 = 0; - let rc = undostack_count(stack(), &mut c); - if rc == 0 { - Ok(c) - } else { - Err(Error::from_code(rc)) - } + with_stack(|s| Ok(s.command_count())) } /// Current position in the history (done-command count). pub fn index() -> Result { - let mut i: i64 = 0; - let rc = undostack_index(stack(), &mut i); - if rc == 0 { - Ok(i) - } else { - Err(Error::from_code(rc)) - } + with_stack(|s| Ok(s.done_count())) } /// Whether an undo is possible (1/0 via `out_value`; a module error code /// otherwise). pub fn can_undo(out_value: *mut c_int) -> Result<()> { - let rc = undostack_can_undo(stack(), out_value); - if rc == 0 { - Ok(()) - } else { - Err(Error::from_code(rc)) + if out_value.is_null() { + return Err(Error::Invalid); } + let value = with_stack(|s| Ok(if s.can_undo() { 1 } else { 0 }))?; + // SAFETY: `out_value` points to at least one writable `c_int`, per the + // facade contract. + unsafe { *out_value = value }; + Ok(()) } /// Whether a redo is possible (1/0 via `out_value`; a module error code /// otherwise). pub fn can_redo(out_value: *mut c_int) -> Result<()> { - let rc = undostack_can_redo(stack(), out_value); - if rc == 0 { - Ok(()) - } else { - Err(Error::from_code(rc)) + if out_value.is_null() { + return Err(Error::Invalid); } + let value = with_stack(|s| Ok(if s.can_redo() { 1 } else { 0 }))?; + // SAFETY: `out_value` points to at least one writable `c_int`, per the + // facade contract. + unsafe { *out_value = value }; + Ok(()) } /// Undo/redo until the done-command count equals `index`. On success the /// bound projects are written through (the jump executed the undo/redo /// callbacks that mutated them) via the command observers. pub fn jump(index: i64) -> Result<()> { - let rc = undostack_jump(stack(), index); - if rc == 0 { - notify_observers(); + with_stack(|s| { + s.jump(index); Ok(()) - } else { - Err(Error::from_code(rc)) - } + })?; + notify_observers(); + Ok(()) } /// Delete all commands and push the fresh "New/Open Project" empty command. pub fn clear() -> Result<()> { - let rc = undostack_clear(stack()); - if rc == 0 { + with_stack(|s| { + s.clear(); Ok(()) - } else { - Err(Error::from_code(rc)) - } + }) } /// Two-stage label getter for the row at `row` (see /// [`crate::undostack::undostack_command_text`]): returns the required /// size including the NUL, or a module error code. pub fn command_text(row: i64, buf: *mut c_char, buf_size: c_int) -> c_int { - undostack_command_text(stack(), row, buf, buf_size) + let result = catch_unwind(AssertUnwindSafe(|| -> Result { + with_stack(|s| { + if row < 0 || row >= s.command_count() { + return Err(Error::NotFound); + } + let name = s.command_name(row)?; + let required = (name.len() + 1) as i32; + if !buf.is_null() && buf_size > 0 { + let copy_len = name.len().min((buf_size as usize).saturating_sub(1)); + let bytes = name.as_bytes(); + // SAFETY: `buf` points to `buf_size` writable bytes and we + // write at most `copy_len` (+ one NUL) of them. + unsafe { + std::ptr::copy_nonoverlapping(bytes.as_ptr(), buf as *mut u8, copy_len); + *buf.add(copy_len) = 0; + } + } + Ok(required) + }) + })); + match result { + Ok(Ok(required)) => required, + Ok(Err(e)) => e.code(), + Err(_) => OAKUNDO_E_FAILED, + } } /// Whether the row at `row` is done (1/0 via `out_value`; a module error /// code otherwise — `-20004` for an out-of-range row). pub fn command_is_done(row: i64, out_value: *mut c_int) -> Result<()> { - let rc = undostack_command_is_done(stack(), row, out_value); - if rc == 0 { - Ok(()) - } else { - Err(Error::from_code(rc)) + if out_value.is_null() { + return Err(Error::Invalid); } + let done = with_stack(|s| s.command_is_done(row))?; + // SAFETY: `out_value` points to at least one writable `c_int`, per the + // facade contract. + unsafe { *out_value = if done { 1 } else { 0 } }; + Ok(()) } #[cfg(test)] diff --git a/crates/oakundo/src/undocommand.rs b/crates/oakundo/src/undocommand.rs index 9d13d49af..fb018a0de 100644 --- a/crates/oakundo/src/undocommand.rs +++ b/crates/oakundo/src/undocommand.rs @@ -201,6 +201,14 @@ impl UndoCommand { } } + /// Mutable reference to the child at `index` of a composite command. + pub fn multi_child_mut(&mut self, index: usize) -> Result<&mut UndoCommand> { + match &mut self.kind { + CommandKind::Multi(m) => m.child_mut(index), + CommandKind::Vtable { .. } => Err(Error::Invalid), + } + } + /// `redo_now` semantics: a no-op if already done. pub fn redo_now(&mut self) { if !self.done { @@ -387,6 +395,11 @@ impl MultiUndoCommand { self.children.get(index).ok_or(Error::NotFound) } + /// Mutable child at `index`. + pub fn child_mut(&mut self, index: usize) -> Result<&mut UndoCommand> { + self.children.get_mut(index).ok_or(Error::NotFound) + } + /// Redo all children in order. pub fn redo(&mut self) { for child in self.children.iter_mut() { diff --git a/src/oakui/graphops.rs b/src/oakui/graphops.rs index 0a079aeed..3fdee525d 100644 --- a/src/oakui/graphops.rs +++ b/src/oakui/graphops.rs @@ -561,10 +561,14 @@ pub fn markers_of(list: &CHandle) -> Vec<(Rational, String, i32)> { } // SAFETY: `list` boxes a `TimelineMarkerList` (created by // `marker_list_create`); the read is shared and brief. - let Some(l) = (unsafe { oaktimeline::handle::get::(list) }) - else { + let Some(l) = (unsafe { + oaktimeline::handle::get::>>( + list, + ) + }) else { return Vec::new(); }; + let l = l.lock().unwrap_or_else(|e| e.into_inner()); (0..l.size()) .filter_map(|i| l.at(i)) .map(|m| (m.time().in_(), m.name().to_string(), m.color())) @@ -577,7 +581,12 @@ pub fn marker_index_at(list: &CHandle, time: Rational) -> Option { return None; } // SAFETY: as `markers_of`. - let l = unsafe { oaktimeline::handle::get::(list) }?; + let l = unsafe { + oaktimeline::handle::get::>>( + list, + ) + }?; + let l = l.lock().unwrap_or_else(|e| e.into_inner()); (0..l.size()).find(|&i| l.at(i).map(|m| m.time().in_()) == Some(time)) } @@ -588,7 +597,12 @@ pub fn workarea_state(wa: &CHandle) -> Option<(bool, TimeRange)> { } // SAFETY: `wa` boxes a `TimelineWorkArea` (created by // `workarea_create`); the read is shared and brief. - let w = unsafe { oaktimeline::handle::get::(wa) }?; + let w = unsafe { + oaktimeline::handle::get::>>( + wa, + ) + }?; + let w = w.lock().unwrap_or_else(|e| e.into_inner()); Some((w.enabled(), *w.range())) } @@ -599,8 +613,12 @@ pub fn workarea_set(wa: &CHandle, enabled: bool, range: TimeRange) { } // SAFETY: `wa` boxes a `TimelineWorkArea`; the engine writes it only // from the UI thread. - if let Some(w) = unsafe { oaktimeline::handle::get_mut::(wa) } - { + if let Some(w) = unsafe { + oaktimeline::handle::get_mut::>>( + wa, + ) + } { + let mut w = w.lock().unwrap_or_else(|e| e.into_inner()); w.set_enabled(enabled); w.set_range(range); } @@ -675,11 +693,9 @@ pub fn library_create(name: &str) -> Result { .insert("projectname".to_string(), name.to_string()); guard.uuid.clone() }; - let handle = oakstorage::nodeutil::make_project_owned(project); let result = oakstorage::writethrough::backend() - .save(handle, &uri, 0) + .save_project(&project, &uri, 0) .map_err(|e| e.to_string()); - oakstorage::nodeutil::release_project(handle); result?; Ok(uuid) } @@ -758,12 +774,8 @@ pub fn library_open(uuid: &str) -> Result { )); } let handle = result.project; - let project = oakstorage::nodeutil::project_arc_of(&handle) - .ok_or_else(|| "library load returned a foreign project handle".to_string()); - // The loaded handle's ownership moves to the caller's Arc; release the - // handle shell (the Arc keeps the project alive). - oakstorage::nodeutil::release_project(handle); - let project = project?; + let project = unsafe { oakstorage::nodeutil::project_arc(&handle) } + .map_err(|e| e.to_string())?; lock(&project).set_modified(false); Ok(project) }