sentriz / gonic

music streaming server / free-software subsonic server API implementation
ircs://irc.libera.chat/#gonic
GNU General Public License v3.0
1.5k stars 105 forks source link

feat: store and expose individual track artists #397

Closed sentriz closed 8 months ago

sentriz commented 8 months ago

stores and exposes individual track artists over the subsonic api

related #388

(note that endpoints like getArtists will still be album-artists only, but at least in clients you can click on track artists to see if there are any albums they are credited as an album artist on)

image

https://github.com/sentriz/gonic/assets/6832539/b61e7586-87d6-472b-84e9-2bcf5c87f3e9

sentriz commented 8 months ago

@Tolriq this may be of interest to you :eyes:

Tolriq commented 8 months ago

Yes a lot but I fear about note that endpoits like getArtists will still be album-artists only does this means that it could return artist ids that are not seen during a search3 for artists too? (That would be problematic for Symfonium as it would remove them, I do not support artists without a valid id)

sentriz commented 8 months ago

Yes a lot but I fear about note that endpoints like getArtists will still be album-artists only does this means that it could return artist ids that are not seen during a search3 for artists too? (That would be problematic for Symfonium as it would remove them, I do not support artists without a valid id)

currently search3 would still return only album artists. but it's a good question. if search3 were to return track artists too, what happens when i click on an artist who has no albums, but appeared on a compilation some time? (just one track as an track artist)

Tolriq commented 8 months ago

Well this is client side problem then. Symfonium would only display the tracks and it would not been seen as an album artist as no album associated with it.

Don't know about other clients.

sentriz commented 8 months ago

that sounds like a pretty good compromise. you're okay with this implementation then?

in future maybe we could make this more concrete like getArtists?type=track or ?type=album

Tolriq commented 8 months ago

Yes we need to add new filter stuff there's a discussion started but no reactions ;)

But for Symfonium and so personal usage, the track artists needs to be returned on search3 else dropped. An ugly solution for now may be to only return all artists for search3 with empty query?

sentriz commented 8 months ago

i wouldnt be a fan of returning track artists from search3, otherwise we'll get 100s of results of artists i've never head of from compilation albums. very much cluttering the results

and returning them with an empty query seems a little magic. maybe we could add a ?includeTrackArtists or something? or come up with something better defined at opensubsonic

Tolriq commented 8 months ago

LMS and others returns all artists, you are kinda the exception. There's album count field if it's 0 the client can hide as it's not album artist if that's your issue.

But you are searching artists, the API does not say it's album artist. If something was to be added it would probably be the opposite onlyAlbumArtists.

sentriz commented 8 months ago

mm yes i see do you know of any clients that filter results where albumCount > 0? I primarily use airsonic-refix and DSub so they would need to support it

and for LMS I believe you can control the behavior with a setting. any thoughts behavior here @epoupon ? thanks!

epoupon commented 8 months ago

Hi! Indeed LMS returns all artists, but albumCount is the sum of albums where the artist appears on (this is not related to album artist). When you query an artist, you expect to see all the albums they appear on, and it should as many albums as albumCount. Of course that is a lot of artists returned, so I have been asked to only return album artists. Then I made a setting to return all artists or only album artists.

That being said, now we have the roles, we really have to add a role filter in the query and that would solve all this.

Tolriq commented 8 months ago

I don't know or use other clients :)

All I know is that having artists in the db that can't be searched for is not good. This is data useful for the user.

There's https://github.com/opensubsonic/open-subsonic-api/discussions/57 to discuss about the filtering.

But right now the API does not dictate that all the artists endpoints should return only albumArtists.

So having track artists that can't be synced or searched, limits everything a lot. No clients have a song view page that would be listing the artists so they would only be seen and eventually clickable during playback. And if you play from offline cache then those artists are not in the cache and error.

So while it's nice to have track artists in this PR if we can't really use them it's not really present.

So if you do not like the idea of returning all artists please propose something on OpenSubsonic but default from current API definition would be return all.

This is a search endpoint you know you have a song from artist X you search X and it's not found.

sentriz commented 8 months ago

I would be in favour of extending search3 with a filter. since that endpoint first and foremost is for the user to search media, not to be used as a method of dumping the db

and for me as a user I would find it really annoying to have track artists appear in search results

so definitely a filter extension or setting somewhere

