From b08544a4508beb623b4727e358dbe50ac7b71984 Mon Sep 17 00:00:00 2001 From: tbrackbill Date: Wed, 26 Aug 2026 16:13:43 -0700 Subject: [PATCH 1/2] fix(android): remote playback desyncs from the renderer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Android, remote playback drifts out of sync with the renderer: the UI advances tracks while the speaker keeps playing the old one, and the media session freezes so the notification, lock screen and car head-unit controls stop working. Verified against UPnP/DLNA; Cast shares three of the four causes. 1. The media session could never show remote playback. MuslyAudioHandler used playbackEventStream.pipe(playbackState), and pipe() is addStream() on the rxdart Subject, so every other playbackState.add() in the class throws "You cannot add items while items are being added from addStream". updateRemotePlaybackState() therefore never worked and the session stayed pinned to the idle local player — hence dead pause and frozen controls. Now listen()+add(), gated so the idle local player cannot overwrite remote state. 2. _isRenderingRemotely was stored state written from eight places. One stale write sent skipNext() down its local branch (seeking the silent local player while the renderer carried on) and made _updateAndroidAuto() skip the media-session update. Now derived from the services that own the audio, with radio keeping its genuine local-only exception. 3. Track URIs were compared without decoding XML entities. Renderers echo a URI back through more escaping layers than we send it (SOAP envelope, then embedded DIDL-Lite), so "...&v=1.16.1..." returns as "...&amp;v=1.16.1...". The comparison could never match, so the poll concluded the renderer had changed track every time and answered with a blind _currentIndex++, walking the UI up the queue while the speaker stayed put. Now compared canonically, and only the transition we actually queued via SetNextAVTransportURI is accepted; anything unrecognised is left alone. 4. Concurrent remote switches were unserialised, so rapid skips started overlapping Stop/SetAVTransportURI/Play pipelines whose completion order varies with per-track URL resolution latency — an older Stop could land after a newer Play. A generation token makes an overtaken switch abandon quietly and stops it tearing down a connection a newer switch is using. Also adds a low-rate poll heartbeat: the healthy poll logged nothing, so this failure left no trace and needed a custom instrumented build to diagnose. Adds 17 tests (119 -> 126). The URI-decoding and media-session tests were mutation-checked — reverting the fix makes them fail. Co-Authored-By: Claude Opus 5 --- lib/providers/player_provider.dart | 106 ++++++++++++++----- lib/services/audio_handler.dart | 20 +++- lib/services/upnp_service.dart | 45 +++++++- test/providers/player_remote_state_test.dart | 77 ++++++++++++++ test/services/audio_handler_test.dart | 69 ++++++++++++ test/services/upnp_service_test.dart | 79 ++++++++++++++ 6 files changed, 366 insertions(+), 30 deletions(-) create mode 100644 test/providers/player_remote_state_test.dart create mode 100644 test/services/audio_handler_test.dart create mode 100644 test/services/upnp_service_test.dart 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..19eb9c3 100644 --- a/lib/services/audio_handler.dart +++ b/lib/services/audio_handler.dart @@ -57,14 +57,32 @@ 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()); } } + /// Stop mirroring the local player into the media session (tests/teardown). + Future cancelLocalStateMirror() async { + await _localStateSub?.cancel(); + _localStateSub = null; + } + @override Future play() => onPlay?.call() ?? _player.play(); diff --git a/lib/services/upnp_service.dart b/lib/services/upnp_service.dart index f6cd336..0ca04aa 100644 --- a/lib/services/upnp_service.dart +++ b/lib/services/upnp_service.dart @@ -202,7 +202,41 @@ 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. + /// + /// `&` is decoded last so `&lt;` yields the literal `<` the sender + /// meant, not `<`. + static String decodeXmlEntities(String input) { + return input + .replaceAll('<', '<') + .replaceAll('>', '>') + .replaceAll('"', '"') + .replaceAll(''', "'") + .replaceAll(''', "'") + .replaceAll(''', "'") + .replaceAll('&', '&'); + } + + /// 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 +343,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..b864ae6 --- /dev/null +++ b/test/services/audio_handler_test.dart @@ -0,0 +1,69 @@ +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('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..a168f53 --- /dev/null +++ b/test/services/upnp_service_test.dart @@ -0,0 +1,79 @@ +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(''), ''); + }); + }); + + 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('trims renderer whitespace padding', () { + expect(UpnpService.canonicalUri(' http://h/x?a=1&b=2 '), + 'http://h/x?a=1&b=2'); + }); + }); +} From 78bd6b8f02d7ad5f531da0ee09338426e146f4df Mon Sep 17 00:00:00 2001 From: tbrackbill Date: Thu, 27 Aug 2026 14:02:57 -0700 Subject: [PATCH 2/2] fix(upnp): numeric XML character references, and cancel the state mirror on dispose Addresses two CodeRabbit findings on #239. decodeXmlEntities only understood named entities plus the numeric forms of the apostrophe, so a renderer emitting & or & for '&' left canonicalUri with a URI that never matched and the gapless auto-advance went unrecognised. It now decodes decimal and hex references generally, leaving malformed and out-of-range ones untouched. Everything denoting '&' is decoded last, together, so &lt; still yields the literal < rather than collapsing to '<'. customAction('dispose') did not cancel the local-state mirror subscription; AudioPlayer.dispose() does not do it for us. It is now cancelled first, and isMirroringLocalState makes that assertable. Tests 126 -> 132; reverting either source file fails 5 of them. Co-Authored-By: Claude Opus 5 --- lib/services/audio_handler.dart | 8 +++++- lib/services/upnp_service.dart | 37 +++++++++++++++++++++------ test/services/audio_handler_test.dart | 16 ++++++++++++ test/services/upnp_service_test.dart | 37 +++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 9 deletions(-) diff --git a/lib/services/audio_handler.dart b/lib/services/audio_handler.dart index 19eb9c3..f37d245 100644 --- a/lib/services/audio_handler.dart +++ b/lib/services/audio_handler.dart @@ -77,7 +77,10 @@ class MuslyAudioHandler extends BaseAudioHandler with SeekHandler { } } - /// Stop mirroring the local player into the media session (tests/teardown). + /// 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; @@ -521,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 0ca04aa..9065358 100644 --- a/lib/services/upnp_service.dart +++ b/lib/services/upnp_service.dart @@ -206,19 +206,40 @@ class UpnpService extends ChangeNotifier { return raw == null ? null : decodeXmlEntities(raw); } - /// Decode one layer of XML character entities. + /// Decode one layer of XML character entities, named or numeric. /// - /// `&` is decoded last so `&lt;` yields the literal `<` the sender - /// meant, not `<`. + /// 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) { - return input + var out = input .replaceAll('<', '<') .replaceAll('>', '>') .replaceAll('"', '"') - .replaceAll(''', "'") - .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. diff --git a/test/services/audio_handler_test.dart b/test/services/audio_handler_test.dart index b864ae6..d5a9c4b 100644 --- a/test/services/audio_handler_test.dart +++ b/test/services/audio_handler_test.dart @@ -56,6 +56,22 @@ void main() { 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', diff --git a/test/services/upnp_service_test.dart b/test/services/upnp_service_test.dart index a168f53..bc6230e 100644 --- a/test/services/upnp_service_test.dart +++ b/test/services/upnp_service_test.dart @@ -48,6 +48,29 @@ void main() { 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', () { @@ -71,6 +94,20 @@ void main() { 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');