refactoring plan
This commit is contained in:
@@ -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.
|
||||
Reference in New Issue
Block a user