refactor: Introduce SpotifyUri struct

[SapiensAnatis]

Contributes to #1266

Introduces a new SpotifyUri struct which is layered on top of the existing SpotifyId, but has the capability to support URIs that do not confirm to the canonical base62 encoded format. This allows it to describe URIs like spotify:local, spotify:genre and others that SpotifyId cannot represent.

Changed the internal player state to use these URIs as much as possible, such that the player could in the future accept a URI of the type spotify:local, as a means of laying the groundwork for local file support.

---

I know #1266 is not a very good issue for my first one on the project, but I would really like to implement local file support and it doesn't seem like that will be possible for the player to handle without hacks unless we change the way we store the track URI. This is because local files URIs are of the form spotify:local:{artist}:{album_title}:{track_title}:{duration_in_seconds} which cannot be represented under a u128 inside SpotifyId like the base62 names that all official tracks have.

I have opened this PR to show what the proposed refactor in #1266 would involve, particularly with regard to the breaking changes I have had to make. So there are probably some things that can be polished more and tests added, but I wanted to get some feedback on the overall direction first. If the proposed change is too radical I will not be offended if you decide to close this PR :slightly_smiling_face:

p.s. I have put all the changelogs etc. as 0.7.0 but it sounds like that is coming soon so just to be clear I have no expectation of this making it into the upcoming release

[photovoltex]

I know https://github.com/librespot-org/librespot/issues/1266 is not a very good issue for my first one on the project

I mean what is, right? So initial thanks for the work on this front, we will probably take a look over in the next few days. But I think the general idea is a good direction to get this issue rolling :D

[photovoltex]

So I had a look over the PR and the general issue. This is quite a tricky problem we have here which heavily relies on always copying the id which is probably fine, but seems rather like a workaround then a good solution, as it is quite restricted. I will think a bit more about how we could tackle it without relying too much cloning.

What you could try to improve right ahead. It seems that a lot of SpotifyUri impl is copied out of SpotifyId. It would be better if you forward the methods from the underlying structs and handle only the non existing cases in the wrapper.

[SapiensAnatis]

@photovoltex Yes I wasn't happy with all the cloning I had to do either. But given the event based architecture of the player it is not possible to hand around references since usually the SpotifyUri ends up in an event struct added to the queue which needs to own it, unless we want to start messing with lifetimes. Maybe there is a better way to do it and I would be grateful if someone more experienced than me could spend some time on it.

I am away this weekend but will look to reduce the duplication in SpotifyUri when I get back -- I assume you're talking about the parsing and formatting methods?

Long term it would be nice if we could make SpotifyId just the u128 as that would map closest to the underlying concept, and in that case most of the formatting and parsing would only be on SpotifyUri, but I thought that was too much for one PR.

[SapiensAnatis]

I have removed some duplication in to_uri and done some misc. cleanup in https://github.com/librespot-org/librespot/pull/1538/commits/0b2688d7508bee1af63691271b90207afafd099f.

However, I can't see how to remove the duplication in from_uri since we need to split the URI and check the type anyway to determine if the URI is one that can be forwarded to SpotifyId. As soon as we have checked the type and checked it starts with spotify: we have already basically duplicated SpotifyId::from_uri.

Perhaps we could try SpotifyId::from_uri always and start returning errors from that for unsupported types, then handle those with alternative parsing?

[photovoltex]

Do we even need the from_uri parsing in SpotifyUri anymore? It's probably still necessary because we currently need the type of the item in SpotifyId, right?

Maybe wrapping is in the end more hurtful than helpful and to get this cleanly done we need to break a lot of the existing api.

I also tried a bit over the last few days to work on removing the copy trait of the existing id (see my fork the chore branches), but only ended up refactoring a lot of the player loop and still had to add a few clones to get it all working.

Overall, I can see why this was pushed back for a while^^;

[SapiensAnatis]

Do we even need the from_uri parsing in SpotifyUri anymore? It's probably still necessary because we currently need the type of the item in SpotifyId, right?

Yes we do, it's used in e.g. the TryFrom<&protocol::playlist4_external::Item> and also a bit in the metadata and connect crates. I didn't want to touch those, especially metadata as I imagine our mechanism for getting metadata for local files will follow an entirely different path.

Was your extracted player loop branch more efficient or are we still doing the same amount of cloning?

