[GSoC 2019] RetroPlayer: OpenGL back-end for shaders

[gusandrianos]

Description

This PR extends the functionality #86 brought to RetroPlayer by introducing support for other platforms through an OpenGL implementation.
The code in this PR is part of this GSoC project

In short, this code adds support for multi-pass shader presets, as defined in the libretro spec, which opens a whole new world of customization to the look and feel of games, through the massive amount of presets available in this format.

While the spec is written with cg shaders in mind, it is also extended to GLSL and slang shaders.

You can read more about the current state of my solution, what works and what doesn't(yet), here.

Motivation and Context

Shaders can be used to:

• Scale lower resolution games to higher resolutions
• Emulate the look of CRT or other types of monitors
• Apply noise reduction or anti-aliasing filters

Of course, the list goes on. Most importantly, shaders can be used to elevate the gameplay experience and make it more immersive. A game can either look like it's being displayed on a CRT or a Gameboy or an LCD monitor or be upscaled and viewed on a 4K monitor, the possibilities are endless.

Regarding the commits in this PR.

I should mention that the last commit that falls under the GSoC umbrella is this one. Anything that follows it should not be considered as part of the GSoC project.

Let's give you some context about each commit.
The first three commits are setting up the relevant parts of code for usage in the next commit.
The fourth commit contains all the work that involves the processing and rendering of the shader presets.
The fifth commit connects the code to RetroPlayer.

How Has This Been Tested?

I took a brute force approach by testing almost every preset in the glsl-shaders repo. It has only been tested on Linux and it is unknown if it works on other platforms that use OpenGL.

To test this code yourself

• Build Kodi using the guides on the main repository
• Download and install the game.shader.presets binary add-on
• Get the latest glsl-shaders from libretro's repository
• Extract the shaders in /your/kodi/directory/addons/game.shader.presets/resources/libretro/glsl
• Replace the contents of ShaderPresetsDefault.xml in the resources directory with this file. This is a temporary workaround, I will update the add-on to choose the correct file depending on the situation.

Screenshots (if appropriate):

