From 326a8d68907384c2b45af3e57a306f0cdd5905dd Mon Sep 17 00:00:00 2001 From: Dmitry Sergeev Date: Wed, 16 Sep 2026 11:24:25 +0300 Subject: [PATCH] makefile fix --- ARCHITECTURE_REVIEW.md | 133 +++++++++++++++++++++++++++++++++++++++++ Makefile | 2 +- 2 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 ARCHITECTURE_REVIEW.md diff --git a/ARCHITECTURE_REVIEW.md b/ARCHITECTURE_REVIEW.md new file mode 100644 index 0000000..f2748ae --- /dev/null +++ b/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. diff --git a/Makefile b/Makefile index 334a71b..d15a47b 100644 --- a/Makefile +++ b/Makefile @@ -17,4 +17,4 @@ test: clean: go clean - fm -f ./build/mxl-gen + rm -f ./build/mxl-gen