diff --git a/REFACTORING_PLAN.md b/REFACTORING_PLAN.md new file mode 100644 index 0000000..5888fba --- /dev/null +++ b/REFACTORING_PLAN.md @@ -0,0 +1,578 @@ +# MXL Player refactoring plan + +## Goal + +Build a player with two persistent logical feed slots: video and audio. Each +slot may be configured, started, stopped, replaced, reconnected, or removed +while the program is running. + +When both slots are configured, the user may enable or disable synchronization +at any time. Synchronization is a runtime relationship between the slots, not a +startup mode. + +The design must leave a clean extension point for an `mxlfabrics` reader after +the local MXL player is stable. It must also support a later playlist layer +without moving playlist timing or selection into media readers. + +## Required behaviour + +### No configured feeds + +- Start the normal GUI and renderer. +- Show a placeholder, simple shader, or empty player surface. +- Allow the user to enter either or both UUIDs. + +### One configured feed + +- An audio UUID starts audio playback. +- A video UUID starts video playback. +- A failed feed reconnects according to its retry policy. +- The user may stop and later resume the feed without clearing its UUID. +- The user may replace or remove the UUID at runtime. + +### Two configured feeds, synchronization disabled + +- Audio and video run as independent workers. +- Failure of one feed must not stop, close, or restart the other. +- Each feed has its own state, last error, and retry counter. +- The user may stop or resume either feed independently. +- Synchronization can be enabled at runtime. + +### Two configured feeds, synchronization enabled + +- Both feeds are read through one synchronization group. +- Failure of either member fails the group attempt. +- Reconnection recreates the complete group. +- Synchronization can be disabled at runtime. The two feeds then continue as + independent workers. +- The user may stop both feeds together. Stopping only one member dismantles + the group and lets the other member continue independently. + +### Stop, resume, and remove semantics + +- `StopVideo` stops video reading and retrying but retains the video UUID. +- `StopAudio` stops audio reading and retrying but retains the audio UUID. +- `StopAll` stops both feeds and all retry activity but retains both UUIDs. +- A stopped feed may be resumed without re-entering its UUID. +- Removing a feed stops it and clears its UUID. +- Stopping one detached feed has no effect on the other feed. +- Stopping one member of a synchronized group disables synchronization, + stops the selected feed, and continues the other feed independently. +- Stopping both members of a synchronized group closes the group atomically. +- Stop commands cancel active reads and retry backoff promptly. + +### Future playlist behaviour + +A playlist is an ordered list of playback entries. Each entry describes a +complete desired session state and may contain: + +- synchronized video and audio UUIDs; +- independent video and audio UUIDs; +- video only; +- audio only; +- an optional per-entry playback duration. + +Example: + +```text +1. synchronized video A + audio A, 10 seconds +2. synchronized video B + audio B, 15 seconds +3. video C, 10 seconds +4. video D, 20 seconds +5. audio E, 30 seconds +``` + +The user may select any entry manually. Automatic playback advances after the +current entry's duration and loops from the final entry back to the first. + +Playlist orchestration belongs above `SessionController`. Selecting an entry +must translate its desired state into the same controller operations used by +CLI and GUI controls. Source readers, workers, retry supervisors, renderer, and +audio output must not know that a playlist exists. + +## Decisions to confirm before implementation + +- `MaxAttempts == 0` means retry indefinitely. +- The initial connection counts as attempt 1. +- Retry counters reset only after useful media has been received for a defined + stability period, not merely after a reader opens. +- Enabling or disabling synchronization starts a new lifecycle and resets the + applicable retry counters. +- Explicitly removing either feed disables requested synchronization. +- Explicitly stopping one member of a synchronized group disables requested + synchronization. Resuming that feed does not silently recreate the group; + the user can enable synchronization again. +- Requesting synchronization with only one configured feed records the request, + continues independent playback, and waits for the second feed. +- Exhausting attempts leaves the UUID configured and the unit in `Failed` until + the user retries, replaces the UUID, changes synchronization, or removes it. + +These are proposed defaults. Change them here before implementing the affected +stage if different behaviour is desired. + +## Target architecture + +```text +CLI initial values GUI runtime commands + | | + +----------------+-----------------+ + v + SessionController + desired vs actual state + | + +-----------+-----------+ + | | + sync disabled sync enabled + VideoWorker SyncGroupWorker + AudioWorker (video + audio) + | | + +-----------+-----------+ + v + stable output layer + video sink / audio sink +``` + +### Ownership rules + +- `SessionController` is the only owner allowed to change playback topology. +- A worker exclusively owns its reader and closes it when the worker stops. +- Independent audio and video workers never close each other's resources. +- A synchronized worker owns and closes the entire group. +- GUI and renderer consume status or media; they never inspect live readers. +- Every topology change increments a generation number. Late events from an old + generation are ignored. + +### Package direction + +```text +internal/source + Local MXL reader adapters and media metadata. + No retry, GUI, SDL, Vulkan, or application policy. + +internal/playback + Feed configuration, commands, state, retry supervisor, independent workers, + synchronized worker, and session controller. + +internal/output (optional once extraction is useful) + Stable video and audio sink interfaces and their adapters. + +cmd/mxl-player + Process initialization, CLI translation, GUI event translation, and wiring. +``` + +Do not introduce a single broad media-source interface. Video, audio, and a +synchronized pair have different results and should use small explicit +interfaces. + +## Runtime state + +Each independently supervised playback unit uses: + +```text +Idle -> Connecting -> Playing -> Reconnecting -> Failed + ^ | | | + +---------+------------+------------+-> Stopping -> Idle +``` + +`Idle` may still have a configured UUID. Configuration and running state are +separate: a configured slot can be stopped without being removed. + +Synchronization additionally distinguishes desired and actual state: + +```text +Disabled +TransitioningOn +Enabled +TransitioningOff +WaitingForSecondFeed +``` + +The GUI should receive immutable status snapshots containing at least: + +- configured UUID; +- desired and actual synchronization state; +- playback state per active unit; +- current attempt and configured limit; +- last error; +- last successful media time; +- received and dropped media counters. + +## Retry policy + +Retry belongs above `internal/source`. + +One attempt is: + +```text +open reader(s) -> receive media -> terminal read error -> close reader(s) +``` + +The supervisor then either stops or waits using cancellable capped exponential +backoff with small jitter before opening again. + +Requirements: + +- Apply the same policy to initial connection and later disconnection. +- Allow finite and infinite attempts. +- Cancel immediately on shutdown, UUID replacement, or topology change. +- Cancel immediately when the user stops the supervised feed. +- Never run two retry loops for the same playback unit. +- Retain and publish the final error after attempt exhaustion. +- Classify configuration/format errors separately from transient availability + errors. Whether permanent errors should retry is decided explicitly. + +## Runtime synchronization transitions + +### Enable synchronization + +1. Record synchronization as desired and increment the topology generation. +2. Cancel both independent workers. +3. Wait until both workers close their readers. +4. Clear stale queued audio. +5. Create the synchronized worker with the existing UUIDs. +6. Establish a new shared timeline and publish `Enabled` after valid paired data. +7. If opening or reading fails, retry the group as one unit. + +The renderer may retain the last video frame or show a placeholder during the +transition. + +### Disable synchronization + +1. Record synchronization as disabled and increment the topology generation. +2. Cancel and close the synchronized worker and group. +3. Clear audio belonging to the old synchronized timeline. +4. Start independent workers for both configured slots. +5. Let either worker begin playing without waiting for the other. + +## Future playlist model + +The exact public types can be chosen later, but the intended model is: + +```go +type PlaylistEntry struct { + Name string + VideoUUID string + AudioUUID string + SyncRequested bool + Duration time.Duration +} + +type Playlist struct { + Entries []PlaylistEntry + Loop bool +} +``` + +The playlist controller owns: + +- current entry index; +- manual selection; +- automatic advance timer; +- loop behaviour; +- pause/resume of automatic advance; +- applying one entry atomically to the session controller. + +Before implementing playlists, decide when an entry's duration begins. The +recommended rule is when its requested playback topology first reaches +`Playing`, so connection and retry time do not consume the viewing period. +Also decide whether an entry that exhausts retries waits for user action or +automatically advances; this should be configurable rather than implicit. + +## Implementation stages + +Only begin a stage after the previous stage's acceptance criteria pass. Keep +each stage small enough for one focused review and commit. + +### Stage 0 — Behaviour contract and baseline + +Work: + +- Confirm or edit the decisions in this document. +- Record the current CLI examples and observable behaviour. +- Run the existing build/tests and record known failures. +- Identify commands used for a local audio feed, video feed, and paired feeds. + +Acceptance criteria: + +- Behaviour choices are unambiguous. +- A repeatable baseline command is documented. +- No application code changes. + +### Stage 1 — Pure configuration model + +Work: + +- Introduce configuration types for domain, two optional feed slots, desired + synchronization, and retry policy. +- Make validation pure: return errors instead of printing or exiting. +- Permit no-feed startup. +- Require a domain only when at least one connection is requested. +- Translate CLI arguments into the same configuration later used by GUI actions. + +Acceptance criteria: + +- Unit tests cover zero, one, and two feeds plus invalid domains/policies. +- Existing playable CLI combinations still translate correctly. +- No playback lifecycle is moved yet. + +### Stage 2 — Normalize low-level source APIs + +Work: + +- Use consistent `VideoSource`, `AudioSource`, and `SyncSource` naming. +- Make every blocking read context-aware and bounded. +- Remove duplicate read paths where safe. +- Centralize audio-fragment extraction and video-frame construction. +- Define classifiable source errors. +- Make MXL payload ownership and copying rules explicit. +- Make a synchronized read fail when either member cannot provide its part. + +Acceptance criteria: + +- Source tests cover cancellation and important MXL error classification. +- A read cannot hide forever inside an internal retry loop. +- Existing audio-only, video-only, and synchronized smoke tests still work. + +### Stage 3 — Retry supervisor + +Work: + +- Implement a reusable supervisor around open, run, close, and retry. +- Add finite/infinite limits and cancellable capped backoff. +- Publish state transitions and attempt counts. +- Add deterministic timing hooks so tests do not sleep in real time. + +Acceptance criteria: + +- Tests cover initial failure, runtime failure, exhaustion, successful recovery, + counter reset, and cancellation during backoff. +- The supervisor has no SDL, Vulkan, ImGui, or concrete MXL dependency. + +### Stage 4 — Independent video worker + +Work: + +- Move video reader ownership and retry lifecycle into a video worker. +- Deliver owned frames through a bounded latest-frame channel or video sink. +- Stop writing directly into renderer staging memory from the source worker. + +Acceptance criteria: + +- Video reconnects without application restart. +- Replacing or removing its UUID cancels the old reader promptly. +- Slow rendering does not create an unbounded live-stream backlog. + +### Stage 5 — Independent audio worker + +Work: + +- Move audio reader ownership and retry lifecycle into an audio worker. +- Centralize channel interleaving. +- Use bounded ordered buffering and explicit backpressure. +- Define when the SDL queue is cleared during reconnect/replacement. + +Acceptance criteria: + +- Audio reconnects without application restart. +- Audio is correct for mono, stereo, and more than two channels. +- Cancellation cannot leave stale samples playing indefinitely. + +### Stage 6 — Session controller and independent dual playback + +Work: + +- Add typed controller commands rather than overloaded channel values. +- Own video and audio slot configuration in the controller. +- Track configured and running/desired-active state separately for each slot. +- Add topology generations and ignore stale worker events. +- Run both independent workers when both UUIDs are configured. + +Acceptance criteria: + +- Video failure does not interrupt audio. +- Audio failure does not interrupt video. +- Either UUID can be replaced while the other feed stays alive. +- Either feed can be stopped and resumed while the other stays alive. +- Stopping both feeds leaves the application and configured UUIDs intact. +- Race detector finds no shared-state races in controller/worker tests. + +### Stage 7 — Synchronized worker + +Work: + +- Move sync-group ownership into one supervised worker. +- Treat missing video or audio data as failure of the group attempt. +- Recreate all synchronized resources on reconnect. +- Publish paired media from one established timeline. + +Acceptance criteria: + +- Failure of either member restarts the complete group. +- Retry exhaustion stops the group cleanly and retains both configured UUIDs. +- Cancellation releases the group and both readers promptly. + +### Stage 8 — Runtime sync toggle + +Work: + +- Implement atomic enable and disable transitions described above. +- Preserve configured UUIDs across both transitions. +- Handle toggle commands during connection, playback, and retry backoff. +- Handle per-feed and stop-all commands in detached and synchronized playback. +- Define and implement behaviour when only one slot is configured. + +Acceptance criteria: + +- Detached feeds can be joined without restarting the application. +- A sync group can be detached into independent feeds without restarting. +- Rapid repeated toggles cannot leave duplicate workers or readers. +- Stopping one synchronized member detaches the group and preserves playback of + the other member. +- Stopping both synchronized members closes the group without starting new + independent workers. +- Old-generation errors cannot change the new topology. + +### Stage 9 — Stable dynamic outputs and idle startup + +Work: + +- Initialize GUI and rendering without requiring a successful source open. +- Show a placeholder when no video frame is available. +- Reconfigure video resources when dimensions/stride/format change. +- Reconfigure the audio stream when rate/channel/device properties change. + +Acceptance criteria: + +- `mxl-player` with no feed opens a usable GUI. +- The user can move among idle, audio-only, video-only, detached dual-feed, and + synchronized playback without restarting. +- Output resources do not depend on the initial CLI topology. + +### Stage 10 — GUI and CLI integration + +Work: + +- Convert GUI actions into controller commands. +- Add fields/actions for both UUIDs, reconnect, remove, retry, and sync toggle. +- Add independent start/stop controls and a stop-all action. +- Display desired/actual sync state and independent/group retry status. +- Add CLI options for retry limits and initial synchronization preference. + +Acceptance criteria: + +- CLI values only establish initial desired state. +- Every important runtime operation is available in the GUI. +- GUI never reads or closes source pointers directly. + +### Stage 11 — Resilience verification + +Test at least: + +- missing producer on initial connection; +- producer disappearance during playback; +- independent audio-only and video-only failure; +- one failed detached feed while the other remains alive; +- failure of either synchronized member; +- finite exhaustion and infinite retry cancellation; +- UUID replacement during reads and backoff; +- stopping or resuming either feed during reads and retry backoff; +- stopping both feeds while detached and synchronized; +- synchronization toggles during reads and backoff; +- shutdown during blocked reads; +- resolution, rate, and channel-count changes; +- mono, stereo, and multichannel audio; +- long runtime and repeated connect/disconnect cycles; +- `go test -race` for testable non-GPU packages. + +Acceptance criteria: + +- The specified failure boundaries hold in every test. +- No known goroutine, MXL reader, sync group, SDL stream, or Vulkan resource leak. +- Player state remains understandable after every exhausted retry sequence. + +### Stage 12 — Reader backend extension point + +Work: + +- Introduce small reader factories only after local MXL behaviour is solid. +- Keep playback workers dependent on reader interfaces/factories, not `go-mxl`. +- Implement local MXL as the first backend. +- Add `mxlfabrics` as a second backend without changing controller semantics. + +Acceptance criteria: + +- Backend selection does not alter retry or synchronization semantics. +- Fake readers can drive all controller and supervisor tests. +- Local MXL remains the reference implementation. + +### Stage 13 — Simple playlist + +Work: + +- Add ordered playlist entries containing optional video/audio UUIDs, + synchronization preference, and per-entry duration. +- Apply entry changes atomically through `SessionController`. +- Add manual previous, next, and direct-entry selection. +- Add timed automatic advance and optional looping. +- Add pause/resume for automatic playlist advancement. +- Expose the current entry and remaining time in the GUI. +- Define behaviour for stopped playback, retry exhaustion, invalid entries, and + manual selection while an automatic timer is active. + +Acceptance criteria: + +- A playlist may freely mix synchronized pairs, independent pairs, video-only, + and audio-only entries. +- Manual selection works regardless of the current playback topology. +- Timed entries advance in order and loop without leaking old workers/readers. +- Entry duration follows the documented start rule and uses cancellable timers. +- An old entry's timers and worker events cannot affect a newly selected entry. +- Playlist logic contains no direct MXL, SDL, Vulkan, or renderer operations. + +## Suggested command API + +Names may change, but commands must have one explicit meaning: + +```go +SetVideo(uuid string) +SetAudio(uuid string) +StartVideo() +StartAudio() +StartAll() +StopVideo() +StopAudio() +StopAll() +RemoveVideo() +RemoveAudio() +EnableSync() +DisableSync() +ReconnectVideo() +ReconnectAudio() +RetryNow() +Stop() +``` + +In synchronized operation, reconnecting either slot means reconnecting the +group. The UI should make that consequence visible. + +## Out of scope for the initial player refactoring + +- Playlist implementation before the controller, dynamic topology, and output + lifecycle are stable. The planned playlist work is Stage 13. +- Automatic multi-UUID failure-based failover selection. +- Snapshot, waveform, vectorscope, and advanced diagnostics. +- A large generic media framework. +- `mxlfabrics` implementation before the local player passes resilience tests. +- Unrelated renderer or GUI redesign. + +## Working method + +For every stage: + +1. Agree on the narrow change and expected behaviour. +2. Make the change without pulling later-stage responsibilities forward. +3. Format and run focused tests. +4. Run the relevant manual smoke test. +5. Review ownership, cancellation, and error handling. +6. Record discoveries or revised decisions in this document. +7. Commit the stage separately when accepted.