feat: better error messages for ffmpeg process (#11)

* feat: better error messages for ffmpeg process

* tests: add test cases for process errors

* fix test case error matching

ffmpeg_arger.go

@@ -4,3 +4,10 @@ package spireav
 type FfmpegArger interface {
 	FfmpegArgs() ([]string, error)
 }
+
+// FfmpegArgs is a simple type that implements the FfmpegArger interface.
+type FfmpegArgs []string
+
+func (args FfmpegArgs) FfmpegArgs() ([]string, error) {
+	return args, nil
+}

go.mod

@@ -1,3 +1,11 @@
 module github.com/spiretechnology/spireav
 
 go 1.20
+
+require github.com/stretchr/testify v1.8.4
+
+require (
+	github.com/davecgh/go-spew v1.1.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
+)

go.sum

@@ -0,0 +1,10 @@
+github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
+github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
+github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
+github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
+github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
+github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

process.go

@@ -18,6 +18,7 @@ type Process struct {
 	FfmpegArger           FfmpegArger
 	EstimatedLengthFrames int
 	SysProcAttr           *syscall.SysProcAttr
+	errorOutput           string
 }
 
 // GetCommandString is a utility function that gets the FFmpeg command string that is run by this process
@@ -40,11 +41,10 @@ func (p *Process) Run(
 	ctx context.Context,
 	chanProgress chan<- Progress,
 ) error {
-
 	// Generate the FFmpeg arguments
 	args, err := p.FfmpegArger.FfmpegArgs()
 	if err != nil {
-		return err
+		return fmt.Errorf("generating ffmpeg args: %w", err)
 	}
 
 	// Create the command
@@ -53,48 +53,44 @@ func (p *Process) Run(
 
 	// If progress needs to be reported
 	if chanProgress != nil {
-
 		// Get a readable pipe of the command stdout
 		stderr, err := cmd.StderrPipe()
 		if err != nil {
-			return err
+			return fmt.Errorf("acquiring stderr pipe: %w", err)
 		}
 		defer stderr.Close()
 
 		// Parse the output into progress reports on the channel
-		go p.reportFFmpegProgress(
-			chanProgress,
-			stderr,
-		)
-
+		go p.reportFFmpegProgress(chanProgress, stderr)
 	}
 
 	// Start running the command in the background
 	if err := cmd.Start(); err != nil {
-		return err
+		return fmt.Errorf("starting ffmpeg process: %w", err)
 	}
 
 	// Wait for the process to end
 	if err := cmd.Wait(); err != nil {
-
 		// If the context triggered the process to be killed, we want to see the context's error
 		// instead of the process's error
 		if ctx != nil && ctx.Err() != nil {
 			return ctx.Err()
 		}
-		return err
-
+		switch e := err.(type) {
+		case *exec.ExitError:
+			return fmt.Errorf("ffmpeg exit code %d: %s", e.ExitCode(), p.errorOutput)
+		default:
+			return fmt.Errorf("ffmpeg error: %s: %s", err, p.errorOutput)
+		}
 	}
 	return nil
-
 }
 
 // RunWithProgress executes the process and reports progress back to a progress handler function
 func (p *Process) RunWithProgress(
 	ctx context.Context,
 	progressFunc func(Progress),
 ) error {
-
 	// Create a channel for progress reporting
 	chanProgress := make(chan Progress)
 
@@ -115,14 +111,9 @@ func (p *Process) RunWithProgress(
 
 	// Return the error, if any
 	return err
-
 }
 
-func (p *Process) reportFFmpegProgress(
-	chanProgress chan<- Progress,
-	processOutput io.Reader,
-) {
-
+func (p *Process) reportFFmpegProgress(chanProgress chan<- Progress, processOutput io.Reader) {
 	// Calculate the start time
 	startTime := time.Now()
 
@@ -137,22 +128,26 @@ func (p *Process) reportFFmpegProgress(
 	scanner := bufio.NewScanner(processOutput)
 	scanner.Split(scanLinesWithCR)
 	for scanner.Scan() {
-
 		// Read a line from the input
 		line := scanner.Text()
 		line = strings.ReplaceAll(line, "\r", "\n")
-
-		// Read a new progress value
-		newProgress := parseProgressLine(
-			line,
-			p.EstimatedLengthFrames,
-			startTime,
-		)
-		if newProgress != nil {
-			progress = newProgress
-			chanProgress <- *progress
+		lines := strings.Split(line, "\n")
+		for _, line := range lines {
+			if line != "" {
+				p.errorOutput = line
+			}
+
+			// Read a new progress value
+			newProgress := parseProgressLine(
+				line,
+				p.EstimatedLengthFrames,
+				startTime,
+			)
+			if newProgress != nil {
+				progress = newProgress
+				chanProgress <- *progress
+			}
 		}
-
 	}
 
 	// Send the 100% progress
@@ -168,7 +163,6 @@ func (p *Process) reportFFmpegProgress(
 
 	// Close the channel
 	close(chanProgress)
-
 }
 
 func scanLinesWithCR(data []byte, atEOF bool) (advance int, token []byte, err error) {

process_test.go

@@ -0,0 +1,37 @@
+package spireav_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/spiretechnology/spireav"
+	"github.com/stretchr/testify/require"
+)
+
+func overrideFfmpegPath(path string) func() {
+	original := spireav.FfmpegPath
+	spireav.FfmpegPath = path
+	return func() {
+		spireav.FfmpegPath = original
+	}
+}
+
+func TestProcessErrors(t *testing.T) {
+	t.Run("invalid args", func(t *testing.T) {
+		proc := &spireav.Process{
+			FfmpegArger: spireav.FfmpegArgs{"several", "bad", "args"},
+		}
+		err := proc.RunWithProgress(context.Background(), func(p spireav.Progress) {})
+		require.Error(t, err, "expected error when running process with invalid args")
+		require.ErrorContains(t, err, "Invalid argument")
+	})
+	t.Run("ffmpeg not in path", func(t *testing.T) {
+		defer overrideFfmpegPath("something-that-does-not-exist")()
+		proc := &spireav.Process{
+			FfmpegArger: spireav.FfmpegArgs{"-i", "something"},
+		}
+		err := proc.RunWithProgress(context.Background(), func(p spireav.Progress) {})
+		require.Error(t, err, "expected error when path is missing ffmpeg binary")
+		require.ErrorContains(t, err, "starting ffmpeg process: exec: \"something-that-does-not-exist\": executable file not found")
+	})
+}