Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion media_processor/media_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,23 @@ func (conv *FFMpegMediaProcessor) Concatenate(ctx context.Context, filepaths []s
logAttrs = append(logAttrs, slog.String("resultFilepath", resultFilepath))
span.SetAttributes(attribute.String("result.filepath", resultFilepath))

args := []string{"-y", "-i", "concat:" + strings.Join(filepaths, "|"), "-acodec", audioCodec, resultFilepath}
args := []string{"-y", "-i", "concat:" + strings.Join(filepaths, "|"), "-acodec", audioCodec}

// When re-encoding (audioCodec != "copy"), probe the source bitrate and
// pass it to ffmpeg so the output matches the source quality. Without this,
// ffmpeg uses its default bitrate (128kbps for MP3) which can inflate the output.
if audioCodec != "copy" {
if bitrate, probeErr := conv.getAudioBitrate(filepaths[0]); probeErr == nil {
args = append(args, "-b:a", bitrate)
span.SetAttributes(attribute.String("source_bitrate", bitrate))
logAttrs = append(logAttrs, slog.String("sourceBitrate", bitrate))
} else {
conv.log.Warn("failed to probe source bitrate, using ffmpeg default",
append(logAttrs, slog.Any("error", probeErr))...)
}
}

args = append(args, resultFilepath)
cmd := exec.CommandContext(ctx, "ffmpeg", args...)
errCtx = errCtx.With("cmd", cmd.String())
logAttrs = append(logAttrs, slog.String("cmd", cmd.String()))
Expand All @@ -113,6 +129,29 @@ func (conv *FFMpegMediaProcessor) Concatenate(ctx context.Context, filepaths []s
return resultFilepath, nil
}

// getAudioBitrate probes the audio bitrate of the given file using ffprobe.
// Returns bitrate as a string like "128000" (bits per second).
func (conv *FFMpegMediaProcessor) getAudioBitrate(filepath string) (string, error) {
cmd := exec.Command(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use caller context when running bitrate probe

Concatenate is context-aware, but getAudioBitrate launches ffprobe with exec.Command instead of exec.CommandContext, so cancellation/timeouts are ignored during the new probe step. If ffprobe stalls (for example on problematic media on slow storage), the job can run past the flow timeout before it even starts ffmpeg, which is a regression introduced by this commit’s preflight probe.

Useful? React with 👍 / 👎.

"ffprobe",
"-v", "error",
"-select_streams", "a:0",
"-show_entries", "stream=bit_rate",
"-of", "default=noprint_wrappers=1:nokey=1",
filepath,
)
out, err := cmd.CombinedOutput()
if err != nil {
return "", fmt.Errorf("ffprobe failed: %w", err)
}

bitrate := strings.TrimSpace(string(out))
if bitrate == "" || bitrate == "N/A" {
return "", fmt.Errorf("ffprobe returned no bitrate for %s", filepath)
}
return bitrate, nil
}

func (conv *FFMpegMediaProcessor) ExtractCoverArt(filepath string) (coverArtFilePath string, err error) {
errCtx := oops.With("filepath", filepath)
coverArtFilePath = filepath + ".jpg"
Expand Down
58 changes: 58 additions & 0 deletions media_processor/media_processor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ package media_processor

import (
"context"
"fmt"
"io"
"log/slog"
"os"
"os/exec"
"path/filepath"
"testing"
"time"

Expand Down Expand Up @@ -228,3 +231,58 @@ func TestAddChapterTags_FileWithoutExistingTag(t *testing.T) {
}
}
}

// makeTestMP3 generates a short valid MP3 file with the given bitrate using FFmpeg.
func makeTestMP3(t *testing.T, duration float64, bitrate string) string {
t.Helper()
outFile := filepath.Join(t.TempDir(), "audio.mp3")
cmd := exec.Command("ffmpeg", "-y",
"-f", "lavfi", "-i", fmt.Sprintf("sine=frequency=440:duration=%g", duration),
"-ar", "44100", "-ac", "1", "-b:a", bitrate,
outFile,
)
if out, err := cmd.CombinedOutput(); err != nil {
t.Fatalf("ffmpeg failed to generate test MP3: %v\n%s", err, out)
}
return outFile
}

// TestConcatenate_OutputSizeNotLargerThanInputs verifies that concatenating
// MP3 files with re-encoding does not produce output larger than the sum of inputs.
// This reproduces the bug where ffmpeg used its default 128kbps bitrate instead of
// matching the source bitrate, inflating the output.
func TestConcatenate_OutputSizeNotLargerThanInputs(t *testing.T) {
// Create two MP3 files at 32kbps — a typical audiobook bitrate.
file1 := makeTestMP3(t, 1.0, "32k")
file2 := makeTestMP3(t, 1.0, "32k")

stat1, err := os.Stat(file1)
if err != nil {
t.Fatalf("stat file1: %v", err)
}
stat2, err := os.Stat(file2)
if err != nil {
t.Fatalf("stat file2: %v", err)
}
inputTotal := stat1.Size() + stat2.Size()

processor := &FFMpegMediaProcessor{log: testLogger}
resultPath, err := processor.Concatenate(context.Background(), []string{file1, file2}, "mp3")
if err != nil {
t.Fatalf("Concatenate failed: %v", err)
}
t.Cleanup(func() { _ = os.Remove(resultPath) })

resultStat, err := os.Stat(resultPath)
if err != nil {
t.Fatalf("stat result: %v", err)
}

// Allow 10% overhead for container/header differences, but no more.
maxExpected := int64(float64(inputTotal) * 1.10)
if resultStat.Size() > maxExpected {
t.Errorf("concatenated file is too large: result=%d bytes, inputs total=%d bytes (max allowed=%d). "+
"This suggests ffmpeg is re-encoding at a higher bitrate than the source files.",
resultStat.Size(), inputTotal, maxExpected)
}
}
Loading