Skip to content

fix: Small Android memory safety improvements #1930

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: Small Android memory safety improvements #1930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e117a09
perf: Use WeakReference to allow the garbage collector to reclaim the…
gnarhard Jun 6, 2025
1c38b24
perf: Nullify listeners to avoid memory leaks on release
gnarhard Jun 6, 2025
ede7028
refactor: Remove weak reference and add comment back
gnarhard Aug 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...layers_android/android/src/main/kotlin/xyz/luan/audioplayers/player/MediaPlayerWrapper.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ class MediaPlayerWrapper(
}

override fun release() {
// Remove all listeners to avoid memory leaks
mediaPlayer.setOnPreparedListener(null)
mediaPlayer.setOnCompletionListener(null)
mediaPlayer.setOnSeekCompleteListener(null)
mediaPlayer.setOnErrorListener(null)
mediaPlayer.setOnBufferingUpdateListener(null)
Copy link
Collaborator

@Gustl22 Gustl22 Jun 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I guess I would prefer its own (interface) method dispose which is only called, when the player object is newly set or set to null. Otherwise it is hard to follow and this would implicate that the listeners are not used any more when released, which should not implicitly be the case.


mediaPlayer.reset()
mediaPlayer.release()
}
Expand Down
27 changes: 17 additions & 10 deletions ...udioplayers_android/android/src/main/kotlin/xyz/luan/audioplayers/player/WrappedPlayer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package xyz.luan.audioplayers.player

import java.lang.ref.WeakReference
import android.content.Context
import android.media.AudioManager
import android.media.MediaPlayer
Expand All @@ -17,7 +18,7 @@ import kotlin.math.min
private const val MEDIA_ERROR_SYSTEM = -2147483648

class WrappedPlayer internal constructor(
private val ref: AudioplayersPlugin,
plugin: AudioplayersPlugin,
val eventHandler: EventHandler,
var context: AudioContextAndroid,
private val soundPoolManager: SoundPoolManager,
Expand All @@ -39,7 +40,7 @@ class WrappedPlayer internal constructor(
}
field = value
} else {
ref.handlePrepared(this, true)
plugin.get()?.handlePrepared(this, true)
}
}

Expand Down Expand Up @@ -104,11 +105,13 @@ class WrappedPlayer internal constructor(

var released = true

var isDisposed = false

var prepared: Boolean = false
set(value) {
if (field != value) {
field = value
ref.handlePrepared(this, value)
plugin.get()?.handlePrepared(this, value)
}
}

Expand Down Expand Up @@ -200,10 +203,12 @@ class WrappedPlayer internal constructor(
}

val applicationContext: Context
get() = ref.getApplicationContext()
get() = plugin.get()?.getApplicationContext()
?: error("Plugin reference lost in applicationContext")

val audioManager: AudioManager
get() = ref.getAudioManager()
get() = plugin.get()?.getAudioManager()
?: error("Plugin reference lost in audioManager")

/**
* Playback handling methods
Expand Down Expand Up @@ -286,7 +291,7 @@ class WrappedPlayer internal constructor(
*/
fun onPrepared() {
prepared = true
ref.handleDuration(this)
plugin.get()?.handleDuration(this)
if (playing) {
requestFocusAndStart()
}
Expand All @@ -299,7 +304,7 @@ class WrappedPlayer internal constructor(
if (releaseMode != ReleaseMode.LOOP) {
stop()
}
ref.handleComplete(this)
plugin.get()?.handleComplete(this)
}

@Suppress("UNUSED_PARAMETER")
Expand All @@ -308,15 +313,15 @@ class WrappedPlayer internal constructor(
}

fun onSeekComplete() {
ref.handleSeekComplete(this)
plugin.get()?.handleSeekComplete(this)
}

fun handleLog(message: String) {
ref.handleLog(this, message)
plugin.get()?.handleLog(this, message)
}

fun handleError(errorCode: String?, errorMessage: String?, errorDetails: Any?) {
ref.handleError(this, errorCode, errorMessage, errorDetails)
plugin.get()?.handleError(this, errorCode, errorMessage, errorDetails)
}

fun onError(what: Int, extra: Int): Boolean {
Expand Down Expand Up @@ -389,6 +394,8 @@ class WrappedPlayer internal constructor(
}

fun dispose() {
if (isDisposed) return
isDisposed = true
Copy link
Collaborator

@Gustl22 Gustl22 Jun 15, 2025

Choose a reason for hiding this comment

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

Can we somehow test this? What is the desired outcome? This should be aligned then on all platforms. I would expect it to throw, if a player is disposed twice, as this most likely is a mistake of the developer (?)

release()
eventHandler.dispose()
}
Expand Down
Loading