Here's a video that demonstrates what these changes do.

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Clean up (non-breaking change which removes non-working, unmaintained functionality)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that will cause existing functionality to change)
• Cosmetic change (non-breaking change that doesn't touch code)
• None of the above (please explain below)

Checklist:

• My code follows the Code Guidelines of this project
• My change requires a change to the documentation, either Doxygen or wiki
• I have updated the documentation accordingly
• I have read the Contributing document
• I have added tests to cover my change
• All new and existing tests passed

[garbear]

@KostasAndrianos can you cherry-pick these 7 commits? 327d997~7...327d997

[VelocityRa]

Lines from here to 267 are copied as-is from D3D and should be moved to common code since they're not backend specific. Probably just a method with the passes as input and the scaled size as output.

[gusandrianos]

Indeed, will do that

[garbear]

Yes, there's lots of places where common code can be factored out. The good news is duplication got us this far, and now these places are easy to find.

[garbear]

Missing the header <memory>; added in gusandrianos#2

[garbear]

Style infraction; fixed in gusandrianos#2

[garbear]

Alphabetize to reduce likelihood of git conflicts

[garbear]

The second converts LUTs from being an IShaderTexture to be an ordinary CTexture, because LUTs are basically just an ordinary texture resources and they don't need to be wrapped in an IShaderTexture.

Nice! You solved the memory leak: KOPRajs@e2131fc#diff-f19620d645bf155b9aa2ee717dba0324711fb6f127a6083a73fc2e7b3df2a34bL70-R71

macOS is still getting corruption with your latest work (keep in mind macos GL driver is really old):

But aside from the corruption on macOS's old GL driver, all shaders are now rendering for me!

[garbear]

I found another memory error, and this time it's in both DX and GL: https://github.com/KOPRajs/xbmc/blob/retroplayer-21.2/xbmc/cores/RetroPlayer/shaders/windows/ShaderPresetDX.cpp#L378-L389

Use of new anywhere absolutely needs to go. Probably use of any raw pointers too. I can't think of a place where raw pointers would be needed by shader code.

I went to check out the texture memory leak, but the duplication is really adding some technical debt. I de-duplicated shader presets, saving over 500 (!) lines of code:

https://github.com/garbear/xbmc/commits/retroplayer-21.2

This was super easy thanks to your syncing effort!

[KOPRajs]

Nice! You solved the memory leak: KOPRajs@e2131fc#diff-f19620d645bf155b9aa2ee717dba0324711fb6f127a6083a73fc2e7b3df2a34bL70-R71

I found another memory error, and this time it's in both DX and GL: https://github.com/KOPRajs/xbmc/blob/retroplayer-21.2/xbmc/cores/RetroPlayer/shaders/windows/ShaderPresetDX.cpp#L378-L389

As you've probably found out by now, the one I solved was in LUTs, so it was a new one. The one you've found out in ShaderPresetDX.cpp is the same one, we talked about in GL. I've seen, that you've done a lot of work and hopefully solved all of them now. Nice! This is something far beyond my abilities.

Do you see any errors in the head commit?

I'll test it today.

But aside from the corruption on macOS's old GL driver, all shaders are now rendering for me!

The driver might be old, but I don't think it is so buggy. Also, you've said that the same issue was reported for Android as well.
Btw. Unfortunately, all shaders rendering something doesn't mean they are working 100%, there are still issues with some of them.

This was super easy thanks to your syncing effort!

Great, that was the idea!

Btw. There is one more thing that should be moved to a common code. When I was testing Windows shaders, I've seen this in log:

2025-02-05 21:34:36.179 T:29196   error <general>: CD3DEffect::CreateEffect(): warning X4717: Effects deprecated for D3DCompiler_47
                                                   (1,9-17): warning X3568: 'parameter' : unknown pragma ignored
                                                   (2,9-17): warning X3568: 'parameter' : unknown pragma ignored
                                                   (3,9-17): warning X3568: 'parameter' : unknown pragma ignored
                                                   (4,9-17): warning X3568: 'parameter' : unknown pragma ignored
                                                   (5,9-17): warning X3568: 'parameter' : unknown pragma ignored
                                                   (6,9-17): warning X3568: 'parameter' : unknown pragma ignored
                                                   (7,9-17): warning X3568: 'parameter' : unknown pragma ignored
                                                   (8,9-17): warning X3568: 'parameter' : unknown pragma ignored
                                                   (9,9-17): warning X3568: 'parameter' : unknown pragma ignored
                                                   (68,19-28): error X3047: 'video_size': struct/class members cannot be declared 'uniform'

That is why I created the CShaderUtilsGL::StripParameterPragmas() for GL, but it should be applied to both GL and DX. So we should remove it from GL and GLES utils, place it into the common utils and use it for DX as well.

[garbear]

I finished fixing the texture leaks: https://github.com/garbear/xbmc/commits/retroplayer-21.2

Unfortunately, it breaks some shaders, so I made a mistake somewhere. Reverting it fixes the shaders. I'll have to redo the work in pieces and find what I broke.

[garbear]

Cool, that took me faster than I thought to fix: https://github.com/garbear/xbmc/commits/retroplayer-21.2 . I broke the commit into steps and found the problematic one (Change source texture from pointer to reference for memory safety was initially flawed, now it works).

My branch still has the bug that was introduced in that one refactor, right? Are you able to find any mistakes I made? Sorry to put you bug-hunting in my code, but everything else renders fine on my end and should work. The extra steps in the memory leak fix will whittle down the diff of the refactor.

[KOPRajs]

I've done the testing:

1. Your current retroplayer-21.2 branch (c3d7431):
   The LUT geometry issue is still there for GLES, but apart from that, I haven't seen any other issues compared to my video-shaders-v5 branch. Nice work!

2. The retroplayer-update2 branch (c3b5e48):
   The GLES issue in LUTs is there.
   I've also tried to do a build without the last commit. The issue is still there, even when built from 65cf011!
   I needed to pull 1 line to xbmc/cores/RetroPlayer/shaders/gles/CMakeLists.txt from c3b5e48 to get it to build.

3. Build from retroplayer-update branch (2aa1452):
   The GLES issue in LUTs is not there!
   I've also tried to cherry-pick the c3b5e48 to retroplayer-update branch. I needed to fix the 1 line in CMakeLists.txt again (this time the opposite change) in order to get it to build, but the GLES issue is still not there!

So it must be something else, that was changed between 2aa1452 and 65cf011. Possibly a change in texture handling or in GLES renderer?

P. S. Congrats on solving the texture leaking!

[KOPRajs]

Commits like this might be what we are looking for: 15b817b

The VertexAttribArray is what sets the texture coordinates and could possibly affect the LUT geometry.

EDIT: More candidates:
98dd67a
e70e99b
6e9f089
7ed3554

I'll try to apply these on top of https://github.com/garbear/xbmc/commits/retroplayer-21.2 and we'll see.

[KOPRajs]

I've got it!

It is exactly ONE character, which fixes it!
6e9f089

Just add this one to https://github.com/garbear/xbmc/commits/retroplayer-21.2 and there are no new issues when compared to my video-shaders-v5 branch. It is not a big surprise, given that I was disabling usage of optimal (Power of Two alias POT) textures for this to work in CShaderPresetGL::CreateShaderTextures and this commit handles NPOT textures.

EDIT: The good thing about this is that it is already merged upstream.

Btw. Look at the image in the PR:
xbmc#25479

[KOPRajs]

I've got 3 items remaining on my shader checklist (not counting implementation of new features and fixing even more shader presets):

1. Get rid of the translated strings (names) from the configuration XML if possible, so it is editable and presets can be added and removed easily. It is also beneficial so we don't need to change the XML format in the future and break compatibility with the add-on.

2. Move the StripParameterPragmas() to common utils and use it in the DX code too. This is easy one.

3. And the last but not least, which might be a lot more demanding and maybe we can do it later, but I feel it is not good by design. I've noticed, that the DX version is re-using code from VideoPlayer.
   I think it has some drawbacks:

• Historically, RetroPlayer got its own renderers to avoid using code from VideoPlayer.
• Changes to VideoPlayer shader code can break video shaders for games.
• The shader code in VideoPlayer is not suited for this purpose. It already handles multi-pass shaders on its own, but we are using it to create and execute single pass shaders for each of our own passes. This leads to strange issues and errors in log, which actually revealed the issue to me. Also, we might want to change the code to fix more shaders for Windows, but we currently can't, because the actual code is in the VideoPlayer.

[garbear]

It is exactly ONE character, which fixes it!

Great find!!! First, I backported too much, then, I backported too little. 😄

1. Get rid of the translated strings (names) from the configuration XML if possible, so it is editable and presets can be added and removed easily.

👍

2. Move the StripParameterPragmas() to common utils and use it in the DX code too. This is easy one.

👍👍👍

3. And the last but not least, which might be a lot more demanding and maybe we can do it later, but I feel it is not good by design. I've noticed, that the DX version is re-using code from VideoPlayer.

I actually re-discovered this when I was fixing the memory leak, and was surprised to see it.

Historically, RetroPlayer actually was VideoPlayer with some game hacks, until I rewrote the player in 2017 for GSoC and made it fully independent. Or so I thought. Looks like there's one bit of VP left.

This architecture violation is definitely a blocker for the PR. I'll solve it the same way as GL/GLES, by taking on some duplication as technical debt. But at least then RP will fully own the code and we can modify it/strip it clean to better suit our purposes.

[KOPRajs]

I'll solve it the same way as GL/GLES, by taking on some duplication as technical debt. But at least then RP will fully own the code and we can modify it/strip it clean to better suit our purposes.

Yes, I've got the same idea. Duplicate the needed part to break the dependency on VideoPlayer and then strip down the not needed parts (mostly code which handles multiple passes in VideoPlayer). I think it is not actually going to be that much code and after the stripping it will not be the duplicate anymore.

[garbear]

I did the split, see my branch. It wasn't too bad, a little over 100 lines duplicated. Strip away!

[KOPRajs]

I did the split, see my branch.

It seems that the final breakage was not final after all. I've noticed that CShaderDX was still using VideoPlayer. I've fixed it here along with some cosmetic changes to logging: KOPRajs@9c6cdde

Strip away!

It turned out that the multi pass nature comes directly from the CD3DEffect, so there is not much to strip.

I moved the StripParameterPragmas() here: KOPRajs@c7fbd26
It gets rid of the warning in logs when used for HLSL code, but it also breaks some of the TV-out shaders, so it is only used in GL/GLES for now.

Are we still having the issue with GUI corruption after using shaders on macOS? Have you tried to change skin? Does skin change fix it?

[KOPRajs]

Unfortunately, I've come across another issue in the current https://github.com/garbear/xbmc/commits/retroplayer-21.2 branch, which is not present in the video-shaders-v5 branch. It seems to only affect build for GBM/GL and it breaks rendering of the GUI shaders (nearest and bilinear). The RetroArch shader presets work fine, but I get only black screen for the first two "without shaders" options.

Since it only affects GBM/GL and neither Wayland/GL nor GBM/GLES have this issue, I expect the issue to be in the DMA renderer, maybe in f5e5efe, but more testing is needed.

EDIT: I have confirmed that reverting f5e5efe fixes it, so now we need to find what's broken in the DMA-GL render after this commit. The DMA-GLES works.

[KOPRajs]

I've got it to work: KOPRajs@b8e7c23

I went through several rabbit holes, before I've finally found the cause. Now I understand, why the split was needed in the first place. I did some other small changes along the way and since I was little confused by the new DMA utils module, I've tried to removed it. The issue was not in it, so feel free to add it back, if you want. However, is it really worth to add a new module and 50+ lines of code to get rid of one ifdef? Even when the ifdef is actually moved to another file?

[garbear]

Are we still having the issue with GUI corruption after using shaders on macOS? Have you tried to change skin? Does skin change fix it?

Yes, I've been testing after each change and no luck so far. I'll see if skin changing affects anything.

I've got it to work: KOPRajs@b8e7c23

I'll send a PR upstream

However, is it really worth to add a new module and 50+ lines of code to get rid of one ifdef? Even when the ifdef is actually moved to another file?

50 lines is a steep cost, but it avoids later harm. Once you add one ifdef in the wrong place, you keep adding more, and then end up with the state we had earlier.

Maybe someday buffers need GLES-specific code, and I want the author to follow the pattern of splitting code instead of #ifdeffing more.

Alternatively, if we want to avoid the module, we could split buffers too. In that regard, the hiding the #ifdef in a common module seemed a cheaper solution than taking on more duplication.

[garbear]

I opened the upstream PR: xbmc#26405

Commits are the same as in my branch. Can you give it a test?

[KOPRajs]

It looks ok.
I'm not so sure about the description, but the idea was simple: Before the DMA renderer split it was always using GLES renderer (even when running on GL), because the DMA renderer was inherited from the GLES renderer. It worked, because GLES is a subset of GL. After the split the new DMAOpenGL renderer inherits the renderer setup from the OpenGL renderer, but it used Render() method copied from the GLES renderer (former DMA), hence the issue. So I've changed the Render() method to match the one from OpenGL renderer instead of the OpenGL ES (in DMAOpenGLES it also matches the one from OpenGLES). And I've synced the struct and other details along the way.

P. S. I'm building a new HTPC using LibreELEC on x86 Intel hardware. LE is running GBM/GLES combo by default, but since both the GL video shaders and the GL libretro cores in the future will benefit from switching to full desktop GL, I'm going to try to build GBM/GL LibreELEC. So I need this specific part to work :-)

