Skip to content

Enable SDL2 threads#8301

Merged
kripken merged 4 commits intoemscripten-core:incomingfrom
jakogut:sdl2-threads
Mar 27, 2019
Merged

Enable SDL2 threads#8301
kripken merged 4 commits intoemscripten-core:incomingfrom
jakogut:sdl2-threads

Conversation

@jakogut
Copy link
Copy Markdown
Contributor

@jakogut jakogut commented Mar 17, 2019

Closes #8300

sbc100
sbc100 approved these changes Mar 18, 2019
View reviewed changes
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@jakogut
Copy link
Copy Markdown
Contributor Author

jakogut commented Mar 18, 2019

@sbc100 Hmm, it seems that this PR causes undefined symbols when USE_PTHREAD is not specified.

>> emcc tests/sdl2_audio_beep.cpp -o sdl2_audio_beep.html -s USE_SDL=2
error: undefined symbol: pthread_getschedparam
warning: To disable errors for undefined symbols use `-s ERROR_ON_UNDEFINED_SYMBOLS=0`
error: undefined symbol: pthread_setcanceltype
error: undefined symbol: pthread_setschedparam
error: undefined symbol: sched_get_priority_max
error: undefined symbol: sched_get_priority_min
Error: Aborting compilation due to previous errors
shared:ERROR: '/usr/bin/node /usr/lib/emscripten/src/compiler.js /tmp/tmpTonzPk.txt /usr/lib/emscripten/src/library_pthread_stub.js' failed (1)

I added a commit to create a separate -mt version of libSDL2.bc.

sbc100
sbc100 reviewed Mar 18, 2019
View reviewed changes
Comment thread tools/ports/sdl2.py Outdated
@jakogut jakogut force-pushed the sdl2-threads branch 2 times, most recently from 6f77522 to d2e65e6 Compare March 18, 2019 19:47
kripken
kripken reviewed Mar 18, 2019
View reviewed changes
Comment thread tools/ports/sdl2.py
# get the port
ports.fetch_project('sdl2', 'https://github.com/emscripten-ports/SDL2/archive/' + TAG + '.zip', SUBDIR)
libname = ports.get_lib_name('libSDL2')
libname = ports.get_lib_name('libSDL2' + ('-mt' if settings.USE_PTHREADS else ''))
Copy link
Copy Markdown
Member

@kripken kripken Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs integration in embuilder.py, so that we can build the -mt version there too.

@kripken
Copy link
Copy Markdown
Member

kripken commented Mar 18, 2019

I'm not sure what those test errors are on CI - hopefully they reproduce locally for easy debugging. If not let us know.

@jakogut
Copy link
Copy Markdown
Contributor Author

jakogut commented Mar 19, 2019 •
edited
Loading

@kripken I've added sdl2-mt to embuilder. The CI issues appear to be because the build process was attempting to build libSDL2-mt.bc at compile time, and FROZEN_CACHE was set. Adding sdl2-mt to embuilder resolved this.

Some other failed tests appear to be from unresponsiveness, which led to the rest of the tests being skipped. I'm not sure those if are related.

@jakogut
Copy link
Copy Markdown
Contributor Author

jakogut commented Mar 19, 2019

Nope, it appears related. The JS console is showing that the maximum call stack size has been exceeded.

@jakogut
Copy link
Copy Markdown
Contributor Author

jakogut commented Mar 19, 2019 •
edited
Loading

Found the problem. The non-multithreaded SDL2 variant was being built with the same pthread backend enabled config as the multithreaded variant.

In addition to creating a separate library for enabling threads, we also need to not enable threads in the normal build. :)

@kripken
Copy link
Copy Markdown
Member

kripken commented Mar 20, 2019

I still see 3 failing tests in test-browser-firefox, do you know what's going on there?

@jakogut
Copy link
Copy Markdown
Contributor Author

jakogut commented Mar 22, 2019

Hmm, the failing tests are test_webgl_context_attributes, test_webgl_context_params, and test_webgl_offscreen_canvas_in_proxied_pthread. I'm not sure how these could be related to this pull request. None of these use SDL2.

@jakogut
Copy link
Copy Markdown
Contributor Author

jakogut commented Mar 22, 2019

Hmm, I rebased on origin/incoming and force pushed, and all the CI tests are passing now. ¯_(ツ)_/¯

@jakogut
Copy link
Copy Markdown
Contributor Author

jakogut commented Mar 26, 2019

It's also worth mentioning that any program using SDL2 windowing and graphics, rather than just threading, requires the fixes from this PR to work. emscripten-ports/SDL2#77

@kripken
Copy link
Copy Markdown
Member

kripken commented Mar 27, 2019

Great, thanks @jakogut! I guess those errors were random CI problems then.

@kripken kripken merged commit f185044 into emscripten-core:incoming Mar 27, 2019
@jakogut jakogut deleted the sdl2-threads branch March 27, 2019 19:04
VirtualTim pushed a commit to VirtualTim/emscripten that referenced this pull request May 21, 2019
@jakogut @VirtualTim
Enable SDL2 threads (emscripten-core#8301)
f54f621 
Closes emscripten-core#8300
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
@VirtualTim
Revert "Enable SDL2 threads (emscripten-core#8301)"
45266e9 
This reverts commit f54f621.
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
@VirtualTim
Revert "Enable SDL2 threads (emscripten-core#8301)"
3182932 
This reverts commit f54f621.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kripken kripken kripken left review comments

@sbc100 sbc100 sbc100 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jakogut @kripken @sbc100