From c496ce8d8fb9d91d41cd544800a1f7fabb5d3ca1 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Mon, 28 Sep 2026 08:32:15 +0000 Subject: Correct durations from the file, and make the notification open the app Two from Peter, one of which turns out to matter more than it looks. **The list said 22m, the player said 29:34.** The feed is lying, not the parser: Radiolab publishes Grass Apes as `00:22:05` and the file ExoPlayer opens is 29:34. Checked against the live feed rather than guessed. The seven and a half minutes between them are dynamically inserted advertising -- the feed declares the ad-free content length and the CDN stitches in the rest -- so the error is large, systematic, and worst on exactly the shows the midroll detector exists for. The player is the first thing in the app that sees the truth, so PositionWriter now writes it back, through a new narrow PlaybackStateDao. Once per episode rather than once per tick: this runs on the position timer for the length of an episode, and a lookup on every tick is precisely the per-episode work that does not scale. A two-second disagreement is left alone, being a container arguing with an encoder rather than a lie, and an unprepared player reporting nothing never wipes what the feed said. PlaybackStateDao is a split by meaning, not by count -- it owns the two things playback learns and writes back, the only writes in the app whose loss is unrecoverable. It also keeps EpisodeDao under detekt's interface limit, which the new query had pushed it over; the config's own comment argues against splitting a DAO to satisfy a count, so this splits it for a reason that would stand on its own. **Tapping the notification opens the app**, via setSessionActivity. It had none, so the transport buttons worked and the body did nothing, which reads as breakage rather than omission. Verified on the emulator against the real feed: Grass Apes stored as 1325000ms on subscribe, 1770553ms after twenty-five seconds of playback. The notification now carries `contentIntent=PendingIntent{... startActivity}` in dumpsys -- I confirmed the intent is attached and targets this app's activity, but did not simulate the physical tap. That same dump corroborates what I told Peter about the notification being Auto's doing and not ours: importance=2, sound=null, vibrate=null, category=transport. ExoPlayhead joins the coverage exclusions. It is four one-line getters over ExoPlayer that happen to live in PlaybackService.kt, so it read as covered by that entry while actually being measured -- it is the seam that makes PositionWriter testable, not logic. 540 unit tests, detekt and lint clean. Coverage 92.73% -> 92.96%. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B --- app/build.gradle.kts | 4 + .../main/java/org/terst/listen/data/AppDatabase.kt | 1 + app/src/main/java/org/terst/listen/data/Daos.kt | 31 ++++- .../java/org/terst/listen/data/Repositories.kt | 1 + .../org/terst/listen/playback/PlaybackService.kt | 30 ++++- .../java/org/terst/listen/playback/Playhead.kt | 9 ++ .../org/terst/listen/playback/PositionWriter.kt | 37 +++++- .../test/java/org/terst/listen/data/FakeDaos.kt | 32 ++++- .../terst/listen/playback/PositionWriterTest.kt | 145 +++++++++++++++++++-- 9 files changed, 272 insertions(+), 18 deletions(-) (limited to 'app') diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 811d044..bc8c4bd 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -264,6 +264,10 @@ val coverageExclusions = listOf( "**/playback/LibraryTree*", "**/playback/NotificationControls*", "**/playback/PodcastPlayer*", + // Four one-line getters over ExoPlayer state. It lives inside PlaybackService.kt and so + // reads as covered by the line above, but it compiles to its own class and was being + // measured. It is the seam that makes PositionWriter testable, not logic in its own right. + "**/playback/ExoPlayhead*", // Room's own machinery, including the migrations -- proven on a device by MigrationTest. "**/data/AppDatabase*", diff --git a/app/src/main/java/org/terst/listen/data/AppDatabase.kt b/app/src/main/java/org/terst/listen/data/AppDatabase.kt index 7a7be96..fe6451c 100644 --- a/app/src/main/java/org/terst/listen/data/AppDatabase.kt +++ b/app/src/main/java/org/terst/listen/data/AppDatabase.kt @@ -37,6 +37,7 @@ class Converters { abstract class AppDatabase : RoomDatabase() { abstract fun subscriptions(): SubscriptionDao abstract fun episodes(): EpisodeDao + abstract fun playbackState(): PlaybackStateDao abstract fun queue(): QueueDao abstract fun rules(): RuleDao abstract fun fingerprints(): ShowFingerprintDao diff --git a/app/src/main/java/org/terst/listen/data/Daos.kt b/app/src/main/java/org/terst/listen/data/Daos.kt index ac80f03..efb39b9 100644 --- a/app/src/main/java/org/terst/listen/data/Daos.kt +++ b/app/src/main/java/org/terst/listen/data/Daos.kt @@ -227,8 +227,6 @@ interface EpisodeDao { @Update suspend fun updateAll(episodes: List) - @Query("UPDATE episodes SET positionMs = :positionMs WHERE id = :id") - suspend fun updatePosition(id: String, positionMs: Long) @Query("UPDATE episodes SET playedAtMs = :atMs, positionMs = 0 WHERE id = :id") suspend fun markPlayed(id: String, atMs: Long) @@ -394,6 +392,35 @@ interface MidrollDao { suspend fun setMidrollCheckedAtMs(id: String, ms: Long) } +/** + * The two things playback learns about an episode and writes back. + * + * Split off [EpisodeDao] by meaning rather than by count, the same reasoning + * [ShowFingerprintDao] follows for the detected offsets: these are written by exactly one + * collaborator, [org.terst.listen.playback.PositionWriter], on a timer, and they are the only + * writes in the app whose loss is not recoverable from anywhere else. + */ +@Dao +interface PlaybackStateDao { + @Query("UPDATE episodes SET positionMs = :positionMs WHERE id = :id") + suspend fun updatePosition(id: String, positionMs: Long) + + /** + * Replaces the feed's claimed length with the file's real one. + * + * Publishers are wrong about this often and by a lot: Radiolab declares Grass Apes at + * 22:05 and serves 29:34, the seven and a half minutes between them being advertising the + * CDN stitches in and the feed knows nothing about. The player is the first thing in the + * app to see the truth. + */ + @Query("UPDATE episodes SET durationMs = :durationMs WHERE id = :id") + suspend fun updateDuration(id: String, durationMs: Long) + + /** Just the column, rather than a whole row, because this runs on the position tick. */ + @Query("SELECT durationMs FROM episodes WHERE id = :id") + suspend fun storedDurationMs(id: String): Long? +} + @Dao interface QueueDao { @Query( diff --git a/app/src/main/java/org/terst/listen/data/Repositories.kt b/app/src/main/java/org/terst/listen/data/Repositories.kt index 17b4ca3..1b174cf 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -50,6 +50,7 @@ class AppGraph(private val context: Context) { val subscriptions: SubscriptionDao get() = db.subscriptions() val episodes: EpisodeDao get() = db.episodes() + val playbackState: PlaybackStateDao get() = db.playbackState() val queueDao: QueueDao get() = db.queue() val ruleDao: RuleDao get() = db.rules() val fingerprints: ShowFingerprintDao get() = db.fingerprints() diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt index 6cd8db5..39af65d 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt @@ -1,6 +1,8 @@ package org.terst.listen.playback +import android.app.PendingIntent import android.content.Context +import android.content.Intent import androidx.media3.common.AudioAttributes import androidx.media3.common.C import androidx.media3.common.MediaItem @@ -67,16 +69,38 @@ class PlaybackService : MediaLibraryService() { // Wrapped so next/previous mean skip-forward/back by seconds; see PodcastPlayer. session = MediaLibrarySession.Builder(this, PodcastPlayer(exo), ListenSessionCallback(this, scope)) .setCustomLayout(NotificationControls.customLayout()) + // Without this the notification is inert to a tap -- the transport buttons work and + // the body does nothing, which reads as the app being broken rather than as a + // deliberate omission. + .setSessionActivity(openAppIntent()) .build() val graph = Graph.get(this) - writer = PositionWriter(ExoPlayhead(exo), graph.episodes) + writer = PositionWriter(ExoPlayhead(exo), graph.playbackState) advancer = graph.advancer exo.addListener(completionListener) startPositionTicker() } + /** + * What tapping the body of the media notification opens. + * + * Reuses the existing activity rather than stacking a second copy: coming back from the + * notification should land where the app already was. FLAG_IMMUTABLE is required from + * Android 12 and is correct here anyway -- nothing needs to fill this intent in. + */ + private fun openAppIntent(): PendingIntent { + val intent = packageManager.getLaunchIntentForPackage(packageName) + ?.setFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP or Intent.FLAG_ACTIVITY_CLEAR_TOP) + return PendingIntent.getActivity( + this, + 0, + intent, + PendingIntent.FLAG_IMMUTABLE or PendingIntent.FLAG_UPDATE_CURRENT, + ) + } + /** * Completion, handled here rather than by whoever happens to be showing the UI. * @@ -228,6 +252,10 @@ private class ExoPlayhead(private val player: ExoPlayer) : Playhead { override val episodeId: String? get() = player.currentMediaItem?.mediaId override val positionMs: Long get() = player.currentPosition override val isPlaying: Boolean get() = player.isPlaying + + // C.TIME_UNSET until the player has prepared, which is a negative sentinel rather than a + // zero -- passing it through as a real length would be worse than admitting it is unknown. + override val durationMs: Long? get() = player.duration.takeIf { it > 0 } } /** Builds the Media3 item for an episode, preferring a downloaded file over the network. */ diff --git a/app/src/main/java/org/terst/listen/playback/Playhead.kt b/app/src/main/java/org/terst/listen/playback/Playhead.kt index 478b0b2..d03de8b 100644 --- a/app/src/main/java/org/terst/listen/playback/Playhead.kt +++ b/app/src/main/java/org/terst/listen/playback/Playhead.kt @@ -4,4 +4,13 @@ interface Playhead { val episodeId: String? val positionMs: Long val isPlaying: Boolean + + /** + * The length of the file actually open, or null before the player has prepared. + * + * Worth having separately from the feed's claim because the two disagree, and not by + * rounding: a show using dynamic ad insertion declares its ad-free length and serves + * something minutes longer. + */ + val durationMs: Long? } diff --git a/app/src/main/java/org/terst/listen/playback/PositionWriter.kt b/app/src/main/java/org/terst/listen/playback/PositionWriter.kt index da3e408..276e24e 100644 --- a/app/src/main/java/org/terst/listen/playback/PositionWriter.kt +++ b/app/src/main/java/org/terst/listen/playback/PositionWriter.kt @@ -1,6 +1,6 @@ package org.terst.listen.playback -import org.terst.listen.data.EpisodeDao +import org.terst.listen.data.PlaybackStateDao /** * Persists the playback position while audio plays. @@ -15,11 +15,12 @@ import org.terst.listen.data.EpisodeDao */ class PositionWriter( private val playhead: Playhead, - private val episodes: EpisodeDao, + private val episodes: PlaybackStateDao, private val progress: PlaybackProgress = PlaybackProgress(), ) { private var lastWrittenMs: Long? = null private var lastEpisodeId: String? = null + private var durationCheckedFor: String? = null /** Called on a timer while playing. Honours the write interval. */ suspend fun capture() { @@ -33,10 +34,38 @@ class PositionWriter( lastWrittenMs = null } + correctDuration(episodeId) + if (!progress.shouldPersist(lastWrittenMs, position)) return write(episodeId, position) } + /** + * Replaces the feed's claimed length with the file's, once per episode. + * + * Publishers are wrong about duration routinely and substantially: Radiolab's feed declares + * Grass Apes at 22:05 and the file is 29:34, the seven and a half minutes between them + * being ads stitched in by the CDN, which the feed cannot know about. The list showed 22m + * while the player counted to 29:34, which is how Peter found it. + * + * The player is the earliest thing that sees the real length, so this rides the position + * tick. It costs one lookup per episode, not one per tick: the tick runs every few seconds + * for the length of an episode, and a query on each would be exactly the per-episode work + * that does not scale. Nothing happens until the player has prepared and has a duration to + * offer, which is why the guard is on having *checked*, not on having tried. + */ + private suspend fun correctDuration(episodeId: String) { + if (durationCheckedFor == episodeId) return + val actual = playhead.durationMs?.takeIf { it > 0 } ?: return + + val stored = episodes.storedDurationMs(episodeId) + durationCheckedFor = episodeId + + // A second either way is the container disagreeing with the encoder, not a lie. + if (stored != null && kotlin.math.abs(stored - actual) <= DURATION_TOLERANCE_MS) return + episodes.updateDuration(episodeId, actual) + } + /** * Called on lifecycle transitions, where there may be no further tick. * @@ -52,4 +81,8 @@ class PositionWriter( episodes.updatePosition(episodeId, positionMs) lastWrittenMs = positionMs } + + private companion object { + const val DURATION_TOLERANCE_MS = 2_000L + } } diff --git a/app/src/test/java/org/terst/listen/data/FakeDaos.kt b/app/src/test/java/org/terst/listen/data/FakeDaos.kt index a8d9a33..1e265f5 100644 --- a/app/src/test/java/org/terst/listen/data/FakeDaos.kt +++ b/app/src/test/java/org/terst/listen/data/FakeDaos.kt @@ -92,9 +92,38 @@ class FakeSubscriptionDao : SubscriptionDao { } } +/** + * The playback writes, over the same row list a [FakeEpisodeDao] holds, so a test can set an + * episode up through one and assert through the other. + */ +class FakePlaybackStateDao(private val rows: MutableList) : PlaybackStateDao { + /** Counts duration lookups, so a test can prove one is not happening per tick. */ + var reads = 0 + private set + + override suspend fun updatePosition(id: String, positionMs: Long) { + replace(id) { it.copy(positionMs = positionMs) } + } + + override suspend fun updateDuration(id: String, durationMs: Long) { + replace(id) { it.copy(durationMs = durationMs) } + } + + override suspend fun storedDurationMs(id: String): Long? { + reads++ + return rows.firstOrNull { it.id == id }?.durationMs + } + + private fun replace(id: String, transform: (EpisodeEntity) -> EpisodeEntity) { + val index = rows.indexOfFirst { it.id == id } + if (index >= 0) rows[index] = transform(rows[index]) + } +} + class FakeEpisodeDao : EpisodeDao { val rows = mutableListOf() + /** Lets a test observe intermediate states that the final row no longer shows. */ var onUpdate: ((EpisodeEntity) -> Unit)? = null @@ -166,9 +195,6 @@ class FakeEpisodeDao : EpisodeDao { } } - override suspend fun updatePosition(id: String, positionMs: Long) { - replace(id) { it.copy(positionMs = positionMs) } - } override suspend fun markPlayed(id: String, atMs: Long) { replace(id) { it.copy(playedAtMs = atMs, positionMs = 0) } diff --git a/app/src/test/java/org/terst/listen/playback/PositionWriterTest.kt b/app/src/test/java/org/terst/listen/playback/PositionWriterTest.kt index 587aeef..1af5279 100644 --- a/app/src/test/java/org/terst/listen/playback/PositionWriterTest.kt +++ b/app/src/test/java/org/terst/listen/playback/PositionWriterTest.kt @@ -6,6 +6,7 @@ import org.junit.Assert.assertTrue import org.junit.Test import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.FakeEpisodeDao +import org.terst.listen.data.FakePlaybackStateDao /** * Position persistence, tested against a narrow playhead abstraction rather than Media3's Player @@ -18,6 +19,7 @@ class PositionWriterTest { override var episodeId: String? = "ep-1", override var positionMs: Long = 0, override var isPlaying: Boolean = true, + override var durationMs: Long? = null, ) : Playhead private val dao = FakeEpisodeDao().apply { @@ -34,10 +36,12 @@ class PositionWriterTest { private fun stored() = dao.rows.single().positionMs + private fun playbackOf(episodes: FakeEpisodeDao) = FakePlaybackStateDao(episodes.rows) + @Test fun `records the position once it has moved enough`() = runTest { val playhead = FakePlayhead(positionMs = 30_000) - val writer = PositionWriter(playhead, dao) + val writer = PositionWriter(playhead, playbackOf(dao)) writer.capture() assertEquals(30_000L, stored()) } @@ -45,7 +49,7 @@ class PositionWriterTest { @Test fun `does not write while the position barely moves`() = runTest { val playhead = FakePlayhead(positionMs = 30_000) - val writer = PositionWriter(playhead, dao) + val writer = PositionWriter(playhead, playbackOf(dao)) writer.capture() playhead.positionMs = 31_000 @@ -56,7 +60,7 @@ class PositionWriterTest { @Test fun `writes again once past the interval`() = runTest { val playhead = FakePlayhead(positionMs = 30_000) - val writer = PositionWriter(playhead, dao) + val writer = PositionWriter(playhead, playbackOf(dao)) writer.capture() playhead.positionMs = 40_000 @@ -68,7 +72,7 @@ class PositionWriterTest { @Test fun `writes immediately on a backward seek`() = runTest { val playhead = FakePlayhead(positionMs = 60_000) - val writer = PositionWriter(playhead, dao) + val writer = PositionWriter(playhead, playbackOf(dao)) writer.capture() playhead.positionMs = 59_000 @@ -80,7 +84,7 @@ class PositionWriterTest { @Test fun `flush writes regardless of how little it moved`() = runTest { val playhead = FakePlayhead(positionMs = 30_000) - val writer = PositionWriter(playhead, dao) + val writer = PositionWriter(playhead, playbackOf(dao)) writer.capture() playhead.positionMs = 30_500 @@ -90,7 +94,7 @@ class PositionWriterTest { @Test fun `nothing is written when no episode is loaded`() = runTest { - val writer = PositionWriter(FakePlayhead(episodeId = null, positionMs = 50_000), dao) + val writer = PositionWriter(FakePlayhead(episodeId = null, positionMs = 50_000), playbackOf(dao)) writer.capture() writer.flush() assertEquals(0L, stored()) @@ -99,7 +103,7 @@ class PositionWriterTest { /** An episode removed while playing must not resurrect a row or throw. */ @Test fun `an unknown episode id is harmless`() = runTest { - val writer = PositionWriter(FakePlayhead(episodeId = "gone", positionMs = 50_000), dao) + val writer = PositionWriter(FakePlayhead(episodeId = "gone", positionMs = 50_000), playbackOf(dao)) writer.capture() assertEquals(1, dao.rows.size) assertEquals(0L, stored()) @@ -109,7 +113,7 @@ class PositionWriterTest { fun `switching episode writes against the right row`() = runTest { dao.rows += dao.rows.single().copy(id = "ep-2", guid = "g2", positionMs = 0) val playhead = FakePlayhead(positionMs = 30_000) - val writer = PositionWriter(playhead, dao) + val writer = PositionWriter(playhead, playbackOf(dao)) writer.capture() playhead.episodeId = "ep-2" @@ -128,7 +132,7 @@ class PositionWriterTest { fun `a new episode is written even at a low position`() = runTest { dao.rows += dao.rows.single().copy(id = "ep-2", guid = "g2", positionMs = 0) val playhead = FakePlayhead(positionMs = 600_000) - val writer = PositionWriter(playhead, dao) + val writer = PositionWriter(playhead, playbackOf(dao)) writer.capture() playhead.episodeId = "ep-2" @@ -142,7 +146,128 @@ class PositionWriterTest { @Test fun `paused playback still flushes`() = runTest { val playhead = FakePlayhead(positionMs = 45_000, isPlaying = false) - PositionWriter(playhead, dao).flush() + PositionWriter(playhead, playbackOf(dao)).flush() assertTrue(stored() == 45_000L) } + + // --- duration correction ----------------------------------------------------------- + // + // Publishers lie about length, and not by rounding. Radiolab's feed declares Grass Apes at + // 22:05; the file ExoPlayer opens is 29:34. The gap is dynamically inserted advertising -- + // the feed states the ad-free content length and the CDN stitches in the rest -- so it is + // both large and systematic. The player knows the truth as soon as it prepares, and that + // is the only place the truth is available without downloading the file. + + private fun withFeedDuration(ms: Long?) = FakeEpisodeDao().apply { + rows += EpisodeEntity( + id = "ep-1", + subscriptionId = "s1", + guid = "g1", + title = "Episode", + mediaUrl = "https://cdn/e.mp3", + mediaType = "audio/mpeg", + discoveredAtMs = 0, + durationMs = ms, + ) + } + + @Test + fun `a wrong feed duration is replaced by what the player actually opened`() = runTest { + val dao = withFeedDuration(22 * 60_000 + 5_000) + val playhead = FakePlayhead(positionMs = 30_000, durationMs = 29 * 60_000 + 34_000) + + PositionWriter(playhead, playbackOf(dao)).capture() + + assertEquals(29 * 60_000L + 34_000L, dao.rows.single().durationMs) + } + + @Test + fun `a feed that never declared a duration gets one`() = runTest { + val dao = withFeedDuration(null) + val playhead = FakePlayhead(positionMs = 30_000, durationMs = 1_774_000) + + PositionWriter(playhead, playbackOf(dao)).capture() + + assertEquals(1_774_000L, dao.rows.single().durationMs) + } + + /** Encoders and containers disagree by a second all the time; that is not worth a write. */ + @Test + fun `a trivial disagreement is left alone`() = runTest { + val dao = withFeedDuration(1_774_000) + val playhead = FakePlayhead(positionMs = 30_000, durationMs = 1_775_500) + + PositionWriter(playhead, playbackOf(dao)).capture() + + assertEquals("unchanged", 1_774_000L, dao.rows.single().durationMs) + } + + /** Before the player has prepared there is no duration to believe. */ + @Test + fun `an unknown player duration does not wipe the feed's`() = runTest { + val dao = withFeedDuration(1_325_000) + val playhead = FakePlayhead(positionMs = 30_000, durationMs = null) + + PositionWriter(playhead, playbackOf(dao)).capture() + + assertEquals(1_325_000L, dao.rows.single().durationMs) + } + + /** + * Once corrected it must stop querying. This runs on the position tick, and a lookup per + * tick per episode for the life of playback is exactly the kind of per-episode work the + * feed-scale note in CLAUDE.md warns about. + */ + @Test + fun `the duration is checked once per episode, not once per tick`() = runTest { + val dao = withFeedDuration(1_325_000) + val playback = playbackOf(dao) + val playhead = FakePlayhead(positionMs = 30_000, durationMs = 1_774_000) + val writer = PositionWriter(playhead, playback) + + writer.capture() + val after = playback.reads + playhead.positionMs = 60_000 + writer.capture() + playhead.positionMs = 90_000 + writer.capture() + + assertEquals("no further lookups", after, playback.reads) + } + + /** A duration that only becomes known later still lands. */ + @Test + fun `a duration that arrives after the first tick is still recorded`() = runTest { + val dao = withFeedDuration(1_325_000) + val playhead = FakePlayhead(positionMs = 30_000, durationMs = null) + val writer = PositionWriter(playhead, playbackOf(dao)) + + writer.capture() + playhead.durationMs = 1_774_000 + playhead.positionMs = 60_000 + writer.capture() + + assertEquals(1_774_000L, dao.rows.single().durationMs) + } + + @Test + fun `moving to another episode checks that one too`() = runTest { + val dao = withFeedDuration(1_325_000).apply { + rows += EpisodeEntity( + id = "ep-2", subscriptionId = "s1", guid = "g2", title = "Two", + mediaUrl = "https://cdn/2.mp3", mediaType = "audio/mpeg", discoveredAtMs = 0, + durationMs = 600_000, + ) + } + val playhead = FakePlayhead(positionMs = 30_000, durationMs = 1_774_000) + val writer = PositionWriter(playhead, playbackOf(dao)) + writer.capture() + + playhead.episodeId = "ep-2" + playhead.positionMs = 10_000 + playhead.durationMs = 999_000 + writer.capture() + + assertEquals(999_000L, dao.rows.first { it.id == "ep-2" }.durationMs) + } } -- cgit v1.2.3