[garbear]

Updated the description.

I'm going to try to build GBM/GL LibreELEC. So I need this specific part to work :-)

Cool idea! I wonder if this could be an official flavor of LE? Even if expanded shader support is the only initial impetus.

[KOPRajs]

It seems like it is not impossible to switch the main LE from GLES to GL in the future, if there is enough motivation (like getting the GL libretro cores to work). We would need someone to finish the HDR support in Kodi to work with GBM/GL.
https://forum.libreelec.tv/thread/29484-using-opengl-core-profile-for-x86-generic-build/

[KOPRajs]

P. S. I've got only 2 remaining items on shaders checklist:

• The XML format change a get rid of the strings.
• The GUI corruption on macOS.

There is not much more I can do for this now, so I suggest to finish the XML format change and create the shaders PR. Let's hope that more people interested in the PR will help us solve the GUI corruption.

In the meantime, I'm going to try to resurrect the FBO renderer code and give it another try.

[garbear]

We would need someone to finish the HDR support in Kodi to work with GBM/GL.
https://forum.libreelec.tv/thread/29484-using-opengl-core-profile-for-x86-generic-build/

Wish granted! xbmc#26406

Let's hope that more people interested in the PR will help us solve the GUI corruption.

I rebased my branch with our latest work: https://github.com/garbear/xbmc/commits/retroplayer-21.2-20250210. Everything should be good now but the XML stuff. I did a test build (20250210) for Android arm64: https://mirrors.kodi.tv/test-builds/android/arm64-v8a/ . Loaded on my NVidia Shield and got the corruption problem. So we should have a good number of people able to replicate this.

