From 66d9dc7fefe274d000d19511d9b0bd45ecb63dc3 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Wed, 29 Jul 2026 09:13:30 -0500 Subject: [PATCH] Resume the playlist where it was after an Activity rebuild (#234) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported as "if there are 2 pictures or one picture and one video only one plays", and the reporter had never once seen the second item. PlaylistController is constructed with MainActivity, so every rebuild gives it a fresh, empty instance. The playlist then arrives — from the disk cache or the socket, it does not matter which — and the controller sees "0 -> N items", treats it as a first load, and starts at the top. Anything the panel does that recreates the Activity therefore sends playback back to item 1. That would be survivable if it happened rarely. On the reproduction it happened at every item boundary: the device re-registers, the app relaunches itself with NEW_TASK|CLEAR_TOP, onCreate runs, and playback restarts. The second item was on screen for 135ms each cycle, which is why it read as "only one plays" rather than as a glitch. Prod play_logs agree: the second item logging 0-1s durations while the first accumulated every real second of playtime, on two unrelated customer devices. Position now lives in ServerConfig, outside the object that keeps being rebuilt, and start() resumes from it when the save is recent. A cold start, a stale save, a shrunken playlist, a missing save, or a clock that jumped backwards all fall back to starting at the top, so genuine first-runs are untouched. This does NOT address why the panel relaunches itself once per item — that is the noisier half and wants its own change. It does mean a relaunch costs a restarted item instead of a playlist that can never advance. Reproduced first, on an Android 9 emulator with the reporter's exact shape (12MP portrait JPEG + 40s MP4): image 135ms before, a full 10.05s after, with the video holding its 40.1s, over four clean cycles. --- .../com/remotedisplay/player/MainActivity.kt | 6 +- .../remotedisplay/player/data/ServerConfig.kt | 11 ++++ .../player/player/PlaylistController.kt | 19 +++++- .../player/player/PlaylistSelection.kt | 32 ++++++++++ .../player/player/PlaybackResumeTest.kt | 63 +++++++++++++++++++ 5 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 android/app/src/test/java/com/remotedisplay/player/player/PlaybackResumeTest.kt diff --git a/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt b/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt index e4fdb42..29a7a62 100644 --- a/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt +++ b/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt @@ -227,7 +227,11 @@ class MainActivity : AppCompatActivity() { val cid = item.contentId.ifEmpty { item.widgetId ?: "" } if (event == "play_start") wsService?.sendPlayStart(cid, item.filename, item.durationSec) else wsService?.sendPlayEnd(cid, item.filename, completed) - } + }, + // #234: carry the playback position across Activity rebuilds. Without this a relaunch + // restarts the playlist at item 1, so anything after it never gets a turn. + loadResume = { config.resumeIndex.takeIf { it >= 0 }?.let { it to config.resumeAt } }, + saveResume = { index, atMs -> config.resumeIndex = index; config.resumeAt = atMs } ) // Screen-resilience: an item is playable only when its content is actually available — // a widget, a remote stream, or a fully-downloaded local file. A not-yet/failed download is diff --git a/android/app/src/main/java/com/remotedisplay/player/data/ServerConfig.kt b/android/app/src/main/java/com/remotedisplay/player/data/ServerConfig.kt index dd62c60..e621575 100644 --- a/android/app/src/main/java/com/remotedisplay/player/data/ServerConfig.kt +++ b/android/app/src/main/java/com/remotedisplay/player/data/ServerConfig.kt @@ -98,6 +98,17 @@ class ServerConfig(context: Context) { get() = prefs.getString("cached_playlist", "") ?: "" set(value) = prefs.edit().putString("cached_playlist", value).apply() + // #234: last playing index + when it started. Lives here, not in PlaylistController, precisely + // because the controller is rebuilt with every Activity — which is how a relaunch used to reset + // playback to the first item and starve everything after it. + var resumeIndex: Int + get() = prefs.getInt("resume_index", -1) + set(value) = prefs.edit().putInt("resume_index", value).apply() + + var resumeAt: Long + get() = prefs.getLong("resume_at", 0L) + set(value) = prefs.edit().putLong("resume_at", value).apply() + fun clearPlaylistCache() { prefs.edit().remove("cached_playlist").apply() } diff --git a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt index 353439c..d1b2e42 100644 --- a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt +++ b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt @@ -39,7 +39,11 @@ class PlaylistController( private val onWaitingForContent: (() -> Unit)? = null, // Proof-of-play: emitted on each item show ("play_start") and when it's left ("play_end"), // so the caller can forward device:play-event to the server (populates play_logs / Reports). - private val onPlayLog: ((event: String, item: PlaylistItem, completed: Boolean) -> Unit)? = null + private val onPlayLog: ((event: String, item: PlaylistItem, completed: Boolean) -> Unit)? = null, + // #234: playback position, persisted OUTSIDE this object so it survives the controller being + // rebuilt with a new Activity. Null on both = today's behaviour (always start from the top). + private val loadResume: (() -> Pair?)? = null, + private val saveResume: ((index: Int, atMs: Long) -> Unit)? = null ) { private companion object { const val CONTENT_RECHECK_MS = 3000L @@ -289,7 +293,16 @@ class PlaylistController( if (firstActiveIndex() < 0) { showNothingScheduled(); return } // Screen-resilience: only start on an item whose content is downloaded; if the scheduled // content isn't ready yet, keep current/wait (never blank on a loading state). - val idx = PlaylistSelection.firstPlayableIndex(items.size) { playableNow(it) } + // #234: continue where we left off when this is a reload moments after playing (a new + // Activity => a brand-new controller), not a genuine cold start. Scanning from 0 every time + // is what pinned these playlists on their first item forever. + val saved = try { loadResume?.invoke() } catch (_: Throwable) { null } + val from = PlaybackResume.resumeIndex( + saved?.first ?: -1, saved?.second ?: 0L, System.currentTimeMillis(), items.size) + val idx = if (from > 0 && playableNow(from)) from + else if (from > 0) PlaylistSelection.nextPlayableIndex(items.size, from - 1) { playableNow(it) } + else PlaylistSelection.firstPlayableIndex(items.size) { playableNow(it) } + if (from > 0) Log.i("PlaylistController", "Resuming at index $from (reload within resume window)") if (idx >= 0) { currentIndex = idx; playCurrentItem() } else onContentNotReady() } @@ -365,6 +378,8 @@ class PlaylistController( cancelRetry() val item = currentItem ?: return itemStartedAt = System.currentTimeMillis() + // #234: remember where we are so a controller rebuilt seconds from now can carry on. + try { saveResume?.invoke(currentIndex, itemStartedAt) } catch (_: Throwable) {} Log.i("PlaylistController", "Playing: ${item.filename} (index $currentIndex)") onItemChanged(item) hasContentOnScreen = true // a valid item is now rendered — protect it from being blanked diff --git a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt index eefa073..483464c 100644 --- a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt +++ b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt @@ -44,3 +44,35 @@ object PlaylistSelection { fun whenNonePlayable(hasContentOnScreen: Boolean): NonePlayable = if (hasContentOnScreen) NonePlayable.KEEP_CURRENT else NonePlayable.SHOW_WAITING } + +/** + * #234 — where playback should RESUME when a playlist is (re)loaded. + * + * PlaylistController is created fresh with every MainActivity instance, so a recreate always handed + * it an empty list and then a full one, which reads as "0 -> N items" and starts from the top. On a + * panel that re-registers and relaunches itself at each item boundary, item 2 therefore never + * survived more than a fraction of a second: the reporter of #234 had "never seen the photo, just + * the video", and prod play_logs showed the second item logging 0-1s durations while the first + * accumulated all the playtime. + * + * Starting from the top is only correct for a genuinely COLD start. If we were playing moments ago, + * the right thing is to carry on. Kept pure so the window arithmetic is testable without a device. + */ +object PlaybackResume { + /** How recently we must have been playing for a reload to count as a continuation. */ + const val RESUME_WINDOW_MS = 90_000L + + /** + * Index to begin scanning from. [savedIndex] < 0, an empty/short playlist, a stale save, or a + * clock that jumped backwards all fall back to 0 — i.e. to today's behaviour, so a real cold + * start is unaffected. + */ + fun resumeIndex(savedIndex: Int, savedAtMs: Long, nowMs: Long, itemCount: Int): Int { + if (itemCount <= 0) return 0 + if (savedIndex < 0 || savedIndex >= itemCount) return 0 + if (savedAtMs <= 0L) return 0 + val age = nowMs - savedAtMs + if (age < 0L || age > RESUME_WINDOW_MS) return 0 + return savedIndex + } +} diff --git a/android/app/src/test/java/com/remotedisplay/player/player/PlaybackResumeTest.kt b/android/app/src/test/java/com/remotedisplay/player/player/PlaybackResumeTest.kt new file mode 100644 index 0000000..67f2f4b --- /dev/null +++ b/android/app/src/test/java/com/remotedisplay/player/player/PlaybackResumeTest.kt @@ -0,0 +1,63 @@ +package com.remotedisplay.player.player + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * #234 "Android player dont play playlist properly" — with two items only one ever played. + * + * PlaylistController is rebuilt with every MainActivity instance, so a relaunch handed it an empty + * list and then a full one ("0 -> N items") and it started from the top. On a panel that relaunches + * itself at each item boundary, the second item was preempted after ~135ms every single time — + * reproduced on an Android 9 emulator, and matching prod play_logs where the second item recorded + * 0-1s durations while the first accumulated all the playtime. The reporter had never once seen it. + * + * Starting at the top is right for a genuinely cold start and wrong for a reload seconds later. + */ +class PlaybackResumeTest { + + private val NOW = 1_000_000L + private val W = PlaybackResume.RESUME_WINDOW_MS + + @Test fun THE_BUG_a_reload_moments_after_playing_continues_where_it_was() { + assertEquals(1, PlaybackResume.resumeIndex( + savedIndex = 1, savedAtMs = NOW - 5_000, nowMs = NOW, itemCount = 2)) + } + + @Test fun a_genuine_cold_start_still_begins_at_the_top() { + // Nothing saved: unchanged behaviour, which is what makes this safe to ship. + assertEquals(0, PlaybackResume.resumeIndex(-1, 0L, NOW, 3)) + } + + @Test fun a_stale_save_is_ignored_it_is_a_cold_start_not_a_continuation() { + assertEquals(0, PlaybackResume.resumeIndex(2, NOW - (W + 1), NOW, 3)) + } + + @Test fun just_inside_the_window_still_resumes() { + assertEquals(2, PlaybackResume.resumeIndex(2, NOW - (W - 1), NOW, 3)) + } + + @Test fun an_index_past_the_end_falls_back_rather_than_selecting_nothing() { + // The playlist shrank while we were away. + assertEquals(0, PlaybackResume.resumeIndex(7, NOW - 1_000, NOW, 3)) + assertEquals(0, PlaybackResume.resumeIndex(3, NOW - 1_000, NOW, 3)) + } + + @Test fun an_empty_playlist_never_resumes() { + assertEquals(0, PlaybackResume.resumeIndex(1, NOW - 1_000, NOW, 0)) + } + + @Test fun a_clock_that_jumped_backwards_is_treated_as_stale_not_as_fresh() { + // Signage panels do correct their clocks. A negative age must not read as "0ms ago". + assertEquals(0, PlaybackResume.resumeIndex(1, NOW + 60_000, NOW, 2)) + } + + @Test fun a_zero_timestamp_is_not_1970_it_is_no_save_at_all() { + assertEquals(0, PlaybackResume.resumeIndex(1, 0L, NOW, 2)) + } + + @Test fun resuming_at_index_0_is_indistinguishable_from_starting_fresh() { + // Deliberate: index 0 needs no special handling, and the caller treats >0 as "resume". + assertEquals(0, PlaybackResume.resumeIndex(0, NOW - 1_000, NOW, 2)) + } +}