From f9b9fbf611cf92934bbda7b9cc8e38a7b6e95fca Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Mon, 7 Sep 2026 20:07:57 +0200 Subject: [PATCH] fix(player): restore Android MPV surfaces after screensaver Paused playback could remain stuck after the screensaver destroyed its video and OSD surfaces, leaving later resume and initialization blocked. Attach video and OSD surfaces together, drain placeholder frames, and bound surface retirement with explicit failure handling. Preserve native surface ownership through teardown and cover recreation, pause preservation, and disposal. close #2249 --- .../plezy/mpv/MpvLifecycleDeviceTest.kt | 255 ++++++++++-- .../plezy/mpv/MpvPlaceholderSurface.kt | 54 +++ .../com/edde746/plezy/mpv/MpvPlayerCore.kt | 365 +++++++++--------- .../app/src/test/cpp/mpv_lifecycle_test.cpp | 241 ++++++++++-- android/libmpv/src/main/cpp/render.cpp | 120 +++--- .../com/edde746/plezy/libmpv/MpvPlayer.kt | 33 +- 6 files changed, 736 insertions(+), 332 deletions(-) create mode 100644 android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlaceholderSurface.kt 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