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 = [];