Tolriq commented 8 months ago

There's a conflict in your statement ;)

It's to search but you can't find stuff despite the API not saying it's only album artists.

You search artists X it's not found because Gonic hide it not because it should not be returned ....

I agree a filter to hide track only artist is useful, but adding an extension to show data that should be already here change the default behavior.

In the same vein it's getArtists not getAlbumArtists, so again it should return all with adding a filter to only return album artist to support some specific use cases.

The API says something, the fact that you do not like it's default behavior does not mean the API should be crippled for your need.

So I'll keep disagreeing here. The need is to extend API to allow hide track only artists. Not the other way around that is how the API is currently designed for.

sentriz commented 8 months ago

@Tolriq

i just booted up airsonic-advanced (the closest we have to the original subsonic implementation) and added 2 albums. one by a single artist, and the other a another a compilation album

:point_right: both getArtists.view, and search3.view's "artist" object operate on album artists only

so you will never see a track artist in getArtists.view or search3.view's "artist" field

however you can search for track artists names, and their tracks will appear in the "song" field

so i think actually gonic has the correct behavior already, and the extension should be "includeTrackArtists" or similar, not the other way around

image image image image

Tolriq commented 8 months ago

We had this discussion on many things for OpenSubsonic ... There's an API that says X, the fact that one have it implemented as doing Y does not make Y the API definition. There was plenty of wrong choices in Subsonic hence the OpenSubsonic.

So yes you do not not agree and everything, but the API is not clear, if it needs a proper definition, then by it's naming the API talks about artists and this covers track artists, composers and everything.

What you want is a filter to only include albumArtists. We won't add an includeTrackArtist then an includeComposer and an includeLyricist and every other variations.

Anyway it's currently unclear so if you want to implement it your way it won't be non compatible with OpenSubsonic so do as you want.

If you want to force your interpretation of the API then do the proposal of the definition to achieve that. But it's a breaking change for any server that return all. If I do the proposal I'll do it the way the current API is defined to be a way less breaking change for some servers. Some servers not returning data they do not return already won't be really breaking, forcing servers to remove data they currently return will be breaking.

sentriz commented 8 months ago

airsonic-advanced is not just some Y implementation, it's the original implementation that other servers are based on. I also have a feeling navidrome returns album artists only for getArtists too, does it not?

also, for your specific use case of dumping the db via search3. can't we just say that query=* returns all songs, with artist names and IDs? like in this PR? do you need for them to be included in the artist object?

Tolriq commented 8 months ago

airsonic-advanced is not just some Y implementation, it's the original implementation that other servers are based on.

Many servers are not java and base their implementation on the API documentation and the resulting XML. Anything not documented enough have led to different result in each servers. Like params for coverArts, stream endpoint scrobbling and many others. The source of truth is the XML http://www.subsonic.org/pages/inc/api/schema/subsonic-rest-api-1.16.1.xsd.

Nowhere anything says it's albumArtists only why would similarArtists be only albumArtists why searching an artist would be albumArtist only. A song as a Child have an artistId if the song have no albumTag the artist should be removed and the artist should not exist? What does Gonic do? It creates a fake album?

From the API POV, this makes absolutely no sense that artists are albumArtists only.


For my need (The need of any offline first app ....) , as explained, I need the list of all artists to be able to match Ids. If a song says my artist is ID 12 but no list contains 12 then this means that I need to make a query getArtistInfo for 12, or that this artist does not exist. So 50 000 queries in some cases to have the artist info to be able to display them when offline? And do the link and all the calculations and filters that the API does not support. This is not sustainable.

sentriz commented 8 months ago

that document doesn't describe server semantics, just the response format. and yes it is vague, that's why servers copy some semantics from the original implementation. and it seems to me most implementations go for an album-artist first approach. don't you think this means anything?

A song as a Child have an artistId if the song have no albumTag the artist should be removed and the artist should not exist? What does Gonic do? It creates a fake album?

not sure I understand this, could you elaborate?

as for the offline app needs, maybe we would be better off with a new dump.view that could cover all cases

Tolriq commented 8 months ago

don't you think this means anything?

Again as for other cases, don't you think that servers who have chosen the other way around means anything? Forcing albumArtist is breaking all those servers. So currently LMS works with Symfonium but it would no more ;)

