Skip to content

Add AVPlayer-based streaming player#82

Merged
briankeane merged 9 commits into
mainfrom
briankeane/streaming-player
Mar 21, 2026
Merged

Add AVPlayer-based streaming player#82
briankeane merged 9 commits into
mainfrom
briankeane/streaming-player

Conversation

@briankeane

Copy link
Copy Markdown
Collaborator

Summary

  • Adds StreamingStationPlayer as an alternative to PlayolaStationPlayer that streams audio via AVPlayer instead of downloading entire files first, significantly reducing startup latency
  • Extracts ScheduleService for shared schedule-fetching logic (DRY between both players)
  • Adds FadeScheduleBuilder for timer-based fade automation mirroring the web player's approach
  • Modifies ListeningSessionReporter to accept a publisher-based init, supporting both player types
  • Adds streaming/download toggle to the example app for side-by-side comparison

Test plan

  • 37 new tests across 3 test suites (FadeScheduleBuilder, StreamingSpinPlayer, StreamingStationPlayer)
  • All 124 existing tests still pass
  • Manual testing: run example app, toggle between Streaming and Download modes
  • Manual testing: verify crossfade quality matches download player
  • Manual testing: verify listening sessions are reported correctly
  • Manual testing: test audio interruptions (Bluetooth, phone calls)

🤖 Generated with Claude Code

briankeane and others added 4 commits March 2, 2026 10:41
Introduces StreamingStationPlayer as an alternative to PlayolaStationPlayer
that streams audio via AVPlayer instead of downloading entire files first.
This significantly reduces startup latency for long audio files.

New files:
- FadeScheduleBuilder: pure fade schedule generation (shared utility)
- AVPlayerProviding: protocol wrapping AVPlayer for testability
- StreamingSpinPlayer: AVPlayer-based single spin playback
- StreamingStationPlayer: orchestrator with schedule polling
- ScheduleService: extracted shared schedule-fetching logic

Modified:
- ListeningSessionReporter: added publisher-based init for both players

37 new tests across 3 test suites, all passing.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add player mode picker (Streaming vs Download) to ContentView
- Stop current player before switching stations
- Hard-code default station ID for testing

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Mar 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR adds StreamingStationPlayer as an AVPlayer-based alternative to the existing PlayolaStationPlayer, extracting shared schedule-fetching logic into ScheduleService, adding FadeScheduleBuilder for timer-driven volume automation, and refactoring ListeningSessionReporter to accept a publisher-based init so it works with both player types. The overall design is solid and well-tested.

Notable changes:

  • StreamingStationPlayer streams via AVPlayer instead of downloading full files first, reducing startup latency
  • FadeScheduleBuilder correctly handles overlapping fades via a lastTimeMS monotonicity guard (addressing a prior review concern)
  • AVPlayerWrapper.waitForReady correctly handles .unknown and @unknown default KVO statuses
  • Basic-auth fallback and its hardcoded token have been removed from ListeningSessionReporter; unauthenticated calls now throw instead of silently falling back

Issues found:

  • Logic: loadAndPlaySpin is async and can have stop() interleave at the await player.load suspension point. After stop() sets state to .idle and clears players, the resumed loadAndPlaySpin overwrites state back to .playing(spin) with no active players behind it — a stale UI state with no way to recover
  • Style: ScheduleService.getSchedule uses URLSession.shared directly instead of an injectable URLSessionProtocol, making it untestable and inconsistent with the rest of the codebase
  • Style: handleAudioRouteChange(_:) and handleAudioSessionInterruption(_:) in StreamingStationPlayer are @objc public but should be @objc private

Confidence Score: 3/5

  • Safe to merge after the play()/stop() race condition is addressed; the other issues are non-blocking style improvements.
  • The PR is well-structured and extensively tested (37 new tests). Most of the previously-flagged issues from prior review threads have been addressed. One new logic-level race condition remains: loadAndPlaySpin can set state = .playing(spin) after stop() has already transitioned to .idle, leaving the station player in a permanently broken state. The URLSession injection gap in ScheduleService is a testability concern but not a runtime bug. The @objc public notification handlers are a minor API hygiene issue.
  • Sources/PlayolaPlayer/Player/Streaming/StreamingStationPlayer.swift — specifically the loadAndPlaySpin method and the @objc public notification handlers

Important Files Changed

Filename Overview
Sources/PlayolaPlayer/Player/Streaming/StreamingStationPlayer.swift New station player using AVPlayer streaming; contains a play()/stop() interleaving race where loadAndPlaySpin resumes after stop() and overwrites .idle with .playing. Notification handlers are unnecessarily public.
Sources/PlayolaPlayer/Player/Streaming/StreamingSpinPlayer.swift Well-structured single-spin streaming player; previous race condition in playNow Task and clear() state clobber have been addressed in this version with proper guard checks.
Sources/PlayolaPlayer/Player/ScheduleService.swift Good DRY extraction of schedule-fetching logic, but uses URLSession.shared directly instead of an injectable URLSessionProtocol, inconsistent with the rest of the codebase and preventing unit tests.
Sources/PlayolaPlayer/Player/Streaming/FadeScheduleBuilder.swift Clean, well-tested fade automation utility. Overlapping-fade monotonicity fix (via lastTimeMS guard) is correct and the volumeAtMS early-break assumption now holds.
Sources/PlayolaPlayer/Player/Streaming/AVPlayerProviding.swift Clean testability abstraction over AVPlayer. The @unknown default continuation issue flagged in a prior review has been addressed — it now resumes with an error rather than leaking.
Sources/PlayolaPlayer/Player/ListeningSessionReporter.swift Good refactor: publisher-based init enables reuse with StreamingStationPlayer; Basic-auth fallback and hardcoded token correctly removed. Subscription is properly stored in disposeBag.
PlayolaPlayerExample/PlayolaPlayerExample/ContentView.swift Adds player-mode toggle between streaming and download. stopCurrentPlayer() is now async and correctly awaited in all call sites, addressing the previous fire-and-forget concern.
Sources/PlayolaPlayer/Player/PlayolaStationPlayer.swift Minimal change: base URL stripped of /v1 prefix to match ScheduleService/StreamingStationPlayer convention. The single URL construction call (appending(path: "/v1/...")) is unaffected.

