diff options
Diffstat (limited to 'app/src')
19 files changed, 572 insertions, 91 deletions
diff --git a/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt b/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt index 091d8e6..2c7440d 100644 --- a/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt +++ b/app/src/androidTest/java/org/terst/listen/data/MigrationTest.kt @@ -416,4 +416,67 @@ class MigrationTest { assertEquals(4242L, cursor.getLong(5)) } } + + /** + * Midroll reference clips move out of the database and into files. + * + * A real ad break runs minutes -- Peter named Tangle and The Daily Show, both of which wrap + * theirs in theme music -- and the segment worth matching includes that music, so the clip + * cannot be trimmed to fit a 2MB row. + * + * Existing candidates are dropped rather than converted: a migration cannot write files, + * and every stored candidate was capped at sixty seconds by the code that wrote it, so it + * is the wrong length anyway. They are a cache -- detection simply starts again. The + * confirmed intro/outro fingerprints in the other table are untouched, because those are + * small and correct. + */ + @Test + fun migrates10To11MovingMidrollClipsToFilesAndKeepingFingerprints() { + helper.createDatabase(name, 10).use { db -> + db.execSQL( + """INSERT INTO subscriptions + (id, feedUrl, kind, title, addedAtMs, skipIntroMs, skipOutroMs, dripEnabled, + dripIntervalMs, dripCount, dripOldestFirst, skipAdChapters, bedtime) + VALUES ('s1', 'https://example.com/feed.xml', 'PODCAST', 'A Show', 1000, 0, 0, + 0, 86400000, 1, 1, 1, 0)""" + ) + db.execSQL( + """INSERT INTO show_fingerprints + (subscriptionId, kind, referenceClipPcm, pendingEpisodeId, sampleRate, + confirmedCount, lastConfirmedEpisodeId, updatedAtMs) + VALUES ('s1', 'INTRO', X'0A0B', NULL, 8000, 4, 'e4', 5150)""" + ) + db.execSQL( + """INSERT INTO show_midrolls + (subscriptionId, id, referenceClipPcm, sampleRate, confirmedCount, + lastConfirmedEpisodeId, updatedAtMs) + VALUES ('s1', 'c1', X'0102', 8000, 3, 'e3', 6000)""" + ) + } + + val db = helper.runMigrationsAndValidate(name, 11, true, AppDatabase.MIGRATION_10_11) + + db.query("SELECT COUNT(*) FROM show_midrolls").use { cursor -> + cursor.moveToFirst() + assertEquals("stale capped candidates are dropped, not converted", 0, cursor.getInt(0)) + } + + db.query("SELECT confirmedCount FROM show_fingerprints WHERE subscriptionId = 's1'").use { cursor -> + assertEquals("a confirmed intro must not be collateral damage", 1, cursor.count) + cursor.moveToFirst() + assertEquals(4, cursor.getInt(0)) + } + + // The new shape is usable, holding a path rather than audio. + db.execSQL( + """INSERT INTO show_midrolls + (subscriptionId, id, referenceClipPath, sampleRate, confirmedCount, + lastConfirmedEpisodeId, updatedAtMs) + VALUES ('s1', 'c2', '/data/midrolls/s1/c2.pcm', 8000, 2, 'e5', 7000)""" + ) + db.query("SELECT referenceClipPath FROM show_midrolls WHERE id = 'c2'").use { cursor -> + cursor.moveToFirst() + assertEquals("/data/midrolls/s1/c2.pcm", cursor.getString(0)) + } + } } 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 d1b7ed2..7a7be96 100644 --- a/app/src/main/java/org/terst/listen/data/AppDatabase.kt +++ b/app/src/main/java/org/terst/listen/data/AppDatabase.kt @@ -30,7 +30,7 @@ class Converters { ShowMidrollEntity::class, EpisodeMidrollEntity::class, ], - version = 10, + version = 11, exportSchema = true, ) @TypeConverters(Converters::class) @@ -286,6 +286,42 @@ abstract class AppDatabase : RoomDatabase() { } } + /** + * Moves midroll reference clips out of the database and into files. + * + * A real ad break runs for minutes, and the segment worth matching includes the theme + * music publishers wrap around it -- Peter named Tangle and The Daily Show. At 8kHz + * 16-bit PCM three minutes is 2.88MB, past what a CursorWindow hands back, so the + * first attempt capped the clip at sixty seconds. That was the wrong trade: it + * truncates the thing being identified. [org.terst.listen.fingerprint.MidrollClipStore] + * holds the audio now and the row holds a path. + * + * Existing candidates are dropped, not converted. A migration cannot write files, and + * every stored candidate was written under the sixty-second cap so it is the wrong + * length regardless. They are a cache; detection starts again and costs only time. + * `show_fingerprints` is deliberately untouched -- those clips are eight seconds and + * correct. + */ + val MIGRATION_10_11 = object : Migration(10, 11) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL("DROP TABLE IF EXISTS `show_midrolls`") + db.execSQL( + """CREATE TABLE IF NOT EXISTS `show_midrolls` ( + `subscriptionId` TEXT NOT NULL, `id` TEXT NOT NULL, + `referenceClipPath` TEXT NOT NULL, `sampleRate` INTEGER NOT NULL, + `confirmedCount` INTEGER NOT NULL, `lastConfirmedEpisodeId` TEXT, + `updatedAtMs` INTEGER NOT NULL, + PRIMARY KEY(`subscriptionId`, `id`), + FOREIGN KEY(`subscriptionId`) REFERENCES `subscriptions`(`id`) + ON DELETE CASCADE)""" + ) + db.execSQL( + "CREATE INDEX IF NOT EXISTS `index_show_midrolls_subscriptionId` " + + "ON `show_midrolls` (`subscriptionId`)" + ) + } + } + fun get(context: Context): AppDatabase = instance ?: synchronized(this) { instance ?: build(context.applicationContext).also { instance = it } } @@ -297,6 +333,7 @@ abstract class AppDatabase : RoomDatabase() { .addMigrations( MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4, MIGRATION_4_5, MIGRATION_5_6, MIGRATION_6_7, MIGRATION_7_8, MIGRATION_8_9, MIGRATION_9_10, + MIGRATION_10_11, ) .addCallback(SEED_DEFAULT_FILTERS) // No fallbackToDestructiveMigration. A listener's playback positions and play 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 4fb6342..ac80f03 100644 --- a/app/src/main/java/org/terst/listen/data/Daos.kt +++ b/app/src/main/java/org/terst/listen/data/Daos.kt @@ -67,6 +67,16 @@ interface SubscriptionDao { @Query("SELECT * FROM subscriptions ORDER BY title COLLATE NOCASE") suspend fun getAll(): List<SubscriptionEntity> + /** + * The shows Android Auto lists, which is everything except bedtime ones. + * + * Driving is the opposite of falling asleep, and the car list is scrolled blind at a + * traffic light -- the same reason played episodes never appear there. Voice search still + * goes through [getAll]: asking for a show by name is unambiguous about wanting it. + */ + @Query("SELECT * FROM subscriptions WHERE bedtime = 0 ORDER BY title COLLATE NOCASE") + suspend fun getBrowsable(): List<SubscriptionEntity> + @Query("SELECT * FROM subscriptions WHERE id = :id") suspend fun getById(id: String): SubscriptionEntity? diff --git a/app/src/main/java/org/terst/listen/data/Entities.kt b/app/src/main/java/org/terst/listen/data/Entities.kt index 83ee56a..6f868b1 100644 --- a/app/src/main/java/org/terst/listen/data/Entities.kt +++ b/app/src/main/java/org/terst/listen/data/Entities.kt @@ -371,38 +371,23 @@ data class ShowFingerprintEntity( data class ShowMidrollEntity( val subscriptionId: String, val id: String, - /** The matched segment itself, 16-bit PCM via [org.terst.listen.fingerprint.PcmCodec]. */ - val referenceClipPcm: ByteArray, + /** + * Where the matched segment's audio is, written by + * [org.terst.listen.fingerprint.MidrollClipStore]. + * + * A path, not the audio. An ad break runs minutes and the segment worth matching includes + * the theme music around it, which is far more than a database row can hand back -- see + * [AppDatabase.MIGRATION_10_11]. + */ + val referenceClipPath: String, val sampleRate: Int, val confirmedCount: Int, val lastConfirmedEpisodeId: String?, val updatedAtMs: Long, -) { - // See ShowFingerprintEntity's identical override for why: a ByteArray property breaks the - // generated equals/hashCode, comparing references rather than contents. - override fun equals(other: Any?): Boolean { - if (this === other) return true - if (other !is ShowMidrollEntity) return false - return subscriptionId == other.subscriptionId && - id == other.id && - referenceClipPcm.contentEquals(other.referenceClipPcm) && - sampleRate == other.sampleRate && - confirmedCount == other.confirmedCount && - lastConfirmedEpisodeId == other.lastConfirmedEpisodeId && - updatedAtMs == other.updatedAtMs - } - - override fun hashCode(): Int { - var result = subscriptionId.hashCode() - result = 31 * result + id.hashCode() - result = 31 * result + referenceClipPcm.contentHashCode() - result = 31 * result + sampleRate - result = 31 * result + confirmedCount - result = 31 * result + (lastConfirmedEpisodeId?.hashCode() ?: 0) - result = 31 * result + updatedAtMs.hashCode() - return result - } -} +) +// No hand-written equals/hashCode here, unlike ShowFingerprintEntity: every property is now a +// value type, so the generated ones compare what they should. The overrides only ever existed +// because of the ByteArray this row used to carry. /** * One confirmed midroll range detected in one episode -- where to jump from and to, in 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 fb77ab6..17b4ca3 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -9,6 +9,8 @@ import org.terst.listen.download.EpisodeDownloader import org.terst.listen.feed.ChapterFetcher import org.terst.listen.feed.FeedFetcher import org.terst.listen.feed.FeedResolver +import org.terst.listen.fingerprint.FileMidrollClipStore +import org.terst.listen.fingerprint.MidrollClipStore import org.terst.listen.playback.PlaybackAdvancer import org.terst.listen.playback.PlaybackSources import org.terst.listen.queue.DripReleaser @@ -57,6 +59,16 @@ class AppGraph(private val context: Context) { File(context.filesDir, "audio").also { it.mkdirs() } } + /** + * Midroll reference clips, kept beside the audio library rather than in the database -- + * see [org.terst.listen.fingerprint.MidrollClipStore]. + */ + val midrollClips: MidrollClipStore by lazy { + FileMidrollClipStore(File(context.filesDir, "midrolls").also { it.mkdirs() }) + } + + val unsubscriber: Unsubscriber by lazy { Unsubscriber(subscriptions, episodes, midrollClips) } + val resolver: FeedResolver by lazy { FeedResolver(client) } val preferences: Preferences by lazy { Preferences(context) } diff --git a/app/src/main/java/org/terst/listen/data/Unsubscriber.kt b/app/src/main/java/org/terst/listen/data/Unsubscriber.kt new file mode 100644 index 0000000..6a3b567 --- /dev/null +++ b/app/src/main/java/org/terst/listen/data/Unsubscriber.kt @@ -0,0 +1,36 @@ +package org.terst.listen.data + +import java.io.File +import org.terst.listen.fingerprint.MidrollClipStore + +/** + * Removes a show and everything on disk that belonged to it. + * + * Deleting the row alone is not enough, though it looks like it: the episode rows cascade away + * and take the only record of where their downloaded audio was with them, leaving the files + * orphaned forever. The unsubscribe dialog has always promised the downloads go too. + * + * Order matters and is the whole reason this is a class rather than two lines in a view model. + * The paths have to be read, and the files deleted, *before* the row goes -- afterwards there + * is nothing left to ask. + */ +class Unsubscriber( + private val subscriptions: SubscriptionDao, + private val episodes: EpisodeDao, + private val clipStore: MidrollClipStore, +) { + + suspend fun unsubscribe(subscriptionId: String) { + // getForSubscription, not the visible set: a drip show holds back most of its + // catalogue, and a held-back episode can still have been downloaded. + for (episode in episodes.getForSubscription(subscriptionId)) { + val path = episode.localPath ?: continue + runCatching { File(path).delete() } + } + clipStore.deleteAllFor(subscriptionId) + + // Last, and outside the file handling: a file that refuses to delete must not leave the + // show sitting in the library, which would be the visible failure of the two. + subscriptions.delete(subscriptionId) + } +} diff --git a/app/src/main/java/org/terst/listen/fingerprint/FingerprintUpdater.kt b/app/src/main/java/org/terst/listen/fingerprint/FingerprintUpdater.kt index 7018103..eff48ae 100644 --- a/app/src/main/java/org/terst/listen/fingerprint/FingerprintUpdater.kt +++ b/app/src/main/java/org/terst/listen/fingerprint/FingerprintUpdater.kt @@ -128,7 +128,7 @@ class FingerprintUpdater( ) = ShowFingerprintEntity( subscriptionId = subscriptionId, kind = kind, - referenceClipPcm = state.referenceClip?.let(PcmCodec::encode), + referenceClipPcm = state.referenceClip?.let(::storablePcm), pendingEpisodeId = state.pendingEpisodeId, sampleRate = sampleRate, confirmedCount = state.confirmedCount, @@ -136,8 +136,20 @@ class FingerprintUpdater( updatedAtMs = now(), ) + /** + * The reference clip as bytes, or null if it could not be read back out again. + * + * Unreachable while [DEFAULT_REFERENCE_SECONDS] stays at eight -- that is 128KB against a + * 1MB budget -- and here precisely because this table once held a clip that *did* exceed + * it. The row inserted cleanly and threw `SQLiteBlobTooBigException` on the next read, + * taking the app down. Dropping the candidate restarts detection for that show, which + * costs time; writing the row costs a crash. + */ + private fun storablePcm(clip: DoubleArray): ByteArray? = + PcmCodec.encode(clip).takeIf { StoredClip.fits(it.size) } + companion object { - /** Public so [org.terst.listen.fingerprint.StoredClip]'s guard can measure them. */ + /** Public so [StoredClip]'s guard can measure them. */ const val DEFAULT_REFERENCE_SECONDS = 8 const val DEFAULT_SAMPLE_RATE = 8_000 private const val MS_PER_SECOND = 1_000L diff --git a/app/src/main/java/org/terst/listen/fingerprint/MidrollClipStore.kt b/app/src/main/java/org/terst/listen/fingerprint/MidrollClipStore.kt new file mode 100644 index 0000000..ca4cc23 --- /dev/null +++ b/app/src/main/java/org/terst/listen/fingerprint/MidrollClipStore.kt @@ -0,0 +1,70 @@ +package org.terst.listen.fingerprint + +import java.io.File + +/** + * Where a midroll candidate's reference audio is kept. + * + * Not in the database. A real ad break runs for minutes -- Tangle and The Daily Show wrap + * theirs in theme music, so the segment worth matching includes the music at both ends -- and + * at 8kHz 16-bit PCM three minutes is 2.88MB, past what a CursorWindow will hand back. Capping + * the clip to fit a row was tried and is wrong: it truncates the very thing being identified. + * + * Behind an interface so [MidrollUpdater] stays testable without a device, though the file + * implementation has no Android in it either and is measured like any other class. + */ +interface MidrollClipStore { + /** The stored path, or null if writing failed. */ + suspend fun write(subscriptionId: String, candidateId: String, samples: DoubleArray): String? + + /** Null when the file has gone, which the caller treats as the candidate having gone. */ + suspend fun read(path: String): DoubleArray? + + suspend fun delete(path: String) + + /** Every clip belonging to one show, for when it is unsubscribed. */ + suspend fun deleteAllFor(subscriptionId: String) +} + +class FileMidrollClipStore(private val root: File) : MidrollClipStore { + + override suspend fun write( + subscriptionId: String, + candidateId: String, + samples: DoubleArray, + ): String? = runCatching { + val dir = File(root, safe(subscriptionId)).apply { mkdirs() } + val file = File(dir, "${safe(candidateId)}.pcm") + file.writeBytes(PcmCodec.encode(samples)) + file.absolutePath + }.getOrNull() + + override suspend fun read(path: String): DoubleArray? = runCatching { + val file = File(path) + if (!file.isFile) return null + PcmCodec.decode(file.readBytes()) + }.getOrNull() + + override suspend fun delete(path: String) { + runCatching { File(path).delete() } + } + + override suspend fun deleteAllFor(subscriptionId: String) { + runCatching { File(root, safe(subscriptionId)).deleteRecursively() } + } + + /** + * Ids reach here from feeds, and a separator in one would otherwise write outside the + * store -- the same sanitising [org.terst.listen.download.EpisodeDownloader] does for + * episode files, for the same reason. + */ + private fun safe(id: String) = id + .map { if (it.isLetterOrDigit() || it == '-' || it == '_') it else '_' } + .joinToString("") + .ifBlank { "unnamed" } + .take(MAX_NAME_LENGTH) + + private companion object { + const val MAX_NAME_LENGTH = 100 + } +} diff --git a/app/src/main/java/org/terst/listen/fingerprint/MidrollUpdater.kt b/app/src/main/java/org/terst/listen/fingerprint/MidrollUpdater.kt index 5849d1d..e5e757c 100644 --- a/app/src/main/java/org/terst/listen/fingerprint/MidrollUpdater.kt +++ b/app/src/main/java/org/terst/listen/fingerprint/MidrollUpdater.kt @@ -24,6 +24,7 @@ class MidrollUpdater( private val episodes: EpisodeDao, private val midrolls: MidrollDao, private val clips: AudioClipSource, + private val clipStore: MidrollClipStore, private val tuning: Tuning = Tuning(), ) { @@ -60,7 +61,7 @@ class MidrollUpdater( val stored = midrolls.getCandidates(subscriptionId) val result = MidrollDetector.check( - stored.map(::toDetectorCandidate), + load(subscriptionId, stored), episode.id, decoded.samples, confirmThreshold, @@ -111,12 +112,13 @@ class MidrollUpdater( } for (segment in undiscovered) { - val pcm = storablePcm(segment.referenceClip, newClip.sampleRate) ?: continue + val candidateId = newId() + val path = clipStore.write(subscriptionId, candidateId, segment.referenceClip) ?: continue midrolls.upsertCandidate( ShowMidrollEntity( subscriptionId = subscriptionId, - id = newId(), - referenceClipPcm = pcm, + id = candidateId, + referenceClipPath = path, sampleRate = newClip.sampleRate, confirmedCount = SEEDED_CONFIRM_COUNT, lastConfirmedEpisodeId = newEpisode.id, @@ -126,29 +128,6 @@ class MidrollUpdater( } } - /** - * A discovered segment as bytes a database row can carry back out, or null if even the - * trimmed clip would not fit. - * - * A shared segment has no length limit of its own -- a long sponsor block could run for - * minutes, and at 8kHz 16-bit PCM two minutes is already past the 2MB a CursorWindow will - * return. See [StoredClip]; the fingerprint table shipped with exactly that bug, and it - * crashed the app on read rather than failing on write. - * - * Trimming shortens the range that gets skipped, so a very long break plays its tail rather - * than being cut entirely. That is the safe direction to be wrong in: over-skipping eats - * real content, and not trimming at all makes a row that cannot be read. - * - * The [StoredClip.fits] check afterwards is a backstop, unreachable while the cap and the - * limit agree. It returns null rather than throwing because an untracked ad break is a - * missing feature and an oversized row is a crash. - */ - private fun storablePcm(clip: DoubleArray, sampleRate: Int): ByteArray? { - val maxSamples = StoredClip.MAX_CANDIDATE_SECONDS * sampleRate - val trimmed = if (clip.size <= maxSamples) clip else clip.copyOf(maxSamples) - return PcmCodec.encode(trimmed).takeIf { StoredClip.fits(it.size) } - } - private fun rangesOverlap(aStart: Long, aEnd: Long, bStart: Long, bEnd: Long) = aStart < bEnd && bStart < aEnd private data class DiscoveredSegment(val referenceClip: DoubleArray, val positionInB: Long, val lengthInB: Long) @@ -202,16 +181,25 @@ class MidrollUpdater( sampleRate: Int, ) { val surviving = result.candidates.associateBy { it.id } + val pathsById = stored.associate { it.id to it.referenceClipPath } + for (row in stored) { - if (row.id !in surviving) midrolls.deleteCandidate(subscriptionId, row.id) + if (row.id in surviving) continue + // The file goes with the row. Nothing else would ever reclaim it: the candidate id + // is the only thing that knew where it was. + midrolls.deleteCandidate(subscriptionId, row.id) + clipStore.delete(row.referenceClipPath) } + for (candidate in result.candidates) { - val pcm = storablePcm(candidate.referenceClip, sampleRate) ?: continue + // Rewriting the audio would be pointless -- a surviving candidate's reference clip + // is the one it was created with; only its confirmation count moves. + val path = pathsById[candidate.id] ?: continue midrolls.upsertCandidate( ShowMidrollEntity( subscriptionId = subscriptionId, id = candidate.id, - referenceClipPcm = pcm, + referenceClipPath = path, sampleRate = sampleRate, confirmedCount = candidate.confirmedCount, lastConfirmedEpisodeId = candidate.lastConfirmedEpisodeId, @@ -231,12 +219,30 @@ class MidrollUpdater( midrolls.upsertRange(EpisodeMidrollEntity(episode.id, detected.candidateId, startMs, endMs)) } - private fun toDetectorCandidate(entity: ShowMidrollEntity) = MidrollDetector.Candidate( - id = entity.id, - referenceClip = PcmCodec.decode(entity.referenceClipPcm), - confirmedCount = entity.confirmedCount, - lastConfirmedEpisodeId = entity.lastConfirmedEpisodeId, - ) + /** Null when the clip file has gone, which makes the candidate untrackable -- see [load]. */ + private suspend fun toDetectorCandidate(entity: ShowMidrollEntity): MidrollDetector.Candidate? { + val clip = clipStore.read(entity.referenceClipPath) ?: return null + return MidrollDetector.Candidate( + id = entity.id, + referenceClip = clip, + confirmedCount = entity.confirmedCount, + lastConfirmedEpisodeId = entity.lastConfirmedEpisodeId, + ) + } + + /** + * The candidates whose audio is still on disk, dropping the rows whose file has gone. + * + * A missing file is not recoverable -- there is nothing left to match against -- so the row + * goes too rather than being carried forward as a candidate that can never confirm. + */ + private suspend fun load(subscriptionId: String, stored: List<ShowMidrollEntity>) = + stored.mapNotNull { row -> + toDetectorCandidate(row) ?: run { + midrolls.deleteCandidate(subscriptionId, row.id) + null + } + } private companion object { const val DEFAULT_COARSE_FACTOR = 8 diff --git a/app/src/main/java/org/terst/listen/fingerprint/StoredClip.kt b/app/src/main/java/org/terst/listen/fingerprint/StoredClip.kt index 4d425df..0c39e71 100644 --- a/app/src/main/java/org/terst/listen/fingerprint/StoredClip.kt +++ b/app/src/main/java/org/terst/listen/fingerprint/StoredClip.kt @@ -8,8 +8,9 @@ package org.terst.listen.fingerprint * `SQLiteBlobTooBigException` the next time anything reads it, which is how the intro/outro * half of Tier 3 shipped unable to get past a show's second episode. * - * Every persisted clip goes through [fits], so the limit is one shared fact rather than - * something each caller has to remember. + * The intro/outro reference clip is the only decoded audio still kept in a row, and it goes + * through [fits] before it is written. Midroll candidates outgrew this entirely and live in + * files -- see [MidrollClipStore]. */ object StoredClip { @@ -19,14 +20,5 @@ object StoredClip { /** Half of it, so the columns alongside the blob can never tip an accepted clip over. */ const val SAFE_BYTES = CURSOR_WINDOW_LIMIT_BYTES / 2 - /** - * The longest midroll segment worth keeping, at [MediaCodecClipSource.TARGET_SAMPLE_RATE]. - * - * Sixty seconds is about 960KB, comfortably inside [SAFE_BYTES], and long enough to - * identify an ad break by correlation several times over -- matching is about whether the - * segment recurs, not about hearing all of it. - */ - const val MAX_CANDIDATE_SECONDS = 60 - fun fits(bytes: Int): Boolean = bytes <= SAFE_BYTES } diff --git a/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt b/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt index 33b928e..72fe5de 100644 --- a/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt +++ b/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt @@ -104,7 +104,7 @@ class ListenSessionCallback( } parentId == LibraryTree.SUBSCRIPTIONS -> - graph.subscriptions.getAll().map(LibraryTree::subscription) + graph.subscriptions.getBrowsable().map(LibraryTree::subscription) LibraryTree.subscriptionId(parentId) != null -> { val id = LibraryTree.subscriptionId(parentId)!! diff --git a/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt b/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt index 831ec8f..e278f78 100644 --- a/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt +++ b/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt @@ -176,7 +176,7 @@ class ListenViewModel(app: Application) : AndroidViewModel(app) { } fun unsubscribe(id: String) { - viewModelScope.launch { graph.subscriptions.delete(id) } + viewModelScope.launch { graph.unsubscriber.unsubscribe(id) } } fun enqueue(episodeId: String) { diff --git a/app/src/main/java/org/terst/listen/work/Workers.kt b/app/src/main/java/org/terst/listen/work/Workers.kt index 6ce26c6..87f6814 100644 --- a/app/src/main/java/org/terst/listen/work/Workers.kt +++ b/app/src/main/java/org/terst/listen/work/Workers.kt @@ -104,7 +104,13 @@ class MidrollWorker(context: Context, params: WorkerParameters) : CoroutineWorke override suspend fun doWork(): Result { val graph = Graph.get(applicationContext) - val updater = MidrollUpdater(graph.subscriptions, graph.episodes, graph.midrolls, MediaCodecClipSource()) + val updater = MidrollUpdater( + graph.subscriptions, + graph.episodes, + graph.midrolls, + MediaCodecClipSource(), + graph.midrollClips, + ) for (show in graph.subscriptions.getAll()) updater.processShow(show.id) return Result.success() } 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 863f265..a8d9a33 100644 --- a/app/src/test/java/org/terst/listen/data/FakeDaos.kt +++ b/app/src/test/java/org/terst/listen/data/FakeDaos.kt @@ -20,6 +20,7 @@ class FakeSubscriptionDao : SubscriptionDao { override fun observeEvery(): Flow<List<SubscriptionEntity>> = flowOf(rows.toList()) override suspend fun getAll() = rows.toList() + override suspend fun getBrowsable() = rows.filterNot { it.bedtime } override suspend fun getById(id: String) = rows.firstOrNull { it.id == id } override suspend fun getByFeedUrl(feedUrl: String) = rows.firstOrNull { it.feedUrl == feedUrl } diff --git a/app/src/test/java/org/terst/listen/data/StoredClipSizeTest.kt b/app/src/test/java/org/terst/listen/data/StoredClipSizeTest.kt index 7992098..93ca377 100644 --- a/app/src/test/java/org/terst/listen/data/StoredClipSizeTest.kt +++ b/app/src/test/java/org/terst/listen/data/StoredClipSizeTest.kt @@ -43,14 +43,6 @@ class StoredClipSizeTest { assertFalse("head window is $bytes bytes", StoredClip.fits(bytes)) } - /** A midroll candidate is a matched segment of unbounded length, so it has to be capped. */ - @Test - fun aMidrollCandidateIsCappedBelowTheLimit() { - val cap = encodedBytes(StoredClip.MAX_CANDIDATE_SECONDS, MediaCodecClipSource.TARGET_SAMPLE_RATE) - - assertTrue("cap is $cap bytes", StoredClip.fits(cap)) - } - /** Half the hard limit, so the rest of the row can never tip an accepted clip over it. */ @Test fun theSafeSizeLeavesHeadroomUnderTheHardLimit() { diff --git a/app/src/test/java/org/terst/listen/data/UnsubscribeTest.kt b/app/src/test/java/org/terst/listen/data/UnsubscribeTest.kt new file mode 100644 index 0000000..494b6a9 --- /dev/null +++ b/app/src/test/java/org/terst/listen/data/UnsubscribeTest.kt @@ -0,0 +1,129 @@ +package org.terst.listen.data + +import java.io.File +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.terst.listen.fingerprint.FileMidrollClipStore + +/** + * Unsubscribing takes the show's files with it. + * + * The confirmation dialog has always promised "episodes, downloads and playback positions", and + * the database half was true -- rows cascade. The files did not go anywhere: the episode rows + * that knew where they were cascaded away first, leaving downloaded audio orphaned on disk with + * nothing left pointing at it. Midroll clips would have joined them. + */ +class UnsubscribeTest { + + private lateinit var dir: File + private lateinit var subscriptions: FakeSubscriptionDao + private lateinit var episodes: FakeEpisodeDao + private lateinit var clipStore: FileMidrollClipStore + private lateinit var unsubscriber: Unsubscriber + + @Before + fun setUp() { + dir = File.createTempFile("unsub", "").let { it.delete(); it.mkdirs(); it } + subscriptions = FakeSubscriptionDao() + episodes = FakeEpisodeDao() + clipStore = FileMidrollClipStore(File(dir, "midrolls")) + unsubscriber = Unsubscriber(subscriptions, episodes, clipStore) + } + + @After + fun tearDown() = dir.deleteRecursively().let { } + + private fun show(id: String) { + subscriptions.rows += SubscriptionEntity( + id = id, feedUrl = "https://example.com/$id", title = id, addedAtMs = 0, + ) + } + + private fun downloadedEpisode(id: String, showId: String): File { + val file = File(dir, "$id.mp3").apply { writeBytes(byteArrayOf(1, 2, 3)) } + episodes.rows += EpisodeEntity( + id = id, + subscriptionId = showId, + guid = id, + title = id, + mediaUrl = "https://cdn/$id.mp3", + mediaType = "audio/mpeg", + discoveredAtMs = 0, + downloadState = DownloadState.DOWNLOADED, + localPath = file.absolutePath, + ) + return file + } + + @Test + fun `downloaded audio is deleted, not orphaned`() = runTest { + show("s1") + val audio = downloadedEpisode("e1", "s1") + + unsubscriber.unsubscribe("s1") + + assertFalse(audio.exists()) + assertTrue(subscriptions.rows.isEmpty()) + } + + @Test + fun `another show's downloads are left alone`() = runTest { + show("s1") + show("s2") + val mine = downloadedEpisode("e1", "s1") + val theirs = downloadedEpisode("e2", "s2") + + unsubscriber.unsubscribe("s1") + + assertFalse(mine.exists()) + assertTrue(theirs.exists()) + assertEquals(listOf("s2"), subscriptions.rows.map { it.id }) + } + + @Test + fun `midroll clips go too`() = runTest { + show("s1") + val clip = clipStore.write("s1", "c1", DoubleArray(10))!! + + unsubscriber.unsubscribe("s1") + + assertFalse(File(clip).exists()) + } + + /** An episode that was never downloaded has no file, and that is not an error. */ + @Test + fun `an undownloaded episode is not a problem`() = runTest { + show("s1") + episodes.rows += EpisodeEntity( + id = "e1", subscriptionId = "s1", guid = "e1", title = "t", + mediaUrl = "u", mediaType = "audio/mpeg", discoveredAtMs = 0, localPath = null, + ) + + unsubscriber.unsubscribe("s1") + + assertTrue(subscriptions.rows.isEmpty()) + } + + /** + * Files first, row second. The other order loses the paths to the cascade and leaks every + * file, which is exactly the bug this class exists to fix. + */ + @Test + fun `the show row is gone even when a file cannot be deleted`() = runTest { + show("s1") + episodes.rows += EpisodeEntity( + id = "e1", subscriptionId = "s1", guid = "e1", title = "t", + mediaUrl = "u", mediaType = "audio/mpeg", discoveredAtMs = 0, + localPath = "/definitely/not/a/real/path.mp3", + ) + + unsubscriber.unsubscribe("s1") + + assertTrue(subscriptions.rows.isEmpty()) + } +} diff --git a/app/src/test/java/org/terst/listen/fingerprint/MidrollClipStoreTest.kt b/app/src/test/java/org/terst/listen/fingerprint/MidrollClipStoreTest.kt new file mode 100644 index 0000000..b648491 --- /dev/null +++ b/app/src/test/java/org/terst/listen/fingerprint/MidrollClipStoreTest.kt @@ -0,0 +1,103 @@ +package org.terst.listen.fingerprint + +import java.io.File +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +/** + * Midroll reference clips live in files, not in the database. + * + * A database row cannot carry one: a real ad break runs minutes, and a CursorWindow refuses a + * row over 2MB on the way out. Capping the clip to fit was the first attempt and Peter shot it + * down for the right reason -- Tangle and The Daily Show wrap their breaks in theme music, so + * sixty seconds would clip the music off the front or back of the thing being matched. + * + * [FileMidrollClipStore] is measured because it is ordinary file IO with no Android in it, not + * a framework adapter. + */ +class MidrollClipStoreTest { + + private lateinit var dir: File + private lateinit var store: FileMidrollClipStore + + @Before + fun setUp() { + dir = File.createTempFile("midrolls", "").let { it.delete(); it.mkdirs(); it } + store = FileMidrollClipStore(dir) + } + + @After + fun tearDown() { + dir.deleteRecursively() + } + + private fun clip(n: Int) = DoubleArray(n) { (it % 200 - 100).toDouble() } + + @Test + fun `a clip survives a round trip`() = runTest { + val samples = clip(1000) + + val path = store.write("s1", "c1", samples)!! + + assertEquals(samples.toList(), store.read(path)!!.toList()) + } + + /** Far past what a row could hold -- the whole point of moving to files. */ + @Test + fun `a clip much larger than a database row round trips intact`() = runTest { + val threeMinutes = clip(180 * 8000) + + val path = store.write("s1", "big", threeMinutes)!! + + assertTrue("${File(path).length()} bytes", File(path).length() > StoredClip.CURSOR_WINDOW_LIMIT_BYTES) + assertEquals(threeMinutes.size, store.read(path)!!.size) + } + + @Test + fun `reading a clip that is gone returns null rather than throwing`() = runTest { + val path = store.write("s1", "c1", clip(10))!! + File(path).delete() + + assertNull(store.read(path)) + } + + @Test + fun `deleting a clip removes its file`() = runTest { + val path = store.write("s1", "c1", clip(10))!! + + store.delete(path) + + assertFalse(File(path).exists()) + } + + /** + * Unsubscribing has to take the clips with it. Nothing else ever would -- the database rows + * cascade away and would leave the files behind with no row left pointing at them. + */ + @Test + fun `dropping a show removes every clip it owns and no others`() = runTest { + val mine = store.write("s1", "c1", clip(10))!! + val alsoMine = store.write("s1", "c2", clip(10))!! + val someoneElses = store.write("s2", "c1", clip(10))!! + + store.deleteAllFor("s1") + + assertFalse(File(mine).exists()) + assertFalse(File(alsoMine).exists()) + assertTrue(File(someoneElses).exists()) + } + + /** Ids come from feeds and a candidate id is generated, but neither may escape the directory. */ + @Test + fun `a show id containing path separators cannot write outside the store`() = runTest { + val path = store.write("../../etc", "c1", clip(10))!! + + assertTrue(path, File(path).canonicalPath.startsWith(dir.canonicalPath)) + } +} diff --git a/app/src/test/java/org/terst/listen/fingerprint/MidrollUpdaterTest.kt b/app/src/test/java/org/terst/listen/fingerprint/MidrollUpdaterTest.kt index 16e4c0d..e8e4474 100644 --- a/app/src/test/java/org/terst/listen/fingerprint/MidrollUpdaterTest.kt +++ b/app/src/test/java/org/terst/listen/fingerprint/MidrollUpdaterTest.kt @@ -24,6 +24,7 @@ class MidrollUpdaterTest { private lateinit var subscriptions: FakeSubscriptionDao private lateinit var midrolls: FakeMidrollDao private lateinit var clips: FakeFullClipSource + private lateinit var clipStore: FakeMidrollClipStore private lateinit var updater: MidrollUpdater private val fixedNow = 555_000L @@ -34,11 +35,13 @@ class MidrollUpdaterTest { subscriptions = FakeSubscriptionDao() midrolls = FakeMidrollDao().apply { episodeRows = episodes.rows } clips = FakeFullClipSource() + clipStore = FakeMidrollClipStore() updater = MidrollUpdater( subscriptions, episodes, midrolls, clips, + clipStore, // No decimation and a MidrollScannerTest-scale window: the coarse-to-fine split is // MidrollUpdater's own concern to compose correctly, not something this test needs // to exercise at real (8kHz-vs-1kHz) scale on top of everything else. @@ -81,7 +84,7 @@ class MidrollUpdaterTest { @Test fun `the production tuning defaults construct a working updater`() = runTest { show() - val defaultUpdater = MidrollUpdater(subscriptions, episodes, midrolls, clips) + val defaultUpdater = MidrollUpdater(subscriptions, episodes, midrolls, clips, clipStore) defaultUpdater.processShow("show1") assertTrue(midrolls.candidateRows.isEmpty()) } @@ -180,3 +183,27 @@ class MidrollUpdaterTest { full[episode.id]?.let { DecodedClip(it, sampleRate, startMs = 0) } } } + +/** + * Midroll clips in memory rather than on disk. Keyed by the path it hands back, so a test can + * delete one behind the updater's back the way retention or a wipe would. + */ +class FakeMidrollClipStore : MidrollClipStore { + val clips = mutableMapOf<String, DoubleArray>() + + override suspend fun write(subscriptionId: String, candidateId: String, samples: DoubleArray): String { + val path = "$subscriptionId/$candidateId.pcm" + clips[path] = samples + return path + } + + override suspend fun read(path: String): DoubleArray? = clips[path] + + override suspend fun delete(path: String) { + clips -= path + } + + override suspend fun deleteAllFor(subscriptionId: String) { + clips.keys.filter { it.startsWith("$subscriptionId/") }.forEach { clips -= it } + } +} 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 bf351ae..1fa3b24 100644 --- a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt +++ b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt @@ -62,7 +62,7 @@ class SkipSummaryTest { private fun candidate(id: String, confirmedCount: Int) = ShowMidrollEntity( subscriptionId = "s1", id = id, - referenceClipPcm = byteArrayOf(1, 2), + referenceClipPath = "/clips/s1/$id.pcm", sampleRate = 8000, confirmedCount = confirmedCount, lastConfirmedEpisodeId = null, |