Fix #13401: target file size with VideoToolbox burn-in - #13402
Merged
Conversation
Burn-in always ran a two-pass encode when "target file size" was set. That does not work with the VideoToolbox encoders added in 1520f00: ffmpeg accepts -pass for them, but the encoder writes no statistics, so pass 1 is pure wasted time - it doubles the encode for nothing. Verified locally against ffmpeg 4.4.6 / h264_videotoolbox on Apple silicon: a "-pass 1" run completes without error and leaves ffmpeg2pass-0.log at 0 bytes, and pass 2 produces exactly the same output as a plain single-pass encode at the same bit rate. VideoToolbox target-size encodes now run a single pass with an average bit rate (-b:v), computed by the same GetVideoBitRate the two-pass path uses. The bit-rate-too-low check moved into SetTargetBitRate so both paths share it. The other half is a silent-corruption trap, also from the same measurement: with both -q:v and -b:v on the command line ffmpeg keeps the quality target and ignores the bit rate, with no warning. "-q:v 50 -b:v 800k" produced a byte-identical file to "-q:v 50" alone (56196 bytes), where "-b:v 800k" alone produced 163475 - so a user asking for a file size would silently get whatever size the quality setting happened to give. FfmpegGenerator now drops the quality flag whenever a bit rate target is present. With -pass this was already implicit; it is only reachable now that a bit rate can appear without a pass. Two-pass for every other encoder is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the fast reply. I agree with the fix plan for "target file size" with VideoToolbox burn-in. About the optional flags for bitrate-driven encoding ( |
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 #13401. Thanks @luisblop — both remarks check out, and I measured them rather than taking them on trust.
Measurements
ffmpeg 4.4.6,
h264_videotoolbox, Apple silicon, 4 s test clip.1.
-q:vsilently beats-b:v. This is the nastier of the two — there is no error and no warning:-q:v 50 -b:v 800k-q:v 50alone-b:v 800kaloneSo if a quality flag were ever emitted alongside the bit rate, a user asking for a target file size would silently get whatever size the quality setting happened to produce.
2. Two-pass is a no-op, not an error.
-pass 1completes fine and leavesffmpeg2pass-0.logat 0 bytes — the encoder writes no statistics. Pass 2 then produces exactly the same output as a plain single-pass encode at the same bit rate. So it is not a hard failure; it just doubles the encode time for nothing.What was actually broken
BurnInViewModelrouted every "target file size" job throughRunTwoPassEncoding, regardless of encoder. On macOS with VideoToolbox that meant a wasted analyze pass on every burn-in.The
-q:v/-b:vconflict was not reachable before this change, becausecrfSettingswas gated onpassbeing empty and target-size always set a pass. It becomes reachable the moment a bit rate can appear without a pass — which is exactly what the fix introduces — so it is handled here too.Fix
BurnInViewModel: VideoToolbox target-size jobs run a single pass with-b:v, using the sameGetVideoBitRatethe two-pass path already used. The bit-rate-too-low guard moved into a sharedSetTargetBitRateso both paths keep it.FfmpegGenerator: emits-b:v <rate>with no-passwhen a bit rate is set and no pass is, and drops the quality flag whenever a bit rate target is present.I did not add
-maxrate/-bufsize. You listed them as optional, and they change the rate-control shape (VBV constraint rather than plain ABR), so they felt like a separate, opinionated call rather than part of a bug fix. Easy to add if you want them.Test plan
dotnet test tests/UI/UITests.csproj— 1787 passed (15 new)FfmpegTargetFileSizeTestspins:-b:vwithout-pass, quality dropped when a bit rate is present, quality kept when it is not, two-pass unchanged for other encoders, and the encoder-detection predicatedotnet build src/ui/UI.csproj -c Release🤖 Generated with Claude Code