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 SDL_rwops not thread-safe(?) #552

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

Android SDL_rwops not thread-safe(?) #552

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 2012-02-18 07:16:04 +0000, Ellie wrote:

Please close if invalid.

From the discussion in http://bugzilla.libsdl.org/show_bug.cgi?id=1159 and http://lists.libsdl.org/pipermail/sdl-libsdl.org/2012-February/083954.html and from own tests it seems that the Android SDL_rwops are not thread-safe.

My engine depends on threaded access to the packaged asset files and I cannot continue its port without this feature, therefore I'll choose 'blocker' - please change if not appropriate.

If this SDL_rwops thread-safety issue is truly present, when can we expect a fix? I do a lot of things with threads and I'd need lots of ugly hacks (e.g. block main thread constantly and make it read files for the sound thread or the graphics loader thread) to work around this issue which might in the end not work and cause a totally stuttering broken game, if SDL doesn't fix this.

On 2012-02-20 20:59:41 +0000, Sam Lantinga wrote:

My concern with the patch in bug 1159 is that it looks like it's attaching and detaching the java environment in each API call, which seemed extremely bad for performance. Does the approach taken there fix the threading issues for you?

There are some active Android developers on the mailing list. If you haven't recently broached this there, it might be a good idea to try it and see if anyone has suggestions on how to fix this. I don't do enough JNI programming to know what the best fix is myself.

On 2012-02-20 21:01:01 +0000, Sam Lantinga wrote:

FYI, fixing this may be as simple as using a mutex inside the file functions to protect the data there, and if that's the case, this may be an easy fix.

On 2012-02-21 02:26:21 +0000, Ellie wrote:

Would a mutex allow only one file access at once?

My sound thread keeps some files constantly opened (for streaming) and would therefore block everyone else forever if that was the case.

On 2012-02-21 11:30:14 +0000, Ellie wrote:

I attempted to use the patch from # 1159. It causes a crash when opening a window which I think worked before - probably thanks to the many manual adjustments done by me because half of the patch failed with the current code and I had to guess a lot (and probably guessed wrong).

Even if that patch offers just a slow solution, what about adding it to SDL with a compile time switch that is disabled per default so that people who need threading, like me, can enable it explicitely, while all the others keep the performance?

On 2012-03-06 12:30:03 +0000, Ellie wrote:

I could fiddle around with the patch until maybe I get it to work. Would that be helpful to you?

I tried hacking around file access from other threads than the main thread now (in my application code) which resolves the issue. However, if you imagine an SDL sound callback where .ogg is decoded from a file on disk that is constantly streamed, you can imagine what a mess hacking this file access back into the main thread is.

Any sane solution, even if involving a performance penalty, would be appreciated. Anything in SDL would be faster (and more reliable) than my current application space attempt of redirecting all file access into the busy main thread and making the other threads wait for the result - which is a code and performance nightmare.

On 2012-04-15 14:40:32 +0000, Ellie wrote:

I tried a mailing list posting now, so far it hasn't attracted JNI-savvy people: http://forums.libsdl.org/viewtopic.php?t=8076

Have you considered dropping JNI altogether? It could probably resolve this and it should be faster doing things the direct way. Android 2.3 is pretty much standard by now. I filed a separate enhancement bug for this: http://bugzilla.libsdl.org/show_bug.cgi?id=1466

Also I looked at some native NDK code in the Android docs and it looks nicer to me than the huge JNI wrapping stuff SDL is filled with right now.

On 2012-05-29 07:59:59 +0000, Gabriel Jacobo wrote:

A few comments on this issue from what I've researched...

  • Calling AttachCurrentThread on an already attached thread is a no op, and is actually the recommended method of getting the thread-exclusive JNIEnv environment. So, the first thing will be removing the static mEnv in SDL_android.cpp and replacing it for a local variable obtained via AttachCurrentThread (this is actually what I think is causing problems right now, at some point a thread is trying to use another threads JNIEnv)

  • It seems you can have multiple threads attached at the same time, I haven't seen any mention of this being an exclusive sort of operation

  • Threads have to be detached before they end, and that can be done almost automatically using pthread_key_create...how exactly I don't know :)

References:

http://developer.android.com/guide/practices/design/jni.html
https://groups.google.com/forum/?fromgroups#!topic/android-ndk/vLt3ncT3I4g
http://java.sun.com/docs/books/jni/html/invoke.html

Sam, let me know your thoughts on this, I can try to create a patch based on this asumptions and see if that solves the threading problems we've been seeing.

On 2012-06-19 16:18:46 +0000, Gabriel Jacobo wrote:

Created attachment 881
Store JNIEnv locally in each thread

Can anyone try this patch and see if it solves the problems? Thanks!

On 2012-06-21 07:55:40 +0000, Gabriel Jacobo wrote:

*** Bug 1159 has been marked as a duplicate of this bug. ***

On 2012-06-22 08:28:47 +0000, Gabriel Jacobo wrote:

Created attachment 885
Better thread handling for Android

Attaching improved patch

On 2012-06-23 07:01:26 +0000, Gabriel Jacobo wrote:

Created attachment 887
Better thread handling for Android

On 2012-07-09 14:09:55 +0000, Gabriel Jacobo wrote:

Evgeny Miroshnichenko confirmed this solves the threading issues on Android, this is now changeset http://hg.libsdl.org/SDL/rev/17840f487124

On 2012-07-20 13:28:32 +0000, Ellie wrote:

Sorry, university kept me busy. I will check the results the next three days. Thanks for the fix :-) (assuming it will work for me)

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