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

Subsonic: Android Substreamer artist shuffle play #1000

Closed
mikaelkundert opened this issue Aug 16, 2022 · 15 comments
Closed

Subsonic: Android Substreamer artist shuffle play #1000

mikaelkundert opened this issue Aug 16, 2022 · 15 comments

Comments

@mikaelkundert
Copy link

My Android Substreamer UI gets crazy after pressing shuffle play in artist view.

Deck views gets horizontally offset and piles up.

This doesn't occur when using native subsonic server.

I don't know how backend affects this, but I can't tell what request it makes when clicking shuffle for selected artist.

@mikaelkundert
Copy link
Author

mikaelkundert commented Aug 16, 2022

See post in reddit also.

Screenshot of broken UI:
Screenshot_20220816-222530_substreamer

@paulijar
Copy link
Collaborator

paulijar commented Aug 17, 2022

Thanks for the report. I have seen the UI go haywire on Substreamer but I haven't noticed that it always happens with that Shuffle play button. For me, the UI looks a bit different, but basically the whole view navigation gets anyway broken and the app becomes unusable. I have thought that the Substreamer app is just bugged and broken, but you are right, it does seem to work rather well when using the Subsonic demo server at demo.subsonic.org.

When that shuffle button is pressed in the artist view, the client seems to send a request like this: /nextcloud/index.php/apps/music/subsonic/rest/search3.view?u=myusername&p=mypassword&v=1.13.0&c=substreamer&f=json&query=Apocalyptica&artistCount=0&artistOffset=0&albumCount=0&albumOffset=0&songCount=100&songOffset=0. In previous example, the viewed artist was Apocalyptica.

So Substreamer tries to use the search operation to find the songs of the artist. And here's one significant difference: The search API method on the Music app only finds songs by song name, albums by album name, and artists by artist name. But searching tracks by artist name doesn't find tracks performed by that artist, so the query used by Substreamer usually results in an empty response. Meanwhile, search on the Subsonic demo server seems to return also tracks where the query matches the artist name. Now, Substreamer might be totally unprepared for the possibility that the search operation would return with no results. And somehow, it manages to totally screw itself up after this.

paulijar added a commit that referenced this issue Aug 20, 2022
…ities

Previously, the `search2` and `search3` methods of the Subsonic API searched
only artists by artists name, albums by album name, and songs by song name.
Now the songs can be found also by the name of their parent album or the
performing artist. Similarly, albums can be found also by searching with the
artist name.

The correct logic for these search methods has not been defined in the
Subsonic API specification. However, the Subsonic demo server seems to work
similarly as our modified logic.

The Substreamer client application on Android uses the `search3` method also
to implement its function to shuffle play songs from an artist. Previously,
trying to search songs with an artist name typically resulted in an empty
result set, and Substreamer was totally unprepared for this. As a result, the
application navigation logic broke down totally and the user had to wipe
clean and re-enter his/her account settings to recover.

refs #1000
@paulijar
Copy link
Collaborator

Indeed, the problem disappeared when I modified the search method on the Subsonic API to find also songs based on the artist name.

It's of course logically wrong thing to do for Substreamer to use the free text search to find the songs for the artist shuffle play because the search could happen to match also songs from other artists. However, I can see why Substreamer does that because there is no easy way in the Subsonic API to get all songs from an artist. The only way would be to request separately the songs for each album of the artist. And if the artist happens to be featured on some compilation album, that would cause extra headache. Furthermore, the shuffle on artist doesn't seem to actually enqueue songs from other artists even in the cases where the search should find those; maybe Substreamer does some post-filtering on the results of the search and only includes relevant songs.

Substreamer still seems to have some issues showing all the album covers and the albums without any available cover look broken and ugly. I wonder if the album art loading would work better if we provided some placeholder art for those albums without actual cover available.

@ghenry22
Copy link

Thanks for reporting this.

You’re exactly right that substreamer is not expecting an empty API response and that is causing the issue. I will put in a check to handle this.

The broken UI is because of the previous error putting the app in a broken state, there are occasionally null errors that can do that. Between all the slightly different API implementations it can be hard to stay on top of every possibility.

the search method is used as it’s the most efficient method, yes the subsonic search isn’t great sometimes but this is the easiest way to get a random selection of songs from an artist. Otherwise I have to get all albums and then shuffle them and that makes a lot more api calls and misses anything that is part of a compilation. It’s all a trade off trying to work around the API the best way while providing some new features.

If you tap the 3 dots more icon at the top of the artist view I think you should be able to choose “play similar artists” which will play a mix of this artist and similar. The shuffle button just plays a random selection from the current artist.

Regarding cover art, substreamer relies on the server to provide coverart, if it does substreamer shows it, if it returns nothing then there will be a missing image. The original subsonic will generate cover art for tracks that do not have any so you can always expect a response from the API.

I will have a look at putting something in the app to handle broken coverart API responses but the correct place to fix it is on the server side so that the API is consistent with the original subsonic implementation.

@paulijar
Copy link
Collaborator

paulijar commented Aug 28, 2022

@ghenry22 Thanks for the reply.

the search method is used as it’s the most efficient method

"If it's stupid, but it works, it ain't stupid" :D

I will have a look at putting something in the app to handle broken coverart API responses but the correct place to fix it is on the server side so that the API is consistent with the original subsonic implementation.

I'm already working on providing the placeholder images from the server. However, doing this gives me the feeling that I'm doing something wrong, because I find that this kind of logic belongs on the UI layer and not on the service layer. Furthermore, the Subsonic REST API specification http://www.subsonic.org/pages/inc/api/schema/subsonic-rest-api-1.16.1.xsd does declare the coverArt property as optional on all entities.

Edit: Okay, I tested with the Subsonic demo server, and it seems to work so that it does omit the coverArt property from those albums which don't have any cover art set. But still, if you call getCoverArt with an id matching such album, you will get a generated image placeholder containing the name of the album. So probably I should do it like that, too.

Edit2: But if I do it the same way as the Subsonic demo server, then I still get the "broken image link" kind graphics on Substreamer. The described behavior can be seen on the Subsonic demo server here on the album named "Movies", all the other albums there seem to have an actual cover art set: http://demo.subsonic.org/rest/getAlbumList.view?u=guest&p=guest&c=test&v=1.13.0&type=random&size=100

@paulijar
Copy link
Collaborator

paulijar commented Aug 28, 2022

@ghenry22 Even after adding the placeholder images for the albums without a cover art, there's still serious problem showing all the album covers on Substreamer. My best guess is that this is related to how Substreamer floods the server with calls to the API getTopSongs. There are hundreds of such calls whenever the app is opened. This acts like a DOS attack on my low-power RaspberryPi server, and everything slows down to crawl. Then, the attempts to read the cover art may time out before being served. From the logs I can also see, that there's not just one call to getTopSongs for each artist but all artists have been polled over and over again.

Edit: I found that I can disable the "Recommendations" from the Substreamer settings and this stops the getTopSongs spamming. However, this wasn't in the end the largest contributor on the DOS-like server load. It turned out that there was a perpetual redirect loop when Substreamer tried to call the getCoverArt endpoint.

After much digging, I pinpointed the origin of this redirection to this Nextcloud server file: https://github.com/nextcloud/server/blob/master/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php. I don't quite understand this same site cookie stuff, but apparently, this middle ware investigates the cookies of the request, and if not satisfied, adds another cookie and returns a redirection with the code 302 to the originally requested address. Maybe this added cookie then got somehow lost in the case of Substreamer, and the redirected call got redirected over and over again.

Anyway, I was able to fix the situation by disabling the said middle ware on the Subsonic API of the Music app. I guess it exists for the sake of the Nextcloud web UI and we probably don't need it here for anything.

What still puzzles me is that how is it possible that the cover art loading has sometimes worked for me, even when the SameSiteCookieMiddleware was in action. But I guess this isn't important.

paulijar added a commit that referenced this issue Aug 28, 2022
… art set

The original Subsonic server provides some image for those entities not
having an actual cover art set and the clients may expect this. However, the
`coverArt` property for those entities is still omitted; it's also marked as
optional in the Subsonic REST API specification.

The placeholder image is created with similar logic as on the web UI; this
logic has been ripped from the Nextcloud source file
lib/private/Avatar/Avatar.php.

refs #1000
paulijar added a commit that referenced this issue Aug 28, 2022
For some reason, the Nextcloud middleware defined in
https://github.com/nextcloud/server/blob/master/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php
causes perpetual 302 redirection loops on some Subsonic clients in some
situations. This middleware has now been disabled on our Subsonic API.

The problem has been observed on the Android client Substreamer when loading
the album art and on the HTML5 client Jamstash. On Jamstash, the problem
occurs when using the client from jamstash.com but not when it has been
installed on the same server with Nextcloud. It's possible that this has also
something to do with the HTTP server settings used.

refs #1000
@ghenry22
Copy link

@ghenry22 Even after adding the placeholder images for the albums without a cover art, there's still serious problem showing all the album covers on Substreamer. My best guess is that this is related to how Substreamer floods the server with calls to the API getTopSongs. There are hundreds of such calls whenever the app is opened. This acts like a DOS attack on my low-power RaspberryPi server, and everything slows down to crawl. Then, the attempts to read the cover art may time out before being served. From the logs I can also see, that there's not just one call to getTopSongs for each artist but all artists have been polled over and over again.

Edit: I found that I can disable the "Recommendations" from the Substreamer settings and this stops the getTopSongs spamming. However, this wasn't in the end the largest contributor on the DOS-like server load. It turned out that there was a perpetual redirect loop when Substreamer tried to call the getCoverArt endpoint.

After much digging, I pinpointed the origin of this redirection to this Nextcloud server file: https://github.com/nextcloud/server/blob/master/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php. I don't quite understand this same site cookie stuff, but apparently, this middle ware investigates the cookies of the request, and if not satisfied, adds another cookie and returns a redirection with the code 302 to the originally requested address. Maybe this added cookie then got somehow lost in the case of Substreamer, and the redirected call got redirected over and over again.

Anyway, I was able to fix the situation by disabling the said middle ware on the Subsonic API of the Music app. I guess it exists for the sake of the Nextcloud web UI and we probably don't need it here for anything.

What still puzzles me is that how is it possible that the cover art loading has sometimes worked for me, even when the SameSiteCookieMiddleware was in action. But I guess this isn't important.

To generate dynamic recommendations it's unfortunately necessary to make quite a few API calls. For instance for "artists like soundgarden", it would have to get the artistinfo to get the list of similar artists, then for each similar artist get their top songs, then combine all the top songs from the different artists into an array, shuffle the array and trim it to the right max length. I could change this so that the recommendation appears but it only generates the playlist when that recommendation is tapped, which would be much more efficient, but would break in the event that there are no songs in the resulting list. Which can happen in some specific circumstances.

I'm always looking at ways to make it better or more efficient but sometimes in order to add some functionality I'm having to work around the limitations of the API a bit and do more logic on the client side than I like.

Regarding the cookies thing, substreamer does not use cookies at all, nor does the subsonic API. It's a stateless API where the authorization token is sent with each request. Any cookies that your server might set are simply thrown away as they are not needed. I suspect that middleware might be designed for other scenarios that the server supports (particularly in the case of owncloud and similar that offer a lot of different functionality), in these cases you would definitely want to disable those kind of things for the subsonic API endpoint but likely keep them for things like the web interface for browsing your cloud storage. If you have other API endpoints may be worth disabling for them as well.

Thanks for all the feedback though, always great to get ideas and suggestions on how to improve! I'm just getting back to civilization after a bit of a break and will be picking up substreamer and some new improvements and features over the coming months.

@ghenry22
Copy link

@ghenry22 Thanks for the reply.

the search method is used as it’s the most efficient method

"If it's stupid, but it works, it ain't stupid" :D

I will have a look at putting something in the app to handle broken coverart API responses but the correct place to fix it is on the server side so that the API is consistent with the original subsonic implementation.

I'm already working on providing the placeholder images from the server. However, doing this gives me the feeling that I'm doing something wrong, because I find that this kind of logic belongs on the UI layer and not on the service layer. Furthermore, the Subsonic REST API specification http://www.subsonic.org/pages/inc/api/schema/subsonic-rest-api-1.16.1.xsd does declare the coverArt property as optional on all entities.

Edit: Okay, I tested with the Subsonic demo server, and it seems to work so that it does omit the coverArt property from those albums which don't have any cover art set. But still, if you call getCoverArt with an id matching such album, you will get a generated image placeholder containing the name of the album. So probably I should do it like that, too.

Edit2: But if I do it the same way as the Subsonic demo server, then I still get the "broken image link" kind graphics on Substreamer. The described behavior can be seen on the Subsonic demo server here on the album named "Movies", all the other albums there seem to have an actual cover art set: http://demo.subsonic.org/rest/getAlbumList.view?u=guest&p=guest&c=test&v=1.13.0&type=random&size=100

Yes the coverart property is optional, if it exists substreamer will use that property, if it does not then it will use the ID property which is required on all songs/albums/artists etc. So there is always an ID to use. When you browse by file layout rather than ID3 tags you quite often get just the ID and not the coverArt property.

@paulijar
Copy link
Collaborator

paulijar commented Aug 30, 2022

For instance for "artists like soundgarden", it would have to get the artistinfo to get the list of similar artists, then for each similar artist get their top songs, then combine all the top songs from the different artists into an array, shuffle the array and trim it to the right max length.

Maybe Substreamer could use some caching of this data at least? It's not like this information is changing all the time but now Substreamer may poll getTopSongs for the same artist multiple times even during one application session.

I also once saw an occasion where Substreamer started polling the API method getScanStatus once per second after I had cleared the storage. But after (I think) restarting the app this never happened again. We only provide a dummy implementation for this method which always tells that no scanning is ongoing.

@ghenry22
Copy link

ghenry22 commented Sep 1, 2022

For instance for "artists like soundgarden", it would have to get the artistinfo to get the list of similar artists, then for each similar artist get their top songs, then combine all the top songs from the different artists into an array, shuffle the array and trim it to the right max length.

Maybe Substreamer could use some caching of this data at least? It's not like this information is changing all the time but now Substreamer may poll getTopSongs for the same artist multiple times even during one application session.

I also once saw an occasion where Substreamer started polling the API method getScanStatus once per second after I had cleared the storage. But after (I think) restarting the app this never happened again. We only provide a dummy implementation for this method which always tells that no scanning is ongoing.

The subsonic API provides an endpoint to start a scan for new files and to check the scan status, so there is a little refresh button at the top of the home screen that starts the scan, it then polls the scan status until the scan is complete so that it can provide scan progress updates to the user. Polling is the only way unfortunately. I’ll double check what the response is when the scan is complete and we can probably put in a better dummy endpoint that just responds as complete, this would stop the polling for status.

Substreamer does cache all those results from the recommendations and just as you browse but the catch is if some new albums or tracks have been added and I just use the caches data (if it exists) then the new songs don’t get added to the mix. That was the reason why I built it to pull the latest data anyway.

I just started working on an update for the recommendations generator yesterday, I’m going to set it generate empty recommendations and only build the playlist when you click play, I’ll also build this to use cached data if it exists. This should make the recommendations much more friendly in terms of API load.

Also looking at adding some more recommendations like “more alt rock from the early nineties” or whatever depending on recently played genres and release dates.

@ghenry22
Copy link

ghenry22 commented Sep 2, 2022

These are the two API endpoints relevant to the server scanning.

startScan - starts scanning the server for new files to add to the media library.

getScanStatus - returns the current scan status (can be called any time whether scaning or not.

startScan response

This is the startScan response object - it sets scanning as true and resets the item count to 0. Item count increments as it scans the server and finds files).

getScanStatus response

This is the getScanStatus response object - this is the respons after the scan as finihsed, see scanning value is set back to false and the count value is the total file count found in the media library.

If you want to dummy it I would suggest that when startScan returns scanning true and count 0 any time and getScanStatus always return scanning false and the full count figure.

This way any app the calls the startScan endpoint gets the response that it expects that scanning has started, and then as soon as it calls getScanStatus it thinks that the scan has finished and can get the expected file count.

Obviously you could then go back and put in real scan behaviour to replace these later should you wish.

paulijar added a commit that referenced this issue Sep 10, 2022
Previously, the stubbed method `getScanState` returned the property
`scanStatus` as a string "false" while the real Subsonic server returns here
the boolean value false or true. This caused the Substreamer client to never
stop polling the method since scanning was started. In XML everything is just
string anyway so there was no difference.

Also, added the number of scanned tacks in the library to the result of
`getScanState` because this is what the real Subsonic server does, too, also
when no scanning is ongoing.

refs #1000
@paulijar
Copy link
Collaborator

@ghenry22 Thanks for the suggestions. It turned out that the real problem with getScanState was that my back-end was returning the property scanning as string "false" instead of the boolean false. When I fixed this, the Substreamer client stopped the infinite polling. I hadn't noticed the difference as I had been looking only at the examples of the Subsonic API doc which are all XML and don't have any data types.

I decided not to provide any implementation for the API method startScan. When called, there is an error response about unsupported method. I think this is a more proper way to indicate that we don't provide this service. And any compliant client should be prepared for an error response in this case because also the real Subsonic server will respond with an error in case the user has no permission to trigger the scanning; this can be seen by calling http://demo.subsonic.org/rest/startScan.view?u=guest&p=guest&c=test&v=1.16.1&f=json.

@ghenry22
Copy link

ghenry22 commented Sep 12, 2022

@paulijar yes the joy of types and type-casting, where a string of any length = true :)

Feel free to close this issue once you're happy with the status, seems like it should be resolved to me.

Root-Core pushed a commit to Root-Core/owncloud-music that referenced this issue Sep 17, 2022
… art set

The original Subsonic server provides some image for those entities not
having an actual cover art set and the clients may expect this. However, the
`coverArt` property for those entities is still omitted; it's also marked as
optional in the Subsonic REST API specification.

The placeholder image is created with similar logic as on the web UI; this
logic has been ripped from the Nextcloud source file
lib/private/Avatar/Avatar.php.

refs owncloud#1000
Root-Core pushed a commit to Root-Core/owncloud-music that referenced this issue Sep 17, 2022
For some reason, the Nextcloud middleware defined in
https://github.com/nextcloud/server/blob/master/lib/private/AppFramework/Middleware/Security/SameSiteCookieMiddleware.php
causes perpetual 302 redirection loops on some Subsonic clients in some
situations. This middleware has now been disabled on our Subsonic API.

The problem has been observed on the Android client Substreamer when loading
the album art and on the HTML5 client Jamstash. On Jamstash, the problem
occurs when using the client from jamstash.com but not when it has been
installed on the same server with Nextcloud. It's possible that this has also
something to do with the HTTP server settings used.

refs owncloud#1000
Root-Core pushed a commit to Root-Core/owncloud-music that referenced this issue Sep 17, 2022
Previously, the stubbed method `getScanState` returned the property
`scanStatus` as a string "false" while the real Subsonic server returns here
the boolean value false or true. This caused the Substreamer client to never
stop polling the method since scanning was started. In XML everything is just
string anyway so there was no difference.

Also, added the number of scanned tacks in the library to the result of
`getScanState` because this is what the real Subsonic server does, too, also
when no scanning is ongoing.

refs owncloud#1000
@paulijar
Copy link
Collaborator

All the fixes mentioned above have now been released in Music v1.7.0.

@mikaelkundert
Copy link
Author

Sorry for didn't following and providing more details along your debugging, but I see that you were able to locate the problem and fix it. Thanks! ❤️

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

3 participants