Proposal: unified player-controls architecture (docs only, for direction)#1105
Proposal: unified player-controls architecture (docs only, for direction)#1105larsemig wants to merge 2 commits into
Conversation
Direction proposal (docs only) for a single engine-agnostic player-controls layer shared by every engine, plus a true-overlay embedded-MPV player with real macOS native fullscreen. Implementation lives on a separate branch and is shown in an attached video; this PR is for the maintainer to decide whether the project should move this way before any upstream implementation PR. - docs/proposals/player-controls-architecture.md — the proposal (journey, required vs optional steps, advantages, downsides, open questions) - docs/architecture/player-controls-contract.md — the contract / adapters / flags - docs/architecture/player-controls-refactor.md — immersive overlay design + decision log - docs/architecture/embedded-mpv-native.md — native/IPC + immersive + native fullscreen Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the prose proposal + fork reference docs with three MADR-format Architecture Decision Records (status: proposed) plus an index: - 0001 unified, engine-agnostic player-controls contract - 0002 embedded-MPV immersive overlay compositing - 0003 real macOS native fullscreen for embedded MPV Delete the fork-only reference docs (player-controls-contract.md, player-controls-refactor.md) and the prose proposal, folding their content into the ADRs. embedded-mpv-native.md is left untouched relative to the fork point so the proposal PR does not modify that doc. No fork-branch names are referenced. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Greptile SummaryThis is a docs-only proposal PR that introduces three MADR-format Architecture Decision Records (ADRs) and an index README in a new
Confidence Score: 4/5Safe to accept as a directional proposal; no code changes are included and all three ADRs are clearly marked proposed, not on master. The ADRs are well-reasoned and the option analyses are thorough, but ADR-0002 leaves the transparent-window transparency-bug risk without a documented mitigation, omits the feature-flag name that gates the macOS path, and does not reference the existing docs/architecture/embedded-mpv-native.md that will need significant rewrites on acceptance. These gaps are purely in the documentation itself and do not block accepting the proposal direction, but they leave ambiguity that the implementation PRs would inherit. docs/adr/0002-embedded-mpv-immersive-compositing.md warrants a second look for the transparency-risk mitigation and missing flag reference before implementation begins. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph SharedLayer["Shared Controls Layer (ADR-0001)"]
PC["PlayerController\n(capabilities · state · commands)"]
APC["app-player-controls\n(Angular component)"]
PC -->|binds to| APC
end
subgraph Adapters["Per-Engine Adapters"]
EA["EmbeddedMpvControlsAdapter\n(libmpv IPC)"]
WA["WebVideoControlsAdapter\n(video engines)"]
end
EA -->|implements| PC
WA -->|implements| PC
subgraph MacOS["macOS Compositing (ADR-0002)"]
NW["transparent: true\nElectron Window"]
MPV["libmpv native surface\nNSWindowBelow"]
BD["EmbeddedMpvImmersiveBackdrop\nbox-shadow hole"]
NW --> MPV
NW --> BD
BD -->|hole reveals| MPV
APC -->|floats over hole as DOM| BD
end
subgraph FullscreenDelegate["Fullscreen (ADR-0003)"]
PF["PlayerFullscreenController\n(optional delegate)"]
DF["ControlsFullscreen\n(DOM requestFullscreen)"]
WS["win.setFullScreen\nmacOS native Space"]
PF -->|MPV path| WS
APC -->|default| DF
APC -->|MPV override| PF
end
EA --> MacOS
EA --> FullscreenDelegate
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
subgraph SharedLayer["Shared Controls Layer (ADR-0001)"]
PC["PlayerController\n(capabilities · state · commands)"]
APC["app-player-controls\n(Angular component)"]
PC -->|binds to| APC
end
subgraph Adapters["Per-Engine Adapters"]
EA["EmbeddedMpvControlsAdapter\n(libmpv IPC)"]
WA["WebVideoControlsAdapter\n(video engines)"]
end
EA -->|implements| PC
WA -->|implements| PC
subgraph MacOS["macOS Compositing (ADR-0002)"]
NW["transparent: true\nElectron Window"]
MPV["libmpv native surface\nNSWindowBelow"]
BD["EmbeddedMpvImmersiveBackdrop\nbox-shadow hole"]
NW --> MPV
NW --> BD
BD -->|hole reveals| MPV
APC -->|floats over hole as DOM| BD
end
subgraph FullscreenDelegate["Fullscreen (ADR-0003)"]
PF["PlayerFullscreenController\n(optional delegate)"]
DF["ControlsFullscreen\n(DOM requestFullscreen)"]
WS["win.setFullScreen\nmacOS native Space"]
PF -->|MPV path| WS
APC -->|default| DF
APC -->|MPV override| PF
end
EA --> MacOS
EA --> FullscreenDelegate
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
docs/adr/0002-embedded-mpv-immersive-compositing.md:128-131
**Existing architecture doc will become stale if this ADR is accepted**
`docs/architecture/embedded-mpv-native.md` explicitly documents the current "control dock" approach and directly states: *"The embedded MPV controls use a compositor-safe control dock below the native viewport instead of a true overlay on top of the native video surface."* and *"Do not place critical Angular overlays on top of the video viewport and expect CSS `z-index` to win."* — both of which are the exact constraints that this ADR supersedes. If the proposal is accepted, that document will need significant rewrites across its compositing, bounds, and renderer-architecture sections. Adding a note here (or in the "More Information" section) calling out `docs/architecture/embedded-mpv-native.md` as the doc to update would make the implementation PR's doc-change scope explicit for the reviewer.
### Issue 2 of 4
docs/adr/0002-embedded-mpv-immersive-compositing.md:73-78
**Feature flag name not recorded in the ADR**
The PR description mentions that the immersive overlay and native fullscreen (ADR-0003) are gated by `IPTVNATOR_ENABLE_EMBEDDED_MPV_EXPERIMENT`, but neither ADR-0002 nor ADR-0003 mentions this gate. ADR-0001 does name its rollout flag (`WEB_PLAYER_SHARED_CONTROLS_ENABLED`). For completeness, the "Decision Outcome" or "Consequences" section of ADR-0002 (and ADR-0003) should record the environment variable that gates the macOS-specific path, so a future reader of the ADR alone understands the activation mechanism without having to cross-reference the PR description.
### Issue 3 of 4
docs/adr/0002-embedded-mpv-immersive-compositing.md:92-97
**Transparency-bug risk noted without mitigation**
The "Bad" consequence acknowledges that a bug could make the app or parts of it transparent, exposing the desktop below. The corresponding entry in the PR description adds "Not sure if we can always catch this with tests." For an ADR whose purpose is to capture the full decision context, it would help to document at least the intended mitigation: e.g. error-boundary CSS fallback, a defensive catch in `EmbeddedMpvImmersiveService` that resets to the opaque state on exception, or an explicit statement that this is accepted as an unmitigable risk of the transparent-window approach. Without that, the downstream implementation PR has no authoritative guidance on what the agreed safety net looks like.
### Issue 4 of 4
docs/adr/README.md:1-23
**New `docs/adr/` directory diverges from the CLAUDE.md documentation policy**
`CLAUDE.md` specifies that architectural docs go in `docs/architecture/`, and the rule is to prefer updating or adding to existing canonical locations before creating a new one. Introducing `docs/adr/` is a deliberate MADR convention choice, and MADR is a well-established format that's meaningfully distinct from the narrative arch docs already in `docs/architecture/`. But since the policy is explicit, it is worth confirming with the maintainer that this new directory is the intended home for future decision records (and optionally backfilling old architecture decisions as ADRs) rather than being an artefact of the proposal-branch layout that should eventually fold into `docs/architecture/`.
Reviews (1): Last reviewed commit: "docs(adr): convert player-controls propo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| - Implemented (macOS) on a separate branch; | ||
| demonstrated in the proposal video. | ||
| - Related: [ADR-0001 — Unified player-controls contract](./0001-unified-player-controls-contract.md), | ||
| [ADR-0003 — Real macOS native fullscreen for embedded MPV](./0003-macos-native-fullscreen-embedded-mpv.md) |
There was a problem hiding this comment.
docs/architecture/embedded-mpv-native.md explicitly documents the current "control dock" approach and directly states: "The embedded MPV controls use a compositor-safe control dock below the native viewport instead of a true overlay on top of the native video surface." and "Do not place critical Angular overlays on top of the video viewport and expect CSS z-index to win." — both of which are the exact constraints that this ADR supersedes. If the proposal is accepted, that document will need significant rewrites across its compositing, bounds, and renderer-architecture sections. Adding a note here (or in the "More Information" section) calling out docs/architecture/embedded-mpv-native.md as the doc to update would make the implementation PR's doc-change scope explicit for the reviewer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/adr/0002-embedded-mpv-immersive-compositing.md
Line: 128-131
Comment:
**Existing architecture doc will become stale if this ADR is accepted**
`docs/architecture/embedded-mpv-native.md` explicitly documents the current "control dock" approach and directly states: *"The embedded MPV controls use a compositor-safe control dock below the native viewport instead of a true overlay on top of the native video surface."* and *"Do not place critical Angular overlays on top of the video viewport and expect CSS `z-index` to win."* — both of which are the exact constraints that this ADR supersedes. If the proposal is accepted, that document will need significant rewrites across its compositing, bounds, and renderer-architecture sections. Adding a note here (or in the "More Information" section) calling out `docs/architecture/embedded-mpv-native.md` as the doc to update would make the implementation PR's doc-change scope explicit for the reviewer.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| it — keep the app opaque by default and subtract one rectangle — so new routes | ||
| need no CSS. | ||
|
|
||
| The native/IPC and cross-platform internals, and the full decision log, live with | ||
| the implementation on a separate branch. | ||
|
|
There was a problem hiding this comment.
The PR description mentions that the immersive overlay and native fullscreen (ADR-0003) are gated by IPTVNATOR_ENABLE_EMBEDDED_MPV_EXPERIMENT, but neither ADR-0002 nor ADR-0003 mentions this gate. ADR-0001 does name its rollout flag (WEB_PLAYER_SHARED_CONTROLS_ENABLED). For completeness, the "Decision Outcome" or "Consequences" section of ADR-0002 (and ADR-0003) should record the environment variable that gates the macOS-specific path, so a future reader of the ADR alone understands the activation mechanism without having to cross-reference the PR description.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/adr/0002-embedded-mpv-immersive-compositing.md
Line: 73-78
Comment:
**Feature flag name not recorded in the ADR**
The PR description mentions that the immersive overlay and native fullscreen (ADR-0003) are gated by `IPTVNATOR_ENABLE_EMBEDDED_MPV_EXPERIMENT`, but neither ADR-0002 nor ADR-0003 mentions this gate. ADR-0001 does name its rollout flag (`WEB_PLAYER_SHARED_CONTROLS_ENABLED`). For completeness, the "Decision Outcome" or "Consequences" section of ADR-0002 (and ADR-0003) should record the environment variable that gates the macOS-specific path, so a future reader of the ADR alone understands the activation mechanism without having to cross-reference the PR description.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| subtle tonal layering (negligible in light theme, more visible in dark). | ||
| - Bad — the native-through-the-hole compositing requires on-device macOS | ||
| verification (not covered by unit tests/build). | ||
|
|
||
| ## Pros and Cons of the Options | ||
|
|
There was a problem hiding this comment.
The "Bad" consequence acknowledges that a bug could make the app or parts of it transparent, exposing the desktop below. The corresponding entry in the PR description adds "Not sure if we can always catch this with tests." For an ADR whose purpose is to capture the full decision context, it would help to document at least the intended mitigation: e.g. error-boundary CSS fallback, a defensive catch in EmbeddedMpvImmersiveService that resets to the opaque state on exception, or an explicit statement that this is accepted as an unmitigable risk of the transparent-window approach. Without that, the downstream implementation PR has no authoritative guidance on what the agreed safety net looks like.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/adr/0002-embedded-mpv-immersive-compositing.md
Line: 92-97
Comment:
**Transparency-bug risk noted without mitigation**
The "Bad" consequence acknowledges that a bug could make the app or parts of it transparent, exposing the desktop below. The corresponding entry in the PR description adds "Not sure if we can always catch this with tests." For an ADR whose purpose is to capture the full decision context, it would help to document at least the intended mitigation: e.g. error-boundary CSS fallback, a defensive catch in `EmbeddedMpvImmersiveService` that resets to the opaque state on exception, or an explicit statement that this is accepted as an unmitigable risk of the transparent-window approach. Without that, the downstream implementation PR has no authoritative guidance on what the agreed safety net looks like.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| # Architecture Decision Records | ||
|
|
||
| This directory holds [MADR](https://adr.github.io/madr/)-format Architecture | ||
| Decision Records. Each ADR captures one decision: its context, the options | ||
| considered, the outcome, and the consequences. | ||
|
|
||
| **Status values:** `proposed` → `accepted` → `rejected` / `deprecated` / | ||
| `superseded by NNNN`. The status is in each file's front matter and (for proposed | ||
| work) a banner at the top. | ||
|
|
||
| ## Index | ||
|
|
||
| | ADR | Title | Status | | ||
| |---|---|---| | ||
| | [0001](./0001-unified-player-controls-contract.md) | Unified, engine-agnostic player-controls contract | Proposed | | ||
| | [0002](./0002-embedded-mpv-immersive-compositing.md) | Embedded-MPV immersive overlay compositing | Proposed | | ||
| | [0003](./0003-macos-native-fullscreen-embedded-mpv.md) | Real macOS native fullscreen for embedded MPV | Proposed | | ||
|
|
||
| ADRs 0001–0003 describe a single proposed direction (a unified player-controls | ||
| architecture) implemented on a separate branch and | ||
| **not yet on `master`**. The implementation — full contract API, adapters, | ||
| native compositing, and tests — lives on that branch and is demonstrated in the | ||
| proposal video. |
There was a problem hiding this comment.
docs/adr/ directory diverges from the CLAUDE.md documentation policy
CLAUDE.md specifies that architectural docs go in docs/architecture/, and the rule is to prefer updating or adding to existing canonical locations before creating a new one. Introducing docs/adr/ is a deliberate MADR convention choice, and MADR is a well-established format that's meaningfully distinct from the narrative arch docs already in docs/architecture/. But since the policy is explicit, it is worth confirming with the maintainer that this new directory is the intended home for future decision records (and optionally backfilling old architecture decisions as ADRs) rather than being an artefact of the proposal-branch layout that should eventually fold into docs/architecture/.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/adr/README.md
Line: 1-23
Comment:
**New `docs/adr/` directory diverges from the CLAUDE.md documentation policy**
`CLAUDE.md` specifies that architectural docs go in `docs/architecture/`, and the rule is to prefer updating or adding to existing canonical locations before creating a new one. Introducing `docs/adr/` is a deliberate MADR convention choice, and MADR is a well-established format that's meaningfully distinct from the narrative arch docs already in `docs/architecture/`. But since the policy is explicit, it is worth confirming with the maintainer that this new directory is the intended home for future decision records (and optionally backfilling old architecture decisions as ADRs) rather than being an artefact of the proposal-branch layout that should eventually fold into `docs/architecture/`.
How can I resolve this? If you propose a fix, please make it concise.
|
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #1105 +/- ##
===========================================
- Coverage 71.05% 56.91% -14.15%
===========================================
Files 40 547 +507
Lines 691 32015 +31324
Branches 87 7150 +7063
===========================================
+ Hits 491 18220 +17729
- Misses 176 11035 +10859
- Partials 24 2760 +2736
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks a lot for this, and especially for going down the native-composition rabbit hole so thoroughly. Quick context: I just shipped 0.22, so we have a clean baseline to build on and no release pressure hanging over this work. On the direction: ADR-0001 (unified controls contract + adapters) - yes, let's do it. ADR-0002 / 0003 (immersive compositing + native fullscreen) - the video looks great, and I want to try it. The immersive overlay genuinely looks like it solves the airspace problem that the docked strip never could (video shrinking away when a menu opens was always the worst part). I want to see it running on-device before committing, since the transparent: true window is an app-wide setting and the fullscreen freeze/reconciliation logic is the delicate bit - but the concept and the demo are convincing. I understand and accept that this part is macOS-only; Windows/Linux staying on the docked path is fine. |
|
The diff is quite big...
Tell me what you want to do with the ADRs (discard or pull in) I cannot test linux or windows thats why I didn't change anything (already had insane scope creep) |
3 participants
Note
This is a direction proposal — docs only, no code.
I'd like your call on whether IPTVnator should move toward this architecture proposal before I open implementation PRs. The code already exists. (demonstration video below)
Where does this come from
For me (macOS user) the embedded MPV player just works best.
I wanted to fix a "simple" bug
I thought I can "just" overlay the controls, fix some CSS and I am done.
As you already know: there is a native window composition rabbit hole.
Rabbit hole ;tldr
3 day later and around 350$ worth of tokens I came up with this proposal:
Proposal
I eventually committed to the transparent window appraoch with an underlying native window.
The main advantage being that all wiring of the components (overly) can stay (as is) in the web app.
But why even bother? While I was down there anyway...
What's in this PR
Docs only:
docs/proposals/player-controls-architecture.md— the proposal: required vs. optional steps, advantages, downsides, and open questions for you.docs/architecture/player-controls-contract.md— contract, shared component, adapters, flags.docs/architecture/player-controls-refactor.md— immersive-overlay design + the full decision log.docs/architecture/embedded-mpv-native.md— native/IPC internals, immersive compositing, native fullscreen.Scope, platforms & safety
IPTVNATOR_ENABLE_EMBEDDED_MPV_EXPERIMENTenv.Trade-offs
titleBarOverlayfunctionality on macOS but since it wasn't used we only lost the possibility to use it in the future. (native traffic lights are still there).If you are willing to move in this architectural direction, I can provide these changes though a series of PRs.
This video demonstrates my current implementation (first player is embedded MPV) with the feature flag turned on (all players use the same controls (2x speed)
Bildschirmaufnahme.2026-06-30.um.22.57.50-1_2x.mov