CLI parsing refactored

ARCHITECTURE_REVIEW.md

@@ -73,7 +73,7 @@ For `audio/float32`, the audio package may use `[]float32` internally and keep b
 | 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. |
+| 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. |
 | 8 | The Makefile clean target uses `fm -f` instead of `rm -f`. | **Open** | The typo remains in the `clean` target. |
 
@@ -85,7 +85,7 @@ For `audio/float32`, the audio package may use `[]float32` internally and keep b
 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.
+7. 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.
 
 ## Suggested package layout
 

cmd/mxl-pattern/main.go

@@ -4,10 +4,14 @@
 package main
 
 import (
+	"context"
 	"fmt"
+	"io"
 	"log"
 	"os"
 	"os/signal"
+	"sort"
+	"strings"
 	"syscall"
 
 	"github.com/google/uuid"
@@ -47,6 +51,11 @@ type appArgs struct {
 	audioUUID         string
 }
 
+type parseResult struct {
+	args      appArgs
+	shouldRun bool
+}
+
 var frameRates = map[string]mxl.Rational{
 	"23.97": {Num: 24000, Den: 1001},
 	"24":    {Num: 24, Den: 1},
@@ -67,119 +76,126 @@ var samplingRates = map[string]mxl.Rational{
 	"192":  {Num: 192000, Den: 1},
 }
 
-func printHelp(fs *pflag.FlagSet) {
-	fmt.Printf("%s %s\n", APP_NAME, APP_VER)
-	fmt.Println("Usage: mxl-gen -d <domain> [-v <flowDef.json>] [-a <flowDef.json>] [options]")
-	fmt.Println("   or: mxl-gen -d <domain> [--with <width px>] [--height <height px>] [--fps <framerate>] \\")
-	fmt.Println("   [-c <channels amount>] [-f <sample rate>]")
-	fmt.Println("   or: mxl-gen -d <domain> with default params")
-	fmt.Println("Video and audio feeds params will be ignored, if flow definition file provided.")
-	fmt.Println()
+func printHelp(w io.Writer, fs *pflag.FlagSet) {
+	fmt.Fprintf(w, "%s %s\n", APP_NAME, APP_VER)
+	fmt.Fprintln(w, "Usage: mxl-gen -d <domain> [-v <flowDef.json>] [-a <flowDef.json>] [options]")
+	fmt.Fprintln(w, "   or: mxl-gen -d <domain> [--width <width px>] [--height <height px>] [--fps <framerate>] \\")
+	fmt.Fprintln(w, "   [-c <channels amount>] [-f <sample rate>]")
+	fmt.Fprintln(w, "   or: mxl-gen -d <domain> with default params")
+	fmt.Fprintln(w, "Video and audio feed parameters are ignored when a flow definition file is provided.")
+	fmt.Fprintln(w)
+	fs.SetOutput(w)
 	fs.PrintDefaults()
 }
 
-func printUsage() {
-	fmt.Fprintln(os.Stderr, "Usage: mxl-gen -d <domain> [-v <flowDef.json>] [-a <flowDef.json>] [options]")
-	fmt.Fprintln(os.Stderr, "Try 'mxl-gen -h' for more information.")
+func printUsage(w io.Writer) {
+	fmt.Fprintln(w, "Usage: mxl-gen -d <domain> [-v <flowDef.json>] [-a <flowDef.json>] [options]")
+	fmt.Fprintln(w, "Try 'mxl-gen -h' for more information.")
 }
 
-func checkArgs(args *appArgs) {
-	printUsageAndExit := func() {
-		printUsage()
-		os.Exit(2)
+func validateDomain(domain string) error {
+	if domain == "" {
+		return fmt.Errorf("domain is required")
 	}
-	// domain
-	if args.domain == "" {
-		fmt.Fprintf(os.Stderr, "Domain is required\n")
-		printUsageAndExit()
+	fi, err := os.Stat(domain)
+	if err != nil {
+		return fmt.Errorf("invalid MXL domain %q: %w", domain, err)
 	}
-	fi, err := os.Stat(args.domain)
-	if err != nil || !fi.IsDir() {
-		fmt.Fprintf(os.Stderr, "Invalid MXL domain: %s\n", args.domain)
-		fmt.Fprintf(os.Stderr, "Reason: %v\n", err)
-		printUsageAndExit()
+	if !fi.IsDir() {
+		return fmt.Errorf("invalid MXL domain %q: not a directory", domain)
 	}
-	if ok, err := mxl.IsTmpFs(args.domain); err != nil || !ok {
-		fmt.Fprintf(os.Stderr, "Invalid MXL domain: %s\n", args.domain)
-		fmt.Fprintln(os.Stderr, "Domain must be directory in tmps.")
-		printUsageAndExit()
+	if ok, err := mxl.IsTmpFs(domain); err != nil || !ok {
+		if err != nil {
+			return fmt.Errorf("check MXL domain %q: %w", domain, err)
 		}
-	// FlowDef
-	checkFlowDef := func(label, flowDef string) {
-		fi, err := os.Stat(flowDef)
-		if err != nil || fi.IsDir() {
-			fmt.Fprintf(os.Stderr, "%s flow definition .json file is not accesible\n", label)
-			printUsageAndExit()
-		}
-	}
-	videoFlowDefProvided, audioFlowDefProvided := false, false
-	if args.videoFlowDefFile != "" {
-		checkFlowDef("Video", args.videoFlowDefFile)
-		videoFlowDefProvided = true
-	}
-	if args.audioFlowDefFile != "" {
-		checkFlowDef("Audio", args.audioFlowDefFile)
-		audioFlowDefProvided = true
+		return fmt.Errorf("invalid MXL domain %q: directory must be on tmpfs", domain)
 	}
+	return nil
+}
 
-	if !videoFlowDefProvided {
+func validateFlowDefPath(label, path string) error {
+	if path == "" {
+		return nil
+	}
+	fi, err := os.Stat(path)
+	if err != nil {
+		return fmt.Errorf("%s flow definition %q is not accessible: %w", label, path, err)
+	}
+	if fi.IsDir() {
+		return fmt.Errorf("%s flow definition %q is a directory", label, path)
+	}
+	return nil
+}
+
+func validateVideoArgs(args *appArgs) error {
+	if args.videoFlowDefFile != "" {
+		return nil
+	}
 	if args.videoWidth == 0 || args.videoWidth%6 != 0 {
-			// width%6 == 0 - because of v210 (6 pixels per 16-byte block)
-			fmt.Fprintf(os.Stderr, "Video width must be > 0 and divisible by 6\n")
-			printUsageAndExit()
+		// v210 stores 6 pixels in each 16-byte block.
+		return fmt.Errorf("video width must be greater than zero and divisible by 6")
 	}
 	if args.videoHeight == 0 {
-			fmt.Fprintf(os.Stderr, "Video height must be > 0\n")
-			printUsageAndExit()
+		return fmt.Errorf("video height must be greater than zero")
 	}
 	if _, exists := frameRates[args.videoFPS]; !exists {
-			fmt.Fprintf(os.Stderr, "FPS %s is not in available list.\n", args.videoFPS)
-			fmt.Fprintln(os.Stderr, "If you need more complex solution, use flow definition .json instead.")
-			fmt.Fprintln(os.Stderr, "Available list:")
-			for key, _ := range frameRates {
-				fmt.Fprintf(os.Stderr, "  %s\n", key)
+		return fmt.Errorf("unsupported video FPS %q (supported: %s); use a flow definition for other rates",
+			args.videoFPS, sortedMapKeys(frameRates))
 	}
-			printUsageAndExit()
-		}
-		if args.videoUUID != "" {
-			if _, err := uuid.Parse(args.videoUUID); err != nil {
-				fmt.Fprintf(os.Stderr, "Video UUID %s is not valid.\n", args.videoUUID)
-				printUsageAndExit()
-			}
-		} else {
-			args.videoUUID = uuid.NewString()
-		}
-		if args.pattern != "" {
 	if _, exists := patterns[args.pattern]; !exists {
-				fmt.Fprintf(os.Stderr, "Pattern %s is not in available list.\n", args.pattern)
-				listPatterns(os.Stderr)
-				os.Exit(2)
+		return fmt.Errorf("unknown video pattern %q (use --list-patterns to see available patterns)", args.pattern)
 	}
+	if args.videoUUID == "" {
+		args.videoUUID = uuid.NewString()
+		return nil
 	}
+	if _, err := uuid.Parse(args.videoUUID); err != nil {
+		return fmt.Errorf("invalid video UUID %q: %w", args.videoUUID, err)
 	}
+	return nil
+}
 
-	if !audioFlowDefProvided {
-		if args.audioChannels == 0 {
-			// TODO: ignore audio
+func validateAudioArgs(args *appArgs) error {
+	if args.audioFlowDefFile != "" || args.audioChannels == 0 {
+		return nil
 	}
 	if _, exists := samplingRates[args.audioSamplingFreq]; !exists {
-			fmt.Fprintf(os.Stderr, "Sample rate %s is not in available list.\n", args.audioSamplingFreq)
-			fmt.Fprintln(os.Stderr, "If you need more complex solution, use flow definition .json instead.")
-			fmt.Fprintln(os.Stderr, "Available list:")
-			for key, _ := range samplingRates {
-				fmt.Fprintf(os.Stderr, "  %s\n", key)
+		return fmt.Errorf("unsupported audio sample rate %q (supported: %s); use a flow definition for other rates",
+			args.audioSamplingFreq, sortedMapKeys(samplingRates))
 	}
-			printUsageAndExit()
-		}
-		if args.audioUUID != "" {
-			if err := uuid.Validate(args.audioUUID); err != nil {
-				fmt.Fprintf(os.Stderr, "Audio UUID %s is not valid.\n", args.audioUUID)
-				printUsageAndExit()
-			}
-		} else {
+	if args.audioUUID == "" {
 		args.audioUUID = uuid.NewString()
+		return nil
+	}
+	if err := uuid.Validate(args.audioUUID); err != nil {
+		return fmt.Errorf("invalid audio UUID %q: %w", args.audioUUID, err)
+	}
+	return nil
+}
+
+func validateArgs(args *appArgs) error {
+	checks := []func() error{
+		func() error { return validateDomain(args.domain) },
+		func() error { return validateFlowDefPath("video", args.videoFlowDefFile) },
+		func() error { return validateFlowDefPath("audio", args.audioFlowDefFile) },
+		func() error { return validateVideoArgs(args) },
+		func() error { return validateAudioArgs(args) },
+	}
+	for _, check := range checks {
+		if err := check(); err != nil {
+			return err
 		}
 	}
+	return nil
+}
+
+func sortedMapKeys[V any](values map[string]V) string {
+	keys := make([]string, 0, len(values))
+	for key := range values {
+		keys = append(keys, key)
+	}
+	sort.Strings(keys)
+	return strings.Join(keys, ", ")
 }
 
 type pattern struct {
@@ -252,21 +268,25 @@ var patterns = map[string]pattern{
 	},
 }
 
-func listPatterns(f *os.File) {
-	fmt.Fprintln(f, "List of available video patterns:")
-	var maxNameWidth int = 0
-	for name, _ := range patterns {
+func listPatterns(w io.Writer) {
+	fmt.Fprintln(w, "List of available video patterns:")
+	names := make([]string, 0, len(patterns))
+	maxNameWidth := 0
+	for name := range patterns {
+		names = append(names, name)
 		l := len(name)
 		if l > maxNameWidth {
 			maxNameWidth = l
 		}
 	}
-	for name, p := range patterns {
-		fmt.Fprintf(f, "  %-*s - %s\n", maxNameWidth, name, p.description)
+	sort.Strings(names)
+	for _, name := range names {
+		p := patterns[name]
+		fmt.Fprintf(w, "  %-*s - %s\n", maxNameWidth, name, p.description)
 	}
 }
 
-func flagSetAddFlags(fs *pflag.FlagSet, args *appArgs) {
+func addFlags(fs *pflag.FlagSet, args *appArgs) {
 	// common flags
 	fs.BoolVarP(&args.showHelp, "help", "h", false, "Show help message and exit")
 	// MXL flags
@@ -308,27 +328,45 @@ func flagSetAddFlags(fs *pflag.FlagSet, args *appArgs) {
 	fs.StringVar(&args.audioUUID, "audio-id", "", "Audio UUID. Will be created, if not provided [TODO]")
 }
 
-func main() {
+func parseArgs(argv []string, stdout, stderr io.Writer) (parseResult, error) {
 	var args appArgs
 	flagSet := pflag.NewFlagSet("args", pflag.ContinueOnError)
 	flagSet.SortFlags = false
-	flagSet.Usage = func() { printUsage() }
-	flagSetAddFlags(flagSet, &args)
+	flagSet.SetOutput(stderr)
+	flagSet.Usage = func() { printUsage(stderr) }
+	addFlags(flagSet, &args)
 
-	if err := flagSet.Parse(os.Args[1:]); err != nil {
-		fmt.Fprintln(os.Stderr, err)
-		printUsage()
-		os.Exit(2)
+	if err := flagSet.Parse(argv); err != nil {
+		return parseResult{}, err
 	}
 	if args.showHelp {
-		printHelp(flagSet)
-		return
+		printHelp(stdout, flagSet)
+		return parseResult{args: args}, nil
 	}
 	if args.listPatterns {
-		listPatterns(os.Stderr)
+		listPatterns(stdout)
+		return parseResult{args: args}, nil
+	}
+	if flagSet.NArg() != 0 {
+		return parseResult{}, fmt.Errorf("unexpected positional arguments: %v", flagSet.Args())
+	}
+	if err := validateArgs(&args); err != nil {
+		return parseResult{}, err
+	}
+	return parseResult{args: args, shouldRun: true}, nil
+}
+
+func main() {
+	parsed, err := parseArgs(os.Args[1:], os.Stdout, os.Stderr)
+	if err != nil {
+		fmt.Fprintln(os.Stderr, err)
+		printUsage(os.Stderr)
+		os.Exit(2)
+	}
+	if !parsed.shouldRun {
 		return
 	}
-	checkArgs(&args)
+	args := parsed.args
 
 	type videoInfo struct {
 		uuid   string
@@ -470,3 +508,15 @@ func main() {
 		mxl.SleepNs(mxl.NsUntilIndex(idx, rate))
 	}
 }
+
+func run(ctx context.Context, args []string) error {
+	return nil
+}
+
+func runVideo(ctx context.Context) error {
+	return nil
+}
+
+func runAudio(ctx context.Context) error {
+	return nil
+}

cmd/mxl-pattern/main_test.go

@@ -0,0 +1,101 @@
+package main
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+)
+
+func TestParseArgsHelpStopsBeforeValidation(t *testing.T) {
+	var stdout, stderr bytes.Buffer
+
+	result, err := parseArgs([]string{"--help"}, &stdout, &stderr)
+	if err != nil {
+		t.Fatalf("parseArgs: %v", err)
+	}
+	if result.shouldRun {
+		t.Fatal("shouldRun = true, want false")
+	}
+	if !result.args.showHelp {
+		t.Fatal("showHelp = false, want true")
+	}
+	if !strings.Contains(stdout.String(), "Usage: mxl-gen") {
+		t.Fatalf("help output does not contain usage: %q", stdout.String())
+	}
+	if stderr.Len() != 0 {
+		t.Fatalf("stderr = %q, want empty", stderr.String())
+	}
+}
+
+func TestParseArgsListPatternsStopsBeforeValidation(t *testing.T) {
+	var stdout, stderr bytes.Buffer
+
+	result, err := parseArgs([]string{"--list-patterns"}, &stdout, &stderr)
+	if err != nil {
+		t.Fatalf("parseArgs: %v", err)
+	}
+	if result.shouldRun {
+		t.Fatal("shouldRun = true, want false")
+	}
+	if !result.args.listPatterns {
+		t.Fatal("listPatterns = false, want true")
+	}
+	if !strings.Contains(stdout.String(), "ebu75") {
+		t.Fatalf("pattern output does not contain ebu75: %q", stdout.String())
+	}
+	if stderr.Len() != 0 {
+		t.Fatalf("stderr = %q, want empty", stderr.String())
+	}
+}
+
+func TestParseArgsRejectsUnexpectedPositionalArguments(t *testing.T) {
+	var stdout, stderr bytes.Buffer
+
+	result, err := parseArgs([]string{"unexpected"}, &stdout, &stderr)
+	if err == nil {
+		t.Fatal("parseArgs returned nil error")
+	}
+	if result.shouldRun {
+		t.Fatal("shouldRun = true, want false")
+	}
+	if !strings.Contains(err.Error(), "unexpected positional arguments") {
+		t.Fatalf("error = %q", err)
+	}
+}
+
+func TestParseArgsRejectsUnknownFlag(t *testing.T) {
+	var stdout, stderr bytes.Buffer
+
+	result, err := parseArgs([]string{"--not-a-flag"}, &stdout, &stderr)
+	if err == nil {
+		t.Fatal("parseArgs returned nil error")
+	}
+	if result.shouldRun {
+		t.Fatal("shouldRun = true, want false")
+	}
+}
+
+func TestValidateAudioArgsSkipsDisabledAudio(t *testing.T) {
+	args := appArgs{
+		audioChannels:     0,
+		audioSamplingFreq: "unsupported",
+		audioUUID:         "not-a-uuid",
+	}
+
+	if err := validateAudioArgs(&args); err != nil {
+		t.Fatalf("validateAudioArgs: %v", err)
+	}
+	if args.audioUUID != "not-a-uuid" {
+		t.Fatalf("audio UUID changed while audio is disabled: %q", args.audioUUID)
+	}
+}
+
+func TestListPatternsIsSorted(t *testing.T) {
+	var output bytes.Buffer
+	listPatterns(&output)
+
+	text := output.String()
+	if strings.Index(text, "ebu100") > strings.Index(text, "ebu75") {
+		t.Fatalf("patterns are not sorted: %q", text)
+	}
+}