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.

Sign up for GitHub

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

Jump to bottom

obs-ffmpeg: NVENC additional settings. #1271

Closed
wants to merge 6 commits into from
Closed

obs-ffmpeg: NVENC additional settings. #1271

wants to merge 6 commits into from

Conversation

Dusre
Copy link

@Dusre Dusre commented Apr 23, 2018

Added missing features of nvenc.

@Fenrirthviti
Copy link
Member

Fenrirthviti commented Apr 23, 2018 •
edited

Looks like you have some formatting inconsistencies (you changed the indentation in plugins/obs-ffmpeg/obs-ffmpeg-nvenc.c L230-L260), and you'll want to squash those commits. Also, would recommend updating your title to something like "obs-ffmpeg: Add additional NVENC settings" to adhere to commit message guidelines.

For more info, see the contribution guidelines

EDIT: Also, as a personal note, I'm not sure that adding options that a user can unintentionally hurt themselves by enabling is a good idea. These should be checked and enabled/disabled programatically, or at the very least, issue warnings when set improperly.

@Dusre
Copy link
Author

Dusre commented Apr 23, 2018

Quote from the latest NVENC_VideoEncoder_API_ProgGuide.pdf:

Game-casting &
cloud transcoding
➢ High quality preset
➢ Rate control mode = CBR
➢ Medium VBV buffer size (1 second)
➢ B Frames*
➢ Look-ahead
➢ B frame as reference
➢ Finite GOP length (2 seconds)
➢ Adaptive quantization (AQ) enabled**
*: Recommended for low motion games and natural video.
**: Recommended on second generation Maxwell GPUs and above.

So i dont get why these would be recommended by nvidia them self if its bad.

@Fenrirthviti
Copy link
Member

Fenrirthviti commented Apr 23, 2018 •
edited

Sorry, maybe I wasn't quite clear on what I meant. Currently, you have the options translated as:

NVENC.noscenecut="No-Scenecut (When lookahead is enabled, enable this)"
NVENC.badapt="B-Adapt (When lookahead is enabled, disable this)"

This shouldn't be left to the user, IMO, if it's not recommended and can cause issues. There should be a programmatic check to enable/disable, or at the very least, a warning sent to the UI instead of putting this in the string of the option itself.

@Dusre
Copy link
Author

Dusre commented Apr 23, 2018

Should be better now.

if (rclookahead > 0)
{
av_opt_set_int(enc->context->priv_data, "b_adapt", false, 0);
av_opt_set_int(enc->context->priv_data, "no-scenecut", true, 0);
}

@h1z1
Copy link

h1z1 commented Apr 23, 2018 •
edited

Interesting. Only version of this guide I could find has no mention of the text above? Kind of odd they'd recommend CBR. Not saying it doesn't exist but it's not clear what version is being referenced.

@Dusre
Copy link
Author

Dusre commented Apr 24, 2018

Why would you use VBR for streaming? You can get the documents from here https://developer.nvidia.com/designworks/video_codec_sdk/downloads/v8.1

@h1z1
Copy link

h1z1 commented Apr 25, 2018 •
edited

Because it's technically better? CBR breaks down as you run out of bitrate bandwidth. Sites like Twitch recommend it because it helps THEM control traffic bursts and I don't think their old flash player handled ABR/VBR too well. Given that, should settings like this not be part of their specific service config?

Your link requires a login.

@RytoEX
Copy link
Member

RytoEX commented Apr 25, 2018 •
edited

@Dusre
Hi! Before I dig into this PR, is this thread also you? I'd just like to establish some background information first.

@h1z1
The newer versions of the NVIDIA SDK documentation and developer docs require an NVIDIA Developer Program account, which is a free signup (but yes, it is a required signup). Some of the older versions listed here do not require an account, but they obviously will not have information on newer NVIDIA features.

As for why NVIDIA recommends CBR for "Game-casting & cloud transcoding", I'd guess it's exactly because it helps transcoders (or services that transcode) handle traffic bursts better. To really know why NVIDIA made that recommendation, you'd have to ask NVIDIA.

@Dusre
Copy link
Author

Dusre commented Apr 25, 2018

Yes thats my post but i now decided to post here since no one was interested there.
Also VBR is configured wrong in the code example when you put 6k bitrate it can burst even 10k and then you gonna see dropped frames.
This example fixes that problem so it doesnt go over the max bitrate.

else if (astrcmpi(rc, "cbr") == 0) { /* CBR by default */
av_opt_set_int(enc->context->priv_data, "cbr", true, 0);
enc->context->rc_max_rate = bitrate * 1000;
enc->context->rc_min_rate = bitrate * 1000;
cqp = 0;
}
else if (astrcmpi(rc, "vbr") == 0) {
enc->context->rc_max_rate = bitrate * 1000;
cqp = 0;
}

@RytoEX
Copy link
Member

RytoEX commented Apr 25, 2018 •
edited

Okay then. It's not that there was no one interested, it's just that the video encoder flags/options can be very technical, and not everyone has the technical knowledge to be able to thoroughly understand them and how they should be implemented, myself included. For example, are Spatial AQ and Temporal AQ mutually exclusive, or can they both be enabled together? What does it mean to "force keyframes" such that forced-idr becomes an option? Why implement just these NVENC parameters and not others? What benefit does this grant end-users? What are some common pitfalls that users may find? What settings combinations should they pick? What settings combinations should they not pick?

You're working off of older FFmpeg default settings, so we're going to have to bring some of that up to date. I'll try to cover some of that in an upcoming code review.

RytoEX
RytoEX requested changes Apr 25, 2018
View reviewed changes
Copy link
Member

