From f9ccb831d882f4d5086ee2b1b7e37c4098132c77 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Thu, 1 Jun 2023 15:05:24 -0600 Subject: [PATCH] music: fix comparison issues Fix a few problems with the current comparison algorithm: 1. It wasn't actually comparing the raw music information, only the UIDs, which was redundant. 2. The comparison in the main music loading process occurred on the main thread, which causes massive freeze-up issues. Resolves #457. --- CHANGELOG.md | 2 + .../auxio/image/extractor/CoverExtractor.kt | 1 + .../oxycblt/auxio/music/MusicRepository.kt | 50 ++++--- .../auxio/music/device/DeviceLibrary.kt | 18 +-- .../auxio/music/device/DeviceMusicImpl.kt | 6 +- .../oxycblt/auxio/music/device/RawMusic.kt | 130 ++++++++++-------- .../oxycblt/auxio/music/user/UserLibrary.kt | 2 + .../auxio/playback/system/PlaybackService.kt | 4 +- 8 files changed, 118 insertions(+), 95 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06e0002f3..adec5f29e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ within it - Fixed blurry playing indicator in album/artist/genre/playlist items - Fixed incorrect songs being displayed when adding albums to the end of the queue +- Fixed freezing occuring when scrolling through large music libraries +- Fixed app not responding once music loading completes for large libraries #### What's Changed - Android Lollipop and Marshmallow support have been dropped diff --git a/app/src/main/java/org/oxycblt/auxio/image/extractor/CoverExtractor.kt b/app/src/main/java/org/oxycblt/auxio/image/extractor/CoverExtractor.kt index 0f0b3fbb7..067b8361c 100644 --- a/app/src/main/java/org/oxycblt/auxio/image/extractor/CoverExtractor.kt +++ b/app/src/main/java/org/oxycblt/auxio/image/extractor/CoverExtractor.kt @@ -118,6 +118,7 @@ constructor( * the given [Album] in the given [Song] list. */ fun computeCoverOrdering(songs: List): List { + // TODO: Start short-circuiting in more places if (songs.isEmpty()) return listOf() if (songs.size == 1) return listOf(songs.first().album) diff --git a/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt b/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt index c88eaaf14..8d890b218 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt @@ -32,7 +32,6 @@ import kotlinx.coroutines.Job import kotlinx.coroutines.async import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext import kotlinx.coroutines.yield import org.oxycblt.auxio.music.cache.CacheRepository import org.oxycblt.auxio.music.device.DeviceLibrary @@ -301,44 +300,35 @@ constructor( val userLibrary = synchronized(this) { userLibrary ?: return } logD("Creating playlist $name with ${songs.size} songs") userLibrary.createPlaylist(name, songs) - notifyUserLibraryChange() + emitLibraryChange(device = false, user = true) } override suspend fun renamePlaylist(playlist: Playlist, name: String) { val userLibrary = synchronized(this) { userLibrary ?: return } logD("Renaming $playlist to $name") userLibrary.renamePlaylist(playlist, name) - notifyUserLibraryChange() + emitLibraryChange(device = false, user = true) } override suspend fun deletePlaylist(playlist: Playlist) { val userLibrary = synchronized(this) { userLibrary ?: return } logD("Deleting $playlist") userLibrary.deletePlaylist(playlist) - notifyUserLibraryChange() + emitLibraryChange(device = false, user = true) } override suspend fun addToPlaylist(songs: List, playlist: Playlist) { val userLibrary = synchronized(this) { userLibrary ?: return } logD("Adding ${songs.size} songs to $playlist") userLibrary.addToPlaylist(playlist, songs) - notifyUserLibraryChange() + emitLibraryChange(device = false, user = true) } override suspend fun rewritePlaylist(playlist: Playlist, songs: List) { val userLibrary = synchronized(this) { userLibrary ?: return } logD("Rewriting $playlist with ${songs.size} songs") userLibrary.rewritePlaylist(playlist, songs) - notifyUserLibraryChange() - } - - @Synchronized - private fun notifyUserLibraryChange() { - logD("Dispatching user library change") - for (listener in updateListeners) { - listener.onMusicChanges( - MusicRepository.Changes(deviceLibrary = false, userLibrary = true)) - } + emitLibraryChange(device = false, user = true) } @Synchronized @@ -435,7 +425,7 @@ constructor( val deviceLibraryChannel = Channel() logD("Starting DeviceLibrary creation") val deviceLibraryJob = - worker.scope.tryAsync(Dispatchers.Main) { + worker.scope.tryAsync(Dispatchers.Default) { deviceLibraryFactory.create(rawSongs).also { deviceLibraryChannel.send(it) } } logD("Starting UserLibrary creation") @@ -452,10 +442,22 @@ constructor( val userLibrary = userLibraryJob.await().getOrThrow() logD("Successfully indexed music library [device=$deviceLibrary user=$userLibrary]") - withContext(Dispatchers.Main) { - emitComplete(null) - emitData(deviceLibrary, userLibrary) + emitComplete(null) + + // Comparing the library instances is obscenely expensive, do it within the library + val deviceLibraryChanged = this.deviceLibrary != deviceLibrary + val userLibraryChanged = this.userLibrary != userLibrary + if (!deviceLibraryChanged && !userLibraryChanged) { + logD("Library has not changed, skipping update") + return } + + synchronized(this) { + this.deviceLibrary = deviceLibrary + this.userLibrary = userLibrary + } + + emitLibraryChange(deviceLibraryChanged, userLibraryChanged) } /** @@ -497,14 +499,8 @@ constructor( } @Synchronized - private fun emitData(deviceLibrary: DeviceLibrary, userLibrary: MutableUserLibrary) { - val deviceLibraryChanged = this.deviceLibrary != deviceLibrary - val userLibraryChanged = this.userLibrary != userLibrary - if (!deviceLibraryChanged && !userLibraryChanged) return - - this.deviceLibrary = deviceLibrary - this.userLibrary = userLibrary - val changes = MusicRepository.Changes(deviceLibraryChanged, userLibraryChanged) + private fun emitLibraryChange(device: Boolean, user: Boolean) { + val changes = MusicRepository.Changes(device, user) logD("Dispatching library change [changes=$changes]") for (listener in updateListeners) { listener.onMusicChanges(changes) diff --git a/app/src/main/java/org/oxycblt/auxio/music/device/DeviceLibrary.kt b/app/src/main/java/org/oxycblt/auxio/music/device/DeviceLibrary.kt index 66d1c5d77..9add74b28 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/device/DeviceLibrary.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/device/DeviceLibrary.kt @@ -193,8 +193,8 @@ private class DeviceLibraryImpl(rawSongs: List, settings: MusicSettings private fun buildAlbums(songs: List, settings: MusicSettings): List { // Group songs by their singular raw album, then map the raw instances and their // grouped songs to Album values. Album.Raw will handle the actual grouping rules. - val songsByAlbum = songs.groupBy { it.rawAlbum } - val albums = songsByAlbum.map { AlbumImpl(it.key, settings, it.value) } + val songsByAlbum = songs.groupBy { it.rawAlbum.key } + val albums = songsByAlbum.map { AlbumImpl(it.key.value, settings, it.value) } logD("Successfully built ${albums.size} albums") return albums } @@ -221,22 +221,22 @@ private class DeviceLibraryImpl(rawSongs: List, settings: MusicSettings ): List { // Add every raw artist credited to each Song/Album to the grouping. This way, // different multi-artist combinations are not treated as different artists. - val musicByArtist = mutableMapOf>() + val musicByArtist = mutableMapOf>() for (song in songs) { for (rawArtist in song.rawArtists) { - musicByArtist.getOrPut(rawArtist) { mutableListOf() }.add(song) + musicByArtist.getOrPut(rawArtist.key) { mutableListOf() }.add(song) } } for (album in albums) { for (rawArtist in album.rawArtists) { - musicByArtist.getOrPut(rawArtist) { mutableListOf() }.add(album) + musicByArtist.getOrPut(rawArtist.key) { mutableListOf() }.add(album) } } // Convert the combined mapping into artist instances. - val artists = musicByArtist.map { ArtistImpl(it.key, settings, it.value) } + val artists = musicByArtist.map { ArtistImpl(it.key.value, settings, it.value) } logD("Successfully built ${artists.size} artists") return artists } @@ -253,15 +253,15 @@ private class DeviceLibraryImpl(rawSongs: List, settings: MusicSettings private fun buildGenres(songs: List, settings: MusicSettings): List { // Add every raw genre credited to each Song to the grouping. This way, // different multi-genre combinations are not treated as different genres. - val songsByGenre = mutableMapOf>() + val songsByGenre = mutableMapOf>() for (song in songs) { for (rawGenre in song.rawGenres) { - songsByGenre.getOrPut(rawGenre) { mutableListOf() }.add(song) + songsByGenre.getOrPut(rawGenre.key) { mutableListOf() }.add(song) } } // Convert the mapping into genre instances. - val genres = songsByGenre.map { GenreImpl(it.key, settings, it.value) } + val genres = songsByGenre.map { GenreImpl(it.key.value, settings, it.value) } logD("Successfully built ${genres.size} genres") return genres } diff --git a/app/src/main/java/org/oxycblt/auxio/music/device/DeviceMusicImpl.kt b/app/src/main/java/org/oxycblt/auxio/music/device/DeviceMusicImpl.kt index c07c5a65f..ed73bf885 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/device/DeviceMusicImpl.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/device/DeviceMusicImpl.kt @@ -407,7 +407,8 @@ class ArtistImpl( * [RawArtist] will be within the list. * @return The index of the [Artist]'s [RawArtist] within the list. */ - fun getOriginalPositionIn(rawArtists: List) = rawArtists.indexOf(rawArtist) + fun getOriginalPositionIn(rawArtists: List) = + rawArtists.indexOfFirst { it.key == rawArtist.key } /** * Perform final validation and organization on this instance. @@ -481,7 +482,8 @@ class GenreImpl( * [RawGenre] will be within the list. * @return The index of the [Genre]'s [RawGenre] within the list. */ - fun getOriginalPositionIn(rawGenres: List) = rawGenres.indexOf(rawGenre) + fun getOriginalPositionIn(rawGenres: List) = + rawGenres.indexOfFirst { it.key == rawGenre.key } /** * Perform final validation and organization on this instance. diff --git a/app/src/main/java/org/oxycblt/auxio/music/device/RawMusic.kt b/app/src/main/java/org/oxycblt/auxio/music/device/RawMusic.kt index 10d248991..2a198c687 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/device/RawMusic.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/device/RawMusic.kt @@ -113,28 +113,35 @@ data class RawAlbum( /** @see RawArtist.name */ val rawArtists: List ) { - // Albums are grouped as follows: - // - If we have a MusicBrainz ID, only group by it. This allows different Albums with the - // same name to be differentiated, which is common in large libraries. - // - If we do not have a MusicBrainz ID, compare by the lowercase album name and lowercase - // artist name. This allows for case-insensitive artist/album grouping, which can be common - // for albums/artists that have different naming (ex. "RAMMSTEIN" vs. "Rammstein"). + val key = Key(this) - // Cache the hash-code for HashMap efficiency. - private val hashCode = - musicBrainzId?.hashCode() ?: (31 * name.lowercase().hashCode() + rawArtists.hashCode()) + /** Exposed information that denotes [RawAlbum] uniqueness. */ + data class Key(val value: RawAlbum) { + // Albums are grouped as follows: + // - If we have a MusicBrainz ID, only group by it. This allows different Albums with the + // same name to be differentiated, which is common in large libraries. + // - If we do not have a MusicBrainz ID, compare by the lowercase album name and lowercase + // artist name. This allows for case-insensitive artist/album grouping, which can be common + // for albums/artists that have different naming (ex. "RAMMSTEIN" vs. "Rammstein"). - override fun hashCode() = hashCode + // Cache the hash-code for HashMap efficiency. + private val hashCode = + value.musicBrainzId?.hashCode() + ?: (31 * value.name.lowercase().hashCode() + value.rawArtists.hashCode()) - override fun equals(other: Any?) = - other is RawAlbum && - when { - musicBrainzId != null && other.musicBrainzId != null -> - musicBrainzId == other.musicBrainzId - musicBrainzId == null && other.musicBrainzId == null -> - name.equals(other.name, true) && rawArtists == other.rawArtists - else -> false - } + override fun hashCode() = hashCode + + override fun equals(other: Any?) = + other is Key && + when { + value.musicBrainzId != null && other.value.musicBrainzId != null -> + value.musicBrainzId == other.value.musicBrainzId + value.musicBrainzId == null && other.value.musicBrainzId == null -> + other.value.name.equals(other.value.name, true) && + other.value.rawArtists == other.value.rawArtists + else -> false + } + } } /** @@ -151,33 +158,42 @@ data class RawArtist( /** @see Music.name */ val sortName: String? = null ) { - // Artists are grouped as follows: - // - If we have a MusicBrainz ID, only group by it. This allows different Artists with the - // same name to be differentiated, which is common in large libraries. - // - If we do not have a MusicBrainz ID, compare by the lowercase name. This allows artist - // grouping to be case-insensitive. + val key = Key(this) - // Cache the hashCode for HashMap efficiency. - private val hashCode = musicBrainzId?.hashCode() ?: name?.lowercase().hashCode() + /** + * Allows [RawArtist]s to be compared by "fundamental" information that is unlikely to change on + * an item-by-item + */ + data class Key(val value: RawArtist) { + // Artists are grouped as follows: + // - If we have a MusicBrainz ID, only group by it. This allows different Artists with the + // same name to be differentiated, which is common in large libraries. + // - If we do not have a MusicBrainz ID, compare by the lowercase name. This allows artist + // grouping to be case-insensitive. - // Compare names and MusicBrainz IDs in order to differentiate artists with the - // same name in large libraries. + // Cache the hashCode for HashMap efficiency. + private val hashCode = value.musicBrainzId?.hashCode() ?: value.name?.lowercase().hashCode() - override fun hashCode() = hashCode + // Compare names and MusicBrainz IDs in order to differentiate artists with the + // same name in large libraries. - override fun equals(other: Any?) = - other is RawArtist && - when { - musicBrainzId != null && other.musicBrainzId != null -> - musicBrainzId == other.musicBrainzId - musicBrainzId == null && other.musicBrainzId == null -> - when { - name != null && other.name != null -> name.equals(other.name, true) - name == null && other.name == null -> true - else -> false - } - else -> false - } + override fun hashCode() = hashCode + + override fun equals(other: Any?) = + other is Key && + when { + value.musicBrainzId != null && other.value.musicBrainzId != null -> + value.musicBrainzId == other.value.musicBrainzId + value.musicBrainzId == null && other.value.musicBrainzId == null -> + when { + value.name != null && other.value.name != null -> + value.name.equals(other.value.name, true) + value.name == null && other.value.name == null -> true + else -> false + } + else -> false + } + } } /** @@ -189,20 +205,24 @@ data class RawGenre( /** @see Music.name */ val name: String? = null ) { + val key = Key(this) - // Cache the hashCode for HashMap efficiency. - private val hashCode = name?.lowercase().hashCode() + data class Key(val value: RawGenre) { + // Cache the hashCode for HashMap efficiency. + private val hashCode = value.name?.lowercase().hashCode() - // Only group by the lowercase genre name. This allows Genre grouping to be - // case-insensitive, which may be helpful in some libraries with different ways of - // formatting genres. - override fun hashCode() = hashCode + // Only group by the lowercase genre name. This allows Genre grouping to be + // case-insensitive, which may be helpful in some libraries with different ways of + // formatting genres. + override fun hashCode() = hashCode - override fun equals(other: Any?) = - other is RawGenre && - when { - name != null && other.name != null -> name.equals(other.name, true) - name == null && other.name == null -> true - else -> false - } + override fun equals(other: Any?) = + other is Key && + when { + value.name != null && other.value.name != null -> + value.name.equals(other.value.name, true) + value.name == null && other.value.name == null -> true + else -> false + } + } } diff --git a/app/src/main/java/org/oxycblt/auxio/music/user/UserLibrary.kt b/app/src/main/java/org/oxycblt/auxio/music/user/UserLibrary.kt index f4caab962..f6a888eb7 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/user/UserLibrary.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/user/UserLibrary.kt @@ -149,6 +149,8 @@ private class UserLibraryImpl( private val playlistMap: MutableMap, private val musicSettings: MusicSettings ) : MutableUserLibrary { + override fun hashCode() = playlistMap.hashCode() + override fun equals(other: Any?) = other is UserLibraryImpl && other.playlistMap == playlistMap override fun toString() = "UserLibrary(playlists=${playlists.size})" override val playlists: List diff --git a/app/src/main/java/org/oxycblt/auxio/playback/system/PlaybackService.kt b/app/src/main/java/org/oxycblt/auxio/playback/system/PlaybackService.kt index d4ceaa88f..c1dbbe7c2 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/system/PlaybackService.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/system/PlaybackService.kt @@ -107,8 +107,8 @@ class PlaybackService : // Coroutines private val serviceJob = Job() - private val restoreScope = CoroutineScope(serviceJob + Dispatchers.Main) - private val saveScope = CoroutineScope(serviceJob + Dispatchers.Main) + private val restoreScope = CoroutineScope(serviceJob + Dispatchers.IO) + private val saveScope = CoroutineScope(serviceJob + Dispatchers.IO) // --- SERVICE OVERRIDES ---