fix: apply styling to Output Frames#746
fix: apply styling to Output Frames#746lawrence3699 wants to merge 1 commit intocharmbracelet:mainfrom
Conversation
Output Frames previously bypassed the ffmpeg styling pipeline by directly renaming the raw captured frames directory. This meant frames lacked padding, window bar, border radius, and margin that GIF/MP4/WebM outputs received. Process each frame through ffmpeg using the same screenshot filter pipeline so all output formats receive consistent styling. Fixes charmbracelet#258
There was a problem hiding this comment.
Pull request overview
This PR fixes Output frames/ to generate styled PNG frames by running captured frames through the same ffmpeg screenshot-style filter pipeline used for other outputs, rather than moving the raw capture directory.
Changes:
- Added
MakeFramesinvideo.goto render styled PNGs from raw text/cursor frame pairs via ffmpeg filters. - Wired frame rendering into
VHS.Render()soOutput frames/is produced alongside GIF/MP4/WebM/screenshots. - Removed the previous “rename raw frames directory to output” behavior from the evaluator cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| video.go | Adds MakeFrames that builds ffmpeg commands to produce styled PNG frame outputs. |
| vhs.go | Invokes MakeFrames during render so frame export runs as part of the render pipeline. |
| evaluator.go | Removes the old output-frames directory rename on cleanup (raw frames are no longer moved). |
Comments suppressed due to low confidence (1)
vhs.go:241
- Render currently ignores ffmpeg failures (it logs CombinedOutput but always returns nil). With MakeFrames added, it’s easy for frame export to fail or be partially generated without surfacing an error to the caller. Consider returning an error when any ffmpeg command fails (or at least for frame export) so CI/users can reliably detect failures.
cmds = append(cmds, MakeFrames(vhs.Options.Video, vhs.totalFrames)...)
for _, cmd := range cmds {
if cmd == nil {
continue
}
out, err := cmd.CombinedOutput()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if opts.Output.Frames == "" { | ||
| return nil | ||
| } | ||
|
|
||
| log.Println(GrayStyle.Render("Creating " + opts.Output.Frames + "...")) | ||
| ensureDir(filepath.Join(opts.Output.Frames, "frame.png")) | ||
|
|
||
| var cmds []*exec.Cmd | ||
| for i := 0; i < totalFrames; i++ { | ||
| frameNum := opts.StartingFrame + i | ||
| textStream := filepath.Join(opts.Input, fmt.Sprintf(textFrameFormat, frameNum)) | ||
| cursorStream := filepath.Join(opts.Input, fmt.Sprintf(cursorFrameFormat, frameNum)) | ||
|
|
||
| if _, err := os.Stat(textStream); err != nil { | ||
| continue | ||
| } | ||
|
|
||
| targetFile := filepath.Join(opts.Output.Frames, fmt.Sprintf("frame-%05d.png", i+1)) | ||
|
|
||
| streamCounter := 2 | ||
| streamBuilder := NewStreamBuilder(streamCounter, opts.Input, opts.Style) | ||
| streamBuilder.args = append(streamBuilder.args, | ||
| "-y", | ||
| "-i", textStream, | ||
| "-i", cursorStream, | ||
| ) | ||
|
|
||
| streamBuilder = streamBuilder. | ||
| WithMargin(). | ||
| WithBar(). | ||
| WithCorner() | ||
|
|
||
| filterBuilder := NewScreenshotFilterComplexBuilder(opts.Style). | ||
| WithWindowBar(streamBuilder.barStream). | ||
| WithBorderRadius(streamBuilder.cornerStream). | ||
| WithMarginFill(streamBuilder.marginStream) | ||
|
|
||
| var args []string | ||
| args = append(args, streamBuilder.Build()...) | ||
| args = append(args, filterBuilder.Build()...) | ||
| args = append(args, targetFile) | ||
|
|
||
| cmds = append(cmds, exec.Command("ffmpeg", args...)) //nolint:gosec | ||
| } | ||
|
|
||
| return cmds |
There was a problem hiding this comment.
MakeFrames spawns a separate ffmpeg process per frame. For even moderate recordings (e.g., 50fps * N seconds) this can mean hundreds/thousands of ffmpeg invocations, which is likely to be very slow and operationally expensive compared to processing the image sequences in a single ffmpeg run (two image2 inputs + the existing screenshot filter_complex + an output pattern). Consider switching to a single ffmpeg command that outputs a PNG sequence to avoid the per-frame process overhead.
| if opts.Output.Frames == "" { | |
| return nil | |
| } | |
| log.Println(GrayStyle.Render("Creating " + opts.Output.Frames + "...")) | |
| ensureDir(filepath.Join(opts.Output.Frames, "frame.png")) | |
| var cmds []*exec.Cmd | |
| for i := 0; i < totalFrames; i++ { | |
| frameNum := opts.StartingFrame + i | |
| textStream := filepath.Join(opts.Input, fmt.Sprintf(textFrameFormat, frameNum)) | |
| cursorStream := filepath.Join(opts.Input, fmt.Sprintf(cursorFrameFormat, frameNum)) | |
| if _, err := os.Stat(textStream); err != nil { | |
| continue | |
| } | |
| targetFile := filepath.Join(opts.Output.Frames, fmt.Sprintf("frame-%05d.png", i+1)) | |
| streamCounter := 2 | |
| streamBuilder := NewStreamBuilder(streamCounter, opts.Input, opts.Style) | |
| streamBuilder.args = append(streamBuilder.args, | |
| "-y", | |
| "-i", textStream, | |
| "-i", cursorStream, | |
| ) | |
| streamBuilder = streamBuilder. | |
| WithMargin(). | |
| WithBar(). | |
| WithCorner() | |
| filterBuilder := NewScreenshotFilterComplexBuilder(opts.Style). | |
| WithWindowBar(streamBuilder.barStream). | |
| WithBorderRadius(streamBuilder.cornerStream). | |
| WithMarginFill(streamBuilder.marginStream) | |
| var args []string | |
| args = append(args, streamBuilder.Build()...) | |
| args = append(args, filterBuilder.Build()...) | |
| args = append(args, targetFile) | |
| cmds = append(cmds, exec.Command("ffmpeg", args...)) //nolint:gosec | |
| } | |
| return cmds | |
| if opts.Output.Frames == "" || totalFrames <= 0 { | |
| return nil | |
| } | |
| log.Println(GrayStyle.Render("Creating " + opts.Output.Frames + "...")) | |
| ensureDir(filepath.Join(opts.Output.Frames, "frame.png")) | |
| textStream := filepath.Join(opts.Input, textFrameFormat) | |
| cursorStream := filepath.Join(opts.Input, cursorFrameFormat) | |
| firstTextFrame := filepath.Join(opts.Input, fmt.Sprintf(textFrameFormat, opts.StartingFrame)) | |
| if _, err := os.Stat(firstTextFrame); err != nil { | |
| return nil | |
| } | |
| targetFile := filepath.Join(opts.Output.Frames, "frame-%05d.png") | |
| streamCounter := 2 | |
| streamBuilder := NewStreamBuilder(streamCounter, opts.Input, opts.Style) | |
| streamBuilder.args = append(streamBuilder.args, | |
| "-y", | |
| "-start_number", fmt.Sprintf("%d", opts.StartingFrame), | |
| "-i", textStream, | |
| "-start_number", fmt.Sprintf("%d", opts.StartingFrame), | |
| "-i", cursorStream, | |
| ) | |
| streamBuilder = streamBuilder. | |
| WithMargin(). | |
| WithBar(). | |
| WithCorner() | |
| filterBuilder := NewScreenshotFilterComplexBuilder(opts.Style). | |
| WithWindowBar(streamBuilder.barStream). | |
| WithBorderRadius(streamBuilder.cornerStream). | |
| WithMarginFill(streamBuilder.marginStream) | |
| var args []string | |
| args = append(args, streamBuilder.Build()...) | |
| args = append(args, filterBuilder.Build()...) | |
| args = append(args, "-frames:v", fmt.Sprintf("%d", totalFrames)) | |
| args = append(args, targetFile) | |
| return []*exec.Cmd{exec.Command("ffmpeg", args...)} //nolint:gosec |
| streamBuilder = streamBuilder. | ||
| WithMargin(). | ||
| WithBar(). | ||
| WithCorner() | ||
|
|
There was a problem hiding this comment.
Within the per-frame loop, WithBar() / WithCorner() will regenerate bar.png and mask.png on every iteration (via MakeWindowBar/MakeBorderRadiusMask), rewriting the same files repeatedly. This is unnecessary work and can significantly slow down frame export. Consider generating these assets once per render (or caching them) and reusing them across all frame exports.
| return nil | ||
| } | ||
|
|
||
| log.Println(GrayStyle.Render("Creating " + opts.Output.Frames + "...")) |
There was a problem hiding this comment.
The output frames directory is created but never cleaned before writing. If the directory already exists from a previous run, stale frames beyond the current totalFrames can remain and make the output ambiguous. Consider removing/emptying the directory (or writing to a temp dir and swapping) before generating the new PNG sequence.
| log.Println(GrayStyle.Render("Creating " + opts.Output.Frames + "...")) | |
| log.Println(GrayStyle.Render("Creating " + opts.Output.Frames + "...")) | |
| if err := os.RemoveAll(opts.Output.Frames); err != nil { | |
| log.Fatal(err) | |
| } |
| streamCounter := 2 | ||
| streamBuilder := NewStreamBuilder(streamCounter, opts.Input, opts.Style) | ||
| streamBuilder.args = append(streamBuilder.args, | ||
| "-y", | ||
| "-i", textStream, | ||
| "-i", cursorStream, | ||
| ) | ||
|
|
||
| streamBuilder = streamBuilder. | ||
| WithMargin(). | ||
| WithBar(). | ||
| WithCorner() | ||
|
|
||
| filterBuilder := NewScreenshotFilterComplexBuilder(opts.Style). | ||
| WithWindowBar(streamBuilder.barStream). | ||
| WithBorderRadius(streamBuilder.cornerStream). | ||
| WithMarginFill(streamBuilder.marginStream) | ||
|
|
There was a problem hiding this comment.
MakeFrames duplicates the same ffmpeg arg construction as ScreenshotOptions.buildFFopts (StreamBuilder + NewScreenshotFilterComplexBuilder + WithMargin/WithBar/WithCorner). This duplication increases the risk of the screenshot and frame pipelines drifting over time. Consider extracting a shared helper for building the screenshot-style ffmpeg args so both code paths stay consistent.
Output Framespreviously bypassed the ffmpeg styling pipeline by directly renaming the raw captured frames directory to the output path. This meant frames lacked padding, window bar, border radius, and margin styling that GIF/MP4/WebM outputs received.This processes each frame through ffmpeg using the same screenshot filter pipeline (
NewScreenshotFilterComplexBuilder+StreamBuilder) so all output formats receive consistent styling.Before:
Output frames/produces raw terminal canvas screenshots with no styling applied.After:
Output frames/produces styled frames with padding, window bar, border radius, and margin — matching GIF/MP4/WebM output.Fixes #258
Validation:
go build ./...passesgo test ./...passes (all packages)go vet ./...cleanI have read
CONTRIBUTING.md.