rendering tests fixed

ARCHITECTURE_REVIEW.md

@@ -11,7 +11,7 @@ The current stack is a good fit for a test-pattern generator:
 - 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.
+The project is still prototype-quality in parts of its orchestration and configuration layers, but flow-definition handling, lifecycle management, audio generation, and the previously failing rendering tests have now been addressed.
 
 ## Recommended audio architecture
 
@@ -72,17 +72,15 @@ For `audio/float32`, the audio package may use `[]float32` internally and keep b
 | 2 | An external video flow definition does not populate runtime width, height, FPS, or UUID. | **Fixed** | External definitions are parsed and validated as `flowdef.Video`; `video.Config` supplies their dimensions, rate, and ID to the generator and writer. |
 | 3 | `checkArgs` received `appArgs` by value, so generated UUIDs were discarded. | **Fixed** | Argument validation mutates the actual configuration, and the hard-coded video UUID has been removed. |
 | 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. | **Fixed** | The generic discriminator-based builder was removed. Video and audio have separate schema types, and video uses the typed `NewV210Video` constructor. An audio constructor will be added with audio generation. |
+| 5 | `NewFlowDefJSON(TYPE_AUDIO, ...)` produces a video/v210 definition. | **Fixed** | The generic discriminator-based builder was removed. Video and audio have separate schema types and typed `NewV210Video` and `NewFloat32Audio` constructors. |
 | 6 | The wgpu path was described as zero-copy although it performs GPU readback and a CPU copy. | **Deferred — fix after audio** | 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. This may require substantial benchmarking and architectural work, so audio implementation takes priority. Update the documentation now, but defer optimization or redesign until audio is complete. |
-| 7 | The test suite had three failures. | **Open** | `TestNewTextOverlay`, `TestWGPUMoveSquare`, and `TestWGPUGenerator` still fail. |
+| 7 | The test suite had three failures. | **Fixed** | Text positioning tests now match the explicit-position API and reject invalid bounds/alignment. WGPU tests now verify the actual RP 219 geometry and dynamic overlay behavior. |
 | 8 | The Makefile clean target uses `fm -f` instead of `rm -f`. | **Fixed** | The clean target now uses `rm -f`. |
 
 ## Additional implementation priorities
 
-1. Fix the existing tests or update incorrect expectations after confirming the intended color values and overlay positioning.
-2. Add a typed audio flow-definition constructor, CPU audio generator, and continuous-flow writer loop using `OpenSamples`, `ChannelFragments`, and `Commit`.
-3. Run video and audio as sibling goroutines with shared cancellation and error propagation.
-4. After audio is complete, 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. Treat GPU readback optimization as a separate, potentially large task.
+1. Add integration coverage for simultaneous video/audio startup, cancellation, and error propagation against an MXL instance.
+2. 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. Treat GPU readback optimization as a separate, potentially large task.
 
 ## Suggested package layout
 
@@ -119,12 +117,8 @@ The following command was used:
 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.
+All packages pass. `go vet ./...`, the application build, and `git diff --check` also succeed.
 
 ## 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. Typed external-video configuration and the context-aware video runner are now implemented; the next architectural work is typed audio construction and generation. The three rendering-test failures remain a separate correctness task.
+Keep the chosen Go + go-mxl + wgpu stack. Audio belongs on the CPU in its own goroutine, with video and audio using separate writers, indices, and pacing loops. Coordinate them through a shared context and the common MXL timebase, not through per-frame messages. Typed configuration, media runners, and rendering correctness coverage are now in place; GPU readback optimization remains deferred until after audio work.

internal/generator/text.go

@@ -87,9 +87,8 @@ func NewTextOverlay(text string, frameW, frameH, posX, posY int, textPos string,
 		return nil, fmt.Errorf("text: box %dpx wider than frame %dpx", w, frameW)
 	}
 	h := 2*padY + textH
