[GSoC 2017] Retroplayer: Video Shaders#86
Conversation
| } | ||
| else | ||
| { | ||
| CRect destRect = g_graphicsContext.StereoCorrection(m_destRect); |
There was a problem hiding this comment.
Can you move some globals out of this class? Calling globals reduces the modularity of code.
There was a problem hiding this comment.
This is copied and pasted from WinRenderer.
How would I move it out of this class? Pass it as an arg from RPRenderManager to RenderUpdate -> Render? Could do the same thing for g_Windowing.GetBackBuffer() below but I'm not sure if it would make sense for the other renderers too.
There was a problem hiding this comment.
Any global depending on DX shouldn't be moved to the base class, so those can stay. geometry functions should definitely be moved up the class hierarchy
There was a problem hiding this comment.
Of course, it just seemed weird that the existing Kodi code wasn't written this way.
| VideoFilterProperties videoFilter; | ||
|
|
||
| if (child->FirstChild() == nullptr) | ||
| continue; |
There was a problem hiding this comment.
Can you also add nullptr checks to the FirstChild() calls below?
There was a problem hiding this comment.
Yea. There's an XMLUtils library that might cut down on some of the code.
There was a problem hiding this comment.
I didn't use it so that we can move the parsing code to the add-on at some point.
There was a problem hiding this comment.
OK, then we should add the nullptr checks to the code as it is
961b661 to
0a9d748
Compare
| videoFilter.categoryIndex = atoi(categoryIndexNode->Value()); | ||
| TiXmlNode* descriptionNode; | ||
| if (((descriptionNode = child->FirstChild("description"))) && ((descriptionNode = descriptionNode->FirstChild()))) | ||
| videoFilter.descriptionIndex = atoi(descriptionNode->Value()); |
There was a problem hiding this comment.
Can you expand this a bit? It's hard to debug when so many things happen on one line. This will probably be a lot of code, tinyXML is rather verbose.
There was a problem hiding this comment.
Yeah I was trying to avoid the horrible verboseness, but this way does make it harder to debug parsing.
| virtual VideoShaderPasses& GetPasses() = 0; | ||
| }; | ||
|
|
||
| inline void IVideoShaderPreset::SetVideoSize(const unsigned videoWidth, const unsigned videoHeight) |
There was a problem hiding this comment.
Whoops, leftovers from ReSharper, I'll take care of it later today.
823ced1 to
b44ed3f
Compare
| TiXmlNode* pathNode; | ||
| if(((pathNode = child->FirstChild("path"))) && ((pathNode = pathNode->FirstChild()))) | ||
| videoFilter.path = URIUtils::AddFileToFolder(basePath, pathNode->Value()); | ||
| if((pathNode = child->FirstChild("path"))) |
There was a problem hiding this comment.
Minor, but if statements are missing spaces
| */ | ||
|
|
||
| #include "RPBaseRenderer.h" | ||
| #include "ServiceBroker.h" |
There was a problem hiding this comment.
oh I thought you had said the opposite
| g_graphicsContext.Clear(g_Windowing.UseLimitedColor() ? 0x101010 : 0); | ||
|
|
||
| // Set alpha blend state | ||
| g_Windowing.SetAlphaBlendEnable(alpha < 255); |
There was a problem hiding this comment.
This function is only available on Windows. Can you move it and the other use of g_Windowing back to the windows renderer?
There was a problem hiding this comment.
I'll remove this one but ApplyStateBlock() is fine, it's defined in CRenderSystemBase.
There was a problem hiding this comment.
UseLimitedColor isn't so I'll move that one
There was a problem hiding this comment.
...Oh it's in CWinSystemBase, which is not Windows specific.
| #include "libswscale/swscale.h" | ||
| } | ||
|
|
||
| #include <cstring> |
| // TODO: Doesn't work. Investigate calling this in Execute or binding the SRV first | ||
| //g_Windowing.Get3D11Context()->PSSetSamplers(2, 1, &m_pSampler); | ||
| SetShaderParameters( *sourceDX->GetPointer() ); | ||
| Execute({ targetDX->GetPointer() }, 4); |
There was a problem hiding this comment.
please elaborate on this. What is Execute and what does your comment mean?
There was a problem hiding this comment.
The shader spec I'm implementing allows for specifying either linear, or nearest sampling for each pass. This line was supposed to set the sampler state to the appropriate ones we've read from the shader preset file with PSSetSamplers right before rendering each pass/shader, but it didn't work.
This wasn't a huge problem as these samplers were often hardcoded in the shaders after we started porting them from Cg to HLSL/FX, instead of relying on the implementation reading the shader preset files. Ideally it'd work though, as some shaders files are re-used by different presets with different sampling.
Anyway, I think Execute refered to CWinShader::Execute, you may as well disregard that because:
Upon further inspection with the Graphics Debugger (after making the comment) it became apparent that the reason this didn't work was because the FX11 framework was applying its own samplers right before rendering, thereby overwriting whatever sampler I was setting here. I couldn't find a way to set the sampler with FX11 or overwrite, so I left it at that as it was good enough for now.
20cc976 to
e9c0c30
Compare
daf0011 to
e3ddbc4
Compare
6bfce09 to
0953483
Compare
|
@VelocityRa I rebased this branch on 18.6 (for safety, old commit was e3ddbc4). Ready for you to squash against it |
e9c0c30 to
7368539
Compare
7368539 to
12f3bc7
Compare
This imports several shader presets from libretro common at commit 187c161 (https://github.com/libretro/common-shaders/tree/187c161).
|
Are these Video Shaders for RetroPlayer gaming still on the roadmap? |
|
The code is probably PR-ready but I still haven't gotten around to resolving the distribution issue for shaders/presets. |
Are you thinking about distributing them as Add-ons for Kodi so that the user can install only if they really want to have them? |
|
I'm not sure. I don't plan on working on it for the foreseeable future, sorry. |
Description
This PR adds support for rendering multi-pass shader presets, as defined in this libretro spec.
The spec essentially introduces shader “preset” files: a standardized file format which allow for multi-pass shader configurations to achieve advanced filtering, without manually copying and pasting shader code all over the place.
They have other features too, but this is their core functionality and reason of existence. The biggest advantage of implementing this would be the vast library of pre-existing shaders written in this format, both for OpenGL (ES), and Direct3D.
Not all can be used yet because they porting to HLSL (thankfully the libretro/RetroArch team is on board with helping us port them), but there are hundreds of presets available.
Here's RetroArch's page on shader presets.
Here's a few other examples.
Motivation and Context
Shaders can be used to:
Adjust everything about the colour of the image in a robust way without changing application-wide settings. These including gamma, luminance, contrast, saturation.
These are just a few examples, there can be more applications that I haven’t thought of.
If I end up not implementing support for libretro’s shaders, I aim to develop/port a few shaders that will be provided by default, but shader developers can extend this by providing more.
How Has This Been Tested?
TODO
ScreenshotsVideo:Link

Types of change
Checklist:
TODO