SDL threads should be enabled

[rtrussell]

SDL threads are disabled, e.g. by set(SDL_THREADS_DISABLED 1) in CMakeLists.txt, even when WebAssembly threads are enabled with -s USE_PTHREADS=1. Specifically this means that the SDL_EventQ.lock mutex is inactive, which breaks applications which push events on one thread and read them on another.

[Daft-Freak]

Upstream made it possible to enable threads for configure/cmake in libsdl-org/SDL@a81fe27. When building through ports it should be automatic

SDL2/include/SDL_config_emscripten.h

Lines 137 to 139 in 6a72595

	#ifndef __EMSCRIPTEN_PTHREADS__
	#define SDL_THREADS_DISABLED 1
	#endif

(+ stuff in https://github.com/emscripten-core/emscripten/blob/main/tools/ports/sdl2.py to build the right files)

[rtrussell]

Upstream made it possible to enable threads .... When building through ports it should be automatic

So why am I not seeing any of these changes here?

[Daft-Freak]

We're a few releases behind upstream... and that commit hasn't made it into a release yet. Also, if you're using this with -s USE_SDL=2 then you don't need it because emscripten does the build without configuring.

[rtrussell]

Also, if you're using this with -s USE_SDL=2 then you don't need it because emscripten does the build without configuring.

I updated Emscripten to 2.0.25 to pick up these changes. Big mistake! My app now results in the browser becoming unresponsive on loading (tested in Chrome) with no messages in the console to give me a clue why. My download also became a lot bigger because of sdl2_ttf pulling in harfbuzz as a dependency, which I don't want. So do you happen to know which is the earliest release that will give me the SDL2 threads update?

[Daft-Freak]

Initial thread support was added back in 1.38.31(emscripten-core/emscripten@f185044), last update I can see was in 1.38.39 (emscripten-core/emscripten@5e5e79d).

Hmm, looking at those changes I don't see the config.h for the threads build getting used... Which was probably only fixed with the new config.h in 2.0.22(emscripten-core/emscripten@ab33460#diff-1e94a1e422f5690471b6f9e8df33e4d726ffaeb7bc5bac7a2e3e26659431a192)

[rtrussell]

Hmm, looking at those changes I don't see the config.h for the threads build getting used... Which was probably only fixed with the new config.h in 2.0.22

I'm currently building with 2.0.5 and SDL2 threads are definitely not enabled. Looks like if I want that but not the SDL2_ttf Harfbuzz bloat (first in 2.0.13) I'm not going to be able to use -s USE_SDL=2 -s USE_SDL_TTF=2. For the moment I'm coping with the non-thread SDL2 by adding my own thread-protection mutex. Messy, but it works.

[Daft-Freak]

I think pre 2.0.22 emscripten would need something like this:

diff --git a/tools/ports/sdl2.py b/tools/ports/sdl2.py
index 349d8ba9e..c6bd4512e 100644
--- a/tools/ports/sdl2.py
+++ b/tools/ports/sdl2.py
@@ -293,7 +293,9 @@ sdl_config_h = r'''/* include/SDL_config.h.  Generated from SDL_config.h.in by c
 #define SDL_HAPTIC_DISABLED 1
 /* #undef SDL_LOADSO_DISABLED */
 /* #undef SDL_RENDER_DISABLED */
+#ifndef __EMSCRIPTEN_PTHREADS__
 #define SDL_THREADS_DISABLED 1
+#endif
 /* #undef SDL_TIMERS_DISABLED */
 /* #undef SDL_VIDEO_DISABLED */
 /* #undef SDL_POWER_DISABLED */
@@ -358,7 +360,9 @@ sdl_config_h = r'''/* include/SDL_config.h.  Generated from SDL_config.h.in by c
 /* #undef SDL_LOADSO_WINDOWS */
 
 /* Enable various threading systems */
-/* #undef SDL_THREAD_PTHREAD */
+#ifdef __EMSCRIPTEN_PTHREADS__
+#define SDL_THREAD_PTHREAD 1
+#endif
 /* #undef SDL_THREAD_PTHREAD_RECURSIVE_MUTEX */
 /* #undef SDL_THREAD_PTHREAD_RECURSIVE_MUTEX_NP */
 /* #undef SDL_THREAD_WINDOWS */

(I guess the Harfbuzz dependency could be made optional as it's optional in SDL2_ttf itself and SDL2_image has a similar thing with llibpng/jpeg. Haven't looked much at the ports handling of SDL2_ttf though.)

[rtrussell]

I think pre 2.0.22 emscripten would need something like this:

That's helpful, I might try it.

(I guess the Harfbuzz dependency could be made optional as it's optional in SDL2_ttf itself and SDL2_image has a similar thing with llibpng/jpeg. Haven't looked much at the ports handling of SDL2_ttf though.)

Making it optional would be great (I'm surprised it wasn't done that way).

[rtrussell]

Editing sdl2.py as suggested (I think I did it correctly) is giving this:

Traceback (most recent call last):
  File "C:\emsdk-master\upstream\emscripten\emcc.py", line 3241, in <module>
    sys.exit(run(sys.argv))
  File "C:\emsdk-master\upstream\emscripten\emcc.py", line 1989, in run
    compile_source_file(i, input_file)
  File "C:\emsdk-master\upstream\emscripten\emcc.py", line 1968, in compile_source_file
    cmd = get_clang_command(input_file)
  File "C:\emsdk-master\upstream\emscripten\emcc.py", line 1912, in get_clang_command
    return system_libs.process_args(cmd, shared.Settings)
  File "C:\emsdk-master\upstream\emscripten\tools\system_libs.py", line 1968, in process_args
    port.get(Ports, settings, shared)
  File "C:\emsdk-master\upstream\emscripten\tools\ports\sdl2_net.py", line 19, in get
    assert os.path.exists(sdl_build), 'You must use SDL2 to use SDL2_net'
AssertionError: You must use SDL2 to use SDL2_net

[rtrussell]

Editing sdl2.py as suggested (I think I did it correctly) is giving this:

That error resulted from an incomplete build (I'd done emcc --clear-ports and deleted all my object files, but it needed the .html file to be deleted as well for some reason). However building with threads enabled just gives me a version which freezes the browser on loading again (not even clicking on Chrome's close button works). So something about SDL2's threads system is blocking startup.

[Daft-Freak]

Hmm, did get a browser hang with a minimal thread test... Seems SDL waits for the thread to start after creating it and gets stuck as it's never starts. Setting -s PTHREAD_POOL_SIZE helped.

[rtrussell]

Seems SDL waits for the thread to start after creating it and gets stuck as it's never starts. Setting -s PTHREAD_POOL_SIZE helped.

Interesting. I must admit I'm very tempted to stick with what I have, because it works perfectly (as far as I can tell). That is, I'm configuring SDL2 without threads, but using them anyway, with my own Mutex to prevent concurrent access to the events queue. I expect I am risking some thread issues arising, but so far I've not noticed anything.