From 3f89e7fdbb033c6beaf473d3a5a5ed188f06b9cd Mon Sep 17 00:00:00 2001 From: MarekSpodBiedry <73122968+MarekSpodBiedry@users.noreply.github.com> Date: Sun, 1 Nov 2026 21:15:03 +0100 Subject: [PATCH] Serialize bridge render_frame calls so parallel requests get their own frame (#3837) (#3872) --- crates/automation/src/server.rs | 9 ++++- crates/automation/src/server/tests.rs | 56 +++++++++++++++++++++++++++ docs/agents.md | 2 +- 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/crates/automation/src/server.rs b/crates/automation/src/server.rs index 2dac6366f..e71d30bb8 100644 --- a/crates/automation/src/server.rs +++ b/crates/automation/src/server.rs @@ -35,6 +35,9 @@ fn join_error(e: tokio::task::JoinError) -> AutomationError { #[derive(Clone)] pub struct FilmcraftMcp { backend: Arc, + /// Bridge `render_frame` moves the app's one playhead and screenshots the one Program monitor, so + /// calls (which MCP clients may send in parallel) take turns instead of seeing each other's frame. + render_lock: Arc>, tool_router: ToolRouter, } @@ -197,10 +200,10 @@ fn wrap(r: Result) -> Result { impl FilmcraftMcp { pub fn headless(session: Session) -> Self { - Self { backend: Arc::new(Backend::Headless(Arc::new(Mutex::new(session)))), tool_router: Self::tool_router() } + Self { backend: Arc::new(Backend::Headless(Arc::new(Mutex::new(session)))), render_lock: Arc::default(), tool_router: Self::tool_router() } } pub fn bridge(addr: &str) -> Result { - Ok(Self { backend: Arc::new(Backend::Bridge(Arc::new(BridgeClient::new(addr)?))), tool_router: Self::tool_router() }) + Ok(Self { backend: Arc::new(Backend::Bridge(Arc::new(BridgeClient::new(addr)?))), render_lock: Arc::default(), tool_router: Self::tool_router() }) } pub async fn serve_stdio(self) -> Result<(), AutomationError> { @@ -451,6 +454,8 @@ impl FilmcraftMcp { }) } Backend::Bridge(b) => { + // held until the playhead is back, also for the no-`seconds` screenshot, which must not catch another call's seek + let _turn = self.render_lock.lock().await; let Some(sec) = p.seconds else { return self.screenshot(&b.clone(), Some("Program".into()), max).await; }; diff --git a/crates/automation/src/server/tests.rs b/crates/automation/src/server/tests.rs index 8825af39b..da1a2247e 100644 --- a/crates/automation/src/server/tests.rs +++ b/crates/automation/src/server/tests.rs @@ -401,3 +401,59 @@ async fn doc_inspect_handles_an_active_sequence_id_above_i64_max() { assert_eq!(v["project"]["activeSequence"].as_u64(), Some(HUGE), "{v}"); assert_eq!(v["sequence"]["id"].as_u64(), Some(HUGE), "an id above i64::MAX must still be inspected, not dropped to a null sequence: {v}"); } + +/// Bridge `render_frame` with `seconds` moves the app's single playhead, screenshots the single +/// Program monitor and moves the playhead back. Parallel calls (the tool is annotated read-only and +/// idempotent, so clients send them that way) interleaved those steps and got each other's frames +/// (#3837). Each call has to return the frame at its own time. +#[tokio::test] +async fn parallel_bridge_render_frames_each_return_their_own_frame() { + use base64::Engine as _; + use tokio::io::{AsyncBufReadExt, AsyncWriteExt}; + let playhead = Arc::new(std::sync::atomic::AtomicI64::new(200)); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let app_playhead = playhead.clone(); + tokio::spawn(async move { + while let Ok((s, _)) = listener.accept().await { + let playhead = app_playhead.clone(); + tokio::spawn(async move { + let (r, mut w) = s.into_split(); + let mut lines = tokio::io::BufReader::new(r).lines(); + while let Ok(Some(line)) = lines.next_line().await { + let req: Value = serde_json::from_str(&line).unwrap(); + let params = &req["params"]["params"]; + let result = match (req["method"].as_str().unwrap_or_default(), req["params"]["command"].as_str().unwrap_or_default()) { + (_, "sequence.inspect") => json!({"playhead": playhead.load(std::sync::atomic::Ordering::SeqCst)}), + (_, "playhead.set") => { + let t = params["seconds"].as_f64().map(|x| x as i64).or_else(|| params["time"].as_i64()).unwrap_or(0); + playhead.store(t, std::sync::atomic::Ordering::SeqCst); + Value::Null + } + ("ui.screenshot", _) => { + // the monitor shows whatever the playhead is at when the screenshot is taken + tokio::time::sleep(std::time::Duration::from_millis(30)).await; + let shade = playhead.load(std::sync::atomic::Ordering::SeqCst) as u8; + let img = image::RgbaImage::from_pixel(1, 1, image::Rgba([shade, 0, 0, 255])); + img.save(req["params"]["path"].as_str().unwrap()).unwrap(); + Value::Null + } + _ => Value::Null, + }; + let line = format!("{}\n", json!({"id": req["id"], "ok": true, "result": result})); + w.write_all(line.as_bytes()).await.unwrap(); + } + }); + } + }); + let m = FilmcraftMcp::bridge(&addr).unwrap(); + let call = |seconds: f64| m.render_frame(Parameters(RenderParams { seconds: Some(seconds), max_side: Some(640) })); + let results = tokio::join!(call(5.0), call(20.0), call(35.0), call(50.0)); + for (res, want) in [(results.0, 5u8), (results.1, 20), (results.2, 35), (results.3, 50)] { + let v = serde_json::to_value(res.unwrap()).unwrap(); + let png = base64::engine::general_purpose::STANDARD.decode(v["content"][0]["data"].as_str().unwrap()).unwrap(); + let px = image::load_from_memory(&png).unwrap().to_rgba8().get_pixel(0, 0).0; + assert_eq!(px[0], want, "frame at {want} s came from another call's playhead"); + } + assert_eq!(playhead.load(std::sync::atomic::Ordering::SeqCst), 200, "the playhead must end where it started"); +} diff --git a/docs/agents.md b/docs/agents.md index 90f7fe532..17c5e3427 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -83,7 +83,7 @@ claude mcp add filmcraft -- filmcraft-cli mcp | `project_inspect` | both | bins and items with ids, types, durations; active sequence | | `sequence_inspect` | both | the active sequence: tracks, clips (`start` / `end` and `sourceIn` / `sourceOut` in ticks, frames, `speed`, `reverse`: the clip plays its `sourceIn` to `sourceOut` stretch backward, `gainDb`), effects, transitions, markers (name and `comment`), playhead, selection | | `media_import` | both | import files by absolute path (`paths`, an array; optional `bin`, `image_sequence`) | -| `render_frame` | both | PNG of the program frame at `seconds` (headless renders; bridge screenshots the Program monitor). Read-only: the playhead and selection are left as they were | +| `render_frame` | both | PNG of the program frame at `seconds` (headless renders; bridge screenshots the Program monitor). Read-only: the playhead and selection are left as they were. Parallel bridge calls take turns, so each returns its own frame | | `ui_inspect` | bridge | UI state: tool, workspace, panels, zoom, playback, fps | | `ui_elements` | bridge | on-screen interactive elements with id, label and rect (`prefix` filter) | | `ui_click` | bridge | click by `id` or `x`,`y`; `button`, `count`, `modifiers` |