Sequence Diagram

sequenceDiagram
    participant App
    participant SSP as StreamingStationPlayer
    participant SchedSvc as ScheduleService
    participant SpinPlayer as StreamingSpinPlayer
    participant AVW as AVPlayerWrapper

    App->>SSP: play(stationId, atDate)
    SSP->>SchedSvc: getSchedule(stationId, baseUrl)
    SchedSvc-->>SSP: Schedule
    SSP->>SpinPlayer: load(spin)
    SpinPlayer->>AVW: loadURL(url)
    AVW-->>SpinPlayer: readyToPlay via KVO
    SpinPlayer-->>SSP: .success
    SSP->>SpinPlayer: playNow(from offset) or schedulePlay(at airtime)
    SpinPlayer->>AVW: seek + play
    SpinPlayer-->>SSP: startedPlaying(spin)
    SSP->>SSP: state = .playing(spin)
    loop Every 20s polling
        SSP->>SchedSvc: getSchedule poll
        SchedSvc-->>SSP: Updated schedule
        SSP->>SpinPlayer: load future spin
        SpinPlayer->>SpinPlayer: schedulePlay(at airtime)
    end
    Note over SpinPlayer: clearTimer fires at endtime+1s
    SpinPlayer->>SpinPlayer: state = .finished then clear
    SpinPlayer-->>SSP: didChangeState(.finished)
    SSP->>SSP: remove from spinPlayers
    SSP->>SSP: if empty, state = .idle
    App->>SSP: stop()
    SSP->>SpinPlayer: stop then clear
    SSP->>SSP: state = .idle
Loading

Last reviewed commit: "Fix review round 2: ..."

Comment thread Sources/PlayolaPlayer/Player/Streaming/AVPlayerProviding.swift
Comment thread Sources/PlayolaPlayer/Player/Streaming/StreamingSpinPlayer.swift Outdated
Comment thread PlayolaPlayerExample/PlayolaPlayerExample/ContentView.swift
briankeane and others added 2 commits March 20, 2026 19:32
- Replace 3-member tuple return with PlayerTestContext struct
- Use `where` clause instead of `if` inside `for` loop

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- P1: AVPlayerWrapper.waitForReady now keeps observing on .unknown status
  instead of leaking the continuation; throws on @unknown default
- P1: StreamingSpinPlayer.playNow guards state after await seek to prevent
  clear() race from leaving player in .playing with dangling timers
- P1: StreamingStationPlayer adds deinit to remove NotificationCenter
  observers, preventing retain cycle / memory leak
- P2: stopCurrentPlayer() is now async; callers await it before starting
  new playback to prevent stop/play races

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@briankeane

Copy link
Copy Markdown
Collaborator Author

@greptile one more pass, please!

Comment thread Sources/PlayolaPlayer/Player/Streaming/StreamingStationPlayer.swift
Comment thread Sources/PlayolaPlayer/Player/Streaming/FadeScheduleBuilder.swift
- P1: StreamingStationPlayer transitions to .idle when last spin finishes
  and spinPlayers is empty
- P1: Change @ObservedObject to @StateObject for streamingPlayer in
  ContentView so SwiftUI owns its lifecycle
- P2: FadeScheduleBuilder skips overlapping steps to ensure monotonic
  time ordering when fades are closer than fadeDurationMS
- Add test for overlapping fade edge case

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@briankeane

Copy link
Copy Markdown
Collaborator Author

@greptile one more pass!

Comment on lines +225 to +244

do {
try await Task.sleep(for: .seconds(StreamingStationPlayer.schedulePollingInterval))
} catch {
return
}
}
}

private func performScheduleUpdate(stationId: String) async throws {
let updatedSchedule = try await ScheduleService.getSchedule(
stationId: stationId, baseUrl: baseUrl)

let spinsToLoad = updatedSchedule.current(offsetTimeInterval: scheduleOffset).filter {
$0.airtime < .now + StreamingStationPlayer.scheduleWindow
}

for spin in spinsToLoad {
try Task.checkCancellation()
if spinPlayers[spin.id] == nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 play() state overwritten after interleaved stop() call

loadAndPlaySpin is async and suspends at await player.load(spin). Because StreamingStationPlayer is @MainActor, stop() can run at that suspension point — it clears spinPlayers, nils stationId, and sets self.state = .idle. When loadAndPlaySpin then resumes, it unconditionally proceeds to player.playNow(from:) and sets self.state = .playing(spin), overwriting the .idle state.

This leaves the station player stuck in .playing(spin) with no active spin players, no active scheduling loop, and a stale now-playing spin in the UI.

The fix is to add a cancellation guard immediately after the await player.load(spin) call completes — check self.stationId != nil (or self.state != .idle) before proceeding to start playback, and if stop was called, call player.clear() and remove the player from spinPlayers before returning.

briankeane and others added 2 commits March 20, 2026 20:11
After awaiting player.load(), check that stationId is still set before
proceeding to playback. If stop() ran during the suspension, clean up
the player instead of overwriting the .idle state.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@briankeane
briankeane merged commit b2f872f into main Mar 21, 2026
4 checks passed
@briankeane
briankeane deleted the briankeane/streaming-player branch March 21, 2026 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant