From 2d50a67edc5e8c5ace610aa672dc0a92dde347c5 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Mon, 28 Sep 2026 08:16:16 +0000 Subject: Keep bedtime shows out of the car, and midroll clips out of the database Peter, on the sixty-second cap from the last commit: "won't be enough for tangle or the daily show at least. Both have theme music around ads at the end at least." He is right, and the cap was the wrong trade -- trimming a clip to fit a database row truncates the very thing being identified, and the music wrapping a break is part of what makes it recognisable. So the clips leave the database. MidrollClipStore writes them as files beside the audio library and the row holds a path, which removes the length limit rather than negotiating with it: three minutes is 2.88MB, and a CursorWindow hands back nothing over 2MB. Migration 10->11 drops existing candidates instead of converting them -- a migration cannot write files, and every stored candidate was written under the sixty-second cap, so it is the wrong length anyway. They are a cache; detection starts again. The intro/outro fingerprints are untouched, being eight seconds and correct. StoredClip.fits now guards the one decoded clip still kept in a row, the intro/outro reference, at the point it is written. Unreachable at the current eight seconds, and there precisely because this table already shipped a clip that was not. Bedtime shows are gone from Android Auto's browse list too, as asked. Driving is the opposite of falling asleep and a car list is scrolled blind at a traffic light -- the same argument that already hides played episodes there. Voice search still reaches them: asking for a show by name is unambiguous about wanting it. Found while doing it: unsubscribing never deleted anything from disk. The dialog has always promised "episodes, downloads and playback positions", and the rows did cascade -- taking with them the only record of where the downloaded audio was, so the files stayed forever with nothing pointing at them. Midroll clips would have joined them. Unsubscriber deletes the files first and the row last, because the other order is exactly how the leak happened, and it drops the row even if a file will not delete: a show stuck in the library is the worse failure of the two. Verified: 533 unit tests, detekt and lint clean, 11 migration tests on the emulator. Coverage 91.84% -> 92.73%. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B --- .coverage-baseline | 2 +- .../org.terst.listen.data.AppDatabase/11.json | 747 +++++++++++++++++++++ .../java/org/terst/listen/data/MigrationTest.kt | 63 ++ .../main/java/org/terst/listen/data/AppDatabase.kt | 39 +- app/src/main/java/org/terst/listen/data/Daos.kt | 10 + .../main/java/org/terst/listen/data/Entities.kt | 41 +- .../java/org/terst/listen/data/Repositories.kt | 12 + .../java/org/terst/listen/data/Unsubscriber.kt | 36 + .../terst/listen/fingerprint/FingerprintUpdater.kt | 16 +- .../terst/listen/fingerprint/MidrollClipStore.kt | 70 ++ .../org/terst/listen/fingerprint/MidrollUpdater.kt | 78 ++- .../org/terst/listen/fingerprint/StoredClip.kt | 14 +- .../terst/listen/playback/ListenSessionCallback.kt | 2 +- .../java/org/terst/listen/ui/ListenViewModel.kt | 2 +- app/src/main/java/org/terst/listen/work/Workers.kt | 8 +- .../test/java/org/terst/listen/data/FakeDaos.kt | 1 + .../org/terst/listen/data/StoredClipSizeTest.kt | 8 - .../java/org/terst/listen/data/UnsubscribeTest.kt | 129 ++++ .../listen/fingerprint/MidrollClipStoreTest.kt | 103 +++ .../terst/listen/fingerprint/MidrollUpdaterTest.kt | 29 +- .../terst/listen/fingerprint/SkipSummaryTest.kt | 2 +- 21 files changed, 1320 insertions(+), 92 deletions(-) create mode 100644 app/schemas/org.terst.listen.data.AppDatabase/11.json create mode 100644 app/src/main/java/org/terst/listen/data/Unsubscriber.kt create mode 100644 app/src/main/java/org/terst/listen/fingerprint/MidrollClipStore.kt create mode 100644 app/src/test/java/org/terst/listen/data/UnsubscribeTest.kt create mode 100644 app/src/test/java/org/terst/listen/fingerprint/MidrollClipStoreTest.kt diff --git a/.coverage-baseline b/.coverage-baseline index e797ff0..4b5c51d 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -91.84 +92.73 diff --git a/app/schemas/org.terst.listen.data.AppDatabase/11.json b/app/schemas/org.terst.listen.data.AppDatabase/11.json new file mode 100644 index 0000000..066e134 --- /dev/null +++ b/app/schemas/org.terst.listen.data.AppDatabase/11.json @@ -0,0 +1,747 @@ +{ + "formatVersion": 1, + "database": { + "version": 11, + "identityHash": "ca00e50fc7e97ed174ad49a2c8348c45", + "entities": [ + { + "tableName": "subscriptions", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `feedUrl` TEXT NOT NULL, `kind` TEXT NOT NULL, `title` TEXT NOT NULL, `description` TEXT, `author` TEXT, `imageUrl` TEXT, `link` TEXT, `addedAtMs` INTEGER NOT NULL, `etag` TEXT, `lastModified` TEXT, `lastFetchedAtMs` INTEGER, `lastError` TEXT, `playbackSpeed` REAL, `skipIntroMs` INTEGER NOT NULL, `skipOutroMs` INTEGER NOT NULL, `dripEnabled` INTEGER NOT NULL DEFAULT 0, `dripIntervalMs` INTEGER NOT NULL DEFAULT 86400000, `dripCount` INTEGER NOT NULL DEFAULT 1, `dripOldestFirst` INTEGER NOT NULL DEFAULT 1, `dripLastReleaseAtMs` INTEGER, `skipAdChapters` INTEGER NOT NULL DEFAULT 0, `midrollPendingEpisodeId` TEXT, `bedtime` INTEGER NOT NULL DEFAULT 0, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "feedUrl", + "columnName": "feedUrl", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "kind", + "columnName": "kind", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "title", + "columnName": "title", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "description", + "columnName": "description", + "affinity": "TEXT" + }, + { + "fieldPath": "author", + "columnName": "author", + "affinity": "TEXT" + }, + { + "fieldPath": "imageUrl", + "columnName": "imageUrl", + "affinity": "TEXT" + }, + { + "fieldPath": "link", + "columnName": "link", + "affinity": "TEXT" + }, + { + "fieldPath": "addedAtMs", + "columnName": "addedAtMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "etag", + "columnName": "etag", + "affinity": "TEXT" + }, + { + "fieldPath": "lastModified", + "columnName": "lastModified", + "affinity": "TEXT" + }, + { + "fieldPath": "lastFetchedAtMs", + "columnName": "lastFetchedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "lastError", + "columnName": "lastError", + "affinity": "TEXT" + }, + { + "fieldPath": "playbackSpeed", + "columnName": "playbackSpeed", + "affinity": "REAL" + }, + { + "fieldPath": "skipIntroMs", + "columnName": "skipIntroMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "skipOutroMs", + "columnName": "skipOutroMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "dripEnabled", + "columnName": "dripEnabled", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + }, + { + "fieldPath": "dripIntervalMs", + "columnName": "dripIntervalMs", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "86400000" + }, + { + "fieldPath": "dripCount", + "columnName": "dripCount", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "1" + }, + { + "fieldPath": "dripOldestFirst", + "columnName": "dripOldestFirst", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "1" + }, + { + "fieldPath": "dripLastReleaseAtMs", + "columnName": "dripLastReleaseAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "skipAdChapters", + "columnName": "skipAdChapters", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + }, + { + "fieldPath": "midrollPendingEpisodeId", + "columnName": "midrollPendingEpisodeId", + "affinity": "TEXT" + }, + { + "fieldPath": "bedtime", + "columnName": "bedtime", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_subscriptions_feedUrl", + "unique": true, + "columnNames": [ + "feedUrl" + ], + "orders": [], + "createSql": "CREATE UNIQUE INDEX IF NOT EXISTS `index_subscriptions_feedUrl` ON `${TABLE_NAME}` (`feedUrl`)" + } + ] + }, + { + "tableName": "episodes", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `subscriptionId` TEXT NOT NULL, `guid` TEXT NOT NULL, `title` TEXT NOT NULL, `description` TEXT, `mediaUrl` TEXT NOT NULL, `mediaType` TEXT NOT NULL, `mediaLengthBytes` INTEGER, `durationMs` INTEGER, `publishedAtMs` INTEGER, `seasonNumber` INTEGER, `episodeNumber` INTEGER, `imageUrl` TEXT, `chaptersUrl` TEXT, `discoveredAtMs` INTEGER NOT NULL, `positionMs` INTEGER NOT NULL, `playedAtMs` INTEGER, `downloadState` TEXT NOT NULL, `localPath` TEXT, `downloadedAtMs` INTEGER, `dripReleasedAtMs` INTEGER, `appliedRuleId` TEXT, `appliedRuleReason` TEXT, `speedOverride` REAL, `midrollCheckedAtMs` INTEGER, `chaptersJson` TEXT, `detectedIntroMs` INTEGER, `detectedOutroMs` INTEGER, PRIMARY KEY(`id`), FOREIGN KEY(`subscriptionId`) REFERENCES `subscriptions`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "subscriptionId", + "columnName": "subscriptionId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "guid", + "columnName": "guid", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "title", + "columnName": "title", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "description", + "columnName": "description", + "affinity": "TEXT" + }, + { + "fieldPath": "mediaUrl", + "columnName": "mediaUrl", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "mediaType", + "columnName": "mediaType", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "mediaLengthBytes", + "columnName": "mediaLengthBytes", + "affinity": "INTEGER" + }, + { + "fieldPath": "durationMs", + "columnName": "durationMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "publishedAtMs", + "columnName": "publishedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "seasonNumber", + "columnName": "seasonNumber", + "affinity": "INTEGER" + }, + { + "fieldPath": "episodeNumber", + "columnName": "episodeNumber", + "affinity": "INTEGER" + }, + { + "fieldPath": "imageUrl", + "columnName": "imageUrl", + "affinity": "TEXT" + }, + { + "fieldPath": "chaptersUrl", + "columnName": "chaptersUrl", + "affinity": "TEXT" + }, + { + "fieldPath": "discoveredAtMs", + "columnName": "discoveredAtMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "positionMs", + "columnName": "positionMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "playedAtMs", + "columnName": "playedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "downloadState", + "columnName": "downloadState", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "localPath", + "columnName": "localPath", + "affinity": "TEXT" + }, + { + "fieldPath": "downloadedAtMs", + "columnName": "downloadedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "dripReleasedAtMs", + "columnName": "dripReleasedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "appliedRuleId", + "columnName": "appliedRuleId", + "affinity": "TEXT" + }, + { + "fieldPath": "appliedRuleReason", + "columnName": "appliedRuleReason", + "affinity": "TEXT" + }, + { + "fieldPath": "speedOverride", + "columnName": "speedOverride", + "affinity": "REAL" + }, + { + "fieldPath": "midrollCheckedAtMs", + "columnName": "midrollCheckedAtMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "chaptersJson", + "columnName": "chaptersJson", + "affinity": "TEXT" + }, + { + "fieldPath": "detectedIntroMs", + "columnName": "detectedIntroMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "detectedOutroMs", + "columnName": "detectedOutroMs", + "affinity": "INTEGER" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_episodes_subscriptionId", + "unique": false, + "columnNames": [ + "subscriptionId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_episodes_subscriptionId` ON `${TABLE_NAME}` (`subscriptionId`)" + }, + { + "name": "index_episodes_subscriptionId_guid", + "unique": true, + "columnNames": [ + "subscriptionId", + "guid" + ], + "orders": [], + "createSql": "CREATE UNIQUE INDEX IF NOT EXISTS `index_episodes_subscriptionId_guid` ON `${TABLE_NAME}` (`subscriptionId`, `guid`)" + }, + { + "name": "index_episodes_publishedAtMs", + "unique": false, + "columnNames": [ + "publishedAtMs" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_episodes_publishedAtMs` ON `${TABLE_NAME}` (`publishedAtMs`)" + } + ], + "foreignKeys": [ + { + "table": "subscriptions", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "subscriptionId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "queue", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`episodeId` TEXT NOT NULL, `position` INTEGER NOT NULL, PRIMARY KEY(`episodeId`), FOREIGN KEY(`episodeId`) REFERENCES `episodes`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "episodeId", + "columnName": "episodeId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "position", + "columnName": "position", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "episodeId" + ] + }, + "indices": [ + { + "name": "index_queue_position", + "unique": false, + "columnNames": [ + "position" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_queue_position` ON `${TABLE_NAME}` (`position`)" + } + ], + "foreignKeys": [ + { + "table": "episodes", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "episodeId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "rules", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `name` TEXT NOT NULL, `position` INTEGER NOT NULL, `enabled` INTEGER NOT NULL, `subscriptionId` TEXT, `titleContains` TEXT, `titleExcludes` TEXT, `minDurationMs` INTEGER, `maxDurationMs` INTEGER, `maxAgeMs` INTEGER, `seasonNumber` INTEGER, `action` TEXT NOT NULL, `autoDownload` INTEGER NOT NULL, `speed` REAL, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "name", + "columnName": "name", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "position", + "columnName": "position", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "enabled", + "columnName": "enabled", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "subscriptionId", + "columnName": "subscriptionId", + "affinity": "TEXT" + }, + { + "fieldPath": "titleContains", + "columnName": "titleContains", + "affinity": "TEXT" + }, + { + "fieldPath": "titleExcludes", + "columnName": "titleExcludes", + "affinity": "TEXT" + }, + { + "fieldPath": "minDurationMs", + "columnName": "minDurationMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "maxDurationMs", + "columnName": "maxDurationMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "maxAgeMs", + "columnName": "maxAgeMs", + "affinity": "INTEGER" + }, + { + "fieldPath": "seasonNumber", + "columnName": "seasonNumber", + "affinity": "INTEGER" + }, + { + "fieldPath": "action", + "columnName": "action", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "autoDownload", + "columnName": "autoDownload", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "speed", + "columnName": "speed", + "affinity": "REAL" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_rules_position", + "unique": false, + "columnNames": [ + "position" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_rules_position` ON `${TABLE_NAME}` (`position`)" + } + ] + }, + { + "tableName": "show_fingerprints", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`subscriptionId` TEXT NOT NULL, `kind` TEXT NOT NULL, `referenceClipPcm` BLOB, `pendingEpisodeId` TEXT, `sampleRate` INTEGER NOT NULL, `confirmedCount` INTEGER NOT NULL, `lastConfirmedEpisodeId` TEXT, `updatedAtMs` INTEGER NOT NULL, PRIMARY KEY(`subscriptionId`, `kind`), FOREIGN KEY(`subscriptionId`) REFERENCES `subscriptions`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "subscriptionId", + "columnName": "subscriptionId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "kind", + "columnName": "kind", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "referenceClipPcm", + "columnName": "referenceClipPcm", + "affinity": "BLOB" + }, + { + "fieldPath": "pendingEpisodeId", + "columnName": "pendingEpisodeId", + "affinity": "TEXT" + }, + { + "fieldPath": "sampleRate", + "columnName": "sampleRate", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "confirmedCount", + "columnName": "confirmedCount", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "lastConfirmedEpisodeId", + "columnName": "lastConfirmedEpisodeId", + "affinity": "TEXT" + }, + { + "fieldPath": "updatedAtMs", + "columnName": "updatedAtMs", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "subscriptionId", + "kind" + ] + }, + "indices": [ + { + "name": "index_show_fingerprints_subscriptionId", + "unique": false, + "columnNames": [ + "subscriptionId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_show_fingerprints_subscriptionId` ON `${TABLE_NAME}` (`subscriptionId`)" + } + ], + "foreignKeys": [ + { + "table": "subscriptions", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "subscriptionId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "show_midrolls", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`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 UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "subscriptionId", + "columnName": "subscriptionId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "referenceClipPath", + "columnName": "referenceClipPath", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sampleRate", + "columnName": "sampleRate", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "confirmedCount", + "columnName": "confirmedCount", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "lastConfirmedEpisodeId", + "columnName": "lastConfirmedEpisodeId", + "affinity": "TEXT" + }, + { + "fieldPath": "updatedAtMs", + "columnName": "updatedAtMs", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "subscriptionId", + "id" + ] + }, + "indices": [ + { + "name": "index_show_midrolls_subscriptionId", + "unique": false, + "columnNames": [ + "subscriptionId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_show_midrolls_subscriptionId` ON `${TABLE_NAME}` (`subscriptionId`)" + } + ], + "foreignKeys": [ + { + "table": "subscriptions", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "subscriptionId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "episode_midrolls", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`episodeId` TEXT NOT NULL, `candidateId` TEXT NOT NULL, `startMs` INTEGER NOT NULL, `endMs` INTEGER NOT NULL, PRIMARY KEY(`episodeId`, `candidateId`), FOREIGN KEY(`episodeId`) REFERENCES `episodes`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "episodeId", + "columnName": "episodeId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "candidateId", + "columnName": "candidateId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "startMs", + "columnName": "startMs", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "endMs", + "columnName": "endMs", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "episodeId", + "candidateId" + ] + }, + "indices": [ + { + "name": "index_episode_midrolls_episodeId", + "unique": false, + "columnNames": [ + "episodeId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_episode_midrolls_episodeId` ON `${TABLE_NAME}` (`episodeId`)" + } + ], + "foreignKeys": [ + { + "table": "episodes", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "episodeId" + ], + "referencedColumns": [ + "id" + ] + } + ] + } + ], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, 'ca00e50fc7e97ed174ad49a2c8348c45')" + ] + } +} \ No newline at end of file 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 + /** + * 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 + @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) = + 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> = 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() + + 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, -- cgit v1.2.3