Is it worth me trying to go all the way -- removing the item_type from SpotifyId and doing a clean break, and seeing how many APIs that would break and what that would look like? Note: I still think it's beneficial to keep the SpotifyId struct around in a union, as if you can represent the ID as a u128 I think when you do need to clone you won't need another heap allocated String.

[photovoltex]

Was your extracted player loop branch more efficient or are we still doing the same amount of cloning?

With mine we are at 15 clones, but that's only 3-4 less then your state. So better, but not in the region where it would be a huge benefit I think?

I still think it's beneficial to keep the SpotifyId struct around in a union

Totally agree, separating the types by resource still makes a good amount of sense.

[SapiensAnatis]

With mine we are at 15 clones, but that's only 3-4 less then your state.

Interesting, good you managed to remove a few but yes I feel the player relies heavily on copying the IDs. And I don't see a clean way we can use references + lifetimes unless we want to do something silly like store every URI the player has ever played.

[photovoltex]

Just reduced some more. But for the event system we will not come around cloning as well for some state management. They just need to own the whole thing.

I though a bit about alternative solutions like reworking the event message system to a handler system that could be restricted by a trait. I'm not so sure if that is a good alternative we would want to use, but it would solve the problem with the message based system where it always has to own the object.

[SapiensAnatis]

A handler system sounds interesting. It would make this an even bigger change but we could always split things up with that change coming first.

I probably need to understand the architecture and inner workings of the player a little bit more. I will study it over the next week or so and see if I have any flashes of inspiration.

[photovoltex]

So, I just implemented what I meant with the handler/listener system here https://github.com/photovoltex/librespot/commit/1f1e684453f58057f200283d51b130b7302a3578 (based on my player refactoring, so not a direct commit on the current state). This has currently a huge drawback that the event's are invoked synchronous, so when a handler makes any blocking action this might impact the player performance, but out of the box this seems to work quite well (but I'm on my main desktop so it's not a real comparison to a raspi for example).

The event_handler of the binary hast to clone quite a bit more, but that was done previously from the lib side so it doesn't make a big difference there. But a lot of clones internally I could be dropped with that change.

@roderickvd, @kingosticks, do you have any hard feelings if we would replace the channel system here? This probably will annoy some users of the lib as it is a major breaking change, but it might be worth it and would go more in the direction of less cloning and better memory management from the lib side.

[roderickvd]

No hard feelings up front. The audio is running in a separate thread. And you could still spin threads for tasks that could run too long?

Just to put it out there: In pleezer, I’ve done another hybrid I guess. Although the architecture is different so you can’t fully compare: the remote (which is like the dealer/spirc) has ownership over the player, so just dispatches websocket messages and controls the player as it wishes. This is also “synchronous” in the sense that it’s sequential, which I guess is what you mean. Then the player can fire “events”, to which the remote is subscribed as listener, to notify the remote (like when the next track starts playing). The event channel gets sent light-weight enum variants with as little data as possible.

[photovoltex]

The event channel gets sent light-weight enum variants with as little data as possible.

Yeah we had that before/currently, but when updating the uri/id to contain a string and by that removing the derived copy trait, it isn't as lightweight anymore. Especially when you have to clone it everywhere. With the handler/listener system you gain direct access from the player side and can handle the borrowed event, which in it self holds only a ref to the related id/uri. By which almost no clone is necessary from our side in regards to uri/id.

But that comes with the problem that this will run on the same thread where the audio packet is handled, which what I thought is the audio thread? Or is there another thread I didn't see, yet?

[roderickvd]

Yes, so we just need to work fast enough to fill up the audio sink buffer. If that turns out to be a problem, various strategies:

1. Increase audio buffer size (with that, also latency)
2. Move packet handling into separate thread
3. Schedule packet handling in the same select loop as the event handler

Number 3 wouldn’t be so bad if that’s doable?

[kingosticks]

But is this all really worth it to avoid some copying? The event frequency is always going to be relatively low, isn't it?

[SapiensAnatis]

But is this all really worth it to avoid some copying? The event frequency is always going to be relatively low, isn't it?

This would be my question as well but I didn't want to sound like I was making excuses for my implementation :sweat_smile:

I think if my understanding of Rust is correct then the clone for the new SpotifyUri struct should be a similar bitwise copy as we had for SpotifyId, except with a branch midway through to check if the name is a String or u128. This will always be a u128 unless it is a local file. When we start supporting local files with the String name, then it's true that each clone would incur a heap allocation. But I'm still not sure that this would lead to a performance problem given the event frequency.

