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

Support for NVIDIA Image Scaling #488

Merged
merged 2 commits into from
May 18, 2022

Conversation

esullivan-nvidia
Copy link

@esullivan-nvidia esullivan-nvidia commented May 11, 2022

This PR adds support for selecting NVIDIA Image Scaling as the Gamescope upscaler implementation. It can be enabled by passing the -Y or --nis-upscaling option to Gamescope.

@emersion
Copy link
Collaborator

Maybe it would make sense to use a submodule or a Meson wrap instead of vendoring the SDK? Or maybe not, it's not that large.

@Martinligabue
Copy link

Martinligabue commented May 11, 2022

why is it removing some fsr support?
edit: I mean no problem if it works even on non-Nvidia graphic cards

@esullivan-nvidia
Copy link
Author

Maybe it would make sense to use a submodule or a Meson wrap instead of vendoring the SDK? Or maybe not, it's not that large.

I wanted to avoid pulling in the NIS examples directory which would just take up extra space and be unnecessary. Does Meson have a way to exclude some directories when it pulls in a dependency?

@esullivan-nvidia
Copy link
Author

why is it removing fsr support?

This change is not removing support for FSR. We are adding support for NIS as an upscaling option. This change will not impact any users who are currently using FSR in Gamescope.

@esullivan-nvidia esullivan-nvidia changed the title rendervulkan: Support for NVIDIA Image Scaling Support for NVIDIA Image Scaling May 11, 2022
@eli-schwartz
Copy link

I wanted to avoid pulling in the NIS examples directory which would just take up extra space and be unnecessary. Does Meson have a way to exclude some directories when it pulls in a dependency?

How would this work from the Meson side? I assume you would download https://github.com/NVIDIAGameWorks/NVIDIAImageScaling/archive/refs/tags/v1.0.2.tar.gz and then extract only the files you need... except that you have already gone and downloaded all 27 mb of that release tarball, which is exactly what you did not want. I don't think there is anything that any application can do about that.

Possible options include:

  • not caring about this at all
  • re-hosting a stripped tarball, which breaks the association with the upstream release
  • asking upstream to publish Github Releases archives of just the SDK
  • checking these files into git, but do so in subprojects/NVIDIAImageScaling/NIS/ instead of src/shaders/NVIDIAImageScaling/NIS/, which has the advantage of making it obvious to casual readers that this is imported wholesale from elsewhere rather than code specific to this project, and helps people trivially check the version from the independent project(), consult the SBOM, etc.

I would advise at a minimum doing option 4, option 3 would be the best if it can be arranged.

Copy link
Collaborator

@Joshua-Ashton Joshua-Ashton left a comment

Choose a reason for hiding this comment

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

Looks fine to me

@Joshua-Ashton
Copy link
Collaborator

@DadSchoorse maybe you want to take a look too?

@Joshua-Ashton
Copy link
Collaborator

(also needs rebasing, sorry :( )

Copy link
Collaborator

@DadSchoorse DadSchoorse left a comment

Choose a reason for hiding this comment

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

Generally it's fine for now, I would like to merge it soon-ish so I can integrate it in my bigger renderer rework.

src/rendervulkan.cpp Outdated Show resolved Hide resolved
Comment on lines 15 to 55
#if defined(NIS_SCALER)
layout(binding = 4) uniform const_buffer
{
float kDetectRatio;
float kDetectThres;
float kMinContrastRatio;
float kRatioNorm;

float kContrastBoost;
float kEps;
float kSharpStartY;
float kSharpScaleY;

float kSharpStrengthMin;
float kSharpStrengthScale;
float kSharpLimitMin;
float kSharpLimitScale;

float kScaleX;
float kScaleY;

float kDstNormX;
float kDstNormY;
float kSrcNormX;
float kSrcNormY;

uint kInputViewportOriginX;
uint kInputViewportOriginY;
uint kInputViewportWidth;
uint kInputViewportHeight;

uint kOutputViewportOriginX;
uint kOutputViewportOriginY;
uint kOutputViewportWidth;
uint kOutputViewportHeight;

float reserved0;
float reserved1;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is < 128 bytes, so just use push constants.

Copy link
Author

Choose a reason for hiding this comment

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

I think we would want to keep this as a uniform buffer to allow Gamescope to keep sharing a single layout between all of its pipelines without adding extra space to the push constant range size.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pretty sure we're using like 100 or so bytes of push constant anyway...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, our pipeline layout uses the full 128 byte range.

Copy link
Author

Choose a reason for hiding this comment

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

My bad, I should have double checked that. I can move this over to using push constants.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 54 to 18
layout(binding = 5) uniform sampler2D coef_scaler;
layout(binding = 6) uniform sampler2D coef_usm;
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use s_sampler_extra, and just add a second one.

src/rendervulkan.cpp Outdated Show resolved Hide resolved
src/rendervulkan.cpp Outdated Show resolved Hide resolved
Comment on lines 2485 to 2450
vkCmdCopyBufferToImage(commandBuffer, stagingBuffer, g_output.nisScalerImage, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, 1, &imageCopy);

imageCopy.bufferOffset = coef_scale_size;

vkCmdCopyBufferToImage(commandBuffer, stagingBuffer, g_output.nisUsmImage, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, 1, &imageCopy);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I would really like to reuse vulkan_create_texture_from_bits here, but I'll rework most of this in future anyway so it's fine for now.

The docs, and samples directories have been removed along with the
third_party_licenses.txt file.

Source downloaded from: https://github.com/NVIDIAGameWorks/NVIDIAImageScaling/archive/refs/tags/v1.0.2.tar.gz
@Joshua-Ashton Joshua-Ashton merged commit a515153 into ValveSoftware:master May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants