Repository navigation
feat(presets): video size limit in MB or as a percentage of the original file - #883
Conversation
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks @gianabif14, this is a clean piece of work: intermediate steps drop the limit, CanCopyVideo honours %, the refusal happens before encoding, and the FFmpeg tests use real files. A few things before it goes in.
1. Switching MB β % can turn a large MB value into 99 %
PresetEditorViewModel.OnVideoSizeUnitChoiceChanged raises VideoSizeMaximum (4096 β 99) before VideoMaxSize.
- At that moment the box still shows the MB value. With 500 MB, the
NumericUpDowncuts it to its new maximum, 99. - It writes 99 back through the
VideoMaxSizesetter, which already routes toVideoMaxSizePercent. The preset would save 99 % although the user never typed it.
That is the case the image units guard against in OnImageSizeUnitChanged: "The boxes' maximum is raised before the values grow and lowered after they shrink, so a box never cuts a value to the other unit's maximum." The same order would fix it here:
- going to %: notify
VideoMaxSizefirst, thenVideoSizeMaximum; - going to MB: notify
VideoSizeMaximumfirst, thenVideoMaxSize.
Video_size_unit_switch_keeps_each_value_separately starts from 25 MB, which is under 99, so it can't see this. Please add a case with 100 MB or more.
2. A % limit is ignored for joined videos
- Merge videos checks the result with
SizeLimit.Check(job.Preset, [output])(JobQueue.cs, around line 542), without a source path. ForVideo(media, sourcePath: null)then falls back toMaxSizeMb, whichApplyset to 0. So a merged video gets no limit at all, silently.
Either take the % of all joined sources together, or refuse % for joins with a clear message. Both are fine; silently ignoring it isn't.
3. A missing source also means no limit
SizeLimit.ForVideo(media, string? sourcePath) returns 0 when the source file isn't there. A % that can't be measured should rather be refused (error.over_size_limit or a short new message) than turn into "no limit".
4. Small things
presets.max_size("Size limit (MB)") isn't used anywhere now, but it's still in all 19 language files. Please remove it.
Once 1β3 are in, I'm happy to merge.
β¦oins - Prevent large MB values from clamping to 99% when switching unit - Enforce percentage size limit on joined videos based on combined source size - Refuse unmeasurable percentage limits when source file is missing instead of ignoring - Add unit tests for unit switching, combined sources, and missing source handling
5162952 to
ca484bc
Compare
|
Thanks for the review, @KnifeLemon! I've pushed the fixes for all the items you mentioned (1-4). Let me know if it needs any further adjustments |
KnifeLemon
left a comment
There was a problem hiding this comment.
Thanks @gianabif14, all four points are in:
- Unit switch: the box's maximum now moves on the right side of the value, and the 500 MB case covers it.
- Joined videos:
%is taken of the joined sources together. - Missing source: a
%that can't be measured is refused instead of becoming "no limit". presets.max_sizeis gone from all 19 files.
CI is green. Merging. π
A video preset's Size limit can now be specified either in MB (as before) or as a % of the original file (1β99%).
MediaOptions.MaxSizePercent(default0 = off). Existing presets remain completely unchanged without needing a settings migration step. Only one limit is active at a time.MediaEncoding.VideoTargets), not audio or GIF.FfmpegConverterandMediaEncoding.ForSizeLimitcapped-bitrate encoding and fit-again mechanism; impossible percentages are refused before encoding starts witherror.over_size_limit.presets.size_limitacross all 19i18n/*.jsoncatalogs.docs/ENGINES.md.Closes #680
Screenshots