edit: we could also perhaps mitigate the String case with an Rc/Arc so we don't allocate each time?

edit 2: another idea I had -- although this may be a bit out there -- is that if we scan all the local files on startup and determine their SpotifyUri, then when we receive an event we can try a hash-map lookup of the uri -- if found we could store a reference to the hashmap key where the event has a Cow<'static, SpotifyUri>, where it would be owned for other usages where this lookup isn't possible or fails

[photovoltex]

kingosticks: But is this all really worth it to avoid some copying? The event frequency is always going to be relatively low, isn't it?

In the end, it probably depends on what we aim for. Those clones are probably very minor in terms of performance and memory usage (depending how many mpsc channels we have). But they go in it self against the borrow/ownership concept and just avoid it to a degree. And that is totally fine, if we aim to build a working lib that doesn't care to much about it, but I personally like to build with the limitation and adjust the code around it (and I finally have some free time currently so I just had fun refactoring it :D).

So it's less of a "do we need it" or "is it worth it" (it kinda is, but not all of it) and more of a "do we want it". My refactoring branches are all just proof of concept that just display what it could be (so we can take a look on "what could it be"). They would in terms of refactoring SpotifyId help quite a bit, but are not necessary in the end. That's why I ask if anyone has a problem with changing it here and what's your opinion is on the topic^^

SapiensAnatis: This would be my question as well but I didn't want to sound like I was making excuses for my implementation 😅

No worries just speak your heart out, but keep it respectful. I'm just having fun optimizing :). I think initially we will probably go with this solution to some degree. I just wanted to see where this topic could go if I started refactoring the player loop (which I previously did, but never finished).

The Hashmap solution sounds interesting, but Rc/Arc I would like to avoid with the same reasoning as above. But we don't need to go such length if everyone is fine with the amount of clones we add here.

roderickvd: Yes, so we just need to work fast enough to fill up the audio sink buffer.

Yeah, that's exactly the problem that might occur with the handle/listener solution. I would probably see 2. as easiest. For 3. I'm not so sure what you mean, but let's drop this detailed discussion for now, I will open PR for it, if previous discussion/topics are cleared if it is at all wanted or reasonable to do.

[SapiensAnatis]

Yeah, I can see how clone can be seen as an escape hatch from writing really nicely reasoned out code with proper lifetimes and all. Thank you for spending all this time you have on experimenting with trying to improve things, hopefully we can use some of that to make things even better.

I suppose it depends on what we think about the aforementioned major overhauls but what are the next steps for this PR? I asked earlier if I should go 'all the way' and remove the item_type from SpotifyId as well, and then make the crate use SpotifyUri in metadata etc. as well?

[photovoltex]

I asked earlier if I should go 'all the way' and remove the item_type from SpotifyId as well, and then make the crate use SpotifyUri in metadata etc. as well?

I would say yes, a primary object to use is probably better then multiple ids that are used for various situations (see NamedSpotifyId for example, which was for the time a good solution).

[SapiensAnatis]

Ok, I will begin working on that approach. It will be quite a lot of changes but I'm sure we will be glad to have one ID type across the codebase that we can use for everything.

Speaking of NamedSpotifyId, I have just come across that. Do we want to alter the struct of SpotifyUri then to be

pub struct SpotifyUri {
    pub item_type: SpotifyItemType,
    pub user: Option<String>,
    pub name: SpotifyResourceName,
}

or should we keep NamedSpotifyId and have it as an entry in SpotifyResourceName?

I think the first one because it doesn't seem we ever really use the username in the ID, so it would be unwise to introduce an additional match arm on SpotifyResourceName for all consumers

[photovoltex]

Thinking about it, why not make SpotifyUri an enum altogether? Most only need a u128 (or SpotifyId) value, but TrackLocal, Collection and some others might need a different structure which you could easily display via a enum (struct).

But I would probably also remove NamedSpotifyId or at least deprecated it for the next version to give a single way to display a spotify uri.

[SapiensAnatis]

Thinking about it, why not make SpotifyUri an enum altogether?

Presumably then each member would have to have its own type field (e.g. track/album/artist)? We know all URIs have a type and maybe a user, and then some ID representation which is indeterminate.

[photovoltex]

Not quite, the type field would become the enum variant itself. So a quick example I had in mind would look something like this:

enum SpotifyUri {
    Collection { user: String },
    LocalTrack {
        artist: String,
        album: String,
        name: String,
        position: u32,
    },
    Track { id: u128 },
   ...
}

