diff --git a/internal/audio/factory.go b/internal/audio/factory.go index 48b6dd4..a5bb361 100644 --- a/internal/audio/factory.go +++ b/internal/audio/factory.go @@ -108,16 +108,17 @@ func newBackendWithChecker(backendType string, isWSLFunc func() bool, commandExi } } -// createSystemCommandBackendWithChecker picks the best available system -// audio command and constructs a SystemCommandBackend. +// createSystemCommandBackendWithChecker captures every available system audio +// command in priority order so playback can fall back when the primary command +// fails or cannot handle a file format. func createSystemCommandBackendWithChecker(commandExists func(string) bool) (AudioBackend, error) { - preferred := getPreferredSystemCommandWithChecker(commandExists) - if preferred == "" { + commands := getAvailableSystemCommandsWithChecker(commandExists) + if len(commands) == 0 { slog.Error("no system audio commands available") return nil, fmt.Errorf("%w: no system audio commands found", ErrBackendNotAvailable) } - slog.Debug("system command backend created", "command", preferred) - return NewSystemCommandBackend(preferred), nil + slog.Debug("system command backend created", "commands", commands) + return NewSystemCommandBackend(commands...), nil } // createRegisteredBackend instantiates a backend whose constructor was diff --git a/internal/audio/factory_test.go b/internal/audio/factory_test.go index 8dd5d2b..cc8ee0e 100644 --- a/internal/audio/factory_test.go +++ b/internal/audio/factory_test.go @@ -2,6 +2,7 @@ package audio import ( "errors" + "reflect" "testing" ) @@ -179,13 +180,14 @@ func TestNewBackend_SystemCommandSelection(t *testing.T) { tests := []struct { name string availableCommands []string + expectedCommands []string expectError bool }{ - {"paplay preferred", []string{"aplay", "paplay", "ffplay"}, false}, - {"ffplay when no paplay", []string{"aplay", "ffplay"}, false}, - {"aplay fallback", []string{"aplay"}, false}, - {"afplay on macOS-like", []string{"afplay"}, false}, - {"no commands", []string{}, true}, + {"paplay preferred", []string{"aplay", "paplay", "ffplay"}, []string{"paplay", "ffplay", "aplay"}, false}, + {"ffplay when no paplay", []string{"aplay", "ffplay"}, []string{"ffplay", "aplay"}, false}, + {"aplay fallback", []string{"aplay"}, []string{"aplay"}, false}, + {"afplay on macOS-like", []string{"afplay"}, []string{"afplay"}, false}, + {"no commands", []string{}, nil, true}, } for _, tt := range tests { @@ -208,9 +210,13 @@ func TestNewBackend_SystemCommandSelection(t *testing.T) { t.Errorf("unexpected error: %v", err) } if !tt.expectError { - if _, ok := backend.(*SystemCommandBackend); !ok { + scb, ok := backend.(*SystemCommandBackend) + if !ok { t.Errorf("expected *SystemCommandBackend, got %T", backend) } + if ok && !reflect.DeepEqual(scb.commands, tt.expectedCommands) { + t.Errorf("commands = %v, want %v", scb.commands, tt.expectedCommands) + } if backend != nil { _ = backend.Close() } diff --git a/internal/audio/platform.go b/internal/audio/platform.go index 1e470e8..47e22d7 100644 --- a/internal/audio/platform.go +++ b/internal/audio/platform.go @@ -49,21 +49,33 @@ func detectOptimalBackendWithChecker(isWSL bool, commandChecker func(string) boo // getPreferredSystemCommandWithChecker allows dependency injection for testing func getPreferredSystemCommandWithChecker(commandChecker func(string) bool) string { - // Priority order: paplay (PulseAudio) > ffplay (FFmpeg) > aplay (ALSA) > afplay (macOS) - preferredCommands := []string{ + available := getAvailableSystemCommandsWithChecker(commandChecker) + if len(available) == 0 { + slog.Debug("no preferred system audio commands found") + return "" + } + + slog.Debug("preferred system command found", "command", available[0]) + return available[0] +} + +// getAvailableSystemCommandsWithChecker returns all available system audio +// commands in priority order. +func getAvailableSystemCommandsWithChecker(commandChecker func(string) bool) []string { + allCommands := []string{ "paplay", // PulseAudio - most common on modern Linux "ffplay", // FFmpeg - widely available and versatile "aplay", // ALSA - lower-level Linux audio "afplay", // macOS built-in audio player } - for _, cmd := range preferredCommands { + var available []string + for _, cmd := range allCommands { if commandChecker(cmd) { - slog.Debug("preferred system command found", "command", cmd) - return cmd + available = append(available, cmd) } } - slog.Debug("no preferred system audio commands found") - return "" + slog.Debug("available system audio commands", "commands", available, "count", len(available)) + return available } diff --git a/internal/audio/platform_test.go b/internal/audio/platform_test.go index 7ffb974..aa0f77d 100644 --- a/internal/audio/platform_test.go +++ b/internal/audio/platform_test.go @@ -1,6 +1,7 @@ package audio import ( + "reflect" "runtime" "testing" ) @@ -189,6 +190,48 @@ func TestGetPreferredSystemCommand(t *testing.T) { } } +func TestGetAvailableSystemCommands(t *testing.T) { + tests := []struct { + name string + availableCommands []string + want []string + }{ + { + name: "returns all commands in priority order", + availableCommands: []string{"aplay", "paplay", "afplay", "ffplay"}, + want: []string{"paplay", "ffplay", "aplay", "afplay"}, + }, + { + name: "returns subset in priority order", + availableCommands: []string{"aplay", "ffplay"}, + want: []string{"ffplay", "aplay"}, + }, + { + name: "returns empty slice when none are available", + availableCommands: []string{}, + want: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + commandChecker := func(cmd string) bool { + for _, available := range tt.availableCommands { + if cmd == available { + return true + } + } + return false + } + + got := getAvailableSystemCommandsWithChecker(commandChecker) + if !reflect.DeepEqual(got, tt.want) { + t.Fatalf("getAvailableSystemCommandsWithChecker() = %v, want %v", got, tt.want) + } + }) + } +} + // TestRealSystemIntegration tests against the real system (these may vary by environment) func TestRealSystemIntegration(t *testing.T) { t.Run("real command detection", func(t *testing.T) { diff --git a/internal/audio/system_command_backend.go b/internal/audio/system_command_backend.go index 768ad94..c83449b 100644 --- a/internal/audio/system_command_backend.go +++ b/internal/audio/system_command_backend.go @@ -1,6 +1,7 @@ package audio import ( + "bytes" "context" "fmt" "io" @@ -10,12 +11,13 @@ import ( "os/exec" "path/filepath" "strconv" + "strings" "sync" ) // SystemCommandBackend implements AudioBackend using system commands like paplay type SystemCommandBackend struct { - command string + commands []string volume float32 isPlaying bool closed bool @@ -23,12 +25,15 @@ type SystemCommandBackend struct { warnNoVolumeOnce sync.Once // one WARN per backend instance for aplay } -// NewSystemCommandBackend creates a new SystemCommandBackend with the specified command -func NewSystemCommandBackend(command string) *SystemCommandBackend { - slog.Debug("creating new SystemCommandBackend", "command", command) +// NewSystemCommandBackend creates a new SystemCommandBackend with the specified +// commands in priority order. A single command preserves the historical call +// shape; multiple commands enable best-effort fallback when the primary command +// fails or cannot handle the file format. +func NewSystemCommandBackend(commands ...string) *SystemCommandBackend { + slog.Debug("creating new SystemCommandBackend", "commands", commands) return &SystemCommandBackend{ - command: command, - volume: 1.0, // Default full volume + commands: append([]string(nil), commands...), + volume: 1.0, // Default full volume } } @@ -119,7 +124,7 @@ func (scb *SystemCommandBackend) Play(ctx context.Context, source AudioSource) e scb.mutex.Unlock() }() - slog.Debug("SystemCommandBackend starting playback", "command", scb.command) + slog.Debug("SystemCommandBackend starting playback", "commands", scb.commands) // Fast path: source can provide a file path directly (FileSource). Exec // the player binary against the path without the read-then-write-temp @@ -151,16 +156,27 @@ func (scb *SystemCommandBackend) loadVolume() float32 { return scb.volume } -// buildPlayerArgv returns the argv (NOT including scb.command itself) to play -// filePath at volume v on the configured backend. v is in [0.0, 1.0]; the -// function scales it to the backend's native value space. Backends without a +func (scb *SystemCommandBackend) primaryCommand() string { + if len(scb.commands) == 0 { + return "" + } + return scb.commands[0] +} + +// buildPlayerArgv returns the argv (NOT including the command itself) to play +// filePath at volume v on the primary configured backend. v is in [0.0, 1.0]; +// the function scales it to the backend's native value space. Backends without a // native volume flag (e.g. aplay) ignore v and log a one-time WARN. // // Verified mappings (paplay, ffplay, afplay) come from each player's // authoritative documentation. afplay's mapping is identity — review finding // #4 incorrectly claimed 0..255 scaling; afplay treats `-v 1.0` as 100%. func (scb *SystemCommandBackend) buildPlayerArgv(filePath string, v float64) []string { - switch filepath.Base(scb.command) { + return scb.buildPlayerArgvForCommand(scb.primaryCommand(), filePath, v) +} + +func (scb *SystemCommandBackend) buildPlayerArgvForCommand(command, filePath string, v float64) []string { + switch filepath.Base(command) { case "paplay": // PulseAudio: --volume=N where N is uint32, 65536 = 100%. n := uint32(math.Round(v * 65536)) @@ -180,7 +196,7 @@ func (scb *SystemCommandBackend) buildPlayerArgv(filePath string, v float64) []s if v != 1.0 { scb.warnNoVolumeOnce.Do(func() { slog.Warn("aplay has no native volume flag; configured volume ignored", - "command", scb.command, "volume", v) + "command", command, "volume", v) }) } return []string{filePath} @@ -191,25 +207,65 @@ func (scb *SystemCommandBackend) buildPlayerArgv(filePath string, v float64) []s } } -// playFile plays a file directly using the system command +// commandSupportsFormat reports whether a system audio command should be tried +// for a file extension. aplay is limited to WAV on the supported platforms; the +// other known command players are treated as general-purpose decoders. +func commandSupportsFormat(command, ext string) bool { + if filepath.Base(command) != "aplay" { + return true + } + return strings.EqualFold(ext, ".wav") +} + +// playFile plays a file directly using the configured system command chain. func (scb *SystemCommandBackend) playFile(ctx context.Context, filePath string) error { - slog.Debug("playing file via system command", "file", filePath, "command", scb.command) + slog.Debug("playing file via system command", "file", filePath, "commands", scb.commands) v := scb.loadVolume() - argv := scb.buildPlayerArgv(filePath, float64(v)) + ext := filepath.Ext(filePath) + var lastErr error + var attempted int + + for i, command := range scb.commands { + if !commandSupportsFormat(command, ext) { + slog.Debug("skipping system command unsupported for format", + "command", command, "ext", ext, "file", filePath) + continue + } - // Create command with context for cancellation - cmd := exec.CommandContext(ctx, scb.command, argv...) + attempted++ + argv := scb.buildPlayerArgvForCommand(command, filePath, float64(v)) + cmd := exec.CommandContext(ctx, command, argv...) + var stderr bytes.Buffer + cmd.Stderr = &stderr + + err := cmd.Run() + if err == nil { + if i > 0 { + slog.Info("playback succeeded via fallback", + "command", command, "argv", argv, "file", filePath, "attempt", i+1) + } else { + slog.Debug("file playback completed successfully", "file", filePath, "argv", argv) + } + return nil + } - // Run the command and wait for completion - err := cmd.Run() - if err != nil { - slog.Error("system command failed", "command", scb.command, "argv", argv, "file", filePath, "error", err) - return fmt.Errorf("system command failed: %w", err) + lastErr = err + stderrText := strings.TrimSpace(stderr.String()) + if attempted == 1 { + slog.Warn("primary audio command failed", + "command", command, "argv", argv, "file", filePath, "error", err, "stderr", stderrText) + } else { + slog.Warn("fallback audio command failed", + "command", command, "argv", argv, "file", filePath, "error", err, "stderr", stderrText) + } } - slog.Debug("file playback completed successfully", "file", filePath, "argv", argv) - return nil + if lastErr == nil { + return fmt.Errorf("no audio commands support format %q", ext) + } + slog.Error("all audio commands failed", "file", filePath, "commands_tried", attempted, "error", lastErr) + return fmt.Errorf("all audio commands failed for %s: %w", filepath.Base(filePath), lastErr) } // playReaderViaTempFile writes reader data to a temporary file and plays it diff --git a/internal/audio/system_command_backend_test.go b/internal/audio/system_command_backend_test.go index 7b3400b..e992b7c 100644 --- a/internal/audio/system_command_backend_test.go +++ b/internal/audio/system_command_backend_test.go @@ -1,12 +1,49 @@ package audio import ( + "context" "math" + "os" + "path/filepath" "reflect" "strings" "testing" ) +func TestNewSystemCommandBackendStoresFallbackChain(t *testing.T) { + scb := NewSystemCommandBackend("paplay", "ffplay", "aplay") + want := []string{"paplay", "ffplay", "aplay"} + if !reflect.DeepEqual(scb.commands, want) { + t.Fatalf("commands = %v, want %v", scb.commands, want) + } +} + +func TestCommandSupportsFormat(t *testing.T) { + tests := []struct { + name string + command string + ext string + want bool + }{ + {name: "aplay supports wav", command: "aplay", ext: ".wav", want: true}, + {name: "aplay supports wav case-insensitive", command: "/usr/bin/aplay", ext: ".WAV", want: true}, + {name: "aplay rejects mp3", command: "aplay", ext: ".mp3", want: false}, + {name: "aplay rejects aiff", command: "aplay", ext: ".aiff", want: false}, + {name: "paplay accepts mp3", command: "paplay", ext: ".mp3", want: true}, + {name: "ffplay accepts aiff", command: "ffplay", ext: ".aiff", want: true}, + {name: "unknown accepts mp3", command: "custom-player", ext: ".mp3", want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := commandSupportsFormat(tt.command, tt.ext) + if got != tt.want { + t.Fatalf("commandSupportsFormat(%q, %q) = %v, want %v", tt.command, tt.ext, got, tt.want) + } + }) + } +} + // TestSetVolume_RejectsNaN verifies SetVolume rejects NaN. NaN evaluates as // false for both bounds checks, so without an explicit guard it would slip // past the [0.0, 1.0] range check and reach the subprocess argv. @@ -181,3 +218,49 @@ func TestBuildPlayerArgv_VolumeReachesArgv(t *testing.T) { t.Errorf("volume set via SetVolume should reach argv as --volume=16384; got %v", argv) } } + +func TestPlayFileWithFallbackChain(t *testing.T) { + tmpDir := t.TempDir() + wavFile := filepath.Join(tmpDir, "sound.wav") + if err := os.WriteFile(wavFile, []byte("fake wav"), 0644); err != nil { + t.Fatalf("write wav fixture: %v", err) + } + + t.Run("primary command fails then fallback succeeds", func(t *testing.T) { + scb := NewSystemCommandBackend("nonexistent-command-claudio", successfulNoopCommand()) + if err := scb.playFile(context.Background(), wavFile); err != nil { + t.Fatalf("playFile should succeed via fallback: %v", err) + } + }) + + t.Run("all commands fail", func(t *testing.T) { + scb := NewSystemCommandBackend("nonexistent-command-one", "nonexistent-command-two") + if err := scb.playFile(context.Background(), wavFile); err == nil { + t.Fatal("playFile should fail when every command fails") + } + }) + + t.Run("skips format-incompatible commands", func(t *testing.T) { + mp3File := filepath.Join(tmpDir, "sound.mp3") + if err := os.WriteFile(mp3File, []byte("fake mp3"), 0644); err != nil { + t.Fatalf("write mp3 fixture: %v", err) + } + + scb := NewSystemCommandBackend("aplay", successfulNoopCommand()) + if err := scb.playFile(context.Background(), mp3File); err != nil { + t.Fatalf("playFile should skip aplay for mp3 and use fallback: %v", err) + } + }) + + t.Run("all commands skipped for unsupported format", func(t *testing.T) { + mp3File := filepath.Join(tmpDir, "unsupported.mp3") + if err := os.WriteFile(mp3File, []byte("fake mp3"), 0644); err != nil { + t.Fatalf("write mp3 fixture: %v", err) + } + + scb := NewSystemCommandBackend("aplay") + if err := scb.playFile(context.Background(), mp3File); err == nil { + t.Fatal("playFile should fail when every command is format-incompatible") + } + }) +}