Add support for ARB Program (was Irrlicht Engine bring shader compiling errors)

[kas1e]

Find out that while with the latest gl4es we didn't have anymore "ARGB" errors when it comes to Irrlicht, but all Irrlicht based code brings me on running that now:

LIBGL: Hardware vendor is A-EON Technology Ltd. Written by Daniel 'Daytona675x' MьЯener @ GoldenCode.eu
LIBGL: GLSL 300 es supported
LIBGL: GLSL 310 es supported and used
Using renderer: OpenGL 2.1
GL4ES wrapper: ptitSeb
OpenGL driver version is 1.2 or better.
GLSL version: 1.2
Vertex shader compilation failed at position 585:
Invalid token
Pixel shader compilation failed at position 580:
Invalid value (implicit param?)
Vertex shader compilation failed at position 605:
Invalid token
Pixel shader compilation failed at position 586:
Invalid value (implicit param?)
Loaded texture: cpsplash.png 

And then all continue to work as expected.

[kas1e]

@ptitSeb Just to make you know, we almost find out the problem.

If you take a look on that source code, we have logic like : static SDL_bool InitShaders() -> CompileShaderProgram() -> CompileShader().

Now, as i say adding prinfs/delay after first SDL call in InitShaders() but before calling to CompileShaderPRogramm() fix the compilation. Next, i remove those prinfs/delays from InitShaders, and start adding them to CompileShaderProgramm() , and find out, that things "fixes" when we add it before calling to CompileShader(). But what is funny, only from TOP called functions (which call the CompileShader() ), adding prinfs inside of CompileShader() fix nothing and shaders can't be compiled.

So that already suggest about some issues with stack / variables / corruption / unialization because calling functions from another functions change the stack layout and overwrite probabaly unitialized variables.

So, then we find out that glGetObjectParameterivARB(shader, GL_OBJECT_COMPILE_STATUS_ARB, &status); in CompileShader always return "bad" value for the first time. If i didn't add prinfs/delays on top function (so, no stack change/overwrite happens) ,then the first return of status always 0 ! If i add prinfs on top level function, stack changes, and then, first status value returned are some big shit like 167362927263 (our "status" is "int" so looks very much corrupted /unitialized variable). So for us it works with prinfs by some luck, because we overwrite unitialized 0, and code didn't come to the check if(status ==0) {..}.

We then tried that:

status = 1234;

glGetObjectParameterivARB(shader, GL_OBJECT_COMPILE_STATUS_ARB, &status);

printf("status = %d\n",status);

And result are:

status = 1234
status = 1
status = 1
status = 1
status = 1
status = 1

See, first return from glGetObjectParameterivARB() return nothing which point out us on some unitialized variable/attribute somewhere. And it may works on other platforms because of different stack layouts, etc, but issues can be not only amigaos4 related (but that i not sure of course at the moment, as most of time it ends up still in amigaos4, will see).

So, while for now i can't say what wrong exactly, but we very close to find issue that for sure. But so far looks like something with glGetObjectParameterivARB().

[kas1e]

@ptitSeb Btw cat you remind a little how the lookup of fucntions happens in gl4es ?

I mean, we have agl/amigaos.c file in which we statically point out where to take OGLES2 functions for GL4ES. Now, we have agl/lookup.c file in which we describe how to load up in gl4es the function we need, i.e. for us important are:

    MAP("glXGetProcAddress", gl4es_aglGetProcAddress);
    MAP("glXGetProcAddressARB", gl4es_aglGetProcAddress);

So, for both getprocaddress be it "pure" strandard opengl functions, or ARB extensions, we use gl4es_aglGetProcAddress.

Now we also have gl/getter.c in which we BuildExtensionsList() and so our ARB extensions in as well now.

And we also have in gl/buffers.c that:

#ifndef AMIGAOS4
void glGenBuffersARB(GLsizei n, GLuint * buffers) AliasExport("gl4es_glGenBuffers");
void glBindBufferARB(GLenum target, GLuint buffer) AliasExport("gl4es_glBindBuffer");
void glBufferDataARB(GLenum target, GLsizeiptr size, const GLvoid * data, GLenum usage) AliasExport("gl4es_glBufferData");
void glBufferSubDataARB(GLenum target, GLintptr offset, GLsizeiptr size, const GLvoid * data) AliasExport("gl4es_glBufferSubData");
void glDeleteBuffersARB(GLsizei n, const GLuint * buffers) AliasExport("gl4es_glDeleteBuffers");
GLboolean glIsBufferARB(GLuint buffer) AliasExport("gl4es_glIsBuffer");
void glGetBufferParameterivARB(GLenum target, GLenum value, GLint * data) AliasExport("gl4es_glGetBufferParameteriv");
void *glMapBufferARB(GLenum target, GLenum access) AliasExport("gl4es_glMapBuffer");
GLboolean glUnmapBufferARB(GLenum target) AliasExport("gl4es_glUnmapBuffer");
void glGetBufferSubDataARB(GLenum target, GLintptr offset, GLsizeiptr size, GLvoid * data) AliasExport("gl4es_glGetBufferSubData");
void glGetBufferPointervARB(GLenum target, GLenum pname, GLvoid ** params) AliasExport("gl4es_glGetBufferPointerv");
#endif

So question is:

1). how gl4es load up the ARB shaders, and how SDL_GL_GetProcAddress() got the "right" addresses on them. I mean, internally in meaning of GL4ES.

2). Why we still have that ifndef AMIGAOS4 there ? I just uncommenting it out, and all works seems the same.

What we trying to find now, is why the GetObjectParameterivARB() function call to check compile status fails at least in first call for some reason (either function buggy or since it is a function pointer, it is not pointing to correct function). And the biggest fail is that it does not set status variable at all which should never happen. That's why the status variable stays undefined after first call and why it "fixes" with magic printf() or other function calls in parent function or by setting it to a defined value (like 1234) before call to GetObjectgParamterivARB().

[ptitSeb]

I don't remember why there is an ifdef there Looking at the history, the ifdef has been added with commit a9b9112a00569f2b4dfb2aa33e9c2ce464ae16c6 "[AMIGAOS4] Don't define buffers ARB function (still accesssible with aglGetProcAddress)" by me. So I probably add this at your request, as I don't do stuff on AMIGAOS4 by myslef as I cannot test. And looking in my mails, this was because of #143 (2 years ago)

[ptitSeb]

For your first question, what do you mean with "how gl4es load up the ARB shaders"?

For SDL_GL_GetProcAddress(..) this function should use aglGetProcAddress(..) This function is split in 2 parts: the Amiga specific one is in agl/lookup.c (this is mostly the "agl" version of the linux "glX" function), and the generic one (that contains all the shaders functions and more) defined in gl/gl_lookup.c

[kas1e]

For your first question, what do you mean with "how gl4es load up the ARB shaders"?

I mean is there any differences in terms of gl4es how the usuall functions loads up and how extensions loads up. Because from the side of SDL, for pure functions i need add nothing, but for ARB ones, i need all those:

static PFNGLATTACHOBJECTARBPROC glAttachObjectARB;
static PFNGLCOMPILESHADERARBPROC glCompileShaderARB;
...

        glAttachObjectARB = (PFNGLATTACHOBJECTARBPROC) SDL_GL_GetProcAddress("glAttachObjectARB");
        glCompileShaderARB = (PFNGLCOMPILESHADERARBPROC) SDL_GL_GetProcAddress("glCompileShaderARB");
...

So it looks like that loading are different anyway and for extensions something more done by code.

For SDL_GL_GetProcAddress(..) this function should use aglGetProcAddress(..)

Yeah, i see that SDL_GL_GetProcAddress(..) for us it just:

void *OS4_GL_GetProcAddress(_THIS, const char * proc)
{
    void *func = NULL;

    dprintf("Called for '%s'\n", proc);

    func = (void *)aglGetProcAddress(proc);

    if (func == NULL) {
        dprintf("Failed to load '%s'\n", proc);
        SDL_SetError("Failed to load function");
    }

    return func;

}

I just tried to undestand what can make glGetObjectParameterivARB(shader, GL_OBJECT_COMPILE_STATUS_ARB, &status); fail for only the first time, and why it even didn't initialize status variable at all by default.

It looks like we have some bug in amigaos specific part , maybe our SDL_GL_GetProcAddress(..) are buggy .. But it so small, just one string, what can go wrong there ..

[ptitSeb]

ARB and EXT are optionnal. If a program where linking directly with the symbols, any computeur with driver that doesn't have the symbol would fail to load the program because the function would not be found... making it not optionnal... That why all the extension are behind "GetProcAddress" function call. So to use them, you must create variable of type "function pointer", and initialize them with GetProcAddress result.

the aglGetProcAddress is very simple, and the common to all architecture. It's a simple if(!strcmp(name, "BLAHBLAH") return gl4es_BLAHBLAH; for all the functions names...

[kas1e]

@ptitSeb We still trying to figure out wtf happens, and one of our devs who works on SDL port, has been staring gl4es code a lot but since he can't be sure what functions are called its guesswork. For example, glGetObjectParameterivARB call branches either to glGetShaderiv or glGetProgramiv. According to gl4es code status param should be passed to OGLES2 as such so the logically uninitialized value should be coming from OGLES2. But, when he tested OGLES2, surely status values for glGetShaderiv call were set in case of failure and success.

Can you clarify a bit maybe about?

Thanks!

[kas1e]

@ptitSeb Some more about from one of our devs:

gl4es seems to think that the passed shader object is a program object and then it calls glGetProgramiv with GL_COMPILE_STATUS which triggers GL_INVALID_ENUM and uninitialized status, because "If an error is generated, no change is made to the contents of params." - https://www.khronos.org/registry/OpenGL-Refpages/es2.0/xhtml/glGetProgramiv.xml

Strange why on Pandora all good then ..

[kas1e]

@ptitSeb Good news , we find out what is it, see full explain:

By default in this test case as you can see GLint status; is unitialized. Which mean that usualy it 0, but can be anything once stack is changed. So, the code which check that status are:

static SDL_bool CompileShader(GLhandleARB shader, const char *source)
{

    GLint status;

    glShaderSourceARB(shader, 1, &source, NULL);
    glCompileShaderARB(shader);

    glGetObjectParameterivARB(shader, GL_OBJECT_COMPILE_STATUS_ARB, &status);

    if (status == 0) {      
        GLint length;
        char *info;

        glGetObjectParameterivARB(shader, GL_OBJECT_INFO_LOG_LENGTH_ARB, &length);
        info = SDL_stack_alloc(char, length+1);
        glGetInfoLogARB(shader, length, NULL, info);

        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Failed to compile shader:\n%s\n%s", source, info);
        SDL_stack_free(info);

        return SDL_FALSE;
    } else {
        return SDL_TRUE;
    }
}

See, there is check on if (status == 0) {, but on nothing else. If 0, then no succes, all other mean success. But now as status wasn't initialized, and when we add "prinfs" in the code, then code area of unitialized values were rewritten by random crap like 121312312412412, and then, this if (status == 0) { was correct and things start to works, while of course, "status" was still wrong. It just wrong status check a bit loose our time to understand wtf.

But reall issue are glGetObjectParameterivARB(shader, GL_OBJECT_COMPILE_STATUS_ARB, &status); it just didn't works for us on the first pass (but works for other ones). So, Capehill (who do SDL2 port for us) start debugging stuff, and find out things i wrote in previous post, but then he checked this testshader on Linux (Mesa) and found that programs and shaders share the id pool there:

program 1
vert_shader 2
frag_shader 3
program 4
vert_shader 5
frag_shader 6
program 7
vert_shader 8
frag_shader 9

Next, he suggest that we need to check OpenGL ES 2.0 specs what is said about program/shader id numbers and if program and shader IDs need to use the same pool, then it should be fixed in OGLES2. If not, then maybe gl4es needs to be changed.

But then, Daniel says that standard says nothing about, and ogles2.lib uses dedicated pools which complies to the standard. But, this was incompatible with GL4ES' glGetObjectParameterARB API. Therefore he modified ogles2.library. From the change-log:

New context creation tag OGLES2_CCT_UNIQUE_SHADER_AND_PROGRAM_IDS.
This is a helper tag to allow APIs like GL4ES' glGetObjectParameterARB to work with ogles2.library.
This API asumes that the shader- and program-IDs generated by glCreateShader and glCreateProgram share the same ID pool. Note that this rule does not apply to OpenGL ES 2 (and not even 3 AFAIK).
When this tag is set to TRUE then ogles2.lib will essentially half the ID-space for shaders and programs and return values from the lower half for programs (ID & 0xFF < 128) and values from the upper half for shaders (ID & 0xFF >= 128).
Note that TRUE has been made the default value for that new tag for convenience. That way GL4ES doesn't have to be rebuild.

Now things works , but question remain : why it works on Pandora then ? Should't be something changes in GL4ES in that terms ? Or, it all just matter of implementation, and Mesa do it just differently than we, while, there is nothing about in standard ?

[ptitSeb]

I don't undestand: on Amiga, a program and a shader can have the same ID? If that's the case, than there can be some issue with old ARB type program API yes, because for each ID it receive, it shecks if a program that have that ID exist to now if it's a program or a shader. So if a shader have the same ID as a program, you'll never be able to grab is parameters with this functions.

You should note that this function glGetObjectParameterivARB(...) is to be used with old shader programs. The one writen is a assembly-like lankage, not the new GLSL programs.

[kas1e]

@ptitSeb That what Danel says:

As I said: there's no word about that in the OpenGL ES 2 specs (unless I'm totally blind). So an implementation may chose whichever ID generation algorithm it may like. There is no rule that shader-IDs and program-IDs are related in any way. Which is logical because there's no critical function like glGetObjectParameters in GLES2.

So it was no bug in ogles2.lib.

It's a bug in GL4ES because it asumes something in its OpenGL ES 2 based implementation which the OpenGL ES 2 standard doesn't guarantee. GL4ES was just lucky so far with those other GLES2 implementations which apparently chose to implement ID generation differently.

To fix GL4ES the following would have to be done:

- glCreateShader and glCreateProgram functions must be wrapped and the IDs returned by the respective ogles2 functions must be mapped to GL4ES-internal shared IDs. Those other IDs must then be returned to the caller.

- in each function which accepts a shader- or program-ID the reverse mapping must be done and then the original ID must be passed to ogles2.

Because it was easy for me to artificially restrict ogles2.lib so that it creates IDs which work with GL4ES, I chose to do so. It also has no performance penalty. Fixing GL4ES is certainly a bit harder than that and would require this extra ID translation.
The bug in GL4ES persists of course, but it doesn't affect the ogles2.lib-code-path anymore so it doesn't matter to us anymore.

[ptitSeb]

Yes, this is all correct. I'll see to fix those functions later on gl4es.

[kas1e]

That was interesting issue anyway , now will start to dig in into scummvms issues from this one https://github.com/ptitSeb/gl4es/issues/366

Thanks for help!

[kas1e]

@ptitSeb It turns out that we may wrong, Georg found in https://www.khronos.org/registry/OpenGL/specs/es/2.0/es_full_spec_2.0.pdf, in the chapter "2.10.1:

"The name space for shader objects is the unsigned integers, with zero reserved for the GL. This name space is shared with program objects"

So in end of all it seems gl4es all ok in that terms, it just was issue on our side.

[ptitSeb]

Ah, good news (for me) then :)