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

Android, is "eglGetProcAddress" still busted ? #2789

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 0 comments
Closed

Android, is "eglGetProcAddress" still busted ? #2789

SDLBugzilla opened this issue Feb 11, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

SDLBugzilla commented Feb 11, 2021

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: don't know
Reported for operating system, platform: Android (All), All

Comments on the original bug report:

On 2018-01-11 09:46:16 +0000, Sylvain wrote:

On Android, eglGetProcAddress was not working ... but this has been fixed.

It was added in https://hg.libsdl.org/SDL/rev/f4b73deb9d26 as bug # 1290.
Now it's located https://hg.libsdl.org/SDL/file/00fb5966c44f/src/video/SDL_egl.c#l216

The bug report http://code.google.com/p/android/issues/detail?id=7681 says it's been fixed since Aug 19, 2010.

So maybe we remove this define.

On 2018-01-11 10:02:47 +0000, Sylvain wrote:

See https://discourse.libsdl.org/t/android-update-breaks-opengl-es-1-0/23709

On 2019-05-18 18:48:55 +0000, Ryan C. Gordon wrote:

Tagging a bunch of bugs with "target-2.0.10" so we have a clear list of things to address before a 2.0.10 release.

Please note that "addressing" one of these bugs might mean deciding to defer on it until after 2.0.10, or resolving it as WONTFIX, etc. This is just here to tell us we should look at it carefully, and soon.

If you have new information or feedback on this issue, this is a good time to add it to the conversation, as we're likely to be paying attention to this specific report in the next few days/weeks.

Thanks!

--ryan.

On 2019-05-19 18:02:47 +0000, Sam Lantinga wrote:

Sylvain, can you double check this?

On 2019-05-23 08:27:47 +0000, Sylvain wrote:

I tried to simply remove the conditional pre-processor:
"#if !defined(SDL_VIDEO_DRIVER_ANDROID)"
so that SDL_EGL_GetProcAddress() can use eglGetProcAddress.

I run a minimal test-case to use "glGetString" and display enum GL_RENDERER (0x1F01):

typedef char * (*ff_t)(int);
const char *f = "glGetString";
ff_t ret = NULL;
ret = (ff_t) SDL_GL_GetProcAddress(f);
SDL_Log("SDL load of %s, ret=%p", f, ret);
SDL_Log("SDL getstring=%s", ret(0x1F01));
  • It works on Android 9 and display some Mali-version.

  • On a Nexus 10 with Android 4.2.2 it returns a valid handle but it crashes.
    when it's nonactivated, it would display some Mali-another-version. So we can't enable this code like this.

NB: I am trying with glGetString() because this is the original issue in bug 1290.
(glGetString() is used by SDL_GL_ExtensionSupported().)

From online docs, it seems that eglGetProcAddress() should be used for extension function, not core functions.
And glGetString is a core function.

Maybe glGetString should be loaded with SDL_LoadFunction() ?
https://hg.libsdl.org/SDL/file/06fa9a25d2a0/src/video/SDL_video.c#l2939

Why do we need eglGetProcAddress if we have dlsym ?
Can't we rewrite by reversing steps:

SDL_EGL_GetProcAddress() {
1/ try SDL_LoadFunction
2/ try eglGetProcAddress
}

On 2019-06-09 01:46:37 +0000, Sam Lantinga wrote:

The spec specifically says not to use dlsym() in case the correct functions for the current context come from an internal driver table rather than globally accessible functions. This has in practice crashed when Mesa and hardware accelerated drivers are available in the same application.

From the Khronos documentation:
https://www.khronos.org/registry/EGL/sdk/docs/man/html/eglGetProcAddress.xhtml

"eglGetProcAddress may be queried for all EGL and client API functions supported by the implementation (whether those functions are extensions or not, and whether they are supported by the current client API context or not)."

On 2019-06-09 14:02:59 +0000, Sylvain wrote:

Title says it's for extensions: "eglGetProcAddress — return a GL or an EGL extension function"
but the text you quote says it's for any function.

Maybe because of the notes:
"If the EGL version is not 1.5 or greater, only queries of EGL and client API extension functions will succeed."

That would explain the original issue: querying "glGetString" was failing because EGL version was below 1.5.
Which would still be valid on my old Nexus 10 4.2.2 (which is 2012, whereas EGL 1.5 is from 2014).

On 2019-06-09 21:05:27 +0000, Sam Lantinga wrote:

Yes, that makes sense.

On 2019-10-18 04:20:31 +0000, Ryan C. Gordon wrote:

So the original Android bug was that a valid extension would be reported as available, but the entry points for it would report NULL through eglGetProcAddress(), and this was fixed a million years ago.

I suspect the hesitation after that was probably the same issue as Bug # 4794, and I believe this patch should resolve both of these bugs, allowing us to remove the #ifdef ANDROID check:

https://hg.libsdl.org/SDL/rev/2e673c68ab97

I don't have an Android device set up at the moment; Sylvain, if you get a moment, I'd appreciate if you'd try the latest in revision control with those #ifdefs removed. If it works, I'd say just commit the #ifdef removal and resolve this bug.

--ryan.

On 2019-10-18 11:51:56 +0000, Sylvain wrote:

EGL version wasn't retrieved. Fixed in https://hg.libsdl.org/SDL/rev/9bed6495d9ac

(Was added for KMS/DRM https://hg.libsdl.org/SDL/rev/cbc6a8a5b701#l6.131 )

Two devices gave me version 1.4 (One old and one recent, strange ?).

So I may be hit the case where the version cannot be retrieved with "NO_DISPLAY"
( "A display of EGL_NO_DISPLAY is supported only if the EGL version is 1.5 or greater. "
https://www.khronos.org/registry/EGL/sdk/docs/man/html/eglQueryString.xhtml )

(I copy-pasted it for offscreen, but not tested)

On 2019-10-18 12:11:40 +0000, Sylvain wrote:

So I removed the #ifdef because this isn't crashing my old android nexus10
https://hg.libsdl.org/SDL/rev/18d7c9aa4a56

The test-case:

   /* Core function */    
   {   
       typedef char * (*func_type_t)(int);
       const char *func_name = "glGetString";
       func_type_t func = NULL;
       func = (func_type_t) SDL_GL_GetProcAddress(func_name);
       SDL_Log("SDL load of %s, func=%p", func_name, func);
       if (func) {
           SDL_Log("SDL getstring=%s", func(0x1F01));
       } else {
           SDL_Log("SDL cannot load function %s", func_name);
       }   
   }   

   /* Extension function */
   {   
       /* EGLBoolean eglQueryDevicesEXT(EGLint max_devices, EGLDeviceEXT *devices, EGLint *num_devices); */
       typedef int (*func_type_t)(int, void **, int *); 
       const char *func_name = "eglQueryDevicesEXT";
       func_type_t func = NULL;
       int max_devices = 10;
       void *devices[10];
       int num_devices = 0;
       func = (func_type_t) SDL_GL_GetProcAddress(func_name);
       SDL_Log("SDL load of %s, func=%p", func_name, func);
       if (func) {
           int ret = func(max_devices, devices, &num_devices);
           SDL_Log("SDL ret=%d num_devices=%d", ret, num_devices);
       } else {
           SDL_Log("SDL cannot load function %s", func_name);
       }   
   }

On 2019-10-18 12:17:05 +0000, Sylvain wrote:

But actually SDL_LoadFunction() is a dlsym, and Sam commented:

"The spec specifically says not to use dlsym() in case the correct functions for the current context come from an internal driver table rather than globally accessible functions. This has in practice crashed when Mesa and hardware accelerated drivers are available in the same application."

On 2019-10-18 12:34:50 +0000, Ryan C. Gordon wrote:

(In reply to Sylvain from comment # 11)

But actually SDL_LoadFunction() is a dlsym, and Sam commented:

"The spec specifically says not to use dlsym() in case the correct functions
for the current context come from an internal driver table rather than
globally accessible functions. This has in practice crashed when Mesa and
hardware accelerated drivers are available in the same application."

Yes, but before we removed those ifdefs, it was exclusively using dlsym.

The hope is that 1.5 devices will do the right thing, and legacy devices will survive with the dlsym call like they have so far.

The more appropriate solution is to have a list of core symbols, and only use dlsym if it’s one of those and we aren’t on 1.5, but that’s a huge hassle if we can get away with not doing it.

--ryan.

On 2019-10-18 12:50:05 +0000, Sylvain wrote:

Yes that's true !

On 2019-10-18 13:05:52 +0000, Ryan C. Gordon wrote:

Created attachment 3988
list of GLES core symbols.

(In reply to Ryan C. Gordon from comment # 12)

The more appropriate solution is to have a list of core symbols, and only
use dlsym if it’s one of those and we aren’t on 1.5, but that’s a huge
hassle if we can get away with not doing it.

Here's the list if we want to do this, through GLES 3.2. 358 symbols.

--ryan.

On 2019-10-18 19:55:06 +0000, Sylvain wrote:

But this is too much symbols !

Only if we're <= EGL 1.4, we need to check that the name is within the set of names of <= EGL 1.4.

(there may be a lot of symbol anyway, and to be super rigorous: if we're in version 1.x (<= 4), we need to check for the set 1.x).

On 2019-10-18 20:08:09 +0000, Sylvain wrote:

From src/video/khronos/EGL/egl.h

that would be?

EGL 1.0:
eglChooseConfig
eglCopyBuffers
eglCreateContext
eglCreatePbufferSurface
eglCreatePixmapSurface
eglCreateWindowSurface
eglDestroyContext
eglDestroySurface
eglGetConfigAttrib
eglGetConfigs
eglGetCurrentDisplay
eglGetCurrentSurface
eglGetDisplay
eglGetError
eglGetProcAddress
eglInitialize
eglMakeCurrent
eglQueryContext
eglQueryString
eglQuerySurface
eglSwapBuffers
eglTerminate
eglWaitGL
eglWaitNative

1.1:
eglBindTexImage
eglReleaseTexImage
eglSurfaceAttrib
eglSwapInterval

1.2:
eglBindAPI
eglQueryAPI
eglCreatePbufferFromClientBuffer
eglReleaseThread
eglWaitClient

1.3:
nothing

1.4:
eglGetCurrentContext

On 2019-10-18 22:02:54 +0000, Ryan C. Gordon wrote:

(In reply to Sylvain from comment # 15)

But this is too much symbols !

It's only the EGL symbols themselves we're talking about? I assumed it meant "core" GLES (glActiveTexture, etc, as opposed to GLES extensions).

--ryan.

On 2019-10-19 07:03:34 +0000, Sylvain wrote:

Oops, I'm not sure, you're know better GL than me and are probably right!
But re-reading:

eglGetProcAddress — return a GL or an EGL extension function

https://www.khronos.org/registry/EGL/sdk/docs/man/html/eglGetProcAddress.xhtml

Because of the note:
"If the EGL version is not 1.5 or greater, only queries of EGL and client API extension functions will succeed."

In fact, for EGL < 1.5, it only allows:

  • "EGL" functions
    and
  • "client API extensions" function

So core EGL functions are also ok ?! So we could just check for that the prefix is "egl" ?!

On 2019-10-23 09:10:18 +0000, Sylvain wrote:

Re-reading, I think you are totally correct :)

Closing the issue because this is fixed as we could.
I removed also the android busted comment because this was solved almost 10 years ago ...
and it's not so much related (glGetString finding extension that eglGetProcAddress could not resolved)
https://issuetracker.google.com/issues/36916350
https://hg.libsdl.org/SDL/rev/fdf2fe9ca0b1

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

1 participant