From a810e13cfb17c8482c6797bf468f855d64b61c2c Mon Sep 17 00:00:00 2001 From: Ryan Harg <3821-ryan_harg@users.noreply.dev.funkwhale.audio> Date: Tue, 10 Jan 2023 10:00:41 +0000 Subject: [PATCH] Custom cache layer for cover art which ignores (pre-signed URL) query --- .../funkwhale/ffa/activities/MainActivity.kt | 14 +- .../funkwhale/ffa/adapters/AlbumsAdapter.kt | 7 +- .../ffa/adapters/AlbumsGridAdapter.kt | 6 +- .../funkwhale/ffa/adapters/ArtistsAdapter.kt | 16 +- .../ffa/adapters/FavoritesAdapter.kt | 7 +- .../ffa/adapters/PlaylistTracksAdapter.kt | 6 +- .../ffa/adapters/PlaylistsAdapter.kt | 6 +- .../funkwhale/ffa/adapters/SearchAdapter.kt | 6 +- .../funkwhale/ffa/adapters/TracksAdapter.kt | 7 +- .../funkwhale/ffa/fragments/AlbumsFragment.kt | 6 +- .../ffa/fragments/PlaylistTracksFragment.kt | 8 +- .../funkwhale/ffa/fragments/TracksFragment.kt | 8 +- .../java/audio/funkwhale/ffa/model/Artist.kt | 12 +- .../ffa/playback/MediaControlsManager.kt | 5 +- .../funkwhale/ffa/playback/PlayerService.kt | 7 +- .../audio/funkwhale/ffa/utils/Bottleneck.kt | 89 ++++++ .../audio/funkwhale/ffa/utils/CoverArt.kt | 260 ++++++++++++++++++ .../audio/funkwhale/ffa/utils/Extensions.kt | 10 - .../funkwhale/ffa/utils/BottleneckTest.kt | 207 ++++++++++++++ 19 files changed, 604 insertions(+), 83 deletions(-) create mode 100644 app/src/main/java/audio/funkwhale/ffa/utils/Bottleneck.kt create mode 100644 app/src/main/java/audio/funkwhale/ffa/utils/CoverArt.kt create mode 100644 app/src/test/java/audio/funkwhale/ffa/utils/BottleneckTest.kt diff --git a/app/src/main/java/audio/funkwhale/ffa/activities/MainActivity.kt b/app/src/main/java/audio/funkwhale/ffa/activities/MainActivity.kt index dea1c93..6feffda 100644 --- a/app/src/main/java/audio/funkwhale/ffa/activities/MainActivity.kt +++ b/app/src/main/java/audio/funkwhale/ffa/activities/MainActivity.kt @@ -46,6 +46,7 @@ import audio.funkwhale.ffa.repositories.Repository import audio.funkwhale.ffa.utils.AppContext import audio.funkwhale.ffa.utils.Command import audio.funkwhale.ffa.utils.CommandBus +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.Event import audio.funkwhale.ffa.utils.EventBus import audio.funkwhale.ffa.utils.FFACache @@ -56,7 +57,6 @@ import audio.funkwhale.ffa.utils.Userinfo import audio.funkwhale.ffa.utils.authorize import audio.funkwhale.ffa.utils.log import audio.funkwhale.ffa.utils.logError -import audio.funkwhale.ffa.utils.maybeLoad import audio.funkwhale.ffa.utils.maybeNormalizeUrl import audio.funkwhale.ffa.utils.mustNormalizeUrl import audio.funkwhale.ffa.utils.onApi @@ -69,7 +69,6 @@ import com.google.android.exoplayer2.Player import com.google.android.exoplayer2.offline.DownloadService import com.google.gson.Gson import com.preference.PowerPreference -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation import kotlinx.coroutines.Dispatchers.Default import kotlinx.coroutines.Dispatchers.IO @@ -477,15 +476,15 @@ class MainActivity : AppCompatActivity() { binding.nowPlayingContainer?.nowPlayingDetailsTitle?.text = track.title binding.nowPlayingContainer?.nowPlayingDetailsArtist?.text = track.artist.name - Picasso.get() - .maybeLoad(maybeNormalizeUrl(track.album?.cover?.urls?.original)) + val lic = this.layoutInflater.context + + CoverArt.withContext(lic, maybeNormalizeUrl(track.album?.cover?.urls?.original)) .fit() .centerCrop() .into(binding.nowPlayingContainer?.nowPlayingCover) binding.nowPlayingContainer?.nowPlayingDetailsCover?.let { nowPlayingDetailsCover -> - Picasso.get() - .maybeLoad(maybeNormalizeUrl(track.album?.cover())) + CoverArt.withContext(lic, maybeNormalizeUrl(track.album?.cover())) .fit() .centerCrop() .transform(RoundedCornersTransformation(16, 0)) @@ -498,8 +497,7 @@ class MainActivity : AppCompatActivity() { windowManager.defaultDisplay.getMetrics(this) }.widthPixels - val backgroundCover = Picasso.get() - .maybeLoad(maybeNormalizeUrl(track.album?.cover())) + val backgroundCover = CoverArt.withContext(lic, maybeNormalizeUrl(track.album?.cover())) .get() .run { Bitmap.createScaledBitmap(this, width, width, false).toDrawable(resources) } .apply { diff --git a/app/src/main/java/audio/funkwhale/ffa/adapters/AlbumsAdapter.kt b/app/src/main/java/audio/funkwhale/ffa/adapters/AlbumsAdapter.kt index 03e2c92..7e28bea 100644 --- a/app/src/main/java/audio/funkwhale/ffa/adapters/AlbumsAdapter.kt +++ b/app/src/main/java/audio/funkwhale/ffa/adapters/AlbumsAdapter.kt @@ -8,9 +8,7 @@ import androidx.recyclerview.widget.RecyclerView import audio.funkwhale.ffa.databinding.RowAlbumBinding import audio.funkwhale.ffa.fragments.FFAAdapter import audio.funkwhale.ffa.model.Album -import audio.funkwhale.ffa.utils.maybeLoad -import audio.funkwhale.ffa.utils.maybeNormalizeUrl -import com.squareup.picasso.Picasso +import audio.funkwhale.ffa.utils.CoverArt import jp.wasabeef.picasso.transformations.RoundedCornersTransformation class AlbumsAdapter( @@ -45,8 +43,7 @@ class AlbumsAdapter( override fun onBindViewHolder(holder: ViewHolder, position: Int) { val album = data[position] - Picasso.get() - .maybeLoad(maybeNormalizeUrl(album.cover())) + CoverArt.withContext(layoutInflater.context, album.cover()) .fit() .transform(RoundedCornersTransformation(8, 0)) .into(holder.art) diff --git a/app/src/main/java/audio/funkwhale/ffa/adapters/AlbumsGridAdapter.kt b/app/src/main/java/audio/funkwhale/ffa/adapters/AlbumsGridAdapter.kt index f3bde83..2e00174 100644 --- a/app/src/main/java/audio/funkwhale/ffa/adapters/AlbumsGridAdapter.kt +++ b/app/src/main/java/audio/funkwhale/ffa/adapters/AlbumsGridAdapter.kt @@ -8,9 +8,8 @@ import audio.funkwhale.ffa.R import audio.funkwhale.ffa.databinding.RowAlbumGridBinding import audio.funkwhale.ffa.fragments.FFAAdapter import audio.funkwhale.ffa.model.Album -import audio.funkwhale.ffa.utils.maybeLoad +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.maybeNormalizeUrl -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation class AlbumsGridAdapter( @@ -40,8 +39,7 @@ class AlbumsGridAdapter( override fun onBindViewHolder(holder: ViewHolder, position: Int) { val album = data[position] - Picasso.get() - .maybeLoad(maybeNormalizeUrl(album.cover())) + CoverArt.withContext(layoutInflater.context, maybeNormalizeUrl(album.cover())) .fit() .placeholder(R.drawable.cover) .transform(RoundedCornersTransformation(16, 0)) diff --git a/app/src/main/java/audio/funkwhale/ffa/adapters/ArtistsAdapter.kt b/app/src/main/java/audio/funkwhale/ffa/adapters/ArtistsAdapter.kt index 956860d..27de82e 100644 --- a/app/src/main/java/audio/funkwhale/ffa/adapters/ArtistsAdapter.kt +++ b/app/src/main/java/audio/funkwhale/ffa/adapters/ArtistsAdapter.kt @@ -9,9 +9,8 @@ import audio.funkwhale.ffa.R import audio.funkwhale.ffa.databinding.RowArtistBinding import audio.funkwhale.ffa.fragments.FFAAdapter import audio.funkwhale.ffa.model.Artist -import audio.funkwhale.ffa.utils.maybeLoad +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.maybeNormalizeUrl -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation class ArtistsAdapter( @@ -62,14 +61,11 @@ class ArtistsAdapter( override fun onBindViewHolder(holder: ViewHolder, position: Int) { val artist = active[position] - artist.albums?.let { albums -> - if (albums.isNotEmpty()) { - Picasso.get() - .maybeLoad(maybeNormalizeUrl(albums[0].cover?.urls?.original)) - .fit() - .transform(RoundedCornersTransformation(8, 0)) - .into(holder.art) - } + artist.cover()?.let { coverUrl -> + CoverArt.withContext(layoutInflater.context, maybeNormalizeUrl(coverUrl)) + .fit() + .transform(RoundedCornersTransformation(8, 0)) + .into(holder.art) } holder.name.text = artist.name diff --git a/app/src/main/java/audio/funkwhale/ffa/adapters/FavoritesAdapter.kt b/app/src/main/java/audio/funkwhale/ffa/adapters/FavoritesAdapter.kt index 8a1728e..332d474 100644 --- a/app/src/main/java/audio/funkwhale/ffa/adapters/FavoritesAdapter.kt +++ b/app/src/main/java/audio/funkwhale/ffa/adapters/FavoritesAdapter.kt @@ -16,10 +16,9 @@ import audio.funkwhale.ffa.fragments.FFAAdapter import audio.funkwhale.ffa.model.Track import audio.funkwhale.ffa.utils.Command import audio.funkwhale.ffa.utils.CommandBus -import audio.funkwhale.ffa.utils.maybeLoad +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.maybeNormalizeUrl import audio.funkwhale.ffa.utils.toast -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation import java.util.Collections @@ -67,8 +66,7 @@ class FavoritesAdapter( override fun onBindViewHolder(holder: ViewHolder, position: Int) { val favorite = data[position] - Picasso.get() - .maybeLoad(maybeNormalizeUrl(favorite.album?.cover())) + CoverArt.withContext(layoutInflater.context, maybeNormalizeUrl(favorite.album?.cover())) .fit() .placeholder(R.drawable.cover) .transform(RoundedCornersTransformation(16, 0)) @@ -173,7 +171,6 @@ class FavoritesAdapter( false -> { data.subList(layoutPosition, data.size).plus(data.subList(0, layoutPosition)).apply { CommandBus.send(Command.ReplaceQueue(this)) - context.toast("All tracks were added to your queue") } } diff --git a/app/src/main/java/audio/funkwhale/ffa/adapters/PlaylistTracksAdapter.kt b/app/src/main/java/audio/funkwhale/ffa/adapters/PlaylistTracksAdapter.kt index 0227dbe..23552ad 100644 --- a/app/src/main/java/audio/funkwhale/ffa/adapters/PlaylistTracksAdapter.kt +++ b/app/src/main/java/audio/funkwhale/ffa/adapters/PlaylistTracksAdapter.kt @@ -20,10 +20,9 @@ import audio.funkwhale.ffa.model.PlaylistTrack import audio.funkwhale.ffa.model.Track import audio.funkwhale.ffa.utils.Command import audio.funkwhale.ffa.utils.CommandBus -import audio.funkwhale.ffa.utils.maybeLoad +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.maybeNormalizeUrl import audio.funkwhale.ffa.utils.toast -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation import java.util.Collections @@ -72,8 +71,7 @@ class PlaylistTracksAdapter( override fun onBindViewHolder(holder: ViewHolder, position: Int) { val track = data[position] - Picasso.get() - .maybeLoad(maybeNormalizeUrl(track.track.album?.cover())) + CoverArt.withContext(layoutInflater.context, maybeNormalizeUrl(track.track.album?.cover())) .fit() .placeholder(R.drawable.cover) .transform(RoundedCornersTransformation(16, 0)) diff --git a/app/src/main/java/audio/funkwhale/ffa/adapters/PlaylistsAdapter.kt b/app/src/main/java/audio/funkwhale/ffa/adapters/PlaylistsAdapter.kt index a7d714a..4827f68 100644 --- a/app/src/main/java/audio/funkwhale/ffa/adapters/PlaylistsAdapter.kt +++ b/app/src/main/java/audio/funkwhale/ffa/adapters/PlaylistsAdapter.kt @@ -10,9 +10,8 @@ import audio.funkwhale.ffa.R import audio.funkwhale.ffa.databinding.RowPlaylistBinding import audio.funkwhale.ffa.fragments.FFAAdapter import audio.funkwhale.ffa.model.Playlist -import audio.funkwhale.ffa.utils.maybeLoad +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.toDurationString -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation class PlaylistsAdapter( @@ -80,8 +79,7 @@ class PlaylistsAdapter( else -> RoundedCornersTransformation.CornerType.TOP_LEFT } - Picasso.get() - .maybeLoad(url) + CoverArt.withContext(layoutInflater.context, url) .transform(RoundedCornersTransformation(32, 0, corner)) .into(imageView) } diff --git a/app/src/main/java/audio/funkwhale/ffa/adapters/SearchAdapter.kt b/app/src/main/java/audio/funkwhale/ffa/adapters/SearchAdapter.kt index 46ccf3d..7d8c987 100644 --- a/app/src/main/java/audio/funkwhale/ffa/adapters/SearchAdapter.kt +++ b/app/src/main/java/audio/funkwhale/ffa/adapters/SearchAdapter.kt @@ -20,11 +20,10 @@ import audio.funkwhale.ffa.model.Artist import audio.funkwhale.ffa.model.Track import audio.funkwhale.ffa.utils.Command import audio.funkwhale.ffa.utils.CommandBus -import audio.funkwhale.ffa.utils.maybeLoad +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.maybeNormalizeUrl import audio.funkwhale.ffa.utils.onApi import audio.funkwhale.ffa.utils.toast -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation class SearchAdapter( @@ -175,8 +174,7 @@ class SearchAdapter( else -> tracks[position] } - Picasso.get() - .maybeLoad(maybeNormalizeUrl(item.cover())) + CoverArt.withContext(layoutInflater.context, maybeNormalizeUrl(item.cover())) .fit() .transform(RoundedCornersTransformation(16, 0)) .into(rowTrackViewHolder?.cover) diff --git a/app/src/main/java/audio/funkwhale/ffa/adapters/TracksAdapter.kt b/app/src/main/java/audio/funkwhale/ffa/adapters/TracksAdapter.kt index a2213ea..fefd7a7 100644 --- a/app/src/main/java/audio/funkwhale/ffa/adapters/TracksAdapter.kt +++ b/app/src/main/java/audio/funkwhale/ffa/adapters/TracksAdapter.kt @@ -21,10 +21,9 @@ import audio.funkwhale.ffa.fragments.FFAAdapter import audio.funkwhale.ffa.model.Track import audio.funkwhale.ffa.utils.Command import audio.funkwhale.ffa.utils.CommandBus -import audio.funkwhale.ffa.utils.maybeLoad +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.maybeNormalizeUrl import audio.funkwhale.ffa.utils.toast -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation import java.util.Collections @@ -71,8 +70,7 @@ class TracksAdapter( override fun onBindViewHolder(holder: ViewHolder, position: Int) { val track = data[position] - Picasso.get() - .maybeLoad(maybeNormalizeUrl(track.album?.cover())) + CoverArt.withContext(layoutInflater.context, maybeNormalizeUrl(track.album?.cover())) .fit() .transform(RoundedCornersTransformation(8, 0)) .into(holder.cover) @@ -193,7 +191,6 @@ class TracksAdapter( false -> { data.subList(layoutPosition, data.size).plus(data.subList(0, layoutPosition)).apply { CommandBus.send(Command.ReplaceQueue(this)) - context.toast("All tracks were added to your queue") } } diff --git a/app/src/main/java/audio/funkwhale/ffa/fragments/AlbumsFragment.kt b/app/src/main/java/audio/funkwhale/ffa/fragments/AlbumsFragment.kt index 376a1dd..0574b74 100644 --- a/app/src/main/java/audio/funkwhale/ffa/fragments/AlbumsFragment.kt +++ b/app/src/main/java/audio/funkwhale/ffa/fragments/AlbumsFragment.kt @@ -28,11 +28,10 @@ import audio.funkwhale.ffa.repositories.Repository import audio.funkwhale.ffa.utils.AppContext import audio.funkwhale.ffa.utils.Command import audio.funkwhale.ffa.utils.CommandBus -import audio.funkwhale.ffa.utils.maybeLoad +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.maybeNormalizeUrl import audio.funkwhale.ffa.utils.onViewPager import com.preference.PowerPreference -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation import kotlinx.coroutines.Dispatchers.IO import kotlinx.coroutines.Dispatchers.Main @@ -144,8 +143,7 @@ class AlbumsFragment : FFAFragment() { super.onViewCreated(view, savedInstanceState) binding.cover.let { cover -> - Picasso.get() - .maybeLoad(maybeNormalizeUrl(artistArt)) + CoverArt.withContext(layoutInflater.context, maybeNormalizeUrl(artistArt)) .noFade() .fit() .centerCrop() diff --git a/app/src/main/java/audio/funkwhale/ffa/fragments/PlaylistTracksFragment.kt b/app/src/main/java/audio/funkwhale/ffa/fragments/PlaylistTracksFragment.kt index f3aa86f..61a6545 100644 --- a/app/src/main/java/audio/funkwhale/ffa/fragments/PlaylistTracksFragment.kt +++ b/app/src/main/java/audio/funkwhale/ffa/fragments/PlaylistTracksFragment.kt @@ -21,17 +21,15 @@ import audio.funkwhale.ffa.repositories.ManagementPlaylistsRepository import audio.funkwhale.ffa.repositories.PlaylistTracksRepository import audio.funkwhale.ffa.utils.Command import audio.funkwhale.ffa.utils.CommandBus +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.Request import audio.funkwhale.ffa.utils.RequestBus import audio.funkwhale.ffa.utils.Response -import audio.funkwhale.ffa.utils.maybeLoad import audio.funkwhale.ffa.utils.maybeNormalizeUrl import audio.funkwhale.ffa.utils.toast import audio.funkwhale.ffa.utils.wait -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation import kotlinx.coroutines.Dispatchers.Main -import kotlinx.coroutines.flow.collect import kotlinx.coroutines.launch class PlaylistTracksFragment : FFAFragment() { @@ -137,7 +135,6 @@ class PlaylistTracksFragment : FFAFragment binding.play.setOnClickListener { CommandBus.send(Command.ReplaceQueue(adapter.data.map { it.track }.shuffled())) - context.toast("All tracks were added to your queue") } @@ -191,8 +188,7 @@ class PlaylistTracksFragment : FFAFragment } lifecycleScope.launch(Main) { - Picasso.get() - .maybeLoad(maybeNormalizeUrl(url)) + CoverArt.withContext(layoutInflater.context, maybeNormalizeUrl(url)) .fit() .centerCrop() .transform(RoundedCornersTransformation(16, 0, corner)) diff --git a/app/src/main/java/audio/funkwhale/ffa/fragments/TracksFragment.kt b/app/src/main/java/audio/funkwhale/ffa/fragments/TracksFragment.kt index fe5a407..e13881d 100644 --- a/app/src/main/java/audio/funkwhale/ffa/fragments/TracksFragment.kt +++ b/app/src/main/java/audio/funkwhale/ffa/fragments/TracksFragment.kt @@ -22,24 +22,22 @@ import audio.funkwhale.ffa.repositories.FavoritesRepository import audio.funkwhale.ffa.repositories.TracksRepository import audio.funkwhale.ffa.utils.Command import audio.funkwhale.ffa.utils.CommandBus +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.Event import audio.funkwhale.ffa.utils.EventBus import audio.funkwhale.ffa.utils.Request import audio.funkwhale.ffa.utils.RequestBus import audio.funkwhale.ffa.utils.Response import audio.funkwhale.ffa.utils.getMetadata -import audio.funkwhale.ffa.utils.maybeLoad import audio.funkwhale.ffa.utils.maybeNormalizeUrl import audio.funkwhale.ffa.utils.toast import audio.funkwhale.ffa.utils.wait import com.google.android.exoplayer2.offline.Download import com.google.android.exoplayer2.offline.DownloadManager import com.preference.PowerPreference -import com.squareup.picasso.Picasso import jp.wasabeef.picasso.transformations.RoundedCornersTransformation import kotlinx.coroutines.Dispatchers.IO import kotlinx.coroutines.Dispatchers.Main -import kotlinx.coroutines.flow.collect import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.koin.java.KoinJavaComponent.inject @@ -146,8 +144,7 @@ class TracksFragment : FFAFragment() { override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) - Picasso.get() - .maybeLoad(maybeNormalizeUrl(albumCover)) + CoverArt.withContext(layoutInflater.context, maybeNormalizeUrl(albumCover)) .noFade() .fit() .centerCrop() @@ -194,7 +191,6 @@ class TracksFragment : FFAFragment() { "in_order" -> CommandBus.send(Command.ReplaceQueue(adapter.data)) else -> CommandBus.send(Command.ReplaceQueue(adapter.data.shuffled())) } - context.toast("All tracks were added to your queue") } diff --git a/app/src/main/java/audio/funkwhale/ffa/model/Artist.kt b/app/src/main/java/audio/funkwhale/ffa/model/Artist.kt index 7e779a7..ce76ca8 100644 --- a/app/src/main/java/audio/funkwhale/ffa/model/Artist.kt +++ b/app/src/main/java/audio/funkwhale/ffa/model/Artist.kt @@ -1,5 +1,8 @@ package audio.funkwhale.ffa.model +import java.util.Calendar.DAY_OF_YEAR +import java.util.GregorianCalendar + data class Artist( val id: Int, val name: String, @@ -10,7 +13,14 @@ data class Artist( val cover: Covers? ) - override fun cover(): String? = albums?.getOrNull(0)?.cover?.urls?.original + override fun cover(): String? = albums?.mapNotNull { it.cover?.urls?.original }?.let { covers -> + if (covers.isEmpty()) { + return@let null + } + // Inject a little whimsy: rotate through the album covers daily + val index = GregorianCalendar().get(DAY_OF_YEAR) % covers.size + covers.getOrNull(index) + } override fun title() = name override fun subtitle() = "Artist" } diff --git a/app/src/main/java/audio/funkwhale/ffa/playback/MediaControlsManager.kt b/app/src/main/java/audio/funkwhale/ffa/playback/MediaControlsManager.kt index 088864c..f28633b 100644 --- a/app/src/main/java/audio/funkwhale/ffa/playback/MediaControlsManager.kt +++ b/app/src/main/java/audio/funkwhale/ffa/playback/MediaControlsManager.kt @@ -15,9 +15,8 @@ import audio.funkwhale.ffa.R import audio.funkwhale.ffa.activities.MainActivity import audio.funkwhale.ffa.model.Track import audio.funkwhale.ffa.utils.AppContext -import audio.funkwhale.ffa.utils.maybeLoad +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.maybeNormalizeUrl -import com.squareup.picasso.Picasso import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers.Default import kotlinx.coroutines.launch @@ -69,7 +68,7 @@ class MediaControlsManager( .run { coverUrl?.let { try { - setLargeIcon(Picasso.get().maybeLoad(coverUrl).get()) + setLargeIcon(CoverArt.withContext(context, coverUrl).get()) } catch (_: Exception) { } diff --git a/app/src/main/java/audio/funkwhale/ffa/playback/PlayerService.kt b/app/src/main/java/audio/funkwhale/ffa/playback/PlayerService.kt index b97e5e8..81908aa 100644 --- a/app/src/main/java/audio/funkwhale/ffa/playback/PlayerService.kt +++ b/app/src/main/java/audio/funkwhale/ffa/playback/PlayerService.kt @@ -19,6 +19,7 @@ import audio.funkwhale.ffa.R import audio.funkwhale.ffa.model.Track import audio.funkwhale.ffa.utils.Command import audio.funkwhale.ffa.utils.CommandBus +import audio.funkwhale.ffa.utils.CoverArt import audio.funkwhale.ffa.utils.Event import audio.funkwhale.ffa.utils.EventBus import audio.funkwhale.ffa.utils.FFACache @@ -28,7 +29,6 @@ import audio.funkwhale.ffa.utils.Request import audio.funkwhale.ffa.utils.RequestBus import audio.funkwhale.ffa.utils.Response import audio.funkwhale.ffa.utils.log -import audio.funkwhale.ffa.utils.maybeLoad import audio.funkwhale.ffa.utils.maybeNormalizeUrl import audio.funkwhale.ffa.utils.onApi import com.google.android.exoplayer2.C @@ -37,7 +37,6 @@ import com.google.android.exoplayer2.PlaybackException import com.google.android.exoplayer2.Player import com.google.android.exoplayer2.Tracks import com.preference.PowerPreference -import com.squareup.picasso.Picasso import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers.IO import kotlinx.coroutines.Dispatchers.Main @@ -379,10 +378,10 @@ class PlayerService : Service() { runBlocking(IO) { this@apply.putBitmap( MediaMetadataCompat.METADATA_KEY_ALBUM_ART, - Picasso.get().maybeLoad(coverUrl).get() + CoverArt.withContext(this@PlayerService.applicationContext, coverUrl).get() ) } - } catch (e: Exception) { + } catch (_: Exception) { } }.build() } diff --git a/app/src/main/java/audio/funkwhale/ffa/utils/Bottleneck.kt b/app/src/main/java/audio/funkwhale/ffa/utils/Bottleneck.kt new file mode 100644 index 0000000..31d6f45 --- /dev/null +++ b/app/src/main/java/audio/funkwhale/ffa/utils/Bottleneck.kt @@ -0,0 +1,89 @@ +package audio.funkwhale.ffa.utils + +import java.lang.ref.WeakReference +import java.util.WeakHashMap +import java.util.concurrent.ConcurrentHashMap + +/** + * Similar to a Map, but with the semantic that operations single-thread on a per-key basis. + * That is: given concurrent accesses to keys "apple" and "banana", one "apple" thread + * will block all other "apple" threads, but not any "banana" threads. + * In practical terms, we use this to make sure we don't get weird edge cases when working + * with the filesystem cache. + */ +class Bottleneck { + // It would be nice to use LruCache here, but its behavior of + // replacing values doesn't get us the right results. + // As it is, this should be a trivial amount of memory compared to + // images and media. + // We single-thread this, so it doesn't need to be concurrent. + private val keys = WeakHashMap() + + // This one needs to be concurrent, as we don't want to single-thread it. + private val values = ConcurrentHashMap>() + + /** + * As you would expect from the Map function of the same name, except concurrent + * accesses to the same key will block on each other. If the first call succeeds, + * all other calls will fall through with the same result. (Unlike LRUCache.) + */ + fun getOrCompute(key: String, materialize: (key: String) -> T?): T? { + // First, get the lockable version of the key, no matter how + // many copies of the key exist. + // This map doesn't need to be a synchronized collection, because + // we single-thread access to it. (And there's no compute, so + // it should be low-contention.) + val sharedKey: String = canonical(key) + synchronized(sharedKey) { + val ref = values[sharedKey] + var value = ref?.get() + if (value == null) { + if (ref != null) { + values.remove(sharedKey) // empty ref + } + value = materialize(sharedKey) + if (value != null) { + values[sharedKey] = WeakReference(value) + } + } + return value + } + } + + /** + * The beating heart of this system: each key is is "upgraded" to + * the one which we use for locking. This does mean we block on + * access to `keys` for all concurrent access, but as it's so light- + * weight, this shouldn't be much of a problem in practical terms. + * The hope here is that this is slightly better than interning. + * In theory we could convert this over to also use WeakReference. + */ + private fun canonical(key: String): String { + val sharedKey: String + synchronized(keys) { + val maybeShared = keys[key] + if (maybeShared == null) { + keys[key] = key // first key of its value becomes canonical + sharedKey = key + } else { + sharedKey = maybeShared + } + } + return sharedKey + } + + /** + * Invalidate a key and run the supplied bi-consumer with the old value. + * Note that this will always run the supplied block, even if + * the value is not in the cache. + */ + fun remove(key: String, andDo: ((T?, String) -> Unit)?) { + val sharedKey = canonical(key) + synchronized(sharedKey) { + val oldValue = values.remove(sharedKey) + if (andDo != null) { + andDo(oldValue?.get(), sharedKey) + } + } + } +} diff --git a/app/src/main/java/audio/funkwhale/ffa/utils/CoverArt.kt b/app/src/main/java/audio/funkwhale/ffa/utils/CoverArt.kt new file mode 100644 index 0000000..3cd73d9 --- /dev/null +++ b/app/src/main/java/audio/funkwhale/ffa/utils/CoverArt.kt @@ -0,0 +1,260 @@ +package audio.funkwhale.ffa.utils + +import android.content.Context +import android.net.Uri +import android.util.Log +import audio.funkwhale.ffa.BuildConfig +import audio.funkwhale.ffa.R +import com.squareup.picasso.Downloader +import com.squareup.picasso.NetworkPolicy +import com.squareup.picasso.OkHttp3Downloader +import com.squareup.picasso.Picasso +import com.squareup.picasso.Picasso.LoadedFrom +import com.squareup.picasso.Request +import com.squareup.picasso.RequestCreator +import com.squareup.picasso.RequestHandler +import okhttp3.CacheControl +import okhttp3.HttpUrl +import okhttp3.OkHttpClient +import okio.Okio +import java.io.File +import java.security.MessageDigest + +/** + * Represent bytes as hex values. + */ +fun ByteArray.toHex(): String = joinToString("") { b -> "%02x".format(b) } + +/** + * Convert the string to its SHA-256 hash in hex format. + */ +fun String.sha256(): String = + let { MessageDigest.getInstance("SHA-256").digest(it.encodeToByteArray()).toHex() } + +/** + * Remove the query string and fragment from a URI. + * Mostly, this is to get rid of pre-signed URL silliness. + * If we ever need to keep some query params, we'll need a more robust approach. + */ +fun Uri.asStableKey(): String = buildUpon().clearQuery().fragment("").build().toString() + +/** + * Try to extract a file suffix from the URI. This isn't strictly + * necessary, but it can make debugging easier when you're going through + * the app cache with a filesystem browser. + */ +fun Uri.fileSuffix(): String = let { + val p = it.path + val ext = p?.substringAfterLast(".", "")?.lowercase() ?: "" + if (ext == "") ext else ".$ext" +} + +/** + * Wrapper around Picasso with some smarter caching of image files. + */ +open class CoverArt private constructor() { + companion object { + // For logging + val TAG: String = CoverArt::class.java.simpleName + + // This is just a nice-to-have for API admins + private const val userAgent = + "${BuildConfig.APPLICATION_ID} ${BuildConfig.VERSION_NAME} (${BuildConfig.VERSION_CODE})" + + // This client has the UA above, and has caching intentionally disabled. + // (Because we cache the images ourselves and cannot rely on replaying requests.) + private var httpClient: OkHttpClient? = null + + // Same: this has caching disabled. + private var downloader: OkHttp3Downloader? = null + + // Cache with some useful concurrency semantics. See its docs for details. + val fileCache = Bottleneck() + + /** + * We don't need to hang onto the Context, just the Path it gets us. + */ + fun cacheDirForContext(context: Context): File { + return context.applicationContext.cacheDir.resolve("covers") + } + + /** + * Shim for Picasso which acts like a NetworkRequestHandler, but is opinionated + * about how we want to use it. + */ + open class CoverNetworkRequestHandler(context: Context) : RequestHandler() { + /** + * Path to the actual cache directory. + */ + val coverCacheDir: File + + /** + * This goes out with every request and never changes. + */ + val noCacheControl: CacheControl = CacheControl.Builder() + .noCache() + .noStore() + .noTransform() + .build() + + init { + coverCacheDir = cacheDirForContext(context) + // Make the cache directory if it doesn't already exist. + if (!coverCacheDir.isDirectory) { + coverCacheDir.mkdir() + } + } + + /** + * The primary logic of going from a Request to a usable File. + * tl;dr: Use a local file if you can, otherwise download it and use that. + */ + private fun materializeFile(request: Request): (String) -> File? { + return fun(fileName: String): File? { + val existing = coverCacheDir.resolve(fileName) + if (existing.isFile) { + return existing + } + val key = request.stableKey ?: request.uri.asStableKey() + val httpUrl = HttpUrl.parse(request.uri.toString()) ?: return null + return fetchToFile(httpUrl, fileName, key) + } + } + + /** + * Required by Picasso, we only want to handle HTTP traffic. + */ + override fun canHandleRequest(data: Request?): Boolean { + return data != null && ("http" == data.uri.scheme || "https" == data.uri.scheme) + } + + /** + * Required by Picasso, this is the main entrypoint. + */ + override fun load(request: Request?, networkPolicy: Int): Result? { + if (request == null || !NetworkPolicy.shouldReadFromDiskCache(networkPolicy)) { + return null + } + // Ditch any query params. + val key = request.stableKey ?: request.uri.asStableKey() + // Convert to a short, stable filename. + val fileName = + key.sha256() + request.uri.fileSuffix() // file extension for easier forensics + // Actually find or fetch the file. + val file = fileCache.getOrCompute(fileName, materializeFile(request)) + // Hand it back to Picasso in a way it can understand. + return if (file == null) null else Result(Okio.source(file), LoadedFrom.DISK) + } + + /** + * The actual fetch logic is straightforward: download to a file. + * Sadly, this is more manual than you might expect. + */ + private fun fetchToFile(httpUrl: HttpUrl, fileName: String, cacheKey: String): File? { + val httpRequest = okhttp3.Request.Builder() + .get() + .url(httpUrl) + .cacheControl(noCacheControl) + .build() + val response = nonCachingDownloader().load(httpRequest) + if (!response.isSuccessful) { + return null + } + val body = response.body() ?: return null + val file = coverCacheDir.resolve(fileName) + if (BuildConfig.DEBUG) { + Log.d(TAG, "fetchToFile($cacheKey) <- $fileName <- NETWORK") + } + val bytesWritten: Long + body.use { b -> + Okio.buffer(Okio.sink(file)).use { sink -> + bytesWritten = sink.writeAll(b.source()) + } + } + return if (bytesWritten > 0) file else null + } + } + + /** + * Picasso can send back notification that files are busted. + * In those cases, it could be a transient problem, or credentials, etc. + * We probably don't want to trust the file, so we invalidate it + * from the memory cache and delete it from the filesystem. + * This uses Bottleneck, so it's thread-safe. + */ + fun invalidateIn(context: Context): (Picasso, Uri, Exception) -> Unit { + val coverCacheDir = cacheDirForContext(context) + return fun(_, uri: Uri, _) { + val key = uri.asStableKey() + val fileName = key.sha256() + uri.fileSuffix() + fileCache.remove(fileName) { f, _ -> + val file = f ?: coverCacheDir.resolve(fileName) + if (file.isFile) { + if (BuildConfig.DEBUG) { + Log.d(TAG, "Deleting failed cover: $file") + } + file.delete() + } + } + } + } + + /** + * Low-level Picasso wiring. + */ + private fun buildPicasso(context: Context) = Picasso.Builder(context) + // The bulk of the work happens here + .addRequestHandler(CoverNetworkRequestHandler(context)) + // Be careful with this. There's at least one place in Picasso where it + // doesn't null-check when logging, so it'll throw errors in places you + // wouldn't get them with logging turned off. /sigh + .loggingEnabled(false) // (BuildConfig.DEBUG) + // Occasionally, we may get transient HTTP issues, or bogus files. + // Listen for Picasso errors and invalidate those files + .listener(invalidateIn(context)) + .build() + + /** + * We don't want to cache the HTTP part of the flow, because: + * 1. It's double-caching, since we're saving the images already. + * 2. The URL may include pre-signed credentials, which expire, making the URL useless. + */ + protected fun nonCachingDownloader(): Downloader { + val downloader = this.downloader ?: OkHttp3Downloader(nonCachingHttpClient()) + if (this.downloader == null) { + this.downloader = downloader + } + return downloader + } + + /** + * Same here: build a non-caching version just for cover art. + */ + protected fun nonCachingHttpClient(): OkHttpClient { + val hc = httpClient ?: OkHttpClient.Builder() + .addInterceptor { chain -> + chain.proceed( + chain.request() + .newBuilder() + .addHeader("User-Agent", userAgent) + .build() + ) + } + .cache(null) // No cache here, intentionally + .build() + if (httpClient == null) { + httpClient = hc + } + return hc + } + + /** + * The primary entrypoint for the codebase. + */ + fun withContext(context: Context, url: String?): RequestCreator { + return buildPicasso(context) + .load(url) + .placeholder(R.drawable.cover) + } + } +} diff --git a/app/src/main/java/audio/funkwhale/ffa/utils/Extensions.kt b/app/src/main/java/audio/funkwhale/ffa/utils/Extensions.kt index 274a277..d649766 100644 --- a/app/src/main/java/audio/funkwhale/ffa/utils/Extensions.kt +++ b/app/src/main/java/audio/funkwhale/ffa/utils/Extensions.kt @@ -4,7 +4,6 @@ import android.content.Context import android.os.Build import android.util.Log import androidx.fragment.app.Fragment -import audio.funkwhale.ffa.R import audio.funkwhale.ffa.fragments.BrowseFragment import audio.funkwhale.ffa.model.DownloadInfo import audio.funkwhale.ffa.repositories.Repository @@ -12,8 +11,6 @@ import com.github.kittinunf.fuel.core.FuelError import com.github.kittinunf.fuel.core.Request import com.google.android.exoplayer2.offline.Download import com.google.gson.Gson -import com.squareup.picasso.Picasso -import com.squareup.picasso.RequestCreator import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers.Main @@ -59,13 +56,6 @@ fun Int.onApi(block: () -> T, elseBlock: (() -> U)) { } } -fun Picasso.maybeLoad(url: String?): RequestCreator { - return if (url == null) load(R.drawable.cover) - else load(url) - // Remote storage may have (pre-signed) ephemeral credentials in the query string - .stableKey(url.replace(Regex("\\?.*$"), "")) -} - fun Request.authorize(context: Context, oAuth: OAuth): Request { return runBlocking { this@authorize.apply { diff --git a/app/src/test/java/audio/funkwhale/ffa/utils/BottleneckTest.kt b/app/src/test/java/audio/funkwhale/ffa/utils/BottleneckTest.kt new file mode 100644 index 0000000..812421e --- /dev/null +++ b/app/src/test/java/audio/funkwhale/ffa/utils/BottleneckTest.kt @@ -0,0 +1,207 @@ +package audio.funkwhale.ffa.utils + +import org.junit.Test +import strikt.api.expectThat +import strikt.assertions.isEqualTo +import strikt.assertions.isFalse +import strikt.assertions.isNotSameInstanceAs +import strikt.assertions.isNull +import strikt.assertions.isSameInstanceAs +import strikt.assertions.isTrue +import java.util.concurrent.ArrayBlockingQueue +import java.util.concurrent.ConcurrentLinkedDeque +import java.util.concurrent.ThreadPoolExecutor +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean +import java.util.concurrent.atomic.AtomicInteger + +class BottleneckTest { + + @Test + fun `single threaded cache works like a cache`() { + var callCount = 0 + val cache = Bottleneck() + val materialize = { k: String -> + callCount++ + k.toInt() + } + val key = "34" + val keyCopy = String(key.encodeToByteArray().copyOf()) + expectThat(keyCopy).isEqualTo(key) + expectThat(keyCopy).isNotSameInstanceAs(key) + expectThat(callCount).isEqualTo(0) + val first = cache.getOrCompute(key, materialize) + expectThat(first).isEqualTo(34) + expectThat(callCount).isEqualTo(1) + val second = cache.getOrCompute(keyCopy, materialize) + expectThat(second).isEqualTo(34) + expectThat(second).isSameInstanceAs(first) + expectThat(callCount).isEqualTo(1) + } + + @Test + fun `multi-threaded cache only lets one through for each key at a time`() { + val maxThreads = 8 + val executor = ThreadPoolExecutor( + maxThreads, + maxThreads, + 5, + TimeUnit.SECONDS, + ArrayBlockingQueue(maxThreads) + ) + val running = AtomicBoolean(false) + val computeCount = AtomicInteger(0) + val key = "43" + val materialize = { k: String -> + expectThat(running.getAndSet(true)).isFalse() + expectThat(computeCount.incrementAndGet()).isEqualTo(1) + Thread.sleep(3000) + expectThat(running.getAndSet(false)).isTrue() + expectThat(computeCount.get()).isEqualTo(1) + k.toInt() + } + val cache = Bottleneck() + val threadCount = AtomicInteger(0) + for (c in 1..maxThreads) { + executor.execute { + Thread.currentThread().name = "test-thread-$c" + val keyCopy = String(key.encodeToByteArray().copyOf()) + expectThat(cache.getOrCompute(keyCopy, materialize)).isEqualTo(43) + threadCount.incrementAndGet() + } + } + executor.shutdown() + executor.awaitTermination(5, TimeUnit.SECONDS) + expectThat(threadCount.get()).isEqualTo(maxThreads) + } + + @Test + fun `single-threaded remove does what you would expect`() { + val cache = Bottleneck() + val materialize = { k: String -> k.toInt() } + val key = "24" + val first = cache.getOrCompute(key, materialize) + expectThat(first).isEqualTo(24) + var callCount = 0 + val keyCopy = String(key.encodeToByteArray().copyOf()) + expectThat(keyCopy).isEqualTo(key) + expectThat(keyCopy).isNotSameInstanceAs(key) + cache.remove(keyCopy) { value, k -> + expectThat(value).isSameInstanceAs(first) + expectThat(k).isSameInstanceAs(key) + callCount++ + } + expectThat(callCount).isEqualTo(1) + cache.remove(keyCopy) { value, k -> + expectThat(value).isNull() + expectThat(k).isSameInstanceAs(key) + callCount++ + } + expectThat(callCount).isEqualTo(2) + } + + @Test + fun `multi-threaded remove should synchronize and return correct results`() { + val maxThreads = 8 + val executor = ThreadPoolExecutor( + maxThreads, + maxThreads, + 5, + TimeUnit.SECONDS, + ArrayBlockingQueue(maxThreads) + ) + val running = AtomicBoolean(false) + val computeCount = AtomicInteger(0) + val key = "17" + val dematerialize: (Int?, String) -> Unit = { value: Int?, k: String -> + expectThat(running.getAndSet(true)).isFalse() + if (computeCount.incrementAndGet() == 1) { + expectThat(value).isEqualTo(17) + Thread.sleep(3000) + expectThat(computeCount.get()).isEqualTo(1) // no one else gets through until I'm done + } else { + expectThat(value).isNull() + } + expectThat(running.getAndSet(false)).isTrue() + k.toInt() + } + val cache = Bottleneck() + cache.getOrCompute(key) { k -> k.toInt() } + val threadCount = AtomicInteger(0) + for (c in 1..maxThreads) { + executor.execute { + Thread.currentThread().name = "test-thread-$c" + val keyCopy = String(key.encodeToByteArray().copyOf()) + cache.remove(keyCopy, dematerialize) + threadCount.incrementAndGet() + } + } + executor.shutdown() + executor.awaitTermination(5, TimeUnit.SECONDS) + expectThat(threadCount.get()).isEqualTo(maxThreads) + } + + @Test + fun `blocking happens on a per-key basis`() { + val cache = Bottleneck() + val maxThreads = 4 + val executor = ThreadPoolExecutor( + maxThreads, + maxThreads, + 5, + TimeUnit.SECONDS, + ArrayBlockingQueue(maxThreads) + ) + val running: Map = mapOf( + Pair("tortoise", AtomicBoolean(false)), + Pair("hare", AtomicBoolean(false)), + ) + val count: Map = mapOf( + Pair("tortoise", AtomicInteger(0)), + Pair("hare", AtomicInteger(0)), + ) + val race = ConcurrentLinkedDeque() + val threadCount = AtomicInteger(0) + for (key in arrayListOf("tortoise", "hare")) { + for (n in 1..2) { + executor.execute { + try { + cache.getOrCompute(String(key.encodeToByteArray().copyOf())) { k -> + val num = count[key]?.incrementAndGet() ?: -1 + Thread.currentThread().name = "$key-$num" + threadCount.incrementAndGet() + if (key == "hare") { + Thread.sleep(250) // give tortoise a chance to start + } + race.add("$key $num started") + expectThat(running[key]?.getAndSet(true)).isFalse() + if (num == 1) { + Thread.sleep(if (key == "tortoise") 3000 else 1000) + } + expectThat(running[key]?.getAndSet(false)).isTrue() + race.add("$key $num finished") + null + } + } catch (e: Throwable) { + race.add("Thread $key failed: ${e.message}") + } + } + } + } + executor.shutdown() + executor.awaitTermination(5, TimeUnit.SECONDS) + expectThat(threadCount.get()).isEqualTo(maxThreads) + expectThat(race.joinToString("\n")).isEqualTo( + """ + tortoise 1 started + hare 1 started + hare 1 finished + hare 2 started + hare 2 finished + tortoise 1 finished + tortoise 2 started + tortoise 2 finished + """.trimIndent() + ) + } +}