feat: optional pre-cache of upcoming queue tracks#866
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds configurable pre-caching for upcoming queue tracks, stable streaming cache keys, streaming cache writer access, Wi-Fi-only controls, and media-service lifecycle hooks for starting and cancelling preload work. ChangesQueue pre-caching
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Sequence Diagram(s)sequenceDiagram
participant BaseMediaService
participant QueuePreloader
participant Player
participant DownloadUtil
participant StreamingCache
BaseMediaService->>QueuePreloader: preload current player timeline
QueuePreloader->>Player: collect eligible upcoming HTTP tracks
QueuePreloader->>DownloadUtil: create streaming cache writer
DownloadUtil->>StreamingCache: cache track data
BaseMediaService->>QueuePreloader: cancel during service teardown
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (1)
app/src/main/java/com/cappielloantonio/tempo/util/DownloadUtil.java (1)
129-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a more generic accessor name.
getStreamingCacheForPreloadties a general-purpose cache accessor to one specific caller's concept ("preload"). A neutral name (e.g. a publicgetStreamingCache) would be more reusable if other future consumers need directCacheaccess.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/cappielloantonio/tempo/util/DownloadUtil.java` around lines 129 - 145, The public cache accessor currently exposes preload-specific naming. Rename getStreamingCacheForPreload to the neutral getStreamingCache, update its callers accordingly, and preserve its existing synchronized behavior and Cache return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/cappielloantonio/tempo/service/QueuePreloader.kt`:
- Around line 88-90: Update the next-window calculation in the QueuePreloader
preload loop to pass the player’s current repeat mode, using player.repeatMode
instead of the hardcoded Player.REPEAT_MODE_OFF. Preserve the existing shuffle
behavior and termination checks so repeat-all wraps correctly and repeat-one
resolves to the current track.
- Around line 42-51: Update QueuePreloader.preload and the network capability
handling in BaseMediaService.onCapabilitiesChanged so an active preloader is
cancelled immediately when the connection transitions from Wi‑Fi to metered.
Track the prior network state, invoke QueuePreloader.cancel() on that
transition, and preserve the existing updateMediaItems behavior for other
capability changes.
---
Nitpick comments:
In `@app/src/main/java/com/cappielloantonio/tempo/util/DownloadUtil.java`:
- Around line 129-145: The public cache accessor currently exposes
preload-specific naming. Rename getStreamingCacheForPreload to the neutral
getStreamingCache, update its callers accordingly, and preserve its existing
synchronized behavior and Cache return value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 288d19ab-68cb-49a8-867b-a7b28ae4afdf
📒 Files selected for processing (8)
app/src/main/java/com/cappielloantonio/tempo/service/BaseMediaService.ktapp/src/main/java/com/cappielloantonio/tempo/service/QueuePreloader.ktapp/src/main/java/com/cappielloantonio/tempo/util/DownloadUtil.javaapp/src/main/java/com/cappielloantonio/tempo/util/Preferences.ktapp/src/main/java/com/cappielloantonio/tempo/util/StreamingCacheKeyFactory.ktapp/src/main/res/values/arrays.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/xml/global_preferences.xml
Review findings from eddyizm#866: an in-flight precache kept downloading over mobile data after Wi-Fi dropped, because only the start of a preload checked the network. The service's network callback now re-invokes preload() on Wi-Fi transitions, which cancels the active write on a disallowed network and restarts it on an allowed one. The upcoming-track walk also honours the player's repeat mode instead of assuming repeat-off, so repeat-all precaches the wrap-around tracks.
|
I am unable to replicate the feat, it seems to take into consideration the buffering of the current song. I tried the following edge case:
I checked the code and the WiFi check is only a short circuit, no further differentiation is made between networks. The streaming of heavy files is a common scenario, specially on WiFi, so the feat gets shadowed by the normal buffering of the app. The queue list only has one icon on the right, perhaps adding a loading icon like DSub2000 could hint the user that the fetching is not completed or hint us about a blockage on the pre-fetch logic. Update: I mage the opposite edge case:
This failed as well, but the buffering of the current song was at 80%. I turned on the WiFi and skipped to the middle of the song until the song buffered completely, then turned it off again. The queue moved with no interruptions to the next song, which was pre-buffered on an 80%. The pre-cache feat is being blocked by the current song buffer status, should it run on a thread instead to handle by its own how much songs ahead it can fetch? |
|
Thanks for the thorough testing — you found a real bug, and your edge cases pinpointed it. The correlation with the current song's buffering turned out to be a side effect rather than the cause: the preloader already runs on its own background thread and never waits on the player. What was actually happening:
Fixed in 9793c30:
Your fresh-install airplane test should now behave as intended, with one caveat: if you cut the network before the next track finished caching, the skip plays the cached portion rather than the full song. The per-row loading/progress icon like DSub2000 is a good idea for making the precache state visible — I'd rather keep it out of this PR's scope, but it would make a nice follow-up. |
|
Will test this again under the same scenarios and I'll report back.
Sounds like a reasonable default. In the meantime, I'd like to bargain once more about
If I understand correctly the app is aware of two situations:
It could be possible to add a visual indicator on the player screen to inform that the next song is available on cache. Something as simple as toggling the visibility of a new element on the screen: Example that adds a lightning icon.
This allows to have a visual feedback of the feat, and at the same time check the feat one skip at a time. If we skip beyond the pre-caching window the icon is hidden, because the app is aware if the next song is cached. From the user side they would know the feature is active and working in a predictable manner. From the developer side, we could enhance that feature by using this feat as a starting point. |
|
I also like the idea of an indicator, but I'd suggest that the indicator is added near/around the next button. Maybe an extra outline would do the job. |
|
I checked the logs against logcat and the feat seems to be working as intended. At timestamp Said that, this feat will only work correctly on real-world scenarios where the connection is weak. This app has two network states: yes-wifi & no-wifi. If I force a change between them, the player will request for the raw song so it will become a cache miss and the feat won't get triggered. Given that we can't reliably test this function, I would mark it on settings as experimental just like the hide bottom navigation bar one. It's not that the feat isn't working, it's just that other bugs shadow it.
I thought of it as well, but the impact on the UI would be really high as the bottom part of the player is too clean. I could be proven wrong if apps like Spotify or Apple Music have clever ways of showcasing something. |
| if (uris.isEmpty()) return | ||
|
|
||
| executor.execute { | ||
| for (uri in uris) { |
There was a problem hiding this comment.
If the array of uris is relative to the current song, the first element must correspond to the next song.
This could trigger an intent if the song is fully cached to handle further UI logic somewhere else. I'm not exactly sure if this runs async or not, the success of the intent would depend of that.
Edit: Actually, broadcasting the uri itself would allow to make the check outside.
|
Experimental label added in 9d578a6 — the setting now reads "Pre-cache upcoming tracks [Experimental]", same convention as the hide-bottom-navbar toggle. Agreed on the reasoning: the feature behaved correctly in your logs, but the binary wifi/no-wifi handling can shadow it, so setting expectations is fair until that's sorted. On the indicator — I'd still keep it a follow-up PR, but here's the direction I'd take when we build it, because I think you're each right about a different half of it. @SinTan1729's instinct to put the signal near the action is right in principle: the state ("this skip will be instant") is about the next button. But decorating @tvillega's standalone icon is the right shape, and I'd anchor it somewhere specific: the media-quality sector at the top of the controller ( Two details I'd consider non-negotiable whenever it lands:
Stating the tradeoff openly: the quality strip is less discoverable than a ring around the next button. I think that's the correct trade — this is passive reassurance (and a handy testing aid while the feature is experimental), not something the user must notice to benefit from. |
|
Fair enough. The feat is ready for merge. |
|
Due to some other prs, this one had some merge conflicts that need a update. |
Adds a setting to pre-cache the next N upcoming tracks of the play queue into the existing streaming cache, so playback continues smoothly through short connectivity drops. Off by default; optional Wi-Fi-only restriction. Cached data lands in the size-limited LRU streaming cache, not in downloads, so it cannot grow storage unbounded. Preload work is cancelled and superseded safely across queue changes and service teardown.
Review findings from eddyizm#866: an in-flight precache kept downloading over mobile data after Wi-Fi dropped, because only the start of a preload checked the network. The service's network callback now re-invokes preload() on Wi-Fi transitions, which cancels the active write on a disallowed network and restarts it on an allowed one. The upcoming-track walk also honours the player's repeat mode instead of assuming repeat-off, so repeat-all precaches the wrap-around tracks.
…e preloader The precache could be defeated by its own lifecycle management, which matched the reported symptom of 'never works until the current song is fully buffered': - preload() cancelled and restarted the in-flight CacheWriter on every player event (timeline updates, metadata refreshes, network callbacks), even when the upcoming-track target was unchanged. For unknown-length (transcoded / chunked) content every restart also wiped the partial cache, so progress kept resetting to zero and the precache only ever completed if it got one long uninterrupted window - e.g. after the current song finished buffering and the player went quiet. - Losing connectivity made the writer throw, and the catch handler wiped the partially cached next track at exactly the moment an offline skip needed it. The skip itself made it worse: the media item transition triggered another preload(), whose cancel wiped the track being transitioned into. A track only survived if it had been cached to 100% (EOF stores the content length, which exempts it from the wipe). Changes: - preload() is now a no-op when the in-flight task already targets the same track list; restarts happen only when the upcoming queue actually changes. - Fully cached tracks are skipped instead of re-written. - The unknown-length partial cleanup moved from the failure path to the start of each write (a leftover partial still can't be resumed with a range request into a fresh transcode). On failure the partial is now kept when the device has no network - it is a valid single-run prefix that lets an imminent offline skip keep playing - and cleaned up as before while online. - preload() bails out when the streaming cache is disabled (size 0), in which case the player bypasses the cache and precaching is pure waste.
9d578a6 to
256734e
Compare
4 participants
What
Split out of #861 as requested by @tvillega. This is the pre-cache feature half.
Adds an opt-in setting to fully cache the next N tracks of the play queue in the background, so skips start instantly and playback survives short signal drops (tunnels, elevators, spotty commutes).
This is the track-count-based prefetch users asked for in the #472 discussion (DSub2000 comparison), with the storage concern raised there addressed: pre-cached audio goes into the existing size-limited LRU streaming cache, not into downloads, so it can never grow storage unbounded — old entries are evicted automatically.
UI
Two new options in Settings, directly below the existing "Song preload buffer" option they complement:
(The time-based preload buffer answers "how much of the current song to buffer"; this answers "how many of the next songs to have ready".)
How
QueuePreloaderwatches queue/timeline changes fromBaseMediaServiceand writes upcoming tracks into the streaming cache via Media3'sCacheWriter.Notes
Testing
assembleDebugbuilds clean.Summary by CodeRabbit