In the same spirit, the OpenSubsonic have exposed the different artists with roles and everything with the assumption that those artists would be accessible and searchable else there's little usage of the data.... The https://opensubsonic.netlify.app/docs/responses/artistid3/ have a roles field. If every endpoint returning those only return albumArtists this kinda breaks the purpose no?

So yes you do not agree, but the way it's been built for now based on the API definition and the OpenSubsonic current extension it's that all artists are returned.

A song as a Child have an artistId if the song have no albumTag the artist should be removed and the artist should not exist? What does Gonic do? It creates a fake album?

If you have a song that have no album value so is a single, with an artist. According to your definition everything in current API related to artist so the artist value and the artistId returned on those songs are album artists. Since this is a single and it is not part of an album the artist field and the artistId field should be empty right? If not then it means that in some cases artists are albumArtist and some other it's trackArtist?

as for the offline app needs, maybe we would be better off with a new dump.view that could cover all cases

Again the whole thing was made to be able to keep working with current servers, you want to break current status and all server returning all artists. Again I understand that you do not want all artists returned but this breaks the current status.

There's APIs to getArtists why should we need new API to getArtists? The needed API for offline music is a getSongs.

dweymouth commented 8 months ago

I think the most backward-compatible choice for now at least would be to either:

  1. Do not assign an ID to track artists who are not credited as an album artist on any albums, but include them in each track's artists list with the ID unset (or empty string) - this is what Navidrome currently does. The result is that they will be considered non-navigable by clients and can be rendered with a plain label rather than a hyperlink in tracklist views.
  2. Consider the albums that an artist "appears on" as part of their discography - in this case it's OK to assign an ID to track-only artists as it avoids the appearance of an empty artist page in clients if the artist is navigated to. Since the API currently doesn't have a way to return tracks for an artist outside of albums, or the "top tracks" feature, assigning an ID to artists that have no albums is not a good solution IMO
  3. Edit: I guess another option is to guarantee that the getTopSongs API will always return some tracks, even if there is no LastFM info for that artist. This will keep track-only artists from having a completely empty artist page.

Once we have a way to get all tracks for a given artist (either by extending getArtist or a new API), then this ceases to be an issue, and we can give track-only artists a navigable ID yet have their discography be empty.

sentriz commented 8 months ago

Again as for other cases, don't you think that servers who have chosen the other way around means anything? Forcing albumArtist is breaking all those servers. So currently LMS works with Symfonium but it would no more ;)

from what I can tell - airsonic-advanced, airsonic, subsonic, navidrome, and gonic all return albumArtists by default

have a roles field. If every endpoint returning those only return albumArtists this kinda breaks the purpose no?

ah i missed that field, that is interesting. then what about an extra param for getArtists, like getArtists?role=artist,albumartist then gonic can default to just albumartist keeping existing functionality (and in-line with other subsonic servers) but clients who care about track artists can request to include them. that would cover all cases right?

If you have a song that have no album value so is a single, with an artist. According to your definition everything in current API related to artist so the artist value and the artistId returned on those songs are album artists.

at least for gonic, it's not possible to have a track that is not part of an album. so there will always be albumId, artistId, and artists fields for songs

you want to break current status and all server returning all artists

again i don't think this is breaking the current status. maybe opensubsonic defines this to be all artist, but i mean the orignal subsonic severs. however, i think we can all be happy something like this ?role= parameter. what do you think?

Tolriq commented 8 months ago

Do not assign an ID to track artists who are not credited as an album artist on any albums, but include them in the track artists list with the ID unset (or empty string) - this is what Navidrome currently does. The result is that they will be considered non-navigable by clients and can be rendered with a plain label rather than a hyperlink in tracklist views.

This is the opposite of the need. That means that for my collection with LMS I would loose the information of 19000 artists / composers and all the other roles .... This is actually the more breaking change since it force apps who have the data to drop it ... Gonic did not handle track artists so did not have issue, now it want to handle track artist

This is what I have for my collection on LMS

image

image

And this works with all other providers too.

I can filter and search artist / composers / ... have their images and biography.

at least for gonic, it's not possible to have a track that is not part of an album. so there will always be albumId, artistId, and artists fields for songs

So you invent fake albums to workaround the choice ?

, like getArtists?role=artist,albumartist

And composer,lyricist,producer,... and anything that can be put there. And getArtists is not as efficient as search3 for artist sync, due the stupid split per letter.

again i don't think this is breaking the current status.

LMS returns all by default, so yes this is breaking the current status as it would now be forced to not return all.

But way too much time lost here. As I said the API is not well defined do what you want, when more clients supports OpenSubsonic we'll advise. For now I won't be able to use track artists with Gonic.

But no I will not support breaking LMS that fit my need currently for my personal needs of track artists and composers).

dweymouth commented 8 months ago

I wonder if the most mutually-compatible option is my # 3 above then? It allows track artists to be navigable but prevent totally empty artist pages for track-only artists

sentriz commented 8 months ago

which other providers return album artist by default? we're not talking about your particular LMS + symfonium setup here. it's the whole ecosystem. as far as I can album artist is the default.

why can't we add a ?role= filter to getArtists and all be happy? we can add it to search3 too if needs be

So you invent fake albums to workaround the choice ?

no fake albums. an album is grouped by the containing folder, like subsonic does. this means there's always an album, enables the subsonic browse-by-folder mode, and solves the issue of artists having multiple albums by the same name (American Football), or different releases/remasters/disambiguations

Tolriq commented 8 months ago

Just one provider having a different implementation is a breaking change .... You did not even have track artists before as others so of course less are impacted ... A few other don't support them too so the question does not even arise.

And all other non subsonic providers behave as this.

Returning IDs and artists that can't be found in the search and getArtists is absurd and not logical you'll never change my mind on this. Since you won't change either as I said do as you wish the API is not clear you won't break anything by doing A or B.

why can't we add a ?role= filter to getArtists and all be happy?

Because we build a proper well thought API and don't do quick half solution things? Passing unknown arbitrary strings to add data can't work as explained.... Roles are strings that can be anything...

dweymouth commented 8 months ago

@Tolriq I think the big issue is that there is no current API to get an artist's tracks. This makes it hard for online-first clients to handle track artists as first-class citizens. All this would be moot if we had a getArtistSongs API or extended getArtist to return tracks the artist appears on in addition to their album discography. Maybe we should try to prioritize an OpenSubsonic extension for this? Also I like sentriz's suggestion of a filter parameter for getArtists to select whether you want to include or exclude track-only artists.

Tolriq commented 8 months ago

Well Sentriz says he don't want to see them not that it's missing the track list.

For the filter read again what I wrote we build a proper API this is not enough to cover all needs. As the couple of months required to built the new data it will take time to build the new filter stuff. And probably more as little reaction in the corresponding thread.

A new endpoint for a specific need might be faster to define.

sentriz commented 8 months ago

extended getArtist to return tracks the artist appears on in addition to their album discography

i like this idea. currently clicking on a track with gonic is only useful for if that track artist also happens to have an album. would be nice to see their individual tracks on compilation albums too

dweymouth commented 8 months ago

extended getArtist to return tracks the artist appears on in addition to their album discography

i like this idea. currently clicking on a track with gonic is only useful for if that track artist also happens to have an album. would be nice to see their individual tracks on compilation albums too

I guess this should be true for other "artist" types as well - Composers, etc? (But this is now a conversation for the OpenSubsonic org)

Tolriq commented 8 months ago

I guess this should be true for other "artist" types as well - Composers, etc? (But this is now a conversation for the OpenSubsonic org

Any change is for there and as you say it's more complicated to support everything than just a new random param.

I tried to validate the default badly documented behavior. Failed now we'll see what is proposed and we'll need to continue to have different behavior per server until then. So failed my year long attempt ;)

dweymouth commented 8 months ago

@Tolriq We got multi-valued fields, a lot of new data, transcodeOffset, etc so far - so it's a good start! It will take some time to standardize on new behaviors and APIs for something as "messy" as music tagging and organization :)

sentriz commented 8 months ago

i appreciate the input everyone :+1: for now i'll ship this as-is, closer to my personal needs and original subsonic. but will be happy to implement opensubsonic extensions that cater to track-artists better

Tolriq commented 8 months ago

@dweymouth Do you have any idea the time and energy invested to reach that?

And now we have 2 OpenSubsonic servers and we already have a divergence that will generate a breaking change in the future.

Only 2 servers and the purpose of the project is already failed, that's not what I would call a good start.

I'm glad that you implement the client side too as I feel less alone, but I do hope others will make the proposals and all the analysis, thinking and build the doc and everything, because I need a break for now and will only give comment and advice but stop trying to carry everything on my shoulders.

sentriz commented 8 months ago

side note: i think it's already a breaking change opensubsonic defines getArtists as all artists, when original implementations were album artists only. it should have been a parameter from the start IMO

Tolriq commented 8 months ago

If only the proposals where opened to everyone and the implementation PR opened for a couple of weeks too with many discussions ;) Your side note, means that you agree that getArtists is all artists but deviate from OpenSubsonic on purpose then ;) Anyway I'm out.

sentriz commented 8 months ago

yes i suppose i do disagree. opensubsonic shouldn't have defined getArtists to be all artists, since no other implementation worked that way. hence it's not an extension, but a breaking change.

but yes we're not really making on progress with this discussion. but i'm happy to contribute/implement extensions that cater to to track artists :+1:

Tolriq commented 8 months ago

since no other implementation worked that way.

Except @epoupon you mean ? ;) Who participated a lot in the discussions so it was not seen as breaking by anyone since already working like that for some ... You did not support track artists at all so the question did not even bring any reaction from you.

And now instead of embracing OpenSubsonic you already breaks it's implied API after the first round of new things.

So again 2 real OpenSubsonic servers, 1 already supporting track artists as the API is defined (or implied if it was not clear enough) and the second one add later track artists in a different way ...

Sorry but I can't be happy with that result and the waste of the last year of work.

sentriz commented 8 months ago

true i was not around at the time it was defined. but my point is that before opensubsonic existed, the majority of servers were based on album artists only, were they not? so opensubsonic broke that by changing the behaviour without having an explicit parameter or extension tag

and i'm not discrediting epoupon's work at all. LMS is a fantastic server

Tolriq commented 8 months ago

You are discrediting OpenSubsonic here by being part and going the other way...

And yes you were around since you also participated in the PR and validated it ....

By returning track artists from OpenSubsonic but not giving access to those artists you break the contract and in this case Symfonium too.

I'll use the data to generate the displayArtist or you'll return it but the users won't be able to click as the artists does not exists. So now I'm forced to get back to if server is Gonic do something different. Effectively annihilating all the work done.

So breaking the past or the future you have made your choice.

And again the API was not well defined a choice was made between different interpretation for the one that matches better OpenSubsonic. As for some other cases.

dweymouth commented 8 months ago

Alright I think there's no more value continuing discussion here. We will pick this up in the OpenSubsonic forum with a new discussion on API improvements to handle the distinction between album and track artists.

Tolriq commented 8 months ago

If I may just a copy/paste from OS

Note: All OpenSubsonic added fields are optionals. But if a server support a field it must return it with an empty / default value when not present in it’s database so that clients knows what the server supports.

sentriz commented 8 months ago

what's your point @Tolriq ?

Tolriq commented 8 months ago

That this is not currently the case in Gonic and that added to the merged artist stuff this makes it hard to handle things at client side even if we want to do something special.

Like in your test server "tr-34914" does not return an displayArtist field, so it's assumed the field is not supported and should fallback to artist value (that is also not present for that file but irrelevant). Maybe this is a special cases for that unknown artist you handle internally now but there's many other cases where fallback is hard to choose if we do not know if the value is missing because it's not supported and we need to use old fallback or generate value or because the field is really empty and should be handed as empty. So probably the case here.

That's the whole reason the API was designed like that.

sentriz commented 8 months ago

OK I'll have a look at that track

Tolriq commented 8 months ago

There's other cases just this one is easy to spot as no name so first in the list.

sentriz commented 8 months ago

looks like

should all be fixed now, thanks

Tolriq commented 8 months ago

@sentriz from the PR "albumArtists": null, is wrong it should be [] null are not allowed in OS for now as badly handled by many parsers.

sentriz commented 8 months ago

is that specified somewhere? which parsers can't handle null?

Tolriq commented 8 months ago

It was discussed in the OS PR and the doc says empty / default not null as I assumed it was clear enough from the discussion. The doc can be amended to specify that if it's unclear. But any parser that is based on the doc and so says the field should not be null won't support null as API is enforced.

https://github.com/opensubsonic/open-subsonic-api/pull/51#issuecomment-1741787882