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 optimization for Attach/Detach from C callbacks #243

Merged
merged 1 commit into from
Jul 12, 2018

Conversation

tmm1
Copy link
Contributor

@tmm1 tmm1 commented Jul 11, 2018

fixes #242

@tmm1 tmm1 changed the title [WIP] Android optimization for Attach/Detach from C callbacks Android optimization for Attach/Detach from C callbacks Jul 11, 2018
@tmm1
Copy link
Contributor Author

tmm1 commented Jul 11, 2018

I tested this on my Android device and it works as expected.

@saudet
Copy link
Member

saudet commented Jul 12, 2018

Looks good, thanks!! Although I'm not sure I see why the lock is needed... Anyway, let's prefix the static variables to avoid name clashes with libraries the user might be trying to wrap, something like JavaCPP_current_env, JavaCPP_once, and JavaCPP_lock?

@tmm1
Copy link
Contributor Author

tmm1 commented Jul 12, 2018

I think the lock is needed around the getspecific/setspecific.

Renaming the statics makes sense, I can do that.

@saudet
Copy link
Member

saudet commented Jul 12, 2018

That's what I'm thinking, or maybe the once guy? It doesn't make sense that either of those would not be thread-safe though, and it's not in the docs, but anyway if that's what's needed according to FFmpeg developers, let's just do that.

Yes, please change the names, thanks!

@tmm1
Copy link
Contributor Author

tmm1 commented Jul 12, 2018

The pthread_once likely does not need to be inside the lock, though it was in the ffmpeg code I copied from.

The lock is needed around the get/set only, since that's two separate operations that could race.

@saudet
Copy link
Member

saudet commented Jul 12, 2018

This is a thread-specific variable, there can't be any race conditions. There must be some other reason though.

@tmm1
Copy link
Contributor Author

tmm1 commented Jul 12, 2018

Yes, please change the names, thanks!

Done.

@tmm1
Copy link
Contributor Author

tmm1 commented Jul 12, 2018

cc @mbouron in case he remembers the reason for the lock

@saudet saudet merged commit 428c79d into bytedeco:master Jul 12, 2018
@tmm1 tmm1 deleted the android-auto-detach branch July 12, 2018 18:10
@egolearner
Copy link
Contributor

egolearner commented Jun 1, 2021

I think pthread mutex in ffmpeg is used to protect java_vm global variable.

https://github.com/FFmpeg/FFmpeg/blob/575e52272d42f4278c6620f1a999c41425db2094/libavcodec/ffjni.c#L51-L72

In javacpp, JavaCPP_vm is set elsewhere so it's unnecessary to use pthread mutex.

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

Successfully merging this pull request may close these issues.

FunctionPointer performance suffers due to repeated AttachCurrentThread/DetachCurrentThread
3 participants