From 3afa9c3a1d7469cf800381b7644ededf9ee2cdd8 Mon Sep 17 00:00:00 2001 From: 4gray Date: Fri, 4 Sep 2026 08:19:28 +0200 Subject: [PATCH] fix(playback): drop speculative position update from relative Embedded MPV seeks Review follow-up for the relative seek path. The macOS and Windows/Linux `seekBy` exports advanced `snapshot.positionSeconds` by the delta after dispatching the mpv command. That is not idempotent the way the absolute seek's optimistic write is: the observer (mpv event thread, or the Linux IPC poll) can already have stored the post-seek `time-pos` under the same mutex, so adding the delta on top counted the step twice, and while paused nothing corrected it. On Linux it also advertised a position that a failed socket delivery never reached. Relative steps now leave the snapshot alone; only the observed `time-pos` updates the position. The packaged Linux frame-copy smoke now drives `seekEmbeddedMpvBy` through the built app: a burst of three +2 s steps issued without waiting for snapshots has to land on 6 s, and a -60 s step has to clamp at 0. The generated Y4M fixture grows from 2 s to 12 s (about 415 KB) so the burst and the playing section that follows stay inside the clip. Replayed against a local mpv 0.39 with the same fixture and media server: burst -> 6.0, -60 -> 0. Co-Authored-By: Claude Fable 5.1 --- ...bedded-mpv-frame-copy-packaged-fixtures.ts | 14 ++++- .../embedded-mpv-frame-copy-packaged.e2e.ts | 55 ++++++++++++++++++- .../native/src/embedded_mpv.mm | 12 ++-- .../native/src/embedded_mpv_wid_common.h | 15 ++--- docs/architecture/embedded-mpv-native.md | 2 +- 5 files changed, 80 insertions(+), 18 deletions(-) diff --git a/apps/electron-backend-e2e/src/embedded-mpv-frame-copy-packaged-fixtures.ts b/apps/electron-backend-e2e/src/embedded-mpv-frame-copy-packaged-fixtures.ts index 31c888631..f83f55d37 100644 --- a/apps/electron-backend-e2e/src/embedded-mpv-frame-copy-packaged-fixtures.ts +++ b/apps/electron-backend-e2e/src/embedded-mpv-frame-copy-packaged-fixtures.ts @@ -27,11 +27,19 @@ export type LocalMediaServer = { url: string; }; -function createTwoSecondY4mFixture(): Buffer { +/** + * Length of the generated Y4M clip. Long enough for the relative-seek burst + * in the packaged smoke (three +2 s steps land at 6 s) plus the playing + * section that follows it without hitting EOF; at 64x36 / 10 fps this is + * still well under half a megabyte. + */ +export const Y4M_FIXTURE_DURATION_SECONDS = 12; + +function createY4mFixture(): Buffer { const width = 64; const height = 36; const framesPerSecond = 10; - const frameCount = framesPerSecond * 2; + const frameCount = framesPerSecond * Y4M_FIXTURE_DURATION_SECONDS; const yPlaneBytes = width * height; const chromaPlaneBytes = (width / 2) * (height / 2); const chunks: Buffer[] = [ @@ -84,7 +92,7 @@ async function closeServer(server: Server): Promise { } export async function createLocalMediaServer(): Promise { - const body = createTwoSecondY4mFixture(); + const body = createY4mFixture(); const resourcePath = '/embedded-mpv-frame-copy-smoke.y4m'; const server = createServer((request, response) => { const pathname = (request.url ?? '').split('?')[0]; diff --git a/apps/electron-backend-e2e/src/embedded-mpv-frame-copy-packaged.e2e.ts b/apps/electron-backend-e2e/src/embedded-mpv-frame-copy-packaged.e2e.ts index 0a900e5ca..67cbabc95 100644 --- a/apps/electron-backend-e2e/src/embedded-mpv-frame-copy-packaged.e2e.ts +++ b/apps/electron-backend-e2e/src/embedded-mpv-frame-copy-packaged.e2e.ts @@ -134,7 +134,7 @@ test.describe('Packaged Linux embedded MPV frame-copy runtime', () => { await window.electron.setEmbeddedMpvPaused(sessionId, true); await window.electron.loadEmbeddedMpvPlayback(sessionId, { streamUrl, - title: 'Two-second generated Y4M fixture', + title: 'Generated Y4M fixture', isLive: false, }); }, @@ -180,6 +180,59 @@ test.describe('Packaged Linux embedded MPV frame-copy runtime', () => { }) .toBeGreaterThan(0); + // Relative seek steps (arrow keys, ±10 s buttons) must reach mpv + // as `seek relative+exact` through the real preload → + // main-process → helper `seek-by` path, and a burst issued without + // waiting for snapshots has to accumulate. Absolute targets + // computed from the stale renderer snapshot collapsed such bursts + // onto one target (about one second of progress per press). + await launchedFrameCopyApp.mainWindow.evaluate( + async (sessionId) => { + const seekBy = window.electron.seekEmbeddedMpvBy; + if (!seekBy) { + throw new Error( + 'seekEmbeddedMpvBy is missing from the preload bridge' + ); + } + await Promise.all([ + seekBy(sessionId, 2), + seekBy(sessionId, 2), + seekBy(sessionId, 2), + ]); + }, + created.id + ); + await expect + .poll( + async () => + ( + await getLatestSession( + launchedFrameCopyApp, + created.id + ) + )?.positionSeconds, + { timeout: 15000 } + ) + .toBe(6); + // Stepping back past the start clamps at zero instead of failing. + await launchedFrameCopyApp.mainWindow.evaluate( + (sessionId) => + window.electron.seekEmbeddedMpvBy?.(sessionId, -60), + created.id + ); + await expect + .poll( + async () => + ( + await getLatestSession( + launchedFrameCopyApp, + created.id + ) + )?.positionSeconds, + { timeout: 15000 } + ) + .toBe(0); + await launchedFrameCopyApp.mainWindow.evaluate( (sessionId) => window.electron.setEmbeddedMpvPaused(sessionId, false), diff --git a/apps/electron-backend/native/src/embedded_mpv.mm b/apps/electron-backend/native/src/embedded_mpv.mm index 4d0c4c63f..3df139bae 100644 --- a/apps/electron-backend/native/src/embedded_mpv.mm +++ b/apps/electron-backend/native/src/embedded_mpv.mm @@ -2066,6 +2066,12 @@ Napi::Value SeekBy(const Napi::CallbackInfo& info) // against its own playback position and merges relative seeks that are // still queued, so a burst of presses accumulates instead of collapsing // onto one target computed from the renderer's stale snapshot. + // + // Unlike the absolute Seek above, the snapshot is deliberately NOT + // advanced here: the event thread may already have stored the observed + // post-seek `time-pos` under the same mutex, and adding the delta on top + // of that would count the step twice with nothing to correct it while + // paused. Only the observed `time-pos` updates the position. const char* command[] = { "seek", deltaValue.c_str(), @@ -2086,12 +2092,6 @@ Napi::Value SeekBy(const Napi::CallbackInfo& info) ); } - { - std::lock_guard lock(session->mutex); - session->snapshot.positionSeconds = - std::max(0.0, session->snapshot.positionSeconds + delta); - } - return env.Undefined(); } diff --git a/apps/electron-backend/native/src/embedded_mpv_wid_common.h b/apps/electron-backend/native/src/embedded_mpv_wid_common.h index a346fa5a5..aae6ff1c2 100644 --- a/apps/electron-backend/native/src/embedded_mpv_wid_common.h +++ b/apps/electron-backend/native/src/embedded_mpv_wid_common.h @@ -2014,13 +2014,19 @@ Napi::Value SeekBy(const Napi::CallbackInfo& info) // against its own playback position and merges relative seeks that are // still queued, so a burst of presses accumulates instead of collapsing // onto one target computed from the renderer's stale snapshot. + // + // Unlike the absolute Seek above, the snapshot is deliberately NOT + // advanced here: the observer (event thread or Linux IPC poll) may + // already have stored the post-seek `time-pos`, and adding the delta on + // top of that would count the step twice with nothing to correct it + // while paused; on Linux it would also advertise a position that a + // failed socket delivery never reached. Only the observed `time-pos` + // updates the position. #ifdef __linux__ std::string socketPath; { std::lock_guard lock(session->mutex); socketPath = session->mpvIpcSocketPath; - session->snapshot.positionSeconds = - std::max(0.0, session->snapshot.positionSeconds + delta); } if (!socketPath.empty()) { sendLinuxMpvCommand( @@ -2041,11 +2047,6 @@ Napi::Value SeekBy(const Napi::CallbackInfo& info) if (result < 0) { throw Napi::Error::New(env, mpv_error_string(result)); } - { - std::lock_guard lock(session->mutex); - session->snapshot.positionSeconds = - std::max(0.0, session->snapshot.positionSeconds + delta); - } return env.Undefined(); } diff --git a/docs/architecture/embedded-mpv-native.md b/docs/architecture/embedded-mpv-native.md index ac812d901..e26770d91 100644 --- a/docs/architecture/embedded-mpv-native.md +++ b/docs/architecture/embedded-mpv-native.md @@ -476,7 +476,7 @@ VOD and episode payloads carry `contentInfo` and are treated as non-live unless Live catchup is different: the catchup URL already encodes the archive window, so live catchup playback must not pass an absolute Unix timestamp as `startTime`. -Seeking has two IPC shapes. The timeline scrub commits one absolute target (`seekEmbeddedMpv` → mpv `seek absolute`). Arrow-key and ±10 s button steps go through `seekEmbeddedMpvBy`, which every backend forwards as a relative mpv seek (`seek relative+exact`): the macOS and Windows addons via their `seekBy` export, the frame-copy helper via the `seek-by\tseconds=` stdin command, and Linux over the MPV JSON IPC socket. The renderer must never derive an absolute target for a step from `session.positionSeconds`: that value is floored to whole seconds and refreshed at most every 500 ms (the helper emits snapshots at most every 250 ms), and a seek reply does not carry the new position yet, so every press inside that window landed on the same target and a burst of presses advanced by roughly one second each. mpv resolves relative seeks against its own position and merges the ones still queued, so presses accumulate exactly as they do in mpv itself. `EmbeddedMpvNativeService.seekBy` keeps an absolute fallback computed from the addon's own snapshot only for an addon binary built before `seekBy` existed, and `EmbeddedMpvCommandRunner.seekBy` keeps the same fallback for a preload without `seekEmbeddedMpvBy`. +Seeking has two IPC shapes. The timeline scrub commits one absolute target (`seekEmbeddedMpv` → mpv `seek absolute`). Arrow-key and ±10 s button steps go through `seekEmbeddedMpvBy`, which every backend forwards as a relative mpv seek (`seek relative+exact`): the macOS and Windows addons via their `seekBy` export, the frame-copy helper via the `seek-by\tseconds=` stdin command, and Linux over the MPV JSON IPC socket. The renderer must never derive an absolute target for a step from `session.positionSeconds`: that value is floored to whole seconds and refreshed at most every 500 ms (the helper emits snapshots at most every 250 ms), and a seek reply does not carry the new position yet, so every press inside that window landed on the same target and a burst of presses advanced by roughly one second each. mpv resolves relative seeks against its own position and merges the ones still queued, so presses accumulate exactly as they do in mpv itself. `EmbeddedMpvNativeService.seekBy` keeps an absolute fallback computed from the addon's own snapshot only for an addon binary built before `seekBy` existed, and `EmbeddedMpvCommandRunner.seekBy` keeps the same fallback for a preload without `seekEmbeddedMpvBy`. Unlike the absolute seek, a relative step never speculates about the resulting position in the snapshot: only mpv's observed `time-pos` updates it, because an optimistic `position + delta` could land on top of an observer write that already reflects the completed seek and count the step twice, with nothing to correct it while paused (on Linux it would also advertise a position that a failed socket delivery never reached). The packaged Linux frame-copy smoke (`electron-backend-e2e:packaged-frame-copy-smoke`) drives a burst of `seekEmbeddedMpvBy` calls through the built app and asserts the accumulated position. Audio tracks are discovered from MPV's `track-list` property. The selected track is controlled through MPV's `aid` property. Switching tracks must not reload the stream.