laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-08-09 08:04:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

Manual testing fixes:

Browse Source 
- fix(playback): remove pool from livePools after its own teardown
- fix(playback): floor decoder decrements and report teardown drift
- fix(playback): own the player across the acquire-to-register window
- fix(playback): give every pooled player exactly one owner
This commit is contained in:
davotoula
2026-07-24 19:18:34 +02:00
parent bb41a867a4
commit 0bd08bf013
2 changed files with 143 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files
amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/playerPool/ExoPlayerPool.kt
+37 -6
View File
@@ -25,6 +25,7 @@ import androidx.annotation.OptIn
import androidx.media3.common.C
import androidx.media3.common.util.UnstableApi
import androidx.media3.exoplayer.ExoPlayer
import com.vitorpamplona.amethyst.service.playback.PLAYBACK_DIAG_TAG
import com.vitorpamplona.quartz.utils.Log
import kotlinx.coroutines.CoroutineExceptionHandler
import kotlinx.coroutines.CoroutineScope
@@ -132,17 +133,22 @@ class ExoPlayerPool(
Log.d("PlaybackService") { "ExoPlayerPool discarding errored warm player: $preferredMediaId (${error.errorCodeName})" }
PcmTapRegistry.unregisterPlayer(warm)
warm.release()
liveDecoders.decrementAndGet()
releaseDecoder()
} else {
Log.d("PlaybackService") { "ExoPlayerPool warm hit: $preferredMediaId" }
// Already counted against the decoder budget for as long as it sat warm.
Log.d(PLAYBACK_DIAG_TAG) { "DECODERS warm-hit -> ${liveDecoders.get()} / budget $poolSize (unchanged)" }
return warm
}
}
}
ensureDecoderHeadroom()
// Count only once the player exists — incrementing first leaves a phantom decoder on the
// books if builder.build() throws.
val player = coldPool.poll() ?: builder.build(context)
liveDecoders.incrementAndGet()
return coldPool.poll() ?: builder.build(context)
Log.d(PLAYBACK_DIAG_TAG) { "DECODERS acquire -> ${liveDecoders.get()} / budget $poolSize" }
return player
}
/**
@@ -165,6 +171,15 @@ class ExoPlayerPool(
}
}
// Floored at zero. An under-count is the harmful direction: it lets ensureDecoderHeadroom
// hand out more concurrent decoders than the device grants, which surfaces as MediaCodec
// NO_MEMORY and "can't play this video". An over-count only costs the warm cache.
private fun releaseDecoder(): Int {
val now = liveDecoders.updateAndGet { (it - 1).coerceAtLeast(0) }
Log.d(PLAYBACK_DIAG_TAG) { "DECODERS release -> $now / budget $poolSize" }
return now
}
private fun evictOldestWarm(): Boolean {
val oldest = synchronized(warmPoolLock) { warmPool.removeFirstOrNull() } ?: return false
Log.d("PlaybackService") { "ExoPlayerPool decoder-budget evict: ${oldest.mediaId}" }
@@ -213,7 +228,7 @@ class ExoPlayerPool(
Log.d("PlaybackService") { "ExoPlayerPool dropping errored player: ${player.currentMediaItem?.mediaId} (${error.errorCodeName})" }
PcmTapRegistry.unregisterPlayer(player)
player.release()
liveDecoders.decrementAndGet()
releaseDecoder()
return@withLock
}
@@ -261,7 +276,7 @@ class ExoPlayerPool(
// stop() tears the renderers down, which is what actually hands the MediaCodec instance
// back to the system — so this is the point where the player stops costing budget.
player.stop()
liveDecoders.decrementAndGet()
releaseDecoder()
player.clearVideoSurface()
player.clearMediaItems()
@@ -307,7 +322,6 @@ class ExoPlayerPool(
}
fun destroy() {
livePools.remove(this)
scope
.launch {
mutex.withLock {
@@ -320,13 +334,30 @@ class ExoPlayerPool(
warmSnapshot.forEach {
PcmTapRegistry.unregisterPlayer(it.player)
it.player.release()
liveDecoders.decrementAndGet()
releaseDecoder()
}
coldPool.forEach {
PcmTapRegistry.unregisterPlayer(it)
it.release()
}
coldPool.clear()
// Remove from the shared registry only after this pool has decremented its own
// players, so livePools.isEmpty() becomes true only once EVERY pool has finished
// tearing down. Removing synchronously at the top of destroy() (the previous
// approach) let the first pool's body see an empty list while a sibling's
// decoders were still counted, firing a false drift warning on every ordinary
// two-pool shutdown.
livePools.remove(this@ExoPlayerPool)
if (livePools.isEmpty()) {
// Last pool out. A non-zero value here is now a genuine accounting leak —
// a player acquired and never returned — worth seeing.
val stranded = liveDecoders.getAndSet(0)
if (stranded != 0) {
Log.w(PLAYBACK_DIAG_TAG) { "decoder accounting drift at teardown: $stranded" }
}
}
}
}.invokeOnCompletion {
scope.cancel()
amethyst/src/main/java/com/vitorpamplona/amethyst/service/playback/playerPool/MediaSessionPool.kt
+106 -73
View File
@@ -25,7 +25,6 @@ package com.vitorpamplona.amethyst.service.playback.playerPool
import android.app.PendingIntent
import android.content.Context
import android.content.Intent
import android.util.LruCache
import androidx.annotation.OptIn
import androidx.core.net.toUri
import androidx.media3.common.MediaItem
@@ -46,12 +45,17 @@ import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancel
import kotlinx.coroutines.launch
import java.util.concurrent.TimeUnit
import java.util.concurrent.atomic.AtomicBoolean
import java.util.concurrent.atomic.AtomicLong
class SessionListener(
val session: MediaSession,
val playerListener: Player.Listener,
) {
// Set once, by the retire funnel. Guards the re-entrant path session.release() ->
// onDisconnected -> releaseSession -> registry.drop() landing on this same entry.
val retired = AtomicBoolean(false)
fun removeListeners() {
session.player.removeListener(playerListener)
}
@@ -123,29 +127,39 @@ class MediaSessionPool(
return if (resolved > 0) resolved else (DEFAULT_METADATA_BITMAP_DP * resources.displayMetrics.density).toInt()
}
// protects from LruCache killing playing sessions
private val playingMap = mutableMapOf<String, SessionListener>()
private val cache =
object : LruCache<String, SessionListener>(maxSessions.coerceIn(1, MAX_CACHED_SESSIONS)) {
override fun entryRemoved(
evicted: Boolean,
key: String?,
oldValue: SessionListener?,
newValue: SessionListener?,
) {
super.entryRemoved(evicted, key, oldValue, newValue)
if (!playingMap.contains(key)) {
oldValue?.let { pair ->
pair.removeListeners()
exoPlayerPool.releasePlayerAsync(pair.session.player as ExoPlayer)
pair.session.release()
}
}
}
private val registry =
SessionRegistry<SessionListener>(maxSessions.coerceIn(1, MAX_CACHED_SESSIONS)) { entry ->
retireSession(entry)
}
/**
* The one place a session's player goes back to the pool.
*
* The CAS is not redundant even though [SessionRegistry] signals each drop exactly once:
* session.release() below can reach onDisconnected -> releaseSession -> registry.drop() on
* the same entry, and that re-entrant path is what the guard stops. Do not remove it.
*
* Both orderings below are load-bearing, not tidiness:
* - removeListeners() before release(), because releasing a session can fire
* onIsPlayingChanged, which would re-enter setPlaying while we are still inside
* the registry's entryRemoved callback.
* - releasePlayerAsync() before release(), so a throw from session teardown cannot
* strand the player.
*/
private fun retireSession(entry: SessionListener) {
if (!entry.retired.compareAndSet(false, true)) return
entry.removeListeners()
exoPlayerPool.releasePlayerAsync(entry.session.player as ExoPlayer)
entry.session.release()
}
internal fun setPlaying(
id: String,
isPlaying: Boolean,
) {
registry.setPlaying(id, isPlaying)
}
@OptIn(UnstableApi::class)
fun newSession(
id: String,
@@ -156,31 +170,55 @@ class MediaSessionPool(
// is reused so the populated buffer survives. Null falls back to a cold acquire.
preferredMediaId: String?,
): MediaSession {
val mediaSession =
MediaSession
.Builder(context, exoPlayerPool.acquirePlayer(context, preferredMediaId))
.apply {
setBitmapLoader(sharedBitmapLoader)
setId(id)
setCallback(globalCallback)
}.build()
val player = exoPlayerPool.acquirePlayer(context, preferredMediaId)
val listener = MediaSessionExoPlayerConnector(mediaSession, this)
// newSession owns the player until registry.register() hands ownership over. Anything that
// throws in between — MediaSession.Builder.build(), reset(), or the PendingIntent in
// bindSessionActivity() — would otherwise strand a counted player with no owner.
//
// The throw sites named above are all *after* the session exists, so returning the player
// alone is not enough: a live MediaSession bound to it, and a listener attached to it,
// must come off first, or the pool re-issues a player that something else still holds.
var session: MediaSession? = null
var connector: MediaSessionExoPlayerConnector? = null
mediaSession.player.addListener(listener)
try {
val mediaSession =
MediaSession
.Builder(context, player)
.apply {
setBitmapLoader(sharedBitmapLoader)
setId(id)
setCallback(globalCallback)
}.build()
session = mediaSession
reset(mediaSession, keepPlaying)
val listener = MediaSessionExoPlayerConnector(mediaSession, this)
connector = listener
// Warm-pool fast path acquires a player that still holds its MediaItem, so the
// client side skips setMediaItem (see GetVideoController) and onAddMediaItems
// never fires for this fresh session — leaving the notification's tap target
// unset. Re-bind it from the player's current item so tapping the playback
// notification opens the originating nostr URI.
bindSessionActivity(mediaSession, mediaSession.player.currentMediaItem)
mediaSession.player.addListener(listener)
cache.put(mediaSession.id, SessionListener(mediaSession, listener))
reset(mediaSession, keepPlaying)
return mediaSession
// Warm-pool fast path acquires a player that still holds its MediaItem, so the
// client side skips setMediaItem (see GetVideoController) and onAddMediaItems
// never fires for this fresh session — leaving the notification's tap target
// unset. Re-bind it from the player's current item so tapping the playback
// notification opens the originating nostr URI.
bindSessionActivity(mediaSession, mediaSession.player.currentMediaItem)
registry.register(mediaSession.id, SessionListener(mediaSession, listener))
return mediaSession
} catch (e: Throwable) {
// Unwind in reverse. releasePlayerAsync only *queues* the return, so the session and
// listener are detached synchronously before the queued release ever runs — same
// rationale as the ordering inside retireSession.
exoPlayerPool.releasePlayerAsync(player)
connector?.let { player.removeListener(it) }
session?.release()
throw e
}
}
fun bindSessionActivity(
@@ -199,14 +237,15 @@ class MediaSessionPool(
}
fun releaseSession(session: MediaSession) {
val listener = playingMap.get(session.id) ?: cache.get(session.id)
if (listener != null) {
session.player.removeListener(listener.playerListener)
}
playingMap.remove(session.id)
cache.remove(session.id)
session.release()
// The registry removes the entry and then signals; retireSession does the teardown.
// Nothing here depends on a cache removal firing a callback — that dependency is what
// stranded players whose entry had already been evicted while playing.
//
// Unlike the old code this does NOT release the session when the id is unknown. Not in the
// registry means already retired (retireSession released it) or never owned, so releasing
// again would be exactly the double-release being removed here. newSession's failure path
// produces such a session.
registry.drop(session.id)
cleanupUnused()
}
@@ -217,8 +256,7 @@ class MediaSessionPool(
// CAS so only one caller actually launches the sweep when many releases fire at once.
if (!lastCleanupNs.compareAndSet(previous, now)) return
scope.launch {
val snap = cache.snapshot()
snap.values.forEach {
registry.idleSnapshot().forEach {
if (it.session.connectedControllers.isEmpty()) {
releaseSession(it.session)
}
@@ -227,16 +265,12 @@ class MediaSessionPool(
}
fun destroy() {
scope.launch {
cache.evictAll()
playingMap.forEach {
it.value.removeListeners()
exoPlayerPool.releasePlayer(it.value.session.player as ExoPlayer)
it.value.session.release()
}
playingMap.clear()
}
// Synchronous: retireSession uses the non-suspending releasePlayerAsync, so no coroutine
// is needed here. Ordering still holds — releasePlayerAsync and ExoPlayerPool.destroy()
// both launch on ExoPlayerPool's single main-thread scope and serialize on its one Mutex,
// so queued returns run first. The old version launched its teardown and then cancelled
// the scope on the same frame, so the teardown never ran at all.
registry.dropAll()
exoPlayerPool.destroy()
scope.cancel()
}
@@ -247,17 +281,17 @@ class MediaSessionPool(
context: Context,
preferredMediaId: String? = null,
): MediaSession {
val existingSession = playingMap.get(id) ?: cache.get(id)
if (existingSession != null) {
return existingSession.session
}
registry.get(id)?.let { return it.session }
return newSession(id, keepPlaying, context, preferredMediaId)
}
fun playingContent() = playingMap.values
fun playingContent() = registry.playingEntries()
fun getSession(id: String) = cache.get(id)?.session
// Widened from cache-only to cache-or-playing. Inert at its only caller
// (PlaybackService.onUpdateNotification), which is inside a `playing.isEmpty()` branch
// covering both pools, so no session is playing when it runs.
fun getSession(id: String) = registry.get(id)?.session
class MediaSessionCallback(
val pool: MediaSessionPool,
@@ -288,12 +322,11 @@ class MediaSessionPool(
val pool: MediaSessionPool,
) : Player.Listener {
override fun onIsPlayingChanged(isPlaying: Boolean) {
if (isPlaying) {
pool.playingMap.put(mediaSession.id, SessionListener(mediaSession, this))
} else {
pool.cache.put(mediaSession.id, SessionListener(mediaSession, this))
pool.playingMap.remove(mediaSession.id)
}
// Moves the existing entry. Allocating a fresh SessionListener here (the old
// behaviour) meant one session could have two or three live wrappers at once, so a
// drop of one could hand its player back while another still pointed at that live
// session.
pool.setPlaying(mediaSession.id, isPlaying)
}
}