Add AirPlay Receiver Integration & Process Supervision - #60
Conversation
|
|
||
| companion object { | ||
| private val TAG = AirplayMetadataReader::class.java.simpleName | ||
| private const val BUFFER_SIZE = 4096 |
There was a problem hiding this comment.
While this is likely sufficient for metadata, if a shairport-sync metadata burst exceeds 4KB, reader.read(buf) will chunk it. Ensure the AirplayMetadataParser logic remains resilient to items being split across these 4KB boundaries (your current buffer implementation seems to handle this, but it is worth verifying with a stress test).
There was a problem hiding this comment.
The unit test handlesItemSplitAcrossFeeds() in AirplayMetadataParserTest explicitly verifies this: it splits a metadata item at an arbitrary midpoint across two feed() calls and asserts the title is still parsed correctly.
| } | ||
| } finally { | ||
| process.destroy() | ||
| process.waitFor() |
There was a problem hiding this comment.
waitFor() is a blocking call. If a process hangs in a zombie state, this could potentially block the coroutine indefinitely. Consider using withTimeout(TIMEOUT_MS) { withContext(Dispatchers.IO) { process.waitFor() } } to ensure the supervisor can eventually recover even if the native process refuses to exit gracefully.
There was a problem hiding this comment.
The same pattern should be applied to SnapclientProcess and SnapserverProcess for consistency.
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R && | ||
| SdkExtensions.getExtensionVersion(Build.VERSION_CODES.TIRAMISU) >= 7 | ||
| ) { | ||
| nsdManager.stopServiceResolution(listener) |
There was a problem hiding this comment.
While functional, verify that these listeners aren't leaking in memory over long periods (e.g., days of uptime), as resolveListeners is a mutableMapOf. You may want to implement a TTL or a size-limit for this map if the user frequently changes AirPlay senders.
There was a problem hiding this comment.
Entries are removed in both onResolveFailed and onServiceResolved, and stop() / updateCredentials(null) clears the entire map, so under normal disconnect cycles nothing leaks.
| ) { | ||
| IPv4AddressesCard(uiState.ipv4AddressesResult, onRefreshHostAddresses) | ||
|
|
||
| ReceiverStatusBanner( |
There was a problem hiding this comment.
ou could add a "Restart" button inside the Card that triggers the supervisor to reset the specific process, rather than forcing the user to wait for the next backoff interval or manually restart the service
There was a problem hiding this comment.
Good idea, we can add a follow-up PR for this
| // Finished loading and didn't transition to the next screen | ||
| // therefore start nsd routine | ||
| // No stored session: show the Spotify Connect screen. Discovery + | ||
| // login are advertised by RadioBroadcasterService for the life of |
There was a problem hiding this comment.
Consider moving the airplayBridgeCallbacks and the native process launching logic into a dedicated AirplayServiceWrapper or MediaController class to keep the Service class focused on lifecycle and Binder management.
There was a problem hiding this comment.
For now the Service isn't overly large (~200 lines of actual logic post-split) and keeping the callbacks inline makes the DACP↔Snapcast mapping easy to read in one place.
garpernica
left a comment
There was a problem hiding this comment.
Overall, the code is very clean and follows modern Android/Kotlin development patterns (Hilt, Coroutines, StateFlow). The most important area to monitor is the stability of the native process supervisor under varying network conditions, as that is the most likely point of failure in a production environment.
This change introduces AirPlay support via shairport-sync, piping audio through Snapserver as a second stream alongside Spotify. A meta stream activates whichever input is playing, and a separate control bridge handles AirPlay transport commands over DACP.
A restart supervisor with exponential backoff now manages all native processes (snapserver, snapclient, shairport-sync), surfacing health status to the UI through a ReceiverStatusBanner. The Spotify login flow is also decoupled from the receiver stack, adding a "Broadcast without Spotify" button so users can start streaming AirPlay immediately and connect Spotify later.