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

Windows renderer: Nearest neighbor video scaling #85

Merged
merged 3 commits into from Aug 5, 2017

Conversation

Projects
None yet
2 participants
@VelocityRa

This PR implements nearest neighbor video filtering/scaling for the Pixel Shader renderer used on Windows.

Description

Nearest neighbor scaling is the conventional scaling used in emulators for older systems, as it results in the most accurate and crisp image. The lack of it was the first thing I noticed when I initially tried RetroPlayer, so I decided to take a crack at it.

It's implemented on Linux, but for windows it was only implemented in the software renderer. DXVA and the Pixel Shader renderer didn't support it. This PR implements it for the Pixel Shader renderer.
It's not needed for DXVA (would be pretty hacky anyway; it doesn't seem to allow you to change the scaling), since we'll probably ditch it for RetroPlayer.

For a few implementation details, here's a copy of the main commit message:

  • I tried to change as few things as possible by using the
    color shader used in the pixel renderer to do the sampling.
  • It's all compile-time and the shader gets recompiled when
    it needs to (if the sampling option changed from/to nearest
    neigbor, to/from something else).

Motivation and Context

For RetroPlayer, this was without a doubt needed eventually.
Low res games were really ugly/smudgy with the default filtering. With nearest neighbor the image looks as crisp as it should.

How Has This Been Tested?

Tested in RetroPlayer by loading a Game Boy emulator and a game. The difference is quite obvious :)

Screenshots (if appropriate):

Before:
image

After:
image

(more obvious full-screen)

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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
Show outdated Hide outdated xbmc/cores/VideoPlayer/VideoRenderers/VideoShaders/WinVideoFilter.h
@@ -144,7 +156,8 @@ class CYUV2RGBShader : public CWinShader
CPoint m_dest[4];
EBufferFormat m_format;
float m_texSteps[2];
COutputShader *m_pOutShader;
COutputShader *m_pOutShader;
bool m_isNearest;

This comment has been minimized.

@garbear

garbear Jul 23, 2017

Owner

No need to touch the existing line, just add your variable without the additional whitespace. We discourage this kind of alignment, even though I use it in some of the code I maintain.

@garbear

garbear Jul 23, 2017

Owner

No need to touch the existing line, just add your variable without the additional whitespace. We discourage this kind of alignment, even though I use it in some of the code I maintain.

This comment has been minimized.

@VelocityRa

VelocityRa Jul 23, 2017

Hm, just felt that inconsistency is less preferred than strictly following code guidelines on this one.
Will revert it regardless.

@VelocityRa

VelocityRa Jul 23, 2017

Hm, just felt that inconsistency is less preferred than strictly following code guidelines on this one.
Will revert it regardless.

Show outdated Hide outdated xbmc/cores/VideoPlayer/VideoRenderers/VideoShaders/WinVideoFilter.h
static const float limitedRange[] = { 16.f / 255.f, (235.f - 16.f) / 255.f };
static const float *ranges[] = { fullRange, limitedRange };
return ranges[isLimitedRange];

This comment has been minimized.

@garbear

garbear Jul 23, 2017

Owner

casting a bool to int is unsafe, because a bool is anything not 0. How about return isLimitedRange ? limitedRange : fullRange;?

@garbear

garbear Jul 23, 2017

Owner

casting a bool to int is unsafe, because a bool is anything not 0. How about return isLimitedRange ? limitedRange : fullRange;?

Show outdated Hide outdated xbmc/cores/VideoPlayer/VideoRenderers/VideoShaders/WinVideoFilter.cpp
(useLimitRange ? 219.f / 255.f : 1.f)
};
m_effect.SetFloatArray("g_colorRange", colorRange, _countof(colorRange));
m_effect.SetFloatArray("g_colorRange", getColorRange(useLimitRange), 2);

This comment has been minimized.

@garbear

garbear Jul 23, 2017

Owner

To avoid the magic number, how about

const std::vector<float> &range = getColorRange(useLimitRange);
m_effect.SetFloatArray("g_colorRange", range.data(), range.size());
@garbear

garbear Jul 23, 2017

Owner

To avoid the magic number, how about

const std::vector<float> &range = getColorRange(useLimitRange);
m_effect.SetFloatArray("g_colorRange", range.data(), range.size());
@VelocityRa

This comment has been minimized.

Show comment
Hide comment
@VelocityRa

VelocityRa Jul 24, 2017

Handled your remarks, capitalized the getColorRange function and moved its implementation to the source file. Btw, the function needed to return

const std::vector<const float>&

for your last remark to compile.

VelocityRa commented Jul 24, 2017

Handled your remarks, capitalized the getColorRange function and moved its implementation to the source file. Btw, the function needed to return

const std::vector<const float>&

for your last remark to compile.

@garbear

This comment has been minimized.

Show comment
Hide comment
@garbear

garbear Jul 24, 2017

Owner

Good job on the update, now the only comment I have left is the use of m_isNearest. Is this because of the extra define when using NN?

Owner

garbear commented Jul 24, 2017

Good job on the update, now the only comment I have left is the use of m_isNearest. Is this because of the extra define when using NN?

@VelocityRa

This comment has been minimized.

Show comment
Hide comment
@VelocityRa

VelocityRa Jul 24, 2017

I think it's needed because we need to keep this state to detect/compare (Render -> HandleScalerChange) whether a relevant change in the settings has occured.
It's indeed possible that we already have this information from someplace else, however. I'll check later to make sure.

I think it's needed because we need to keep this state to detect/compare (Render -> HandleScalerChange) whether a relevant change in the settings has occured.
It's indeed possible that we already have this information from someplace else, however. I'll check later to make sure.

@VelocityRa

This comment has been minimized.

Show comment
Hide comment
@VelocityRa

VelocityRa Jul 25, 2017

Yeah m_isNearest is needed. It's the simplest solution, the alternative (if it even works) would be pretty hacky.
Edit: MSVC decided it has problems with const in the vector now. Fixed it.

VelocityRa commented Jul 25, 2017

Yeah m_isNearest is needed. It's the simplest solution, the alternative (if it even works) would be pretty hacky.
Edit: MSVC decided it has problems with const in the vector now. Fixed it.

Show outdated Hide outdated xbmc/cores/VideoPlayer/VideoRenderers/VideoShaders/WinVideoFilter.cpp
{
m_format = fmt;
m_pOutShader = pCLUT;
m_isNearest = scalingMethod == VS_SCALINGMETHOD_NEAREST;

This comment has been minimized.

@garbear

garbear Jul 28, 2017

Owner

instead of storing this as a binary value, I think m_scalingMethod makes more sense. That way, future code could depend on this logic instead of having to add a binary variable for each shader type.

@garbear

garbear Jul 28, 2017

Owner

instead of storing this as a binary value, I think m_scalingMethod makes more sense. That way, future code could depend on this logic instead of having to add a binary variable for each shader type.

Show outdated Hide outdated xbmc/cores/VideoPlayer/VideoRenderers/VideoShaders/WinVideoFilter.cpp
@@ -580,6 +595,15 @@ CYUV2RGBShader::CYUV2RGBShader()
memset(&m_texSteps, 0, sizeof(m_texSteps));
}
void CYUV2RGBShader::HandleScalerChange(ESCALINGMETHOD scalingMethod)
{
if (m_isNearest != (scalingMethod == VS_SCALINGMETHOD_NEAREST)) {

This comment has been minimized.

@garbear

garbear Jul 28, 2017

Owner

Also our coding style is { on next line

@garbear

garbear Jul 28, 2017

Owner

Also our coding style is { on next line

Show outdated Hide outdated xbmc/cores/VideoPlayer/VideoRenderers/WinRenderer.cpp
@@ -623,7 +623,7 @@ void CWinRenderer::UpdatePSVideoFilter()
}
m_colorShader = new CYUV2RGBShader();
if (!m_colorShader->Create(m_bufferFormat, m_bUseHQScaler ? nullptr : m_outputShader.get()))
if (!m_colorShader->Create(m_bufferFormat, m_bUseHQScaler ? nullptr : m_outputShader.get()), m_scalingMethod)

This comment has been minimized.

@garbear

garbear Aug 5, 2017

Owner

This line has an error, m_scalingMethod is in the wrong place.

@garbear

garbear Aug 5, 2017

Owner

This line has an error, m_scalingMethod is in the wrong place.

This comment has been minimized.

@garbear garbear referenced this pull request Aug 5, 2017

Merged

Game OSD: New dialog for video settings #12639

1 of 4 tasks complete

VelocityRa added some commits Aug 5, 2017

WinRenderer: Nearest neighbor sampling for Pixel Renderer
* I tried to change as few things as possible by using the
  color shader used in the pixel renderer to do the sampling.

* It's all compile-time and the shader gets recompiled when
  it needs to (if the sampling option changed from/to nearest
  neigbor, to/from something else).

@garbear garbear merged commit dfe1d99 into garbear:retroplayer-18alpha1 Aug 5, 2017

@VelocityRa VelocityRa deleted the VelocityRa:nearest-neighbor branch Aug 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment