Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

derrod
Copy link
Member

@derrod derrod commented Dec 13, 2024

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

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@derrod derrod force-pushed the nvenc-hard-fail branch 4 times, most recently from 386ec83 to e18007e Compare December 13, 2024 03:45
@Lain-B
Copy link
Collaborator

Lain-B commented Dec 14, 2024

Could you specify more context? What happened exactly?

Copy link
Collaborator

@Lain-B Lain-B left a 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.

@derrod
Copy link
Member Author

derrod commented Dec 14, 2024

Could you specify more context? What happened exactly?

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.

@WizardCM WizardCM added the Bug Fix Non-breaking change which fixes an issue label Dec 14, 2024
@DeeDeeG
Copy link

DeeDeeG commented Dec 16, 2024

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, averageBitRate=someNonsenseString (should be a number instead) and the encode session appears to fall back to some default bitrate ~40Mbit/sec instead of complaining (at least not in any way surfaced to me as an OBS user?? if the driver surfaces info about the wrong value at all I'm not sure?). (This much is the same with or without this PR.)

Second example: I can set lookaheadDepth=9999999999999999999999 and I don't see any indication of what actual # of frames of lookahead the encode session is really using, as the 9999999999999999999999 figure (or whatever still ridiculously large and implausible value it gets clamped to) is logged.

What's new with this PR, (for me in testing) is that if I try to do a nonexistent custom option (for example customOptionThatDoesNotExist=12) I do get an error popup ("Invalid Custom Encoder options [ . . .]"), with the specific nonexistent option logged in OBS's logs, and the recording does not proceed. Without this PR OBS would silently ignore any made up, non-existent (or typo'd) option (the Nvidia driver ignores the invalid params too, I guess?) and without this PR OBS would proceed with the recording without any popups or a log specifically indicating an invalid param. At least outside of some debug logging that's off by default IIRC.

(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 blog(LOG_DEBUG)), but maybe I am more interested in the fine details of this "for science" than most people? But it's some of the few clues I have as to how my custom options are even being noticed by OBS, even if the driver isn't surfacing much about what really happens after that, at least in my testing with the current implementation in OBS. EDIT: Although, as of this PR, one can assume any options they provide are applied to the encode session in some way shape or form if the error popup does not show, as it implies the parameter name was valid. No guarantee the driver honors rather than ignoring the particular value, though. So I dunno how useful logging the exact value the user entered is now? 🤷)

Some options do appear to return more specific and helpful error messages when set "wrong" (regardless of this PR)... I can set level=99 and I get a popup message saying "NVENC Error: Invalid level. (NV_ENC_ERR_INVALID_PARAM)". But other than that, for the most part, the driver and/or this implementation doesn't seem to validate most values as being appropriate/usable, it only seems to let you know (as of this PR) when the option names themselves are set to something invalid/not a real option the driver knows about.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants