Skip to content
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

Same-named albums from different artists are confused with each other #330

Closed
untitaker opened this issue Dec 3, 2013 · 49 comments
Closed

Comments

@untitaker
Copy link

When having the albums:

Aphex Twin -- Classics
Ratatat -- Classics

Going to the album listing for Aphex Twin and doing "Add, Replace and Play" for his Classics album will also add titles from Ratatat's album. Similarly, albumart from Ratatat's album is shown on titles from Aphex Twin (using a local webserver for albumart)

@untitaker
Copy link
Author

I think the issue with the albumart is related to its cache. Removing Ratatat's songs from the playlist (leaving only Aphex Twin), clearing the albumart cache and replacing the playlist with Ratatat's songs now shows Aphex Twin's cover on the Ratatat songs.

@untitaker
Copy link
Author

Looking into https://github.com/abarisain/dmix/blob/master/JMPDComm/src/org/a0z/mpd/AlbumInfo.java#L90, i probably should mention that i have the "Album Artist" option enabled in MPDroid, and have organized my music accordingly. However, the normal artist field for a song equals the album artist field in this case, and most others.

@jcnoir
Copy link
Collaborator

jcnoir commented Dec 3, 2013

Which version do you use ?

@untitaker
Copy link
Author

Server: 0.18.0
Android app: 1.06 beta 2

@jcnoir
Copy link
Collaborator

jcnoir commented Dec 3, 2013

I think this issue has already been fixed in the current git version.
If you want to check it you can install the last nightly from here : http://anpmech.com:8080/mpdroid-git-builds/?C=M;O=D
But you'll have to uninstall your MPDroid version first and clear the cover cache.

@hurzl
Copy link
Contributor

hurzl commented Dec 3, 2013

It depends whether you come from artist -> album or via the album list.
In the latter case the artist is not known, so it takes all tracks

@untitaker
Copy link
Author

The latest build doesn't seem to show any album art in the global album listing. The original issue seems to be fixed in that build though. Thanks!

@abarisain
Copy link
Owner

The latest build doesn't seem to show any album art in the global album listing

That is now by design. The global album listing will be made a list again. We can't know whose artist the album is (especially for albums like "Classics"), so we get horribly wrong covers. In that case, it's better not to try to show anything at all, and keep the covers elsewhere, where we can get more reliable data.

I hope you understand that decision !

@ranperry
Copy link

ranperry commented Dec 4, 2013

What about those that use the local cover option via HTTP?

On Tue, Dec 3, 2013 at 6:44 PM, Arnaud Barisain Monrose <
notifications@github.com> wrote:

The latest build doesn't seem to show any album art in the global album
listing

That is now by design. The global album listing will be made a list again.
We can't know whose artist the album is (especially for albums like
"Classics"), so we get horribly wrong covers. In that case, it's better not
to try to show anything at all, and keep the covers elsewhere, where we can
get more reliable data.

I hope you understand that decision !


Reply to this email directly or view it on GitHubhttps://github.com//issues/330#issuecomment-29763922
.

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

You don't know the directory either ...

@untitaker
Copy link
Author

You could build the albumlist by iterating through all artists and then all their albums. That way, you would be able to identify all album's authors and show more than one Classics album.

Arnaud Barisain Monrose notifications@github.com wrote:

The latest build doesn't seem to show any album art in the global
album listing

That is now by design. The global album listing will be made a list
again. We can't know whose artist the album is (especially for albums
like "Classics"), so we get horribly wrong covers. In that case, it's
better not to try to show anything at all, and keep the covers
elsewhere, where we can get more reliable data.

I hope you understand that decision !


Reply to this email directly or view it on GitHub:
#330 (comment)

@abarisain
Copy link
Owner

Of course, but that would be too slow. I have over 800 artists, making a request for each one of them would take too long !

Le 4 déc. 2013 à 06:42, Markus Unterwaditzer notifications@github.com a écrit :

You could build the albumlist by iterating through all artists and then all their albums. That way, you would be able to identify all album's authors and show more than one Classics album.

Arnaud Barisain Monrose notifications@github.com wrote:

The latest build doesn't seem to show any album art in the global
album listing

That is now by design. The global album listing will be made a list
again. We can't know whose artist the album is (especially for albums
like "Classics"), so we get horribly wrong covers. In that case, it's
better not to try to show anything at all, and keep the covers
elsewhere, where we can get more reliable data.

I hope you understand that decision !


Reply to this email directly or view it on GitHub:
#330 (comment)

Reply to this email directly or view it on GitHub.

@untitaker
Copy link
Author

maybe this is an issue that should be fixed in the server itself, adding new features to the API.

@ZenithDK
Copy link

ZenithDK commented Dec 4, 2013

@untitaker You're talking about MPD (the mpd protocol) - that is completely out of scope for MPDroid - until such a feature is implemented - if ever.

@abarisain
Copy link
Owner

Yeaaaaah they believe that it's the client job. I believe that it's theirs. They are very conservative with the API, so ... never gonna happen.

EDIT : Just to be clear, I don't have anything against the MPD team. Before submitting anything upstream, we should discuss a lot between us, and exlpain clearly why a server change is needed, and why other clients never needed anything like that (or were able to work around it). MPDroid would also have to keep a fallback behaviour for old MPD servers (Old ubuntus/debians, BSD and friends lag a lot behind the newest versions)
So interpret my hot-blooded "never gonna happen" as "not anytime soon" :)

I reverted the global album list to ... a list, with no cover art. Until we can get a better global album list, this will not be improved.

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

