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

Nearest Neighbour filtering #1486

Closed
wants to merge 2 commits into from

Conversation

RomaRogov
Copy link

Description

Hello!
I've implemented new lever in config to control resize algorithm for textures with captured screen. This allows to get high-resolution pixel perfect stream video for some retro games or just low resolution which can be doubled.
For example, I'm using this with my Abernic RG505 device which has PlayStation Vita screen (960x544px), which has artifacts when stream has native resolution. But if I double stream resolution with nearest neighbour scaling I'm getting pixel-perfect image without any compression artifacts. May be also useful for PS Vita users.
Also now I can play retro games with 1080p streaming on high resolution devices and they are not blurry at all. So, this lever, as I think, definitely has a few applications.
I'm not very used to C++, so please give feedback if my changes should be refactored before merge. Thanks in advance!

Screenshot

Screenshot_20230731-150626_Moonlight (Debug)

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2023

CLA assistant check
All committers have signed the CLA.

@ns6089
Copy link
Contributor

ns6089 commented Jul 31, 2023

So you want host-side nearest-neighbour upscaler? Then maybe it's better to simply use dedicated game upscaler like https://github.com/Blinue/Magpie. Adding it as processing option is basically a work multiplier, since we have to implement (and support) it across every platform and capture method. Just my two cents.

@RomaRogov
Copy link
Author

Yes, thank you for the hint! Maybe it's actually better solution.
Option which I've offered just was looking simple and effective enough to be a part of configuration. So, of course - it's up to you, but on platform-side it's pretty simple to maintain (just selection between linear and point in texture filter type).

@ns6089
Copy link
Contributor

ns6089 commented Jul 31, 2023

just selection between linear and point in texture filter type

If we stick to texture sampler scaling, which already doesn't work that well (any amount of image downscaling produces chroma artifacts because paired with 4:2:0 subsampling it results in >2x downscaling ratio). And with kernel-base scaling, nearest-neighbour will require different kernel of different size. It's certainly possible, but it will require more work, probably sooner than later.

@RomaRogov
Copy link
Author

Oh, well, I see - things are actually more complicated and not so universal to be public change, though it works good for my case.
Maybe authors just can consider such kind of feature in the future, who knows :)

@psyke83
Copy link
Collaborator

psyke83 commented Oct 25, 2023

Perhaps this idea might have still have merit in the specific case where the client resolution is evenly divisible by the host resolution? Here's a quick proof of concept: nightly...psyke83:nearest_scaling

This should improve clarity when the host and client resolution are matched, but there is additional benefit when the client resolution is a multiple of the host.

Consider my use case:

  • Client machine is a laptop with a 1360x768 screen.
  • Host machine is using a HDMI EDID emulator, so resolution is flexible (for the purpose of supersampling).

Streaming Moonlight at 1360x768 with current Sunshine nightly:

  • Host at 1360x768 looks ok (no aliasing artifacts), but is slightly blurry.
  • Standard host resolutions (1080p, 1440p, 4K) have aliasing artifacts and look somewhat blurry
  • 2720x1536 (2x client panel res) has noticeable aliasing artifacts and still looks blurrier than 1360x768.

Streaming Moonlight at 2720x1536 (2x panel res) with current Sunshine nightly:

  • Host at 1360x768 has no noticeable aliasing artifacts, but looks very blurry
  • Standard host resolutions (1080p, 1440p, 4K) has noticeable (but less severe) aliasing artifacts, yet still looks quite blurry
  • 2720x1536 has little to no aliasing artifacts, but still looks blurrier than expected.

Streaming Moonlight at 1360x768 with nearest scaling branch:

  • Host at 1360x768 looks much sharper.
  • All other resolutions are unchanged compared to nightly.

Streaming Moonlight at 2720x1536 (2x panel res) with nearest scaling branch:

  • Host at 1360x768 looks much sharper.
  • Host at 2720x1536 looks excellent (no aliasing artifacts or blurring) <- this is now my preferred streaming setup
  • All other resolutions unchanged compared to nightly

Granted this may be an odd use case, but I can envison the same benefit for standard resolutions (1080p panel with Moonlight @ 1080p to 1080p host, as well as 1080p panel with Moonlight @ 4K to 4K or 1080p host, etc.).

@ReenigneArcher
Copy link
Member

@psyke83 thanks for the details. It seems there are definitely some possible benefits for image quality, with a potential for a small performance hit. If this is an option that can be disabled, then I see little harm in adding it.

@psyke83
Copy link
Collaborator

psyke83 commented Oct 26, 2023

Nearest neighbour filtering should involve less overhead than bilinear filtering, but it's unlikely to cause any real performance change. The important part to stress is that having a simple on/off toggle as seen in this PR is not optimal.

To illustrate, looking at my setup, enabling "point filtering" from this PR would do the following:

  • Moonlight @ 1360x768: host at 1360x768 looks good, all other resolutions will have scaling artifacts.
  • Moonlight @ 2720x1536: host resolutions 1360x768 and 2720x1536 look good, but all others will have scaling artifacts.

I know this is a niche setup, but the behaviour will replicate for standard resolutions (1080P and 4K, 1440P and 720P resolutions pairs) as well as OP's 960x544 & doubled resolution. The only sensible user configuration (if at all needed) would be an Auto/Off toggle, where Auto will only use nearest filtering when the client resolution is divisible by the host (i.e. my example branch) and Off to preserve the old behaviour for users that prefer the softer look provided by bilinear filtering for all resolutions.

@ns6089
Copy link
Contributor

ns6089 commented Oct 29, 2023

@psyke83 Our chroma shader relies on linear texture sampler for some math / texture fetch optimizations.
I've explained it here #1621 (comment).

@ReenigneArcher ReenigneArcher requested a review from cgutman January 1, 2024 03:14
@LizardByte-bot
Copy link
Member

It looks like this PR has been idle for 90 days. If it's still something you're working on or would like to pursue, please leave a comment or update your branch. Otherwise, we'll be closing this PR in 10 days to reduce our backlog. Thanks!

@ReenigneArcher
Copy link
Member

@RomaRogov Thank you for the PR. Unfortunately at this time, we have decided not to accept this change. In order to re-consider this PR the comments by @ns6089 would need to be addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants