diff --git a/lib/services/music/music_playback_service_impl.dart b/lib/services/music/music_playback_service_impl.dart index 7d8dacc2..fb1573ce 100644 --- a/lib/services/music/music_playback_service_impl.dart +++ b/lib/services/music/music_playback_service_impl.dart @@ -109,6 +109,13 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO MusicPlayContext? _playContext; PlaybackProgressTracker? _tracker; _ArmedTrack? _armed; + + /// The arm most recently cleared for a re-arm (generation-gated): a queue + /// edit can un-arm an entry in the same instant mpv rolls into it, so the + /// resulting transition must stay adoptable — dropping it leaves the + /// service tracking the finished track for the entire next file. + _ArmedTrack? _staleArm; + int _staleArmGeneration = -1; Timer? _completedConfirmTimer; /// Bumped on every open/advance/stop so stale async continuations @@ -255,6 +262,7 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO _currentTrack = track; _currentSource = null; _armed = null; + _staleArm = null; _setStatus(MusicPlaybackStatus.loading, forceNotify: true); await _coordinator.claimMusic(); @@ -331,13 +339,13 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO if (target == null) { if (_armed == null) return; appLogger.d('Music: clearing arm (queue end / end-of-track sleep)'); - _armed = null; + _rememberStaleArm(); await _trySetNext(player, null); return; } if (_armed?.track.globalKey == target.globalKey) return; - _armed = null; + _rememberStaleArm(); await _trySetNext(player, null); if (generation != _generation || _player != player) return; @@ -367,6 +375,16 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO } } + /// Un-arm bookkeeping: [_armed] is cleared but remembered so + /// [_onTrackTransition] can adopt a transition that raced the clear. + void _rememberStaleArm() { + if (_armed != null) { + _staleArm = _armed; + _staleArmGeneration = _generation; + } + _armed = null; + } + Future _trySetNext(Player player, Media? media) async { try { await player.setNext(media); @@ -446,11 +464,22 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO /// The backend auto-advanced into the pre-armed item: authoritative /// track change. void _onTrackTransition(String uri) { - final armed = _armed; + var armed = _armed; + if ((armed == null || armed.source.url != uri) && + _staleArm != null && + _staleArmGeneration == _generation && + _staleArm!.source.url == uri) { + // mpv rolled into the entry in the same instant a queue edit un-armed + // it — the transition is still authoritative for what is audibly + // playing. + armed = _staleArm; + } + _staleArm = null; if (armed == null || armed.source.url != uri) { appLogger.w('Unexpected track transition to $uri (armed: ${armed?.source.url})'); return; } + final adopted = armed; _armed = null; final generation = ++_generation; @@ -461,19 +490,33 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO // Move the cursor to the armed entry: the expected natural-next when it // still matches, otherwise wherever the armed track now sits. final expected = _queue.nextIndex(); - if (expected != null && _queue.trackAt(expected)?.globalKey == armed.track.globalKey) { + if (expected != null && _queue.trackAt(expected)?.globalKey == adopted.track.globalKey) { _queue.jumpTo(expected); } else { - final index = _queue.queue.indexWhere((t) => t.globalKey == armed.track.globalKey); - if (index >= 0) _queue.jumpTo(index); + final index = _queue.queue.indexWhere((t) => t.globalKey == adopted.track.globalKey); + if (index < 0) { + // The track mpv advanced into was removed from the queue at the + // boundary: don't adopt it — play what the queue says comes next, + // or park when nothing does. + appLogger.d('Music: transition into removed track "${adopted.track.title}" — advancing past it'); + final nextCursor = _queue.nextIndex(); + if (nextCursor != null) { + unawaited(_advanceTo(nextCursor)); + } else { + unawaited(_player?.pause()); + _parkAtEnd(); + } + return; + } + _queue.jumpTo(index); } - _currentTrack = _queue.current ?? armed.track; - _currentSource = armed.source; + _currentTrack = _queue.current ?? adopted.track; + _currentSource = adopted.source; _consecutiveFailures = 0; - appLogger.d('Music: transition received "${armed.track.title}" → cursor ${_queue.cursor}'); + appLogger.d('Music: transition received "${adopted.track.title}" → cursor ${_queue.cursor}'); _setStatus(MusicPlaybackStatus.playing, forceNotify: true); - _bindTrackServices(_currentTrack!, armed.source); + _bindTrackServices(_currentTrack!, adopted.source); unawaited(_armNext(generation)); } @@ -938,6 +981,7 @@ class MusicPlaybackServiceImpl extends MusicPlaybackService with WidgetsBindingO _currentTrack = null; _currentSource = null; _armed = null; + _staleArm = null; _playContext = null; _resumeAfterInterruption = false; diff --git a/test/services/music/music_playback_service_test.dart b/test/services/music/music_playback_service_test.dart index 7b3250f7..0dae1ce0 100644 --- a/test/services/music/music_playback_service_test.dart +++ b/test/services/music/music_playback_service_test.dart @@ -87,6 +87,9 @@ class FakePlayer implements Player { final List openedUris = []; final List setNextCalls = []; final List seeks = []; + + /// Arming these URIs throws, simulating a native setNext failure. + final Set failingSetNextUris = {}; int playCalls = 0; int pauseCalls = 0; int stopCalls = 0; @@ -202,6 +205,9 @@ class FakePlayer implements Player { @override Future setNext(Media? media) async { setNextCalls.add(media); + if (media != null && failingSetNextUris.contains(media.uri)) { + throw StateError('setNext failed for ${media.uri}'); + } _armedMedia = media; } @@ -409,6 +415,9 @@ class FakeMusicSourceResolver implements MusicSourceResolver { final Set failingIds = {}; final Map resolveCounts = {}; + /// Per-track URL overrides (e.g. content:// shapes for offline tracks). + final Map urlOverrides = {}; + @override Future resolve(MediaItem track) async { resolveCounts[track.id] = (resolveCounts[track.id] ?? 0) + 1; @@ -416,7 +425,7 @@ class FakeMusicSourceResolver implements MusicSourceResolver { throw StateError('resolve failed for ${track.id}'); } return MusicSource( - url: _urlFor(track), + url: urlOverrides[track.id] ?? _urlFor(track), playSessionId: 'ps-${track.id}', playMethod: 'DirectPlay', reportingClient: client, @@ -480,6 +489,10 @@ class _Harness { final FakeMediaControlsManager controls; final List players; + /// Seeded into every created FakePlayer — lets a test configure arm + /// failures before the first player exists. + final Set failingSetNextUris = {}; + FakePlayer get player => players.last; factory _Harness.create() { @@ -487,11 +500,13 @@ class _Harness { final resolver = FakeMusicSourceResolver(client: client); final controls = FakeMediaControlsManager(); final players = []; + late final _Harness harness; final service = MusicPlaybackServiceImpl( serverManager: MultiServerManager(), resolver: resolver, audioPlayerFactory: () { final player = FakePlayer(); + player.failingSetNextUris.addAll(harness.failingSetNextUris); players.add(player); return player; }, @@ -500,7 +515,8 @@ class _Harness { // paths resolve within pumpEventQueue. completedConfirmDelay: Duration.zero, ); - return _Harness._(service, resolver, client, controls, players); + harness = _Harness._(service, resolver, client, controls, players); + return harness; } Future playTracks(List tracks, {MediaItem? startTrack, bool shuffle = false}) async { @@ -814,6 +830,105 @@ void main() { expect(h.client.reportsFor('stopped').map((r) => r.itemId), ['t1']); }); + group('gapless boundary races', () { + // The sync stream controllers make the race drivable deterministically: + // a transition emitted between the queue edit (which un-arms the entry + // synchronously) and the event pump lands exactly like mpv rolling into + // the armed entry as it is being cleared. + + test('transition raced by a reorder is adopted via the stale-arm memo', () async { + await h.playTracks([t1, t2, t3]); + expect(h.player.armed?.uri, _urlFor(t2)); + + h.service.reorder(1, 2); // queue [t1, t3, t2] — un-arms t2 + h.player.emitTransition(_urlFor(t2)); // ...but mpv already rolled into it + await pumpEventQueue(); + + expect(h.service.currentTrack?.id, 't2'); + expect(h.service.currentIndex, 2); + expect(h.service.status, MusicPlaybackStatus.playing); + expect(h.player.openedUris, [_urlFor(t1)], reason: 'adopted gaplessly, no re-open'); + final stopped = h.client.reportsFor('stopped').toList(); + expect(stopped.single.itemId, 't1'); + expect(stopped.single.position, _trackDuration); + expect(h.client.reportsFor('started').map((r) => r.itemId), ['t1', 't2']); + }); + + test('transition into a track removed at the boundary advances to the real next', () async { + final t4 = _track('t4'); + await h.playTracks([t1, t2, t3, t4]); + + h.service.removeAt(1); // queue [t1, t3, t4] — un-arms t2 + h.player.emitTransition(_urlFor(t2)); // mpv rolled into the removed track + await pumpEventQueue(); + + expect(h.service.currentTrack?.id, 't3'); + expect(h.player.openedUris, [_urlFor(t1), _urlFor(t3)]); + expect(h.player.armed?.uri, _urlFor(t4)); + expect(h.client.reportsFor('stopped').single.itemId, 't1'); + + // A stale boundary completed pulse must not double-advance past t3. + h.player.emitCompleted(); + await pumpEventQueue(); + expect(h.service.currentTrack?.id, 't3'); + expect(h.player.openedUris, [_urlFor(t1), _urlFor(t3)]); + }); + + test('removed-at-boundary with no next parks paused', () async { + await h.playTracks([t1, t2]); + + h.service.removeAt(1); // queue [t1] — un-arms t2 + h.player.emitTransition(_urlFor(t2)); + await pumpEventQueue(); + + expect(h.service.status, MusicPlaybackStatus.paused); + expect(h.service.currentTrack?.id, 't1'); + expect(h.player.pauseCalls, 1, reason: 'the removed track is audibly playing — silence it'); + expect(h.player.openedUris, [_urlFor(t1)]); + }); + + test('stale transition after a manual advance is dropped', () async { + await h.playTracks([t1, t2, t3]); + + h.service.removeAt(1); // memo t2 + await h.service.next(); // manual advance clears the memo + await pumpEventQueue(); + expect(h.service.currentTrack?.id, 't3'); + + h.player.emitTransition(_urlFor(t2)); + await pumpEventQueue(); + + expect(h.service.currentTrack?.id, 't3'); + expect(h.service.currentIndex, 1); + expect(h.player.openedUris, [_urlFor(t1), _urlFor(t3)]); + }); + + test('failed native arm falls back to an explicit open at completion', () async { + h.failingSetNextUris.add(_urlFor(t2)); + await h.playTracks([t1, t2]); + expect(h.player.armed, isNull); + + h.player.emitCompleted(); + await pumpEventQueue(); + + expect(h.player.openedUris, [_urlFor(t1), _urlFor(t2)]); + expect(h.service.currentTrack?.id, 't2'); + expect(h.service.status, MusicPlaybackStatus.playing); + }); + + test('transition matching is URL-shape agnostic (offline content://)', () async { + h.resolver.urlOverrides['t2'] = 'content://downloads/t2'; + await h.playTracks([t1, t2]); + expect(h.player.armed?.uri, 'content://downloads/t2'); + + h.player.emitTransition('content://downloads/t2'); + await pumpEventQueue(); + + expect(h.service.currentTrack?.id, 't2'); + expect(h.service.status, MusicPlaybackStatus.playing); + }); + }); + test('end-of-track sleep timer suppresses arming and pauses at completion', () async { await h.playTracks([t1, t2]); expect(h.player.armed?.uri, _urlFor(t2));