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

Add v4 effects from the port of mpv-prescalers #806

Merged
merged 13 commits into from
Jan 22, 2024

Conversation

hauuau
Copy link
Contributor

@hauuau hauuau commented Jan 10, 2024

This is generated effects from the port of mpv-prescalers adapted to MagpieFX v4.

The source code for generation is in the source-magpie-v4 branch

@Blinue
Copy link
Owner

Blinue commented Jan 10, 2024

Thank you for your contribution! I will primarily review this pull request, which will become a new feature in v1.0. If you don't want to maintain two versions, feel free to close #805, as there are no plans to add new features to v0.10.

Close #554 according to #803 (comment)

@Blinue
Copy link
Owner

Blinue commented Jan 10, 2024

I tried a few simple ones, RAVU worked well, but NNEDI3 didn't. Here’s a screenshot:
屏幕截图(68)

@hauuau
Copy link
Contributor Author

hauuau commented Jan 10, 2024

Do you have any more information which might help to trigger it?

Every NNEDI3 works for me on AMD hardware with 5c007a1 build of Magpie.

@hauuau
Copy link
Contributor Author

hauuau commented Jan 10, 2024

I have managed to trigger it on Intel hardware. I'll try investigating.

@hauuau
Copy link
Contributor Author

hauuau commented Jan 12, 2024

This should hopefully fix NNEDI3.
And I've also synced RAVU-Zoom with upstream changes.

src/Effects/NNEDI3/nnedi3-nns16-win8x4.hlsl Outdated Show resolved Hide resolved
src/Effects/RAVU/ravu-r2.hlsl Outdated Show resolved Hide resolved
src/Effects/RAVU/ravu-r2.hlsl Outdated Show resolved Hide resolved
@Blinue
Copy link
Owner

Blinue commented Jan 14, 2024

The names of these effects should be consistent with other effects, such as "RAVU_Lite_R3". The old RAVU_Lite_R3 should be deleted.

@hauuau
Copy link
Contributor Author

hauuau commented Jan 16, 2024

I have modified scripts to generate shaders with names in that format.
I have also added them to project files.

But I also suggests simplifying build system for effects with wildcards as it will remove or at least reduce the need to modify project files when adding new effects. The main downside of this approach is that it will be impossible to modify project files from Visual Studio and they would need to be modified manually as text files.

Feel free to remove that part if that approach is undesirable.

@Blinue
Copy link
Owner

Blinue commented Jan 16, 2024

But I also suggests simplifying build system for effects with wildcards as it will remove or at least reduce the need to modify project files when adding new effects. The main downside of this approach is that it will be impossible to modify project files from Visual Studio and they would need to be modified manually as text files.

As far as I know, MSBuild still has bugs with wildcard support, and it is not recommended to use them.

Since it is rare to add a large number of effects at the same time, the current solution is still acceptable. Of course, as more and more effects are added, it may eventually become unsuitable to put them in the project file.

@hauuau
Copy link
Contributor Author

hauuau commented Jan 16, 2024

Ok. Reverted.

@Blinue
Copy link
Owner

Blinue commented Jan 16, 2024

Two suggestions:

  1. Please state at the beginning of each effect file that it is generated by hauuau/magpie-prescalers and not to be modified directly.
  2. The RGB versions are slower than the regular versions, but there is no visual difference (at least to me), so is it really necessary to introduce them?

@bjin
Copy link

bjin commented Jan 16, 2024

The RGB versions are slower than the regular versions, but there is no visual difference (at least to me), so is it really necessary to introduce them?

Actually, I believe RGB version makes more sense for Magpie than for mpv, for the following reasons:

  1. Most video are yuv420 subsampled, so for mpv, a chroma scaler will be used before RGB shader. But for magpie, if I understand correctly, the captured texture is already RGB. If it's a captured from a game, it's even RGB-native. So I just see no reason to convert it to yuv and convert back later.
  2. Some games, especially retro 8-bit pixel games, are very prone to different kind of chroma artifacts during upscaling, if luma and chroma are processed differently. A widely used test pattern for chroma upscalers are also using bitmap fonts, see 1. A bicubic scaler for chroma with any "fancy" luma upscaler (which usually enhance details a lot) will surely makes inferior result compare to RGB scaler for pixel games, because luma and chroma are using different and even distinct algorithms for upscaling.

@Blinue
Copy link
Owner

Blinue commented Jan 17, 2024

@bjin Thank you for your explanation! The test pattern is very helpful, and now I can see the clear difference. Although I can’t tell them apart with my naked eye in real-world scenarios, it’s better to keep them for different situations.

@hauuau
Copy link
Contributor Author

hauuau commented Jan 17, 2024

It's also possible to notice some differences on some real content: image.
Look especially at the eyes:

Ravu Zoom AR R3 + Bilinear Chroma: Ravu Zoom AR R3 RGB:
ravu_zoom_ar_r3_crop ravu_zoom_ar_r3_rgb_crop

Of course it could be debated if that rather subtle difference is worth performance penalty. It also might be quite interesting to add something like CfL to this comparison but porting and integrating it probably won't be easy.

I have added some information about where to find generation scripts to the top comment. Would this be enough?

@Blinue
Copy link
Owner

Blinue commented Jan 18, 2024

I have added some information about where to find generation scripts to the top comment. Would this be enough?

Very nice, thank you!

@Blinue
Copy link
Owner

Blinue commented Jan 18, 2024

I think this PR is ready to be merged, thank you for your excellent work! I will do some more testing and make sure nothing is missed.

@hauuau
Copy link
Contributor Author

hauuau commented Jan 18, 2024

Thank you for reviews and all the help with bringing it to this point!

@Blinue
Copy link
Owner

Blinue commented Jan 21, 2024

I tested each effect under the same conditions, and this is a table that records their performance.

Effect Timing (ms) RGB version
Nearest 0.05
Bicubic 0.18
Lanczos 0.46
RAVU_3x_R2 0.27 0.28
RAVU_3x_R3 0.33 0.38
RAVU_3x_R4 0.44 0.58
RAVU_Lite_AR_R2 0.14
RAVU_Lite_AR_R3 0.21
RAVU_Lite_AR_R4 0.27
RAVU_Lite_R2 0.11
RAVU_Lite_R3 0.14
RAVU_Lite_R4 0.21
RAVU_R2 0.22 0.32
RAVU_R3 0.26 0.44
RAVU_R4 0.47 0.83
RAVU_Zoom_AR_R2 0.56 0.96
RAVU_Zoom_AR_R3 0.89 1.34
RAVU_Zoom_R2 0.32 0.46
RAVU_Zoom_R3 0.61 0.80
NNEDI3_nns16_win8x4 0.63
NNEDI3_nns16_win8x6 0.89
NNEDI3_nns32_win8x4 1.13
NNEDI3_nns32_win8x6 1.70
NNEDI3_nns64_win8x4 2.18
NNEDI3_nns64_win8x6 3.17
NNEDI3_nns128_win8x4 4.33
NNEDI3_nns128_win8x6 6.51
NNEDI3_nns256_win8x4 9.69
NNEDI3_nns256_win8x6 15

@bjin
Copy link

bjin commented Jan 21, 2024

@hauuau I did another look and find that lerp() function in HLSL doesn't support boolean input like GLSL. That's why in the initial nnedi3 port, if one parameter of mix/lerp is nan, the result becomes nan even if the input is not selected. Because in HLSL, it's simple linear interpolation after boolean value is casted into 0.0 or 1.0.

I used the same boolean mix() to calculate theta mu strength and coherence in RAVU as well, and at least calculation of mu is effected by the nan issue as well. But since value of mu will only be used for comparison, this issue doesn't become visual artifacts.

An easy way to fix this is to define another boolean mix macro like:

#define bmix(a, b, c) ((c) ? (b) : (a)) 

@Blinue
Copy link
Owner

Blinue commented Jan 21, 2024

@bjin Thank you for the investigation. When one input is nan, the result of lerp is undefined, meaning different hardware can produce different results. This is a subtle difference between hlsl and glsl.

@hauuau I noticed that the NNEDI3 effects are not sorted correctly in the user interface. Please use SORT_NAME to fix this.
image

@hauuau
Copy link
Contributor Author

hauuau commented Jan 22, 2024

@bjin Thank you for bringing attention to possible problems in RAVU and explaining the difference between lerp and mix!
It feels that using function overloading in this case is more cleaner than using an additional macro. If I understand it correctly HLSL always inlines functions so there shouldn't be any performance penalty compared to a macro.

@Blinue Thank you for performance data! I have added SORT_NAME to NNEDI3.

@Blinue
Copy link
Owner

Blinue commented Jan 22, 2024

This document states that functions in hlsl are always inlined, so there is no overhead for calling them. I think your fix is appropriate.

This PR is ready to be merged, and I’m happy to do so. Do you have any more changes to make?

@hauuau
Copy link
Contributor Author

hauuau commented Jan 22, 2024

I can't think of anything to add or change at this time. If something comes to mind later I'll make a separate PR.

@Blinue
Copy link
Owner

Blinue commented Jan 22, 2024

Awesome! Thank you for your creative work and your responsiveness to the code review. Please don’t hesitate to share more ideas with us if you have any.

@bjin Thank you also for creating these excellent algorithms and providing a lot of help during the porting process!

@Blinue Blinue merged commit 2886841 into Blinue:render-system Jan 22, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

One pixel bleed from the bottom to the top of the screen on RAVU_Zoom_R3
3 participants