-
Notifications
You must be signed in to change notification settings - Fork 53
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
[GSoC 2017] Retroplayer: Video Shaders #86
base: feature_shaders
Are you sure you want to change the base?
[GSoC 2017] Retroplayer: Video Shaders #86
Conversation
} | ||
else | ||
{ | ||
CRect destRect = g_graphicsContext.StereoCorrection(m_destRect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move some globals out of this class? Calling globals reduces the modularity of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, it just seemed weird that the existing Kodi code wasn't written this way.
@@ -118,6 +118,9 @@ void CDialogGameVideoFilter::InitVideoFilters() | |||
{ | |||
VideoFilterProperties videoFilter; | |||
|
|||
if (child->FirstChild() == nullptr) | |||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add nullptr checks to the FirstChild() calls below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All 8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea. There's an XMLUtils library that might cut down on some of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then we should add the nullptr checks to the code as it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, leftovers from ReSharper, I'll take care of it later today.
823ced1
to
b44ed3f
Compare
@@ -127,17 +127,21 @@ void CDialogGameVideoFilter::InitVideoFilters() | |||
continue; | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but if statements are missing spaces
@@ -19,14 +19,15 @@ | |||
*/ | |||
|
|||
#include "RPBaseRenderer.h" | |||
#include "ServiceBroker.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Directories before files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this one but ApplyStateBlock()
is fine, it's defined in CRenderSystemBase
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseLimitedColor
isn't so I'll move that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...Oh it's in CWinSystemBase
, which is not Windows specific.
@@ -34,8 +34,6 @@ extern "C" { | |||
#include "libswscale/swscale.h" | |||
} | |||
|
|||
#include <cstring> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required by std::memcpy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry. damnit msvc
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please elaborate on this. What is Execute and what does your comment mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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