Allow extra HandBrakeCLI arguments via config - #14
Open
jakobwesthoff wants to merge 3 commits into
Open
jakobwesthoff wants to merge 3 commits into
jakobwesthoff wants to merge 3 commits into
Conversation
The argument list was assembled inline in encode(), immediately before the exec.Cmd it feeds, which left no way to assert on it without running HandBrakeCLI. Pulling it into a plain function makes the resulting arguments testable.
A HandBrake preset can only select audio tracks by language and by "first" or "all", and applies a single audio rule to every track it selects. Discs carrying several tracks in the same language cannot be handled by a preset alone. The arguments are appended after the preset, relying on HandBrakeCLI applying an imported preset first and letting later flags override it, so a preset can still govern video while these take over track selection. Arguments handymkv derives itself are rejected rather than merged. Accepting them would let a config silently redirect an encode away from the staged input or the configured output directory. The setting is not offered by the configuration wizard, since promptForStringSlice splits on commas and HandBrakeCLI values such as `-a 1,2` contain them.
The encode parameters were assembled field by field from the config, so a setting could be read from `config.json`, printed by the `config` subcommand, and still never reach HandBrakeCLI. `extra_handbrake_args` was dropped this way, and so were `include_all_relevant_audio` and `include_all_relevant_subtitles`, which the wizard has always prompted for but which never resulted in `--all-audio` or `--all-subtitles` being passed. The configured settings already are an `EncodingParams`, so they are now copied wholesale and only the per-title fields are overwritten. A setting added to the struct is carried without having to be listed in a second place.
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
1 participant
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.
With a custom preset configured, HandyMKV runs HandBrakeCLI with only
--input,--output,--preset-import-fileand--preset. Everything else has to come from the preset, and some things simply cannot.A preset picks audio tracks by language only, as either "first" or "all". It cannot pick a track by index, and it applies one audio rule to every track it selects. So a disc with several tracks in the same language leaves you choosing between dropping tracks you want and keeping ones you don't.
I hit this on a Blu-ray with three English tracks: an LPCM mono original, a DTS-HD MA 5.1 remix, and the lossy DTS core of that same remix. "first" silently threw away the 5.1. "all" kept the redundant core and forced one bitrate onto both a 2-channel and a 6-channel track.
A new optional
extra_handbrake_argssetting:The arguments go after the preset, so HandBrakeCLI's own override rules apply: the preset still governs video, while these take over audio. The example keeps source tracks 1 and 2 at 160 and 640 kb/s and ignores the rest.
Arguments HandyMKV sets itself are rejected at config load rather than merged:
--input,-i,--output,-o,--preset,--preset-import-file. Letting a config override those would quietly redirect an encode away from the staged input or the configured output directory.The wizard does not prompt for the setting, since
promptForStringSlicesplits on commas and values like-a 1,2contain them. It has to be added toconfig.jsonby hand.Track indexes refer to the ripped MKV, not the disc, and only hold while every title shares a stream layout. Noted in the README.