Fix UPnP/DLNA: auto-play on connect, position polling, rich metadata, auto-advance#80
Open
mthwJsmith wants to merge 9 commits intodddevid:masterfrom
Open
Fix UPnP/DLNA: auto-play on connect, position polling, rich metadata, auto-advance#80mthwJsmith wants to merge 9 commits intodddevid:masterfrom
mthwJsmith wants to merge 9 commits intodddevid:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds UPnP renderer polling and playback state exposure, enriches UPnP DIDL metadata (album/art/duration), integrates UPnP state into PlayerProvider (connect/disconnect, position/play state, end-of-track detection), and modifies Android service/manifest and AndroidAuto wiring for more robust playback service handling. Changes
Sequence DiagramsequenceDiagram
participant PP as PlayerProvider
participant US as UpnpService
participant R as UPnP Renderer
Note over PP,US: Play request targeting UPnP renderer
PP->>US: loadAndPlay(url,title,artist,album,albumArtUrl,durationSecs)
US->>R: SetAVTransportURI(DIDL‑Lite metadata)
US->>R: Play (with retry/backoff)
Note over US,R: Polling loop
loop interval
US->>R: GetTransportInfo / GetPositionInfo
R-->>US: transportState, position, duration
US->>PP: notifyListeners()
PP->>PP: _onUpnpStateChanged() → update UI / Android Auto / advance on STOPPED
end
Note over PP,US: Disconnect
PP->>US: disconnect()
US->>R: Stop (async) and stop polling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/services/upnp_service.dart (1)
403-435: Cap retry delay so backoff matches intended upper bound.Line 434 doubles delay each failure; with 6 attempts this reaches a 6400ms pre-attempt wait. If intended max is 3200ms, clamp the delay.
Proposed refactor
const maxAttempts = 6; + const maxDelay = Duration(milliseconds: 3200); var delay = const Duration(milliseconds: 200); @@ if (state == 'TRANSITIONING') { debugPrint('UPnP: Renderer TRANSITIONING, waiting… (attempt $attempt)'); - delay *= 2; + delay = delay * 2; + if (delay > maxDelay) delay = maxDelay; continue; } @@ } catch (e) { debugPrint('UPnP: Play attempt $attempt/$maxAttempts failed: $e'); if (attempt == maxAttempts) rethrow; - delay *= 2; + delay = delay * 2; + if (delay > maxDelay) delay = maxDelay; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/upnp_service.dart` around lines 403 - 435, The exponential backoff in the play retry loop currently doubles the Duration `delay` each failure (using the local `delay` and `maxAttempts` logic with `_soapQuery` and `_soap`), which can exceed the intended upper bound; after each doubling of `delay` (both in the TRANSITIONING branch and the Play catch branch) clamp it to the intended max (e.g., Duration(milliseconds: 3200)) by replacing the raw doubling with a bounded update (use min logic on the `delay` variable) so the pre-attempt wait never exceeds the cap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/providers/player_provider.dart`:
- Around line 1219-1224: When UPnP disconnects (detected by the block that
checks !connected && _upnpWasConnected) or when the renderer reports an
unknown/zero duration (the code path around handling duration at lines ~1255),
reset the timeline state to avoid stale UI: clear the position and duration
fields and any related flags in addition to setting _upnpWasConnected = false,
_upnpWasPlaying = false and _isPlaying = false. Specifically, in the disconnect
branch that currently updates _upnpWasConnected/_upnpWasPlaying/_isPlaying, also
zero or null out the timeline variables (e.g., _position, _duration,
seek/position-related flags) and call notifyListeners(); likewise, when you
detect duration == 0 or duration is unavailable, clear the same timeline
variables instead of leaving prior values intact.
In `@lib/services/upnp_service.dart`:
- Around line 563-595: In _soapQuery, reject HTTP or SOAP faults before
returning the body: stop using a permissive success check that returns all
statuses without error—inspect the Dio response.statusCode (or restrict
validateStatus to 200–299) and if statusCode is not a success, throw an
exception; additionally scan response.data for a SOAP fault marker (e.g.
'<s:Fault' or '<fault') and throw a descriptive error instead of returning the
fault XML string, so callers of _soapQuery (and consumers that parse AVTransport
state) won't silently accept fault responses.
- Around line 263-300: The poll loop can overlap because Timer.periodic fires
every second even if the previous _poll() is still running; make _poll()
reentrant-safe by adding a private boolean (e.g., _isPolling) or a simple mutex:
at the top of _poll() return immediately if _isPolling is true, set _isPolling =
true before any await, and in a finally block set _isPolling = false; this
ensures updates to _rendererState/_rendererPosition/_rendererDuration and
notifyListeners() are serialized and prevents out-of-order UI state while
keeping _startPolling()/_stopPolling() and _pollTimer unchanged.
---
Nitpick comments:
In `@lib/services/upnp_service.dart`:
- Around line 403-435: The exponential backoff in the play retry loop currently
doubles the Duration `delay` each failure (using the local `delay` and
`maxAttempts` logic with `_soapQuery` and `_soap`), which can exceed the
intended upper bound; after each doubling of `delay` (both in the TRANSITIONING
branch and the Play catch branch) clamp it to the intended max (e.g.,
Duration(milliseconds: 3200)) by replacing the raw doubling with a bounded
update (use min logic on the `delay` variable) so the pre-attempt wait never
exceeds the cap.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/services/upnp_service.dart (2)
572-605:⚠️ Potential issue | 🟠 Major
_soapQuery()should reject HTTP/SOAP faults before returning.The method accepts all HTTP statuses and returns the body unconditionally. On fault responses, callers like
getPlaybackState()will parse defaults (STOPPED/0:00:00) from missing XML elements, incorrectly reporting renderer state.Unlike
_soap()which validates responses,_soapQuery()lacks these checks.Proposed fix
Future<String> _soapQuery( String controlUrl, String action, String body, ) async { // ... envelope building unchanged ... final response = await _dio.post<String>( controlUrl, data: envelope, options: Options( headers: { 'Content-Type': 'text/xml; charset="utf-8"', 'SOAPAction': '"$serviceType#$action"', }, validateStatus: (_) => true, responseType: ResponseType.plain, ), ); - return response.data ?? ''; + final status = response.statusCode ?? 0; + final responseBody = response.data ?? ''; + + if (status < 200 || status >= 300) { + throw Exception('UPnP SOAP $action failed — HTTP $status'); + } + + final lowerBody = responseBody.toLowerCase(); + if (lowerBody.contains('<s:fault>') || + lowerBody.contains('<soap:fault>') || + lowerBody.contains('<fault>')) { + throw Exception('UPnP SOAP fault for $action'); + } + + return responseBody; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/upnp_service.dart` around lines 572 - 605, The _soapQuery function currently accepts all HTTP statuses and returns the body unconditionally; update it to reject faults by checking the HTTP status on the response (use the existing response variable) and throw an error if statusCode is not 200, and also inspect the returned XML string for SOAP Fault markers (e.g., "<Fault>", "<s:Fault>", "<SOAP-ENV:Fault>", or "<faultcode>") and throw a descriptive exception including the SOAP fault text and the action/controlUrl; mirror the validation behavior used in the existing _soap method so callers like getPlaybackState() only receive successful SOAP responses.
283-310:⚠️ Potential issue | 🟠 MajorSerialize
_poll()to prevent overlapping network cycles.With a 1-second periodic timer, if
getPlaybackState()takes longer than 1 second (e.g., slow network), the next poll will start while the previous one is still running. This can cause out-of-order state updates and race conditions on_rendererState/_rendererPosition/_rendererDuration.Proposed fix
Timer? _pollTimer; + bool _pollInFlight = false; Future<void> _poll() async { + if (_pollInFlight) return; final device = _connectedDevice; if (device == null) return; + _pollInFlight = true; try { final state = await getPlaybackState(); if (state == null) return; + // Verify we're still connected to the same device + if (_connectedDevice?.avTransportUrl != device.avTransportUrl) return; bool changed = false; // ... rest of the method } catch (e) { debugPrint('UPnP: poll error: $e'); + } finally { + _pollInFlight = false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/upnp_service.dart` around lines 283 - 310, The _poll() method can overlap when getPlaybackState() is slow; guard it with a simple reentrancy flag or mutex (e.g., add a private bool _isPolling or a Lock) at the start of _poll() to return immediately if a previous poll is still running, set the flag before awaiting getPlaybackState() and clear it in a finally block so it always resets, and keep the existing logic that updates _rendererState/_rendererPosition/_rendererDuration and calls notifyListeners() only when changed; reference the _poll() function, getPlaybackState(), and the _rendererState/_rendererPosition/_rendererDuration fields when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/services/upnp_service.dart`:
- Around line 572-605: The _soapQuery function currently accepts all HTTP
statuses and returns the body unconditionally; update it to reject faults by
checking the HTTP status on the response (use the existing response variable)
and throw an error if statusCode is not 200, and also inspect the returned XML
string for SOAP Fault markers (e.g., "<Fault>", "<s:Fault>", "<SOAP-ENV:Fault>",
or "<faultcode>") and throw a descriptive exception including the SOAP fault
text and the action/controlUrl; mirror the validation behavior used in the
existing _soap method so callers like getPlaybackState() only receive successful
SOAP responses.
- Around line 283-310: The _poll() method can overlap when getPlaybackState() is
slow; guard it with a simple reentrancy flag or mutex (e.g., add a private bool
_isPolling or a Lock) at the start of _poll() to return immediately if a
previous poll is still running, set the flag before awaiting getPlaybackState()
and clear it in a finally block so it always resets, and keep the existing logic
that updates _rendererState/_rendererPosition/_rendererDuration and calls
notifyListeners() only when changed; reference the _poll() function,
getPlaybackState(), and the _rendererState/_rendererPosition/_rendererDuration
fields when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/services/upnp_service.dart`:
- Around line 619-622: The SOAP fault detection in _soapQuery is case-sensitive
and may miss variants like "<s:fault>" — update the detection to match the
approach used in _soap by performing case-insensitive checks (e.g., convert
responseBody to lowercase or use a case-insensitive regex) and look for tags
such as "<s:fault>", "<soap:fault>", "<fault>", and "<faultcode>" before
throwing the Exception('UPnP SOAP fault for $action'); ensure you reference
responseBody and action in the same check inside _soapQuery.
lib/services/upnp_service.dart
Outdated
Comment on lines
619
to
622
| if (responseBody.contains('<s:Fault') || | ||
| responseBody.contains('<faultcode>')) { | ||
| throw Exception('UPnP SOAP fault for $action'); | ||
| } |
There was a problem hiding this comment.
Inconsistent SOAP fault detection between _soap and _soapQuery.
_soap checks lowercase patterns (<s:fault>, <soap:fault>, <fault>), while _soapQuery uses case-sensitive checks (<s:Fault, <faultcode>). A renderer returning <s:fault> (lowercase) would be caught by _soap but missed by _soapQuery.
Proposed fix: align with _soap's case-insensitive approach
- if (responseBody.contains('<s:Fault') ||
- responseBody.contains('<faultcode>')) {
+ final lowerBody = responseBody.toLowerCase();
+ if (lowerBody.contains('<s:fault>') ||
+ lowerBody.contains('<soap:fault>') ||
+ lowerBody.contains('<fault>')) {
throw Exception('UPnP SOAP fault for $action');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (responseBody.contains('<s:Fault') || | |
| responseBody.contains('<faultcode>')) { | |
| throw Exception('UPnP SOAP fault for $action'); | |
| } | |
| final lowerBody = responseBody.toLowerCase(); | |
| if (lowerBody.contains('<s:fault>') || | |
| lowerBody.contains('<soap:fault>') || | |
| lowerBody.contains('<fault>')) { | |
| throw Exception('UPnP SOAP fault for $action'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/services/upnp_service.dart` around lines 619 - 622, The SOAP fault
detection in _soapQuery is case-sensitive and may miss variants like "<s:fault>"
— update the detection to match the approach used in _soap by performing
case-insensitive checks (e.g., convert responseBody to lowercase or use a
case-insensitive regex) and look for tags such as "<s:fault>", "<soap:fault>",
"<fault>", and "<faultcode>" before throwing the Exception('UPnP SOAP fault for
$action'); ensure you reference responseBody and action in the same check inside
_soapQuery.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/services/upnp_service.dart (1)
417-451: Unreachable code after retry loop.Line 451 (
return false) is unreachable because:
- If all attempts fail, line 445 rethrows on the final attempt
- If any attempt succeeds, line 442 returns
trueThis is dead code.
Proposed fix
} catch (e) { debugPrint('UPnP: Play attempt $attempt/$maxAttempts failed: $e'); if (attempt == maxAttempts) rethrow; delay = delay * 2 < const Duration(milliseconds: 2400) ? delay * 2 : const Duration(milliseconds: 2400); } } - return false; + // Unreachable: loop either returns true on success or rethrows on final failure + throw StateError('Unexpected exit from retry loop'); }Alternatively, simply remove the
return falsesince it's unreachable, but the Dart analyzer may require a return statement for type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/services/upnp_service.dart` around lines 417 - 451, The final `return false` is unreachable because the catch currently does `rethrow` on the last attempt; replace `if (attempt == maxAttempts) rethrow;` with `if (attempt == maxAttempts) return false;` (in the retry loop that calls `_soap`/`_soapQuery` against `device.avTransportUrl` and checks `CurrentTransportState`) so the method can fall through to the existing `return false;` instead of throwing, or alternatively remove the trailing `return false;` and keep `rethrow` if you want to propagate the error—pick one approach and make the change in the method that wraps `_soap`/`_soapQuery`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/services/upnp_service.dart`:
- Around line 417-451: The final `return false` is unreachable because the catch
currently does `rethrow` on the last attempt; replace `if (attempt ==
maxAttempts) rethrow;` with `if (attempt == maxAttempts) return false;` (in the
retry loop that calls `_soap`/`_soapQuery` against `device.avTransportUrl` and
checks `CurrentTransportState`) so the method can fall through to the existing
`return false;` instead of throwing, or alternatively remove the trailing
`return false;` and keep `rethrow` if you want to propagate the error—pick one
approach and make the change in the method that wraps `_soap`/`_soapQuery`.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/providers/player_provider.dart (1)
597-613:⚠️ Potential issue | 🟠 MajorHandle
loadAndPlay()returningfalsebefore setting playing state.At Line 597,
loadAndPlay()returnsbool, but the result is ignored; Line 613 sets_isPlaying = trueunconditionally. If Play retries are exhausted (returnsfalse), UI/services can show a false playing state.Proposed fix
- try { - await _upnpService.loadAndPlay( + try { + final started = await _upnpService.loadAndPlay( url: playUrl, title: song.title, artist: song.artist ?? 'Unknown Artist', album: song.album, albumArtUrl: song.coverArt != null ? _subsonicService.getCoverArtUrl(song.coverArt, size: 0) : null, durationSecs: song.duration, ); + if (!started) { + _upnpService.disconnect(); + throw Exception('UPnP Play failed after retries'); + } } catch (e) { // SOAP call failed — disconnect so the UI reflects the real state _upnpService.disconnect(); debugPrint('UPnP playback failed, disconnected: $e'); rethrow; } _isPlaying = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/player_provider.dart` around lines 597 - 613, The call to _upnpService.loadAndPlay(...) can return false but the result is ignored and _isPlaying is set true unconditionally; change the flow to capture the boolean result from _upnpService.loadAndPlay(...) and only set _isPlaying = true when that result is true, and when it returns false call _upnpService.disconnect() (and log or rethrow as appropriate) so the UI doesn't show a playing state when playback failed; update references around the loadAndPlay call and the _isPlaying assignment to use the captured result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/kotlin/com/musly/musly/AndroidAutoPlugin.kt`:
- Around line 71-77: MusicService may be null immediately after calling
startMusicService() because startForegroundService() is asynchronous, so replace
the current immediate call to
MusicService.getInstance()?.updatePlaybackState(...) with logic that waits for
the service to be ready and then performs the update: after calling
startMusicService() (in AndroidAutoPlugin), either bind to the service or
register a short-lived listener/callback/StickyBroadcast from MusicService that
signals readiness, or implement a small retry/queue loop that polls
MusicService.getInstance() and invokes updatePlaybackState(songId, title,
artist, album, artworkUrl, duration, position, playing) once non-null; apply the
same pattern anywhere startMusicService() is followed immediately by
MusicService.getInstance()?.updatePlaybackState (also check the
AndroidSystemPlugin occurrences).
---
Outside diff comments:
In `@lib/providers/player_provider.dart`:
- Around line 597-613: The call to _upnpService.loadAndPlay(...) can return
false but the result is ignored and _isPlaying is set true unconditionally;
change the flow to capture the boolean result from _upnpService.loadAndPlay(...)
and only set _isPlaying = true when that result is true, and when it returns
false call _upnpService.disconnect() (and log or rethrow as appropriate) so the
UI doesn't show a playing state when playback failed; update references around
the loadAndPlay call and the _isPlaying assignment to use the captured result.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
android/app/src/main/AndroidManifest.xmlandroid/app/src/main/kotlin/com/musly/musly/AndroidAutoPlugin.ktandroid/app/src/main/kotlin/com/musly/musly/AndroidSystemPlugin.ktandroid/app/src/main/kotlin/com/musly/musly/MusicService.ktlib/providers/player_provider.dartlib/services/upnp_service.dart
Comment on lines
71
to
77
| // Ensure the service is running before updating state | ||
| if (MusicService.getInstance() == null) { | ||
| Log.d(TAG, "MusicService not running, starting it now") | ||
| startMusicService() | ||
| } | ||
| MusicService.getInstance()?.updatePlaybackState( | ||
| songId, title, artist, album, artworkUrl, duration, position, playing |
There was a problem hiding this comment.
Avoid dropping playback-state updates while MusicService is still starting.
startForegroundService() is async. On Line 72-77, MusicService.getInstance() can still be null right after start, so the first updatePlaybackState is silently lost.
Proposed fix
+import android.os.Handler
+import android.os.Looper
@@
object AndroidAutoPlugin : FlutterPlugin, MethodChannel.MethodCallHandler {
@@
+ private val mainHandler = Handler(Looper.getMainLooper())
@@
"updatePlaybackState" -> {
@@
- if (MusicService.getInstance() == null) {
- Log.d(TAG, "MusicService not running, starting it now")
- startMusicService()
- }
- MusicService.getInstance()?.updatePlaybackState(
- songId, title, artist, album, artworkUrl, duration, position, playing
- )
+ val pushState = {
+ MusicService.getInstance()?.updatePlaybackState(
+ songId, title, artist, album, artworkUrl, duration, position, playing
+ )
+ }
+ if (MusicService.getInstance() == null) {
+ Log.d(TAG, "MusicService not running, starting it now")
+ startMusicService()
+ mainHandler.postDelayed({ pushState() }, 200)
+ } else {
+ pushState()
+ }
result.success(null)
}#!/bin/bash
# Verify all places where service start and immediate singleton usage happen.
rg -n -C3 'startMusicService\(\)|MusicService\.getInstance\(\)\?\.updatePlaybackState|MusicService\.getInstance\(\) == null' \
android/app/src/main/kotlin/com/musly/musly/AndroidAutoPlugin.kt \
android/app/src/main/kotlin/com/musly/musly/AndroidSystemPlugin.ktAlso applies to: 123-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/kotlin/com/musly/musly/AndroidAutoPlugin.kt` around
lines 71 - 77, MusicService may be null immediately after calling
startMusicService() because startForegroundService() is asynchronous, so replace
the current immediate call to
MusicService.getInstance()?.updatePlaybackState(...) with logic that waits for
the service to be ready and then performs the update: after calling
startMusicService() (in AndroidAutoPlugin), either bind to the service or
register a short-lived listener/callback/StickyBroadcast from MusicService that
signals readiness, or implement a small retry/queue loop that polls
MusicService.getInstance() and invokes updatePlaybackState(songId, title,
artist, album, artworkUrl, duration, position, playing) once non-null; apply the
same pattern anywhere startMusicService() is followed immediately by
MusicService.getInstance()?.updatePlaybackState (also check the
AndroidSystemPlugin occurrences).
… auto-advance - Auto-play on DLNA connect: pause local audio and send current song to renderer immediately (mirrors Chromecast behavior) - 1s GetTransportInfo + GetPositionInfo polling with ChangeNotifier so seek bar, lock screen, and play/pause state update in real time - Rich DIDL-Lite metadata: album, albumArtURI (Subsonic getCoverArt), and duration attribute on <res> - Track auto-advance: detect renderer STOPPED + position >= duration, call _onSongComplete() to play next song - Robust Play-after-SetAVTransportURI: exponential backoff retry (200ms -> 3.2s, 6 attempts) with TRANSITIONING state check, fixes race where GStreamer hasn't finished async pipeline setup for HTTPS - Fix dispose() memory leak: remove UPnP and Cast listeners - Fix connect() validation: use _soap (not _soapQuery) so HTTP errors and SOAP faults are detected
- disconnect() now fires a Stop SOAP command so the renderer actually stops playback when the user disconnects from the cast device - getCoverArtUrl with size=0 now omits the size parameter entirely, letting Navidrome return the original full-resolution cover art (needed for DIDL metadata sent to UPnP renderers)
…state reset - Serialize _poll() with _isPolling flag to prevent overlapping network cycles - _soapQuery() now rejects HTTP errors and SOAP faults instead of returning fault XML silently — prevents polling from misinterpreting fault as STOPPED - Exponential backoff in Play retry clamped to 3200ms max - Reset _position/_duration to zero on UPnP disconnect to clear stale UI - Accept zero duration from renderer instead of ignoring it
- Remove 300ms post-Stop delay (unnecessary for gmrender-resurrect) - Try Play immediately after SetAVTransportURI with no initial wait - Only fall back to retry/backoff if the instant Play fails - Reduce initial backoff from 200ms to 150ms, cap at 2400ms - Cuts ~1-3 seconds off track-to-track transition time
…ction On fresh connect, playSong() was short-circuiting into togglePlayPause() because _currentSong already matched — just resuming whatever old track the renderer had loaded. Now clear _currentSong first so playSong() does a full loadAndPlay. Also align _soapQuery fault detection with _soap: use toLowerCase() and check <s:fault>, <soap:fault>, <fault> consistently.
- Add POST_NOTIFICATIONS permission (required since API 33) - Change MusicService stopWithTask to false so notification persists when app is backgrounded - Start MusicService lazily on first playback instead of eagerly on plugin attach (fixes silent failure on Android 16) - Auto-restart MusicService if killed: both AndroidAutoPlugin and AndroidSystemPlugin detect null instance and restart before updating playback state - Set instance=null in onDestroy so null-check restart triggers - Change START_STICKY to START_NOT_STICKY (service should only live while media is playing) - Remove duplicate updatePlaybackState call in _updateAndroidAuto() - Update notification on Cast/UPnP play, pause, and disconnect - Fix unreachable code: return false instead of rethrow on final Play retry attempt (CodeRabbit review)
Volume control: - UPnP: RenderingControl SOAP (SetVolume/GetVolume), polled every 5s - Cast: wire setVolume() to native sessionManager.setDeviceVolume() - PlayerProvider routes volume to active Cast/UPnP device - Volume slider in UPnP control dialog with live feedback - Android VolumeProviderCompat on MediaSession for system cast volume slider - Hardware volume buttons control renderer when Cast/UPnP connected CodeRabbit fixes: - Defer updatePlaybackState after async startForegroundService (postDelayed) - Handle loadAndPlay() returning false instead of ignoring result
Same fix as f2233ba but for desktop_player_bar.dart — hide RepeatMode from flutter/material.dart to avoid collision with player_provider's RepeatMode.
mthwJsmith
force-pushed
the
fix/upnp-playback-and-polling
branch
from
dd8d2f5 to
9174693
Compare
The player_provider's _volume (0.0-1.0 double) was never synced with the UPnP renderer's actual volume. When connected to DLNA, the stale local phone volume (e.g. 0.95) would leak back to the renderer via Android system volume callbacks, causing the volume to oscillate between the user's intended value and the old stored local value. Fix: on UPnP connect, set _volume to the renderer's current volume. During polling, keep _volume in sync with any external volume changes (e.g. from CEC/TV remote or other control points).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes several issues with UPnP/DLNA playback that made the feature essentially non-functional:
Auto-play on DLNA connect — When connecting to a DLNA renderer, nothing happened. Chromecast had
_onCastStateChangedthat auto-plays the current track on connect, but UPnP had no equivalent. Now pauses local audio and sends the current song to the renderer immediately.Position/state polling — The UPnP service had no polling, so the seek bar, lockscreen, and play/pause state never updated. Added 1-second
GetTransportInfo+GetPositionInfopolling withChangeNotifierintegration so the player provider syncs position, duration, and transport state in real time. The "ideal" approach uses GENA events for state changes and polls only for position, but GENA requires running an HTTP callback server on the phone — most apps skip it entirely and poll everything at 1s (e.g. Castafiore).Rich DIDL metadata —
SetAVTransportURIonly sent title and artist. Now includes<upnp:album>,<upnp:albumArtURI>(from SubsonicgetCoverArt), anddurationattribute on<res>, so renderers can display full now-playing info.Track auto-advance — Song would end on the renderer but the app never played the next track. Added detection: when renderer transitions from PLAYING to STOPPED with position >= duration - 1s, calls
_onSongComplete()to advance the queue.Robust Play-after-SetAVTransportURI — Some renderers (e.g. gmrender-resurrect with GStreamer) return UPnP error 501 if
Playis sent before the async pipeline setup completes for HTTPS streams. Added exponential backoff retry (200ms → 3.2s, 6 attempts) withTRANSITIONINGstate check, inspired by Home Assistant's async_upnp_client.Dispose memory leak fix — Added missing
removeListenercalls for both UPnP and Cast listeners indispose().Connect validation fix —
connect()now uses_soap(not_soapQuery) so HTTP errors and SOAP faults are properly detected instead of silently succeeding.Changes
upnp_service.dart_pollTimerwith 1sGetTransportInfo/GetPositionInfo,UpnpPlaybackStatemodel,_soapQuery()for non-throwing queries,_parseTime()helper, extended_didl()with album/albumArtUrl/durationSecs params, exponential backoff Play retryplayer_provider.dart_onUpnpStateChangedlistener: auto-play on connect, sync position/duration/isPlaying from renderer, track-end detection with auto-advance, reset on disconnect. Pass album, albumArtUrl, durationSecs toloadAndPlay(). AddedremoveListenerindispose()Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Chores