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

LinuxRendererGLES: implement hq scalers #12474

Merged
merged 4 commits into from
Sep 12, 2017
Merged

Conversation

lrusak
Copy link
Contributor

@lrusak lrusak commented Jul 12, 2017

taken from @koying PR #12357

This is not working properly yet. It can run with the fbo but the video does not get scaled properly, AKA. a 1280x720 video will only be 1280 pixels wide in a 1920 display. Everything including the gui isn't scaled properly so maybe that is a clue.

I haven't been able to track down the problem yet.

@lrusak lrusak added Type: Feature non-breaking change which adds functionality v18 Leia Component: Video WIP PR that is still being worked on labels Jul 12, 2017
@@ -53,10 +53,14 @@ CFrameBufferObject::CFrameBufferObject()

bool CFrameBufferObject::IsSupported()
{
#if HAS_GLES >= 2

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -75,12 +76,70 @@ BaseVideoFilterShader::BaseVideoFilterShader()
"gl_FragColor.a = gl_Color.a;"
"}";
PixelShader()->SetSource(shaderp);
#elif HAS_GLES >= 2

This comment was marked as spam.

m_internalformat = GL_RGBA;
#elif HAS_GLES >= 2

This comment was marked as spam.

@@ -152,6 +217,8 @@ ConvolutionFilterShader::~ConvolutionFilterShader()

void ConvolutionFilterShader::OnCompiledAndLinked()
{
BaseVideoFilterShader::OnCompiledAndLinked();

This comment was marked as spam.

@yol
Copy link
Member

yol commented Jul 12, 2017

@lrusak just cherry-picked this for wayland (yol/xbmc@1da938569), same problem - so it's not gbm. whole UI gets drawn at the video resolution when I switch to a HQ scaler. Switching back does not fix it.

@FernetMenta
Copy link
Contributor

btw: you get a much better performance if you scale the yuv image first, then do conversion to rgb.

@@ -873,7 +873,7 @@ void CLinuxRendererGLES::RenderToFBO(int index, int field, bool weave /*= false*
return;
}
//if (!m_fbo.fbo.CreateAndBindToTexture(GL_TEXTURE_2D, m_sourceWidth, m_sourceHeight, GL_RGBA))
if (!m_fbo.fbo.CreateAndBindToTexture(GL_TEXTURE_2D, m_sourceWidth, m_sourceHeight, GL_RGBA, GL_SHORT))
if (!m_fbo.fbo.CreateAndBindToTexture(GL_TEXTURE_2D, m_sourceWidth, m_sourceHeight, GL_RGBA16_EXT, GL_SHORT))

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@koying
Copy link
Contributor

koying commented Jul 13, 2017

@lrusak You miss https://github.com/xbmc/xbmc/pull/12357/files#diff-41e0ceb3de6f45669f617deae0dbeb0cR1638 for whatever reason.
That's the cause of the "corner" issue.

@lrusak
Copy link
Contributor Author

lrusak commented Jul 13, 2017

@koying thanks, that indeed fixes the issue.

@@ -1109,6 +1109,12 @@ void CLinuxRendererGL::RenderToFBO(int index, int field, bool weave /*= false*/)

if (!m_fbo.fbo.IsValid())
{
if (!g_Windowing.IsExtSupported("GL_EXT_framebuffer_object"))

This comment was marked as spam.

virtual void SetSourceTexture(GLint ytex) { m_sourceTexUnit = ytex; }
virtual void SetWidth(int w) { m_width = w; m_stepX = w>0?1.0f/w:0; }
virtual void SetHeight(int h) { m_height = h; m_stepY = h>0?1.0f/h:0; }
virtual void SetNonLinStretch(float stretch) { m_stretch = stretch; }
virtual bool GetTextureFilter(GLint& filter) { return false; }
#if HAS_GLES >= 2
virtual GLint GetVertexLoc() { return m_hVertex; }

This comment was marked as spam.

@lrusak
Copy link
Contributor Author

lrusak commented Jul 14, 2017

Updated, this now works properly with all scalers. This should be tested with GL also as it changes the format used for the 6x6 convolutions to use one with an alpha channel.

@FernetMenta
Copy link
Contributor

before testing with openGL you should address my comments. It won't have a change to get in with those ifdefs.

@lrusak
Copy link
Contributor Author

lrusak commented Jul 14, 2017

@FernetMenta yes of course. This is still very much a WIP. I'd like to clean it up as much as possible.

pYUVShader->SetField(1);
else if(field == FIELD_BOT)
pYUVShader->SetField(0);

This comment was marked as spam.

This comment was marked as spam.

@lrusak
Copy link
Contributor Author

lrusak commented Jul 19, 2017

@FernetMenta I've updated to remove much of the ifdefs. I hope that is better!

@lrusak
Copy link
Contributor Author

lrusak commented Jul 19, 2017

jenkins build this please

@@ -57,11 +57,41 @@ namespace Shaders {
GLint m_hStretch = 0;
};

class BaseVideoFilterShaderGLES : public BaseVideoFilterShaderGL

This comment was marked as spam.

};

#if HAS_GLES >= 2
using BaseVideoFilterShader = BaseVideoFilterShaderGLES;

This comment was marked as spam.

@@ -77,49 +77,116 @@ BaseVideoFilterShader::BaseVideoFilterShader()
PixelShader()->SetSource(shaderp);
}

BaseVideoFilterShaderGLES::BaseVideoFilterShaderGLES()

This comment was marked as spam.

@lrusak
Copy link
Contributor Author

lrusak commented Jul 19, 2017

I'm not going to split the file. I think that is counterproductive. If OpenGL breaks OpenGLES then it will get fixed. If it was split then OpenGLES will be forgotten about.

I'm not happy about the VAAPI change but it was the only way forward. The RendererVAAPIGL.cpp and RendererVAAPIGLES.cpp are the exact same, there is no reason for them to be split other than the fact that that's how you wanted them to be.

In my opinion the path forward is to make the OpenGL code compatible with OpenGLES. This should be done by only using OpenGLES compatible calls.

@FernetMenta
Copy link
Contributor

Without splitting the file this won't get merged. No discussion about this. You can do by yourself or wait until I'll find time to do so. The latter will delay this PR.

glMultiTexCoord2fARB(GL_TEXTURE0, 0.0f, 0.0f);
glVertex4f(m_rotatedDestCoords[0].x, m_rotatedDestCoords[0].y, 0, 1.0f );
GLint vertLoc = m_pVideoFilterShader->GetVertexLoc();
GLint loc = m_pVideoFilterShader->GetcoordLoc();

This comment was marked as spam.

}

