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

Geometry shader parameter OpenGL error #715

Closed
AnyOldName3 opened this issue Feb 25, 2019 · 18 comments
Closed

Geometry shader parameter OpenGL error #715

AnyOldName3 opened this issue Feb 25, 2019 · 18 comments

Comments

@AnyOldName3
Copy link
Contributor

On some systems (the user who reported this to me is using Mesa with a Radeon Fury), OpenGL errors are being triggered here: https://github.com/openscenegraph/OpenSceneGraph/blob/OpenSceneGraph-3.6.4-rc3/src/osg/Program.cpp#L714

Investigation has lead me to believe that it's because of the differences between the OpenGL extensions exposing geometry shaders and the core feature of GL 3.2 that does the same thing.

In the extensions (e.g. GL_EXT_geometry_shader4 and GL_ARB_geometry_shader4) parameters such as input and output type are set as program parameters, just as OSG is doing on the linked lines. However, in the core feature, they're set in the shader source itself. On systems which have the core feature, but not the older extensions, the glProgramParameteri calls are erroneous, and are causing errors to be reported.

I'm not sure how OSG handles the two different implementations of geometry shaders (if at all), but a short-term fix would be to change the final call on this line https://github.com/openscenegraph/OpenSceneGraph/blob/OpenSceneGraph-3.6/src/osg/GLExtensions.cpp#L458 from isGLExtensionOrVersionSupported to isGLExtensionSupported. It's definitely not correct to have it as isGLExtensionOrVersionSupported given that the extension and core feature are different, so the presence of the extension isn't implied by the GL version.

@AnyOldName3
Copy link
Contributor Author

In addition to that, it looks like the osg::isGLExtensionSupported(contextID,"GL_OES_geometry_shader") call is bad, too. GL_OES_geometry_shader exposes geometry shaders in the same way as the core OpenGL 3.2 feature, so that extension being present doesn't indicate that the glProgramParameteri calls should be happening, either.

@mp3butcher
Copy link
Contributor

mp3butcher commented Feb 26, 2019

use Program::setParameter as in the example osgtransformfeedback

@AnyOldName3
Copy link
Contributor Author

That won't help with this - the parameters simply don't exist in core OpenGL 3.2 or GL_OES_geometry_shader (as they're part of the GS source instead), so they shouldn't be set at all.

@AnyOldName3
Copy link
Contributor Author

Just to clarify in case it wasn't clear - the error is triggered whether or not the application actually uses any geometry shaders. Whenever OSG links a per-context program, it sets these parameters just in case a geometry shader is in use.

@emminizer
Copy link
Contributor

I think I was having a problem similar to what you're describing. It only impacted Mesa drivers. I'm using a GL 3.3 core profile context, and I got a 3.3 core profile. I use the following code to get around the problem:

  osg::ref_ptr<osg::GLExtensions> glExtensions = osg::GLExtensions::Get(contextId, true);
  if (strstr(glVersionString, "Mesa") != 0 && glExtensions.valid())
  {
    // Disable geometry shader support until GL 4.1, or explicit support for shader4
    if (glExtensions->isGeometryShader4Supported &&
      !(osg::isGLExtensionSupported(contextId, "GL_EXT_geometry_shader4") ||
        osg::isGLExtensionSupported(contextId, "GL_OES_geometry_shader") ||
        osg::isGLExtensionOrVersionSupported(contextId, "GL_ARB_geometry_shader4", 4.1f)))
    {
      std::cerr << "Applying Mesa work-around for Geometry shader support.\n";
      glExtensions->isGeometryShader4Supported = false;
    }
  }

@AnyOldName3
Copy link
Contributor Author

AnyOldName3 commented Feb 26, 2019

I don't understand why you'd make it check against OpenGL 4.1. Geometry shaders work the same there as they do in 3.2 and still don't want the parameters setting.

I'm going to write a list.

Requires setting geometry shader parameters before linking a program:

  • GL_EXT_geometry_shader4
  • GL_ARB_geometry_shader4

Requires setting geometry shader parameters within the geometry shader source:

  • The core feature of OpenGL 3.2 and later.
  • GL_OES_geometry_shader
  • GL_EXT_geometry_shader

@emminizer
Copy link
Contributor

I'm not even using Geometry shaders. I was getting an error at the same spot. Mesa is the only one that complained. I figured it was due to my lack of understanding or a driver bug -- and given that we've seen a lot of Mesa driver bugs, I put it up to Mesa.

I wasn't seeing the problem past 3.2. I didn't feel confident enough in the work-around to bring it up on the mailing list, and again it felt like a driver bug. It's been a while, so I'm going off memory -- but I was seeing OSG under Mesa reporting that it supported GL_EXT_geometry_shader4 when it did not.

Perhaps that means the core problem is in osg/GLExtensions.cpp: https://github.com/openscenegraph/OpenSceneGraph/blob/OpenSceneGraph-3.6.4-rc3/src/osg/GLExtensions.cpp#L458

It appears to set isGeometryShader4Supported when 3.2 or greater, or when GL_OES_geometry_shader. According to your text above it sounds like that's an error.

Just trying to lend a hand on the workaround we found on the various hardware we tested. Never saw the issue on GL past 4.1.

@AnyOldName3
Copy link
Contributor Author

Maybe Mesa does something weird like only supporting both geometry shader implementations with GL 4.1+, and just supporting the core version below that.

@AnyOldName3
Copy link
Contributor Author

It looks like isGeometryShader4Supported is only actually queried in that one place in the whole of OSG, so changing its meaning to is one of the geometry shader extensions with 4 in the name supported from is any kind of geometry shader supported should be safe.

@mp3butcher
Copy link
Contributor

Can you provide a test example that MESA doesn't like, please?

@AnyOldName3
Copy link
Contributor Author

Literally any OSG application that uses a shader of any kind will exhibit the issue.

@AnyOldName3
Copy link
Contributor Author

I've asked the person who reported this to me to test a 35-line test program. Being not on Mesa myself, I can't actually test it, so I'm not going to post it until I know it breaks in only the expected way.

@mp3butcher
Copy link
Contributor

mp3butcher commented Feb 27, 2019

Strange, It should be one of the latest commit as I have made some VAO experiments with openmw/osg36branch/mesa in January without any problem...
Further if you haven't reproduce the bug yourself, It would be greatly appreciated that you don't pr stuff in the wild...Haven't you got an intel graphic chipset on your motherboard?
Further if your GPU doesn't support OpenGL 3.2 (GLSL 1.50), then it doesn't support mesa added Geometry Shader support via GLSL.

@emminizer
Copy link
Contributor

If it helps @mp3butcher I was seeing the issue, which I'm pretty sure is the same one, as far back as June 2018. That's when we started our transition to GL Core profile to support these Mesa drivers that only support GL 3.3+ when in core profile. So I don't think it's impacted by your January changes.

@AnyOldName3
Copy link
Contributor Author

Further if you haven't reproduce the bug yourself, It would be greatly appreciated that you don't pr stuff in the wild...

I can read the OpenGL spec and see that what OSG is doing now is definitely not allowed and what it would be doing after the PR would be allowed. This isn't some wild shot in the dark, and doing testing on someone else's computer in a different timezone takes ages.

Strange, It should be one of the latest commit as I have made some VAO experiments with openmw/osg36branch/mesa in January without any problem...

I'm thinking that Mesa exposes different geometry shader extensions depending on either:

  • Which specific GPU is in use. The reporter is using some kind of AMD Radeon Fury. Maybe @emminizer can help work out if this is the case by saying what GPUs were in use when he saw the error.
  • Which driver backend is in use. The reporter's driver uses AMDGPU. Again, @emminizer might be able to confirm or disprove this hypothesis.
  • What OpenGL version was requested and whether it was requested in core or compatibility mode. This is fairly likely based on what @emminizer is saying, but OpenMW is just asking SDL for an OpenGL context without any specific version and definitely uses deprecated stuff, so is a compatibility context and therefore it can't be a core vs compatibility issue unless I'm misunderstanding @emminizer

With the information I have right now, I can only guarantee that I could reproduce the issue if I:

  • Bought a Radeon Fury (in case it's GPU-specific).
  • Bought another boot drive and installed Linux onto it (as all of the Linux installs I have are on laptops, so wouldn't be happy with a desktop GPU).

It might turn out to be less specific than that, but it might not. It's too much time and money to investigate a bug of this severity.

@AnyOldName3
Copy link
Contributor Author

I've got the reporter to run the test and it's enough to reproduce the error. The minimum broken example is this:

#include <osg/ShapeDrawable>
#include <osgViewer/Viewer>


static const char* vertexShader = R"GLSL(
#version 120

void main(void)
{
    gl_Position = gl_ModelViewProjectionMatrix * gl_Vertex;
}
)GLSL";

static const char* fragmentShader = R"GLSL(
#version 120

void main(void)
{
    gl_FragData[0] = vec4(1.0, 1.0, 1.0, 1.0);
}
)GLSL";

int main()
{
    osg::ref_ptr<osg::Drawable> drawable = new osg::ShapeDrawable(new osg::Sphere(osg::Vec3(0.0f, 0.0f, 0.0f), 1.0f));

    osg::ref_ptr<osg::Program> program = new osg::Program();
    program->addShader(new osg::Shader(osg::Shader::VERTEX, vertexShader));
    program->addShader(new osg::Shader(osg::Shader::FRAGMENT, fragmentShader));;

    drawable->getOrCreateStateSet()->setAttributeAndModes(program, osg::StateAttribute::ON);

    osgViewer::Viewer viewer;
    viewer.setSceneData(drawable);
    return viewer.run();
}

I expect that I'll have confirmation that #716 is enough to fix it soon.

@AnyOldName3
Copy link
Contributor Author

I now have confirmation that #716 does prevent the error being printed.

@mp3butcher
Copy link
Contributor

mp3butcher commented Feb 28, 2019

I'd prefer to exchange directly (using discord) with the original reporter (@JDGBOLT) to track this bug...
At least, he's got the hardware...
I believe to understand:
you're wanting to make geometry shader portable among implementations
but i think it's not possible with current design: either you comply with arb/ext either you comply with core fx, but both is not possible...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants