From 94f2d28936b6dc3869691f5daf77d648bc08b553 Mon Sep 17 00:00:00 2001 From: OxygenCobalt Date: Thu, 7 Jul 2022 16:45:25 -0600 Subject: [PATCH] music: rework indexer thread safety Move the switch from IO to Main to within Indexer itself, through withContext. This is much easier to work with than the previous system of a separate "update" coroutine, which isn't really needed anymore given that I no longer need to do IO work when sanitizing the playback state. --- app/src/main/AndroidManifest.xml | 2 +- .../java/org/oxycblt/auxio/MainActivity.kt | 2 +- .../oxycblt/auxio/detail/DetailViewModel.kt | 2 +- .../org/oxycblt/auxio/home/HomeFragment.kt | 4 +- .../java/org/oxycblt/auxio/music/Music.kt | 5 +- .../org/oxycblt/auxio/music/MusicStore.kt | 14 ++++- .../auxio/music/backend/ExoPlayerBackend.kt | 2 +- .../auxio/music/backend/MediaStoreBackend.kt | 14 ++--- .../auxio/music/dirs/MusicDirAdapter.kt | 2 +- .../org/oxycblt/auxio/music/dirs/MusicDirs.kt | 2 +- .../auxio/music/dirs/MusicDirsDialog.kt | 2 +- .../auxio/music/{ => system}/Indexer.kt | 54 ++++++++++++------- .../music/{ => system}/IndexerNotification.kt | 2 +- .../music/{ => system}/IndexerService.kt | 40 +++++++------- .../music/{ => system}/IndexerViewModel.kt | 2 +- .../music/{ => system}/StorageFramework.kt | 2 +- .../auxio/playback/system/PlaybackService.kt | 2 + .../org/oxycblt/auxio/settings/Settings.kt | 2 +- .../oxycblt/auxio/settings/SettingsCompat.kt | 8 +-- .../auxio/settings/SettingsListFragment.kt | 4 +- .../org/oxycblt/auxio/ui/BottomSheetLayout.kt | 2 +- info/ARCHITECTURE.md | 6 ++- 22 files changed, 102 insertions(+), 73 deletions(-) rename app/src/main/java/org/oxycblt/auxio/music/{ => system}/Indexer.kt (92%) rename app/src/main/java/org/oxycblt/auxio/music/{ => system}/IndexerNotification.kt (98%) rename app/src/main/java/org/oxycblt/auxio/music/{ => system}/IndexerService.kt (80%) rename app/src/main/java/org/oxycblt/auxio/music/{ => system}/IndexerViewModel.kt (97%) rename app/src/main/java/org/oxycblt/auxio/music/{ => system}/StorageFramework.kt (99%) diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 6ef440e3f..1177fc4b3 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -77,7 +77,7 @@ extracting metadata, and constructing the music library. --> , val shouldInclude: Boolean) diff --git a/app/src/main/java/org/oxycblt/auxio/music/dirs/MusicDirsDialog.kt b/app/src/main/java/org/oxycblt/auxio/music/dirs/MusicDirsDialog.kt index ac7aae92f..0d5c78576 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/dirs/MusicDirsDialog.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/dirs/MusicDirsDialog.kt @@ -28,7 +28,7 @@ import androidx.core.view.isVisible import org.oxycblt.auxio.BuildConfig import org.oxycblt.auxio.R import org.oxycblt.auxio.databinding.DialogMusicDirsBinding -import org.oxycblt.auxio.music.Directory +import org.oxycblt.auxio.music.system.Directory import org.oxycblt.auxio.settings.Settings import org.oxycblt.auxio.ui.fragment.ViewBindingDialogFragment import org.oxycblt.auxio.util.context diff --git a/app/src/main/java/org/oxycblt/auxio/music/Indexer.kt b/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt similarity index 92% rename from app/src/main/java/org/oxycblt/auxio/music/Indexer.kt rename to app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt index beb5d6b60..23d27a761 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/Indexer.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt @@ -15,7 +15,7 @@ * along with this program. If not, see . */ -package org.oxycblt.auxio.music +package org.oxycblt.auxio.music.system import android.Manifest import android.content.Context @@ -24,7 +24,14 @@ import android.database.Cursor import android.os.Build import androidx.core.content.ContextCompat import kotlin.coroutines.cancellation.CancellationException +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext import org.oxycblt.auxio.BuildConfig +import org.oxycblt.auxio.music.Album +import org.oxycblt.auxio.music.Artist +import org.oxycblt.auxio.music.Genre +import org.oxycblt.auxio.music.MusicStore +import org.oxycblt.auxio.music.Song import org.oxycblt.auxio.music.backend.Api21MediaStoreBackend import org.oxycblt.auxio.music.backend.Api29MediaStoreBackend import org.oxycblt.auxio.music.backend.Api30MediaStoreBackend @@ -34,6 +41,7 @@ import org.oxycblt.auxio.ui.Sort import org.oxycblt.auxio.util.logD import org.oxycblt.auxio.util.logE import org.oxycblt.auxio.util.logW +import org.oxycblt.auxio.util.requireBackgroundThread /** * Auxio's media indexer. @@ -122,7 +130,9 @@ class Indexer { this.callback = null } - fun index(context: Context) { + suspend fun index(context: Context) { + requireBackgroundThread() + val generation = synchronized(this) { ++currentGeneration } val notGranted = @@ -180,11 +190,7 @@ class Indexer { @Synchronized private fun emitIndexing(indexing: Indexing?, generation: Long) { - if (currentGeneration != generation) { - // Not the running task anymore, cancel this co-routine. This allows a yield-like - // behavior to be implemented in a far cheaper manner for each backend. - throw CancellationException() - } + checkGenerationImpl(generation) if (indexing == indexingState) { // Ignore redundant states used when the backends just want to check for @@ -203,23 +209,33 @@ class Indexer { callback?.onIndexerStateChanged(state) } - @Synchronized - private fun emitCompletion(response: Response, generation: Long) { + private suspend fun emitCompletion(response: Response, generation: Long) { + synchronized(this) { + checkGenerationImpl(generation) + + // Do not check for redundancy here, as we actually need to notify a switch + // from Indexing -> Complete and not Indexing -> Indexing or Complete -> Complete. + + lastResponse = response + indexingState = null + } + + val state = State.Complete(response) + + // Swap to the Main thread so that downstream callbacks don't crash from being on + // a background thread. Does not occur in emitIndexing due to efficiency reasons. + withContext(Dispatchers.Main) { + controller?.onIndexerStateChanged(state) + callback?.onIndexerStateChanged(state) + } + } + + private fun checkGenerationImpl(generation: Long) { if (currentGeneration != generation) { // Not the running task anymore, cancel this co-routine. This allows a yield-like // behavior to be implemented in a far cheaper manner for each backend. throw CancellationException() } - - // Do not check for redundancy here, as we actually need to notify a switch - // from Indexing -> Complete and not Indexing -> Indexing or Complete -> Complete. - - lastResponse = response - indexingState = null - - val state = State.Complete(response) - controller?.onIndexerStateChanged(state) - callback?.onIndexerStateChanged(state) } /** diff --git a/app/src/main/java/org/oxycblt/auxio/music/IndexerNotification.kt b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerNotification.kt similarity index 98% rename from app/src/main/java/org/oxycblt/auxio/music/IndexerNotification.kt rename to app/src/main/java/org/oxycblt/auxio/music/system/IndexerNotification.kt index 3df87c034..0cfd1f1a4 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/IndexerNotification.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerNotification.kt @@ -15,7 +15,7 @@ * along with this program. If not, see . */ -package org.oxycblt.auxio.music +package org.oxycblt.auxio.music.system import android.app.NotificationChannel import android.app.NotificationManager diff --git a/app/src/main/java/org/oxycblt/auxio/music/IndexerService.kt b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt similarity index 80% rename from app/src/main/java/org/oxycblt/auxio/music/IndexerService.kt rename to app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt index d461e0431..824a44a36 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/IndexerService.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt @@ -15,7 +15,7 @@ * along with this program. If not, see . */ -package org.oxycblt.auxio.music +package org.oxycblt.auxio.music.system import android.app.Service import android.content.Intent @@ -28,6 +28,7 @@ import kotlinx.coroutines.Job import kotlinx.coroutines.launch import org.oxycblt.auxio.IntegerTable import org.oxycblt.auxio.R +import org.oxycblt.auxio.music.MusicStore import org.oxycblt.auxio.playback.state.PlaybackStateManager import org.oxycblt.auxio.settings.Settings import org.oxycblt.auxio.util.logD @@ -51,7 +52,6 @@ class IndexerService : Service(), Indexer.Controller, Settings.Callback { private val serviceJob = Job() private val indexScope = CoroutineScope(serviceJob + Dispatchers.IO) - private val updateScope = CoroutineScope(serviceJob + Dispatchers.Main) private val playbackManager = PlaybackStateManager.getInstance() private lateinit var settings: Settings @@ -108,30 +108,26 @@ class IndexerService : Service(), Indexer.Controller, Settings.Callback { val newLibrary = state.response.library - // Load was completed successfully. However, we still need to do some - // extra work to update the app's state. - updateScope.launch { - if (musicStore.library != null) { - // This is a new library to replace a pre-existing one. + if (musicStore.library != null) { + // This is a new library to replace a pre-existing one. - // Wipe possibly-invalidated album covers - imageLoader.memoryCache?.clear() + // Wipe possibly-invalidated album covers + imageLoader.memoryCache?.clear() - // Clear invalid models from PlaybackStateManager. - playbackManager.sanitize(newLibrary) - } - - musicStore.updateLibrary(newLibrary) - - stopForegroundSession() + // Clear invalid models from PlaybackStateManager. This is not connected + // to a callback as it is bad practice for a shared object to attach to + // the callback system of another. + playbackManager.sanitize(newLibrary) } - } else { - // On errors, while we would want to show a notification that displays the - // error, in practice that comes into conflict with the upcoming Android 13 - // notification permission, and there is no point implementing permission - // on-boarding for such when it will only be used for this. - stopForegroundSession() + + musicStore.updateLibrary(newLibrary) } + + // On errors, while we would want to show a notification that displays the + // error, in practice that comes into conflict with the upcoming Android 13 + // notification permission, and there is no point implementing permission + // on-boarding for such when it will only be used for this. + stopForegroundSession() } is Indexer.State.Indexing -> { // When loading, we want to enter the foreground state so that android does diff --git a/app/src/main/java/org/oxycblt/auxio/music/IndexerViewModel.kt b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerViewModel.kt similarity index 97% rename from app/src/main/java/org/oxycblt/auxio/music/IndexerViewModel.kt rename to app/src/main/java/org/oxycblt/auxio/music/system/IndexerViewModel.kt index 2ff162d9b..bd42c765d 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/IndexerViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerViewModel.kt @@ -15,7 +15,7 @@ * along with this program. If not, see . */ -package org.oxycblt.auxio.music +package org.oxycblt.auxio.music.system import androidx.lifecycle.ViewModel import kotlinx.coroutines.flow.MutableStateFlow diff --git a/app/src/main/java/org/oxycblt/auxio/music/StorageFramework.kt b/app/src/main/java/org/oxycblt/auxio/music/system/StorageFramework.kt similarity index 99% rename from app/src/main/java/org/oxycblt/auxio/music/StorageFramework.kt rename to app/src/main/java/org/oxycblt/auxio/music/system/StorageFramework.kt index 910e2a1d3..f36691143 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/StorageFramework.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/StorageFramework.kt @@ -15,7 +15,7 @@ * along with this program. If not, see . */ -package org.oxycblt.auxio.music +package org.oxycblt.auxio.music.system import android.annotation.SuppressLint import android.content.Context 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 66c03a1ff..0bf123a49 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 @@ -69,6 +69,8 @@ import org.oxycblt.auxio.widgets.WidgetProvider * * TODO: Android Auto * + * TODO: Get MediaSessionConnector (or the media3 equivalent) working or die trying + * * @author OxygenCobalt */ class PlaybackService : diff --git a/app/src/main/java/org/oxycblt/auxio/settings/Settings.kt b/app/src/main/java/org/oxycblt/auxio/settings/Settings.kt index 196374779..605f22b46 100644 --- a/app/src/main/java/org/oxycblt/auxio/settings/Settings.kt +++ b/app/src/main/java/org/oxycblt/auxio/settings/Settings.kt @@ -25,8 +25,8 @@ import androidx.core.content.edit import androidx.preference.PreferenceManager import org.oxycblt.auxio.R import org.oxycblt.auxio.home.tabs.Tab -import org.oxycblt.auxio.music.Directory import org.oxycblt.auxio.music.dirs.MusicDirs +import org.oxycblt.auxio.music.system.Directory import org.oxycblt.auxio.playback.replaygain.ReplayGainMode import org.oxycblt.auxio.playback.replaygain.ReplayGainPreAmp import org.oxycblt.auxio.playback.state.PlaybackMode diff --git a/app/src/main/java/org/oxycblt/auxio/settings/SettingsCompat.kt b/app/src/main/java/org/oxycblt/auxio/settings/SettingsCompat.kt index 0747919c8..694848be2 100644 --- a/app/src/main/java/org/oxycblt/auxio/settings/SettingsCompat.kt +++ b/app/src/main/java/org/oxycblt/auxio/settings/SettingsCompat.kt @@ -27,10 +27,10 @@ import android.util.Log import androidx.core.content.edit import java.io.File import org.oxycblt.auxio.R -import org.oxycblt.auxio.music.Directory -import org.oxycblt.auxio.music.directoryCompat -import org.oxycblt.auxio.music.isInternalCompat -import org.oxycblt.auxio.music.storageVolumesCompat +import org.oxycblt.auxio.music.system.Directory +import org.oxycblt.auxio.music.system.directoryCompat +import org.oxycblt.auxio.music.system.isInternalCompat +import org.oxycblt.auxio.music.system.storageVolumesCompat import org.oxycblt.auxio.ui.accent.Accent import org.oxycblt.auxio.util.logD import org.oxycblt.auxio.util.queryAll diff --git a/app/src/main/java/org/oxycblt/auxio/settings/SettingsListFragment.kt b/app/src/main/java/org/oxycblt/auxio/settings/SettingsListFragment.kt index 66ded97f0..12320f83b 100644 --- a/app/src/main/java/org/oxycblt/auxio/settings/SettingsListFragment.kt +++ b/app/src/main/java/org/oxycblt/auxio/settings/SettingsListFragment.kt @@ -31,8 +31,8 @@ import androidx.recyclerview.widget.RecyclerView import coil.Coil import org.oxycblt.auxio.R import org.oxycblt.auxio.home.tabs.TabCustomizeDialog -import org.oxycblt.auxio.music.IndexerViewModel import org.oxycblt.auxio.music.dirs.MusicDirsDialog +import org.oxycblt.auxio.music.system.IndexerViewModel import org.oxycblt.auxio.playback.PlaybackViewModel import org.oxycblt.auxio.playback.replaygain.PreAmpCustomizeDialog import org.oxycblt.auxio.settings.ui.IntListPreference @@ -53,6 +53,8 @@ import org.oxycblt.auxio.util.systemBarInsetsCompat * TODO: Add option to not restore state * * TODO: Disable playback state options when music is loading + * + * TODO: Indicate music loading in the "reload music" option??? */ @Suppress("UNUSED") class SettingsListFragment : PreferenceFragmentCompat() { diff --git a/app/src/main/java/org/oxycblt/auxio/ui/BottomSheetLayout.kt b/app/src/main/java/org/oxycblt/auxio/ui/BottomSheetLayout.kt index 004fb674d..ed0768d5a 100644 --- a/app/src/main/java/org/oxycblt/auxio/ui/BottomSheetLayout.kt +++ b/app/src/main/java/org/oxycblt/auxio/ui/BottomSheetLayout.kt @@ -511,7 +511,7 @@ constructor(context: Context, attrs: AttributeSet? = null, defStyle: Int = 0) : // TODO: Improve accessibility by: // 1. Adding a (non-visible) handle. Material components now technically does have // this, but it relies on the busted BottomSheetBehavior. - // 2. Adding the controls that BottomSheetBehavior defines in-app. + // 2. Adding the controls that BottomSheetBehavior defines sendAccessibilityEvent(AccessibilityEvent.TYPE_WINDOW_STATE_CHANGED) } diff --git a/info/ARCHITECTURE.md b/info/ARCHITECTURE.md index 7a499449c..033e77d36 100644 --- a/info/ARCHITECTURE.md +++ b/info/ARCHITECTURE.md @@ -205,8 +205,12 @@ This package also contains the two UI components used for all covers in Auxio: This package contains all `Music` implementations, the music loading implementation, and the music folder system. This is the second most complicated package in the app, as loading music in a sane way is horribly difficult. +Unlike other apps, Auxio does not load music from `MediaStore` as it is shown in the UI. That is dumb and stupid and prevents +any advanced features like Album Artists. Instead, we have a single loading process that constructs an entire in-memory music +library, which does increase memory usage, but allows for very high-quality metadata. + The major classes are: -- `MusicStore`, which is the container for a `Library` instance. Any code wanting to access the library should use this +- `MusicStore`, which is the container for a `Library` instance. Any code wanting to access the library should use this. - `Indexer`, which manages how music is loaded. This is only used by code that must reflect the music loading state. Internally, there are several other major systems: