Skip to content
This repository has been archived by the owner on Jan 22, 2022. It is now read-only.

Add support for free radio stations #552

Closed
foreverguest opened this issue May 23, 2017 · 29 comments
Closed

Add support for free radio stations #552

foreverguest opened this issue May 23, 2017 · 29 comments

Comments

@foreverguest
Copy link
Contributor

Hi,

I have added support for free radios on the Kodi addon adding the missing calls in gmusicapi and changing others, please check fork below:
develop...foreverguest:develop

But this changes the data returned by create_station function, is that ok?
Do you suggest another approach?

@thebigmunch
Copy link
Contributor

This sounds like it should be a separate method from get_stream_url. While get_stream_url is a generic name, it is a different action semantically than what is going on with this call. Commenting out an intentional check for something in get_stream_url is a pretty clear sign of this.

You say this is for getting a stream URL from a radio station using a free account. Are you sure this is only for free accounts and not just a way to get a stream URL for station tracks in general? Either way, it would be better to name the call and the protocol class appropriately to better signify what it does.

You didn't add session_token nor wentry_id to the docstring; do you know what they are, specifically wentry_id? Where do you get them?

I'm curious as to why a change in the create_station method is part of the change regarding getting a stream URL from a radio station. Or why it's needed at all. Keep in mind that returning the station dict rather than the station id would be a breaking change requiring a major version bump. As of now, you can simply get the tracks using the id from create_station as the argument to get_station_tracks. In my opinion, it doesn't necessarily make sense to do this to the create_station method.

Unfortunately, my phone is on Nougat right now, so I can't investigate the call myself at the moment : ( Looking forward to hearing more about this call, though.

@simon-weber
Copy link
Owner

Thanks! I've been super busy lately, but I hope to have a chance to look at this over the weekend.

@foreverguest
Copy link
Contributor Author

foreverguest commented May 24, 2017

@thebigmunch

This sounds like it should be a separate method from get_stream_url. While get_stream_url is a generic name, it is a different action semantically than what is going on with this call. Commenting out an intentional check for something in get_stream_url is a pretty clear sign of this

I have commented out the check because the track_id starting with 'T' can be returned to free accounts in this case.

You didn't add session_token nor wentry_id to the docstring; do you know what they are, specifically wentry_id? Where do you get them?

They come in the station dict, example code:

station=gmusicapi.create_station(name,track_id,artist_id,album_id,genre_id,playlist_token,curated_station_id)
sessiontoken = station['sessionToken']
trackid = station['tracks'][0]['storeId']
wentryid = station['tracks'][0]['wentryId']
url = gmusicapi.get_stream_url(trackid, session_token=sessiontoken, wentry_id=wentryid)

I'm curious as to why a change in the create_station method is part of the change regarding getting a stream URL from a radio station. Or why it's needed at all. Keep in mind that returning the station dict rather than the station id would be a breaking change requiring a major version bump. As of now, you can simply get the tracks using the id from create_station as the argument to get_station_tracks. In my opinion, it doesn't necessarily make sense to do this to the create_station method.

The mobile client uses the same 'radio/editstation' call used by create_station for free radios, I just wanted to use the same call.

So, you think creating a new get_stream_url_free and get_free_station is the better aproach?

@thebigmunch
Copy link
Contributor

I understood why you commented out the check. My point was that having to do that is a clue that it should probably be a separate method. Another is that you had to create a separate class in the protocol layer for it. I think the main reason you're re-using get_stream_url is because it was named so generically due to it being implemented long before most of Google Music's current functionality existed. If it were named now, it'd likely be more specific to its actual functionality.

The thing is, if this is for streaming from a station, station should be in the name. get_stream_url is for streaming songs from the user library or from the store. At least that's my opinion. I don't think free really needs to be part of the name, probably even if this is only for free accounts; that could be in the docstring. I don't think there are "free" stations; you can listen to stations without a subscription, just with some limitations. Is wentryId in all station tracks regardless of subscription status? This call probably works for both free and subscribed accounts. Have you tested that?

As much as I can suss out right now, probably the better naming is get_station_[entry|track]_stream_url on the client level and GetStation[Entry|Track]StreamUrl on the protocol level.

The mobile client uses the same 'radio/editstation' call used by create_station for free radios, I just wanted to use the same call.

But do you need to change anything there to use it? You don't actually change anything but some parameter values and returning the full station dict instead of just the ID. Can you not get the station tracks by doing as I said before: using get_station_tracks with the station id from create_station?

Simon will give his opinion after he looks it over.

@thebigmunch
Copy link
Contributor

Is wentryId in all station tracks regardless of subscription status? Have you tested that?

Just tested this myself. It is only present for free accounts.

But do you need to change anything there to use it? You don't actually change anything but some parameter values and returning the full station dict instead of just the ID. Can you not get the station tracks by doing as I said before: using get_station_tracks with the station id from create_station?

You can, in fact, get the station tracks by the method I've described. But, you never clearly said why you changed it. It is so you can get the session token, right? A better way to do this might be to add a get_station_info method that returns the full station dict in the same vein as get_artist_info, get_album_info, and get_track_info. This would keep both backward compatibility and consistency with the returns of other methods like create_playlist.

As much as I can suss out right now, probably the better naming is get_station_[entry|track]_stream_url on the client level and GetStation[Entry|Track]StreamUrl on the protocol level.

I'm pretty sure these should be get_station_track_stream_url and GetStationTrackStreamUrl with proper documentation of it being for free accounts, how to get the necessary keys, and that subscribed accounts should use get_stream_url.

@NonaSuomy
Copy link

NonaSuomy commented May 26, 2017

Looking forward to this inclusion, thanks a lot! 👍

@simon-weber
Copy link
Owner

Nice work figuring this out! I'm +1 to thebigmunch's suggested changes. Listing them out, I think it's:

  • move new client logic to get_station_track_stream_url and protocol logic to GetStationTrackStreamUrl; leave get_stream_url and create_station as before
  • add get_station_info for the purpose of retrieving session_token + wentry_id

Want to open a PR with the changes?

@thebigmunch
Copy link
Contributor

thebigmunch commented May 29, 2017

A note: schemas should probably be updated even though there are no exceptions due to #492.

There will have to be a station track schema that can copy the track schema and adds wentryId. Stations will need to be updated with that, the new sessionToken key, and enforcementResult which might need some more looking into but isn't technically related to this.

The get_station_info call can use the radio/stationfeed endpoint used in the ListStationTracks protocol class (kind of a misnomer). It returns a list of station dicts. You'd use it like in get_station_tracks but without recently_played and just return the station dict.

I could probably bang this out sometime this week if you're not comfortable with the changes needed.

@NonaSuomy
Copy link

@foreverguest are you ok with @thebigmunch implementing the changes?

@foreverguest
Copy link
Contributor Author

Sure, I would give a shot working on the changes, but I'm a little busy at the moment.

@NonaSuomy
Copy link

@thebigmunch are you able to proceed with the modifications?

@NonaSuomy
Copy link

If anyone is able to proceed with the modifications it would be appreciated! 😸

@thebigmunch
Copy link
Contributor

Sorry, @NonaSuomy, I was so busy last week I forgot to respond. I definitely didn't have the time : P I'm not sure when I'll have some extra time again to do it either.

@foreverguest
Copy link
Contributor Author

Ok, did the changes you asked:
develop...foreverguest:develop

@thebigmunch
Copy link
Contributor

Just from a cursory glance, you don't need song_id as a parameter as it isn't used. wentry_id should be in that position. This is what was being pointed out before. This is a separate thing than get_stream_url, so you should not be trying to do both things in the same methods/protocol classes.

With that there needs to be a note in the docstring that the method is for free accounts only and point subscribed users to the get_stream_url method using the song ID instead.

And the new methods need to be added to the mobile client docs index. And some tests need to be added.

I'm on my phone, so I haven't looked at the info method yet.

@simon-weber
Copy link
Owner

Cool, this is looking pretty good. Thanks!

I agree there's a few odds and ends to clean up, but it's probably a good time to open a PR with the existing changes. We could either discuss the suggestions there or decide to merge the meat of it and fix up the rest later (which I've got time for this weekend).

@foreverguest
Copy link
Contributor Author

Just from a cursory glance, you don't need song_id as a parameter as it isn't used. wentry_id should be in that position. This is what was being pointed out before. This is a separate thing than get_stream_url, so you should not be trying to do both things in the same methods/protocol classes

Probably I'm missing something, I tried a version replacing song_id with wentry_id, but always receive a Bad Request answer.

@thebigmunch
Copy link
Contributor

thebigmunch commented Jun 15, 2017

Hmm, I assumed that the song ID wouldn't be needed for the call on Google's end. Seems odd but wouldn't be surprised. I tried using your changes as-is on my free account, but I kept getting a 403. May just be too early in the morning. Looking forward to testing it out more later. I'd still move wentry_id directly after song_id, since they are related and ID parameters generally come first in calls.

@thebigmunch
Copy link
Contributor

thebigmunch commented Jun 15, 2017

From playing around quickly, I didn't notice I wasn't getting a redirect from this call like the other stream calls. We instead getting back a JSON response with a location key among others.

@thebigmunch
Copy link
Contributor

thebigmunch commented Jun 15, 2017

For the curious, the keys I'm seeing in the response are:

'activeFeatures',
'experimentIdsToLog',
'isFreeRadioUser',
'itag',
'location',
'now',
'obfuscatedIdStr',
'replayGain',
'sessionToken',
'subscriptionTier',
'streamAuthId',
'trackDurationMs',
'url'

Some of these make me wonder if the other stream calls might be changed in the future. But it could also just be Google's servers being stupid. Or it could just be Google being inconsistent again.

@NonaSuomy
Copy link

NonaSuomy commented Jun 19, 2017

Well, they did tack on the Songza service they bought to their music service, so likely inconsistency between the two.

@thebigmunch
Copy link
Contributor

thebigmunch commented Jun 19, 2017

Free account station streaming has always been there.

The Songza stuff is what are called Situations in Google Music. Situations were implemented as stations, but they don't have any relation to this.

Edit: This response almost seems more like an internal thing to me.

@NonaSuomy
Copy link

Hopefully, it gets figured out!

@NonaSuomy
Copy link

NonaSuomy commented Jun 23, 2017

@foreverguest are you able to submit the Pull Request as @simon-weber was requesting =>
https://github.com/simon-weber/gmusicapi/pulls

@simon-weber
Copy link
Owner

Thanks; I cleaned up a few things and merged it!

I'd still like to add tests before cutting a release with this in it, but the folks waiting on support can go ahead and try integrating off the develop branch.

@NonaSuomy
Copy link

NonaSuomy commented Jun 29, 2017

@foreverguest and @simon-weber keep up the great work! Please post back when it is in release as well. Thank you!

@NonaSuomy
Copy link

@simon-weber is this in the release now?

@simon-weber
Copy link
Owner

Yup, in 11.0.0. Sorry that I forgot to clean this up.

@NonaSuomy
Copy link

Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants