diff --git a/app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt b/app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt index c3e044349..1196c02ad 100644 --- a/app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/detail/DetailViewModel.kt @@ -24,9 +24,11 @@ import androidx.annotation.StringRes import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.launch +import kotlinx.coroutines.yield import org.oxycblt.auxio.R import org.oxycblt.auxio.music.Album import org.oxycblt.auxio.music.Artist @@ -40,7 +42,6 @@ import org.oxycblt.auxio.settings.Settings import org.oxycblt.auxio.ui.Sort import org.oxycblt.auxio.ui.recycler.Header import org.oxycblt.auxio.ui.recycler.Item -import org.oxycblt.auxio.util.TaskGuard import org.oxycblt.auxio.util.application import org.oxycblt.auxio.util.logD import org.oxycblt.auxio.util.logW @@ -70,6 +71,8 @@ class DetailViewModel(application: Application) : val currentSong: StateFlow get() = _currentSong + private var currentSongJob: Job? = null + private val _currentAlbum = MutableStateFlow(null) val currentAlbum: StateFlow get() = _currentAlbum @@ -114,8 +117,6 @@ class DetailViewModel(application: Application) : currentGenre.value?.let(::refreshGenreData) } - private val songGuard = TaskGuard() - fun setSongUid(uid: Music.UID) { if (_currentSong.value?.run { song.uid } == uid) return val library = unlikelyToBeNull(musicStore.library) @@ -124,7 +125,6 @@ class DetailViewModel(application: Application) : } fun clearSong() { - songGuard.newHandle() _currentSong.value = null } @@ -159,11 +159,11 @@ class DetailViewModel(application: Application) : } private fun generateDetailSong(song: Song) { + currentSongJob?.cancel() _currentSong.value = DetailSong(song, null) - viewModelScope.launch(Dispatchers.IO) { - val handle = songGuard.newHandle() + currentSongJob = viewModelScope.launch(Dispatchers.IO) { val info = generateDetailSongInfo(song) - songGuard.yield(handle) + yield() _currentSong.value = DetailSong(song, info) } } diff --git a/app/src/main/java/org/oxycblt/auxio/image/BitmapProvider.kt b/app/src/main/java/org/oxycblt/auxio/image/BitmapProvider.kt index cb004fc4b..330c423df 100644 --- a/app/src/main/java/org/oxycblt/auxio/image/BitmapProvider.kt +++ b/app/src/main/java/org/oxycblt/auxio/image/BitmapProvider.kt @@ -25,7 +25,6 @@ import coil.request.Disposable import coil.request.ImageRequest import coil.size.Size import org.oxycblt.auxio.music.Song -import org.oxycblt.auxio.util.TaskGuard /** * A utility to provide bitmaps in a manner less prone to race conditions. @@ -38,7 +37,8 @@ import org.oxycblt.auxio.util.TaskGuard */ class BitmapProvider(private val context: Context) { private var currentRequest: Request? = null - private var guard = TaskGuard() + private var currentHandle = 0L + private var handleLock = Any() /** If this provider is currently attempting to load something. */ val isBusy: Boolean @@ -50,7 +50,9 @@ class BitmapProvider(private val context: Context) { */ @Synchronized fun load(song: Song, target: Target) { - val handle = guard.newHandle() + val handle = synchronized(handleLock) { + ++currentHandle + } currentRequest?.run { disposable.dispose() } currentRequest = null @@ -62,13 +64,17 @@ class BitmapProvider(private val context: Context) { .size(Size.ORIGINAL) .target( onSuccess = { - if (guard.check(handle)) { - target.onCompleted(it.toBitmap()) + synchronized(handleLock) { + if (currentHandle == handle) { + target.onCompleted(it.toBitmap()) + } } }, onError = { - if (guard.check(handle)) { - target.onCompleted(null) + synchronized(handleLock) { + if (currentHandle == handle) { + target.onCompleted(null) + } } } ) diff --git a/app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataLayer.kt b/app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataLayer.kt index 74b91b32d..f40debe8c 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataLayer.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataLayer.kt @@ -54,7 +54,7 @@ class MetadataLayer(private val context: Context, private val mediaStoreLayer: M /** Finalize the sub-layers that this layer relies on. */ fun finalize(rawSongs: List) = mediaStoreLayer.finalize(rawSongs) - fun parse(emit: (Song.Raw) -> Unit) { + suspend fun parse(emit: suspend (Song.Raw) -> Unit) { while (true) { val raw = Song.Raw() if (mediaStoreLayer.populateRaw(raw) ?: break) { diff --git a/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt b/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt index fbe4b142f..b64cd5bcd 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt @@ -25,6 +25,7 @@ import androidx.core.content.ContextCompat import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext +import kotlinx.coroutines.yield import org.oxycblt.auxio.BuildConfig import org.oxycblt.auxio.music.Album import org.oxycblt.auxio.music.Artist @@ -37,7 +38,6 @@ import org.oxycblt.auxio.music.extractor.Api30MediaStoreLayer import org.oxycblt.auxio.music.extractor.CacheLayer import org.oxycblt.auxio.music.extractor.MetadataLayer import org.oxycblt.auxio.ui.Sort -import org.oxycblt.auxio.util.TaskGuard import org.oxycblt.auxio.util.logD import org.oxycblt.auxio.util.logE import org.oxycblt.auxio.util.logW @@ -62,14 +62,11 @@ import org.oxycblt.auxio.util.logW * directly work with music loading, making such redundant. * * @author OxygenCobalt - * - * TODO: Try to replace TaskGuard with yield when possible */ class Indexer { private var lastResponse: Response? = null private var indexingState: Indexing? = null - private var guard = TaskGuard() private var controller: Controller? = null private var callback: Callback? = null @@ -136,21 +133,19 @@ class Indexer { * complete, a new completion state will be pushed to each callback. */ suspend fun index(context: Context) { - val handle = guard.newHandle() - val notGranted = ContextCompat.checkSelfPermission(context, PERMISSION_READ_AUDIO) == PackageManager.PERMISSION_DENIED if (notGranted) { - emitCompletion(Response.NoPerms, handle) + emitCompletion(Response.NoPerms) return } val response = try { val start = System.currentTimeMillis() - val library = indexImpl(context, handle) + val library = indexImpl(context) if (library != null) { logD( "Music indexing completed successfully in " + @@ -171,7 +166,7 @@ class Indexer { Response.Err(e) } - emitCompletion(response, handle) + emitCompletion(response) } /** @@ -192,16 +187,14 @@ class Indexer { @Synchronized fun cancelLast() { logD("Cancelling last job") - val handle = guard.newHandle() - emitIndexing(null, handle) + emitIndexing(null) } /** - * Run the proper music loading process. [handle] must be a truthful handle of the task calling - * this function. + * Run the proper music loading process. */ - private fun indexImpl(context: Context, handle: Long): MusicStore.Library? { - emitIndexing(Indexing.Indeterminate, handle) + private suspend fun indexImpl(context: Context): MusicStore.Library? { + emitIndexing(Indexing.Indeterminate) // Create the chain of layers. Each layer builds on the previous layer and // enables version-specific features in order to create the best possible music @@ -221,7 +214,7 @@ class Indexer { val metadataLayer = MetadataLayer(context, mediaStoreLayer) - val songs = buildSongs(metadataLayer, handle) + val songs = buildSongs(metadataLayer) if (songs.isEmpty()) { return null } @@ -248,12 +241,15 @@ class Indexer { * [buildGenres] functions must be called with the returned list so that all songs are properly * linked up. */ - private fun buildSongs(metadataLayer: MetadataLayer, handle: Long): List { + private suspend fun buildSongs(metadataLayer: MetadataLayer): List { + logD("Starting indexing process") + val start = System.currentTimeMillis() // Initialize the extractor chain. This also nets us the projected total // that we can show when loading. val total = metadataLayer.init() + yield() // Note: We use a set here so we can eliminate effective duplicates of // songs (by UID). @@ -263,7 +259,10 @@ class Indexer { metadataLayer.parse { raw -> songs.add(Song(raw)) rawSongs.add(raw) - emitIndexing(Indexing.Songs(songs.size, total), handle) + + // Check if we got cancelled after every song addition. + yield() + emitIndexing(Indexing.Songs(songs.size, total)) } metadataLayer.finalize(rawSongs) @@ -351,15 +350,7 @@ class Indexer { } @Synchronized - private fun emitIndexing(indexing: Indexing?, handle: Long) { - guard.yield(handle) - - if (indexing == indexingState) { - // Ignore redundant states used when the backends just want to check for - // a cancellation - return - } - + private fun emitIndexing(indexing: Indexing?) { indexingState = indexing // If we have canceled the loading process, we want to revert to a previous completion @@ -371,8 +362,8 @@ class Indexer { callback?.onIndexerStateChanged(state) } - private suspend fun emitCompletion(response: Response, handle: Long) { - guard.yield(handle) + private suspend fun emitCompletion(response: Response) { + yield() // 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. diff --git a/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt index a52499c48..90272be9d 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt @@ -57,6 +57,7 @@ class IndexerService : Service(), Indexer.Controller, Settings.Callback { private val serviceJob = Job() private val indexScope = CoroutineScope(serviceJob + Dispatchers.IO) + private var currentIndexJob: Job? = null private val playbackManager = PlaybackStateManager.getInstance() @@ -118,10 +119,11 @@ class IndexerService : Service(), Indexer.Controller, Settings.Callback { override fun onStartIndexing() { if (indexer.isIndexing) { + currentIndexJob?.cancel() indexer.cancelLast() } - indexScope.launch { indexer.index(this@IndexerService) } + currentIndexJob = indexScope.launch { indexer.index(this@IndexerService) } } override fun onIndexerStateChanged(state: Indexer.State?) { diff --git a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt index 70aeac6f5..4fb2888db 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt @@ -307,6 +307,7 @@ class PlaybackViewModel(application: Application) : override fun onStateChanged(state: InternalPlayer.State) { _isPlaying.value = state.isPlaying + _positionDs.value = state.calculateElapsedPosition().msToDs() // Start watching the position again lastPositionJob?.cancel() diff --git a/app/src/main/java/org/oxycblt/auxio/playback/state/InternalPlayer.kt b/app/src/main/java/org/oxycblt/auxio/playback/state/InternalPlayer.kt index 30daae003..14c44f02f 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/state/InternalPlayer.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/state/InternalPlayer.kt @@ -30,7 +30,7 @@ interface InternalPlayer { /** Whether the player should rewind instead of going to the previous song. */ val shouldRewindWithPrev: Boolean - val currentState: State + fun makeState(durationMs: Long): State /** Called when a new song should be loaded into the player. */ fun loadSong(song: Song?, play: Boolean) diff --git a/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt b/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt index 3613232c1..16107810c 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt @@ -342,7 +342,7 @@ class PlaybackStateManager private constructor() { return } - val newState = internalPlayer.currentState + val newState = internalPlayer.makeState(song?.durationMs ?: 0) if (newState != playerState) { playerState = newState notifyStateChanged() 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 ed77ab441..8a137b032 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 @@ -58,6 +58,7 @@ import org.oxycblt.auxio.util.logD import org.oxycblt.auxio.widgets.WidgetComponent import org.oxycblt.auxio.widgets.WidgetProvider import kotlin.math.max +import kotlin.math.min /** * A service that manages the system-side aspects of playback, such as: @@ -214,6 +215,60 @@ class PlaybackService : logD("Service destroyed") } + // --- CONTROLLER OVERRIDES --- + + override val audioSessionId: Int + get() = player.audioSessionId + + override val shouldRewindWithPrev: Boolean + get() = settings.rewindWithPrev && player.currentPosition > REWIND_THRESHOLD + + override fun makeState(durationMs: Long) = + InternalPlayer.State.new( + player.playWhenReady, + player.isPlaying, + max(min(player.currentPosition, durationMs), 0) + ) + + override fun loadSong(song: Song?, play: Boolean) { + if (song == null) { + // Stop the foreground state if there's nothing to play. + logD("Nothing playing, stopping playback") + player.stop() + if (openAudioEffectSession) { + // Make sure to close the audio session when we stop playback. + broadcastAudioEffectAction(AudioEffect.ACTION_CLOSE_AUDIO_EFFECT_CONTROL_SESSION) + openAudioEffectSession = false + } + + stopAndSave() + return + } + + logD("Loading ${song.rawName}") + player.setMediaItem(MediaItem.fromUri(song.uri)) + player.prepare() + + if (!openAudioEffectSession) { + // Android does not like it if you start an audio effect session without having + // something within your player buffer. Make sure we only start one when we load + // a song. + broadcastAudioEffectAction(AudioEffect.ACTION_OPEN_AUDIO_EFFECT_CONTROL_SESSION) + openAudioEffectSession = true + } + + player.playWhenReady = play + } + + override fun seekTo(positionMs: Long) { + logD("Seeking to ${positionMs}ms") + player.seekTo(positionMs) + } + + override fun changePlaying(isPlaying: Boolean) { + player.playWhenReady = isPlaying + } + // --- PLAYER OVERRIDES --- override fun onEvents(player: Player, events: Player.Events) { @@ -270,52 +325,27 @@ class PlaybackService : } } - // --- CONTROLLER OVERRIDES --- + // --- MUSICSTORE OVERRIDES --- - override val audioSessionId: Int - get() = player.audioSessionId - - override val shouldRewindWithPrev: Boolean - get() = settings.rewindWithPrev && player.currentPosition > REWIND_THRESHOLD - - override val currentState: InternalPlayer.State - get() = - InternalPlayer.State.new( - player.playWhenReady, - player.isPlaying, - max(player.currentPosition, 0) - ) - - override fun loadSong(song: Song?, play: Boolean) { - if (song == null) { - // Stop the foreground state if there's nothing to play. - logD("Nothing playing, stopping playback") - player.stop() - if (openAudioEffectSession) { - // Make sure to close the audio session when we stop playback. - broadcastAudioEffectAction(AudioEffect.ACTION_CLOSE_AUDIO_EFFECT_CONTROL_SESSION) - openAudioEffectSession = false - } - - stopAndSave() - return + override fun onLibraryChanged(library: MusicStore.Library?) { + if (library != null) { + playbackManager.requestAction(this) } - - logD("Loading ${song.rawName}") - player.setMediaItem(MediaItem.fromUri(song.uri)) - player.prepare() - - if (!openAudioEffectSession) { - // Android does not like it if you start an audio effect session without having - // something within your player buffer. Make sure we only start one when we load - // a song. - broadcastAudioEffectAction(AudioEffect.ACTION_OPEN_AUDIO_EFFECT_CONTROL_SESSION) - openAudioEffectSession = true - } - - player.playWhenReady = play } + // --- SETTINGSMANAGER OVERRIDES --- + + override fun onSettingChanged(key: String) { + if (key == getString(R.string.set_key_replay_gain) || + key == getString(R.string.set_key_pre_amp_with) || + key == getString(R.string.set_key_pre_amp_without) + ) { + onTracksChanged(player.currentTracks) + } + } + + // --- OTHER FUNCTIONS --- + private fun broadcastAudioEffectAction(event: String) { sendBroadcast( Intent(event) @@ -337,15 +367,6 @@ class PlaybackService : } } - override fun seekTo(positionMs: Long) { - logD("Seeking to ${positionMs}ms") - player.seekTo(positionMs) - } - - override fun changePlaying(isPlaying: Boolean) { - player.playWhenReady = isPlaying - } - override fun onAction(action: InternalPlayer.Action): Boolean { val library = musicStore.library if (library != null) { @@ -397,27 +418,6 @@ class PlaybackService : } } - // --- MUSICSTORE OVERRIDES --- - - override fun onLibraryChanged(library: MusicStore.Library?) { - if (library != null) { - playbackManager.requestAction(this) - } - } - - // --- SETTINGSMANAGER OVERRIDES --- - - override fun onSettingChanged(key: String) { - if (key == getString(R.string.set_key_replay_gain) || - key == getString(R.string.set_key_pre_amp_with) || - key == getString(R.string.set_key_pre_amp_without) - ) { - onTracksChanged(player.currentTracks) - } - } - - // --- OTHER FUNCTIONS --- - /** A [BroadcastReceiver] for receiving general playback events from the system. */ private inner class PlaybackReceiver : BroadcastReceiver() { private var initialHeadsetPlugEventHandled = false diff --git a/app/src/main/java/org/oxycblt/auxio/search/SearchViewModel.kt b/app/src/main/java/org/oxycblt/auxio/search/SearchViewModel.kt index 1e1f476f2..3b8b2f3c5 100644 --- a/app/src/main/java/org/oxycblt/auxio/search/SearchViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/search/SearchViewModel.kt @@ -23,9 +23,11 @@ import androidx.annotation.IdRes import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.launch +import kotlinx.coroutines.yield import org.oxycblt.auxio.R import org.oxycblt.auxio.music.Album import org.oxycblt.auxio.music.Artist @@ -38,7 +40,6 @@ import org.oxycblt.auxio.ui.DisplayMode import org.oxycblt.auxio.ui.Sort import org.oxycblt.auxio.ui.recycler.Header import org.oxycblt.auxio.ui.recycler.Item -import org.oxycblt.auxio.util.TaskGuard import org.oxycblt.auxio.util.application import org.oxycblt.auxio.util.logD import java.text.Normalizer @@ -62,15 +63,16 @@ class SearchViewModel(application: Application) : get() = settings.searchFilterMode private var lastQuery: String? = null - private var guard = TaskGuard() + private var currentSearchJob: Job? = null /** * Use [query] to perform a search of the music library. Will push results to [searchResults]. */ fun search(query: String?) { - val handle = guard.newHandle() lastQuery = query + currentSearchJob?.cancel() + val library = musicStore.library if (query.isNullOrEmpty() || library == null) { logD("No music/query, ignoring search") @@ -81,7 +83,7 @@ class SearchViewModel(application: Application) : logD("Performing search for $query") // Searching can be quite expensive, so get on a co-routine - viewModelScope.launch { + currentSearchJob = viewModelScope.launch { val sort = Sort(Sort.Mode.ByName, true) val results = mutableListOf() @@ -115,7 +117,7 @@ class SearchViewModel(application: Application) : } } - guard.yield(handle) + yield() _searchResults.value = results } } diff --git a/app/src/main/java/org/oxycblt/auxio/util/PrimitiveUtil.kt b/app/src/main/java/org/oxycblt/auxio/util/PrimitiveUtil.kt index 43ccf2d90..f4fba0968 100644 --- a/app/src/main/java/org/oxycblt/auxio/util/PrimitiveUtil.kt +++ b/app/src/main/java/org/oxycblt/auxio/util/PrimitiveUtil.kt @@ -21,7 +21,6 @@ import android.os.Looper import org.oxycblt.auxio.BuildConfig import java.lang.reflect.Field import java.lang.reflect.Method -import java.util.concurrent.CancellationException import kotlin.reflect.KClass /** Assert that we are on a background thread. */ @@ -57,34 +56,3 @@ fun lazyReflectedField(clazz: KClass<*>, field: String) = lazy { fun lazyReflectedMethod(clazz: KClass<*>, method: String) = lazy { clazz.java.getDeclaredMethod(method).also { it.isAccessible = true } } - -/** - * An abstraction that allows cheap cooperative multi-threading in shared object contexts. Every new - * task should call [newHandle], while every running task should call [check] or [yield] depending - * on the situation to determine if it should continue. Failure to follow the expectations of this - * class will result in bugs. - * - * @author OxygenCobalt - */ -class TaskGuard { - private var currentHandle = 0L - - /** - * Returns a new handle to the calling task while invalidating the handle of the previous task. - */ - @Synchronized fun newHandle() = ++currentHandle - - /** Check if the given [handle] is still valid. */ - @Synchronized fun check(handle: Long) = handle == currentHandle - - /** - * Alternative to [kotlinx.coroutines.yield], that achieves the same behavior but in a much - * cheaper manner. - */ - @Synchronized - fun yield(handle: Long) { - if (!check(handle)) { - throw CancellationException() - } - } -}