diff options
9 files changed, 123 insertions, 22 deletions
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 8dbaa32..b971aac 100644 --- a/app/src/main/java/org/terst/listen/data/Daos.kt +++ b/app/src/main/java/org/terst/listen/data/Daos.kt @@ -380,7 +380,10 @@ interface MidrollDao { @Query("DELETE FROM show_midrolls WHERE subscriptionId = :subscriptionId AND id = :id") suspend fun deleteCandidate(subscriptionId: String, id: String) - @Query("SELECT * FROM episode_midrolls WHERE episodeId = :episodeId") + // Ordered by start: MidrollSkip.skipTarget filters by kind before firstOrNull, so an + // unordered result could let a short cue nested inside an ad pod win over the pod and + // produce a 5-second seek instead of the full jump. + @Query("SELECT * FROM episode_midrolls WHERE episodeId = :episodeId ORDER BY startMs") suspend fun getRanges(episodeId: String): List<EpisodeMidrollEntity> @Insert(onConflict = OnConflictStrategy.REPLACE) 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 e09f34e..46731c8 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -95,7 +95,15 @@ 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() + // DataStore's flow can throw (CorruptionException, IOException) if the preferences + // file is damaged, and this runs on PlaybackService's bare scope.launch with no + // CoroutineExceptionHandler -- unguarded, that kills the media service process the + // moment an episode starts. Default to true (furniture skipped) rather than false: + // that is today's behaviour, and it is the fail-safe direction -- a break that + // silently stops being skipped is the failure mode that matters here, so losing the + // preference must not quietly let a show's own audio start playing again. + override suspend fun skipFurniture() = + runCatching { preferences.skipShowFurniture.first() }.getOrDefault(true) }) } diff --git a/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt b/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt index 47bb248..71907bd 100644 --- a/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt +++ b/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt @@ -96,6 +96,20 @@ object SkipSummary { else -> "Ad break" } + /** + * Whether a break is advertising, as its own behavioural predicate rather than a comparison + * against [describeKind]'s display string. + * + * Reuses the exact rule [org.terst.listen.playback.MidrollSkip] already encodes for what + * actually gets skipped unconditionally: anything that is not the show's own recurring audio + * (intro, cue, outro) counts as advertising, including a kind this version does not + * recognise and an empty string. The episode screen's ad/furniture colour split calls this + * instead of comparing against the English label, because renaming that label used to invert + * the colours for every band -- and since describeKind's else branch returns "Ad break" for + * anything unrecognised, renaming the ad label would have made every band draw as furniture. + */ + fun isAdvertising(kind: String): Boolean = kind !in setOf("intro", "cue", "outro") + /** One end of a show's episodes -- its intro or its outro. */ fun candidateState(state: ShowFingerprintEntity?, threshold: Int): String { // No reference clip means no candidate: either nothing has been processed, or a single 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 a1dafac..02b2768 100644 --- a/app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt +++ b/app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt @@ -31,8 +31,11 @@ object PlayheadPolicy { return TickAction.Finish } - // 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. + // Advertising is skipped unconditionally; the show's own audio (intro, cue, outro) is + // skipped only when context.skipFurniture says to -- the global preference read at + // episode start. There is still no *per-show* toggle: the confirmation threshold behind + // a detected midroll break is the safety mechanism for whether a break is trusted at + // all, and the furniture preference is the only switch on top of that. MidrollSkip.skipTarget(context.midrolls, positionMs, context.skipFurniture) ?.let { return TickAction.SeekTo(it) } diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt b/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt index 9baff97..70f4a77 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt @@ -295,9 +295,10 @@ private fun EpisodeScrubber(playback: EpisodePlayback, actions: EpisodeDetailAct /** * The detected breaks as bands across the track, advertising in one colour and the show's own - * audio (intro, cue, outro) in another -- the same distinction [SkipSummary.describeKind] names - * in words, so a kind this version does not recognise draws as advertising too, matching what - * playback does with it. + * audio (intro, cue, outro) in another, decided by [SkipSummary.isAdvertising] rather than by + * comparing against [SkipSummary.describeKind]'s display string -- a renamed label must not + * silently invert which colour means advertising. A kind this version does not recognise draws + * as advertising too, matching what playback does with it. */ @Composable private fun BreakMarks(breaks: List<SkipSummary.BreakMarker>, modifier: Modifier) { @@ -312,7 +313,7 @@ private fun BreakMarks(breaks: List<SkipSummary.BreakMarker>, modifier: Modifier breaks.forEach { mark -> val left = inset + usable * mark.start val right = inset + usable * mark.end - val isAd = SkipSummary.describeKind(mark.kind) == "Ad break" + val isAd = SkipSummary.isAdvertising(mark.kind) drawRect( color = if (isAd) adColour else furnitureColour, topLeft = Offset(left, 0f), diff --git a/app/src/test/java/org/terst/listen/ReachabilityTest.kt b/app/src/test/java/org/terst/listen/ReachabilityTest.kt index da7d9a3..ec603fa 100644 --- a/app/src/test/java/org/terst/listen/ReachabilityTest.kt +++ b/app/src/test/java/org/terst/listen/ReachabilityTest.kt @@ -27,6 +27,14 @@ class ReachabilityTest { "YouTubeAudio.ytDlpVersion" to "Called from YouTubeAudioTest, which fails on a stale bundled yt-dlp. That guard " + "is the whole point; it has no job in production.", + "SkipSummary.describeKind" to + "Only called from SkipSummary.breakLines, in the same file, since the episode " + + "screen's break-list labels and its ad/furniture colour split (FIX 3) were " + + "deliberately decoupled -- the colour split now uses isAdvertising, not this " + + "display string. This heuristic only credits a cross-file call as a caller, so " + + "it cannot see that breakLines is reached from EpisodeScreen.kt via " + + "SkipSummary.episode(); the function is genuinely live, just one hop further " + + "than the check looks.", ) private val mainDir = File("src/main/java/org/terst/listen") diff --git a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt index b1decbd..367bf04 100644 --- a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt +++ b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt @@ -390,6 +390,25 @@ class SkipSummaryTest { assertEquals("Ad break", SkipSummary.describeKind("sponsorship")) } + /** + * [isAdvertising] is the behavioural predicate the episode screen's colour split must use + * instead of comparing against [describeKind]'s English copy -- the bug it fixes: renaming + * "Ad break" would otherwise invert the colours for every band, because describeKind's else + * branch would still return the renamed string for nothing while every real "ad" stopped + * matching it. Includes the unknown-kind and empty-string cases, which both have to read as + * advertising -- the same rule [org.terst.listen.playback.MidrollSkip] already encodes for + * what actually gets skipped. + */ + @Test + fun `isAdvertising matches the kinds that are actually skipped unconditionally`() { + assertEquals(true, SkipSummary.isAdvertising("ad")) + assertEquals(false, SkipSummary.isAdvertising("intro")) + assertEquals(false, SkipSummary.isAdvertising("cue")) + assertEquals(false, SkipSummary.isAdvertising("outro")) + assertEquals(true, SkipSummary.isAdvertising("sponsorship")) + assertEquals(true, SkipSummary.isAdvertising("")) + } + @Test fun `break markers carry the kind so the scrubber can distinguish them`() { val ranges = listOf( diff --git a/server/internal/skips/kinds.go b/server/internal/skips/kinds.go index e2518dd..89ee1cb 100644 --- a/server/internal/skips/kinds.go +++ b/server/internal/skips/kinds.go @@ -29,19 +29,29 @@ const ( // // The length gap is the evidence, and the point of using it is that the detector does not // produce it: the feed declares the ad-free length and the file is whatever was served. An -// episode no longer than its declared length had nothing inserted, so whatever recurs in it is -// the show's own audio however strongly it matched. +// episode served at the same length as declared -- within encoding slop, in either direction -- +// had nothing inserted, so whatever recurs in it is the show's own audio however strongly it +// matched. // -// Everything else is called advertising. That is deliberately the unrefined answer -- an -// ad-carrying episode also contains its own intro and cue -- but it matches what the app does -// today, and separating the two inside such an episode needs the furniture library, which cannot -// confirm anything until a second fetch occasion has passed. +// Everything else is called advertising, including a served length materially *shorter* than +// declared: a feed wrong about its own length in that direction cannot be trusted to judge +// whether anything was inserted in the other one either, and treating "shorter" as proof of +// nothing-inserted let a real pre-roll (a feed declaring 30:00 and serving 29:30) fall into the +// opening window and get labelled the show's own intro. Calling everything else advertising is +// also deliberately the unrefined answer for the case that is judgeable -- an ad-carrying episode +// also contains its own intro and cue -- but it matches what the app does today, and separating +// the two inside such an episode needs the furniture library, which cannot confirm anything until +// a second fetch occasion has passed. func Classify(breaks []AdBreak, servedMs, declaredMs int64) []AdBreak { out := make([]AdBreak, 0, len(breaks)) // If we can't judge (missing served or declared length), treat conservatively as inserted ads. - // If both lengths are known and the file is longer than declared by more than encoding slop, - // it contains inserted ads. - inserted := servedMs <= 0 || declaredMs <= 0 || (servedMs > 0 && declaredMs > 0 && servedMs-declaredMs > durationSlopMs) + // If both lengths are known and they differ by more than encoding slop in either direction, + // the feed's declared length cannot be trusted as the ad-free baseline, so treat it as inserted. + gapMs := servedMs - declaredMs + if gapMs < 0 { + gapMs = -gapMs + } + inserted := servedMs <= 0 || declaredMs <= 0 || gapMs > durationSlopMs for _, b := range breaks { b.Kind = KindAd @@ -60,7 +70,10 @@ func Classify(breaks []AdBreak, servedMs, declaredMs int64) []AdBreak { // by the nearest edge. func furnitureKindAt(b AdBreak, servedMs int64) BreakKind { if servedMs <= 0 { - return KindCue // shouldn't happen but be safe + // Unreachable today -- Classify sets inserted = true whenever servedMs <= 0, so this is + // never called with one. Fail-safe anyway: if that precondition ever moves, an + // unjudgeable length must still read as advertising, not as the show's own furniture. + return KindAd } // Use a window size proportional to the episode length for very short episodes. diff --git a/server/internal/skips/kinds_test.go b/server/internal/skips/kinds_test.go index 544b9ea..16c4e74 100644 --- a/server/internal/skips/kinds_test.go +++ b/server/internal/skips/kinds_test.go @@ -54,13 +54,45 @@ func TestAnEpisodeMatchingItsDeclaredLengthIsAllFurniture(t *testing.T) { } } -// The served file is sometimes *shorter* than declared -- Sleepy History runs about 1:36 short on -// every episode. A feed can be wrong in that direction too, and it is still not advertising. +// The served file is sometimes *shorter* than declared by more than encoding slop -- and that is +// exactly the case Classify cannot trust: a feed that understates how much was served is wrong in +// the same way one that overstates it is, and the fix for FIX 1 made the test symmetric instead of +// one-sided. This used to assert KindOutro (the old one-sided rule read any served-at-or-under- +// declared length as ad-free); it now asserts KindAd, because a feed wrong by 96 seconds cannot be +// trusted to judge whether anything was inserted, which is exactly the unjudgeable case Classify +// already answers with KindAd. Kept rather than deleted: it is still the case that documents why. func TestAnEpisodeShorterThanDeclaredIsAlsoAllFurniture(t *testing.T) { got := Classify([]AdBreak{{StartMs: 2_793_000, EndMs: 2_989_000}}, 2_963_000, 3_059_000) - if got[0].Kind != KindOutro { - t.Errorf("kind %q, want %q -- a short file has no insertion either", got[0].Kind, KindOutro) + if got[0].Kind != KindAd { + t.Errorf("kind %q, want %q -- a feed wrong by 96s in the short direction can't be trusted either", got[0].Kind, KindAd) + } +} + +// A feed can also be wrong in the short direction by only a few seconds, which is exactly what +// slop exists to absorb -- this is the symmetric case of TestASecondOfDisagreementIsNotAnAdvertisement. +func TestASecondOfDisagreementInTheShortDirectionIsNotAnAdvertisement(t *testing.T) { + got := Classify([]AdBreak{{StartMs: 0, EndMs: 30_000}}, 3_040_000, 3_040_900) + + if got[0].Kind == KindAd { + t.Error("900ms of rounding in the short direction was read as inserted advertising") + } +} + +// FIX 1: a show that declares 30:00 and is served 29:30 -- 28:00 of programme plus a 1:30 +// pre-roll -- used to pass Classify's one-sided test (servedMs - declaredMs is negative, so +// nothing was read as inserted) and the pre-roll at 0-90s fell inside the opening window and was +// labelled KindIntro. With the furniture toggle off, that advertisement would play in full on +// every episode of the show forever, with nothing anywhere reporting it as advertising. The rule +// has to be symmetric: only a served length within slop of declared, in either direction, is +// evidence that nothing was inserted. A served length materially shorter than declared means the +// feed cannot be trusted as a judge, which is the unjudgeable case Classify already answers with +// KindAd. +func TestAServedLengthShorterThanDeclaredByMoreThanSlopIsUnjudgeableAndTreatedAsAdvertising(t *testing.T) { + got := Classify([]AdBreak{{StartMs: 0, EndMs: 90_000}}, 1_770_000, 1_800_000) + + if got[0].Kind != KindAd { + t.Errorf("kind %q, want %q -- a feed declaring 30:00 and serving 29:30 can't be trusted to judge whether its own pre-roll is advertising", got[0].Kind, KindAd) } } |