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

RenderCapture: Only query Occlusion if GL lower 1.5 #15351

Merged
merged 1 commit into from Feb 3, 2019

Conversation

@fritsch
Copy link
Member

fritsch commented Jan 28, 2019

See: #15345

Fixes occlusion query which is in the standard since opengl version 1.5, see: https://www.khronos.org/opengl/wiki/History_of_OpenGL#OpenGL_1.5_.282003.29

This is something for 18.1 I think

@fritsch fritsch requested a review from yol Jan 28, 2019
@yol

This comment has been minimized.

Copy link
Member

yol commented Jan 28, 2019

Mh I think that's not really correct. You are right: Occlusion queries have been merged into the standard in OpenGL 1.5. Merging into the standard means that the ARB function prefix is dropped.

From the spec:

Extensions can be promoted to required core features in later revisions of OpenGL.
When this occurs, the extension specifications are merged into the core specifica-
tion. Functions and enumerants that are part of such promoted extensions will have
the ARB, KHR, EXT, or vendor affix removed.
Implementations of such later revisions should continue to export the name
strings of promoted extensions in the EXTENSIONS string and continue to support
the affixed versions of functions and enumerants as a transition aid.

So it is recommended that the driver still reports and supports the extension. If that is the case, this change is not necessary. If that is not the case, why should we assume that the glBeginQueryARB function is still working, which is also only recommended? To be correct, you would also need to change the function calls to glBeginQuery etc.

Also didn't we want to drop pre-3.3 (compatibility) OpenGL in v19 anyway?

@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Jan 28, 2019

fritsch@L380:~/Desktop/xbmc-fritsch/xbmc$ glxinfo |grep -i occlus
    GL_ARB_multi_draw_indirect, GL_ARB_occlusion_query2, 
    GL_ARB_occlusion_query, GL_ARB_occlusion_query2, 
    GL_EXT_occlusion_query_boolean, GL_EXT_polygon_offset_clamp
@fritsch fritsch force-pushed the fritsch:eglsyncfix branch from 734ccb3 to f6858f6 Jan 28, 2019
@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Jan 28, 2019

Wanted to avoid the warning. After correctly reading the bugreport I think we can just try occlusion_query2 and don't error out if available.

@yol

This comment has been minimized.

Copy link
Member

yol commented Jan 29, 2019

Not sure what you want to say with the grep? Just cutting the lines mixes extensions from GL core and compat profile. For me, it reports GL_ARB_occlusion_query for the compat but not for core profile. Which is logical in a way since with core profile there is no possibility that occlusion queries could not be supported and to use it you have to write special code which means that there are no backwards-compatibility issues (in theory).

GL_ARB_occlusion_query2 extends GL_ARB_occlusion_query with additional functionality which we don't need or use, so checking for it also seems wrong.

@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Jan 29, 2019

@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Jan 29, 2019

What I want to fix here is performance of render capture. We disable occlusion when running with a higher profile, which is wrong. Querying for query2 will make the functionality we use, work: https://www.khronos.org/registry/OpenGL/extensions/ARB/ARB_occlusion_query2.txt see especially: "ARB_occlusion_query interacts with this extension."

@yol

This comment has been minimized.

Copy link
Member

yol commented Jan 29, 2019

To reiterate: Correct solution is to use the non-suffixed functions on GL >= 1.5 and not check for the extension. ARB_occlusion_query2 has nothing to do with the issue at hand.

@yol

This comment has been minimized.

Copy link
Member

yol commented Jan 29, 2019

IMO we could also switch to using the non-suffixed versions exclusively. Who is still running OpenGL 1.4 anyway? That was 2002....

@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Jan 30, 2019

If we do that, we don't need to check for the extension at all. I want to provide a solution for 18.1 where RenderCapture performance does not suck.

@yol

This comment has been minimized.

Copy link
Member

yol commented Jan 30, 2019

Switching to GL core functions is minimal effort, improves things for the biggest part of our user base (running on core profile) and decreases performance only for the abysmally small number of users that is still running GL 1.4 or earlier. Sounds reasonable to me.

@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Jan 30, 2019

Okay. Will do that tonight.

@fritsch fritsch force-pushed the fritsch:eglsyncfix branch 3 times, most recently from b891b61 to 7de8f0a Jan 31, 2019
@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Feb 1, 2019

jenkins build this please

@a1rwulf

This comment has been minimized.

Copy link
Member

a1rwulf commented Feb 1, 2019

See also: #14571

@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Feb 1, 2019

@a1rwulf is my code correct?

What should we do for 18.1 - I want to get occlusion working for RenderCapture. The combination of pbo and occlusion is without alternatives in my eyes :-)

@a1rwulf

This comment has been minimized.

Copy link
Member

a1rwulf commented Feb 1, 2019

@fritsch yes, Imho your code is correct.
I just wanted to mention here, that I already worked on getting rid of all other ARB usages.

@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Feb 1, 2019

@a1rwulf Would above be okay for 18.1? Do you have another idea to fix this for v18 branch?

@a1rwulf

This comment has been minimized.

Copy link
Member

a1rwulf commented Feb 1, 2019

@fritsch I'd be OK with this for 18.1 and I don't really see another solution for v18.

@fritsch

This comment has been minimized.

Copy link
Member Author

fritsch commented Feb 1, 2019

thanks @a1rwulf @pkerling fine with you, too?

@yol
yol approved these changes Feb 3, 2019
if (m_flags & CAPTUREFLAG_CONTINUOUS)
{
if (!m_occlusionQuerySupported)
CLog::Log(LOGWARNING, "CRenderCaptureGL: GL_ARB_occlusion_query not supported, performance might suffer");
CLog::Log(LOGWARNING, "CRenderCaptureGL: GL_occlusion_query not supported, performance might suffer");

This comment has been minimized.

Copy link
@yol

yol Feb 3, 2019

Member
Suggested change
CLog::Log(LOGWARNING, "CRenderCaptureGL: GL_occlusion_query not supported, performance might suffer");
CLog::Log(LOGWARNING, "CRenderCaptureGL: Occlusion queries not supported, performance might suffer");

There is no GL_occlusion_query :-)

This comment has been minimized.

Copy link
@fritsch

fritsch Feb 3, 2019

Author Member

Jep - thx.

@fritsch fritsch added this to the Leia 18.1-rc1 milestone Feb 3, 2019
@fritsch fritsch force-pushed the fritsch:eglsyncfix branch from 7de8f0a to a0de76f Feb 3, 2019
@fritsch fritsch merged commit 08e0593 into xbmc:master Feb 3, 2019
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@Rechi Rechi added the v18 Leia label Feb 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.