// Setup texture coordinates
tex[0][0] = tex[3][0] = 0.0f;

This comment was marked as spam.

@@ -56,70 +56,106 @@ BaseVideoFilterShader::BaseVideoFilterShader()

m_stretch = 0.0f;

m_hVertex = -1;
m_hcoord = -1;
m_hProj = -1;

This comment was marked as spam.

@lrusak
Copy link
Contributor Author

lrusak commented Sep 8, 2017

Updated to conform to @FernetMenta 's requests.

@lrusak
Copy link
Contributor Author

lrusak commented Sep 8, 2017

jenkins build this please


if (m_floattex)
{
m_internalformat = GL_RGBA16F_EXT;

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@lrusak
Copy link
Contributor Author

lrusak commented Sep 8, 2017

I'm assuming android, rbpi, aml, and imx don't use LinuxRendererGLES can anyone confirm?

@FernetMenta
Copy link
Contributor

@koying
Copy link
Contributor

koying commented Sep 8, 2017

I'm assuming android, rbpi, aml, and imx don't use LinuxRendererGLES can anyone confirm?

Well, all GLES platforms use it in ffmpeg/software mode, really (besides maybe rpi)

@lrusak
Copy link
Contributor Author

lrusak commented Sep 8, 2017

Well, all GLES platforms use it in ffmpeg/software mode, really (besides maybe rpi)

You're right, if I disable mediacodec on android I can use the HQ scaler methods.

I've updated to not build only on RPi

NOT CORE_PLATFORM_NAME STREQUAL aml AND
NOT CORE_PLATFORM_NAME STREQUAL android AND
NOT CORE_PLATFORM_NAME STREQUAL imx))
if(OPENGLES_FOUND AND NOT CORE_PLATFORM_NAME STREQUAL rbpi)

This comment was marked as spam.

This comment was marked as spam.

CORE_PLATFORM_NAME STREQUAL gbm OR
CORE_PLATFORM_NAME STREQUAL imx OR
CORE_PLATFORM_NAME STREQUAL mir OR
CORE_PLATFORM_NAME STREQUAL wayland)

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@lrusak
Copy link
Contributor Author

lrusak commented Sep 8, 2017

Updated with the best possible solution that our build system provides.

@lrusak lrusak changed the title [WIP] LinuxRendererGLES: implement hq scalers LinuxRendererGLES: implement hq scalers Sep 8, 2017
@lrusak lrusak removed the WIP PR that is still being worked on label Sep 8, 2017
@lrusak
Copy link
Contributor Author

lrusak commented Sep 9, 2017

Rebased and squashed. Would be nice if I could get an ok from other GLES platform maintainers. ping @koying @Memphiz @peak3d

@Memphiz
Copy link
Member

Memphiz commented Sep 9, 2017

I have not the slightest amount of time for testing this currently. But given the fact that the last test definitely didn't break anything, and I am pretty sure this is still the case in the current state of this PR - you have my OK ;)

@lrusak
Copy link
Contributor Author

lrusak commented Sep 11, 2017

jenkins build this please

1 similar comment
@Rechi
Copy link
Member

Rechi commented Sep 12, 2017

jenkins build this please

CORE_PLATFORM_NAME STREQUAL gbm OR
CORE_PLATFORM_NAME STREQUAL imx OR
CORE_PLATFORM_NAME STREQUAL mir OR
CORE_PLATFORM_NAME STREQUAL wayland))

This comment was marked as spam.

This comment was marked as spam.

@lrusak
Copy link
Contributor Author

lrusak commented Sep 12, 2017

squshed, rbpi built successfully. Ready to merge 👍

@lrusak lrusak merged commit aa6c18e into xbmc:master Sep 12, 2017
@Rechi Rechi added this to the L 18.0-alpha1 milestone Sep 12, 2017
@Memphiz
Copy link
Member

Memphiz commented Sep 13, 2017

Thx for getting it through :)

@lrusak lrusak deleted the gles-hq-scalers branch February 11, 2022 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Video Type: Feature non-breaking change which adds functionality v18 Leia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants