slimserver
slimserver copied to clipboard
LMS-Community
Missing artist_id field in browsonlineartist
As mentioned in the Material Skin thread on forums.slimdevices.com (here and here) the artist_id parameter is, I think, missing from the SlimBrowse JSONRPC responses. My understanding is that a client (e.g. Material Skin) uses the go/do/etc. actions listed in these responses when an items is clicked, and each action should list its required parameters. As stated, artist_id does not appear to be in the response, so further clicks fail.
I'm not 100% certain if the SlimBrowse response should be updated for these, or if a client should always just add an "artist_id:xxxx" parameter if the client currently has "artist_id" for its current list. (Hope that makes sense!).
Well, I guess it could happen with other other queries if LMS assumes a persistent connection. However, I've only had reports about browseonlineartist.
If you mean which level on browseonlineartist - then Material calls "browseonlineartist","items",0,25000,"service_id:SERVICE","artist_id:ARTISTID","menu:1" to get the applicable list for an artist on a service. In the response to this, the actions do not have artist_id:ARTISTID - which, I think, is where the issue arises. Also, this seems to be for Qobuz, not sure about others.
As stated Material could add artist_id:ARTISTID, as it knows the ID (as its used to get the list itself). I'm just not sure if this is what should happen, or if LMS should already have this.
Is there agreement that this problem needs to be fixed in LMS? Specifically that the action list returned from the initial "browseonlineartist" request should include "artist_id:ARTISTID" in its list of required parameters? This is a major problem for those of us using Qobuz with Material Skin, although it can be worked around by using the Qobuz plugin directly.
I'm sorry, that comment about agreement made me laugh 😁: even if all of you agree, somebody has to do the work. If I can't free time to work on this, and nobody else is stepping up, it won't happen.
So maybe you can be of more help if you provided easy to follow step-by-step instructions how to reproduce this issue? Thanks!
I looked at the code without even knowing how to reproduce. But unfortunately it's based on some of the most convoluted code in LMS... I'm not sure how easy it'll be to implement this...
The problem is easily reproducable if you have a Qobuz subscription and Qobuz favorites integrated with your music library, and use the Material Skin client.
- Select a favorited Qobuz artist from the My Music artist list to get a list of the artist's albums.
- Select 'Qobuz' from the dropdown list generated by selecting the circle icon containing three dots, just to the right of the artist name.
- From the resulting menu, choose any of the 5 items, such as Albums.
Nothing is returned, and the debug log shows the following message:
[22-08-22 13:34:42.0761] Slim::Web::JSONRPC::requestMethod (477) Request failed with error: Bad params!
Hmm... that's working as expected here. I not only get the albums, but I can also play them. What am I missing?
{
"params": [
"00:04:20:22:01:0c",
[
"browseonlineartist",
"items",
"0",
25000,
"menu:browseonlineartist",
"item_id:0"
]
],
"id": 8,
"result": {
"title": "Alben",
"window": {
"windowStyle": "icon_list"
},
"count": 20,
"item_loop": [
{
"text": "Alors On Danse\nStromae",
"params": {
"item_id": "0.0",
"isContextMenu": 1
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F24%2F25%2F0060252752524_600.jpg/image.jpg"
},
{
"text": "Cheese\nStromae",
"params": {
"isContextMenu": 1,
"item_id": "0.1"
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F25%2F71%2F0060075327125_600.jpg/image.jpg"
},
{
"type": "playlist",
"params": {
"isContextMenu": 1,
"item_id": "0.2"
},
"text": "Formidable\nStromae",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F58%2F37%2F0060253763758_600.jpg/image.jpg"
},
{
"type": "playlist",
"text": "Multitude\nStromae",
"params": {
"item_id": "0.3",
"isContextMenu": 1
},
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F4b%2Fd1%2Fwv5ey906nd14b_600.jpg/image.jpg"
},
{
"params": {
"item_id": "0.4",
"isContextMenu": 1
},
"text": "Multitude\nStromae",
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fpb%2F2d%2Ff13hljeed2dpb_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F32%2F01%2F0060253760132_600.jpg/image.jpg",
"params": {
"item_id": "0.5",
"isContextMenu": 1
},
"text": "papaoutai\nStromae",
"type": "playlist"
},
{
"text": "Peace Or Violence Remixes\nStromae",
"params": {
"item_id": "0.6",
"isContextMenu": 1
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F39%2F42%2F0060252774239_600.jpg/image.jpg"
},
{
"params": {
"isContextMenu": 1,
"item_id": "0.7"
},
"text": "Racine carrée\nStromae",
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F60%2F14%2F0060254701460_600.jpg/image.jpg"
},
{
"text": "Alors On Danse\nStromae",
"params": {
"item_id": "0.8",
"isContextMenu": 1
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F40%2F56%2F0060075325640_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fmb%2Fiy%2Fzk3tza7o8iymb_600.jpg/image.jpg",
"type": "playlist",
"params": {
"isContextMenu": 1,
"item_id": "0.9"
},
"text": "Alors On Danse\nStromae"
},
{
"params": {
"item_id": "0.10",
"isContextMenu": 1
},
"text": "Alors On Danse \nStromae",
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fha%2F8p%2Fjp76yze4p8pha_600.jpg/image.jpg"
},
{
"type": "playlist",
"text": "Défiler\nStromae",
"params": {
"isContextMenu": 1,
"item_id": "0.11"
},
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fhc%2Fg5%2Fm6ewryerfg5hc_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F79%2F43%2F0060253744379_600.jpg/image.jpg",
"params": {
"item_id": "0.12",
"isContextMenu": 1
},
"text": "Formidable\nStromae",
"type": "playlist"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fwc%2Fxe%2Fcoladm6v2xewc_600.jpg/image.jpg",
"type": "playlist",
"text": "L’enfer\nStromae",
"params": {
"item_id": "0.13",
"isContextMenu": 1
}
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fab%2F0y%2Fn26tf8i8q0yab_600.jpg/image.jpg",
"type": "playlist",
"params": {
"item_id": "0.14",
"isContextMenu": 1
},
"text": "Mon amour\nStromae"
},
{
"text": "Papaoutai\nStromae",
"params": {
"item_id": "0.15",
"isContextMenu": 1
},
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F83%2F11%2F0060253741183_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2Fab%2Fh5%2Flfmxf3bhfh5ab_600.jpg/image.jpg",
"type": "playlist",
"params": {
"isContextMenu": 1,
"item_id": "0.16"
},
"text": "Santé\nStromae"
},
{
"type": "playlist",
"params": {
"item_id": "0.17",
"isContextMenu": 1
},
"text": "Te Quiero\nStromae",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F49%2F67%2F0060252746749_600.jpg/image.jpg"
},
{
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F75%2F49%2F0060075324975_600.jpg/image.jpg",
"type": "playlist",
"text": "Up Saw Liz\nStromae",
"params": {
"isContextMenu": 1,
"item_id": "0.18"
}
},
{
"params": {
"item_id": "0.19",
"isContextMenu": 1
},
"text": "Up Saw Liz \nStromae",
"type": "playlist",
"icon": "/imageproxy/https%3A%2F%2Fstatic.qobuz.com%2Fimages%2Fcovers%2F74%2F49%2F0060075324974_600.jpg/image.jpg"
}
],
"base": {
"actions": {
"go": {
"itemsParams": "params",
"params": {
"menu": "browseonlineartist"
},
"cmd": [
"browseonlineartist",
"items"
]
},
"play": {
"nextWindow": "nowPlaying",
"params": {
"menu": "browseonlineartist"
},
"cmd": [
"browseonlineartist",
"playlist",
"play"
],
"itemsParams": "params",
"player": 0
},
"playControl": {
"params": {
"artist_id": "3785",
"item_id": "0",
"passthrough": [
{
"artist_id": "3785"
}
],
"menu": "browseonlineartist",
"_quantity": "25000",
"_index": "0"
},
"cmd": [
"browseonlineartist",
"items"
],
"itemsParams": "playControlParams",
"player": 0,
"window": {
"isContextMenu": 1
}
},
"add": {
"itemsParams": "params",
"params": {
"menu": "browseonlineartist"
},
"cmd": [
"browseonlineartist",
"playlist",
"add"
],
"player": 0
},
"add-hold": {
"cmd": [
"browseonlineartist",
"playlist",
"insert"
],
"params": {
"menu": "browseonlineartist"
},
"itemsParams": "params",
"player": 0
},
"more": {
"params": {
"menu": "browseonlineartist"
},
"itemsParams": "params",
"cmd": [
"browseonlineartist",
"items"
],
"player": 0,
"window": {
"isContextMenu": 1
}
}
}
},
"offset": 0
},
"method": "slim.request"
}
@CDrummond - I'm sorry I don't always follow up on all postings on the forum. But has this issue been widely reported? I'm using master (commit 83d9756b). As you can see things are working despite the missing artist_id.
Just to clarify, on my system it fails only on the first request for the album list for a given artist, where nothing is returned and the error message is logged. If I then back out to the artist level and come back in a second time via the 'Qobuz" icon, the requested album list is displayed and the albums are playable despite the missing artist_id, as you observed. Additionally if, after that, another artist is selected and a request made to display that artist's albums, either the problem happens again OR the albums for the previous artist are displayed instead. Again, that can be worked around by backing out and re-requesting. The problem is very easy to recreate on my system, which is the current (Aug 22) nightly build of LMS 8.3 and the current released (master) version of Material Skin (v2.10.3). I know of only one other user who has reported the same behavior on the MS forum but I think that might be because there aren't that many Qobuz users active there.
Hmm... different computer, same code, different behaviour. Now I'm seeing what you're describing. Would you be able to apply the following change and test again?
diff --git a/Slim/Plugin/OnlineLibrary/BrowseArtist.pm b/Slim/Plugin/OnlineLibrary/BrowseArtist.pm
index 71dc54799..7956c4d37 100644
--- a/Slim/Plugin/OnlineLibrary/BrowseArtist.pm
+++ b/Slim/Plugin/OnlineLibrary/BrowseArtist.pm
@@ -94,7 +94,7 @@ sub cliQuery {
my $client = $request->client;
my $artist_id = $request->getParam('artist_id');
my $service_id = $request->getParam('service_id');
- my $connectionId = $request->connectionID || '';
+ my $connectionId = ($request->connectionID && $request->connectionID->clid) || $request->clientid || '';
my $feed;
if ($service_id && $artist_id) {
@michaelherger - first of all, no need to appologise for anything, you do a massive amount of work for/on LMS
But, I fail to see how it can reliably work without a persistent connection (I guess the connectionID above). e.g. One user is browsing these menus with Material Skin on a desktop for Artist A, another at the same time is browsing Artist B. Seeing as Material has no persistent connection (each JSONRPC call is a new HTTP POST) how can the plugin know which subsequent call is for Artist A or Artist B ???
That's why I didn't understand it would work sometimes (and I actually still don't understand). But sometimes we seem to get a $request->connectioniID->clid, sometimes we don't. Don't ask me why.
The above workaround would fall back to using the client's MAC address. This could still fail if multiple users were interacting with the same device at the same time, though...
@michaelherger ...and that's why artist_id is required. Or is there a way Material can create a 'fake' connectionID? I'm pretty sure I could create a UUID for each instance and append this to all JSONRPC/HTTP calls. How does the Default skin handle this? As I assume that is not using persistent HTTP connections as well.
Well, the change/workaround to BrowseArtist.pm works great for me as the only LMS user in my household. (My cats have not yet figured out the UI...) ;-)
It still seems to me that the "correct" way to handle this issue would be to populate the artist_id in the actions list returned from the previous browseonlineartist call, as stated in the original post. I personally am happy with the workaround for now though. Thanks, Michael!
Let me revise my last post. I am again seeing albums from the previously viewed artist displayed sometimes, so apparently it is still using a cached artist_id if one exists.
How does the Default skin handle this? As I assume that is not using persistent HTTP connections as well.
As I mentioned this is going through some of the most convoluted code of LMS (XMLBrowser). A dark place I prefer to stay away from 😁. But I'm currently following that route, trying to find something.
Another fly in the ointment. The change to BrowseArtist.pm causes requests from Squeezer to fail with the following error in the log:
[22-08-26 13:38:44.6931] Slim::Control::Request::execute (1885) Error: While trying to run function coderef [Slim::Plugin::OnlineLibrary::BrowseArtist::cliQuery]: [Can't call method "clid" without a package or object reference at C:/PROGRA~2/SQUEEZ~1/server/Slim/Plugin/OnlineLibrary/BrowseArtist.pm line 98.
A few hours of digging later I'm not really any smarter... What I've found, though, is that the response would have a playControl element in the base actions:
{
"id": 3,
"result": {
"base": {
"actions": {
"playControl": {
"itemsParams": "playControlParams",
"window": {
"isContextMenu": 1
},
"player": 0,
"cmd": [
"browseonlineartist",
"items"
],
"params": {
"_index": "0",
"menu": "1",
"artist_id": "3798",
"service_id": "spotify",
"_quantity": "25000"
}
},
...
Could this element be of help? It has all the parameters you'd need, doesn't it?
Whether this is by chance or by design, I unfortunately don't know. I didn't even find any use of that playControl element. Neither in LMS nor Squeezeplay... But the base element is supposed to be a container for common parameters etc. So using that probably makes sense.
@mherger Looks like "params" is just those used for the initial request. I can update Material to always pass parameters from the previous request that are not in the current - which would then add "artist_id" here. I'm just not sure if that will break other API calls? For instance, when adding initial support for MAI I passed back all fields (artist name, artist_id, etc) but this then caused issues.
I have been doing a little exploring in XMLBrowser myself and have, I believe, identified where the actions=>go=>params that are returned from the first browseonlineartist,items request (and used for subsequent specific item requests) are built. I would like to make a change to test whether adding the artist_id to these parms would fix this particular problem, ignoring for now the possible impact on other requests. However I have no experience with, and no knowledge of, how to build my own copy of LMS to test with. So for now I will just post here the one-line change to the current 8.3 version of XMLBrowser.pm that I am proposing.
Insert the line with the comment after line 1815 in subroutine _playlistControlContextMenu:
my $itemParams = {
menu => $request->getParam('menu'),
artist_id => $request->getParam('artist_id'), # this is a test
item_id => $item_id,
};
If anyone can point me in the direction of how to build my own LMS to test with, I would appreciate it. I apologize for my lack of knowledge but am more than willing to learn. Thanks!
@mherger Looks like "params" is just those used for the initial request. I can update Material to always pass parameters from the previous request that are not in the current - which would then add "artist_id" here. I'm just not sure if that will break other API calls? For instance, when adding initial support for MAI I passed back all fields (artist name, artist_id, etc) but this then caused issues.
I don't know, really. But I believe it's what Web::XMLBrowser is doing. I tried to figure out how they handled requests differently.
Insert the line with the comment after line 1815 in subroutine
_playlistControlContextMenu:my $itemParams = { menu => $request->getParam('menu'), artist_id => $request->getParam('artist_id'), # this is a test item_id => $item_id, };
This might work for this particular case, where we're missing the artist_id. But it could be any other value, we don't even know the possible parameter names.
@mherger As I said, I am ignoring for now the possible effects on other requests, which I believe could be handled with a conditional statement if necessary. This section of the code, however, seems to be fairly specific to the case at hand.
So how would I go about building a test version of LMS? I apologize for my ignorance in this area but I can't seem to find any information on it.
What OS are you on? With any but Windows you'd simply edit the file and restart the server. Windows is complicated...
I'm currently running LMS on Windows 10 but I have a backup system on a RPi B+ that I can test with. Thanks!
Okay, I give up. I can't find XMLBrowser.pm anywhere on the RPi. When you say "edit the file and restart the server", what file are you talking about?
find / -name XMLBrowser.pm. That should find three files. You'll need the one in a folder Control.
Oops! I thought I had essentially done that but, upon further research, I realized that I had made an apparently common rookie mistake. The command I used was:
find / -name XMLBrowser.*
Of course, using the full file name (or quoting the wildcarded one) worked. Sorry for taking up your time with this. Thanks for your help. I'll report my findings here.
After doing some testing with XMLBrowser.pm, I have the following to report:
- The change I originally wanted to make had no effect on the problem at hand.
- I found the correct place to make the change and was successful in adding
artist_idto the list of params returned for each of the menu items in the response to the firstbrowseonlineartistrequest, e.g.
actions":{"go":{"params":{"artist_id":"42442","item_id":"0","menu":"browseonlineartist"},"cmd":["browseonlineartist","items"]}},"icon":"html/images/albums.png","text":"Albums"}
- Although the subsequent request for a list of albums using the newly-returned
artist_iddid not result in the JSONRPC error seen previously, it also did not return correct data. Not only that, but returning theartist_idin the params seems to have also caused Squeezer to behave in a similarly incorrect manner on its following menu requests. The only 'good' news is that, with the change, Material Skin and Squeezer both exhibited identical aberrant behavior. ;)
@michaelherger I now see why you referred to the code as "convoluted", as it is nearly impossible (for me anyway) to predict what unintended affect any changes might have. I will leave that part of the code to more experienced and capable hands, of which there are many. Warning: Spending more than 8 hours in XMLBrowser.pm may cause headaches and dizziness.
@CDrummond if you decide to approach this problem from the Material Skin side, I will be glad to help in any way I can.
Ignore my previous post. Lol!!! I now have Qobuz library integration working correctly in both Squeezer and Material Skin after making another small change to XMLBrowser.pm, adding the service_id to the returned params in addition to the artist_id. I understand that these changes could possibly have unintended consequences for other components of LMS and I would like to get some feedback on them, including suggestions on what type of testing to do. As Squeezer seemed to work just fine without the changes as well as with them, I am particularly interested to know whether adding these params might introduce unnecessary additional overhead with clients using persistent connections. If that is the case, maybe the code to add the params could be made conditional based on that fact. I'm open to anything at this point. Right now, however, things seem to be working very well.
Here are the changes:
pi@max2play:/usr/share/perl5/Slim/Control $ diff XMLBrowser.pm.save XMLBrowser.pm
312a313,314
> my $artist_id = $request->getParam('artist_id');
> my $service_id = $request->getParam('service_id');
1130a1133,1140
>
> if ( $service_id ) {
> $itemParams->{'service_id'} = $service_id;
> }
>
> if ( $artist_id ) {
> $itemParams->{'artist_id'} = $artist_id;
> }