-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
obs-nvenc: Abort encoder init if custom options are invalid #11625
base: master
Are you sure you want to change the base?
Conversation
386ec83
to
e18007e
Compare
Could you specify more context? What happened exactly? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, but would be nice to have a little more context just so we have a little bit of a record of how this change/commit came about.
Users trying random settings that either don't do anything or are invalid. Feels like we should error out in those cases and prevent them from shooting themselves in the foot. |
e18007e
to
458408a
Compare
I have some notes I can add from testing, as to how this option checking behaves: Details (click to expand if interested, hidden to not be too distracting):(Bolded key info for easier skimming) OBS (for me anyway testing this implementation) doesn't complain much when setting real custom options with a wrong value. I can set, for example, Second example: I can set What's new with this PR, (for me in testing) is that if I try to do a nonexistent custom option (for example (An aside: I personally would like the logging for when a custom option is actually being applied to be logged outside of just debugging situations (i.e. different log level than Some options do appear to return more specific and helpful error messages when set "wrong" (regardless of this PR)... I can set FWIW I am using Studio Driver 561.09, on an RTX 3070. tl;dr at least for me this change doesn't surface if a custom option's set value is wrong, for the majority of options. This PR does have OBS show an error popup for any completely made up (non-existent, typo'd) custom option names, adds a line to logs of the specific option that was not a real option name, and does not "record anyway." Unlike before where even nonexistent custom option [names] would be silently ignored. But for me anyway, a lot of wacky values to real option names are still silently accepted, even if the driver seems to ignore them or fallback to some default values. |
Description
Makes invalid user options a hard failure.
Motivation and Context
Users are being silly.
How Has This Been Tested?
Put in some invalid options, observed failure.
Types of changes
Checklist: