diff options
| -rw-r--r-- | .coverage-baseline | 2 | ||||
| -rw-r--r-- | CLAUDE.md | 15 | ||||
| -rw-r--r-- | app/build.gradle.kts | 4 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/data/AppDatabase.kt | 1 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/data/Daos.kt | 31 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/data/Repositories.kt | 1 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/playback/PlaybackService.kt | 30 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/playback/Playhead.kt | 9 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/playback/PositionWriter.kt | 37 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/data/FakeDaos.kt | 32 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/playback/PositionWriterTest.kt | 145 |
11 files changed, 288 insertions, 19 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index 4b5c51d..d7655a6 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -92.73 +92.96 @@ -210,6 +210,21 @@ by default and correct; it is simply waiting for a publisher who marks breaks. ## Domain notes worth keeping +- **A feed's declared duration is often wrong, and not by rounding.** Radiolab publishes Grass + Apes as `00:22:05`; the file ExoPlayer opens is 29:34. The seven and a half minutes between + them are dynamically inserted ads -- the feed states 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 cares about. `PositionWriter` replaces the stored value with the player's + once per episode, on the position tick, via the narrow `PlaybackStateDao`. Once per episode, + not once per tick: a lookup on every tick for the length of an episode is the per-episode + work the feed-scale note below warns about. + +- **The media notification opens the app**, via `setSessionActivity` on the session. Without it + the transport buttons work and tapping the body does nothing, which reads as breakage rather + than omission. Worth knowing for diagnosis: `dumpsys notification --noredact` shows this app's + notification as `importance=2`, `sound=null`, `vibrate=null`, `category=transport` -- it is a + control surface, and it is silent. + - **Playback position is the crown jewel of the data model.** It must survive process death. Save on a timer, on pause, and on lifecycle transitions — not just on stop. - **Never assume audio.** An enclosure carries a MIME type. Video was cut from scope, but the model 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<EpisodeEntity>) - @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,10 +69,14 @@ 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) @@ -78,6 +84,24 @@ class PlaybackService : MediaLibraryService() { } /** + * 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. * * The queue used to advance from a view model scoped to the activity, so swiping Listen off @@ -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,11 +34,39 @@ 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. * * Ignores the interval deliberately: the next thing that happens may be the process ending. @@ -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<EpisodeEntity>) : 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<EpisodeEntity>() + /** 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) + } } |