-	const topMargin = 48
-	if topMargin+h > frameH {
-		return nil, fmt.Errorf("text: box %dpx does not fit frame height %dpx", topMargin+h, frameH)
+	if h > frameH {
+		return nil, fmt.Errorf("text: box %dpx higher than frame %dpx", h, frameH)
 	}
 
 	if textPos != "" {
@@ -120,6 +119,18 @@ func NewTextOverlay(text string, frameW, frameH, posX, posY int, textPos string,
 			posY = frameH - h
 		}
 	}
+	if posX < 0 || posY < 0 {
+		return nil, fmt.Errorf("text: position (%d, %d) must not be negative", posX, posY)
+	}
+	if posX%6 != 0 {
+		return nil, fmt.Errorf("text: x position %d must be divisible by 6 for v210", posX)
+	}
+	if posX+w > frameW || posY+h > frameH {
+		return nil, fmt.Errorf(
+			"text: box at (%d, %d), size %dx%d, does not fit frame %dx%d",
+			posX, posY, w, h, frameW, frameH,
+		)
+	}
 	o := &TextOverlay{
 		frameW: frameW,
 		x:      posX,

internal/generator/text_test.go

@@ -1,8 +1,8 @@
 package generator
 
 import (
-	"encoding/binary"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"golang.org/x/image/font"
@@ -32,11 +32,8 @@ func TestNewTextOverlay(t *testing.T) {
 	if o.x%6 != 0 {
 		t.Fatalf("x=%d not a multiple of 6", o.x)
 	}
-	if center := o.x + o.w/2; center < frameW/2-3 || center > frameW/2+3 {
-		t.Fatalf("box center %d not near frame center %d", center, frameW/2)
-	}
-	if o.y != 48 {
-		t.Fatalf("y=%d, want 48", o.y)
+	if o.x != 0 || o.y != 0 {
+		t.Fatalf("position = (%d,%d), want (0,0)", o.x, o.y)
 	}
 	if o.blocks == nil || len(o.blocks) != o.h*(o.w/6)*16 {
 		t.Fatalf("pre-packed tile has wrong size")
@@ -87,6 +84,78 @@ func TestNewTextOverlay(t *testing.T) {
 	}
 }
 
+func TestNewTextOverlayPositioning(t *testing.T) {
+	const frameW, frameH = 1920, 1080
+	face := testFace(t, 48)
+
+	t.Run("explicit", func(t *testing.T) {
+		o, err := NewTextOverlay("AB", frameW, frameH, 120, 48, "", face)
+		if err != nil {
+			t.Fatalf("NewTextOverlay: %v", err)
+		}
+		if o.x != 120 || o.y != 48 {
+			t.Fatalf("position = (%d,%d), want (120,48)", o.x, o.y)
+		}
+	})
+
+	for _, tc := range []struct {
+		name string
+		pos  string
+		want func(*TextOverlay) bool
+	}{
+		{name: "top center", pos: "tc", want: func(o *TextOverlay) bool {
+			return o.x%6 == 0 && absInt((o.x+o.w/2)-frameW/2) <= 3 && o.y == 0
+		}},
+		{name: "center", pos: "cc", want: func(o *TextOverlay) bool {
+			return o.x%6 == 0 && absInt((o.x+o.w/2)-frameW/2) <= 3 && o.y == (frameH-o.h)/2
+		}},
+		{name: "bottom right", pos: "br", want: func(o *TextOverlay) bool {
+			return o.x+o.w == frameW && o.y+o.h == frameH
+		}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			o, err := NewTextOverlay("AB", frameW, frameH, 0, 0, tc.pos, face)
+			if err != nil {
+				t.Fatalf("NewTextOverlay: %v", err)
+			}
+			if !tc.want(o) {
+				t.Fatalf("unexpected %s position: box=(%d,%d %dx%d)", tc.pos, o.x, o.y, o.w, o.h)
+			}
+		})
+	}
+}
+
+func TestNewTextOverlayRejectsInvalidPosition(t *testing.T) {
+	const frameW, frameH = 1920, 1080
+	face := testFace(t, 48)
+
+	tests := []struct {
+		name       string
+		x, y       int
+		wantErrSub string
+	}{
+		{name: "unaligned x", x: 7, y: 0, wantErrSub: "divisible by 6"},
+		{name: "negative x", x: -6, y: 0, wantErrSub: "must not be negative"},
+		{name: "right overflow", x: 1902, y: 0, wantErrSub: "does not fit"},
+		{name: "bottom overflow", x: 0, y: 1070, wantErrSub: "does not fit"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := NewTextOverlay("AB", frameW, frameH, tc.x, tc.y, "", face)
+			if err == nil || !strings.Contains(err.Error(), tc.wantErrSub) {
+				t.Fatalf("error = %v, want substring %q", err, tc.wantErrSub)
+			}
+		})
+	}
+}
+
+func absInt(v int) int {
+	if v < 0 {
+		return -v
+	}
+	return v
+}
+
 func TestTextOverlayApplyV210(t *testing.T) {
 	const frameW, frameH = 1920, 1080
 	o, err := NewTextOverlay("MXL", frameW, frameH, 0, 0, "", testFace(t, 48))
@@ -102,34 +171,11 @@ func TestTextOverlayApplyV210(t *testing.T) {
 		t.Fatalf("ApplyV210: %v", err)
 	}
 
-	sample := func(x, y int) (yc, cb, cr uint32) {
-		p := y*frameW + x
-		off := (p / 6) * 16
-		w0 := binary.LittleEndian.Uint32(frame[off:])
-		w1 := binary.LittleEndian.Uint32(frame[off+4:])
-		w2 := binary.LittleEndian.Uint32(frame[off+8:])
-		w3 := binary.LittleEndian.Uint32(frame[off+12:])
-		switch p % 6 {
-		case 0:
-			return (w0 >> 10) & 0x3FF, w0 & 0x3FF, (w0 >> 20) & 0x3FF
-		case 1:
-			return w1 & 0x3FF, w0 & 0x3FF, (w0 >> 20) & 0x3FF
-		case 2:
-			return (w1 >> 20) & 0x3FF, (w1 >> 10) & 0x3FF, w2 & 0x3FF
-		case 3:
-			return (w2 >> 10) & 0x3FF, (w1 >> 10) & 0x3FF, w2 & 0x3FF
-		case 4:
-			return w3 & 0x3FF, (w2 >> 20) & 0x3FF, (w3 >> 10) & 0x3FF
-		default:
-			return (w3 >> 20) & 0x3FF, (w2 >> 20) & 0x3FF, (w3 >> 10) & 0x3FF
-		}
-	}
-
 	// Every box pixel: Y from coverage, neutral chroma at even columns.
 	for row := 0; row < o.h; row++ {
 		for col := 0; col < o.w; col++ {
 			wantY := uint32(64 + (int(o.cov[row*o.w+col])*876+127)/255)
-			yc, cb, cr := sample(o.x+col, o.y+row)
+			yc, cb, cr := sampleV210(frame, frameW, o.x+col, o.y+row)
 			if yc != wantY {
 				t.Fatalf("box pixel (%d,%d): Y=%d, want %d", o.x+col, o.y+row, yc, wantY)
 			}

internal/generator/v210_test.go

@@ -0,0 +1,27 @@
+package generator
+
+import "encoding/binary"
+
+func sampleV210(buf []byte, width, x, y int) (yc, cb, cr uint32) {
+	pixel := y*width + x
+	offset := (pixel / 6) * 16
+	w0 := binary.LittleEndian.Uint32(buf[offset:])
+	w1 := binary.LittleEndian.Uint32(buf[offset+4:])
+	w2 := binary.LittleEndian.Uint32(buf[offset+8:])
+	w3 := binary.LittleEndian.Uint32(buf[offset+12:])
+
+	switch pixel % 6 {
+	case 0:
+		return (w0 >> 10) & 0x3FF, w0 & 0x3FF, (w0 >> 20) & 0x3FF
+	case 1:
+		return w1 & 0x3FF, w0 & 0x3FF, (w0 >> 20) & 0x3FF
+	case 2:
+		return (w1 >> 20) & 0x3FF, (w1 >> 10) & 0x3FF, w2 & 0x3FF
+	case 3:
+		return (w2 >> 10) & 0x3FF, (w1 >> 10) & 0x3FF, w2 & 0x3FF
+	case 4:
+		return w3 & 0x3FF, (w2 >> 20) & 0x3FF, (w3 >> 10) & 0x3FF
+	default:
+		return (w3 >> 20) & 0x3FF, (w2 >> 20) & 0x3FF, (w3 >> 10) & 0x3FF
+	}
+}

internal/generator/wgpu_move_test.go

@@ -1,7 +1,6 @@
 package generator
 
 import (
-	"encoding/binary"
 	"path/filepath"
 	"testing"
 )
@@ -15,28 +14,6 @@ func TestWGPUMoveSquare(t *testing.T) {
 	defer g.Close()
 
 	buf := make([]byte, width*height*8/3)
-	sample := func(x, y int) (yc, cb, cr uint32) {
-		p := y*width + x
-		off := (p / 6) * 16
-		w0 := binary.LittleEndian.Uint32(buf[off:])
-		w1 := binary.LittleEndian.Uint32(buf[off+4:])
-		w2 := binary.LittleEndian.Uint32(buf[off+8:])
-		w3 := binary.LittleEndian.Uint32(buf[off+12:])
-		switch p % 6 {
-		case 0:
-			return (w0 >> 10) & 0x3FF, w0 & 0x3FF, (w0 >> 20) & 0x3FF
-		case 1:
-			return w1 & 0x3FF, w0 & 0x3FF, (w0 >> 20) & 0x3FF
-		case 2:
-			return (w1 >> 20) & 0x3FF, (w1 >> 10) & 0x3FF, w2 & 0x3FF
-		case 3:
-			return (w2 >> 10) & 0x3FF, (w1 >> 10) & 0x3FF, w2 & 0x3FF
-		case 4:
-			return w3 & 0x3FF, (w2 >> 20) & 0x3FF, (w3 >> 10) & 0x3FF
-		default:
-			return (w3 >> 20) & 0x3FF, (w2 >> 20) & 0x3FF, (w3 >> 10) & 0x3FF
-		}
-	}
 
 	// square center x=960 sits on the green bar (534/253/207):
 	// inverted -> Y=1004-534=470, Cb=1024-253=771, Cr=1024-207=817
@@ -47,28 +24,28 @@ func TestWGPUMoveSquare(t *testing.T) {
 	if err := g.GenerateFrame(buf, 0); err != nil {
 		t.Fatalf("tick 0: %v", err)
 	}
-	if y, cb, cr := sample(960, 540); y != invY || cb != invCb || cr != invCr {
+	if y, cb, cr := sampleV210(buf, width, 960, 540); y != invY || cb != invCb || cr != invCr {
 		t.Fatalf("tick 0 center: got %d/%d/%d, want inverted green 470/771/817", y, cb, cr)
 	}
-	if y, _, _ := sample(860, 540); y != greenY {
+	if y, _, _ := sampleV210(buf, width, 860, 540); y != greenY {
 		t.Fatalf("tick 0 left of square: Y=%d, want green %d", y, greenY)
 	}
 
 	// amplitude = centerX - half = 885: tick 79 (sin~1) puts the square at
-	// the far right, x in [1770,1920), over the blue bar (111/848/481):
-	// inverted -> Y=1004-111=893, Cb=1024-848=176, Cr=1024-481=543
-	const invBlueY, invBlueCb, invBlueCr = 893, 176, 543
+	// the far right, x in [1770,1920), over the gray side panel
+	// (414/512/512): inverted -> 590/512/512.
+	const invFlankY, invFlankCb, invFlankCr = 590, 512, 512
 	if err := g.GenerateFrame(buf, 79); err != nil {
 		t.Fatalf("tick 79: %v", err)
 	}
-	if y, cb, cr := sample(1840, 540); y != invBlueY || cb != invBlueCb || cr != invBlueCr {
-		t.Fatalf("tick 79 shifted center: got %d/%d/%d, want inverted blue 893/176/543", y, cb, cr)
+	if y, cb, cr := sampleV210(buf, width, 1840, 540); y != invFlankY || cb != invFlankCb || cr != invFlankCr {
+		t.Fatalf("tick 79 shifted center: got %d/%d/%d, want inverted gray 590/512/512", y, cb, cr)
 	}
-	if y, _, _ := sample(960, 540); y != greenY {
+	if y, _, _ := sampleV210(buf, width, 960, 540); y != greenY {
 		t.Fatalf("tick 79 old center: Y=%d, want green %d (square moved away)", y, greenY)
 	}
 	// bars untouched far from the square
-	if y, _, _ := sample(100, 100); y != 721 {
-		t.Fatalf("tick 79 bars: Y=%d, want white 721", y)
+	if y, _, _ := sampleV210(buf, width, 100, 100); y != 414 {
+		t.Fatalf("tick 79 bars: Y=%d, want gray flank 414", y)
 	}
 }

internal/generator/wgpu_test.go

@@ -1,12 +1,11 @@
 package generator
 
 import (
-	"encoding/binary"
 	"path/filepath"
 	"testing"
 )
 
-func TestWGPUGenerator(t *testing.T) {
+func TestWGPUSMPTEPattern(t *testing.T) {
 	const width, height = 1920, 1080
 	g, err := NewWGPUGenerator(width, height, filepath.Join("..", "..", "kernels", "static", "smpteBars.wgsl"))
 	if err != nil {
@@ -22,46 +21,31 @@ func TestWGPUGenerator(t *testing.T) {
 		t.Fatalf("GenerateFrame 2: %v", err)
 	}
 
-	want := [7][3]uint32{
-		{721, 512, 512}, {674, 176, 543}, {581, 589, 176},
-		{534, 253, 207}, {251, 771, 817}, {204, 435, 848}, {111, 848, 481},
-	}
-	barOf := func(x int) int {
-		if b := x * 7 / width; b < 7 {
-			return b
-		}
-		return 6
-	}
-	for y := 0; y < height; y++ {
-		for x := 0; x < width; x++ {
-			p := y*width + x
-			off := (p / 6) * 16
-			w0 := binary.LittleEndian.Uint32(buf[off:])
-			w1 := binary.LittleEndian.Uint32(buf[off+4:])
-			w2 := binary.LittleEndian.Uint32(buf[off+8:])
-			w3 := binary.LittleEndian.Uint32(buf[off+12:])
-			var yv, cb, cr uint32
-			switch p % 6 {
-			case 0:
-				yv, cb, cr = (w0>>10)&0x3FF, w0&0x3FF, (w0>>20)&0x3FF
-			case 1:
-				yv = w1 & 0x3FF
-			case 2:
-				yv, cb, cr = (w1>>20)&0x3FF, (w1>>10)&0x3FF, w2&0x3FF
-			case 3:
-				yv = (w2 >> 10) & 0x3FF
-			case 4:
-				yv, cb, cr = w3&0x3FF, (w2>>20)&0x3FF, (w3>>10)&0x3FF
-			case 5:
-				yv = (w3 >> 20) & 0x3FF
-			}
-			b := want[barOf(x)]
-			if yv != b[0] {
-				t.Fatalf("pixel (%d,%d): Y=%d want %d", x, y, yv, b[0])
-			}
-			if x%2 == 0 && (cb != b[1] || cr != b[2]) {
-				t.Fatalf("pixel (%d,%d): Cb=%d Cr=%d want %d/%d", x, y, cb, cr, b[1], b[2])
+	tests := []struct {
+		name                  string
+		x, y                  int
+		wantY, wantCb, wantCr uint32
+	}{
+		{"top left gray flank", 100, 100, 414, 512, 512},
+		{"top white bar", 300, 100, 721, 512, 512},
+		{"top green bar", 960, 100, 534, 253, 207},
+		{"top right gray flank", 1800, 100, 414, 512, 512},
+		{"section 2 cyan flank", 100, 650, 754, 615, 64},
+		{"section 2 minus I", 300, 650, 244, 612, 395},
+		{"section 2 white", 600, 650, 721, 512, 512},
+		{"section 2 blue flank", 1800, 650, 127, 960, 471},
+		{"section 3 yellow flank", 100, 750, 877, 64, 553},
+		{"section 3 plus Q", 300, 750, 141, 697, 606},
+		{"section 3 red flank", 1800, 750, 250, 409, 960},
+		{"bottom gray flank", 100, 900, 195, 512, 512},
 	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			y, cb, cr := sampleV210(buf, width, tt.x, tt.y)
+			if y != tt.wantY || cb != tt.wantCb || cr != tt.wantCr {
+				t.Fatalf("pixel (%d,%d): got %d/%d/%d, want %d/%d/%d",
+					tt.x, tt.y, y, cb, cr, tt.wantY, tt.wantCb, tt.wantCr)
 			}
+		})
 	}
 }