fix: render on a live context so the encoders are not killed at Start - #791
Open
blackswanalpha wants to merge 1 commit into
Open
blackswanalpha wants to merge 1 commit into
blackswanalpha wants to merge 1 commit into
Conversation
Evaluate cancelled the recording context in teardown and then handed that same context to Render. Since the encoders are built with exec.CommandContext, every ffmpeg invocation was killed at Start and wrote nothing, while Render logged the empty output and returned nil. VHS printed "Creating out.gif...", exited 0, and produced no file. Give the recorder its own context and render on the parent, and return the encode error instead of swallowing it. Fixes charmbracelet#787
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #787.
What happens
v0.12.0 runs a tape to completion, prints
Creating out.gif..., prints the publish hint, exits 0, and writes no file. The gap between the two printed lines is a few hundred milliseconds, which is the tell: the encode never runs.Why
Evaluatecancels the recording context inteardown, then hands that same context toRender:makeMediabuilds each encoder withexec.CommandContext, and a command started on a context that is already done is killed atStartbefore it writes anything. Every ffmpeg invocation dies immediately.The failure is silent because
Renderlogs the output and drops the error:CombinedOutputon a killed command returns empty, so the log prints a blank line andEvaluateseesnil. That is the whole visible symptom: two blank lines, exit 0.Why v0.11.0 is unaffected
The context was threaded through this path in v0.12.0. In v0.11.0 the signatures are
func (vhs *VHS) Render() errorandfunc makeMedia(opts VideoOptions, targetFile string) *exec.Cmd— the encoders used plainexec.Commandand nothing could cancel them. The cancellation inteardownis not new; carrying it into the encoders is.The fix
Two changes, both small:
evaluator.go— the recorder gets its own context.teardownstill cancels it to stop frame capture, andRenderruns on the parent, which is still live.vhs.go—Renderreturns the encode error instead of logging and returningnil, so a failure surfaces as a non-zero exit rather than a silent empty run.The second change is not strictly needed to make rendering work, but it is why this took so long to pin down from the outside, and it keeps the next encode failure from looking like success.
Verification
TestRenderReportsEncodeFailureis added. It fails onmain(Renderreturnsnil) and passes with the fix.Output min.gif+Type "echo hello"produced no file onmain, and produces a valid 600x200, 63-frame GIF with the fix.gofmt,go vetandgo test -short ./...are clean.Nothing in the suite currently renders a tape and asserts the file exists —
browser_e2e_test.gocoversstartBrowseronly and is opt-in behindVHS_TEST_BROWSER=1— which is why this reached a release. The added test locks the error path; catching the context bug itself would need an end-to-end render in CI, which is a larger change than this PR should carry.Environment