diff options
8 files changed, 126 insertions, 13 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index 76690f3..d6985ea 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -93.75 +93.77 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 f7333f0..e09f34e 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -95,6 +95,7 @@ class AppGraph(private val context: Context) { override suspend fun midrolls(episode: EpisodeEntity) = midrolls.getRanges(episode.id) override suspend fun fetchAudio(episode: EpisodeEntity, kind: SubscriptionKind) = downloader.download(episode, kind).map { } + override suspend fun skipFurniture() = preferences.skipShowFurniture.first() }) } diff --git a/app/src/main/java/org/terst/listen/playback/MidrollSkip.kt b/app/src/main/java/org/terst/listen/playback/MidrollSkip.kt index fdaf7a3..c13656b 100644 --- a/app/src/main/java/org/terst/listen/playback/MidrollSkip.kt +++ b/app/src/main/java/org/terst/listen/playback/MidrollSkip.kt @@ -11,8 +11,30 @@ import org.terst.listen.data.EpisodeMidrollEntity */ object MidrollSkip { - fun skipTarget(ranges: List<EpisodeMidrollEntity>, positionMs: Long): Long? = - ranges.firstOrNull { positionMs >= it.startMs && positionMs < it.endMs } + /** + * The show's own audio -- everything else, including anything unrecognised or empty, counts + * as advertising. That is deliberate: a kind added by a newer server, or a malformed + * response, must not quietly stop being skipped by an older app. A break that silently stops + * being skipped is the failure mode that matters here. + */ + private val SHOW_FURNITURE = setOf("intro", "cue", "outro") + + /** + * Where to jump to if the playhead has entered a break that should be skipped. + * + * Advertising is skipped whatever [skipFurniture] says. The show's own intro, break cue and + * credits are skipped only when asked for. + */ + fun skipTarget( + ranges: List<EpisodeMidrollEntity>, + positionMs: Long, + skipFurniture: Boolean, + ): Long? = + ranges.asSequence() + .filter { skipFurniture || !isShowsOwnAudio(it.kind) } + .firstOrNull { positionMs >= it.startMs && positionMs < it.endMs } ?.endMs ?.takeIf { it > positionMs } + + private fun isShowsOwnAudio(kind: String) = kind in SHOW_FURNITURE } diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt b/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt index 0e76075..254b636 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt @@ -17,6 +17,8 @@ data class PlayingContext( val episode: EpisodeEntity? = null, /** Tier 3 midroll breaks confirmed for this episode -- see [MidrollSkip]. */ val midrolls: List<EpisodeMidrollEntity> = emptyList(), + /** Whether a show's own intro, cue and credits are skipped as well as the advertising. */ + val skipFurniture: Boolean = true, ) /** @@ -33,12 +35,16 @@ interface PlaybackSources { suspend fun fetchAudio(episode: EpisodeEntity, kind: SubscriptionKind): Result<Unit> + /** Whether a show's own intro, cue and credits are skipped as well as the advertising. */ + suspend fun skipFurniture(): Boolean + /** For the cases that never reach outside: no chapters, no midrolls, nothing to extract with. */ object None : PlaybackSources { override suspend fun chapters(episode: EpisodeEntity) = emptyList<Chapter>() override suspend fun midrolls(episode: EpisodeEntity) = emptyList<EpisodeMidrollEntity>() override suspend fun fetchAudio(episode: EpisodeEntity, kind: SubscriptionKind) = Result.failure<Unit>(IllegalStateException("no audio source configured")) + override suspend fun skipFurniture() = true } } @@ -109,7 +115,7 @@ class PlaybackAdvancer( val show = subscriptions.getById(episode.subscriptionId) // Only worth fetching for a show that asked for it; most publish no chapters at all. val chapters = if (show?.skipAdChapters == true) sources.chapters(episode) else emptyList() - return PlayingContext(show, chapters, episode, sources.midrolls(episode)) + return PlayingContext(show, chapters, episode, sources.midrolls(episode), sources.skipFurniture()) } /** diff --git a/app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt b/app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt index 2eff5be..a1dafac 100644 --- a/app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt +++ b/app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt @@ -33,7 +33,8 @@ object PlayheadPolicy { // No per-show toggle, same as intro/outro: the confirmation threshold behind a detected // midroll break is the safety mechanism, not an extra switch nobody would think to flip. - MidrollSkip.skipTarget(context.midrolls, positionMs)?.let { return TickAction.SeekTo(it) } + MidrollSkip.skipTarget(context.midrolls, positionMs, context.skipFurniture) + ?.let { return TickAction.SeekTo(it) } if (!show.skipAdChapters) return TickAction.Nothing return AdChapters.skipTarget(context.chapters, positionMs, durationMs) diff --git a/app/src/test/java/org/terst/listen/playback/MidrollSkipTest.kt b/app/src/test/java/org/terst/listen/playback/MidrollSkipTest.kt index 9e8aea3..617ab59 100644 --- a/app/src/test/java/org/terst/listen/playback/MidrollSkipTest.kt +++ b/app/src/test/java/org/terst/listen/playback/MidrollSkipTest.kt @@ -7,40 +7,92 @@ import org.terst.listen.data.EpisodeMidrollEntity class MidrollSkipTest { - private fun range(startMs: Long, endMs: Long) = EpisodeMidrollEntity("e1", "c1", startMs, endMs) + private fun range(startMs: Long, endMs: Long, kind: String = "ad") = + EpisodeMidrollEntity("e1", "c1", startMs, endMs, kind) @Test fun `no ranges means nothing to skip`() { - assertNull(MidrollSkip.skipTarget(emptyList(), positionMs = 5_000)) + assertNull(MidrollSkip.skipTarget(emptyList(), positionMs = 5_000, skipFurniture = false)) } @Test fun `before a range does nothing`() { val ranges = listOf(range(60_000, 90_000)) - assertNull(MidrollSkip.skipTarget(ranges, positionMs = 10_000)) + assertNull(MidrollSkip.skipTarget(ranges, positionMs = 10_000, skipFurniture = false)) } @Test fun `inside a range jumps to its end`() { val ranges = listOf(range(60_000, 90_000)) - assertEquals(90_000L, MidrollSkip.skipTarget(ranges, positionMs = 75_000)) + assertEquals(90_000L, MidrollSkip.skipTarget(ranges, positionMs = 75_000, skipFurniture = false)) } @Test fun `exactly at the start of a range jumps to its end`() { val ranges = listOf(range(60_000, 90_000)) - assertEquals(90_000L, MidrollSkip.skipTarget(ranges, positionMs = 60_000)) + assertEquals(90_000L, MidrollSkip.skipTarget(ranges, positionMs = 60_000, skipFurniture = false)) } @Test fun `after a range does nothing`() { val ranges = listOf(range(60_000, 90_000)) - assertNull(MidrollSkip.skipTarget(ranges, positionMs = 90_000)) + assertNull(MidrollSkip.skipTarget(ranges, positionMs = 90_000, skipFurniture = false)) } @Test fun `the right one of several non-overlapping ranges is picked`() { val ranges = listOf(range(60_000, 90_000), range(600_000, 630_000)) - assertEquals(630_000L, MidrollSkip.skipTarget(ranges, positionMs = 610_000)) + assertEquals(630_000L, MidrollSkip.skipTarget(ranges, positionMs = 610_000, skipFurniture = false)) + } + + /** Advertising is what the detection is for; there is no case for sitting through it. */ + @Test + fun `advertising is skipped even with furniture skipping off`() { + val ranges = listOf(range(0, 60_000, "ad")) + + assertEquals(60_000L, MidrollSkip.skipTarget(ranges, 10_000, skipFurniture = false)) + } + + @Test + fun `a show's own audio is left alone when furniture skipping is off`() { + val ranges = listOf(range(0, 98_000, "intro")) + + assertNull(MidrollSkip.skipTarget(ranges, 10_000, skipFurniture = false)) + } + + @Test + fun `a show's own audio is skipped when furniture skipping is on`() { + val ranges = listOf(range(0, 98_000, "intro")) + + assertEquals(98_000L, MidrollSkip.skipTarget(ranges, 10_000, skipFurniture = true)) + } + + /** + * A break cue sits immediately before an advertisement, so with furniture skipping off the + * cue plays and the advertisement after it still goes -- which is a useful signal that + * something was cut rather than a gap in the programme. + */ + @Test + fun `a cue is left but the advertisement after it is not`() { + val ranges = listOf(range(600_000, 605_000, "cue"), range(605_000, 725_000, "ad")) + + assertNull(MidrollSkip.skipTarget(ranges, 601_000, skipFurniture = false)) + assertEquals(725_000L, MidrollSkip.skipTarget(ranges, 606_000, skipFurniture = false)) + } + + /** An unknown kind from a newer server must not silently stop being skipped. */ + @Test + fun `an unrecognised kind is treated as advertising`() { + val ranges = listOf(range(0, 60_000, "sponsorship")) + + assertEquals(60_000L, MidrollSkip.skipTarget(ranges, 10_000, skipFurniture = false)) + } + + /** A malformed response can produce an empty kind; it must also be treated as advertising. */ + @Test + fun `an empty kind is treated as advertising`() { + val ranges = listOf(range(0, 60_000, "")) + + assertEquals(60_000L, MidrollSkip.skipTarget(ranges, 10_000, skipFurniture = false)) } } diff --git a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt index a3b5b0d..76d5302 100644 --- a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt +++ b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt @@ -108,6 +108,8 @@ class PlaybackAdvancerTest { override suspend fun fetchAudio(episode: EpisodeEntity, kind: SubscriptionKind) = onFetch(episode) + + override suspend fun skipFurniture() = true } /** Unwraps the common case so the assertions stay about the thing under test. */ diff --git a/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt b/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt index a7ddf77..516df2a 100644 --- a/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt +++ b/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt @@ -30,13 +30,22 @@ class PlayheadPolicyTest { Chapter(startMs = 3 * minute, title = "The interview"), ) + // Six parameters because PlayingContext itself has that many fields worth varying per test; + // a data class wrapper here would just move the long list rather than shorten it. + @Suppress("LongParameterList") private fun decide( show: SubscriptionEntity? = this.show, chapters: List<Chapter> = emptyList(), midrolls: List<EpisodeMidrollEntity> = emptyList(), positionMs: Long, durationMs: Long? = 30 * minute, - ) = PlayheadPolicy.decide(PlayingContext(show, chapters, midrolls = midrolls), positionMs, durationMs, progress) + skipFurniture: Boolean = true, + ) = PlayheadPolicy.decide( + PlayingContext(show, chapters, midrolls = midrolls, skipFurniture = skipFurniture), + positionMs, + durationMs, + progress, + ) @Test fun `mid-episode, with nothing configured, there is nothing to do`() { @@ -147,4 +156,24 @@ class PlayheadPolicyTest { ), ) } + + @Test + fun `does not seek past a show's own intro when furniture skipping is off`() { + val midrolls = listOf(EpisodeMidrollEntity("e1", "c1", 0, 98_000, "intro")) + + assertEquals( + TickAction.Nothing, + decide(midrolls = midrolls, positionMs = 10_000, skipFurniture = false), + ) + } + + @Test + fun `still seeks past advertising when furniture skipping is off`() { + val midrolls = listOf(EpisodeMidrollEntity("e1", "c1", 0, 98_000, "ad")) + + assertEquals( + TickAction.SeekTo(98_000), + decide(midrolls = midrolls, positionMs = 10_000, skipFurniture = false), + ) + } } |