From 3f03262833f732fb4ae473ee76408c8a71bbc8ca Mon Sep 17 00:00:00 2001 From: edde746 <86283021+edde746@users.noreply.github.com> Date: Wed, 26 Aug 2026 18:26:11 +0200 Subject: [PATCH] fix(player): recover playback after a lost native-channel handoff A player whose predecessor had not released the shared native channel within three seconds skipped its own native dispose and chained its release onto that predecessor. If the predecessor's teardown never completed - one hung 4K session was enough - every later player waited on the chain, failed to initialize, and playback stayed broken with "Playback could not be started" until the app was killed. Initialize and dispose now carry the creating instance's token. The Android plugins remember which token created the current core and acknowledge a dispose from any other token without touching the core, so a dispose that lost the ownership race is provably stale and safe to send. With that guard, a timed-out ownership wait force-disposes instead of skipping, settles its release unchained, and frees the channel for the next session; a dispose watchdog answers Dart even if a native teardown hangs, leaking that one core instead of wedging all future playback. Commands wait eight seconds (was three) so a slow but healthy teardown delays the next session instead of failing it. Verified on a Shield Pro: 38 back-to-back session races at 0.8-2.2s gaps with zero failures and balanced teardowns, and a deep-link-over- playback collision whose stale dispose is ignored, after which Back returns to the still-playing session and Retry starts the new one. Apple and desktop handlers ignore the token and keep the historical skip semantics until they gain the guard. --- .../plezy/exoplayer/ExoPlayerPlugin.kt | 18 +++- .../com/edde746/plezy/mpv/MpvPlayerPlugin.kt | 55 +++++++++++-- .../edde746/plezy/mpv/MpvPlayerPluginTest.kt | 48 +++++++++++ lib/mpv/player/platform/player_android.dart | 6 ++ lib/mpv/player/player_base.dart | 56 +++++++++++-- lib/mpv/player/player_native.dart | 15 +++- test/mpv/player_native_bridge_test.dart | 82 ++++++++++++++++++- 7 files changed, 258 insertions(+), 22 deletions(-) diff --git a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPlugin.kt b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPlugin.kt index d758c33e8..002507408 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPlugin.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/exoplayer/ExoPlayerPlugin.kt @@ -38,6 +38,11 @@ class ExoPlayerPlugin : private val mainHandler get() = channels.mainHandler private fun runOnMain(block: () -> Unit) = channels.runOnMain(block) private var playerCore: ExoPlayerCore? = null + + // The Dart instanceId that created the current core. A `dispose` carrying a + // different token lost the ownership race to a successor and must not tear + // down that successor's session; it is acknowledged without touching it. + private var coreInstanceId: Long? = null private var mpvCore: MpvPlayerCore? = null // MPV fallback player private var usingMpvFallback: Boolean = false private var fallbackInProgress: Boolean = false @@ -161,6 +166,7 @@ class ExoPlayerPlugin : val exoCore = playerCore val fallbackCore = mpvCore playerCore = null + coreInstanceId = null mpvCore = null usingMpvFallback = false fallbackInProgress = false @@ -228,7 +234,7 @@ class ExoPlayerPlugin : override fun onMethodCall(call: MethodCall, result: MethodChannel.Result) { when (call.method) { "initialize" -> handleInitialize(call, result) - "dispose" -> handleDispose(result) + "dispose" -> handleDispose(call, result) "open" -> handleOpen(call, result) "play" -> handlePlay(result) "pause" -> handlePause(result) @@ -337,6 +343,7 @@ class ExoPlayerPlugin : this.debugLoggingEnabled = this@ExoPlayerPlugin.debugLoggingEnabled } playerCore = core + coreInstanceId = call.argument("instanceId")?.toLong() val success = core.initialize( tunnelingEnabled = tunnelingEnabled, audioPassthroughEnabled = audioPassthroughEnabled, @@ -365,8 +372,15 @@ class ExoPlayerPlugin : } } - private fun handleDispose(result: MethodChannel.Result) { + private fun handleDispose(call: MethodCall, result: MethodChannel.Result) { + val token = call.argument("instanceId")?.toLong() runOnMain { + val owner = coreInstanceId + if ((playerCore != null || mpvCore != null) && token != null && owner != null && token != owner) { + Log.d(TAG, "Ignoring stale dispose (token=$token, core owner=$owner)") + result.success(null) + return@runOnMain + } teardownSession(clearActivity = false) Log.d(TAG, "Disposed") result.success(null) diff --git a/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerPlugin.kt b/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerPlugin.kt index b5ab5d282..b66db158f 100644 --- a/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerPlugin.kt +++ b/android/app/src/main/kotlin/com/edde746/plezy/mpv/MpvPlayerPlugin.kt @@ -15,6 +15,7 @@ import io.flutter.plugin.common.EventChannel import io.flutter.plugin.common.MethodCall import io.flutter.plugin.common.MethodChannel import java.util.concurrent.CancellationException +import java.util.concurrent.atomic.AtomicBoolean internal fun completeMpvPropertyResult( result: MethodChannel.Result, @@ -63,6 +64,17 @@ open class MpvPlayerPlugin( private val nameToId = mutableMapOf() private var sessionGeneration = 0 + // The Dart instanceId that created the current core. A `dispose` carrying a + // different token lost the ownership race to a successor and must not tear + // down that successor's core; it is acknowledged without touching anything. + private var coreInstanceId: Long? = null + + // How long a Dart `dispose` waits for the native teardown before being + // answered anyway. Generous against slow-but-healthy teardowns (a 4K HDR + // session's surface/audio release); small against the alternative, which + // is wedging every subsequent playback session behind a hung teardown. + private val disposeWatchdogMs = 6_000L + /** Same semantics as Activity.runOnUiThread, without needing an Activity. */ private fun runOnMain(block: () -> Unit) = channels.runOnMain(block) @@ -96,6 +108,7 @@ open class MpvPlayerPlugin( ++sessionGeneration val core = playerCore playerCore = null + coreInstanceId = null cancelPendingInits() return core } @@ -150,7 +163,7 @@ open class MpvPlayerPlugin( override fun onMethodCall(call: MethodCall, result: MethodChannel.Result) { when (call.method) { "initialize" -> handleInitialize(call, result) - "dispose" -> handleDispose(result) + "dispose" -> handleDispose(call, result) "setProperty" -> handleSetProperty(call, result) "getProperty" -> handleGetProperty(call, result) "getStats" -> handleGetStats(result) @@ -255,6 +268,7 @@ open class MpvPlayerPlugin( delegate = this@MpvPlayerPlugin } playerCore = core + coreInstanceId = call.argument("instanceId")?.toLong() } catch (e: Exception) { Log.e(tag, "Failed to initialize: ${e.message}", e) completePendingInits(attempt, success = false, errorMessage = e.message) @@ -266,7 +280,10 @@ open class MpvPlayerPlugin( playerCore !== core || !isCurrentInitAttempt(attempt) if (stale || !success) { - if (playerCore === core) playerCore = null + if (playerCore === core) { + playerCore = null + coreInstanceId = null + } core.dispose() if (stale) { Log.d(tag, "Stale init callback (gen=$gen, current=$sessionGeneration)") @@ -323,13 +340,37 @@ open class MpvPlayerPlugin( } } - private fun handleDispose(result: MethodChannel.Result) { + private fun handleDispose(call: MethodCall, result: MethodChannel.Result) { + val token = call.argument("instanceId")?.toLong() runOnMain { - val core = takeCoreForTeardown() - core?.dispose { - Log.d(tag, "Disposed") + val owner = coreInstanceId + if (playerCore != null && token != null && owner != null && token != owner) { + // This dispose lost the ownership race: a successor already created + // the current core. Acknowledge without touching it. + Log.d(tag, "Ignoring stale dispose (token=$token, core owner=$owner)") result.success(null) - } ?: result.success(null) + return@runOnMain + } + val core = takeCoreForTeardown() + if (core == null) { + result.success(null) + return@runOnMain + } + // A hung native teardown must not wedge the Dart-side release chain: + // answer after the watchdog even if the teardown thread is stuck, so + // the next session can start on a fresh core. The stuck core leaks its + // resources until the process ends — recoverable, unlike the wedge. + val completed = AtomicBoolean(false) + fun completeOnce(reason: String) { + if (completed.compareAndSet(false, true)) { + Log.d(tag, reason) + result.success(null) + } + } + channels.mainHandler.postDelayed({ + completeOnce("Dispose watchdog fired after ${disposeWatchdogMs}ms; teardown continues in background") + }, disposeWatchdogMs) + core.dispose { completeOnce("Disposed") } } } diff --git a/android/app/src/test/kotlin/com/edde746/plezy/mpv/MpvPlayerPluginTest.kt b/android/app/src/test/kotlin/com/edde746/plezy/mpv/MpvPlayerPluginTest.kt index ec6d42cfa..cfb99425a 100644 --- a/android/app/src/test/kotlin/com/edde746/plezy/mpv/MpvPlayerPluginTest.kt +++ b/android/app/src/test/kotlin/com/edde746/plezy/mpv/MpvPlayerPluginTest.kt @@ -24,6 +24,7 @@ import java.util.concurrent.atomic.AtomicInteger import kotlinx.coroutines.suspendCancellableCoroutine import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test @@ -632,6 +633,53 @@ class MpvPlayerPluginTest { assertEquals(0, pending.size) } + @Test + fun staleDisposeIsAcknowledgedWithoutTearingDownTheCore() { + // A dispose whose instanceId is not the core creator's lost the ownership + // race to a successor; tearing the core down anyway would kill that + // successor's session. It must be acknowledged as a no-op instead. + val plugin = MpvPlayerPlugin() + installCore(plugin, testCore { _, _ -> }) + setPluginField(plugin, "coreInstanceId", 2L) + val result = RecordingResult() + + plugin.onMethodCall(MethodCall("dispose", mapOf("instanceId" to 1)), result) + awaitCompletion(result) + + assertNull(result.errorCode) + assertNotNull(getPluginField(plugin, "playerCore")) + assertEquals(2L, getPluginField(plugin, "coreInstanceId")) + } + + @Test + fun matchingDisposeTearsDownTheCoreAndClearsTheToken() { + val plugin = MpvPlayerPlugin() + installCore(plugin, testCore { _, _ -> }) + setPluginField(plugin, "coreInstanceId", 7L) + val result = RecordingResult() + + plugin.onMethodCall(MethodCall("dispose", mapOf("instanceId" to 7)), result) + awaitCompletion(result) + + assertNull(result.errorCode) + assertNull(getPluginField(plugin, "playerCore")) + assertNull(getPluginField(plugin, "coreInstanceId")) + } + + @Test + fun tokenlessDisposeKeepsLegacySemanticsAndTearsDownTheCore() { + val plugin = MpvPlayerPlugin() + installCore(plugin, testCore { _, _ -> }) + setPluginField(plugin, "coreInstanceId", 7L) + val result = RecordingResult() + + plugin.onMethodCall(MethodCall("dispose", null), result) + awaitCompletion(result) + + assertNull(result.errorCode) + assertNull(getPluginField(plugin, "playerCore")) + } + @Test fun configDetachThenEngineDetachTearsDownVideoCoreAndPendingInitOnce() { val activity = Robolectric.buildActivity(Activity::class.java).setup().get() diff --git a/lib/mpv/player/platform/player_android.dart b/lib/mpv/player/platform/player_android.dart index e5671b1b5..950736b21 100644 --- a/lib/mpv/player/platform/player_android.dart +++ b/lib/mpv/player/platform/player_android.dart @@ -54,6 +54,11 @@ class PlayerAndroid extends PlayerBase { @override String get playerType => 'exoplayer'; + // ExoPlayerPlugin no-ops a dispose whose instanceId is not the core's + // creator, so a timed-out ownership wait may still force-dispose. + @override + bool get nativeDisposeIsStaleGuarded => true; + @override bool get supportsSecondarySubtitles => false; @@ -112,6 +117,7 @@ class PlayerAndroid extends PlayerBase { Future _doInitialize() async { try { final result = await invoke('initialize', { + 'instanceId': nativeInstanceId, 'bufferTier': _bufferTier, 'tunnelingEnabled': _tunnelingEnabled, 'dvConversionMode': _dvConversionMode, diff --git a/lib/mpv/player/player_base.dart b/lib/mpv/player/player_base.dart index 5e865e8a9..2c02402cc 100644 --- a/lib/mpv/player/player_base.dart +++ b/lib/mpv/player/player_base.dart @@ -162,9 +162,26 @@ abstract class PlayerBase with PlayerStreamControllersMixin implements Player { bool _primaryMediaLoadStarted = false; bool _primaryMediaReadyEmitted = false; + /// How long a disposing player waits for its predecessor's native release + /// before force-disposing with its own [nativeInstanceId] (the native side + /// no-ops a stale token, so this can never tear down a successor's core). @visibleForTesting static Duration debugNativeOwnershipDisposeTimeout = const Duration(seconds: 3); + /// How long a command waits for a predecessor's native release before + /// giving up. Longer than the dispose-side wait: a slow but healthy + /// teardown should delay the next session's first command, not fail it. + @visibleForTesting + static Duration debugNativeOwnershipInvokeTimeout = const Duration(seconds: 8); + + /// Identifies this instance to the native side across `initialize` and + /// `dispose`, so a dispose that lost the ownership race is provably stale + /// and can be sent anyway instead of being skipped. Skipping is what used + /// to leave a hung predecessor's release chained forever (the permanent + /// "Playback could not be started" wedge). + static int _nativeInstanceCounter = 0; + final int nativeInstanceId = ++_nativeInstanceCounter; + static const _maximumDurationMilliseconds = 9223372036854775; static double? _finiteDouble(Object? value) { @@ -1066,7 +1083,7 @@ abstract class PlayerBase with PlayerStreamControllersMixin implements Player { if (_disposed) return null; if (_nativeOwnershipReady case final ready?) { try { - await ready.timeout(debugNativeOwnershipDisposeTimeout); + await ready.timeout(debugNativeOwnershipInvokeTimeout); } on TimeoutException { return null; } @@ -1407,6 +1424,16 @@ abstract class PlayerBase with PlayerStreamControllersMixin implements Player { errorController.add(PlayerError('HTTP $status', cause: cause)); } + /// Whether this backend's native `dispose` handler validates the + /// `instanceId` token and no-ops a stale one. Only a guarded handler may + /// receive a dispose after the ownership wait times out — an unguarded + /// handler would tear down whatever core is current, including a + /// successor's. Unguarded platforms keep the historical skip-and-chain + /// behavior (and with it the theoretical wedge) until they gain the guard. + @protected + bool get nativeDisposeIsStaleGuarded => false; + + /// Returns whether the native `dispose` may be sent. Future _waitForNativeOwnershipForDispose() async { final ready = _nativeOwnershipReady; if (ready == null) return true; @@ -1414,6 +1441,15 @@ abstract class PlayerBase with PlayerStreamControllersMixin implements Player { await ready.timeout(debugNativeOwnershipDisposeTimeout); return true; } on TimeoutException catch (error, stackTrace) { + if (nativeDisposeIsStaleGuarded) { + appLogger.w( + 'Timed out waiting for the previous player to release the native channel; ' + 'force-disposing with a stale-guarded token', + error: error, + stackTrace: stackTrace, + ); + return true; + } appLogger.w( 'Timed out waiting for the previous player to release the native channel; skipping native dispose', error: error, @@ -1450,11 +1486,18 @@ abstract class PlayerBase with PlayerStreamControllersMixin implements Player { } _eventSubscription = null; await _logSubscription?.cancel(); - final ownsNativeChannel = await _waitForNativeOwnershipForDispose(); + final sendNativeDispose = await _waitForNativeOwnershipForDispose(); try { - if (ownsNativeChannel) { + if (sendNativeDispose) { + // Sent even when the ownership wait timed out on a guarded backend: + // the token makes a stale dispose provable, so the native side no-ops + // it rather than tearing down a successor's core. Skipping instead + // used to chain this release onto a predecessor that might never + // complete, wedging every future playback session until the app was + // killed. await methodChannel.invokeMethod('dispose', { 'preserveDisplayMode': preserveDisplayMode, + 'instanceId': nativeInstanceId, }); // Direct call — invoke() is disabled once _disposed is set. } } on PlatformException catch (e, st) { @@ -1462,11 +1505,12 @@ abstract class PlayerBase with PlayerStreamControllersMixin implements Player { } on MissingPluginException catch (e, st) { appLogger.w('Player native dispose plugin missing during teardown', error: e, stackTrace: st); } finally { - if (ownsNativeChannel && !_nativeRelease.isCompleted) _nativeRelease.complete(); + if (sendNativeDispose && !_nativeRelease.isCompleted) _nativeRelease.complete(); } - // A timed-out predecessor is still represented by this release future. - // Do not expose an empty ownership slot until that chained release settles. + // On the skip path the release above was completed *with* the + // predecessor's future, so the ownership slot stays occupied until that + // chain settles; on every other path it settles in the finally. if (_nativeRelease.isCompleted) { unawaited( _nativeRelease.future.whenComplete(() { diff --git a/lib/mpv/player/player_native.dart b/lib/mpv/player/player_native.dart index 8cb9f9bae..e702cf947 100644 --- a/lib/mpv/player/player_native.dart +++ b/lib/mpv/player/player_native.dart @@ -127,6 +127,11 @@ class PlayerNative extends PlayerBase { @override bool get providesNativeStats => Platform.isAndroid; + // The Android plugin no-ops a dispose whose instanceId is not the core's + // creator; other platforms' handlers do not read the token yet. + @override + bool get nativeDisposeIsStaleGuarded => Platform.isAndroid; + @override bool get attachesExternalSubtitlesAtOpen => true; @@ -254,9 +259,13 @@ class PlayerNative extends PlayerBase { Future _doInitialize() async { try { // The video core carries the session's decode intent so Android can - // choose its vo before mpv_initialize; every other platform's handler - // ignores initialize arguments. - final result = await invoke('initialize', audioOnly ? null : {'hardwareDecoding': _hardwareDecoding}); + // choose its vo before mpv_initialize. `instanceId` names this Dart + // instance so a later `dispose` that lost the ownership race is + // provably stale; handlers that predate either argument ignore them. + final result = await invoke('initialize', { + if (!audioOnly) 'hardwareDecoding': _hardwareDecoding, + 'instanceId': nativeInstanceId, + }); if (result != true) { throw const PlayerInitializationException(); } diff --git a/test/mpv/player_native_bridge_test.dart b/test/mpv/player_native_bridge_test.dart index 7df0be6cb..3310a80b6 100644 --- a/test/mpv/player_native_bridge_test.dart +++ b/test/mpv/player_native_bridge_test.dart @@ -41,6 +41,18 @@ final class _InvokingPlayerNative extends PlayerNative { Future debugInvoke(String method) => invoke(method); } +final class _GuardedPlayerNative extends PlayerNative { + @override + bool get nativeDisposeIsStaleGuarded => true; +} + +final class _GuardedInvokingPlayerNative extends PlayerNative { + @override + bool get nativeDisposeIsStaleGuarded => true; + + Future debugInvoke(String method) => invoke(method); +} + void main() { TestWidgetsFlutterBinding.ensureInitialized(); @@ -85,7 +97,7 @@ void main() { ); }); - test('video initialize carries the decode intent for the Android vo choice, audio stays bare (#2010)', () async { + test('initialize carries the decode intent and the instance token (#2010)', () async { for (final hardwareDecoding in [true, false]) { final calls = []; await withMockPlayerChannels( @@ -101,7 +113,7 @@ void main() { try { await player.setLogLevel('warn'); final init = calls.singleWhere((call) => call.method == 'initialize'); - expect(init.arguments, {'hardwareDecoding': hardwareDecoding}); + expect(init.arguments, {'hardwareDecoding': hardwareDecoding, 'instanceId': player.nativeInstanceId}); } finally { await player.dispose(); } @@ -123,7 +135,7 @@ void main() { try { await player.setLogLevel('warn'); final init = audioCalls.singleWhere((call) => call.method == 'initialize'); - expect(init.arguments, isNull); + expect(init.arguments, {'instanceId': player.nativeInstanceId}); } finally { await player.dispose(); } @@ -258,7 +270,11 @@ void main() { test('invoke returns null when a predecessor release remains stalled', () async { PlayerBase.debugNativeOwnershipDisposeTimeout = const Duration(milliseconds: 5); - addTearDown(() => PlayerBase.debugNativeOwnershipDisposeTimeout = const Duration(seconds: 3)); + PlayerBase.debugNativeOwnershipInvokeTimeout = const Duration(milliseconds: 5); + addTearDown(() { + PlayerBase.debugNativeOwnershipDisposeTimeout = const Duration(seconds: 3); + PlayerBase.debugNativeOwnershipInvokeTimeout = const Duration(seconds: 8); + }); final stalledNativeDispose = Completer(); final calls = []; @@ -299,6 +315,64 @@ void main() { ); }); + test('a stale-guarded backend force-disposes past a hung predecessor and frees the channel', () async { + PlayerBase.debugNativeOwnershipDisposeTimeout = const Duration(milliseconds: 5); + PlayerBase.debugNativeOwnershipInvokeTimeout = const Duration(milliseconds: 50); + addTearDown(() { + PlayerBase.debugNativeOwnershipDisposeTimeout = const Duration(seconds: 3); + PlayerBase.debugNativeOwnershipInvokeTimeout = const Duration(seconds: 8); + }); + // The first player's native dispose never completes — the hung-teardown + // scenario that used to wedge every later session behind a release chain. + // Later disposes answer normally, standing in for the plugin's dispose + // watchdog and stale-token acknowledgement. + final hungNativeDispose = Completer(); + var sawFirstDispose = false; + final calls = []; + + await withMockPlayerChannels( + methodChannelName: 'com.plezy/mpv_player', + eventChannelName: 'com.plezy/mpv_player/events', + methodHandler: (call) { + calls.add(call); + if (call.method == 'initialize') return Future.value(true); + if (call.method == 'dispose' && !sawFirstDispose) { + sawFirstDispose = true; + return hungNativeDispose.future; + } + return Future.value(null); + }, + testBody: () async { + final first = _GuardedPlayerNative(); + final second = _GuardedPlayerNative(); + final third = _GuardedInvokingPlayerNative(); + Future? firstDisposal; + try { + await first.setLogLevel('warn'); + firstDisposal = first.dispose(); + await Future.delayed(Duration.zero); + + // The successor's dispose times out waiting, then force-sends its + // own token instead of skipping, and settles its release at once. + await second.dispose().timeout(const Duration(seconds: 1)); + final disposes = calls.where((call) => call.method == 'dispose').toList(); + expect(disposes, hasLength(2)); + expect((disposes.last.arguments as Map)['instanceId'], second.nativeInstanceId); + + // The channel is usable again while the hung teardown is still + // pending: the third player's commands are not wedged behind it. + await third.setLogLevel('warn'); + expect(calls.where((call) => call.method == 'initialize'), hasLength(2)); + } finally { + if (!hungNativeDispose.isCompleted) hungNativeDispose.complete(); + await firstDisposal; + await second.dispose(); + await third.dispose(); + } + }, + ); + }); + test('initialization cannot publish readiness after disposal starts', () async { final initialize = Completer(); final calls = [];