ARCHITECTURE_REVIEW.md

@@ -0,0 +1,133 @@
+# MXL Pattern Generator Architecture Review
+
+Reviewed on 2026-09-16 against commit `f37f480`.
+
+## Summary
+
+The current stack is a good fit for a test-pattern generator:
+
+- Go is well suited to CLI handling, MXL lifecycle management, timing, and synthetic audio generation.
+- `go-mxl` provides the correct data model: discrete grains for video and continuous samples for audio.
+- wgpu/WGSL is a reasonable portable GPU abstraction for producing v210 video.
+- CPU-rendered text is appropriate because text changes infrequently and font rendering does not need to be implemented in a shader.
+
+The project is still prototype-quality in its orchestration and configuration layers. Before adding substantial audio functionality, flow-definition handling, lifecycle management, and the failing tests should be corrected.
+
+## Recommended audio architecture
+
+Video and audio should run in separate goroutines. Each goroutine should own its own MXL writer and timing loop because video and audio have different units and rates:
+
+```text
+run
+  |-- shared context, signal handling, and error propagation
+  |-- video goroutine
+  |     OpenGrain -> render -> overlay -> Commit
+  `-- audio goroutine
+        OpenSamples -> synthesize -> Commit
+```
+
+This is also consistent with `go-mxl`: a writer is not safe for concurrent use, so a writer should belong to exactly one goroutine.
+
+The goroutines should not synchronize by sending a notification for every video frame. That would make audio timing depend on GPU latency and Go scheduler jitter. Instead, both loops should use the MXL clock independently:
+
+```go
+videoIndex := mxl.CurrentIndex(videoRate)
+audioIndex := mxl.CurrentIndex(audioRate)
+```
+
+After committing data, each loop advances its own index and uses `mxl.NsUntilIndex` for pacing. Both loops should share a `context.Context`; an error in either flow should cancel the other flow and be returned to the caller.
+
+For audio, start with CPU generation and approximately 10 ms batches (480 samples at 48 kHz). A sine wave is inexpensive to calculate, so GPU audio generation would add complexity without a useful performance benefit.
+
+Audio phase should be derived from the absolute sample index:
+
+```go
+phase := 2 * math.Pi * frequency *
+    float64(firstSample+i) / sampleRate
+```
+
+This preserves continuity across batches and after a timing resynchronization.
+
+Video and audio should expose different interfaces because a video frame and a range of audio samples are fundamentally different units:
+
+```go
+type VideoGenerator interface {
+    GenerateFrame(dst []byte, frameIndex uint64) error
+    Close() error
+}
+
+type AudioGenerator interface {
+    GenerateSamples(channels [][]byte, firstSample, sampleCount uint64) error
+    Close() error
+}
+```
+
+For `audio/float32`, the audio package may use `[]float32` internally and keep byte encoding at the MXL boundary.
+
+## Issue status
+
+| # | Issue | Status | Current observation |
+|---|---|---|---|
+| 1 | The validated `--domain` value was ignored in favor of `/dev/shm/mxl`. | **Fixed** | Logging, `mxl.NewInstance`, and reuse messages now use `args.domain`. |
+| 2 | An external video flow definition does not populate runtime width, height, FPS, or UUID. | **Open** | The JSON is read as an opaque string while `vi` remains zero-valued. `NewWGPUGenerator` therefore receives `0, 0`. Parse and validate the definition before generator creation, or introduce a typed configuration source. |
+| 3 | `checkArgs` received `appArgs` by value, so generated UUIDs were discarded. | **Partially fixed** | `checkArgs` now receives `*appArgs`. However, the generated video UUID is still overwritten by a hard-coded UUID in `main`; remove that assignment before release. |
+| 4 | The default pattern was `bars`, which did not exist. | **Fixed** | The default is now `ebu75`, which exists in the pattern registry. |
+| 5 | `NewFlowDefJSON(TYPE_AUDIO, ...)` produces a video/v210 definition. | **Open** | The function accepts `TYPE_AUDIO`, but all emitted format and media fields remain video-specific. Create separate typed video and audio definition builders. |
+| 6 | The wgpu path was described as zero-copy although it performs GPU readback and a CPU copy. | **Open** | Every frame is copied from GPU storage to a mapped host buffer and then copied into the MXL payload. The path is synchronous and serial. Update the documentation and benchmark it before redesigning it. |
+| 7 | The test suite had three failures. | **Open** | `TestNewTextOverlay`, `TestWGPUMoveSquare`, and `TestWGPUGenerator` still fail. |
+| 8 | The Makefile clean target uses `fm -f` instead of `rm -f`. | **Open** | The typo remains in the `clean` target. |
+
+## Additional implementation priorities
+
+1. Fix the existing tests or update incorrect expectations after confirming the intended color values and overlay positioning.
+2. Split video and audio flow-definition types. Avoid an integer `feedType` accepted by a function that has a video-only parameter list.
+3. Parse external flow definitions into typed configuration before creating generators. Validate format, media type, dimensions, rate, and channel count.
+4. Remove the hard-coded video UUID.
+5. Introduce a `run(...) error` orchestration function using shared cancellation and error propagation instead of calling `log.Fatalf` throughout the media loop.
+6. Add a CPU audio generator and continuous-flow writer loop using `OpenSamples`, `ChannelFragments`, and `Commit`.
+7. Measure end-to-end frame time and missed deadlines at 1080p50/60 and UHD. The current wgpu path may be adequate, but it is neither zero-copy nor asynchronous.
+
+## Suggested package layout
+
+```text
+cmd/mxl-pattern/
+    main.go
+
+internal/config/
+    config.go
+    flow.go
+
+internal/video/
+    generator.go
+    wgpu.go
+    overlay.go
+    runner.go
+
+internal/audio/
+    generator.go
+    sine.go
+    runner.go
+
+internal/app/
+    run.go
+```
+
+The exact directory names are less important than keeping CLI parsing, typed configuration, media generation, and MXL writing as separate responsibilities.
+
+## Verification
+
+The following command was used:
+
+```sh
+GOCACHE=/tmp/go-mxl-gen-cache go test ./...
+```
+
+Package compilation succeeds, but the generator package fails these tests:
+
+- `TestNewTextOverlay`: expected a centered text box, but its center was reported as 45 instead of approximately 960.
+- `TestWGPUMoveSquare`: tick 79 produced `590/512/512` instead of expected `893/176/543`.
+- `TestWGPUGenerator`: pixel `(0,0)` produced Y=414 instead of expected Y=721.
+
+## Conclusion
+
+Keep the chosen Go + go-mxl + wgpu stack. Implement audio on the CPU in its own goroutine and give video and audio separate writers, indices, and pacing loops. Coordinate them through a shared context and the common MXL timebase, not through per-frame messages. The immediate blockers are external-flow configuration, audio flow-definition support, and the currently failing tests.

Makefile

@@ -17,4 +17,4 @@ test:
 
 clean:
 	go clean
-	fm -f ./build/mxl-gen
+	rm -f ./build/mxl-gen