-
-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Per-song album covers #342
Comments
Very good observation. This is largely a consequence of Android's cover art source doing this kind of dumb grouping by album, and me retaining the behavior since I usually flatten the cover art of an album. I think the settings approach is a good idea, albeit it could also become an intrinstic behavior to the "Quality" Album Covers mode. My primary issue as it stands is figuring out how to still leveraging in-memory bitmap caching functionality, since I'll no longer be able to depend on the album to cache a single bitmap (Perhaps I'll use a SHA-256 hash?). |
Of course it falls back to Google's design in the end, haha. It seems that this is a common trend for Android audio players. Unfortunately I don't think I'd be of much help to suggest technical solutions. I was hoping it would be as simple as referencing the current song's actual tags instead of utilizing caching and relying on Android functionality, but I guess that kind of defeats the purpose of, "library loading" in the first place. If you were to hash to compare images, perhaps a more performative hashing strategy could be used? It's not like using something which is considered, "insecure" these days (eg MD5) is a real problem for this use case. Alternatively, or in addition to the above, perhaps only hashing a small portion of the image could result in higher performance? Or better yet, if there's a way to measure file sizes of the image stored in tags, you could first check to see if the file sizes match, and only compare a hash if they have identical file sizes (which should be an edge case in the first place, I would strongly presume). This isn't exactly my expertise, so apologies if these aren't useful suggestions. I'm trying to help where I can, though I recognize it may not be the most useful. |
Yep. Android's media framework (and filesystem API writ large) is notorious for being borderline unusable. I've been progressively trying to expunge as much reliance on the bugged system code as possible (Read: #322, #128) as time goes on.
I mean, that's what the "Quality" album cover mode does. It's just slower and uses more memory. It's generally much faster if you leverage a link to an image rather than parsing image data out. I want start using this everywhere (with my own independent image code) with #327, but the specifics are not exactly well-defined on how this works. As for the caching bit, Auxio uses a library that keeps loaded album covers in memory for a little while so I don't have to keep re-loading them from songs or the system. This is super nice, but it relies on a "unique id" for loaded album covers, something that would have to be switched from the album ID to something a bit more unique to prevent sorting multiple versions of the same album cover.
SHA-256 tends to be hardware-accelerated these days, making it often as cheap as MD5 AFAIK. I'm already using it for music identification and the performance loss is largely minimal.
Maybe. I'll have to do some performance analysis on which strategy is the best. Figuring this out is actually quite important, as it will be what backs #327. |
Actually, no, this is blocked by #327. I realized that my alternative cache ID strategy is dependent on extracting the cover art for each song, more or less negating the point of it. The only way I can do this efficiently is if I had a "birds-eye-view" of all the cover art, which only occurs when loading music, thus requiring #327. |
Hey @tearfulnumpty, I wanted to ask a few things about this change.
|
Sorry for the delayed response.
|
It would probably be very memory intensive, hence it would be a setting with a heap of warnings surrounding it.
I'm referring to albums in this case. Individual songs will display their individual cover art, but albums would have to pick some kind of cover art from the songs it contains. I was asking if there was any preferred strategy for picking a cover art to use in this case. |
I believe my phone has 4 GiB of memory, and I have no idea how it'd handle a massive memory challenge. If it was easy for you to implement, I'd be willing to try it for the meantime. I suppose with my low-mid tier phone, it may not work out for me until the proper fix with #327 comes out. In regard to the second point, thanks for the clarification! I don't have any massive preference of any specific strategy or algorithm for picking this, no. I suppose perhaps the first track method would make the most sense to me personally. |
Okay, if that memory limit's the case, I'll not go for it then. Note that it will be a very long time before you see this (1 or 2 years), since #327 is probably the most distant architectural rework I'm considering.
Cool, I'll do that. Thanks for the clarification. |
Dang, I really hate to hear that. I totally get it's not a simple fix, but it's very frustrating all the same. Is there any way to implement something similar to your high memory usage temporary fix solution using storage space instead of memory? Something which could be done during the, "scanning music" phase? I do have a very large microSD card installed which I throw all of my media on, and I wouldn't mind Auxio taking up some Gigs of it if that would be useful, or some other type of similar caching. I really appreciate Auxio being around, since it seems like almost all other music players have some pretty serious drawbacks and highly questionable decision making going on. Thanks for what you do. |
What you're describing is #327 @tearfulnumpty, save the grouping by hash. What makes this hard is the logistics of collating and storing cover art itself in an efficient way. While this is a fun problem, given the somewhat minor scope of this issue, I don't want to get distracted working on this at the moment, given that playlists are still only 3/4th done right now and and the app's background work system needs to be massively reworked. I mean, getting nerd-sniped by #128 pushed playlists back by a year, and I don't want to let that happen with other in-demand functionality again. |
i dont know if this helps but https://github.com/MuntashirAkon/Metro doesnt have this issue |
another difference is that while auxio assumes all files with the same album name tag are the same metro treats each file the songs are stored in as its own environment / scope. if i have two songs under the album tag 'beer fish' but stored in different files, metro correctly places them as different albums both called 'beer fish' while auxio assumes theyre the same album |
this is coded in kotlin too btw |
Because Metro uses JAudioTagger and
That assumes a well-formed folder structure. I do not do this since folder structures are significantly more subjective than tags, and so such heuristics break down. For example, another person that organizes by genre would suddenly have all their albums split.
Using the same language doesn't necessarily imply that design choices are transferable. For example, Auxio's lighter feature set and complex music model differs significantly from Retro's/Metro's rich feature set and simple music model. |
sounds like it should be a setting in auxio then ngl |
what is a 'music model' |
Not really. I don't like extensively customizing exactly how Auxio interprets music libraries unless there really is no unambiguous standard that all libraries rally around (i.e multi-value separators). Here, nearly everyone facing the problem of albums being accidentally merged used MusicBrainz IDs, and basically never by folder characteristics. I would much rather implement something like this as a heuristic in the music loader itself, but since it harms users with different library configurations, I can't really do it.
It references to how a music player interpretation of a music library and how it should be organized into albums, artists, genres. |
so if two albums had the same name then what |
Updates: So, symphony just did this: I thought this was not possible, as android indexed cover art by album instead of song. If this actually isn't the case, then suddenly this is a lot more feasible, as I don't need to rely on the high-quality cover fetcher. Memory issues still apply though.
They would have different MusicBrainz IDs and Auxio would separate them out. |
@OxygenCobalt I would also love this feature. For example, Everywhere At The End Of Time by The Caretaker has six different album covers, and they change as the album progresses. It's also possible that some albums have different covers for every single track, like compilation albums. Also, there's the issue with singles sharing the same random cover sometimes, as OP mentioned. What is the current status on this issue? Also, I really love your music player, it's by far the best Android music player I've used, and I've tried MANY over the years. What's the best way to support you/this project? |
Hey, I'm really happy to see that this is implemented! However, I've noticed a few bugs while testing the Auxio_PerSong.zip debug apk. I'm on Android 13. My library has a cover.png file for albums with only one cover and embedded covers on albums with multiple covers. The cover.png for albums with only one cover has worked perfectly so far (v3.4.3), although I know that this is not yet supported by the app itself (#104). Albums with multiple (embedded) covers: each songs correctly displays its cover, but the media notification uses the same artwork for all songs on an album. For albums with only one cover (cover.png), the behavior is quite strange: album (albums view) and song (now playing) covers are sometimes different and can both be incorrect. The app also displays the same album art for all albums. The media notification however seems to always display the correct artwork. I'd be willing to send screen recordings or sample files is you can't reproduce this issue @OxygenCobalt. |
I can reproduce basically none of those behaviors @TL-P, I need screenshots and sample files. Just trim the audio if it's copyrighted. Include the cover.png files. |
Thanks @OxygenCobalt for your efforts towards solving this! I also tested the Auxio_PerSong.zip build with my own music collection (located on an SD card in a Fairphone 3, running LineageOS 20, Android 13). Results (cover.jpg / embedded art):
So in conclusion with my own collection: Files with embedded album art now correctly show their art everywhere. Files without embedded album art show incorrect album art in small thumbnails like the list views or folded bottom sheet, but show correct album art (cover.jpg) in album view, lockscreen and expanded "now playing" bottom sheet. Reproduction:To reproduce the behavior, i have created two zip files, they contain two CC-BY songs. "Nangdo_issue.zip" contain two songs with no embedded cover art. "Nangdo_fixed.zip" contains the same two songs, but with cover art embedded. I also appended "_fixed" and "_issue" to artist and album tags. In the Auxio build, the "Nangdo_issue.zip" takes on a random album art in the artist list view, while "Nangdo_fixed" correctly displays the albums art. screnshot Nangdo_fixed.zip |
Wait. I think I get it. I'm not properly keying album art when there is nothing embedded in the file that I can obtain a perceptual hash from. To fix that I need to fall back to UID, which will be quite inefficient @bastianilso. However, since most people use embedded album art (At least, I hope so...) this should be okay. You'll probably face increased memory usage until I can do #104. |
Updated build with that embedded cover art fix (I think). Don't have the time to test right now, but you can try it out @bastianilso @TL-P |
@OxygenCobalt Almost fixed it! All album artwork now correctly show for the respective album. But a side-effect of that fix is that albums without embedded art show a 2x2 grid of the same album artwork. Edit: and regarding inefficiency, yea fine for most use cases i think? Personally, my music library strive to have individual artwork embedded, so I wont mind. I mostly have |
Oh, I should have seen that coming. I'm just going to assume that if I could not find a song's cover, I should fall back to the album cover.jpg URI instead, given that this only occurs when using cover.jpg or with possibly unsupported metadata formats (not likely). Will do that. |
Thanks @OxygenCobalt for the quick fix and @bastianilso for saving me some time :) After testing Auxio_PerSong2.zip for a little while, I can confirm that everything is working for albums using cover.png, except for the issue that @bastianilso mentioned:
However, this issue that (I assume only) I've encountered has not been resolved:
This is a screen recording of what I mean: screen-20240422-203858.mp4Not sure why this is happening. Could this be OS specific? (I'm running PixelOS, Android 13.) |
That's giving me memories of a really bad notification desync issue I had to debug awhile back. It's more likely to be a consequence of #37 rather than this. Let me check the sample files. |
Okay, I've replaced the audio with 5 seconds of silence and included tracks 1-3 in the folder. |
New build that should hopefully have fixed non-embedded album art: Auxio_PerSong3.zip (Still no time to test) @bastianilso Also: That issue is indeed device-specific @TL-P. I'm just going to have to ram my head and try random things until Media3 gets the memo. Try this: Auxio_PerSongVariant.zip |
@OxygenCobalt ✅ did a quick test and it all looks good to me now. Might not be worth anything, but in artist view I'm not sure how Auxio decides whether a 2x2 grid is employed as thumbnail or a single album art piece is employed as thumbnail for different artists. But personally having consistency there doesnt really matter much to me. |
It depends on the amount of covers available. If there are more than 4 distinct covers, a mosaic is shown. Otherwise, just one is shown. I've been wanting to change this a little actually, but haven't decided on what. |
Unfortunately, the issue is still present. The behavior changed a bit though (the output switcher now also uses the same incorrect cover art as the media notification): screen-20240423-135405.mp4 |
Weird @TL-P. This implies either:
Will need to debug both. |
Okay @TL-P, I've clobbered the notification code so it's all being managed by Media3 and doesn't have odd extensions. If this doesn't work, it's Media3's fault and I have to figure out how to reproduce it with their demo app. Otherwise, it's my fault and I have no idea why. |
Well, this is interesting. Auxio_PerSongVariant2.zip behaves the same as Auxio_PerSongVariant.zip. |
Okay, this might help: I've tested the samples with VLC for Android and symphony and both work! |
So excited for this! Thank you very much for seeing this through, it looks like it was certainly no easy task, but I really appreciate it! |
Sorry for taking so long @TL-P, need to do school stuff. Think I figured out the issue though. |
Good to hear! Honestly, I'm just happy you're doing all the work in your free time @OxygenCobalt :) |
Can confirm the cover issue is fixed now! Side note: I noticed the updated UI in this build, looks nice. However, round mode seems to be enabled even when the setting is turned off? |
Yeah @TL-P I'm still working on the new UI, I just accidentally worked on this patch in that branch. It's still under construction (I haven't planned out how the UI will change when round mode is off) |
Just one question: Can you explain what you mean by "the notification can no longer respond to music settings"? Thanks! |
@TL-P basically, if you change Album covers or turn on 1:1 album covers, the notification cannot respond to it. It'll always be non-1:1 and possibly not be high quality. |
Looks like due to #802 I'm going to have to modify the image keyer a little. A perceptual hash will be so horribly inefficient that it will make libraries with large cover arts unusable. I think I'm just going to randomly pick a string of bytes in the middle of a cover data stream. This means if you use different images for your cover art they will end up being duplicated in memory, but until #327 (which itself needs to be retooled aggressively) I can't risk tanking music loading performance even more. |
@OxygenCobalt I'm a bit confused, so is this done or no? Because it says closed, but you're also saying you need to do something else first. |
This is shipped in 3.5.0 @dmint789 but in a less-than-ideal state. More or less:
Hence, I need to make the system more robust, but these edge cases are rare enough for me to push it back. |
Thank you, I tried it out and it works great! |
Description
Let's assume we have song A, and song B. Both have the same artist tags, and the same Album tags. However, their art tag is different.
Currently, Auxio's behavior seems to show that if the artist and album tags match between two given songs, there is an assumption that the two art tags for said songs are also matching. Thus, in the instance where the art is NOT identical between the two songs, as is the case for the song A and B example, an incorrect image is likely to be displayed when the song is playing. Of course, the likelihood of this changes based on the number of songs with matching album and artist tags. If there's only two matching, then only one song will be displaying the wrong art (as the other would be displaying it's own). If there's 100+ matching as is the case for me with just one pair, it's a big issue.
In addition, some of my songs without art tags will end up with an art tag displayed which is from another song altogether. This is a minor annoyance to myself and I can live with this part, but I assume it's related to the rest of the above stated behavior so I wanted to mention it as well.
I realize that this functionality may be for performance reasons, and it may be more desirable for some users to function this way. However, I run into this problem on a regular basis, and I can't imagine that I'm the only person in this situation who would desire accuracy of tags over the speed of a rare/uncommon library reload occurrence.
So, my proposal would simply be an additional setting, similar to the existing, "Settings -> Content -> Images -> Album Covers" settings. The way I see it, either an additional option could be provided for the above stated setting, or a simple on/off checkbox could be added nearby under the, "Content" grouping of settings to not automatically assume all tracks within an album by the same artist have the same cover art.
Problem solved
It's frustrating to see the incorrect art for a track, and I run into this problem on a very regular basis as it impacts a rather large amount of my library. It makes me wish I hadn't tagged my songs by artist in the first place sometimes.
Other implementations
While multiple strategies exist (performance vs quality and accuracy), other players that I have used in the past use the tags existing within the given song, and don't make assumptions like this - including mobile players.
Benefit
This would benefit Auxio users by allowing a simple option to prioritize performance in the one time library load, versus prioritizing quality and accuracy of the track's art. Most users aren't reloading their libraries frequently enough to see massive performance impacts, and those who do will of course always have the existing setup and options - no harm done with a (presumably) relatively simple addition of a quality of life option.
Duplicates
The text was updated successfully, but these errors were encountered: