feat(player): add embedded-mpv player for macOS as experimental feature#877
feat(player): add embedded-mpv player for macOS as experimental feature#877
Conversation
- Introduced tooling for building and staging the macOS `libmpv` runtime for IPTVnator's embedded MPV player. - Added `build-macos-runtime.mjs` for building an LGPL-compatible runtime from source. - Created `stage-macos-runtime.mjs` for staging the built runtime artifacts. - Implemented validation for the packaged embedded MPV runtime in `electron-after-pack.cjs` and `embedded-mpv-macos.cjs`. - Updated packaging scripts to ensure the embedded MPV runtime is correctly integrated and validated during the build process. - Added README files to document the expected layout and usage for the embedded MPV runtime artifacts. Entire-Checkpoint: c6e522b4276c
Greptile SummaryThis PR introduces an embedded MPV player for macOS as an experimental feature, implementing a full native Objective-C++/NAPI addon ( Confidence Score: 4/5PR is safe to merge; all findings are P2 suggestions with no blocking defects. No P0 or P1 issues were found. The volume scale conversion, session lifecycle, IPC bridge, and packaging validation all appear correct. Findings are limited to: a misleading no-op bootstrap method, missing pinned checksums in the build script, a silent wrong-directory fallback in the after-pack hook, and a per-mousemove getBoundingClientRect call that may show up in profiling. tools/packaging/electron-after-pack.cjs (silent fallback when .app not found), tools/embedded-mpv/build-macos-runtime.mjs (no checksum pinning) Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Angular Component
participant Preload as Preload (IPC Bridge)
participant Events as EmbeddedMpvEvents (main)
participant Service as EmbeddedMpvNativeService
participant Addon as Native Addon (.node)
participant MPV as libmpv
UI->>Preload: getEmbeddedMpvSupport()
Preload->>Events: IPC: EMBEDDED_MPV_SUPPORT
Events->>Service: getSupport()
Service-->>UI: EmbeddedMpvSupport
UI->>Preload: prepareEmbeddedMpv()
Preload->>Events: IPC: EMBEDDED_MPV_PREPARE
Events->>Service: prepareAddon() loads .node
Service->>Addon: isSupported()
Addon-->>Service: true
UI->>Preload: createEmbeddedMpvSession(bounds, title, vol)
Preload->>Events: IPC: EMBEDDED_MPV_CREATE_SESSION
Events->>Service: createSession(bounds, title, vol)
Service->>Addon: createSession(windowHandle, bounds, title, vol)
Addon->>MPV: mpv_create() + mpv_initialize()
Addon-->>Service: sessionId
Service-->>UI: EmbeddedMpvSession
UI->>Preload: loadEmbeddedMpvPlayback(sessionId, playback)
Preload->>Events: IPC: EMBEDDED_MPV_LOAD_PLAYBACK
Events->>Service: loadPlayback(sessionId, playback)
Service->>Addon: loadPlayback(sessionId, playback)
Addon->>MPV: mpv_command loadfile url
loop 500ms polling
Service->>Addon: getSessionSnapshot(sessionId)
Addon-->>Service: NativeSnapshot
Service->>Preload: webContents.send EMBEDDED_MPV_SESSION_UPDATE
Preload-->>UI: onEmbeddedMpvSessionUpdate callback
end
UI->>Preload: disposeEmbeddedMpvSession(sessionId)
Preload->>Events: IPC: EMBEDDED_MPV_DISPOSE_SESSION
Events->>Service: disposeSession(sessionId)
Service->>Addon: disposeSession(sessionId)
Addon->>MPV: mpv_terminate_destroy()
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/electron-backend/src/app/events/embedded-mpv.events.ts
Line: 20-24
Comment:
**`bootstrapEmbeddedMpvEvents()` is a no-op stub**
The method only returns `ipcMain` without registering anything. All IPC handlers are registered at module scope the moment this file is imported, so the `bootstrapEmbeddedMpvEvents()` call in `main.ts` has no effect. This mirrors the pattern of other event classes but is misleading here because the side-effectful work happens at import time rather than in the method, making it easy to accidentally move the import without realising the call order matters.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tools/embedded-mpv/build-macos-runtime.mjs
Line: 226-258
Comment:
**No pinned checksum verification for downloaded sources**
`sha256File` is called after each download and the result is stored in `sourcePackage.sha256`, but it is only recorded into the build manifest — it is never compared against a known-good expected hash. If any of the upstream URLs were compromised (MITM, CDN tampering) the build would silently proceed with the corrupt archive. Since these are HTTPS downloads from reputable sources the practical risk is low, but for a reproducible build tool that vendors security-sensitive native code it is worth pinning expected SHA-256 values next to each entry in `sourcePackages` and failing fast on a mismatch.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tools/packaging/electron-after-pack.cjs
Line: 35-48
Comment:
**Silent fallback to wrong `resourceDir` when no `.app` bundle is found**
If `params.appOutDir` neither ends with `.app` nor contains any directory entry ending with `.app`, `appPath` resolves to `undefined`. The ternary then sets `resourceDir = params.appOutDir`, so `validatePackagedEmbeddedMpv` looks inside the raw output directory rather than inside `Contents/Resources`. For a non-required run this silently passes; for a required run the errors would reference paths under the wrong root. Consider throwing early when `appPath` is `undefined` to surface the misconfiguration immediately.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-player.component.ts
Line: 139-145
Comment:
**Global `document` `pointermove` listener fires `getBoundingClientRect` on every cursor move**
`onDocumentPointerMove` is a passive document-level listener that calls `isPointerInsidePlayer` (which invokes `getBoundingClientRect`) on every single pointer-move event in the entire page. While `passive: true` prevents scroll blocking, a forced layout reflow on every mouse movement can be visible in profiling. Consider throttling with `requestAnimationFrame` or using an `IntersectionObserver`/`mouseenter` approach so the bounds check is only paid when the pointer is already known to be near the component.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(player): add embedded-mpv player fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| export default class EmbeddedMpvEvents { | ||
| static bootstrapEmbeddedMpvEvents(): Electron.IpcMain { | ||
| return ipcMain; | ||
| } |
There was a problem hiding this comment.
bootstrapEmbeddedMpvEvents() is a no-op stub
The method only returns ipcMain without registering anything. All IPC handlers are registered at module scope the moment this file is imported, so the bootstrapEmbeddedMpvEvents() call in main.ts has no effect. This mirrors the pattern of other event classes but is misleading here because the side-effectful work happens at import time rather than in the method, making it easy to accidentally move the import without realising the call order matters.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/electron-backend/src/app/events/embedded-mpv.events.ts
Line: 20-24
Comment:
**`bootstrapEmbeddedMpvEvents()` is a no-op stub**
The method only returns `ipcMain` without registering anything. All IPC handlers are registered at module scope the moment this file is imported, so the `bootstrapEmbeddedMpvEvents()` call in `main.ts` has no effect. This mirrors the pattern of other event classes but is misleading here because the side-effectful work happens at import time rather than in the method, making it easy to accidentally move the import without realising the call order matters.
How can I resolve this? If you propose a fix, please make it concise.| function downloadSources() { | ||
| fs.mkdirSync(archiveRoot, { recursive: true }); | ||
| fs.mkdirSync(sourceRoot, { recursive: true }); | ||
|
|
||
| for (const sourcePackage of sourcePackages) { | ||
| const archivePath = archivePathFor(sourcePackage); | ||
| if (!fs.existsSync(archivePath)) { | ||
| run('curl', [ | ||
| '-fL', | ||
| '--retry', | ||
| '3', | ||
| '--retry-delay', | ||
| '5', | ||
| '-o', | ||
| archivePath, | ||
| sourcePackage.url, | ||
| ]); | ||
| } | ||
|
|
||
| const packageSourcePath = sourcePathFor(sourcePackage.id); | ||
| fs.rmSync(packageSourcePath, { recursive: true, force: true }); | ||
| fs.mkdirSync(packageSourcePath, { recursive: true }); | ||
| run('tar', [ | ||
| '-xf', | ||
| archivePath, | ||
| '-C', | ||
| packageSourcePath, | ||
| '--strip-components', | ||
| '1', | ||
| ]); | ||
| sourcePackage.sha256 = sha256File(archivePath); | ||
| } | ||
| } |
There was a problem hiding this comment.
sha256File is called after each download and the result is stored in sourcePackage.sha256, but it is only recorded into the build manifest — it is never compared against a known-good expected hash. If any of the upstream URLs were compromised (MITM, CDN tampering) the build would silently proceed with the corrupt archive. Since these are HTTPS downloads from reputable sources the practical risk is low, but for a reproducible build tool that vendors security-sensitive native code it is worth pinning expected SHA-256 values next to each entry in sourcePackages and failing fast on a mismatch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/embedded-mpv/build-macos-runtime.mjs
Line: 226-258
Comment:
**No pinned checksum verification for downloaded sources**
`sha256File` is called after each download and the result is stored in `sourcePackage.sha256`, but it is only recorded into the build manifest — it is never compared against a known-good expected hash. If any of the upstream URLs were compromised (MITM, CDN tampering) the build would silently proceed with the corrupt archive. Since these are HTTPS downloads from reputable sources the practical risk is low, but for a reproducible build tool that vendors security-sensitive native code it is worth pinning expected SHA-256 values next to each entry in `sourcePackages` and failing fast on a mismatch.
How can I resolve this? If you propose a fix, please make it concise.| const appPath = params.appOutDir.endsWith('.app') | ||
| ? params.appOutDir | ||
| : fs | ||
| .readdirSync(params.appOutDir) | ||
| .find((entry) => entry.endsWith('.app')); | ||
| const resourceDir = appPath | ||
| ? path.join( | ||
| params.appOutDir.endsWith('.app') | ||
| ? params.appOutDir | ||
| : path.join(params.appOutDir, appPath), | ||
| 'Contents', | ||
| 'Resources' | ||
| ) | ||
| : params.appOutDir; |
There was a problem hiding this comment.
resourceDir when no .app bundle is found
If params.appOutDir neither ends with .app nor contains any directory entry ending with .app, appPath resolves to undefined. The ternary then sets resourceDir = params.appOutDir, so validatePackagedEmbeddedMpv looks inside the raw output directory rather than inside Contents/Resources. For a non-required run this silently passes; for a required run the errors would reference paths under the wrong root. Consider throwing early when appPath is undefined to surface the misconfiguration immediately.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/packaging/electron-after-pack.cjs
Line: 35-48
Comment:
**Silent fallback to wrong `resourceDir` when no `.app` bundle is found**
If `params.appOutDir` neither ends with `.app` nor contains any directory entry ending with `.app`, `appPath` resolves to `undefined`. The ternary then sets `resourceDir = params.appOutDir`, so `validatePackagedEmbeddedMpv` looks inside the raw output directory rather than inside `Contents/Resources`. For a non-required run this silently passes; for a required run the errors would reference paths under the wrong root. Consider throwing early when `appPath` is `undefined` to surface the misconfiguration immediately.
How can I resolve this? If you propose a fix, please make it concise.| ); | ||
|
|
||
| private readonly unsubscribeSessionUpdate = | ||
| window.electron?.onEmbeddedMpvSessionUpdate?.((session) => { | ||
| if (session.id !== this.sessionId()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
document pointermove listener fires getBoundingClientRect on every cursor move
onDocumentPointerMove is a passive document-level listener that calls isPointerInsidePlayer (which invokes getBoundingClientRect) on every single pointer-move event in the entire page. While passive: true prevents scroll blocking, a forced layout reflow on every mouse movement can be visible in profiling. Consider throttling with requestAnimationFrame or using an IntersectionObserver/mouseenter approach so the bounds check is only paid when the pointer is already known to be near the component.
Prompt To Fix With AI
This is a comment left during a code review.
Path: libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-player.component.ts
Line: 139-145
Comment:
**Global `document` `pointermove` listener fires `getBoundingClientRect` on every cursor move**
`onDocumentPointerMove` is a passive document-level listener that calls `isPointerInsidePlayer` (which invokes `getBoundingClientRect`) on every single pointer-move event in the entire page. While `passive: true` prevents scroll blocking, a forced layout reflow on every mouse movement can be visible in profiling. Consider throttling with `requestAnimationFrame` or using an `IntersectionObserver`/`mouseenter` approach so the bounds check is only paid when the pointer is already known to be near the component.
How can I resolve this? If you propose a fix, please make it concise.…rkflow Entire-Checkpoint: c6e522b4276c
Entire-Checkpoint: c6e522b4276c
…ment for libplacebo Entire-Checkpoint: c6e522b4276c
…ocess Entire-Checkpoint: c6e522b4276c
…mbedded MPV integration Entire-Checkpoint: c6e522b4276c
…aging process Entire-Checkpoint: c6e522b4276c
1 participant
libmpvruntime for IPTVnator's embedded MPV player.build-macos-runtime.mjsfor building an LGPL-compatible runtime from source.stage-macos-runtime.mjsfor staging the built runtime artifacts.electron-after-pack.cjsandembedded-mpv-macos.cjs.Entire-Checkpoint: c6e522b4276c