diff --git a/lib/providers/player_provider.dart b/lib/providers/player_provider.dart index 34186e0..791932f 100644 --- a/lib/providers/player_provider.dart +++ b/lib/providers/player_provider.dart @@ -68,7 +68,16 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { double _volume = 1.0; double _lastNonZeroVolume = 1.0; - bool _isRenderingRemotely = false; + /// True when audio renders on another device rather than this phone. + /// + /// Derived rather than stored. As a bool assigned from eight places, a single + /// stale write sent `skipNext()` down its local branch (UI advanced, renderer + /// kept playing) and made `_updateAndroidAuto()` skip the media-session + /// update (frozen notification, pause dead over DLNA). Radio is the one real + /// exception and reuses the existing [_isPlayingRadio] flag. + bool get _isRenderingRemotely => + !_isPlayingRadio && + (_castService.isConnected || _upnpService.isConnected); String? _resolvedArtworkUrl; @@ -1851,7 +1860,6 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { } } - _isRenderingRemotely = true; _isPlaying = success; _isLoading = false; if (initialPosition != null && initialPosition > Duration.zero) { @@ -1863,15 +1871,28 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { _updateAndroidAuto(); return; } else if (_upnpService.isConnected) { + // Claim this switch. Each await below lets another press start a + // second pipeline; without a token an older one's Stop can land after + // a newer one's Play, leaving the renderer on a track nobody asked for. + final switchGeneration = ++_remoteSwitchGeneration; + bool superseded() { + if (switchGeneration == _remoteSwitchGeneration) return false; + debugPrint('UPnP: switch #$switchGeneration superseded by ' + '#$_remoteSwitchGeneration — abandoning "${song.title}"'); + return true; + } + _upnpWasPlaying = false; debugPrint( 'UPnP: playSong() taking UPnP branch, isConnected=${_upnpService.isConnected}', ); if (_audioPlayer.playing) await _audioPlayer.stop(); + if (superseded()) return; final playUrl = song.isLocal == true && song.path != null ? Uri.file(song.path!).toString() : await _subsonicService.resolveStreamUrlAsync(song); + if (superseded()) return; try { final mimeType = @@ -1887,6 +1908,7 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { durationSecs: song.duration, contentType: mimeType, ); + if (superseded()) return; if (!success) { _upnpService.disconnect(); debugPrint( @@ -1894,12 +1916,16 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { return; } } catch (e) { + // A superseded switch must not tear down the connection a newer one + // is using. + if (superseded()) return; _upnpService.disconnect(); debugPrint('UPnP playback failed, disconnected: $e'); rethrow; } - _currentUpnpTrackUrl = playUrl; - _isRenderingRemotely = true; + _currentUpnpTrackUrl = UpnpService.canonicalUri(playUrl); + // Anything pre-queued belonged to the track we just replaced. + _nextUpnpTrackUrl = null; _isPlaying = true; _isLoading = false; if (initialPosition != null && initialPosition > Duration.zero) { @@ -1916,7 +1942,6 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { } return; } else { - _isRenderingRemotely = false; final youtubeSource = song.isLocal != true ? await _subsonicService.getYoutubeAudioSource(song) @@ -2057,7 +2082,6 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { _queue = []; _currentIndex = -1; _isPlayingRadio = true; - _isRenderingRemotely = false; _currentRadioStation = station; _position = Duration.zero; _duration = Duration.zero; @@ -3297,7 +3321,6 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { if (connected && !_castWasConnected) { _castWasConnected = true; _castWasPlaying = false; - _isRenderingRemotely = true; if (_audioPlayer.playing) _audioPlayer.pause(); final vol = _castService.mediaState.volume; if (vol >= 0) { @@ -3319,7 +3342,6 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { if (!connected && _castWasConnected) { _castWasConnected = false; _castWasPlaying = false; - _isRenderingRemotely = false; _isPlaying = false; _audioHandler.setRemotePlayback(isRemote: false); notifyListeners(); @@ -3380,7 +3402,15 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { bool _upnpWasConnected = false; bool _upnpWasPlaying = false; + /// Canonical URIs of the track the renderer is playing and the one pre-queued + /// via SetNextAVTransportURI. Canonical because renderers echo URIs back with + /// different escaping than we sent — see [UpnpService.canonicalUri]. + /// Identifies the most recent remote switch so slower in-flight ones can + /// detect they were overtaken. + int _remoteSwitchGeneration = 0; + String? _currentUpnpTrackUrl; + String? _nextUpnpTrackUrl; final bool _isA2dpAudioActive = false; @@ -3403,6 +3433,8 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { durationSecs: nextSong.duration, contentType: mimeType, ); + // Lets the poll recognise a genuine gapless auto-advance. + _nextUpnpTrackUrl = UpnpService.canonicalUri(nextUrl); } catch (e) { debugPrint('UPnP: Failed to set next URI: $e'); } @@ -3434,7 +3466,7 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { _upnpWasConnected = false; _upnpWasPlaying = false; _currentUpnpTrackUrl = null; - _isRenderingRemotely = false; + _nextUpnpTrackUrl = null; _isPlaying = false; _audioHandler.setRemotePlayback(isRemote: false); @@ -3461,26 +3493,44 @@ class PlayerProvider extends ChangeNotifier with WidgetsBindingObserver { return; } - final currentTrackUri = _upnpService.currentTrackUri; - if (_currentUpnpTrackUrl != null && - currentTrackUri != null && - currentTrackUri.isNotEmpty && - currentTrackUri != _currentUpnpTrackUrl && - _currentIndex + 1 < _queue.length) { - debugPrint( - 'UPnP: Renderer switched track to $currentTrackUri — advancing index'); - _upnpWasPlaying = playing; - _currentIndex++; - _currentSong = _queue[_currentIndex]; - _currentUpnpTrackUrl = currentTrackUri; - notifyListeners(); - _updateAllServices(); - _saveQueueState(); - if (_currentIndex + 1 < _queue.length) { - _queueNextSongForUpnp(_queue[_currentIndex + 1]).catchError((_) {}); + // Follow a gapless auto-advance on the renderer. + // + // This used to compare a decoded URI against an undecoded one — never equal + // — and respond to any difference with a blind `_currentIndex++`, which + // walked the UI up the queue a track per second while the speaker stayed + // put. Now only the transition we actually queued via + // SetNextAVTransportURI is accepted; anything else is left alone. + final rendererUri = _upnpService.currentTrackUri; + if (rendererUri != null && rendererUri.isNotEmpty) { + final canonical = UpnpService.canonicalUri(rendererUri); + final isNext = _nextUpnpTrackUrl != null && + canonical == _nextUpnpTrackUrl && + _currentIndex + 1 < _queue.length; + + if (isNext) { + debugPrint('UPnP: renderer auto-advanced to queued next track ' + '— following to index ${_currentIndex + 1}'); + _upnpWasPlaying = playing; + _currentIndex++; + _currentSong = _queue[_currentIndex]; + _currentUpnpTrackUrl = canonical; + _nextUpnpTrackUrl = null; + _position = Duration.zero; + notifyListeners(); + _updateAndroidAuto(); + _saveQueueState(); + if (_currentIndex + 1 < _queue.length) { + _queueNextSongForUpnp(_queue[_currentIndex + 1]).catchError((_) {}); + } + _checkAndRefillAutoQueue().catchError((_) {}); + return; + } + + if (_currentUpnpTrackUrl != null && canonical != _currentUpnpTrackUrl) { + // Another controller, or a switch of ours still in flight. Never guess. + debugPrint('UPnP: renderer on an unrecognised track — leaving queue ' + 'position alone (was index $_currentIndex)'); } - _checkAndRefillAutoQueue().catchError((_) {}); - return; } _upnpWasPlaying = playing; diff --git a/lib/services/audio_handler.dart b/lib/services/audio_handler.dart index f30892a..f37d245 100644 --- a/lib/services/audio_handler.dart +++ b/lib/services/audio_handler.dart @@ -57,14 +57,35 @@ class MuslyAudioHandler extends BaseAudioHandler with SeekHandler { static const _remoteMaxVolume = 100; static const _remoteVolumeStep = 5; + StreamSubscription? _localStateSub; + MuslyAudioHandler() { - _player.playbackEventStream.map(_buildPlaybackState).pipe(playbackState); + // listen()+add() rather than pipe(): pipe() is addStream() on the rxdart + // Subject, which makes every other playbackState.add() in this class throw + // "You cannot add items while items are being added from addStream". That + // silently broke updateRemotePlaybackState(), so the media session could + // never follow Cast/DLNA — notification, lock screen and head-unit controls + // stayed pinned to the idle local player and pause did nothing. The gate + // stops that idle player from overwriting remote state. + _localStateSub = _player.playbackEventStream.listen((event) { + if (_remotePlayback) return; + playbackState.add(_buildPlaybackState(event)); + }); if (!kIsWeb && Platform.isAndroid) { androidPlaybackInfo.add(LocalAndroidPlaybackInfo()); } } + /// True while local player events are still being mirrored into the session. + bool get isMirroringLocalState => _localStateSub != null; + + /// Stop mirroring the local player into the media session. + Future cancelLocalStateMirror() async { + await _localStateSub?.cancel(); + _localStateSub = null; + } + @override Future play() => onPlay?.call() ?? _player.play(); @@ -503,6 +524,9 @@ class MuslyAudioHandler extends BaseAudioHandler with SeekHandler { @override Future customAction(String name, [Map? extras]) async { if (name == 'dispose') { + // Before _player.dispose(): AudioPlayer.dispose() does not cancel our + // own subscription to its event stream. + await cancelLocalStateMirror(); for (final sub in _childrenSubjects.values) { await sub.close(); } diff --git a/lib/services/upnp_service.dart b/lib/services/upnp_service.dart index f6cd336..9065358 100644 --- a/lib/services/upnp_service.dart +++ b/lib/services/upnp_service.dart @@ -202,7 +202,62 @@ class UpnpService extends ChangeNotifier { static String? _xmlText(String xml, String tag) { final pattern = RegExp('<$tag>([^<]*)', caseSensitive: false); - return pattern.firstMatch(xml)?.group(1)?.trim(); + final raw = pattern.firstMatch(xml)?.group(1)?.trim(); + return raw == null ? null : decodeXmlEntities(raw); + } + + /// Decode one layer of XML character entities, named or numeric. + /// + /// Everything denoting `&` is decoded last, together, so `&lt;` yields + /// the literal `<` the sender meant rather than collapsing to `<`. + /// Renderers are inconsistent about which form they emit, so `&` and + /// `&` have to be understood as well as `&`. + static String decodeXmlEntities(String input) { + var out = input + .replaceAll('<', '<') + .replaceAll('>', '>') + .replaceAll('"', '"') + .replaceAll(''', "'"); + + // Numeric references, except those denoting '&' — those wait for the final + // step below so they cannot re-form a named entity mid-pass. + out = out.replaceAllMapped( + RegExp(r'&#([xX][0-9a-fA-F]+|[0-9]+);'), + (m) { + final ref = m.group(1)!; + final isHex = ref.startsWith('x') || ref.startsWith('X'); + final code = isHex + ? int.tryParse(ref.substring(1), radix: 16) + : int.tryParse(ref); + // Leave malformed, out-of-range and '&' references untouched. + if (code == null || code == 0x26 || code < 0x20 || code > 0x10FFFF) { + return m.group(0)!; + } + return String.fromCharCode(code); + }, + ); + + return out + .replaceAll('&', '&') + .replaceAllMapped(RegExp(r'&#(0*38|[xX]0*26);'), (_) => '&'); + } + + /// Canonical form for comparing two URIs, never for reconstructing one. + /// + /// Renderers echo a URI back through more escaping layers than we sent it + /// through (SOAP envelope, then embedded DIDL-Lite), so `...&v=1.16.1...` + /// returns as `...&amp;v=1.16.1...`. Since the depth is not knowable in + /// advance this decodes to a fixed point — more aggressive than XML + /// semantics, but applied to both sides of every comparison, so equal tracks + /// still match and different ones still differ. Bounded so it cannot spin. + static String canonicalUri(String uri) { + var out = uri.trim(); + for (var i = 0; i < 5; i++) { + final next = decodeXmlEntities(out); + if (next == out) break; + out = next; + } + return out; } static String? _extractAvTransportUrl(String xml, String location) { @@ -309,6 +364,15 @@ class UpnpService extends ChangeNotifier { try { final state = await getPlaybackState(); + + // Low-rate heartbeat: a healthy poll was previously silent, so a + // renderer drifting out of sync left no trace in the logs at all. + if (_pollCount % 30 == 1) { + debugPrint('UPnP: poll #$_pollCount healthy — ' + 'state=${state?.transportState ?? "null"} ' + 'pos=${state?.position.inSeconds ?? -1}s errs=$_consecutivePollErrors'); + } + if (state == null) { _consecutivePollErrors++; _safeNotifyListeners(); diff --git a/test/providers/player_remote_state_test.dart b/test/providers/player_remote_state_test.dart new file mode 100644 index 0000000..9233366 --- /dev/null +++ b/test/providers/player_remote_state_test.dart @@ -0,0 +1,77 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:musly/providers/player_provider.dart'; +import 'package:musly/services/audio_handler.dart'; +import 'package:musly/services/jukebox_service.dart'; +import 'package:musly/services/storage_service.dart'; +import 'package:musly/services/subsonic_service.dart'; +import 'package:musly/services/transcoding_service.dart'; +import 'package:musly/services/upnp_service.dart'; +import 'package:shared_preferences/shared_preferences.dart'; + +import '../bootstrap.dart'; +import '../test_helpers.dart'; + +/// `isRemotePlayback` used to be a stored bool written from eight places. A +/// single missed write silently rerouted skipNext() to the local player (UI +/// advanced, renderer kept playing the old track) and suppressed the media +/// session update (frozen notification, pause doing nothing over DLNA). +/// +/// It is now derived from the services that own the audio, so it cannot drift. +/// These tests exist to stop it being turned back into stored state. +void main() { + initializeTestEnvironment(); + + late FakeCastService cast; + late PlayerProvider player; + + setUp(() { + SharedPreferences.setMockInitialValues({}); + cast = FakeCastService(); + player = PlayerProvider( + SubsonicService(), + StorageService(), + cast, + UpnpService(), + MuslyAudioHandler(), + JukeboxService(), + TranscodingService(), + ); + }); + + tearDown(() => player.dispose()); + + test('tracks the renderer with no explicit assignment anywhere', () async { + expect(player.isRemotePlayback, isFalse); + + cast.setMockConnected(true); + await Future.delayed(const Duration(milliseconds: 10)); + expect(player.isRemotePlayback, isTrue); + + cast.setMockConnected(false); + await Future.delayed(const Duration(milliseconds: 10)); + expect(player.isRemotePlayback, isFalse); + }); + + test('survives repeated connect/disconnect without drifting', () async { + // The old stored flag drifted precisely because these transitions were + // handled by separate hand-written assignments that could disagree. + for (var i = 0; i < 5; i++) { + cast.setMockConnected(true); + await Future.delayed(const Duration(milliseconds: 5)); + expect(player.isRemotePlayback, isTrue, reason: 'iteration $i connect'); + + cast.setMockConnected(false); + await Future.delayed(const Duration(milliseconds: 5)); + expect(player.isRemotePlayback, isFalse, + reason: 'iteration $i disconnect'); + } + }); + + test('reports remote immediately, with no round trip needed', () { + // Derivation means there is no window where the services say "connected" + // but the provider still says local — which is the window in which + // skipNext() would have taken the wrong branch. + cast.setMockConnected(true); + expect(player.isRemotePlayback, isTrue); + }); +} diff --git a/test/services/audio_handler_test.dart b/test/services/audio_handler_test.dart new file mode 100644 index 0000000..d5a9c4b --- /dev/null +++ b/test/services/audio_handler_test.dart @@ -0,0 +1,85 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:musly/services/audio_handler.dart'; + +import '../bootstrap.dart'; + +/// The media session is what the notification, lock screen, Android Auto and a +/// car head unit all read. If the handler cannot publish remote playback state +/// into it, every one of those controls silently operates on the idle local +/// player instead of the Cast/DLNA renderer. +/// +/// That is exactly what happened: the constructor used +/// `playbackEventStream.pipe(playbackState)`, and pipe() is addStream() on the +/// underlying rxdart Subject — so every other playbackState.add() in the class +/// threw "You cannot add items while items are being added from addStream". +void main() { + initializeTestEnvironment(); + + late MuslyAudioHandler handler; + + setUp(() => handler = MuslyAudioHandler()); + tearDown(() => handler.cancelLocalStateMirror()); + + test('updateRemotePlaybackState publishes instead of throwing', () { + expect( + () => handler.updateRemotePlaybackState( + playing: true, + position: const Duration(seconds: 42), + ), + returnsNormally, + reason: 'a pipe()d playbackState makes every add() throw', + ); + + expect(handler.playbackState.value.playing, isTrue); + expect(handler.playbackState.value.updatePosition, + const Duration(seconds: 42)); + }); + + test('remote pause state reaches the session', () { + handler.updateRemotePlaybackState( + playing: false, + position: const Duration(seconds: 10), + ); + expect(handler.playbackState.value.playing, isFalse); + }); + + test('repeated remote updates keep working', () { + // A 1 Hz poll drives this continuously; one throw would leave the session + // frozen for the rest of the session. + for (var i = 1; i <= 20; i++) { + handler.updateRemotePlaybackState( + playing: true, + position: Duration(seconds: i), + ); + } + expect(handler.playbackState.value.updatePosition, + const Duration(seconds: 20)); + }); + + test('disposal cancels the local state mirror', () async { + expect(handler.isMirroringLocalState, isTrue); + + await handler.customAction('dispose'); + + expect(handler.isMirroringLocalState, isFalse, + reason: 'AudioPlayer.dispose() does not cancel our own subscription ' + 'to its event stream, so the handler must do it'); + }); + + test('cancelling the mirror twice is safe', () async { + await handler.cancelLocalStateMirror(); + await handler.cancelLocalStateMirror(); + expect(handler.isMirroringLocalState, isFalse); + }); + + test('updateNowPlaying publishes media item metadata', () { + handler.updateNowPlaying( + id: 'song-1', + title: 'Track', + artist: 'Artist', + duration: const Duration(minutes: 3), + ); + expect(handler.mediaItem.value?.id, 'song-1'); + expect(handler.mediaItem.value?.title, 'Track'); + }); +} diff --git a/test/services/upnp_service_test.dart b/test/services/upnp_service_test.dart new file mode 100644 index 0000000..bc6230e --- /dev/null +++ b/test/services/upnp_service_test.dart @@ -0,0 +1,116 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:musly/services/upnp_service.dart'; + +void main() { + group('UpnpService.decodeXmlEntities', () { + test('decodes a single layer of escaping', () { + expect( + UpnpService.decodeXmlEntities('a&b'), + 'a&b', + ); + }); + + test('decodes exactly one layer', () { + // Single-pass by design: '&amp;' is one layer of escaping over the + // text '&', so one decode yields '&'. Collapsing all the way is + // canonicalUri's job, not this function's. + expect( + UpnpService.decodeXmlEntities('v=1.16.1&amp;c=Musly'), + 'v=1.16.1&c=Musly', + ); + }); + + test('decodes the other entities a DIDL document can carry', () { + expect(UpnpService.decodeXmlEntities('<tag>'), ''); + expect(UpnpService.decodeXmlEntities('"q"'), '"q"'); + expect(UpnpService.decodeXmlEntities(''a''), "'a'"); + }); + + test('decodes & last so escaped entities survive one round', () { + // '&lt;' means the sender wanted a literal '<', not '<'. + // Decoding '&' before '<' would wrongly collapse it to '<'. + expect(UpnpService.decodeXmlEntities('&lt;'), '<'); + }); + + test('is a fixed point for text with no entities', () { + const plain = 'http://host/rest/stream?id=abc&v=1'; + expect(UpnpService.decodeXmlEntities(plain), plain); + }); + + test('terminates on pathologically nested escaping', () { + // Must not spin. The bound means very deep nesting is left partly + // encoded, which is fine — it just fails to match, and the caller + // treats an unrecognised URI as "leave the queue alone". + final deep = '&' * 40; + expect(() => UpnpService.canonicalUri(deep), returnsNormally); + }); + + test('handles empty input', () { + expect(UpnpService.decodeXmlEntities(''), ''); + }); + + test('decodes numeric character references, decimal and hex', () { + expect(UpnpService.decodeXmlEntities('a&b'), 'a&b'); + expect(UpnpService.decodeXmlEntities('a&b'), 'a&b'); + expect(UpnpService.decodeXmlEntities('a&b'), 'a&b'); + expect(UpnpService.decodeXmlEntities('a&b'), 'a&b', + reason: 'leading zeros are valid in a numeric reference'); + expect(UpnpService.decodeXmlEntities('<tag>'), ''); + }); + + test('numeric ampersand refs are decoded last, like &', () { + // Same single-pass rule: '&lt;' means a literal '<', so decoding + // the numeric reference early would wrongly collapse it to '<'. + expect(UpnpService.decodeXmlEntities('&lt;'), '<'); + expect(UpnpService.decodeXmlEntities('&lt;'), '<'); + }); + + test('leaves malformed or out-of-range references alone', () { + expect(UpnpService.decodeXmlEntities('&#;'), '&#;'); + expect(UpnpService.decodeXmlEntities('&#zz;'), '&#zz;'); + expect(UpnpService.decodeXmlEntities('�'), '�'); + expect(UpnpService.decodeXmlEntities('�'), '�'); + }); + }); + + group('UpnpService.canonicalUri', () { + test('a sent URI and the renderer echo of it compare equal', () { + // This single assertion is the whole bug: these two strings are the same + // track, and comparing them raw reported a track change on every poll, + // which drove a blind _currentIndex++ and walked the UI up the queue. + const sent = + 'http://192.168.1.5:4533/rest/stream?u=tim&v=1.16.1&c=Musly&id=xyz'; + const echoed = + 'http://192.168.1.5:4533/rest/stream?u=tim&amp;v=1.16.1&amp;c=Musly&amp;id=xyz'; + + expect(UpnpService.canonicalUri(sent), + UpnpService.canonicalUri(echoed)); + }); + + test('genuinely different tracks still differ', () { + const a = 'http://h/rest/stream?id=AAA&v=1'; + const b = 'http://h/rest/stream?id=BBB&v=1'; + expect(UpnpService.canonicalUri(a) == UpnpService.canonicalUri(b), + isFalse); + }); + + test('matches when the renderer uses numeric refs for &', () { + // Renderers are inconsistent about which escaping form they emit; a + // numeric one must still be recognised as the same track, or the gapless + // auto-advance silently stops being followed. + const sent = 'http://h/rest/stream?u=t&v=1.16.1&id=xyz'; + const echoedDecimal = 'http://h/rest/stream?u=t&v=1.16.1&id=xyz'; + const echoedHex = 'http://h/rest/stream?u=t&v=1.16.1&id=xyz'; + + expect(UpnpService.canonicalUri(echoedDecimal), + UpnpService.canonicalUri(sent)); + expect(UpnpService.canonicalUri(echoedHex), + UpnpService.canonicalUri(sent)); + }); + + test('trims renderer whitespace padding', () { + expect(UpnpService.canonicalUri(' http://h/x?a=1&b=2 '), + 'http://h/x?a=1&b=2'); + }); + }); +}