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

SDL_Window recreation (or rather an SDL_Renderer associated with the secondary window) fails #900

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

Comments

@SDLBugzilla
Copy link
Collaborator

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: HG 2.0
Reported for operating system, platform: Android (All), ARM

Comments on the original bug report:

On 2013-06-04 09:02:56 +0000, wrote:

Created attachment 1171
Patch

On Android, the following scenario results in a failure on step 3:

  1. Destroy an SDL_Window originally associated with an SDL_Renderer (not before the renderer itself is destroyed).
  2. Recreate a window from scratch.
  3. Finally create for it a new renderer (using a different renderer driver).

There are more scenarios where it seems to fail (like using SDL_GetWindowSurface for a window first, then replace it with another window and create a renderer for that).

Apparently, the main cause for this (possibly not the only one) is that the initial EGL context binding is not reset. It can be done using EGL10's eglMakeCurrent.

I have attached a patch which fixes this for me. It's smaller than what one may initially guess, since the code spacing changes a bit.

Another minor technical change (not directly related) is that after applying the patch, EGL surface creation is considered "failed" if an EGL context cannot be succcessfully created.

On 2013-06-04 09:04:35 +0000, wrote:

Created attachment 1172
Sample code

I have just added sample code that can be used for testing, in case someone is interested.

On 2013-06-06 12:37:09 +0000, wrote:

Oh yeah, before it gets missed:

With the patch in its current form applied, the current GL context is always destroyed and recreated. For now it won't really change much, but I guess that simple modifications can be applied to the patch if one wants to send an event exactly when this occurs.

On 2013-07-07 10:42:21 +0000, wrote:

Comment on attachment 1171
Patch

I have just marked the last patch as "obsolete", due to a recent commit Gabriel Jacobo has added to the HG tree (which really intended to fix a whole different problem). It seems to partially resolve the issue.

One limitation that still remains is that you don't have much freedom over the choice of the GL version (after a GL context has already been created). Other related GL parameters may similarly apply.

On 2013-07-12 22:15:32 +0000, Ryan C. Gordon wrote:

(Sorry if you get a lot of copies of this email, we're touching dozens of bug reports right now.)

Tagging a bunch of bugs as target-2.0.0, Priority 2.

This means we're in the final stretch for an official SDL 2.0.0 release! These are the bugs we really want to fix before shipping if humanly possible.

That being said, we don't promise to fix them because of this tag, we just want to make sure we don't forget to deal with them before we bless a final 2.0.0 release, and generally be organized about what we're aiming to ship.

Hopefully you'll hear more about this bug soon. If you have more information (including "this got fixed at some point, nevermind"), we would love to have you come add more information to the bug report when you have a moment.

Thanks!
--ryan.

On 2013-07-13 11:34:06 +0000, wrote:

Created attachment 1230
Updated patch, now handling GL context recreation

I have attached an update to the patch. After looking a bit at the code, it worths to mention a couple of warnings actually intended for various application programmers, although these may not be new at all:

  • SDL_PumpEvents should never be called after SDL_DestroyWindow and before SDL_CreateWindow (with no available window in the middle); Not even implicitly (e.g. by SDL_PollEvent).
  • In general, if the window should be recreated (say for toggling between software and GL rendering), it's safer to not do too much between the calls to SDL_DestroyWindow and SDL_CreateWindow.
  • The main reason for mentioning these is the risk of doing something wrong while the application is in some stage of a pause/resume.

====

As for the actual changes, basically the patch does the following:

  • The Java function createGLContext accepts a new boolean argument, telling if a new GL context must be (re)created or not.
  • Usually, whenever the C function SDL_GL_CreateContext is called (by either the application or the SDL library), the answer is "yes".
  • The answer is "no" on resume from a pause (i.e. when SDL_GL_CreateContext is called right from SDL_androidevents.c). In such a case the current "old" behaviors apply, namely: Use the current context if possible and create a new one otherwise (using the most recently set GL attributes).

On 2013-07-13 12:53:15 +0000, wrote:

Oh yeah, there is a minor instance of an SDL_bool being implicitly casted to a jboolean. As long as 0 (intuitively) means "false" and 1 means "true" for both types this should work...

Alternatively, the SDL_bool variable I refer to can be declared as a jboolean, but then jni.h has to be included wherever it's necessary.

On 2013-08-01 12:19:31 +0000, Sam Lantinga wrote:

Gabriel, can you look at this?

On 2013-08-01 17:44:23 +0000, Gabriel Jacobo wrote:

ny00, you reported that the fix in # 1896 solved this (partially), can you elaborate on what's left to do? Maybe update the patch if there are lingering issues? If we can save ourselves from yet another global variable that would be great! :)

On 2013-08-02 04:27:18 +0000, wrote:

First, just a short comment about the sample code: I'm not sure it worths to update it here, but, with the fix to bug # 1896 applied, the sample code will probably fail if it is modified so it creates an ES 2.0 renderer first and then an ES 1.0 one (or vice versa). It's no more than a change of a single line of code.

As for the patch itself, at this point, what's left to do is (re)create a new OpenGL context from scratch on almost every call to SDL_GL_CreateContext. As an exception to the rule, this is not the desired behavior when the app is restored from a pause (even though a context recreation may still be required).

At the moment, when an app is restored, the following sequence of function calls is done from SDL_androidevents.c:

SDL_GL_CreateContext -> Android_GL_CreateContext -> Android_JNI_CreateContext -> (Java function) createGLContext.

The problem here is finding a way to tell createGLContext if a forced context recreation is desired (usually the case) or not (when resuming from a pause).

For now, the added global variable is used for controlling that. Originally, I wasn't sure it's good to bypass the given sequence of function calls (or alternatively shorten it).

On a second thought, it may be possible to call Android_JNI_CreateContext directly, eliminating the need for another global variable. Of course, this means a new boolean should be added to Android_JNI_CreateContext as an argument. One possible problem I see is that there may be no way to access the current GL parameters in a direct manner.

On 2013-08-02 09:47:03 +0000, Gabriel Jacobo wrote:

Created attachment 1266
Implements SDL_GL_DeleteContext for Android

The attached patch implements SDL_GL_DeleteContext for Android, it should serve to accomplish a context switch by deleting the current context and letting you create a new one.. I haven't been able to test it (beyond making sure it compiles), you are welcome to try it and if you provide confirmation I'll incorporate it.

On 2013-08-03 05:46:27 +0000, wrote:

Thanks for the patch!

While it seems to improve some things, there is still a bit which is left to do. Basically, while the context may be destroyed, mEGLDisplay is not set to NULL. As a consequence, on the next call to initEGL the currently set GL parameters are ignored (and maybe more than that).

Of course, merely setting mEGLDisplay to NULL (or alternatively using EGL_NO_DISPLAY) may be insufficient. For instance, in at least one of the patch prototypes I uploaded before, any secondary call to eglInitialize is preceded by a call to eglInitialize. That may, however, be unnecessary, at least according to the following notes (assuming a proper implementation): http://www.khronos.org/registry/egl/sdk/docs/man/xhtml/eglInitialize.html

On 2013-08-03 05:53:17 +0000, wrote:

Created attachment 1267
Sample code

I have just updated the sample code. While it does not seem to get affected by my last report (having no unimplemented GL call message reported), it may still help in some way for now.

Changes: A window is first created using an ES 1.0 renderer. A second one follows with an ES 2.0 renderer. Finally a third one is created and a surface returned by SDL_GetWindowSurface is used.

On 2013-08-03 08:53:09 +0000, wrote:

Created attachment 1268
Implements SDL_GL_DeleteContext for Android

I have just updated the patch implementing. A few samples of changes (if not all):

  • EGL10.EGL_NO_DISPLAY is used wherever it's relevant, rather than null.
  • initEGL will do its job whenever mEGLContext is set to EGL10.EGL_NO_CONTEXT. If mEGLDisplay has been set before, then the older value is used.
  • A message is printed to the log when a cnotext is deleted from deleteGLContext.

The modified Java file seems to work with the sample code, although I have not yet applied the patch against a clean SDL tree, nor tested it with an actual application.

On 2013-08-03 09:14:28 +0000, wrote:

Alright, based on log messages, I believe the right things are done now!

On 2013-08-03 11:55:22 +0000, Gabriel Jacobo wrote:

This is now changeset http://hg.libsdl.org/SDL/rev/52da75545aaa

Thanks!

On 2013-11-13 19:08:55 +0000, wrote:

Re-opening since both sample codes (from 2013-06-04 and 2013-08-03) seem to fail at some points (after displaying in blue).

On 2013-11-14 12:47:53 +0000, wrote:

Created attachment 1444
Updated patch prototype for C code

I have attached a patch prototype which seems to do the job here. As expected, there is a lot that can break, especially on other platforms. See the now relocated comment about X11 in the patch for an example. The choice of arguments for SDL_EGL_LoadLibrary may further be wrong on some setups.

The idea itself, though, is as follows. Based on earlier modifications to SDLActivity.java (before the migration to common EGL code for Android and X11), the following steps should be done after a window is destroyed and upon recreating a new window:

  • If there is an EGL context, change the current context to EGL10.EGL_NO_CONTEXT and then destroy the old context.
  • Then, if there is an EGL surface, destroy it.
  • Finally, if there is an EGL display connection, terminate it using eglTerminate.

Afterwards, when a new window is created, essential resources should be recreated accordingly.

On 2013-11-14 13:05:31 +0000, Gabriel Jacobo wrote:

I see the need to destroy the surface when destroying the window, that's going in.

What I don't understand is why you moved around the "fix" to SDL_EGL_UnloadLibrary and then added an additional SDL_EGL_LoadLibrary. Before accepting that you'll have to test at least on X11.

On 2013-11-14 14:00:44 +0000, wrote:

Created attachment 1445
Another update to the patch (C code)

(In reply to Gabriel Jacobo from comment # 18)

What I don't understand is why you moved around the "fix" to
SDL_EGL_UnloadLibrary and then added an additional SDL_EGL_LoadLibrary.

You know what? It is indeed the case that SDL_EGL_UnloadLibrary does not have to be relocated at all, and so the patch is updated. Maybe I've done this so the surface is destroyed before the display connection is terminated, but it looks like the order of execution of these operations can be safely reversed.

On the other hand, the call to SDL_EGL_LoadLibrary is still required. Otherwise, eglCreateWindowSurface crashes due to lack of data.

On 2013-11-14 18:47:25 +0000, wrote:

Created attachment 1446
Third update to patch (C code)

Alright, I think that I've found the correct solution for the crash, resulting in the last update to the patch.

Basically, rather than calling SDL_EGL_UnloadLibrary directly (from SDL_EGL_DeleteContext), SDL_GL_UnloadLibrary should be called.

Explanation: The way I understand it and with the current behaviors in mind, when a new SDL_Window is created (which is always a GL window on Android), SDL_GL_LoadLibrary might be called, but it is wrongly assumed that the driver is already loaded. Due to the call to SDL_EGL_UnloadLibrary this is not the case, but SDL_GL_UnloadLibrary is not called, so there is no way to tell that. Eventually this results in a crash at the call to eglCreateWindowSurface (from SDL_EGL_CreateSurface).

On 2013-11-14 19:17:33 +0000, Gabriel Jacobo wrote:

Did you test this on X11?

On 2013-11-14 19:55:39 +0000, wrote:

(In reply to Gabriel Jacobo from comment # 21)

Did you test this on X11?

Not so far.

Based on the following info regarding the current desktop PC I'm using, it seems like it should be possible to do so, if I limit myself to 32-bit executables, and GL ES (as done on Android).

OS: Ubuntu 12.04 for x86-64.
GPU drivers: NVIDIA drivers, version 331.20.

On 2013-11-14 22:23:35 +0000, Gabriel Jacobo wrote:

You can test it with your system, you just need to find the line that says:

_this->gl_data->HAS_GLX_EXT_create_context_es2_profile = SDL_TRUE;

in SDL_x11opengl.c and comment it out, otherwise GL ES contexts will be created with GLX instead of EGL (EGL contexts in this case will be software accelerated, which shouldn't matter much in this case).

On 2013-11-14 23:15:04 +0000, Gabriel Jacobo wrote:

I moved the hack for X11 into the X11 code, and added up a few additional clean ups for good measure. Let me know if this works for you.

https://hg.libsdl.org/SDL/rev/ffb7bf531644

On 2013-11-15 08:28:06 +0000, wrote:

Thanks for the assistance. With SDL not yet updated and the last patch I've uploaded still applied, I've had some difficulties getting testgles to run.

So I've later commented out the line you've mentioned (about GLX_EXT_create_context_es2_profile) and installed a few more packages as follows:

sudo apt-get install libegl1-mesa:i386 libgles1-mesa:i386 libgles2-mesa:i386

As of now, it seems like the testgles can be run with good performance, using Gallium. If I try forcing the loading of nvidia's copy of libGLESv1_CM.so (from /usr/lib32/nvidia-331), with LD_PRELOAD, then the app crashes on a call to a GL function.

For now, though, I'd just test the latest revision from HG on Android.

On 2013-11-15 08:31:36 +0000, wrote:

Oh yeah, the fact that I've built SDL2 (with the patch) from a 32-bit chrooted enviroment may result in its own problems, even though SDL2 load certain libraries dynamically.

But if I go by the Gallium path, I can simply revert to 64-bit.

For now, though, it's Android which matters.

On 2013-11-15 09:03:06 +0000, wrote:

Alright, I confirm that the two sample apps show the expected behaviors. Thanks for the commit!

Two minor comments:

  1. Yeah, I agree that it is unnecessary to set data->egl_surface to EGL_NO_SURFACE in Android_DestroyWindow (since the data itself is going to be destroyed anyway). Any reason that's done on the Pi, though?
  2. After the update, I still wonder if the call to X11_GLES_UnloadLibrary (SDL_EGL_UnloadLibrary) from X11_GLES_DeleteContext is the right thing to do, rather than calling SDL_GL_UnloadLibrary (hence decrementing _this->gl_config.driver_loaded in SDL_video.c). However, since it is done in X11-specific code now, it is less relevant to this report.

On 2013-11-15 11:05:14 +0000, Gabriel Jacobo wrote:

Awesome, the test app seems to work fine for me too...if you want I think it could be submitted as an additional test (it will need to be cleaned up and use the common test framework).

Thanks!

On 2013-11-15 15:39:14 +0000, wrote:

(In reply to Gabriel Jacobo from comment # 28)

Awesome, the test app seems to work fine for me too...if you want I think it
could be submitted as an additional test (it will need to be cleaned up and
use the common test framework).

Thanks!

Great to hear (well read) it works for you as well!

I think that a rewrite of a whole new test would be better. The idea, of course, is the same: Cycle between various "video modes". To count all possible so-called "video modes" that I'm aware of, we have:

  1. Direct management of a GL context (using SDL_GL_CreateContext and friends).
  2. Doing things the SDL 1.2 way, with SDL_GetWindowSurface and more.
  3. The SDL_render API; Here, each possible driver should be considered its own "video mode".

Some modifications to apply, in comparison to the sample apps:

  1. Poll events in the loops.
  2. Cycle, or even randomize, the "video modes" with, say, a total of 10 tests (loops), or until some user input is detected.

What is actually done in the tests themselves should probably be simple - no more than a few calls to glClearColor/SDL_SetRenderDrawColor, glClear/SDL_RenderClear/SDL_FillRect and SDL_GL_SwapWindow/SDL_RenderPresent/SDL_UpdateWindowSurface.

Which means, there are multiple choices of the GL version. (Desktop vs ES.)

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