From 0065fc17fe3256342e1500069a7a4999ef8e9f19 Mon Sep 17 00:00:00 2001 From: 4gray Date: Sat, 9 May 2026 22:31:17 +0200 Subject: [PATCH] fix(playback): address embedded mpv recording review Entire-Checkpoint: f957cd9849e0 --- .../native/src/embedded_mpv.mm | 122 +++++++++++++++--- .../embedded-mpv-native.service.spec.ts | 101 ++++++++++++++- .../services/embedded-mpv-native.service.ts | 76 +++++++++-- 3 files changed, 265 insertions(+), 34 deletions(-) diff --git a/apps/electron-backend/native/src/embedded_mpv.mm b/apps/electron-backend/native/src/embedded_mpv.mm index 120109c60..4bd14100b 100644 --- a/apps/electron-backend/native/src/embedded_mpv.mm +++ b/apps/electron-backend/native/src/embedded_mpv.mm @@ -127,6 +127,11 @@ struct Session { std::atomic totalRenderNanoseconds{0}; std::mutex mutex; SessionSnapshot snapshot; + uint64_t pendingRecordingStartRequestId = 0; + uint64_t pendingRecordingStopRequestId = 0; + std::string pendingRecordingTargetPath; + std::string pendingRecordingStartedAt; + std::string pendingRecordingStopStartedAt; std::weak_ptr* renderCallbackContext = nullptr; RenderBackend renderBackend = RenderBackend::OpenGL; int renderWidthPixels = 0; @@ -1030,6 +1035,59 @@ std::string currentUtcTimestamp() return buffer; } +bool reconcileRecordingPropertyReply( + const std::shared_ptr& session, + uint64_t requestId, + int error +) +{ + if (requestId == 0) { + return false; + } + + if (requestId == session->pendingRecordingStartRequestId) { + session->pendingRecordingStartRequestId = 0; + const std::string targetPath = session->pendingRecordingTargetPath; + const std::string startedAt = session->pendingRecordingStartedAt; + session->pendingRecordingTargetPath.clear(); + session->pendingRecordingStartedAt.clear(); + + if (error < 0) { + session->snapshot.recordingActive = false; + session->snapshot.recordingTargetPath = targetPath; + session->snapshot.recordingStartedAt.clear(); + session->snapshot.recordingError = mpv_error_string(error); + return true; + } + + session->snapshot.recordingActive = true; + session->snapshot.recordingTargetPath = targetPath; + session->snapshot.recordingStartedAt = startedAt; + session->snapshot.recordingError.clear(); + return true; + } + + if (requestId == session->pendingRecordingStopRequestId) { + session->pendingRecordingStopRequestId = 0; + const std::string startedAt = session->pendingRecordingStopStartedAt; + session->pendingRecordingStopStartedAt.clear(); + + if (error < 0) { + session->snapshot.recordingActive = true; + session->snapshot.recordingStartedAt = startedAt; + session->snapshot.recordingError = mpv_error_string(error); + return true; + } + + session->snapshot.recordingActive = false; + session->snapshot.recordingStartedAt.clear(); + session->snapshot.recordingError.clear(); + return true; + } + + return false; +} + double clampVolumePercent(double volume) { return std::clamp(volume, 0.0, 1.0) * 100.0; @@ -1227,6 +1285,13 @@ void runEventLoop(const std::shared_ptr& session) } case MPV_EVENT_COMMAND_REPLY: case MPV_EVENT_SET_PROPERTY_REPLY: + if (reconcileRecordingPropertyReply( + session, + event->reply_userdata, + event->error + )) { + break; + } if (event->error < 0) { session->snapshot.status = SessionStatus::Error; session->snapshot.error = mpv_error_string(event->error); @@ -2095,9 +2160,22 @@ Napi::Value StartRecording(const Napi::CallbackInfo& info) const auto session = getSessionOrThrow(env, sessionId); const char* targetValue = targetPath.c_str(); + const uint64_t requestId = nextAsyncRequestId(); + const std::string startedAt = currentUtcTimestamp(); + { + std::lock_guard lock(session->mutex); + session->pendingRecordingStartRequestId = requestId; + session->pendingRecordingTargetPath = targetPath; + session->pendingRecordingStartedAt = startedAt; + session->snapshot.recordingActive = true; + session->snapshot.recordingTargetPath = targetPath; + session->snapshot.recordingStartedAt = startedAt; + session->snapshot.recordingError.clear(); + } + const int result = mpv_set_property_async( session->handle, - nextAsyncRequestId(), + requestId, "stream-record", MPV_FORMAT_STRING, const_cast(&targetValue) @@ -2106,7 +2184,13 @@ Napi::Value StartRecording(const Napi::CallbackInfo& info) if (result < 0) { { std::lock_guard lock(session->mutex); + if (session->pendingRecordingStartRequestId == requestId) { + session->pendingRecordingStartRequestId = 0; + session->pendingRecordingTargetPath.clear(); + session->pendingRecordingStartedAt.clear(); + } session->snapshot.recordingActive = false; + session->snapshot.recordingStartedAt.clear(); session->snapshot.recordingError = mpv_error_string(result); } throw Napi::Error::New( @@ -2116,14 +2200,6 @@ Napi::Value StartRecording(const Napi::CallbackInfo& info) ); } - { - std::lock_guard lock(session->mutex); - session->snapshot.recordingActive = true; - session->snapshot.recordingTargetPath = targetPath; - session->snapshot.recordingStartedAt = currentUtcTimestamp(); - session->snapshot.recordingError.clear(); - } - return env.Undefined(); } @@ -2137,9 +2213,20 @@ Napi::Value StopRecording(const Napi::CallbackInfo& info) const std::string sessionId = info[0].As().Utf8Value(); const auto session = getSessionOrThrow(env, sessionId); const char* disabledValue = ""; + const uint64_t requestId = nextAsyncRequestId(); + { + std::lock_guard lock(session->mutex); + session->pendingRecordingStopRequestId = requestId; + session->pendingRecordingStopStartedAt = + session->snapshot.recordingStartedAt; + session->snapshot.recordingActive = false; + session->snapshot.recordingStartedAt.clear(); + session->snapshot.recordingError.clear(); + } + const int result = mpv_set_property_async( session->handle, - nextAsyncRequestId(), + requestId, "stream-record", MPV_FORMAT_STRING, const_cast(&disabledValue) @@ -2148,6 +2235,14 @@ Napi::Value StopRecording(const Napi::CallbackInfo& info) if (result < 0) { { std::lock_guard lock(session->mutex); + const std::string startedAt = + session->pendingRecordingStopStartedAt; + if (session->pendingRecordingStopRequestId == requestId) { + session->pendingRecordingStopRequestId = 0; + session->pendingRecordingStopStartedAt.clear(); + } + session->snapshot.recordingActive = true; + session->snapshot.recordingStartedAt = startedAt; session->snapshot.recordingError = mpv_error_string(result); } throw Napi::Error::New( @@ -2157,13 +2252,6 @@ Napi::Value StopRecording(const Napi::CallbackInfo& info) ); } - { - std::lock_guard lock(session->mutex); - session->snapshot.recordingActive = false; - session->snapshot.recordingStartedAt.clear(); - session->snapshot.recordingError.clear(); - } - return env.Undefined(); } diff --git a/apps/electron-backend/src/app/services/embedded-mpv-native.service.spec.ts b/apps/electron-backend/src/app/services/embedded-mpv-native.service.spec.ts index 82dfc16dc..a3c5da0fe 100644 --- a/apps/electron-backend/src/app/services/embedded-mpv-native.service.spec.ts +++ b/apps/electron-backend/src/app/services/embedded-mpv-native.service.spec.ts @@ -3,6 +3,9 @@ import type { EmbeddedMpvSessionStatus, ResolvedPortalPlayback, } from 'shared-interfaces'; +import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import path from 'path'; import type { EmbeddedMpvNativeService as EmbeddedMpvNativeServiceType } from './embedded-mpv-native.service'; const powerSaveBlockerMock = { @@ -95,6 +98,7 @@ describe('EmbeddedMpvNativeService power blocker', () => { let addon: MockAddon; let nextBlockerId: number; let originalPlatform: NodeJS.Platform; + let tempDirs: string[]; beforeEach(async () => { jest.resetModules(); @@ -103,6 +107,7 @@ describe('EmbeddedMpvNativeService power blocker', () => { powerSaveBlockerMock.isStarted.mockReset(); mainWindowSendMock.mockReset(); + tempDirs = []; nextBlockerId = 1; powerSaveBlockerMock.start.mockImplementation(() => nextBlockerId++); powerSaveBlockerMock.isStarted.mockReturnValue(true); @@ -123,11 +128,22 @@ describe('EmbeddedMpvNativeService power blocker', () => { afterEach(() => { service.shutdown(); jest.useRealTimers(); + for (const tempDir of tempDirs) { + rmSync(tempDir, { recursive: true, force: true }); + } Object.defineProperty(process, 'platform', { value: originalPlatform, }); }); + function createTempDir(): string { + const tempDir = mkdtempSync( + path.join(tmpdir(), 'iptvnator-recording-') + ); + tempDirs.push(tempDir); + return tempDir; + } + function startSession(sessionId: string, snapshot: MockSnapshot): void { addon.createSession.mockReturnValueOnce(sessionId); addon.getSessionSnapshot.mockReturnValueOnce(snapshot); @@ -249,12 +265,13 @@ describe('EmbeddedMpvNativeService power blocker', () => { expect(support.capabilities?.recording).toBe(true); }); - it('starts recording to a sanitized unique ts file in the requested directory', () => { + it('starts recording to a sanitized reserved unique ts file in the requested directory', () => { jest.useFakeTimers(); jest.setSystemTime(new Date(2026, 4, 9, 10, 11, 12)); + const directory = createTempDir(); startSession('s1', snapshot('playing')); - const targetPath = '/tmp/News_Live-20260509-101112.ts'; + const targetPath = path.join(directory, 'News_Live-20260509-101112.ts'); addon.getSessionSnapshot.mockReturnValueOnce( snapshot('playing', { recording: { @@ -266,15 +283,53 @@ describe('EmbeddedMpvNativeService power blocker', () => { ); const updated = service.startRecording('s1', { - directory: '/tmp', + directory, title: 'News/Live', }); expect(addon.startRecording).toHaveBeenCalledWith('s1', targetPath); + expect(existsSync(targetPath)).toBe(true); expect(updated?.recording?.active).toBe(true); expect(updated?.recording?.targetPath).toBe(targetPath); }); + it('reserves the next unique recording path when the first candidate exists', () => { + jest.useFakeTimers(); + jest.setSystemTime(new Date(2026, 4, 9, 10, 11, 12)); + const directory = createTempDir(); + const firstCandidate = path.join( + directory, + 'News_Live-20260509-101112.ts' + ); + const reservedCandidate = path.join( + directory, + 'News_Live-20260509-101112-2.ts' + ); + writeFileSync(firstCandidate, 'existing recording'); + startSession('s1', snapshot('playing')); + + addon.getSessionSnapshot.mockReturnValueOnce( + snapshot('playing', { + recording: { + active: true, + targetPath: reservedCandidate, + }, + }) + ); + + service.startRecording('s1', { + directory, + title: 'News/Live', + }); + + expect(addon.startRecording).toHaveBeenCalledWith( + 's1', + reservedCandidate + ); + expect(existsSync(firstCandidate)).toBe(true); + expect(existsSync(reservedCandidate)).toBe(true); + }); + it('stops recording and keeps the last target path in the session snapshot', () => { const targetPath = '/tmp/News_Live-20260509-101112.ts'; startSession( @@ -302,4 +357,44 @@ describe('EmbeddedMpvNativeService power blocker', () => { expect(updated?.recording?.active).toBe(false); expect(updated?.recording?.targetPath).toBe(targetPath); }); + + it('preserves the recording target path when disposing an active recording session', () => { + const targetPath = '/tmp/News_Live-20260509-101112.ts'; + startSession( + 's1', + snapshot('playing', { + recording: { + active: true, + targetPath, + startedAt: '2026-05-09T08:11:12.000Z', + }, + }) + ); + addon.getSessionSnapshot.mockReturnValueOnce( + snapshot('playing', { + recording: { + active: true, + targetPath, + startedAt: '2026-05-09T08:11:12.000Z', + }, + }) + ); + + const disposed = service.disposeSession('s1'); + + expect(addon.disposeSession).toHaveBeenCalledWith('s1'); + expect(disposed?.recording).toEqual({ + active: false, + targetPath, + }); + expect(mainWindowSendMock).toHaveBeenLastCalledWith( + 'EMBEDDED_MPV_SESSION_UPDATE', + expect.objectContaining({ + recording: { + active: false, + targetPath, + }, + }) + ); + }); }); diff --git a/apps/electron-backend/src/app/services/embedded-mpv-native.service.ts b/apps/electron-backend/src/app/services/embedded-mpv-native.service.ts index 427c70611..aaa49c35d 100644 --- a/apps/electron-backend/src/app/services/embedded-mpv-native.service.ts +++ b/apps/electron-backend/src/app/services/embedded-mpv-native.service.ts @@ -1,5 +1,12 @@ import { app, dialog, powerSaveBlocker } from 'electron'; -import { existsSync, mkdirSync, readFileSync } from 'fs'; +import { + closeSync, + existsSync, + mkdirSync, + openSync, + readFileSync, + unlinkSync, +} from 'fs'; import { createRequire } from 'module'; import path from 'path'; import App from '../app'; @@ -383,11 +390,16 @@ export class EmbeddedMpvNativeService { options.directory?.trim() || this.getDefaultRecordingFolder(); mkdirSync(directory, { recursive: true }); - const targetPath = this.resolveRecordingTargetPath( + const targetPath = this.reserveRecordingTargetPath( directory, options.title || session.title || 'IPTVnator recording' ); - addon.startRecording(sessionId, targetPath); + try { + addon.startRecording(sessionId, targetPath); + } catch (error) { + this.releaseReservedRecordingTargetPath(targetPath); + throw error; + } return this.refreshSession(sessionId); } @@ -427,8 +439,15 @@ export class EmbeddedMpvNativeService { return null; } + let lastRecording: EmbeddedMpvRecordingState | undefined; try { - this.getAddon().disposeSession(sessionId); + const addon = this.getAddon(); + try { + lastRecording = addon.getSessionSnapshot(sessionId)?.recording; + } catch { + lastRecording = undefined; + } + addon.disposeSession(sessionId); } finally { this.sessions.delete(sessionId); const payload: EmbeddedMpvSession = { @@ -445,7 +464,7 @@ export class EmbeddedMpvNativeService { selectedSubtitleTrackId: null, playbackSpeed: 1, aspectOverride: 'no', - recording: { active: false }, + recording: this.createClosedRecordingState(lastRecording), startedAt: session.startedAt, updatedAt: new Date().toISOString(), }; @@ -614,7 +633,7 @@ export class EmbeddedMpvNativeService { return App.mainWindow.getNativeWindowHandle(); } - private resolveRecordingTargetPath( + private reserveRecordingTargetPath( directory: string, title: string ): string { @@ -623,15 +642,44 @@ export class EmbeddedMpvNativeService { let candidate = path.join(directory, `${baseName}-${timestamp}.ts`); let suffix = 2; - while (existsSync(candidate)) { - candidate = path.join( - directory, - `${baseName}-${timestamp}-${suffix}.ts` - ); - suffix += 1; - } + while (true) { + try { + const fd = openSync(candidate, 'wx'); + closeSync(fd); + return candidate; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'EEXIST') { + candidate = path.join( + directory, + `${baseName}-${timestamp}-${suffix}.ts` + ); + suffix += 1; + continue; + } - return candidate; + throw error; + } + } + } + + private releaseReservedRecordingTargetPath(targetPath: string): void { + try { + unlinkSync(targetPath); + } catch { + // Ignore cleanup failures; the start error is more useful. + } + } + + private createClosedRecordingState( + recording?: EmbeddedMpvRecordingState + ): EmbeddedMpvRecordingState { + return { + active: false, + ...(recording?.targetPath + ? { targetPath: recording.targetPath } + : {}), + ...(recording?.error ? { error: recording.error } : {}), + }; } private sanitizeRecordingFileName(title: string): string {