[android][audio] Introduce audio playlist support#42937
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
30c2bd7 to
91101cf
Compare
32ae329 to
30a187e
Compare
|
The Pull Request introduced fingerprint changes against the base commit: 4b407a7 Fingerprint diff[
{
"op": "changed",
"beforeSource": {
"type": "dir",
"filePath": "../../packages/expo-audio/android",
"reasons": [
"expoAutolinkingAndroid"
],
"hash": "f7f53d2eb5fbdbc351ffb6c3cb1dd8273bbb509a"
},
"afterSource": {
"type": "dir",
"filePath": "../../packages/expo-audio/android",
"reasons": [
"expoAutolinkingAndroid"
],
"hash": "b78b71e75b5f288dcb3392b0fcd9780f445b24d6"
}
}
]Generated by PR labeler 🤖 |
|
Subscribed to pull request
Generated by CodeMention |
There was a problem hiding this comment.
1 issue found.
About Unblocked
Unblocked has been set up to automatically review your team's pull requests to identify genuine bugs and issues.
📖 Documentation — Learn more in our docs.
💬 Ask questions — Mention @unblocked to request a review or summary, or ask follow-up questions about your code.
👍 Give feedback — React to comments with 👍 or 👎 to help us improve.
⚙️ Customize — Adjust settings in your preferences.
| override fun sharedObjectDidRelease() { | ||
| super.sharedObjectDidRelease() | ||
| appContext?.mainQueue?.launch { | ||
| playerScope.cancel() | ||
| ref.release() | ||
| } | ||
| } |
There was a problem hiding this comment.
The Player.Listener added in addPlayerListeners() (line 122) is never removed before calling ref.release(). This creates a memory leak because the listener holds a reference to the AudioPlaylist instance, preventing garbage collection.
The same pattern in expo-video (VideoPlayer.kt:360) explicitly stores and removes the listener:
player.removeListener(playerListener)
player.release()To fix this, store the listener as a property and remove it before release:
private var playerListener: Player.Listener? = null
private fun addPlayerListeners() {
val listener = object : Player.Listener {
// ... existing listener implementation ...
}
playerListener = listener
ref.addListener(listener)
}
override fun sharedObjectDidRelease() {
super.sharedObjectDidRelease()
appContext?.mainQueue?.launch {
playerListener?.let { ref.removeListener(it) }
playerScope.cancel()
ref.release()
}
}
packages/expo-audio/android/src/main/java/expo/modules/audio/AudioPlaylist.kt
Outdated
Show resolved
Hide resolved
packages/expo-audio/android/src/main/java/expo/modules/audio/AudioPlaylist.kt
Outdated
Show resolved
Hide resolved
packages/expo-audio/android/src/main/java/expo/modules/audio/AudioPlaylist.kt
Outdated
Show resolved
Hide resolved
packages/expo-audio/android/src/main/java/expo/modules/audio/AudioModule.kt
Outdated
Show resolved
Hide resolved
30a187e to
0865a65
Compare
91101cf to
06d773d
Compare
0865a65 to
5d5cd55
Compare
d9000d9 to
95bb19b
Compare
015d662 to
0fa113f
Compare
95bb19b to
0301384
Compare
|
Holy cow! I need this right now! Can playlist's audio files be randomly played? And looped (forever)? |
0fa113f to
8d7add3
Compare
27dd489 to
2810cb8
Compare
b050c86 to
b21c687
Compare
57537c2 to
a50f495
Compare
894341b to
1462a92
Compare
a50f495 to
d53e5a0
Compare
1462a92 to
4296b2c
Compare
d53e5a0 to
27a60de
Compare
Merge activity
|
27a60de to
66a70e7
Compare
|
Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines. I've found some issues in your pull request that should be addressed (click on them for more details) 👇
|
# Why Adds support for creating audio playlists on android # How Introduce a new shared object type `AudioPlaylist`. Exoplayer already has good support for playlists we just need to use the relevant methods. # Test Plan Bare expo
Why
Adds support for creating audio playlists on android
How
Introduce a new shared object type
AudioPlaylist. Exoplayer already has good support for playlists we just need to use the relevant methods.Test Plan
Bare expo