edit: But this is just a thought. If the other version would work better stick with that, so no need to build it like this^^

[SapiensAnatis]

It's an interesting idea, but I feel like you would end up repeating { id: u128 } for a lot of variants -- presumably Album, Artist, Episode, Playlist, Show, and Track? I'm concerned about how you could share the logic for parsing to/from base62 between those variants.

However it does have the advantage that presumably a function signature could call for a specific type of URI? It certainly helps primitive obsession because it makes it impossible to e.g. assign a Track id to a Playlist.

[photovoltex]

It's an interesting idea, but I feel like you would end up repeating { id: u128 } for a lot of variants

Either you repeat it or you have combinations that don't make to much sense. You could also use a struct as the id type Track { id: SpotifyId } or just a tuple of the type Track(SpotifyId) or Track(u128).

[SapiensAnatis]

Still (slowly) working on this, I now have a build where the metadata crate and the Metadata trait uses SpotifyUri throughout. However, I kept spclient as using SpotifyId, since presumably if you are requesting data from Spotify you will be sending an ID that Spotify recognises. It's still worth converting trait Metadata though over in case we need an implementation of that trait for local files later.

I need to do some more tidying up, fixing the warnings from CI and then add some tests most likely. @photovoltex I will also try your DU suggestion since once we have removed the dependency on IDs being Copy-able it will be easier to implement and evaluate.

[SapiensAnatis]

Rough draft of making the struct a union here -- it builds and works. It was much easier than I thought, turns out a lot of logic doesn't really care what item type we have as long as we can get a string/URI representation out of it:

https://github.com/SapiensAnatis/librespot/commit/150000b7169c910336b9d2e4fa8db47a8407a663

However, that being said I'm not sure I really see the benefits here. Most of the time we don't destructure the ID out since we want to use the SpotifyUri everywhere; particularly around metadata where the trait requires all methods to take a SpotifyUri. Plus this approach is less flexible in that if there was an item type whose IDs could be either a SpotifyId or String, then you would be forced to encode the ID always as a string.

Also,

However it does have the advantage that presumably a function signature could call for a specific type of URI

I was wrong about this, it turns out that variants aren't fully fledged types so we can't actually do this without making separate structs for each variant.

[photovoltex]

Plus this approach is less flexible in that if there was an item type whose IDs could be either a SpotifyId or String, then you would be forced to encode the ID always as a string.

But for that reason the new struct exist, right? So everywhere where previously SpotifyId was used, would be the URI from now on. At least that was my understanding of the situation. By doing that, you can reduce SpotifyId to only be a wrapper of u128 and the type itself is delegated to SpotifyUri.

The main advantage of the enum I see is, that each variant describes what it's made of and the type is already decided by the variant itself, so SpotifyItemType could be removed later on.

But as previously stated, it's just an idea. If you find the struct with additional values works better then go that way for now :)

[SapiensAnatis]

The main advantage of the enum I see is, that each variant describes what it's made of and the type is already decided by the variant itself, so SpotifyItemType could be removed later on.

Yeah, I can see it, I think it is a nicer solution than SpotifyResourceName::NamedId in particular. But on the flip side there were a few times I had to match on every single variant of SpotifyUri which could become a pain and means adding new item types would be a breaking change to consumers because of exhaustiveness (maybe we could use #[non_exhaustive]).

It begs the question whether, given the local file URI is

spotify:local:{artist}:{album_title}:{track_title}:{duration_in_seconds}

would we actually have

enum SpotifyUri {
    // ...
    Local { artist: String, album_title: String, track_title: String, duration: u32 }
}

In that case, I can see the benefits since if you get a local file URI it is already parsed and ready to go for you. And unlike with SpotifyResourceName there is no possibility that you somehow end up with a SpotifyUri { item_type: Artist, name: SpotifyResourceName::LocalFile(...) }

So... it's kind of growing on me :) Will think about it some more.

[photovoltex]

In the example here I had displayed that idea of splitting the local playback hehe. But yeah that would be another benefit of it :D

I find the matching not that bad (you can always use _ to match all others or if let to match a single) and it kinda helps when migrating versions as you are forced to match the new type when you matched all previously. There is also the matches! macro which can be used to just do a "is it this type" check. But yeah it would be a breaking change when a new type is added, but this would happen anyway when a type is matched and we add a new one.