-
Notifications
You must be signed in to change notification settings - Fork 209
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
Fix MediaPlayer Position Access Issue #558
Fix MediaPlayer Position Access Issue #558
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@efstathiosntonas Thank you for your PR and this is really insightful 👀
Although the added isPlaying
check prevents inaccurate event emissions, the current implementation still schedules mTask
via mTimer
, potentially utilizing system resources even when not playing.
I think the code should be changed as below to be more idealistic.
@ReactMethod
fun pausePlayer(promise: Promise) {
...
mTimer?.cancel() // Cancel TimerTask to prevent unnecessary resource usage.
...
}
@ReactMethod
fun resumePlayer(promise: Promise) {
...
// Re-schedule TimerTask only upon resumption to ensure resource efficiency.
mTimer = Timer()
mTimer!!.schedule(mTask, 0, subsDurationMillis.toLong())
...
}
How do you think?
@hyochan your knowledge in this is better than mine since I do not know kotlin/java, I'll do whatever you think best. I've just gave it a chance by checking if the player is already running. can you give me a full example of the changes above, it's not quite clear what is the order. |
@hyochan I think we need to recreate the something like this: cancel timer on mediaPlayer!!.setOnPreparedListener { mp ->
Log.d(tag, "mediaplayer prepared and start")
mp.start()
/**
* Set timer task to send event to RN.
*/
mTask = object : TimerTask() {
override fun run() {
val obj = Arguments.createMap()
obj.putInt("duration", mp.duration)
obj.putInt("currentPosition", mp.currentPosition)
sendEvent(reactContext, "rn-playback", obj)
}
}
mTimer?.cancel() // <---- cancel timer
mTimer = Timer()
mTimer!!.schedule(mTask, 0, subsDurationMillis.toLong())
val resolvedPath = if (((path == "DEFAULT"))) "${reactContext.cacheDir}/$defaultFileName" else path
promise.resolve(resolvedPath)
} cancel timer before resuming, start a new timer task on @ReactMethod
fun resumePlayer(promise: Promise) {
if (mediaPlayer == null) {
promise.reject("resume", "mediaPlayer is null on resume.")
return
}
if (mediaPlayer!!.isPlaying) {
promise.reject("resume", "mediaPlayer is already running.")
return
}
try {
mTimer?.cancel() // <---- cancel timer on resume
mediaPlayer!!.seekTo(mediaPlayer!!.currentPosition)
mTask = object : TimerTask() { // <---- create a new timer task
override fun run() {
val obj = Arguments.createMap()
obj.putInt("duration", mediaPlayer!!.duration)
obj.putInt("currentPosition", mediaPlayer!!.currentPosition)
sendEvent(reactContext, "rn-playback", obj)
}
}
mTimer = Timer()
mTimer!!.schedule(mTask, 0, subsDurationMillis.toLong())
mediaPlayer!!.start()
promise.resolve("resume player")
} catch (e: Exception) {
Log.e(tag, "mediaPlayer resume: " + e.message)
promise.reject("resume", e.message)
}
}
@ReactMethod
fun pausePlayer(promise: Promise) {
if (mediaPlayer == null) {
promise.reject("pausePlay", "mediaPlayer is null on pause.")
return
}
try {
mediaPlayer!!.pause()
mTimer?.cancel() // <---- on pause cancel timer
promise.resolve("pause player")
} catch (e: Exception) {
Log.e(tag, "pausePlay exception: " + e.message)
promise.reject("pausePlay", e.message)
}
} |
@hyochan I've wrapped it in a try/catch, at least it will silence the error and avoid crash. I really don't know what else to do with this one. @@ -231,13 +233,19 @@ class RNAudioRecorderPlayerModule(private val reactContext: ReactApplicationCont
*/
mTask = object : TimerTask() {
override fun run() {
- val obj = Arguments.createMap()
- obj.putInt("duration", mp.duration)
- obj.putInt("currentPosition", mp.currentPosition)
- sendEvent(reactContext, "rn-playback", obj)
+ try {
+ val obj = Arguments.createMap()
+ obj.putInt("duration", mp.duration)
+ obj.putInt("currentPosition", mp.currentPosition)
+ sendEvent(reactContext, "rn-playback", obj)
+ } catch (e: IllegalStateException) {
+ // Handle the exception
+ Log.e(tag, "MediaPlayer is not in a valid state", e)
+ }
}
}
+ mTimer?.cancel()
mTimer = Timer()
mTimer!!.schedule(mTask, 0, subsDurationMillis.toLong())
val resolvedPath = if (((path == "DEFAULT"))) "${reactContext.cacheDir}/$defaultFileName" else path |
2ad8841
to
eba5d86
Compare
I have managed to handle this in c5ef0f1. Let's see what happens 🤔 |
thanks @hyochan, keep it up! |
probably fixes #557