[KOPRajs]

Loaded on my NVidia Shield and got the corruption problem. So we should have a good number of people able to replicate this.

How about the skin change? Does it fix the issue?

Also, are you testing it on a clean installation or on fully loaded setup? Maybe try to reproduce on a clean setup, so we know if it might interfere with something in your setup.

[KOPRajs]

I removed the translated strings for my build: KOPRajs@767560f

I've changed the category to folder, because it better suites my video shader organization and it better justifies the removal of the translation. I've also got rid of the description. The GetLocalizedString() method is not removed, because we will need it in the future, when we add the new functionality to the GUI (Add, Rename, Remove, Move, etc. will need translation). It is probably not perfect, but it works perfectly for me. The XML is now ready to be editable.

Here is my current XML for the reference (we can use it for the start):

<?xml version="1.0" ?>
<presets>
    <preset>
        <path>libretro/glsl/anti-aliasing/advanced-aa.glslp</path>
        <folder>Anti-Aliasing</folder>
        <name>Advanced AA</name>
    </preset>
    <preset>
        <path>libretro/glsl/scalenx/scale2x.glslp</path>
        <folder>Scale</folder>
        <name>Scale2x</name>
    </preset>
...
</presets>

The idea is to auto-generate this in the future, when user adds new preset from the GUI by browsing and selecting the file:

    <preset>
        <path>libretro/glsl/anti-aliasing/advanced-aa.glslp</path>
        <folder>anti-aliasing</folder>
        <name>advanced-aa</name>
    </preset>