@RytoEX RytoEX left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Congrats on your first PR here and on GitHub!

I've reviewed this PR. I focused on code style compliance and default option values.

Additionally, please squash your commits into a single commit, and use a commit message appropriate for this project, such as:
obs-ffmpeg: Enable additional NVENC parameters

plugins/obs-ffmpeg/data/locale/en-US.ini
@@ -18,6 +18,12 @@ NVENC.Preset.ll="Low-Latency"
NVENC.Preset.llhq="Low-Latency High Quality"
NVENC.Preset.llhp="Low-Latency High Performance"
NVENC.Level="Level"
NVENC.SpatialStrength="Spatial AQ Strength"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably use "NVENC.SpatialAQ.Strength" for the string name and place it below "NVENC.SpatialAQ".

plugins/obs-ffmpeg/data/locale/en-US.ini
@@ -18,6 +18,12 @@ NVENC.Preset.ll="Low-Latency"
NVENC.Preset.llhq="Low-Latency High Quality"
NVENC.Preset.llhp="Low-Latency High Performance"
NVENC.Level="Level"
NVENC.SpatialStrength="Spatial AQ Strength"
NVENC.rclookahead="RC-Lookahead"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably use "NVENC.RCLookahead" here.

plugins/obs-ffmpeg/data/locale/en-US.ini
@@ -18,6 +18,12 @@ NVENC.Preset.ll="Low-Latency"
NVENC.Preset.llhq="Low-Latency High Quality"
NVENC.Preset.llhp="Low-Latency High Performance"
NVENC.Level="Level"
NVENC.SpatialStrength="Spatial AQ Strength"
NVENC.rclookahead="RC-Lookahead"
NVENC.surfaces="Surfaces"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "NVENC.Surfaces" here.

plugins/obs-ffmpeg/data/locale/en-US.ini
NVENC.surfaces="Surfaces"
NVENC.SpatialAQ="Use Spatial AQ"
NVENC.TemporalAQ="Use Temporal AQ"
NVENC.forcedidr="Forced-IDR"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "NVENC.ForcedIDR" here.

plugins/obs-ffmpeg/obs-ffmpeg-nvenc.c
@@ -139,6 +139,12 @@ static bool nvenc_update(void *data, obs_data_t *settings)
int gpu = (int)obs_data_get_int(settings, "gpu");
bool cbr_override = obs_data_get_bool(settings, "cbr");
int bf = (int)obs_data_get_int(settings, "bf");
int spstrength = (int)obs_data_get_int(settings, "spatialaqstrength");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to abbreviate "Spatial AQ Strength" for the variable here, perhaps "saqstrength" would be slightly more appropriate?

plugins/obs-ffmpeg/obs-ffmpeg-nvenc.c
0, 32, 1);
obs_properties_add_int(props, "surfaces",
obs_module_text("NVENC.surfaces"),
32, 64, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: This line can safely be at the end of the preceding line without exceeding the column limit.

plugins/obs-ffmpeg/obs-ffmpeg-nvenc.c
obs_module_text("NVENC.SpatialStrength"),
1, 15, 1);
obs_properties_add_int(props, "rclookahead",
obs_module_text("NVENC.rclookahead"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Tab in twice to indicate a line continuation.

plugins/obs-ffmpeg/obs-ffmpeg-nvenc.c
obs_module_text("NVENC.rclookahead"),
0, 32, 1);
obs_properties_add_int(props, "surfaces",
obs_module_text("NVENC.surfaces"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Tab in twice to indicate a line continuation.

plugins/obs-ffmpeg/obs-ffmpeg-nvenc.c
0, 32, 1);
obs_properties_add_int(props, "surfaces",
obs_module_text("NVENC.surfaces"),
32, 64, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The range for surfaces should be an integer from 0 to 64. These three numbers should be 0, 64, 1

plugins/obs-ffmpeg/obs-ffmpeg-nvenc.c
obs_data_set_default_bool(settings, "forcedidr", false);
obs_data_set_default_int(settings, "spatialaqstrength", 1);
obs_data_set_default_int(settings, "rclookahead", 0);
obs_data_set_default_int(settings, "surfaces", 32);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default setting for surfaces should be 0, according to FFmpeg.

@jp9000
Copy link
Member

jp9000 commented Apr 25, 2018

Hi there, thank you for the PR, however I have some disagreement with adding these specific settings directly as OBS settings. Instead, I would prefer an option similar to how x264 works for advanced settings (its "x264opts" setting), where you can use a settings string to specify additional settings that are available. I realize that this may be a bit frustrating, but I would prefer to respectfully reject it in favor of using a settings string instead.

Additionally, there are a few important notes about your pull request:

1.) You didn't follow the contribution guidelines. Please see RytoEX's review comments, and read the "Coding guidelines" and "Commit guidelines" sections of https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst
2.) You have multiple duplicate commits, and an unnecessary merge commit. This is usually an indicator of lack of git experience. I'm assuming you performed a merge rather than a rebase. You should have rebased to fix the commits of your pull request and then force pushed to your fork's branch associated with this pull request (although in this case you used your fork's master branch, which you typically shouldn't do with a pull request). I would also recommend taking the time to review our commit history, as that can give you a pretty quick understanding of how our commits and code normally look.

I will be willing to allow the additional NVENC settings, however, I definitely have to opt for an options string rather than have additional advanced parameters as properties.

I realize that having proposed changes can be frustrating -- please don't let that discourage you from submitting pull requests in the future.

@jp9000 jp9000 closed this Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RytoEX RytoEX RytoEX requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Dusre @Fenrirthviti @h1z1 @RytoEX @jp9000