Add Android Auto Support with Improvements to Album Art Handling#890
Add Android Auto Support with Improvements to Album Art Handling#890banksio wants to merge 5 commits intoFoedusProgramme:betafrom
Conversation
…sed cache separation This refactor centralizes album art caching and loading logic into a new ArtCacheManager. Key changes include: - Rename GramophoneArtResolver to just ArtResolver - New ArtCacheManager for shared in-process and cross-process caching. - Added ArtCacheManager.clearCache() and integrated it into the ViewPagerFragment's quick_refresh action. This ensures that when a user performs a quick library refresh, the album art cache is also purged. - Updated the standard Refresh menu item in ViewPagerFragment to also clear the artwork cache, ensuring consistency with the Quick Refresh behavior. - Updated ArtResource hierarchy in ArtResolver for canonical resolution. - Simplified GramophoneAlbumArtProvider and Coil fetchers. - Improved MIME type preservation via .mime companion files. - Maximize reuse by defaulting songs to folder-based album art. - Implemented image resizing (1024x1024 high-res, 300x300 low-res) and cache key separation. - ArtResolver now resizes folder art and compresses to JPEG 85. - ArtCacheManager handles size-specific cache buckets (300 vs 1024). - GramophoneApplication Coil fetcher chooses cache size based on request dimensions. - Song resolution for 300px thumbnails now follows Folder -> MediaStore chain, skipping embedded art.
|
#842 also implements AA, no idea why that pr isn't showing up in the pull request screen though |
nift4
left a comment
There was a problem hiding this comment.
Hi, thanks for the PR! It's currently a busy time in my life, so I will need 2-3 weeks until I can review it in detail. Thanks for your understanding!
Until then, I'll do a surface-level review of these changes with some questions. Note that I haven't tried or looked into detail a lot yet, it's only from a quick look.
I surface the Albums, Artists, Songs and Playlists tabs (AA limits to 4 tabs).
You could have a "More" tab as fourth tab that offers the other tabs. Also, I think it would be nice if it would respect the tab order setting from Appearance settings, with which user can customize the most important tabs in the phone UI.
| ) | ||
| ) | ||
| .setSystemUiPlaybackResumptionOptIn(Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) | ||
| .setPeriodicPositionUpdateEnabled(false) |
There was a problem hiding this comment.
The media3 issue you linked about this said that they assume it's fixed in newer Android Auto app versions. Which version did you try? (Also, in such cases it's helpful to leave a one-line comment stating why this was done)
There was a problem hiding this comment.
I'm on 16.6.661444-release.
This one took me a good couple of days of debugging/testing. I am assuming that it either was fixed and regressed again, or was never fixed. I tested a few apps. YouTube Music, Spotify and Tidal don't seem to have the bug (though, they might be using the workaround...), but https://github.com/Ghost-Applications/gizz-tapes does have it (and does not use the workaround).
| internalPlayer = player | ||
|
|
||
| val sessionPlayer = object : androidx.media3.common.ForwardingPlayer(player) { | ||
|
|
||
| override fun getCurrentMediaItem(): MediaItem? { | ||
| return super.getCurrentMediaItem()?.let { convertItem(it) } | ||
| } | ||
| override fun getMediaItemAt(index: Int): MediaItem { | ||
| return convertItem(super.getMediaItemAt(index)) | ||
| } | ||
| override fun getMediaMetadata(): MediaMetadata { | ||
| return convertMetadata(super.getMediaMetadata()) | ||
| } | ||
| override fun getCurrentTimeline(): androidx.media3.common.Timeline { | ||
| val original = super.getCurrentTimeline() | ||
| return object : androidx.media3.exoplayer.source.ForwardingTimeline(original) { | ||
| override fun getWindow(windowIndex: Int, window: androidx.media3.common.Timeline.Window, defaultPositionProjectionUs: Long): androidx.media3.common.Timeline.Window { | ||
| super.getWindow(windowIndex, window, defaultPositionProjectionUs) | ||
| window.mediaItem = convertItem(window.mediaItem) | ||
| return window | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure if wrapping these media items at this level is the best solution. The gramophoneAlbumCover:// custom scheme thing doesn't only cause issues with Android Auto, there is also #577 (playback resumption), Bluetooth AVRCP and https://developer.android.com/media/optimize/mct with which you can see the issue. So my plan was to migrate the entire app's data structures away (see Reader.kt) from that custom scheme and instead add a Coil hook that bypasses the provider if we load the uri in-process
There was a problem hiding this comment.
I understand and I see where you are going with migrating off the custom schemes in Reader.kt. I think it would simplify the code a lot.
I'll play around with refactoring the code to achieve that migration and let you know how I get on.
| } | ||
|
|
||
| // Materialize | ||
| val artStream = ArtResolver.openResourceStream(context, resource, size) ?: return null |
There was a problem hiding this comment.
I disabled coil's disk cache because (if enhanced loading is disabled):
- MediaStore has its own disk cache, and it provides album covers with 50% of screen resolution as quality (so it's fine for every thumbnail use)
- The only time we need a higher-quality cover is in the "now playing" screen, and that is only needed for one song at a time, so we have basically no advantage of caching that. So we get that from ThumbnailUtils directly (which checks both embedded covers and .jpg files in the folder)
- If ThumbnailUtils can't find embedded cover and we don't have picture permission (-> can't see .jpg file), we have to ask MediaStore even in high quality case because there may be a .jpg file we can't see. It will just fail if there's no cover
Enhanced album cover setting then has two distinct functionality changes:
- Due to being granted picture permission, ThumbnailUtils can now find .jpg files automatically. Thus, we no longer need to ask MediaStore as fallback because we now have same set. This doesn't need to be cached because it's still only the high quality case
- We can also ourselves look if the album has a different cover than any of the songs (I don't remember why I added it to be honest, but I still remember someone gave me an album where every song had a different cover and it probably was a result of that). This one should probably be cached
The high quality version of the cover is provided to Android Auto using MediaSession metadata as Bitmap by media3, it doesn't have to be queried using the content provider. So the content provider can be a proxy to MediaStore except for the case where album cover is different from song cover. In this last case only it would need custom loading logic and caching.
As you can see there is only one edge case we need to cache, and I'm still wondering whether we could reuse the Coil implementation for that instead of doing it in a custom way.
(Also, I know, none of this is documented... I really should, but well...)
There was a problem hiding this comment.
Thanks for explaining. I didn't know the quality that MediaStore provided or that ThumbnailUtils will pull the folder .jpg.
Yep, it doesn't actually make sense to duplicate MediaStore's cache.
To explain why I did it this way; I was considering the case of the user storing a ridiculously large album art in a directory (or embedded in the audio file). Perhaps we should resize everything to a certain size (if it's above, say, 100% of the devices' screen resolution) even for the high quality art. And then cache that downsized art.
Gramophone could handle that resizing, or it could treat it as the user's problem with the mindset that they should not have such big album art. Both are valid, I'd say.
Anecdotally I have a 7MB cover in one of my albums. I noted that Wear OS fails to load the album art if Enhanced loading is on, likely due to that art being so large. Ideally this PR could set up the art infrastructure such that a future PR could fix that Wear OS support as well.
There was a problem hiding this comment.
And yes I'll revert the removal of the per-song album art as default since you mention it is on purpose. We could have a setting for it maybe, as it does reduce a lot of work on the CPU if the user doesn't require it?
There was a problem hiding this comment.
Perhaps we should resize everything to a certain size (if it's above, say, 100% of the devices' screen resolution) even for the high quality art. And then cache that downsized art.
The reason why I did not is actually something you found out 2 paragraphs later: it's not only displayed on the phone screen, it might also get synced to WearOS, Chromebook / Link to Windows, etc, and I don't know how good of a quality they need.
WearOS failing to display the cover because it's too big is quite stupid and wasn't what I would expect....
There was a problem hiding this comment.
We could have a setting for it maybe, as it does reduce a lot of work on the CPU if the user doesn't require it?
Do you use audio files with no embedded art + sidecar image file (this one can be detected/optimized to some extent)? Or do you use files that have embedded art individually, but they just have all the same file?
There was a problem hiding this comment.
I currently use files that all have the same embedded art, then I also put the sidecar cover.jpg (which is slightly higher quality) in the folder for redundancy.
There was a problem hiding this comment.
I don't think this is a great fit for a setting, as both consequences (wrong album art displayed, or high CPU usage) are very much not obvious. I would rather leave it has high CPU usage for now, and when Gramophone gets its own tag cache (see #881) it can be checked if multiple songs use the same cover as part of building that cache. Then the optimization will be invisible to the user and won't require toggling a setting.
| private val lifecycleScope: LifecycleCoroutineScope, | ||
| private val convertItem: (MediaItem) -> MediaItem, | ||
| private val delegate: SessionDelegate | ||
| ) : MediaLibrarySession.Callback { |
There was a problem hiding this comment.
I think your delegate pattern is a really good idea, but I'd switch that order (the service remains the implementation of the actual Callback class, and this one is called by the service for everything related to library loading)
|
Oh, also:
As I said above, this was on purpose, as I got sent an album where each song had a different cover in a bug report some time ago.
the content://media/audio/albumart/X uris on platform side actually don't enforce any permission, and that's because ecosystem support for that is just not there... I don't think we'll get away with any kind of permission checks while making apps like Musixmatch, Android Auto, Bluetooth AVRCP, last.fm scrobblers, etc work. And even if we do check permission, an attacker could always just use the platform version of the Uris and access them without any check, so it would be a bit pointless.
That's fine, the plan was indeed to first to a browser service as that is also used for Bluetooth AVRCP |
|
Hey @nift4, thanks for the detailed comments and explanations! This really helps me to understand your goals for the code. No worries on the delay, life is busy sometimes. I'd love to help out where I can. I'll address your comments and work on the code in the PR over the next couple of weeks.
Thanks again for the detail, I've learnt a lot from reading your replies! |
Hi! I use Gramophone and love it, so I wanted to try my hand at a PR to add support for Android Auto. I've tested this with the Desktop Head Unit and my own car, and it works great.
I've made the following changes to implement robust Android Auto support to Gramophone:
Implemented full MediaSessionService support to get Android Auto working. I surface the Albums, Artists, Songs and Playlists tabs (AA limits to 4 tabs). The queue behaviour is consistent between tapping an item in the mobile UI and the Android Auto UI.
Refactored the media library browsing logic into a separate GramophoneLibrarySessionCallback class, separating the UI/tree-building logic from the main playback service.
Added
setPeriodicPositionUpdateEnabled(false). This is a workaround for a known Media3 bug (Can't Scroll Content in Android Auto with MediaBrowserService androidx/media#2192) and doesn't negatively impact the player or UI in any meaningful way from my testing.Added a new GramophoneAlbumArtProvider that serves artwork over a standard
content://URI, so that Android Auto can read it. Based on dab7607. This provider is limited to Android Auto only for now via thecom.google.android.finsky.permission.GEARHEAD_SERVICEpermission, for privacy.Given that work was required to get album art working properly on Android auto, I also took the opportunity to improve the artwork infrastructure for the entire app:
Centralised Artwork Resolution and Caching: Created
ArtCacheManagerandArtResolverto handle artwork discovery, resizing, and materialisation. The cache is capped at 50MB. The cache will be cleared automatically when a user presses Refresh or Quick refresh in the main context menu.Reduce Duplication of Work: If Enhanced album cover loading is on, the album art file found in the filesystem will be used for all songs in an album by default, rather than loading and caching the art from each song.
Shared Efficiency: Updated the Coil
ImageLoaderto use this new system. If the user has browsed the artwork locally, then Android Auto just pulls from the cache rather than re-extracting.I realise this is quite a big PR, but I kept the changes together since Android Auto benefits from the new artwork infrastructure. If you wanted, I could probably split it up.
Also note that I did not go down the template route (#593), as they are in beta and currently not able to be published publicly to the Play Store. Therefore it is much easier to use the current MediaLibrarySession for now. Furthermore, Gramophone doesn't require the use of any custom flows for Android Auto as it is a fairly simple media player.
Closes #93.
Thanks for the app, and keen to hear your thoughts on my PR and if any changes are recommended! I can provide screenshots if requested also.
Please feel free to test in your own vehicle or with the Desktop Head Unit.