librespot-org/spotify-connect-resources

The community for ongoing Spotify Connect research and the librespot-org/librespot project.

Roderick van Domburg

@roderickvd

Second point yeah I know but what would be a good solution that doesn’t cost too much boilerplate? Pretty much everything in the protobufs is optional well, 90%) so this would be a lot of options and inspecting them.

At the time I thought: deal with it later and in many cases it may be logical that a 0 default in fact means: option missing. But if you want to put in a proposal…

dnlmlr

@dnlmlr

2nd point is problematic indeed. I get why you wouldn't want to make everything optional, as there is also stuff where I think it can always be assumed to be present like for example Track::{name, duration}. The problem imo is to figure out what is actually missing in some cases.

After looking through the code again I think most critical would be Track::{number, disc_number as I don't think all tracks need to be tagged with disc / track number. Since you can't determin if disc_number== 0 is actually disc-0 or not present, maybe those should be explicitely optional. With something like Track::version_title it would default to an empty string which could easily be checked.
But on the other hand if you need to check is_empty() anyways before using it, you can also use an optional value instead. At least that way it forces you to write working code.

dnlmlr

@dnlmlr

The actual real life problem I see with the optional values being siletly defaulted is something like this:
Imagine you want to print some info on a track that is being played. I would rather have to match an option than printing invalid data. If a track for example has no track number, I'd want to just skip that, or fill it with a sensible "not present" or something

Roderick van Domburg

@roderickvd

By all means come up with a proposal. I'm not against it at all, just didn't have the itch to come up with something smart.

dnlmlr

@dnlmlr

I'll postpone the optional stuff and think about it a bit more, but I'll be sure to make a proposal if I come up with something sensible.

Another point: Should the wrapper types (like Tracks) implement DerefMut in addition to Deref? I think that would be consistent and allow for basically full transparency of the types, even with mutable functions. If there is no reason against it, I'll make a PR for it

I also think that this could be done pretty well with a simple macro that takes the outer and inner type and implements Deref and DerefMut at the same time. This would further slim down the boilerplate for those wrapper types

dnlmlr

@dnlmlr

I just created a PR for the DerefMut stuff using a macro. I was going to try it out for myself anyways so I figured I'd just create the PR :)

Roderick van Domburg

@roderickvd

No reason against from me. Thanks for the PR, good idea. Let’s discuss there.

dnlmlr

@dnlmlr

Could you elaborate on the part of the comment about the playlist: https://github.com/librespot-org/librespot/pull/1036#issuecomment-1201639830

In playlist/list.rs you see that the Playlist struct is decorated on top of an inner SelectedListContent. Maybe that can serve as an example for having inner structs.

I have to admit that I generally don't quite understand the difference between Playlist and RootPlaylist. As far as I can tell they contain mostly the same data (RootPlaylist just wrapping the SelectedListContent). Compared RootPlaylist is missing the idand Playlist is missing the owner_username

2 replies

Michael Herger

@michaelherger

AP lookup issues continued: "classic" librespot had an AP_FALLBACK, in case the lookup failed. This seems to have been removed in new-api. If I fail to reach that host, I'm getting an exception. Is this by design or shall I raise an issue?

21 replies

dnlmlr

@dnlmlr

Hey! Where does the list of endpoints (like /playlist/v2/playlist/) come from? I actually can't manage to find this. Also the hm:// prefix is for usage with mercury instead of http?

Roderick van Domburg

@roderickvd

Yes, hm:// is the Mercury prefix. (Hermes Mercury? I don't know.)

Those new endpoints I ported from librespot-org/librespot-java@6b45130 but that playlist one actually keeps on returning HTTP 400 for me when running the example.

I don't have time anymore to investigate further. It may be that we need to set the ACCEPT header specifically to application/x-protobuf or application/json or something else may be wrong, like the brand new client-token header. Honestly I am assuming this thing in librespot-java is working, I did not test that either.

Roderick van Domburg

@roderickvd

Second point, I would like to migrate to the HTTP Apollo radio resolver in spirc for the context_fut and autoplay_fut but it will require quite some refactoring and "fun" with futures.

dnlmlr

@dnlmlr

Thanks for the infos.
I just took a quick look at the http 400 failing request and found that the issue was basically a typo. The enpoint included the debug printed Option(id) instead of the inner id value. I made a small PR to fix it, but it's basically trivial

Roderick van Domburg

@roderickvd

How terrible 🙈 thanks

dnlmlr

@dnlmlr

I took the opportunity of the playlist mercury -> http switch did some very basic testing. In my tests the mercury endpoint was on average more than 4 times faster then the http endpoint for querying a playlist. The time for http from call to data was 0.6 to 0.7 seconds and for mercury it was only 0.13 to 0.3 with the average hovering around 0.6 and 0.15 .
From a technical standpoint (with me knowing literally nothing about mercury) it would also make sense that http with totally separate requests and a very large per-request overhead (headers, tls, ...) would be slower than using an already open channel

Just putting this information out there for now

dnlmlr

@dnlmlr

I got the idea to test this because I noticed that the playlist example felt kinda slow when loading the track-metadata. And it actually takes about 0.2 secs with http / track vs 0.06 sec with mercury. For a larger playlist like this (randomly picked) spotify:playlist:37i9dQZF1DWZeKCadgRdKQ with 180 tracks the difference is very noticable.
It takes 11 seconds to pull all track-metadata with the old mercury requests and a whopping 38 seconds with http

11 replies

dnlmlr

@dnlmlr

In a (currently experimental and not public) application using librespot implemented a metadata cache. That could help at least if the same tracks get queried a again. But I'm not sure how you would decide when to drop stuff from the cache. In my case the cache was purely in-memory. Track Metadata could probably be kept almost indefinitely, but especially actual playlist metadata could be relatively volatile with changing tracks or orderings. Also that would of course do absolutely nothing for the initial queries

Roderick van Domburg

@roderickvd

You are right. Mercury is super lightweight TCP. HTTPS not only has more headers for small requests, worse is that the SSL/TLS setup and handshake is expensive. A good optimization could be to keep a connection open and reuse it.

dnlmlr

@dnlmlr

Good point. I get about double the performance when using a single hyper Client compared to building a new one on each request. The only problem is how to store that client, considering the (in cases like this really annoying) way how it's generic over connector and body (Client<C, B>). The fact that there are two different connectors (normal Https and Proxy<Https>) makes it pretty hard

dnlmlr

@dnlmlr

One option could also be to always use ProxyConnector and just set Intercept::None with something like uri=0.0.0.0 when not using a proxy. That would introduce error handling into HttpClient::new since URI parsing and ProxyConnector::new can technically fail

dnlmlr

@dnlmlr

I made PR with the changes I talked about in the last comment. I switched to lazily initializing the client to avoid the Result return type for new. You can check it out and see if you like it, or maybe use it as inspiration if you dont ;)

Michael Herger

@michaelherger

Has anybody recently tried to build for Linux/i686? I'm failing it, likely due to briansmith/ring#1262.

Roderick van Domburg

@roderickvd

Would like to add to this discussion that the HTTP client is used for much more than metadata. Of note also for downloading files, multiple chunks in parallel, and this is an area ripe for optimization. There was some discussion on this in the repo with the new-api discussion. Great first optimization this, let’s continue from here!

Have not tried i686 myself nor do I have a host running that.

Michael Herger

@michaelherger

And another observation: I'm using the tokens received by librespot to access the web API. Now when I call eg. /me/player/devices to get a list of connected devices, I'd get an empty list back. If I replaced the KEYMASTER_CLIENT_ID with what is being used on https://developer.spotify.com/console/get-users-available-devices/, I'd get the correct list of devices. I tried two of my own client IDs, and they would work, too. Has the one we're using by default been burnt?

7 replies

It wouldn't be rejected, but does not give access to all endpoints?

Roderick van Domburg

@roderickvd

Another option is that we need to request another scope when we request with the client ID — that would actually make sense and we could revert part of that commit.

14 replies

It currently only asks for playlist-read.

Michael Herger

@michaelherger

Ok, here's my test:

cargo run --example get_token <user> <password> | fgrep access_token | export TOKEN=$(cut -d '"' -f2) && curl -X "GET" "https://api.spotify.com/v1/me/player/devices" -H "Accept: application/json" -H "Content-Type: application/json" -H "Authorization: Bearer $TOKEN"

With the default client ID baked in I'd always get an empty device list:

{
  "devices" : [ ]
}

If I replace KEYMASTER_CLIENT_ID with my own client ID, I get the expected result:

{
  "devices" : [ {
    "id" : "...",
    "is_active" : false,
    "is_private_session" : false,
    "is_restricted" : false,
    "name" : "My Librespot Player",
    "type" : "Speaker",
    "volume_percent" : 49
  } ]
}

Michael Herger

@michaelherger

With your recent change session.set_client_id() somehow becomes irrelevant, as the value is ignored anyway?

Roderick van Domburg

@roderickvd

That makes sense because the keymaster ID is not associated with the playback devices of your user of course. However if you change that piece of code in the commit I linked to so the client ID is reintroduced just for the “normal” token getter is all well, also for the playlist example?

8 replies

Michael Herger

@michaelherger

Totally different issue today, with the old code: I've got an application based on early June dev code. It has been used by thousands of users, incl. myself since then. Yesterday my server suddenly started to behave oddly, until I figured out that rsyslogd was filling up my system with logs coming from librespot (my application is launching librespot from Perl, therefore the "perl" in the log):

Aug  8 14:10:20 musicpi perl[5569]: thread 'main' panicked at 'Spotify servers returned an error. Restart librespot.', core/src/mercury/mod.rs:205:13
Aug  8 14:10:20 musicpi perl[5569]: stack backtrace:
Aug  8 14:10:20 musicpi perl[5569]:    0:   0x75c958 - <unknown>
Aug  8 14:10:20 musicpi perl[5569]:    1:   0x49c258 - <unknown>
Aug  8 14:10:20 musicpi perl[5569]:    2:   0x75bec0 - <unknown>
Aug  8 14:10:20 musicpi perl[5569]:    3:   0x75b8dc - <unknown>
Aug  8 14:10:20 musicpi perl[5569]:    4:   0x75ad34 - <unknown>
Aug  8 14:10:20 musicpi perl[5569]:    5:   0x51c810 - <unknown>
Aug  8 14:10:20 musicpi perl[5569]:    6:   0x51c810 - <unknown>
Aug  8 14:10:20 musicpi perl[5569]:    7:   0x51c810 - <unknown>
...

After that the last line was repeated 64M times before I shut things down...

Aug  8 15:26:46 musicpi perl[5569]: 64354566:   0x51c810 - <unknown>
Aug  8 15:26:46 musicpi perl[5569]: 64354567:   0x51c810 - <unknown>
Aug  8 15:26:46 musicpi perl[5569]: 64354568:   0x51c810 - <unknown>
Aug  8 15:26:46 musicpi perl[5569]: 64354569:   0x51c810 - <unknown>

It looks as if that panic was causing an infinite loop or something in librespot. Anybody else seen this? Today it happened again. As if their continued dismantling of old infrastructure was causing more issues :-(

Unfortunately I'm not able to reproduce this at will.

Michael Herger

@michaelherger

Hmm... what's odd is that I'd get a stack trace, even though I'm using the abort panic mode. I thought that would suppress the backtrace?

Jason Gray

@JasonLG1979

I get the error every once in a great while. I don't run a stripped binary so I actually get a backtrace. It's a 500 error code for me so server error.

Jason Gray

@JasonLG1979

It my case librespot always just crashes and systemd just restarts the service. It's a very minor inconvenience like once every couple months.

Jason Gray

@JasonLG1979

abort also exits without any sort of cleanup, as in it dies without releasing it's allocated memory leaving that up to the OS. (0x51c810 is a memory address) There could be some funkyness going on because of that? I personally would not distribute stripped or abort binaries, because although they will be very tiny they're basically useless for debugging so you end up preventing the user from help you help them if that makes sense? A log full of 0: 0x75c958 - <unknown> is pretty useless.

Michael Herger

@michaelherger

TBH: I would not even have expected those entries in the log, as I thought abort would not give a backtrace at all. But that probably was a misunderstanding. The reason why I opted for the stripped version is that I provide binaries for multiple platforms (Mac Intel/ARM, Win, Linux ARM/686/x86_64...). Even compressed this makes for a lot of data. I'm now using a CDN, so this is not that much of a problem any more (my hosting service didn't like it). But OTOH I've been doing this for 5+ years without any regret - until a week ago.

Jason Gray

@JasonLG1979

Host it from a gitub.io page?

Jason Gray

@JasonLG1979

The Raspotify apt repo is hosted from a github.io page.

Roderick van Domburg

@roderickvd

What’s your project called? MusicPi? The website says that was discontinued so probably something else?

2 replies

Where communities thrive

People

Repo info

Activity