diff --git a/android/app/src/androidTestDebug/kotlin/com/edde746/plezy/mpv/MpvLifecycleDeviceTest.kt b/android/app/src/androidTestDebug/kotlin/com/edde746/plezy/mpv/MpvLifecycleDeviceTest.kt index 12e9b3968..b90483c60 100644 --- a/android/app/src/androidTestDebug/kotlin/com/edde746/plezy/mpv/MpvLifecycleDeviceTest.kt +++ b/android/app/src/androidTestDebug/kotlin/com/edde746/plezy/mpv/MpvLifecycleDeviceTest.kt @@ -2,9 +2,13 @@ package com.edde746.plezy.mpv import android.app.Instrumentation import android.content.Intent +import android.graphics.Color import android.os.Handler import android.os.Looper import android.os.SystemClock +import android.view.SurfaceHolder +import android.view.SurfaceView +import android.view.View import android.view.ViewGroup import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry @@ -13,6 +17,7 @@ import java.io.File import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicReference +import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Test @@ -21,23 +26,64 @@ import org.junit.runner.RunWith @RunWith(AndroidJUnit4::class) class MpvLifecycleDeviceTest { @Test - fun repeatedMediaCodecPlaybackCompletesTerminalTeardown() { + fun repeatedMediaCodecPlaybackCompletesTerminalTeardown() = runPlaybackTest(recreateSurfaces = false) + + @Test + fun pausedMediaCodecPlaybackSurvivesRepeatedSurfaceRecreation() = runPlaybackTest(recreateSurfaces = true) + + @Test + fun pausedGpuPlaybackSurvivesRepeatedSurfaceRecreation() = runPlaybackTest(recreateSurfaces = true, hardwareDecoding = false) + + @Test + fun placeholderConsumesFramesWithoutBlockingProducer() { + val placeholder = runBlocking { MpvPlaceholderSurface.create() } + val completed = CountDownLatch(1) + val failure = AtomicReference() + val producer = Thread { + try { + // More than a BufferQueue can retain without an active consumer. + repeat(16) { + val canvas = placeholder.surface.lockCanvas(null) + canvas.drawColor(Color.BLACK) + placeholder.surface.unlockCanvasAndPost(canvas) + } + } catch (error: Throwable) { + failure.set(error) + } finally { + completed.countDown() + } + }.apply { isDaemon = true } + try { + producer.start() + assertCompletes(completed, "placeholder buffer consumption", 0) + failure.get()?.let { throw AssertionError("Placeholder producer failed", it) } + } finally { + placeholder.close() + producer.join(2_000) + assertTrue("Placeholder producer survived cleanup", !producer.isAlive) + } + } + + private fun runPlaybackTest(recreateSurfaces: Boolean, hardwareDecoding: Boolean = true) { val instrumentation = InstrumentationRegistry.getInstrumentation() val fixtureBytes = instrumentation.context.assets.open("ffmpeg/mediacodec_teardown.mp4").use { it.readBytes() } val fixture = copyFixture(fixtureBytes, instrumentation.targetContext.cacheDir) - val activity = instrumentation.startActivitySync( - Intent(instrumentation.targetContext, MpvLifecycleTestActivity::class.java) - .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) - ) as MpvLifecycleTestActivity - instrumentation.waitForIdleSync() try { - repeat(CYCLE_COUNT) { cycle -> - runPlaybackCycle(instrumentation, activity, fixture, cycle) + val activity = instrumentation.startActivitySync( + Intent(instrumentation.targetContext, MpvLifecycleTestActivity::class.java) + .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + ) as MpvLifecycleTestActivity + try { + instrumentation.waitForIdleSync() + repeat(CYCLE_COUNT) { cycle -> + runPlaybackCycle(instrumentation, activity, fixture, cycle, recreateSurfaces, hardwareDecoding) + } + } finally { + instrumentation.runOnMainSync(activity::finish) + instrumentation.waitForIdleSync() } } finally { - instrumentation.runOnMainSync(activity::finish) - instrumentation.waitForIdleSync() fixture.delete() } } @@ -46,7 +92,9 @@ class MpvLifecycleDeviceTest { instrumentation: Instrumentation, activity: MpvLifecycleTestActivity, fixture: File, - cycle: Int + cycle: Int, + recreateSurfaces: Boolean, + hardwareDecoding: Boolean ) { val initialized = CountDownLatch(1) val initializationResult = AtomicReference() @@ -55,7 +103,7 @@ class MpvLifecycleDeviceTest { instrumentation.runOnMainSync { core.set( - MpvPlayerCore(activity, hardwareDecoding = true).also { playerCore -> + MpvPlayerCore(activity, hardwareDecoding = hardwareDecoding).also { playerCore -> playerCore.delegate = events playerCore.initialize { success -> initializationResult.set(success) @@ -65,30 +113,39 @@ class MpvLifecycleDeviceTest { ) } - assertCompletes(initialized, "MPV initialization", cycle) - assertTrue("MPV initialization failed in cycle $cycle", initializationResult.get()) - setProperty(instrumentation, core.get(), "hwdec", "mediacodec", cycle) - setProperty(instrumentation, core.get(), "aid", "no", cycle) + try { + assertCompletes(initialized, "MPV initialization", cycle) + assertTrue("MPV initialization failed in cycle $cycle", initializationResult.get()) + setProperty(instrumentation, core.get(), "hwdec", if (hardwareDecoding) "mediacodec" else "no", cycle) + setProperty(instrumentation, core.get(), "aid", "no", cycle) + if (recreateSurfaces) setProperty(instrumentation, core.get(), "loop-file", "inf", cycle) - val commandCompleted = CountDownLatch(1) - val commandResult = AtomicReference() - instrumentation.runOnMainSync { - core.get().command(arrayOf("loadfile", fixture.absolutePath, "replace")) { success -> - commandResult.set(success) - commandCompleted.countDown() + val commandCompleted = CountDownLatch(1) + val commandResult = AtomicReference() + instrumentation.runOnMainSync { + core.get().command(arrayOf("loadfile", fixture.absolutePath, "replace")) { success -> + commandResult.set(success) + commandCompleted.countDown() + } } + assertCompletes(commandCompleted, "loadfile command", cycle) + assertTrue("loadfile command failed in cycle $cycle", commandResult.get()) + assertCompletes(events.fileLoaded, "file-loaded event", cycle) + assertCompletes(events.playbackRestart, "playback-restart event", cycle) + + assertVideoOutput(core.get(), cycle, hardwareDecoding) + if (recreateSurfaces) exerciseSurfaceRecreation(instrumentation, activity, core.get(), cycle, hardwareDecoding) + } finally { + disposeCore(instrumentation, activity, core.get(), cycle) } - assertCompletes(commandCompleted, "loadfile command", cycle) - assertTrue("loadfile command failed in cycle $cycle", commandResult.get()) - assertCompletes(events.fileLoaded, "file-loaded event", cycle) - assertCompletes(events.playbackRestart, "playback-restart event", cycle) - - assertEquals("mediacodec", core.get().getProperty("current-vo")) - assertTrue( - "Expected MediaCodec hardware decoding in cycle $cycle", - core.get().getProperty("hwdec-current")?.startsWith("mediacodec") == true - ) + } + private fun disposeCore( + instrumentation: Instrumentation, + activity: MpvLifecycleTestActivity, + core: MpvPlayerCore, + cycle: Int + ) { val disposed = CountDownLatch(1) val nextMainTurn = CountDownLatch(1) val disposeElapsedMs = AtomicReference() @@ -96,19 +153,22 @@ class MpvLifecycleDeviceTest { val disposeStartedAt = SystemClock.elapsedRealtime() instrumentation.runOnMainSync { val synchronousDisposeStartedAt = SystemClock.elapsedRealtime() - core.get().dispose { + core.dispose { disposeElapsedMs.set(SystemClock.elapsedRealtime() - disposeStartedAt) disposed.countDown() } Handler(Looper.getMainLooper()).post(nextMainTurn::countDown) synchronousDisposeElapsedMs.set(SystemClock.elapsedRealtime() - synchronousDisposeStartedAt) } - assertTrue( - "dispose() blocked the main thread for ${synchronousDisposeElapsedMs.get()}ms in cycle $cycle", - synchronousDisposeElapsedMs.get() <= MAX_SYNCHRONOUS_DISPOSE_MS - ) - assertCompletes(nextMainTurn, "main-looper turn after dispose", cycle, MAIN_LOOP_TIMEOUT_SECONDS) - assertCompletes(disposed, "terminal teardown", cycle, DISPOSE_TIMEOUT_SECONDS) + try { + assertTrue( + "dispose() blocked the main thread for ${synchronousDisposeElapsedMs.get()}ms in cycle $cycle", + synchronousDisposeElapsedMs.get() <= MAX_SYNCHRONOUS_DISPOSE_MS + ) + assertCompletes(nextMainTurn, "main-looper turn after dispose", cycle, MAIN_LOOP_TIMEOUT_SECONDS) + } finally { + assertCompletes(disposed, "terminal teardown", cycle, DISPOSE_TIMEOUT_SECONDS) + } assertTrue( "Terminal teardown took ${disposeElapsedMs.get()}ms in cycle $cycle", disposeElapsedMs.get() <= MAX_DISPOSE_LATENCY_MS @@ -119,6 +179,123 @@ class MpvLifecycleDeviceTest { } } + private fun exerciseSurfaceRecreation( + instrumentation: Instrumentation, + activity: MpvLifecycleTestActivity, + core: MpvPlayerCore, + cycle: Int, + hardwareDecoding: Boolean + ) { + val surfaces = mutableListOf() + instrumentation.runOnMainSync { + val content = activity.findViewById(android.R.id.content) + val container = content.getChildAt(0) as ViewGroup + // The host's first plane only punches out letterboxing; the remaining + // SurfaceViews are the actual video and OSD planes. + for (index in 1 until container.childCount) { + (container.getChildAt(index) as? SurfaceView)?.let(surfaces::add) + } + } + assertEquals("Expected video and optional OSD surfaces", if (hardwareDecoding) 2 else 1, surfaces.size) + + // Separate visibility changes and callback latches guarantee both real + // destruction/creation orders, including video returning before the OSD. + val orders = if (hardwareDecoding) listOf(surfaces, surfaces.reversed()) else listOf(surfaces) + for (order in orders) { + setProperty(instrumentation, core, "pause", "yes", cycle) + awaitProperty(core, "pause", "public pause", cycle) { it == "yes" } + for (surface in order) { + changeSurfaceVisibility(instrumentation, surface, visible = false, cycle = cycle) + assertEquals("Surface loss cleared public pause in cycle $cycle", "yes", core.getProperty("pause")) + } + for (surface in order) { + changeSurfaceVisibility(instrumentation, surface, visible = true, cycle = cycle) + assertEquals("Surface restoration cleared public pause in cycle $cycle", "yes", core.getProperty("pause")) + } + + val pausedPosition = awaitProperty(core, "time-pos", "paused playback position", cycle) { + it?.toDoubleOrNull() != null + }.toDouble() + setProperty(instrumentation, core, "pause", "no", cycle) + awaitProperty(core, "pause", "public resume", cycle) { it == "no" } + var progressStart = pausedPosition + awaitProperty(core, "time-pos", "playback progress after surface restoration", cycle) { value -> + val position = value?.toDoubleOrNull() + if (position == null) { + false + } else { + // The fixture is two seconds long and loops. Start measuring again + // across a loop boundary rather than mistaking a wrap for a stall. + if (position < progressStart) progressStart = position + position >= progressStart + 0.1 + } + } + assertVideoOutput(core, cycle, hardwareDecoding) + } + } + + private fun changeSurfaceVisibility( + instrumentation: Instrumentation, + surface: SurfaceView, + visible: Boolean, + cycle: Int + ) { + val changed = CountDownLatch(1) + val callback = object : SurfaceHolder.Callback { + override fun surfaceCreated(holder: SurfaceHolder) { + if (visible) changed.countDown() + } + + override fun surfaceChanged(holder: SurfaceHolder, format: Int, width: Int, height: Int) = Unit + + override fun surfaceDestroyed(holder: SurfaceHolder) { + if (!visible) changed.countDown() + } + } + try { + instrumentation.runOnMainSync { + surface.holder.addCallback(callback) + surface.visibility = if (visible) View.VISIBLE else View.INVISIBLE + } + assertCompletes(changed, if (visible) "real surface creation" else "real surface destruction", cycle) + instrumentation.runOnMainSync { + assertEquals("Unexpected surface validity in cycle $cycle", visible, surface.holder.surface.isValid) + } + } finally { + instrumentation.runOnMainSync { surface.holder.removeCallback(callback) } + } + } + + private fun awaitProperty( + core: MpvPlayerCore, + name: String, + operation: String, + cycle: Int, + matches: (String?) -> Boolean + ): String { + val deadline = SystemClock.elapsedRealtime() + TimeUnit.SECONDS.toMillis(OPERATION_TIMEOUT_SECONDS) + var value: String? + do { + value = core.getProperty(name) + if (matches(value)) return requireNotNull(value) + SystemClock.sleep(20) + } while (SystemClock.elapsedRealtime() < deadline) + throw AssertionError("$operation timed out in cycle $cycle; $name=$value") + } + + private fun assertVideoOutput(core: MpvPlayerCore, cycle: Int, hardwareDecoding: Boolean) { + if (hardwareDecoding) { + assertEquals("mediacodec", core.getProperty("current-vo")) + assertTrue( + "Expected MediaCodec hardware decoding in cycle $cycle", + core.getProperty("hwdec-current")?.startsWith("mediacodec") == true + ) + } else { + assertEquals("gpu", core.getProperty("current-vo")) + assertEquals("no", core.getProperty("hwdec-current")) + } + } + private fun setProperty( instrumentation: Instrumentation, core: MpvPlayerCore, diff --git a/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlaceholderSurface.kt b/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlaceholderSurface.kt new file mode 100644 index 000000000..f0f0fd8d1 --- /dev/null +++ b/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlaceholderSurface.kt @@ -0,0 +1,54 @@ +package com.edde746.plezy.mpv + +import android.os.Handler +import android.os.HandlerThread +import android.view.Surface +import androidx.media3.common.util.EGLSurfaceTexture +import kotlinx.coroutines.android.asCoroutineDispatcher +import kotlinx.coroutines.withContext + +/** + * An offscreen consumer, not an undrained ImageReader queue. MediaCodec and GPU + * output can both keep posting buffers while the real SurfaceViews are absent. + * Media3's common EGL utility consumes each frame on its own GL thread, including + * while the main thread is waiting for MPV to acknowledge a surface handoff. + */ +internal class MpvPlaceholderSurface private constructor( + val surface: Surface, + private val handler: Handler, + private val texture: EGLSurfaceTexture +) : AutoCloseable { + companion object { + /** The caller must retain or close the result even if initialization is canceled. */ + suspend fun create(): MpvPlaceholderSurface { + val thread = HandlerThread("MpvPlaceholder").apply { start() } + val handler = Handler(thread.looper) + val texture = EGLSurfaceTexture(handler) + return withContext(handler.asCoroutineDispatcher("MpvPlaceholder")) { + try { + texture.init(EGLSurfaceTexture.SECURE_MODE_NONE) + MpvPlaceholderSurface(Surface(texture.surfaceTexture), handler, texture) + } catch (error: Throwable) { + try { + texture.release() + } finally { + thread.quitSafely() + } + throw error + } + } + } + } + + /** Call only after native consumers have retired; destruction stays on the GL thread. */ + override fun close() { + handler.post { + try { + surface.release() + texture.release() + } finally { + handler.looper.quitSafely() + } + } + } +} diff --git a/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerCore.kt b/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerCore.kt index 883169bd3..eff9e6309 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerCore.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerCore.kt @@ -3,9 +3,7 @@ package com.edde746.plezy.mpv import android.app.Activity import android.app.ActivityManager import android.content.Context -import android.graphics.PixelFormat import android.media.AudioAttributes -import android.media.ImageReader import android.os.Build import android.os.Handler import android.os.Looper @@ -25,10 +23,12 @@ import com.edde746.plezy.shared.MediaCodecQuery import com.edde746.plezy.shared.PlayerDelegate import com.edde746.plezy.shared.PlayerSurfaceHost import com.edde746.plezy.shared.SurfacePlayerCore +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicReference import kotlinx.coroutines.* import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock - /** * mpv playback core. Two modes: * - Video (default): [context] is the host Activity, which is needed for the @@ -81,6 +81,7 @@ class MpvPlayerCore private constructor( companion object { private const val TAG = "MpvPlayerCore" + private const val SURFACE_HANDOFF_TIMEOUT_MS = 2_000L /** * The initial `vo` chain, decided by whether this session will hardware- @@ -195,11 +196,12 @@ class MpvPlayerCore private constructor( private var overlayLayoutListener: ViewTreeObserver.OnGlobalLayoutListener? = null @Volatile private var disposing: Boolean = false + private var nativeDisposalComplete: CountDownLatch? = null @Volatile private var pendingSurface: Surface? = null @Volatile private var attachedSurface: Surface? = null - private var placeholderImageReader: ImageReader? = null + private var placeholder: MpvPlaceholderSurface? = null @Volatile private var placeholderSurface: Surface? = null @@ -255,6 +257,8 @@ class MpvPlayerCore private constructor( @Volatile private var videoOutputRestoring: Boolean = false + @Volatile private var videoOutputFailure: Exception? = null + @Volatile private var deferredResumeRequested: Boolean = false @Volatile private var resumeBlockedByPublicPause: Boolean = false @@ -271,7 +275,6 @@ class MpvPlayerCore private constructor( @Volatile private var videoOutputEpoch: Long = 0L private val videoOutputMutex = Mutex() - private var pendingVideoOutputDisableJob: Job? = null private var pendingVideoOutputRefreshJob: Job? = null private var flutterOverlayApplied = false @@ -285,14 +288,6 @@ class MpvPlayerCore private constructor( } } - private fun ensurePlaceholderSurface() { - if (placeholderSurface?.isValid == true) return - placeholderImageReader?.close() - placeholderImageReader = ImageReader.newInstance(1, 1, PixelFormat.RGBA_8888, 2) - placeholderSurface = placeholderImageReader?.surface - Log.d(TAG, "Created MPV placeholder surface") - } - @Suppress("DEPRECATION") private fun currentDisplayFpsOverride(): String? { if (audioOnly) return null @@ -351,14 +346,13 @@ class MpvPlayerCore private constructor( attachedToPlaceholder = false hasAttachedSurface = false videoOutputRestoring = false + videoOutputFailure = null deferredResumeRequested = false synchronized(publicPauseIntentLock) { publicPauseIntentGeneration += 1L resumeBlockedByPublicPause = false } videoOutputEpoch = 0L - pendingVideoOutputDisableJob?.cancel() - pendingVideoOutputDisableJob = null lastAppliedSurfaceSize = null lastKnownSurfaceWidth = 0 lastKnownSurfaceHeight = 0 @@ -385,7 +379,6 @@ class MpvPlayerCore private constructor( currentDvConversionMode = "auto" hdrSurfaceDecided = false hdrDisplayActive = false - if (!audioOnly) ensurePlaceholderSurface() // Initialize audio focus handling. mpv has none built in, so both modes // use the shared manager: pause on (transient) loss, auto-resume on @@ -433,6 +426,18 @@ class MpvPlayerCore private constructor( scope.launch { try { + if (!audioOnly) { + // Cancellation must not lose an EGL consumer created on its GL thread. + withContext(NonCancellable) { + val created = MpvPlaceholderSurface.create() + if (disposing) { + created.close() + } else { + placeholder = created + placeholderSurface = created.surface + } + } + } if (disposing) { onResult(false) return@launch @@ -687,7 +692,6 @@ class MpvPlayerCore private constructor( val surface = holder.surface pendingSurface = surface.takeIf { it.isValid } - pendingVideoOutputDisableJob?.cancel() videoOutputEpoch += 1L rememberCurrentSurfaceSize() if (player == null) { @@ -707,8 +711,12 @@ class MpvPlayerCore private constructor( override fun surfaceDestroyed(holder: SurfaceHolder) { Log.d(TAG, "Surface destroyed") pendingSurface = null - if (player == null || disposing) return - detachSurfaceInternal(reason = "surfaceDestroyed") + if (disposing) { + awaitNativeDisposal() + return + } + if (player == null) return + handoffDestroyedSurface("surfaceDestroyed", videoLost = true) } // OSD surface (the vo=mediacodec subtitle/OSD plane) @@ -718,7 +726,8 @@ class MpvPlayerCore private constructor( if (disposing) return pendingOsdSurface = holder.surface.takeIf { it.isValid } Log.d(TAG, "OSD surface created") - if (player != null && hasAttachedRealSurface()) { + videoOutputEpoch += 1L + if (player != null && currentCandidateSurface() != null) { refreshVideoOutput("osdSurfaceCreated") } } @@ -728,38 +737,18 @@ class MpvPlayerCore private constructor( override fun surfaceDestroyed(holder: SurfaceHolder) { Log.d(TAG, "OSD surface destroyed") pendingOsdSurface = null - val wasAttached = attachedOsdSurface != null - attachedOsdSurface = null - val p = player ?: return - if (disposing || !wasAttached) return - // Ordered against every other mpv write: a detach that overtook the - // re-attach of a recreated surface would clear the option the attach - // just set, and nothing re-arms it — subtitles would stay dead for the - // rest of the session. - scope.launch(mpvWriteDispatcher) { - try { - p.detachOsdSurface() - } catch (e: Exception) { - Log.w(TAG, "Failed to detach OSD surface", e) - } + if (disposing) { + awaitNativeDisposal() + return } + if (player == null) return + // Rebuild the VO without this plane before Android invalidates it. + // Keep the video target: hiding the OSD when switching to GPU output + // is not a loss of the video surface and must not pause playback. + handoffDestroyedSurface("osdSurfaceDestroyed", videoLost = false) } } - /** - * Hands the OSD Surface to the vo. The vo reads the option when it is - * created, so this has to run before the `vo` or `wid` write that creates - * it, never after. - */ - private fun attachOsdSurfaceIfNeeded(p: MpvPlayer) { - if (!usesMediaCodecVo || activeGpuVoTarget != null) return - val osd = pendingOsdSurface?.takeIf { it.isValid } ?: return - if (osd === attachedOsdSurface) return - p.attachOsdSurface(osd) - attachedOsdSurface = osd - Log.d(TAG, "Attached OSD surface for vo=mediacodec") - } - private fun collectVideoDimensions(p: MpvPlayer) { scope.launch(start = CoroutineStart.UNDISPATCHED) { p.observeInt("dwidth").collect { value -> @@ -816,50 +805,48 @@ class MpvPlayerCore private constructor( */ private fun applyGpuVoTarget() { scope.launch(mpvWriteDispatcher, start = CoroutineStart.ATOMIC) { - val target = synchronized(gpuVoReasons) { activeGpuVoTarget } - runOnMain { - if (disposing) return@runOnMain - if (target == null) { - osdSurfaceView?.visibility = View.VISIBLE - } else { - // The gpu VOs draw their own OSD; a stale subtitle frame must not - // linger on the overlay plane. - osdSurfaceView?.visibility = View.GONE - resetVideoSurfaceToFullContainer() - } - } try { - // Before the vo write, which recreates the VO: it reads the OSD - // surface option at creation. - val p = player - if (target == null && p != null) attachOsdSurfaceIfNeeded(p) - writeProperty("vo", target ?: "mediacodec") - if (p == null) return@launch - applyRenderTier(p, glVoActive = target != null) - if (target != null) { - // A failed conversion chain makes mpv deselect the video track - // ("Could not initialize video chain" -> vid=no) before the VO - // switch lands. Re-select the explicit track id: mid-file "auto" - // resolves to no selection rather than re-running selection. - if (p.getString("vid").let { it == null || it == "no" }) { - val videoTrackId = videoTracks(p).firstOrNull()?.optLong("id") - if (videoTrackId != null) { - Log.i(TAG, "Re-selecting video track $videoTrackId after chain failure") - writeProperty("vid", videoTrackId.toString()) + videoOutputMutex.withLock { + if (disposing || videoOutputFailure != null) return@withLock + val target = synchronized(gpuVoReasons) { activeGpuVoTarget } + runOnMain { + if (disposing) return@runOnMain + if (target == null) { + osdSurfaceView?.visibility = View.VISIBLE + } else { + // GPU output draws its own OSD. + osdSurfaceView?.visibility = View.GONE + resetVideoSurfaceToFullContainer() } } - } - applySurfaceSizeInternal(p, force = true) - if (target == null) { - // Refit the surfaces to the aspect rectangle now that the plane - // owns scaling again (the gpu VOs letterboxed within full - // containers). - applyVideoRectLayout() + val p = player + val surface = attachedSurface?.takeIf { it.isValid } + val osd = if (target == null && !attachedToPlaceholder) pendingOsdSurface?.takeIf { it.isValid } else null + if (p != null && surface != null && osd !== attachedOsdSurface) { + p.attachSurfaces(surface, osd) + attachedOsdSurface = osd + } + writeProperty("vo", target ?: "mediacodec") + if (p == null) return@withLock + applyRenderTier(p, glVoActive = target != null) + if (target != null) { + // A failed conversion chain deselects video before this switch. + // Re-select its explicit id; mid-file "auto" resolves to none. + if (p.getString("vid").let { it == null || it == "no" }) { + val videoTrackId = videoTracks(p).firstOrNull()?.optLong("id") + if (videoTrackId != null) { + Log.i(TAG, "Re-selecting video track $videoTrackId after chain failure") + writeProperty("vid", videoTrackId.toString()) + } + } + } + applySurfaceSizeInternal(p, force = true) + if (target == null) applyVideoRectLayout() } } catch (e: CancellationException) { Log.d(TAG, "Canceled vo transition write") } catch (e: Exception) { - Log.w(TAG, "Failed to move the video output to ${target ?: "mediacodec"}", e) + runOnMain { failVideoOutput("VO transition", e) } } } } @@ -1116,25 +1103,25 @@ class MpvPlayerCore private constructor( rememberSurfaceSize(sv.width, sv.height) } - private fun currentCandidateSurface(): Surface? = surfaceView?.holder?.surface?.takeIf { it.isValid } - ?: pendingSurface?.takeIf { it.isValid } + // Only callbacks publish usable surfaces. SurfaceHolder.isValid can still + // be true inside surfaceDestroyed, after we have revoked that surface. + private fun currentCandidateSurface(): Surface? = pendingSurface?.takeIf { it.isValid } private fun hasAttachedRealSurface(): Boolean = hasAttachedSurface && !attachedToPlaceholder && (attachedSurface?.isValid == true) // Audio-only mode has no video output to wait for — playback and resume // paths gated on output readiness must always proceed there. - private fun hasReadyVideoOutput(): Boolean = audioOnly || (hasAttachedRealSurface() && !videoOutputRestoring) + private fun hasReadyVideoOutput(): Boolean = audioOnly || (videoOutputFailure == null && hasAttachedRealSurface() && !videoOutputRestoring) - private fun isCurrentVideoOutputEpoch(epoch: Long): Boolean = !disposing && epoch == videoOutputEpoch + private fun isCurrentVideoOutputEpoch(epoch: Long): Boolean = !disposing && videoOutputFailure == null && epoch == videoOutputEpoch private fun isVideoOutputRefreshCurrent(epoch: Long): Boolean { - if (disposing) return false - if (epoch != videoOutputEpoch) return false + if (!isCurrentVideoOutputEpoch(epoch)) return false return hasAttachedRealSurface() } private fun refreshVideoOutput(reason: String) { - if (audioOnly || disposing) return + if (audioOnly || disposing || videoOutputFailure != null) return rememberCurrentSurfaceSize() val p = player @@ -1146,18 +1133,12 @@ class MpvPlayerCore private constructor( } if (surface == null || !surface.isValid) { - hasAttachedSurface = false - attachedSurface = null - attachedToPlaceholder = false - pendingSurface = null - lastAppliedSurfaceSize = null videoOutputRestoring = true Log.d(TAG, "refreshVideoOutput($reason): no valid surface available") return } val refreshEpoch = videoOutputEpoch - pendingVideoOutputDisableJob?.cancel() videoOutputRestoring = true flutterOverlayApplied = false ensureFlutterOverlayOnTop() @@ -1170,30 +1151,21 @@ class MpvPlayerCore private constructor( return@withLock } if (!surface.isValid) { - hasAttachedSurface = false - attachedSurface = null - attachedToPlaceholder = false - pendingSurface = null - lastAppliedSurfaceSize = null videoOutputRestoring = true Log.d(TAG, "Skipping MPV video output refresh with invalid surface ($reason, epoch=$refreshEpoch)") return@withLock } - // An unattached OSD Surface forces a wid re-attach: the VO reads the - // OSD surface option at creation, and setting wid recreates the VO. - // Suppressed off the plane, where the GL vo draws its own OSD. - val osdNeedsAttach = activeGpuVoTarget == null && pendingOsdSurface !== attachedOsdSurface - val needsAttach = !hasAttachedSurface || attachedSurface !== surface || osdNeedsAttach + val osd = pendingOsdSurface?.takeIf { usesMediaCodecVo && activeGpuVoTarget == null && it.isValid } + val needsAttach = !hasAttachedSurface || attachedSurface !== surface || osd !== attachedOsdSurface val wasAttachedToPlaceholder = attachedToPlaceholder val wasPausedForSurfaceLoss = pausedForSurfaceLoss if (needsAttach) { - attachOsdSurfaceIfNeeded(p) - p.attachSurface(surface) + p.attachSurfaces(surface, osd) + attachedOsdSurface = osd attachedSurface = surface hasAttachedSurface = true attachedToPlaceholder = false - pendingSurface = null Log.d(TAG, "refreshVideoOutput($reason): attached surface") } else { Log.d(TAG, "refreshVideoOutput($reason): surface already attached, refreshing surface state") @@ -1222,7 +1194,7 @@ class MpvPlayerCore private constructor( } catch (e: CancellationException) { Log.d(TAG, "Canceled pending MPV video output refresh ($reason, epoch=$refreshEpoch)") } catch (e: Exception) { - Log.w(TAG, "Failed to finalize MPV video output refresh ($reason)", e) + runOnMain { failVideoOutput("refresh ($reason)", e) } } } } @@ -1254,80 +1226,107 @@ class MpvPlayerCore private constructor( Log.d(TAG, "Applied MPV surface size $size${if (force) " (forced)" else ""}") } - private fun schedulePlaceholderSurfaceAttach( - p: MpvPlayer, - reason: String, - epoch: Long - ) { - pendingVideoOutputDisableJob?.cancel() - pendingVideoOutputDisableJob = scope.launch(Dispatchers.IO) { + /** + * SurfaceHolder requires consumers to stop using a surface before destruction + * returns. The worker and GL placeholder never need the main looper to finish. + * A timeout is a terminal output failure, not permission to mark it ready or + * release references still held by native code. + */ + private fun handoffDestroyedSurface(reason: String, videoLost: Boolean) { + val p = player ?: return + if (videoOutputFailure != null) return + videoOutputRestoring = true + val epoch = videoOutputEpoch + 1L + videoOutputEpoch = epoch + val completed = CountDownLatch(1) + val failure = AtomicReference() + scope.launch(Dispatchers.IO, start = CoroutineStart.ATOMIC) { try { videoOutputMutex.withLock { - if (!isCurrentVideoOutputEpoch(epoch)) { - Log.d(TAG, "Skipping stale MPV placeholder attach ($reason, epoch=$epoch)") - return@withLock - } - publicPauseWriteMutex.withLock { - val wasPaused = try { - p.getFlag("pause") == true - } catch (e: Exception) { - cachedPaused - } - if (!wasPaused) { - try { + if (!isCurrentVideoOutputEpoch(epoch)) return@withLock + val target = if (videoLost) placeholderSurface else currentCandidateSurface() ?: placeholderSurface + check(target != null && target.isValid) { "No valid MPV surface for $reason" } + val isPlaceholder = target === placeholderSurface + if (isPlaceholder && !attachedToPlaceholder) { + publicPauseWriteMutex.withLock { + val wasPaused = p.getFlag("pause") ?: cachedPaused + if (!wasPaused) { p.setProperty("pause", true) cachedPaused = true pausedForSurfaceLoss = true - Log.d(TAG, "Paused MPV for surface loss ($reason, epoch=$epoch)") - } catch (e: Exception) { - pausedForSurfaceLoss = false - Log.w(TAG, "Failed to pause MPV before placeholder attach ($reason)", e) } - } else { - pausedForSurfaceLoss = false } } - val surface = placeholderSurface?.takeIf { it.isValid } ?: run { - Log.w(TAG, "No valid MPV placeholder surface available for $reason") - return@withLock + // Revoke both planes when the video disappears. Otherwise retain + // the currently available video and remove only the destroyed OSD. + val osd = if (isPlaceholder) { + null + } else { + pendingOsdSurface?.takeIf { + usesMediaCodecVo && activeGpuVoTarget == null && it.isValid + } } - p.attachSurface(surface) - attachedSurface = surface + if (attachedSurface !== target || attachedOsdSurface !== osd) { + p.attachSurfaces(target, osd) + } + attachedSurface = target + attachedOsdSurface = osd hasAttachedSurface = true - attachedToPlaceholder = true + attachedToPlaceholder = isPlaceholder lastAppliedSurfaceSize = null - Log.d(TAG, "Attached MPV placeholder surface ($reason, epoch=$epoch)") + if (!isCurrentVideoOutputEpoch(epoch)) return@withLock + videoOutputRestoring = isPlaceholder + if (!isPlaceholder) { + applySurfaceSizeInternal(p, force = true) + if (!isCurrentVideoOutputEpoch(epoch)) return@withLock + applyDeferredResumeIfNeeded(p, reason) + pausedForSurfaceLoss = false + } + Log.d(TAG, "Surface handoff complete ($reason, epoch=$epoch, placeholder=$isPlaceholder)") } - } catch (e: CancellationException) { - Log.d(TAG, "Canceled pending MPV placeholder attach ($reason, epoch=$epoch)") - } catch (e: Exception) { - Log.w(TAG, "Failed to attach MPV placeholder surface ($reason)", e) + } catch (error: Exception) { + failure.set(error) + } finally { + completed.countDown() } } + val acknowledged = try { + completed.await(SURFACE_HANDOFF_TIMEOUT_MS, TimeUnit.MILLISECONDS) + } catch (error: InterruptedException) { + Thread.currentThread().interrupt() + failure.set(error) + false + } + if (!acknowledged || failure.get() != null) { + failVideoOutput(reason, failure.get() ?: MpvException("Surface handoff timed out after ${SURFACE_HANDOFF_TIMEOUT_MS}ms")) + } } - private fun detachSurfaceInternal(reason: String) { - val hadAttachedSurface = hasAttachedSurface || attachedSurface != null - hasAttachedSurface = false - attachedSurface = null - attachedToPlaceholder = false + private fun failVideoOutput(reason: String, error: Exception) { + if (disposing || videoOutputFailure != null) return + videoOutputFailure = error + videoOutputEpoch += 1L videoOutputRestoring = true - lastAppliedSurfaceSize = null - val detachEpoch = videoOutputEpoch + 1L - videoOutputEpoch = detachEpoch - - val p = player ?: return - if (!hadAttachedSurface) { - Log.d(TAG, "detachSurfaceInternal($reason): no attached surface to clear") - return - } - - schedulePlaceholderSurfaceAttach( - p = p, - reason = reason, - epoch = detachEpoch + deferredResumeRequested = false + Log.e(TAG, "MPV video output failed during $reason", error) + // Same terminal playback-error envelope as the other Android player. + // Do not claim that the native call finished or retire its surfaces here. + delegate?.onEvent( + "end-file", + mapOf("reason" to "error", "message" to "Video output failed", "cause" to "$reason: ${error.message}") ) - Log.d(TAG, "Cleared MPV surface attachment ($reason, epoch=$detachEpoch)") + } + + private fun awaitNativeDisposal() { + val completed = nativeDisposalComplete ?: return + try { + if (!completed.await(SURFACE_HANDOFF_TIMEOUT_MS, TimeUnit.MILLISECONDS)) { + Log.e(TAG, "Native teardown did not release the destroyed surface within ${SURFACE_HANDOFF_TIMEOUT_MS}ms") + } + } catch (error: InterruptedException) { + Thread.currentThread().interrupt() + Log.e(TAG, "Interrupted waiting for native surface retirement", error) + } } private fun normalizePauseValue(value: String): Boolean? = when (value.lowercase()) { @@ -1667,6 +1666,12 @@ class MpvPlayerCore private constructor( } val paused = if (name == "pause") normalizePauseValue(value) else null + if (paused == false) { + videoOutputFailure?.let { error -> + runOnMain { onComplete?.invoke(Result.failure(error)) } + return + } + } val pauseIntent = paused?.let { synchronized(publicPauseIntentLock) { PublicPauseIntent( @@ -2030,10 +2035,10 @@ class MpvPlayerCore private constructor( check(Looper.myLooper() == Looper.getMainLooper()) Log.d(TAG, "Disposing") - surfaceContainer?.let { container -> - container.visibility = View.INVISIBLE - Log.d(TAG, "Hiding surface container during dispose") - } + val disposalComplete = CountDownLatch(1) + nativeDisposalComplete = disposalComplete + // Hiding a SurfaceView destroys its surface. Keep the views visible until + // native teardown retires both consumers; Flutter's overlay stays above. handler.removeCallbacksAndMessages(null) @@ -2048,8 +2053,6 @@ class MpvPlayerCore private constructor( // Cancel all coroutines scope.cancel() - pendingVideoOutputDisableJob?.cancel() - pendingVideoOutputDisableJob = null pendingVideoOutputRefreshJob?.cancel() pendingVideoOutputRefreshJob = null @@ -2070,8 +2073,7 @@ class MpvPlayerCore private constructor( val osdSv = osdSurfaceView val container = surfaceContainer val contentView = if (audioOnly) null else activity.findViewById(android.R.id.content) - val retiringPlaceholderSurface = placeholderSurface - val retiringPlaceholderImageReader = placeholderImageReader + val retiringPlaceholder = placeholder surfaceContainer = null surfaceView = null @@ -2087,7 +2089,7 @@ class MpvPlayerCore private constructor( pendingSurface = null placeholderSurface = null - placeholderImageReader = null + placeholder = null pausedForSurfaceLoss = false pausedForAudioFocusLoss = false attachedToPlaceholder = false @@ -2099,7 +2101,6 @@ class MpvPlayerCore private constructor( desiredPaused = true } videoOutputEpoch = 0L - pendingVideoOutputDisableJob = null isInitialized = false // Close the player on a background thread, then release surfaces and remove views. @@ -2112,8 +2113,8 @@ class MpvPlayerCore private constructor( } catch (e: Exception) { Log.w(TAG, "MPV close failed", e) } - retiringPlaceholderSurface?.release() - retiringPlaceholderImageReader?.close() + disposalComplete.countDown() + retiringPlaceholder?.close() player = null Log.d(TAG, "Disposed (native)") Handler(Looper.getMainLooper()).post { @@ -2127,8 +2128,8 @@ class MpvPlayerCore private constructor( }.start() } else { // No player — safe to remove views immediately - retiringPlaceholderSurface?.release() - retiringPlaceholderImageReader?.close() + disposalComplete.countDown() + retiringPlaceholder?.close() Handler(Looper.getMainLooper()).postAtFrontOfQueue { sv?.holder?.removeCallback(this) osdSv?.holder?.removeCallback(osdSurfaceCallback) diff --git a/android/app/src/test/cpp/mpv_lifecycle_test.cpp b/android/app/src/test/cpp/mpv_lifecycle_test.cpp index 96d4482ba..de4342dec 100644 --- a/android/app/src/test/cpp/mpv_lifecycle_test.cpp +++ b/android/app/src/test/cpp/mpv_lifecycle_test.cpp @@ -32,7 +32,9 @@ static int controlled_thread_create(pthread_t* thread, const pthread_attr_t* att #undef pthread_rwlock_wrlock #undef pthread_mutex_lock #include "../../../../libmpv/src/main/cpp/event.cpp" +#define pthread_mutex_lock tracked_mutex_lock #include "../../../../libmpv/src/main/cpp/render.cpp" +#undef pthread_mutex_lock struct mpv_handle { bool initialized = false; @@ -52,6 +54,8 @@ struct mpv_handle { jlong callback_session = 0; jobject video = nullptr; jobject osd = nullptr; + jobject osd_option = nullptr; + int rebuilds = 0; mpv_event_hook hook{"on_preloaded", 17}; mpv_event event{}; }; @@ -68,7 +72,16 @@ mpv_handle* active_handle = nullptr; // fails explicitly rather than depending on allocator reuse or undefined UAF. std::vector> handles; std::vector> callback_strings; -std::map global_refs; +// Each NewGlobalRef is a distinct handle, even for the same Java Surface. +std::vector> reference_handles; +std::map global_refs; +int fail_ref_after = 0; +std::vector option_failures; +bool consume_osd_before_failure = false; +bool hold_wid = false; +bool wid_entered = false; +bool allow_wid = false; +bool surface_waiting = false; mpv_handle* held_termination = nullptr; bool allow_termination = false; bool allow_command = false; @@ -76,7 +89,7 @@ bool successor_waiting = false; bool reader_draining = false; bool fail_thread_create = false; thread_local mpv_handle* event_handle = nullptr; -enum class Operation { ordinary, successor, replacing_reader }; +enum class Operation { ordinary, successor, replacing_reader, surface_handoff }; thread_local Operation operation = Operation::ordinary; void require(bool condition, const char* message) { @@ -95,24 +108,45 @@ void require_live(mpv_handle* handle) { } void require_surfaces(mpv_handle* handle) { - if (handle->video) require(global_refs[handle->video] == 1, "video Surface released before termination completed"); - if (handle->osd) require(global_refs[handle->osd] == 1, "OSD Surface released before termination completed"); + for (jobject ref : {handle->video, handle->osd, handle->osd_option}) { + if (ref) require(global_refs.count(ref) == 1, "Surface option/consumer retained a deleted JNI reference"); + } } jobject new_global_ref(jobject object) { std::lock_guard lock(gate); - if (object) ++global_refs[object]; - return object; + if (!object) return nullptr; + if (fail_ref_after > 0 && --fail_ref_after == 0) return nullptr; + reference_handles.push_back(std::make_unique(object)); + jobject ref = reference_handles.back().get(); + global_refs.emplace(ref, object); + return ref; } void delete_global_ref(jobject object) { if (!object) return; std::lock_guard lock(gate); - require(global_refs[object] > 0, "JNI global reference released more than once"); - if (active_handle && (active_handle->video == object || active_handle->osd == object)) { - require(active_handle->terminated, "Surface cleanup preceded native termination"); + require(global_refs.count(object) == 1, "JNI global reference released more than once"); + if (active_handle && + (active_handle->video == object || active_handle->osd == object || active_handle->osd_option == object)) { + require(active_handle->terminated, "Surface released while still referenced by MPV"); } - --global_refs[object]; + global_refs.erase(object); +} + +int live_surface_refs() { + int count = 0; + for (const auto& entry : global_refs) { + if (entry.second != &app_context) ++count; + } + return count; +} + +void require_pair(mpv_handle* handle, jobject video, jobject osd) { + require_surfaces(handle); + require(global_refs.at(handle->video) == video, "VO retained the wrong video Surface"); + require((handle->osd ? global_refs.at(handle->osd) : nullptr) == osd, "VO retained the wrong OSD Surface"); + require(handle->osd_option == handle->osd, "OSD option and active consumer disagree"); } void detach_event_thread() { @@ -166,15 +200,25 @@ void initialize_player(jlong session) { void destroy_player(jlong session) { jni_func_name(nativeDestroy)(&jni, nullptr, session); } +jint attach_surfaces(jlong session, jobject video, jobject osd) { + return jni_func_name(nativeAttachSurfaces)(&jni, nullptr, session, video, osd); +} + void reset_dependencies() { std::lock_guard lock(gate); require(active_handle == nullptr, "previous test left a live MPV instance"); - for (const auto& entry : global_refs) { - require(entry.first == &app_context || entry.second == 0, "terminal Surface cleanup leaked a global reference"); - } + require(live_surface_refs() == 0, "terminal Surface cleanup leaked a global reference"); handles.clear(); callback_strings.clear(); global_refs.clear(); + reference_handles.clear(); + fail_ref_after = 0; + option_failures.clear(); + consume_osd_before_failure = false; + hold_wid = false; + wid_entered = false; + allow_wid = false; + surface_waiting = false; held_termination = nullptr; allow_termination = false; allow_command = false; @@ -189,8 +233,7 @@ void rejected_hook_and_successor_retirement() { const jlong old_session = create_player(); mpv_handle* old = active_handle; int video, osd; - jni_func_name(nativeAttachSurface)(&jni, nullptr, old_session, &video); - jni_func_name(nativeAttachOsdSurface)(&jni, nullptr, old_session, &osd); + require(attach_surfaces(old_session, &video, &osd) == 0, "initial surface handoff failed"); { std::lock_guard lock(gate); old->hook_pending = true; @@ -225,8 +268,9 @@ void rejected_hook_and_successor_retirement() { // without touching the retiring core, surfaces, or a future successor. require(command(old_session) == MPV_ERROR_UNINITIALIZED, "revoked command was admitted during termination"); jni_func_name(nativeHookContinue)(&jni, nullptr, old_session, old->hook.id); - jni_func_name(nativeDetachSurface)(&jni, nullptr, old_session); - jni_func_name(nativeDetachOsdSurface)(&jni, nullptr, old_session); + require( + attach_surfaces(old_session, nullptr, nullptr) == MPV_ERROR_UNINITIALIZED, + "revoked surface handoff was admitted during termination"); { std::lock_guard lock(gate); require_surfaces(old); @@ -241,8 +285,7 @@ void rejected_hook_and_successor_retirement() { std::lock_guard lock(gate); successor->hook_pending = true; } - jni_func_name(nativeAttachSurface)(&jni, nullptr, successor_session, &video); - jni_func_name(nativeAttachOsdSurface)(&jni, nullptr, successor_session, &osd); + require(attach_surfaces(successor_session, &video, &osd) == 0, "successor surface handoff failed"); initialize_player(successor_session); { std::unique_lock lock(gate); @@ -255,8 +298,9 @@ void rejected_hook_and_successor_retirement() { jni_func_name(nativeInit)(&jni, nullptr, old_session) == MPV_ERROR_UNINITIALIZED, "stale init reached successor"); require(command(old_session) == MPV_ERROR_UNINITIALIZED, "stale command reached successor"); jni_func_name(nativeHookContinue)(&jni, nullptr, old_session, old->hook.id); - jni_func_name(nativeDetachSurface)(&jni, nullptr, old_session); - jni_func_name(nativeDetachOsdSurface)(&jni, nullptr, old_session); + require( + attach_surfaces(old_session, nullptr, nullptr) == MPV_ERROR_UNINITIALIZED, + "stale surface handoff reached successor"); require(command(successor_session) == 0, "stale teardown retired the successor"); { std::lock_guard lock(gate); @@ -328,18 +372,132 @@ void partial_initialization_can_retire() { } } +void paired_surface_replacements() { + reset_dependencies(); + const jlong session = create_player(); + initialize_player(session); + mpv_handle* handle = active_handle; + int video_a, video_b, osd_a, osd_b; + require(attach_surfaces(session, &video_a, &osd_a) == 0, "initial paired handoff failed"); + require_pair(handle, &video_a, &osd_a); + require(attach_surfaces(session, &video_b, &osd_b) == 0, "paired replacement failed"); + require_pair(handle, &video_b, &osd_b); + for (jobject next_osd : {static_cast(&osd_a), static_cast(nullptr), static_cast(&osd_b)}) { + const int rebuilds = handle->rebuilds; + require(attach_surfaces(session, &video_b, next_osd) == 0, "OSD-only replacement failed"); + require_pair(handle, &video_b, next_osd); + require(handle->rebuilds == rebuilds + 1, "OSD-only change did not rebuild the VO"); + require(live_surface_refs() == (next_osd ? 2 : 1), "replacement leaked overwritten Surface references"); + } + destroy_player(session); +} + +void surface_handoff_failures() { + reset_dependencies(); + const jlong session = create_player(); + initialize_player(session); + mpv_handle* handle = active_handle; + int video_a, video_b, osd_a, osd_b; + require(attach_surfaces(session, &video_a, &osd_a) == 0, "initial paired handoff failed"); + require( + attach_surfaces(session, nullptr, &osd_b) == MPV_ERROR_INVALID_PARAMETER, + "live session admitted a null video Surface"); + for (int allocation : {1, 2}) { + fail_ref_after = allocation; + require( + attach_surfaces(session, &video_b, &osd_b) == MPV_ERROR_NOMEM, "global-reference allocation failure was lost"); + require_pair(handle, &video_a, &osd_a); + require(live_surface_refs() == 2, "allocation failure leaked a staged Surface"); + } + option_failures = {"vo-mediacodec-osd-surface"}; + require(attach_surfaces(session, &video_b, &osd_b) == MPV_ERROR_GENERIC, "OSD option failure was lost"); + require_pair(handle, &video_a, &osd_a); + require(live_surface_refs() == 2, "rejected OSD option leaked staged references"); + + option_failures = {"wid"}; + require(attach_surfaces(session, &video_b, &osd_b) == MPV_ERROR_GENERIC, "wid option failure was lost"); + require_pair(handle, &video_a, &osd_a); + require(attach_surfaces(session, &video_b, &osd_b) == 0, "handoff did not recover from wid failure"); + require_pair(handle, &video_b, &osd_b); + require(live_surface_refs() == 2, "successful replacement retained failed handoff references"); + + // A VO may start between the option writes. Rollback changes only the OSD + // option, so the staged reference must survive even when rollback succeeds. + consume_osd_before_failure = true; + for (bool fail_rollback : {false, true}) { + option_failures = {"wid"}; + if (fail_rollback) option_failures.push_back("vo-mediacodec-osd-surface"); + require(attach_surfaces(session, &video_a, &osd_a) == MPV_ERROR_GENERIC, "failed handoff reported success"); + require(option_failures.empty(), "OSD option rollback was not attempted"); + require_surfaces(handle); + } + require(attach_surfaces(session, &video_a, nullptr) == 0, "handoff did not recover from rollback failure"); + require_pair(handle, &video_a, nullptr); + require(live_surface_refs() == 1, "successful null-OSD replacement leaked failed handoff references"); + + option_failures = {"wid", "vo-mediacodec-osd-surface"}; + require(attach_surfaces(session, &video_b, &osd_b) < 0, "terminal failed handoff reported success"); + destroy_player(session); + require(live_surface_refs() == 0, "termination leaked pending failed-handoff references"); +} + +void overlapping_handoffs_and_teardown() { + reset_dependencies(); + const jlong session = create_player(); + initialize_player(session); + mpv_handle* handle = active_handle; + int video_a, video_b, video_c, osd_a, osd_b, osd_c; + require(attach_surfaces(session, &video_a, &osd_a) == 0, "initial paired handoff failed"); + hold_wid = true; + jint first_result = MPV_ERROR_GENERIC, second_result = MPV_ERROR_GENERIC; + std::thread first([&] { first_result = attach_surfaces(session, &video_b, &osd_b); }); + { + std::unique_lock lock(gate); + await(lock, [] { return wid_entered; }, "first handoff did not enter synchronous VO rebuild"); + require_surfaces(handle); + require(live_surface_refs() == 4, "old Surface references did not survive the wid rebuild"); + } + std::thread second([&] { + operation = Operation::surface_handoff; + second_result = attach_surfaces(session, &video_c, &osd_c); + }); + { + std::unique_lock lock(gate); + await(lock, [] { return surface_waiting; }, "overlapping handoff bypassed surface serialization"); + require(live_surface_refs() == 4, "waiting handoff mutated active Surface references"); + } + std::thread retiring([&] { + operation = Operation::replacing_reader; + destroy_player(session); + }); + { + std::unique_lock lock(gate); + await(lock, [] { return reader_draining; }, "teardown did not drain admitted surface handoffs"); + require(!handle->termination_entered, "teardown overtook an admitted surface handoff"); + allow_wid = true; + changed.notify_all(); + } + first.join(); + second.join(); + retiring.join(); + require(first_result == 0 && second_result == 0, "admitted handoff lost its session during teardown"); + require(handle->rebuilds == 3, "overlapping handoffs lost a complete VO rebuild"); + require(live_surface_refs() == 0, "overlapping replacement/teardown leaked Surface references"); +} + } // namespace // Observe real contention to arrange overlap without sleeps or a guessed // scheduling delay. The production guards still acquire the real pthread // locks; the notification only lets the test release its external MPV gates. static int tracked_mutex_lock(pthread_mutex_t* mutex) { - if (operation != Operation::successor) return pthread_mutex_lock(mutex); + if (operation != Operation::successor && operation != Operation::surface_handoff) return pthread_mutex_lock(mutex); const int result = pthread_mutex_trylock(mutex); if (result != EBUSY) return result; { std::lock_guard lock(gate); - successor_waiting = true; + if (operation == Operation::successor) successor_waiting = true; + if (operation == Operation::surface_handoff) surface_waiting = true; changed.notify_all(); } return pthread_mutex_lock(mutex); @@ -371,9 +529,7 @@ static int controlled_thread_create(pthread_t* thread, const pthread_attr_t* att extern "C" mpv_handle* mpv_create() { std::lock_guard lock(gate); require(active_handle == nullptr, "successor MPV creation overlapped predecessor termination"); - for (const auto& entry : global_refs) { - require(entry.first == &app_context || entry.second == 0, "successor creation preceded retiring Surface cleanup"); - } + require(live_surface_refs() == 0, "successor creation preceded retiring Surface cleanup"); handles.push_back(std::make_unique()); active_handle = handles.back().get(); return active_handle; @@ -461,15 +617,35 @@ extern "C" int mpv_request_log_messages(mpv_handle* handle, const char* level) { } extern "C" int mpv_set_option(mpv_handle* handle, const char* name, mpv_format format, void* data) { - std::lock_guard lock(gate); + std::unique_lock lock(gate); require_live(handle); + require_surfaces(handle); require(format == MPV_FORMAT_INT64, "unexpected Surface option format"); jobject object = reinterpret_cast(static_cast(*static_cast(data))); - if (std::strcmp(name, "wid") == 0) { - handle->video = object; + require(!object || global_refs.count(object) == 1, "option received an invalid JNI reference"); + const bool is_wid = std::strcmp(name, "wid") == 0; + if (is_wid && hold_wid) { + wid_entered = true; + changed.notify_all(); + await(lock, [] { return allow_wid; }, "test did not release the synchronous wid rebuild"); + require_surfaces(handle); + } + if (!option_failures.empty() && option_failures.front() == name) { + option_failures.erase(option_failures.begin()); + if (is_wid && consume_osd_before_failure) handle->osd = handle->osd_option; + return MPV_ERROR_GENERIC; + } + if (is_wid) { + // mpv ignores equal option values; changing wid synchronously retires the + // consumers and rebuilds using the OSD option, not the former OSD plane. + if (handle->video != object) { + handle->video = object; + handle->osd = handle->osd_option; + ++handle->rebuilds; + } } else { require(std::strcmp(name, "vo-mediacodec-osd-surface") == 0, "unexpected Surface option"); - handle->osd = object; + handle->osd_option = object; } return 0; } @@ -515,7 +691,10 @@ int main() { rejected_hook_and_successor_retirement(); admitted_command_survives_replacement(); partial_initialization_can_retire(); + paired_surface_replacements(); + surface_handoff_failures(); + overlapping_handoffs_and_teardown(); reset_dependencies(); - std::puts("MPV lifecycle: callback overlap, reader lifetime, successor isolation and partial init passed"); + std::puts("MPV lifecycle: session isolation, paired handoffs, rollback ownership and overlapping teardown passed"); return 0; } diff --git a/android/libmpv/src/main/cpp/render.cpp b/android/libmpv/src/main/cpp/render.cpp index 77bc7ced3..69cba21d9 100644 --- a/android/libmpv/src/main/cpp/render.cpp +++ b/android/libmpv/src/main/cpp/render.cpp @@ -1,73 +1,85 @@ #include #include +#include +#include + #include "globals.h" #include "jni_utils.h" #include "log.h" extern "C" { -jni_func(void, nativeAttachSurface, jlong session, jobject surface_); -jni_func(void, nativeDetachSurface, jlong session); -jni_func(void, nativeAttachOsdSurface, jlong session, jobject surface_); -jni_func(void, nativeDetachOsdSurface, jlong session); +jni_func(jint, nativeAttachSurfaces, jlong session, jobject surface_, jobject osd_surface_); }; +// Admission (S) precedes this mutex. Teardown drains S before cleanup and +// never takes this mutex, so a waiting handoff cannot outlive its mpv handle. +static pthread_mutex_t surface_lock = PTHREAD_MUTEX_INITIALIZER; +class SurfaceGuard { + public: + SurfaceGuard() { pthread_mutex_lock(&surface_lock); } + ~SurfaceGuard() { pthread_mutex_unlock(&surface_lock); } + SurfaceGuard(const SurfaceGuard&) = delete; + SurfaceGuard& operator=(const SurfaceGuard&) = delete; +}; static jobject surface; - -jni_func(void, nativeAttachSurface, jlong session, jobject surface_) { - SessionGuard guard(session); - if (!guard.mpv) return; - - surface = env->NewGlobalRef(surface_); - if (!surface) { - die("invalid surface provided"); - return; - } - int64_t wid = reinterpret_cast(surface); - int result = mpv_set_option(guard.mpv, "wid", MPV_FORMAT_INT64, &wid); - if (result < 0) ALOGE("mpv_set_option(wid) returned error %s", mpv_error_string(result)); -} - -jni_func(void, nativeDetachSurface, jlong session) { - SessionGuard guard(session); - if (!guard.mpv) return; - - int64_t wid = 0; - int result = mpv_set_option(guard.mpv, "wid", MPV_FORMAT_INT64, &wid); - if (result < 0) ALOGE("mpv_set_option(wid) returned error %s", mpv_error_string(result)); - - env->DeleteGlobalRef(surface); - surface = NULL; -} - static jobject osd_surface; +// A failed wid update can leave an OSD option/consumer using a staged ref. +// Even a successful option rollback cannot retire a consumer created between +// the two writes. Release these only after a successful wid rebuild or destroy. +static std::vector pending_osd_surfaces; -// The OSD plane of vo=mediacodec. Same lifetime rules as the video surface: -// the global ref must outlive the VO, so detach only after vo has been unset. -jni_func(void, nativeAttachOsdSurface, jlong session, jobject surface_) { +jni_func(jint, nativeAttachSurfaces, jlong session, jobject surface_, jobject osd_surface_) { SessionGuard guard(session); - if (!guard.mpv) return; + if (!guard.mpv) return MPV_ERROR_UNINITIALIZED; + if (!surface_) return MPV_ERROR_INVALID_PARAMETER; + SurfaceGuard lock; - osd_surface = env->NewGlobalRef(surface_); - if (!osd_surface) { - die("invalid osd surface provided"); - return; + if (osd_surface_) { + try { + pending_osd_surfaces.reserve(pending_osd_surfaces.size() + 1); + } catch (const std::bad_alloc&) { + return MPV_ERROR_NOMEM; + } + } + jobject next_surface = env->NewGlobalRef(surface_); + if (!next_surface) return MPV_ERROR_NOMEM; + jobject next_osd = osd_surface_ ? env->NewGlobalRef(osd_surface_) : nullptr; + if (osd_surface_ && !next_osd) { + env->DeleteGlobalRef(next_surface); + return MPV_ERROR_NOMEM; } - int64_t wid = reinterpret_cast(osd_surface); - int result = mpv_set_option(guard.mpv, "vo-mediacodec-osd-surface", MPV_FORMAT_INT64, &wid); - if (result < 0) ALOGE("mpv_set_option(vo-mediacodec-osd-surface) returned error %s", mpv_error_string(result)); -} -jni_func(void, nativeDetachOsdSurface, jlong session) { - SessionGuard guard(session); - if (!guard.mpv || !osd_surface) return; + int64_t osd_wid = reinterpret_cast(next_osd); + int result = mpv_set_option(guard.mpv, "vo-mediacodec-osd-surface", MPV_FORMAT_INT64, &osd_wid); + if (result < 0) { + env->DeleteGlobalRef(next_surface); + if (next_osd) env->DeleteGlobalRef(next_osd); + return result; + } + if (next_osd) pending_osd_surfaces.push_back(next_osd); - int64_t wid = 0; - int result = mpv_set_option(guard.mpv, "vo-mediacodec-osd-surface", MPV_FORMAT_INT64, &wid); - if (result < 0) ALOGE("mpv_set_option(vo-mediacodec-osd-surface) returned error %s", mpv_error_string(result)); + // wid has UPDATE_VO: mpv_set_option synchronously tears down the old VO and + // decoder before rebuilding with both options. A fresh video global ref also + // changes wid when only the Java OSD Surface changed (equal values are ignored). + int64_t wid = reinterpret_cast(next_surface); + result = mpv_set_option(guard.mpv, "wid", MPV_FORMAT_INT64, &wid); + if (result < 0) { + osd_wid = reinterpret_cast(osd_surface); + const int rollback = mpv_set_option(guard.mpv, "vo-mediacodec-osd-surface", MPV_FORMAT_INT64, &osd_wid); + if (rollback < 0) ALOGE("OSD surface rollback failed: %s", mpv_error_string(rollback)); + env->DeleteGlobalRef(next_surface); + return result; + } - env->DeleteGlobalRef(osd_surface); - osd_surface = NULL; + if (next_osd) pending_osd_surfaces.pop_back(); + if (surface) env->DeleteGlobalRef(surface); + if (osd_surface) env->DeleteGlobalRef(osd_surface); + for (jobject pending : pending_osd_surfaces) env->DeleteGlobalRef(pending); + pending_osd_surfaces.clear(); + surface = next_surface; + osd_surface = next_osd; + return 0; } // Caller holds L after revoking admission, draining JNI readers, joining the @@ -76,10 +88,12 @@ jni_func(void, nativeDetachOsdSurface, jlong session) { void render_cleanup(JNIEnv* env) { if (surface) { env->DeleteGlobalRef(surface); - surface = NULL; + surface = nullptr; } if (osd_surface) { env->DeleteGlobalRef(osd_surface); - osd_surface = NULL; + osd_surface = nullptr; } + for (jobject pending : pending_osd_surfaces) env->DeleteGlobalRef(pending); + pending_osd_surfaces.clear(); } diff --git a/android/libmpv/src/main/java/com/edde746/plezy/libmpv/MpvPlayer.kt b/android/libmpv/src/main/java/com/edde746/plezy/libmpv/MpvPlayer.kt index 261a2030a..a5f43b4ab 100644 --- a/android/libmpv/src/main/java/com/edde746/plezy/libmpv/MpvPlayer.kt +++ b/android/libmpv/src/main/java/com/edde746/plezy/libmpv/MpvPlayer.kt @@ -192,13 +192,7 @@ class MpvPlayer private constructor( @JvmStatic private external fun nativeSetOptionString(session: Long, name: String, value: String): Int - @JvmStatic private external fun nativeAttachSurface(session: Long, surface: Surface) - - @JvmStatic private external fun nativeDetachSurface(session: Long) - - @JvmStatic private external fun nativeAttachOsdSurface(session: Long, surface: Surface) - - @JvmStatic private external fun nativeDetachOsdSurface(session: Long) + @JvmStatic private external fun nativeAttachSurfaces(session: Long, surface: Surface, osdSurface: Surface?): Int @JvmStatic private external fun nativeGetPropertyInt(session: Long, name: String): Int? @@ -306,27 +300,12 @@ class MpvPlayer private constructor( requestLogMessages(session, level) } - // Surface — not suspend, called from SurfaceHolder.Callback - - fun attachSurface(surface: Surface) { + /** Installs both planes and synchronously rebuilds the VO on the core's ordered IO writer. */ + fun attachSurfaces(surface: Surface, osdSurface: Surface?) { checkNotClosed() - nativeAttachSurface(session, surface) - } - - fun detachSurface() { - checkNotClosed() - nativeDetachSurface(session) - } - - /** OSD/subtitle plane for `vo=mediacodec`; attach before selecting the VO. */ - fun attachOsdSurface(surface: Surface) { - checkNotClosed() - nativeAttachOsdSurface(session, surface) - } - - fun detachOsdSurface() { - checkNotClosed() - nativeDetachOsdSurface(session) + checkNotMainThread("MPV surface handoff") + val result = nativeAttachSurfaces(session, surface, osdSurface) + if (result < 0) throw MpvException("Failed to attach MPV surfaces: error $result") } // Property getters