And give users the option to rename, if they want. We can even think of automatic Capitalization and removal of slashes when creating new item.

[KOPRajs]

FYI: LibreELEC/LibreELEC.tv#9781

This was fast!

[garbear]

Been traveling the last 4 days, now I'm back and can resume getting shaders over the finish line. I'll pull in your naming change. The existing UI code needs a little more work too. I'll also do testing with skins and fresh installs soon. And I'll get kodi-game/game.shader.presets#17 and kodi-game/game.shader.presets#18 merged.

[garbear]

I merged kodi-game/game.shader.presets#17 and noticed that we lost a lot of translations. For example,

In Danish, "TV" is "TV", but "out" is "ud". So "TV-out" doesn't mean anything to a danish-only speaker but "TV-ud" is understandable. What do you think we should do about this?

Great job on updating the shaders lists! I'll do a round of test builds so we can get the new lists into peoples' hands.

[KOPRajs]

What do you think we should do about this?

I think we need to stop thinking about video shader profiles as of built-in Kodi funtions. The profiles are external resources, which we have inherited from the libretro project. They are organized in folders and they have file names and that's it. You can load whatever shader profile you want. Users can even download custom profiles from internet (forums etc.) and load them via GUI in the future. There is no way, how we can translate these external resources, we only have the folder name and the file name. It is like if you wanted to translate the name of each game (if you load Sonic the Hedgehog, you'll get Sonic the Hedgehog no matter what language you understand). Unless we have the ultimate database of all names, there is no consistent way to do it, unfortunately.

That is why I chose to use the folder name and the file name, but I'd like to give users the option to customize both. Currently it is done via the XML, in the future it should be possible to rename it directly in the GUI. It is clean and simple, it gives us pretty good out-of-box experience as well as it gives users the option to organize shader profiles themselves, whatever they like.

The only other option I can think of would be kind of hybrid approach. Like translate something and fall back to a custom name (generated from the file name), if there is no translated string. Something like that can be added later, when we finish the GUI and we will see, how it works.

[KOPRajs]

So "TV-out" doesn't mean anything to a danish-only speaker but "TV-ud" is understandable.

In the current form, the TV-out simply means, that this is a shader preset from the tv-out folder. Once you start experimenting with shader presets (trying to load more of them), you'll find out, that it is good to know the file name and the folder name to be able to identify, which shader preset you are looking at.

You know, the xBR etc. doesn't mean anything either.

[garbear]

You know, the xBR etc. doesn't mean anything either.

Good point, looking at the translations we lost, so few of them actually meant anything. And users don't read. It's not worth translating them.

I did some builds with the new lists, will announce in the forum soon.