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

Remove ARB postfix from GL functions #14571

Merged
merged 3 commits into from Jul 12, 2019

Conversation

@a1rwulf
Copy link
Member

commented Oct 10, 2018

Description

We use a lot of ARB functions that are already in standard GL.
Remove the ARB postfix where possible as it's not needed anymore.

The first commit is not intended to change behaviour.
If so, it's a mistake and I'm happy if someone tells me so.
Anyway if nobody has objections I'd squash them before merge, because
imho they belong together.

Motivation and Context

If you compile mesa with libglvnd and then link against libOpenGL.so
all ARB symbols are missing and it results in unresolved external references.
According to mesa devs it is not guaranteed in the ABI that ARB symbols
are exposed.

How Has This Been Tested?

Tested with a custom LibreELEC build.

Screenshots (if appropriate):

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)

I'd suggest to not include for 18.

@@ -173,12 +173,7 @@ bool CLinuxRendererGL::ValidateRenderTarget()
{
if (!m_bValidated)
{
//!@todo remove extension check?
if (!CServiceBroker::GetRenderSystem()->IsExtSupported("GL_ARB_texture_non_power_of_two") &&

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 10, 2018

Member

note the not operator

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Oct 10, 2018

Author Member

oh thx...

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Oct 10, 2018

Author Member

Is the non_power_of_two thingy still not in standard and has to be checked with IsExtSupported?
If so, would it be a valid solution to change it to the following?

if (!CServiceBroker::GetRenderSystem()->IsExtSupported("GL_ARB_texture_non_power_of_two")
  m_textureTarget = GL_TEXTURE_RECTANGLE;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 10, 2018

Member

no need for checking non_power_of_two. it is always supported

@a1rwulf

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2018

@FernetMenta so I've removed the GL_TEXTURE_RECTANGLE assignment.
Additionally I removed the other checks for NPOT if it's supported anyway by GL.

@@ -73,7 +73,7 @@ EShaderFormat CRendererVTB::GetShaderFormat()
bool CRendererVTB::LoadShadersHook()
{
CLog::Log(LOGNOTICE, "GL: Using CVBREF render method");
m_textureTarget = GL_TEXTURE_RECTANGLE_ARB;
m_textureTarget = GL_TEXTURE_RECTANGLE;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 10, 2018

Member

now that GL_TEXTURE_RECTANGLE is gone from the other parts, I would check if this is still ok here. I don't think that sw and vtb decoding on osx need different values.

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Oct 18, 2018

Author Member

@FernetMenta if I remove GL_TEXTURE_RECTANGLE in VTB hw rendering is broken on macos.
So I assume it's still needed.

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 18, 2018

Member

I don't think it is really needed. More likely that some other code has hardcoded this assumption. I can't think of any reason why macos should not support TEXTURE_2D

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Oct 18, 2018

Author Member

I have no macos at hand, so I can't go any further with this.
If we can remove this in VTB then I could drop a lot more code and also remove the special handling in all the shaders.
Any help is highly appreciated.

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Oct 18, 2018

Author Member

Forgot to mention:
Broken = green screen

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Oct 18, 2018

Member

confused :)
if current PR is ok on all platforms, even with GL_TEXTURE_RECTANGLE, go ahead. I just wanted to ask if this is the case.

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Oct 18, 2018

Author Member

It needs more testing, I can only test on linux atm.
But this is the last occurance of GL_TEXTURE_RECTANGLE and it's removal would break macos hw rendering.
So it would be nice if we could get rid of it.
Anyway I'll wait for v19 alpha.

@pkerling

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

@a1rwulf status?

@a1rwulf

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

@pkerling planned for v19.
Works on my NUC.
Maybe @Memphiz has some thought's on the GL_TEXTURE_RECTANGLE stuff (see outdated diffs).
Otherwise I'd say it should go into @MilhouseVH testbuilds and also needs testing on other platforms at some 19 alpha stage, so we can be sure it doesn't break stuff.

@Memphiz

This comment has been minimized.

Copy link
Member

commented Feb 1, 2019

I have zero knowledge in that area

@a1rwulf

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2019

OK, np.
Then I'd leave it as is for now.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2019

Added PR to nightly builds.

@a1rwulf a1rwulf force-pushed the a1rwulf:gl-symbols branch from c559257 to 6fbbf50 Feb 2, 2019

@a1rwulf a1rwulf force-pushed the a1rwulf:gl-symbols branch from 6fbbf50 to 4f1e176 Feb 3, 2019

@@ -44,6 +44,8 @@ bool CRenderSystemGL::InitRenderSystem()
m_RenderVersion = ver;
}

CLog::Log(LOGERROR, "CRenderSystemGL::%s - Version: %s, Major: %d, Minor: %d", __FUNCTION__, ver, m_RenderVersionMajor, m_RenderVersionMinor);

This comment has been minimized.

Copy link
@MilhouseVH

MilhouseVH Mar 13, 2019

Contributor

What is the reason to log this as an ERROR rather than NOTICE, now we get:

2019-03-13 03:40:56.390 T:140102144884032 ERROR: CRenderSystemGL::InitRenderSystem - Version: 4.5 (Core Profile) Mesa 18.3.4, Major: 4, Minor: 5

which doesn't look like an error situation too me...

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Mar 13, 2019

Author Member

Looks like a fuckup from my side.
Will change it to notice.

@a1rwulf a1rwulf force-pushed the a1rwulf:gl-symbols branch from 4f1e176 to 81650ea Mar 13, 2019

@a1rwulf a1rwulf removed the WIP label Apr 16, 2019

@a1rwulf

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

@peak3d can we include this in some android testbuilds?

@Rechi

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

@a1rwulf rebase on top of the base branch and you can download a test build for from Jenkins after the automatic PR build succeeded.

@a1rwulf a1rwulf force-pushed the a1rwulf:gl-symbols branch 2 times, most recently from c1568be to 6243d73 Apr 16, 2019

a1rwulf added 3 commits Aug 28, 2018
VideoPlayer: Rendering - replace gl ARB functions with std ones
All the ARB postfixed GL functions we use are already
part of std GL.
ARB symbols are not guaranteed to be exposed by the ABI,
which results in undefined symbols when you link against
libOpenGL.so instead of libGL.so.

With this change we can link against libOpenGL.so successfully.
VideoPlayer: LinuxRendererGL - remove unnecessary extension check
Both extensions are already part of OpenGL std as of now,
which in term makes the extension check obsolete.

@a1rwulf a1rwulf force-pushed the a1rwulf:gl-symbols branch from 6243d73 to 5790d0b Jul 11, 2019

@a1rwulf

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

I've just tested on android shorty, seems to work as well.
@Rechi Am I supposed to apply the code-formatting diff that jenkins shows?

@Rechi

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

IMO it's not necessary, as those are only line length changes.

@a1rwulf

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

OK, thanks @Rechi.
This PR is included in milhouse nightlies since a few month and I tried it on mac and android.
OpenGL is not used on windows afaik.
If nobody objects, I'll merge it tonight or tomorrow.

@a1rwulf a1rwulf merged commit 5ef06bc into xbmc:master Jul 12, 2019

1 check passed

default You're awesome. Have a cookie
Details
@Rechi

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

@a1rwulf please always set milestone before or right after merging a PR.

@a1rwulf a1rwulf added this to the Matrix 19.0-alpha 1 milestone Jul 12, 2019

@a1rwulf

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

@Rechi thx for the reminder, I usually do when creating the PR, but forgot because no Matrix milestone existed when I opened the PR.

@ksooo

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

Could it be the case that some binary addons using GL need to be recompiled after this PR? My visualisation addon - ProjectM on Android 64 - stopped working very recently and I fear it's due to this PR.

@lrusak

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

Could it be the case that some binary addons using GL need to be recompiled after this PR? My visualisation addon - ProjectM on Android 64 - stopped working very recently and I fear it's due to this PR.

Android uses GLES so my initial opinion would be no.

@ksooo

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

I just reverted the PR in my private Android 64 build. It is still building.

@ksooo

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

@lrusak @a1rwulf sorry for the noise. The PR was not the cause of my problem. I reinstalled the addon (same official binary from our repo) and now it's working again. Strange.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.