Fix/some albums not being added

[TwelveP]

Hi, I was running and reviewing ampd in an attempt to fully understand its inner workings.

I noticed some albums would be missing from the webclient when I tried finding them in the "Albums" tab once the album cache had been filled. I did not go into great efforts to validate the exact "problematic" albums (as these seemed to be random), but I did put some logs on the method where it caches them, and found that these albums were skipped for allegedly missing artist tags as read from javampd abstractions.
I took a look at my local files and checked other MPD clients (namely ncmpcpp and cantata) and they all seemed to recognize those missing albums and their artist tags just fine.

So I figured there might be an issue in how javampd is loading them.
And as I navigated through the docs, I found they do slightly warn that under very large collections, the listAllAlbums() method, the one used in ampd, may at least be slow.
I also browsed the mpd docs for a bit and found another interesting bit; the unix socket that javampd reads from, sends data in an unstructured way, that is, a straight list of "key:value" tag pairs. From the way javampd queries the data for "all albums", it'd be easy to mistake which piece of information belongs on which individual album. My guess remains, that javampd might be the one at fault when this specific querying method is used.

But at least I did manage to fix the issue in my machine with the commits provided in this PR: no longer do I have missing albums when I look them up within the ampd webclient.

For the sake of context: all my collection is stored in, plus I'm compiling and running all this software on top of; a single Linux 6.8-based desktop machine with mpd v0.24.6. No Docker, no computer interconnectivity, just the most straightforward deployment to localhost for now.

Please let me know if I can do anything to improve this PR.
PS. I'm done editing this comment, sorry for that lool.

[rain0r]

Hi, I was running and reviewing ampd in an attempt to fully understand its inner workings.

👍

I noticed some albums would be missing from the webclient when I tried finding them in the "Albums" tab once the album cache had been filled. I did not go into great efforts to validate the exact "problematic" albums (as these seemed to be random), but I did put some logs on the method where it caches them, and found that these albums were skipped for allegedly missing artist tags as read from javampd abstractions. I took a look at my local files and checked other MPD clients (namely ncmpcpp and cantata) and they all seemed to recognize those missing albums and their artist tags just fine.

This is something I noticed too. At some point I did try to figure out why some albums were missing, respectively why javampd behaves differently like, for example, ncmpcpp. But diving into javampd is time-consuming and you quickly get to the level of debugging raw network sockets which can be cumbersome.

At the end of the day, ampd is dependent on javampd, its features and its quality.

If you would like to see an example of ampd communicating directly with the MPD server, take a look at EmbeddedCoverService.java

So I figured there might be an issue in how javampd is loading them. And as I navigated through the docs, I found they do slightly warn that under very large collections, the listAllAlbums() method, the one used in ampd, may at least be slow. I also browsed the mpd docs for a bit and found another interesting bit; the unix socket that javampd reads from, sends data in an unstructured way, that is, a straight list of "key:value" tag pairs. From the way javampd queries the data for "all albums", it'd be easy to mistake which piece of information belongs on which individual album. My guess remains, that javampd might be the one at fault when this specific querying method is used.

Yes, that's also my feeling. But I couldn't figure out why, yet. I don't think it's because of listAllAlbums() works slow on large collections. My guess would be it has something to to with tags, but that's just a gut feeling.

But at least I did manage to fix the issue in my machine with the commits provided in this PR: no longer do I have missing albums when I look them up within the ampd webclient.

I tried your branch and you are right: albums which are missing on the master-branch are now in the list.
My only concern is, that the method is now firing a lot more queries than before, which leads to a longer method runtime:

master branch:
Filling albumCache took 21261278500 nanoseconds (21.26 seconds)

TwelveP:fix/some-albums-not-being-added:
Filling albumCache took 65973322041 nanoseconds (65.97 seconds)

But maybe that's the price to pay if you don't want albums missing.

For the sake of context: all my collection is stored in, plus I'm compiling and running all this software on top of; a single Linux 6.8-based desktop machine with mpd v0.24.6. No Docker, no computer interconnectivity, just the most straightforward deployment to localhost for now.

Please let me know if I can do anything to improve this PR. PS. I'm done editing this comment, sorry for that lool.

Sure, there two things:

1. I think the call to peek() is unnecessary for production
2. Please squash your PR into a single commit.

And again, thanks for your work!

[TwelveP]

Sorry for the delay @rain0r, I squashed the changes, removed that call to .peek(), rewrote the commit message to make it more clear.
I also rebased to skip the merge commit :)

[TwelveP]

Actually you know what, I think I understand the problem from javampd's part. The albums that were missing do have something in common: each one of them exist next to another album in the same albumartist/genre/date/artist group (such is the MPD protocol command that the method queries by). In other words, for every album that exists within one group of an albumartist/genre/date/artist, there may be X albums within the same group, and those will go missing.
I have two prime examples of this

1. I own two Black Sabbath albums from 1970: their s/t debut "Black Sabbath", and "Paranoid". From these two albums, javampd always fails to include artist tags specifically for "Paranoid" which is the second in alphabetic order.
2. I own two Faith no More albums, "Angel Dust" and "King for a Day [...]" that I have tagged as if they released in 1999 because I tag with MusicBrainz and these are reissues. So, from these two albums, the artist tags for "King for a Day [...]", the second of both albums in alphabetic order, are always missing.

I'm going to try creating a patch there and see how that goes, perhaps this PR won't be needed, but if it is, I will reopen it.

[rain0r]

Hey there,

thanks for your effort.

I would merge your PR happily, since I'm not sure how active the development of javampd is.

If your PR to javampd gets merged in the future, we can just revert the change.

But in the meantime, it would be nice to have all albums in the albums-view.

[TwelveP]

@rainor I managed, here it is if you wanna take a look as well finnyb/javampd#258