From 55d3bd79ba3b0a8cb7215868d502a425a5a0b58a Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Wed, 22 Jan 2025 12:19:49 -0700 Subject: [PATCH] musikr: refactor graphing The sub-elements are still very deeply coupled, but that's to be expected. What this enables is reasonable testing of the graphing system, given that it's easily the most brittle part of musikr. --- .../org/oxycblt/musikr/graph/AlbumGraph.kt | 42 +++++++------ .../org/oxycblt/musikr/graph/ArtistGraph.kt | 52 +++++++++------ .../org/oxycblt/musikr/graph/GenreGraph.kt | 39 ++++++++---- .../org/oxycblt/musikr/graph/MusicGraph.kt | 41 ++++++------ .../org/oxycblt/musikr/graph/PlaylistGraph.kt | 26 ++++++-- .../org/oxycblt/musikr/graph/SongGraph.kt | 63 +++++-------------- .../oxycblt/musikr/model/LibraryFactory.kt | 14 ++--- .../oxycblt/musikr/pipeline/EvaluateStep.kt | 11 +++- 8 files changed, 154 insertions(+), 134 deletions(-) diff --git a/musikr/src/main/java/org/oxycblt/musikr/graph/AlbumGraph.kt b/musikr/src/main/java/org/oxycblt/musikr/graph/AlbumGraph.kt index fe157fef2..af994530f 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/graph/AlbumGraph.kt +++ b/musikr/src/main/java/org/oxycblt/musikr/graph/AlbumGraph.kt @@ -22,27 +22,27 @@ import org.oxycblt.musikr.tag.interpret.PreAlbum import org.oxycblt.musikr.util.unlikelyToBeNull internal class AlbumGraph(private val artistGraph: ArtistGraph) { - val vertices = mutableMapOf() + private val vertices = mutableMapOf() - fun add(preAlbum: PreAlbum): AlbumVertex { + fun link(vertex: SongVertex) { // Albums themselves have their own parent artists that also need to be // linked up. - val albumartistGraph = preAlbum.preArtists.map { preArtist -> artistGraph.add(preArtist) } - val albumVertex = AlbumVertex(preAlbum, albumartistGraph.toMutableList()) - // Album vertex is linked, now link artists back to album. - albumartistGraph.forEach { artistVertex -> artistVertex.albumGraph.add(albumVertex) } - return albumVertex + val preAlbum = vertex.preSong.preAlbum + val albumVertex = + vertices.getOrPut(preAlbum) { AlbumVertex(preAlbum).also { artistGraph.link(it) } } + vertex.albumVertex = albumVertex + albumVertex.songVertices.add(vertex) } - fun simplify() { + fun solve(): Collection { val albumClusters = vertices.values.groupBy { it.preAlbum.rawName?.lowercase() } for (cluster in albumClusters.values) { simplifyAlbumCluster(cluster) } - // Remove any edges that wound up connecting to the same artist or genre // in the end after simplification. - vertices.values.forEach { it.artistGraph = it.artistGraph.distinct().toMutableList() } + vertices.values.forEach { it.artistVertices = it.artistVertices.distinct().toMutableList() } + return vertices.values } private fun simplifyAlbumCluster(cluster: Collection) { @@ -65,9 +65,9 @@ internal class AlbumGraph(private val artistGraph: ArtistGraph) { val noMbidPreAlbum = it.preAlbum.copy(musicBrainzId = null) val simpleMbidVertex = vertices.getOrPut(noMbidPreAlbum) { - AlbumVertex(noMbidPreAlbum, it.artistGraph.toMutableList()) + AlbumVertex(noMbidPreAlbum).apply { artistVertices = it.artistVertices } } - meldAlbumGraph(it, simpleMbidVertex) + meldAlbumVertices(it, simpleMbidVertex) simpleMbidVertex } simplifyAlbumClusterImpl(strippedCluster) @@ -84,31 +84,33 @@ internal class AlbumGraph(private val artistGraph: ArtistGraph) { val dst = clusterSet.maxBy { it.songVertices.size } clusterSet.remove(dst) for (src in clusterSet) { - meldAlbumGraph(src, dst) + meldAlbumVertices(src, dst) } } - private fun meldAlbumGraph(src: AlbumVertex, dst: AlbumVertex) { + private fun meldAlbumVertices(src: AlbumVertex, dst: AlbumVertex) { if (src == dst) { // Same vertex, do nothing return } // Link all songs and artists from the irrelevant album to the relevant album. dst.songVertices.addAll(src.songVertices) - dst.artistGraph.addAll(src.artistGraph) + dst.artistVertices.addAll(src.artistVertices) // Update all songs and artists to point to the relevant album. src.songVertices.forEach { it.albumVertex = dst } - src.artistGraph.forEach { - it.albumGraph.remove(src) - it.albumGraph.add(dst) + src.artistVertices.forEach { + it.albumVertices.remove(src) + it.albumVertices.add(dst) } // Remove the irrelevant album from the graph. vertices.remove(src.preAlbum) } } -internal class AlbumVertex(val preAlbum: PreAlbum, var artistGraph: MutableList) : - Vertex { +internal class AlbumVertex( + val preAlbum: PreAlbum, +) : Vertex { + var artistVertices = mutableListOf() val songVertices = mutableSetOf() override var tag: Any? = null diff --git a/musikr/src/main/java/org/oxycblt/musikr/graph/ArtistGraph.kt b/musikr/src/main/java/org/oxycblt/musikr/graph/ArtistGraph.kt index 2ca7872df..d6b6f28d4 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/graph/ArtistGraph.kt +++ b/musikr/src/main/java/org/oxycblt/musikr/graph/ArtistGraph.kt @@ -21,17 +21,29 @@ package org.oxycblt.musikr.graph import org.oxycblt.musikr.tag.interpret.PreArtist import org.oxycblt.musikr.util.unlikelyToBeNull -internal class ArtistGraph() { - val vertices = mutableMapOf() +internal class ArtistGraph { + private val vertices = mutableMapOf() - fun add(preArtist: PreArtist): ArtistVertex = - vertices.getOrPut(preArtist) { ArtistVertex(preArtist) } + fun link(songVertex: SongVertex) { + val preArtists = songVertex.preSong.preArtists + val artistVertices = preArtists.map { vertices.getOrPut(it) { ArtistVertex(it) } } + songVertex.artistVertices.addAll(artistVertices) + artistVertices.forEach { it.songVertices.add(songVertex) } + } - fun simplify() { + fun link(albumVertex: AlbumVertex) { + val preArtists = albumVertex.preAlbum.preArtists + val artistVertices = preArtists.map { vertices.getOrPut(it) { ArtistVertex(it) } } + albumVertex.artistVertices = artistVertices.toMutableList() + artistVertices.forEach { it.albumVertices.add(albumVertex) } + } + + fun solve(): Collection { val artistClusters = vertices.values.groupBy { it.preArtist.rawName?.lowercase() } for (cluster in artistClusters.values) { simplifyArtistCluster(cluster) } + return vertices.values } private fun simplifyArtistCluster(cluster: Collection) { @@ -54,7 +66,7 @@ internal class ArtistGraph() { val noMbidPreArtist = it.preArtist.copy(musicBrainzId = null) val simpleMbidVertex = vertices.getOrPut(noMbidPreArtist) { ArtistVertex(noMbidPreArtist) } - meldArtistGraph(it, simpleMbidVertex) + meldArtistVertices(it, simpleMbidVertex) simpleMbidVertex } simplifyArtistClusterImpl(strippedCluster) @@ -69,33 +81,33 @@ internal class ArtistGraph() { val relevantArtistVertex = clusterSet.maxBy { it.songVertices.size } clusterSet.remove(relevantArtistVertex) for (irrelevantArtistVertex in clusterSet) { - meldArtistGraph(irrelevantArtistVertex, relevantArtistVertex) + meldArtistVertices(irrelevantArtistVertex, relevantArtistVertex) } } - private fun meldArtistGraph(src: ArtistVertex, dst: ArtistVertex) { + private fun meldArtistVertices(src: ArtistVertex, dst: ArtistVertex) { if (src == dst) { // Same vertex, do nothing return } // Link all songs and albums from the irrelevant artist to the relevant artist. dst.songVertices.addAll(src.songVertices) - dst.albumGraph.addAll(src.albumGraph) - dst.genreGraph.addAll(src.genreGraph) + dst.albumVertices.addAll(src.albumVertices) + dst.genreVertices.addAll(src.genreVertices) // Update all songs, albums, and genres to point to the relevant artist. src.songVertices.forEach { - val index = it.artistGraph.indexOf(src) + val index = it.artistVertices.indexOf(src) check(index >= 0) { "Illegal state: directed edge between artist and song" } - it.artistGraph[index] = dst + it.artistVertices[index] = dst } - src.albumGraph.forEach { - val index = it.artistGraph.indexOf(src) + src.albumVertices.forEach { + val index = it.artistVertices.indexOf(src) check(index >= 0) { "Illegal state: directed edge between artist and album" } - it.artistGraph[index] = dst + it.artistVertices[index] = dst } - src.genreGraph.forEach { - it.artistGraph.remove(src) - it.artistGraph.add(dst) + src.genreVertices.forEach { + it.artistVertices.remove(src) + it.artistVertices.add(dst) } // Remove the irrelevant artist from the graph. @@ -107,8 +119,8 @@ internal class ArtistVertex( val preArtist: PreArtist, ) : Vertex { val songVertices = mutableSetOf() - val albumGraph = mutableSetOf() - val genreGraph = mutableSetOf() + val albumVertices = mutableSetOf() + val genreVertices = mutableSetOf() override var tag: Any? = null override fun toString() = "ArtistVertex(preArtist=$preArtist)" diff --git a/musikr/src/main/java/org/oxycblt/musikr/graph/GenreGraph.kt b/musikr/src/main/java/org/oxycblt/musikr/graph/GenreGraph.kt index 3dcb34ad8..63477f677 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/graph/GenreGraph.kt +++ b/musikr/src/main/java/org/oxycblt/musikr/graph/GenreGraph.kt @@ -21,15 +21,30 @@ package org.oxycblt.musikr.graph import org.oxycblt.musikr.tag.interpret.PreGenre internal class GenreGraph { - val vertices = mutableMapOf() + private val vertices = mutableMapOf() - fun add(preGenre: PreGenre): GenreVertex = vertices.getOrPut(preGenre) { GenreVertex(preGenre) } + fun link(vertex: SongVertex) { + val preGenres = vertex.preSong.preGenres + val artistVertices = vertex.artistVertices - fun simplify() { + for (preGenre in preGenres) { + val genreVertex = vertices.getOrPut(preGenre) { GenreVertex(preGenre) } + vertex.genreVertices.add(genreVertex) + genreVertex.songVertices.add(vertex) + + for (artistVertex in artistVertices) { + genreVertex.artistVertices.add(artistVertex) + artistVertex.genreVertices.add(genreVertex) + } + } + } + + fun solve(): Collection { val genreClusters = vertices.values.groupBy { it.preGenre.rawName?.lowercase() } for (cluster in genreClusters.values) { simplifyGenreCluster(cluster) } + return vertices.values } private fun simplifyGenreCluster(cluster: Collection) { @@ -43,27 +58,27 @@ internal class GenreGraph { val dst = clusterSet.maxBy { it.songVertices.size } clusterSet.remove(dst) for (src in clusterSet) { - meldGenreGraph(src, dst) + meldGenreVertices(src, dst) } } - private fun meldGenreGraph(src: GenreVertex, dst: GenreVertex) { + private fun meldGenreVertices(src: GenreVertex, dst: GenreVertex) { if (src == dst) { // Same vertex, do nothing return } // Link all songs and artists from the irrelevant genre to the relevant genre. dst.songVertices.addAll(src.songVertices) - dst.artistGraph.addAll(src.artistGraph) + dst.artistVertices.addAll(src.artistVertices) // Update all songs and artists to point to the relevant genre. src.songVertices.forEach { - val index = it.genreGraph.indexOf(src) + val index = it.genreVertices.indexOf(src) check(index >= 0) { "Illegal state: directed edge between genre and song" } - it.genreGraph[index] = dst + it.genreVertices[index] = dst } - src.artistGraph.forEach { - it.genreGraph.remove(src) - it.genreGraph.add(dst) + src.artistVertices.forEach { + it.genreVertices.remove(src) + it.genreVertices.add(dst) } // Remove the irrelevant genre from the graph. vertices.remove(src.preGenre) @@ -72,7 +87,7 @@ internal class GenreGraph { internal class GenreVertex(val preGenre: PreGenre) : Vertex { val songVertices = mutableSetOf() - val artistGraph = mutableSetOf() + val artistVertices = mutableSetOf() override var tag: Any? = null override fun toString() = "GenreVertex(preGenre=$preGenre)" diff --git a/musikr/src/main/java/org/oxycblt/musikr/graph/MusicGraph.kt b/musikr/src/main/java/org/oxycblt/musikr/graph/MusicGraph.kt index c39e00b42..d663e2712 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/graph/MusicGraph.kt +++ b/musikr/src/main/java/org/oxycblt/musikr/graph/MusicGraph.kt @@ -22,11 +22,11 @@ import org.oxycblt.musikr.playlist.interpret.PrePlaylist import org.oxycblt.musikr.tag.interpret.PreSong internal data class MusicGraph( - val songVertex: List, - val albumVertex: List, - val artistVertex: List, - val genreVertex: List, - val playlistVertex: Set + val songVertices: Collection, + val albumVertices: Collection, + val artistVertices: Collection, + val genreVertices: Collection, + val playlistVertices: Collection ) { interface Builder { fun add(preSong: PreSong) @@ -41,35 +41,32 @@ internal data class MusicGraph( } } +internal interface Vertex { + val tag: Any? +} + private class MusicGraphBuilderImpl : MusicGraph.Builder { private val genreGraph = GenreGraph() private val artistGraph = ArtistGraph() private val albumGraph = AlbumGraph(artistGraph) private val playlistGraph = PlaylistGraph() - private val songVertices = SongGraph(albumGraph, artistGraph, genreGraph, playlistGraph) + private val songGraph = SongGraph(albumGraph, artistGraph, genreGraph, playlistGraph) override fun add(preSong: PreSong) { - songVertices.add(preSong) + songGraph.link(SongVertex(preSong)) } override fun add(prePlaylist: PrePlaylist) { - playlistGraph.add(prePlaylist) + playlistGraph.link(PlaylistVertex(prePlaylist)) } override fun build(): MusicGraph { - genreGraph.simplify() - artistGraph.simplify() - albumGraph.simplify() - songVertices.simplify() - - val graph = - MusicGraph( - songVertices.vertices.values.toList(), - albumGraph.vertices.values.toList(), - artistGraph.vertices.values.toList(), - genreGraph.vertices.values.toList(), - playlistGraph.vertices) - - return graph + val genreVertices = genreGraph.solve() + val artistVertices = artistGraph.solve() + val albumVertices = albumGraph.solve() + val songVertices = songGraph.solve() + val playlistVertices = playlistGraph.solve() + return MusicGraph( + songVertices, albumVertices, artistVertices, genreVertices, playlistVertices) } } diff --git a/musikr/src/main/java/org/oxycblt/musikr/graph/PlaylistGraph.kt b/musikr/src/main/java/org/oxycblt/musikr/graph/PlaylistGraph.kt index 6c398ec1e..44641ae02 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/graph/PlaylistGraph.kt +++ b/musikr/src/main/java/org/oxycblt/musikr/graph/PlaylistGraph.kt @@ -18,14 +18,30 @@ package org.oxycblt.musikr.graph +import org.oxycblt.musikr.playlist.SongPointer import org.oxycblt.musikr.playlist.interpret.PrePlaylist internal class PlaylistGraph { - val vertices = mutableSetOf() + private val pointerMap = mutableMapOf() + private val vertices = mutableSetOf() - fun add(prePlaylist: PrePlaylist) { - vertices.add(PlaylistVertex(prePlaylist)) + fun link(vertex: PlaylistVertex) { + for ((pointer, songVertex) in pointerMap) { + vertex.pointerMap[pointer]?.forEach { index -> vertex.songVertices[index] = songVertex } + } } + + fun link(vertex: SongVertex) { + val pointer = SongPointer.UID(vertex.preSong.uid) + pointerMap[pointer] = vertex + for (playlistVertex in vertices) { + playlistVertex.pointerMap[pointer]?.forEach { index -> + playlistVertex.songVertices[index] = vertex + } + } + } + + fun solve(): Collection = vertices } internal class PlaylistVertex(val prePlaylist: PrePlaylist) { @@ -33,7 +49,7 @@ internal class PlaylistVertex(val prePlaylist: PrePlaylist) { val pointerMap = prePlaylist.songPointers .withIndex() - .associateBy { it.value } - .mapValuesTo(mutableMapOf()) { it.value.index } + .groupBy { it.value } + .mapValuesTo(mutableMapOf()) { indexed -> indexed.value.map { it.index } } val tag: Any? = null } diff --git a/musikr/src/main/java/org/oxycblt/musikr/graph/SongGraph.kt b/musikr/src/main/java/org/oxycblt/musikr/graph/SongGraph.kt index 638f8fbf3..3f2834009 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/graph/SongGraph.kt +++ b/musikr/src/main/java/org/oxycblt/musikr/graph/SongGraph.kt @@ -19,7 +19,6 @@ package org.oxycblt.musikr.graph import org.oxycblt.musikr.Music -import org.oxycblt.musikr.playlist.SongPointer import org.oxycblt.musikr.tag.interpret.PreSong internal class SongGraph( @@ -28,66 +27,38 @@ internal class SongGraph( private val genreGraph: GenreGraph, private val playlistGraph: PlaylistGraph ) { - val vertices = mutableMapOf() + private val vertices = mutableMapOf() - fun add(preSong: PreSong): SongVertex? { + fun link(vertex: SongVertex): Boolean { + val preSong = vertex.preSong val uid = preSong.uid if (vertices.containsKey(uid)) { - return null + return false } - - val songGenreVertices = preSong.preGenres.map { preGenre -> genreGraph.add(preGenre) } - - val songArtistVertices = preSong.preArtists.map { preArtist -> artistGraph.add(preArtist) } - - val albumVertex = albumGraph.add(preSong.preAlbum) - - val songVertex = - SongVertex( - preSong, - albumVertex, - songArtistVertices.toMutableList(), - songGenreVertices.toMutableList()) - - songVertex.artistGraph.forEach { artistVertex -> - artistVertex.songVertices.add(songVertex) - songGenreVertices.forEach { genreVertex -> - // Mutually link any new genres to the artist - artistVertex.genreGraph.add(genreVertex) - genreVertex.artistGraph.add(artistVertex) - } - } - - songVertex.genreGraph.forEach { genreVertex -> genreVertex.songVertices.add(songVertex) } - - vertices[uid] = songVertex - - return songVertex + artistGraph.link(vertex) + genreGraph.link(vertex) + albumGraph.link(vertex) + playlistGraph.link(vertex) + vertices[uid] = vertex + return true } - fun simplify() { + fun solve(): Collection { vertices.entries.forEach { entry -> val vertex = entry.value - vertex.artistGraph = vertex.artistGraph.distinct().toMutableList() - vertex.genreGraph = vertex.genreGraph.distinct().toMutableList() - - playlistGraph.vertices.forEach { - val pointer = SongPointer.UID(entry.key) - val index = it.pointerMap[pointer] - if (index != null) { - it.songVertices[index] = vertex - } - } + vertex.artistVertices = vertex.artistVertices.distinct().toMutableList() + vertex.genreVertices = vertex.genreVertices.distinct().toMutableList() } + return vertices.values } } internal class SongVertex( val preSong: PreSong, - var albumVertex: AlbumVertex, - var artistGraph: MutableList, - var genreGraph: MutableList ) : Vertex { + var albumVertex: AlbumVertex? = null + var artistVertices = mutableListOf() + var genreVertices = mutableListOf() override var tag: Any? = null override fun toString() = "SongVertex(preSong=$preSong)" diff --git a/musikr/src/main/java/org/oxycblt/musikr/model/LibraryFactory.kt b/musikr/src/main/java/org/oxycblt/musikr/model/LibraryFactory.kt index 92296c80c..b4dc1b18e 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/model/LibraryFactory.kt +++ b/musikr/src/main/java/org/oxycblt/musikr/model/LibraryFactory.kt @@ -53,23 +53,23 @@ private class LibraryFactoryImpl() : LibraryFactory { playlistInterpreter: PlaylistInterpreter ): MutableLibrary { val songs = - graph.songVertex.mapTo(mutableSetOf()) { vertex -> + graph.songVertices.mapTo(mutableSetOf()) { vertex -> SongImpl(SongVertexCore(vertex)).also { vertex.tag = it } } val albums = - graph.albumVertex.mapTo(mutableSetOf()) { vertex -> + graph.albumVertices.mapTo(mutableSetOf()) { vertex -> AlbumImpl(AlbumVertexCore(vertex)).also { vertex.tag = it } } val artists = - graph.artistVertex.mapTo(mutableSetOf()) { vertex -> + graph.artistVertices.mapTo(mutableSetOf()) { vertex -> ArtistImpl(ArtistVertexCore(vertex)).also { vertex.tag = it } } val genres = - graph.genreVertex.mapTo(mutableSetOf()) { vertex -> + graph.genreVertices.mapTo(mutableSetOf()) { vertex -> GenreImpl(GenreVertexCore(vertex)).also { vertex.tag = it } } val playlists = - graph.playlistVertex.mapTo(mutableSetOf()) { vertex -> + graph.playlistVertices.mapTo(mutableSetOf()) { vertex -> PlaylistImpl(PlaylistVertexCore(vertex)) } return LibraryImpl( @@ -121,8 +121,8 @@ private class LibraryFactoryImpl() : LibraryFactory { } private companion object { - private inline fun tag(vertex: Vertex): T { - val tag = vertex.tag + private inline fun tag(vertex: Vertex?): T { + val tag = vertex?.tag check(tag is T) { "Dead Vertex Detected: $vertex" } return tag } diff --git a/musikr/src/main/java/org/oxycblt/musikr/pipeline/EvaluateStep.kt b/musikr/src/main/java/org/oxycblt/musikr/pipeline/EvaluateStep.kt index 4e195fdbf..fab5d8da1 100644 --- a/musikr/src/main/java/org/oxycblt/musikr/pipeline/EvaluateStep.kt +++ b/musikr/src/main/java/org/oxycblt/musikr/pipeline/EvaluateStep.kt @@ -26,6 +26,8 @@ import kotlinx.coroutines.flow.collect import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.merge import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import org.oxycblt.musikr.Interpretation import org.oxycblt.musikr.MutableLibrary import org.oxycblt.musikr.Storage @@ -78,11 +80,16 @@ private class EvaluateStepImpl( .flowOn(Dispatchers.Default) .buffer(Channel.UNLIMITED) val graphBuilder = MusicGraph.builder() + // Concurrent addition of playlists and songs could easily + // break the grapher (remember, individual pipeline elements + // are generally unaware of the highly concurrent nature of + // the pipeline), prevent that with a mutex. + val graphLock = Mutex() val graphBuild = merge( filterFlow.manager, - preSongs.onEach { graphBuilder.add(it) }, - prePlaylists.onEach { graphBuilder.add(it) }) + preSongs.onEach { graphLock.withLock { graphBuilder.add(it) } }, + prePlaylists.onEach { graphLock.withLock { graphBuilder.add(it) } }) graphBuild.collect() logger.d("starting graph build") val graph = graphBuilder.build()