One could try how long it takes fetching all albums of all (album)artists with a batch ...

@abarisain
Copy link
Owner

And compare it with a batchless command. Great idea

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

later ...

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

I will make MPDCommand a real object that can be passed and queued

@abarisain
Copy link
Owner

Do you mind making this in another pr please ? I'm really interested in merging that change :)

Le 4 déc. 2013 à 15:43, hurzl notifications@github.com a écrit :

I will make MPDCommand a real object that can be passed and queued


Reply to this email directly or view it on GitHub.

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

ok I'll try ...

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

but you have disabled covers now?

@abarisain
Copy link
Owner

Only in the global album list where we lack information. It still works in the albums for artist list and everywhere else. Did I do something wrong ?

Le 4 déc. 2013 à 16:04, hurzl notifications@github.com a écrit :

but you have disabled covers now?


Reply to this email directly or view it on GitHub.

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

When I want to try getting all artists and albums it should show the covers

@abarisain
Copy link
Owner

Oh, I thought we didn't want to do that just yet (as I said, 800 artists make 800 requests, I think the only way to solve this is upstream) and that the batch requests were for the albumartist-artist pull request.

You can probably git revert my commit if you want to test.

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

no two requests as batch command

@abarisain
Copy link
Owner

Well that would be a second request with 800 commands in it, that's still quite huge ;)

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

depends on how fast your server is
this could be done at startup and saved. plus an option to refresh that which you only have to do when the database changes

@abarisain
Copy link
Owner

Also depends on how fast your wireless is (mine is terrible, this would almost never work)
Hmm, starting to work with a cache like that is somewhat a huge task, and puts us on MPoD's road of a database sync on the phone.

Maybe we should try sending this upstream, see their answer and then hack around

@kingosticks
Copy link
Contributor

is that such a bad system? Give the user the option for a one-off request of all this information and then it's cached. You can tell when the data becomes stale from the db_update field in stats. obviously it'll be a bit crap for mopidy where that field isn't implemented (/ is meaningless).

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

but if it is fast, say 5 seconds?
mpd doesnt have a timestamp for its database?

@kingosticks
Copy link
Contributor

http://www.musicpd.org/doc/protocol/ch03.html#idp119728

db_update: last db update in UNIX time

@abarisain
Copy link
Owner

It's not a bad system, but it requires a rework of a big part of mpdroid, and a loading time at startup.

It may be 5 seconds for you, but for me it takes ages. This is only ux, but we can work this around and get away with only an Album cache (a full cache takes minutes for me).

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

Only reload if mpd database is newer than own database

@abarisain
Copy link
Owner

So when the user asks for the Album list, we can just ask mpd for the db_update, compare it with our stored result and then display our cache if it's fine. if it's not, show a progressbar saying "caching albums" and put everything into a SQLite database.

If we do this, we should really use a SQLite database. Having a "Artist, AlbumArtist, Album" table easily queryable would be really helpful.

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

yes
sql I don't know, is it necessary?

@kingosticks
Copy link
Contributor

Sounds good!

@kingosticks
Copy link
Contributor

Well, actually, ideally it'd ask if you want to cache the albums and the whole thing would still be usable if you declined.

@abarisain
Copy link
Owner

That's quite some work though. We need to get a good list of the albumartists, artists, merge it while rememebering what tag was found, remove duplicates, get all their albums and store this into a database.
Once again, please, let's not rush this.

and the whole thing would still be usable if you declined.

We would have to maintain another code path with a broken album list (which is pretty useless).

@kingosticks
Copy link
Contributor

But also remember that the MPD database is essentially dynamic for mopidy so using this cached database for as little as possible (i.e. covers, other bells and whistles) would be best. i.e. still query the server database for anything major.

@abarisain
Copy link
Owner

Yeah, ideally we would use this only for the global album list (can we disable that one for mopidy ? I mean is it even useful knowing that it doesn't specify an artist tag ?). The artist list of mopidy is too dynamic for that, I agree

@kingosticks
Copy link
Contributor

Great, just wanted to check.

@abarisain
Copy link
Owner

I'd like to try to bring this upstream before we do that. @hurzl Do you mind waiting a little before implementing that ? I'll open a bug in their bugtracker http://bugs.musicpd.org/view_all_bug_page.php

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

What kind of bug?
For saving couldn't you just use a serialized list and write it to a file?
List of albuminfo

@abarisain
Copy link
Owner

A serialized list written to a file is not queryable. If we do this, something more flexible (for possible future use) might be a big plus. (Once you get the album list, you can easily get a artist/album artist list).

Plus, if we change the list type or whatever, we will need to be careful and nuke the cache (for exemple when somebody updates his mpd), just like we need to be careful with the cover art blacklist.(We aren't yet)

One thing I'm afraid to break is compilation albums. They sometimes (and often) lack AlbumArtist, but are available in Albums view (even in software like iTunes) because they are tagged as "part of a compilation". I think we would loose these ones.

@abarisain
Copy link
Owner

I submitted a feature request : http://bugs.musicpd.org/view.php?id=3892

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

I would just load it into memory... it can't be that big 1000 albums with 100 bytes even if 10000

What compilation albums are I don't know. What's the tag for that?

@abarisain
Copy link
Owner

Oh yeah, we can keep it in the application object too. We're not really supposed to work that way, but yeah.

If MPD ever implements this we can switch our implementation with theirs (if the server running MPD is new enough)

@hurzl
Copy link
Contributor

hurzl commented Dec 4, 2013

that will be in version 0.20 I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants