-
Notifications
You must be signed in to change notification settings - Fork 111
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
Implements PKCE Authorization to enable access to HiRess files. #221
Conversation
Thanks, will look at this ASAP, need to test with a HiFi enabled account. |
Let me know if you need any help on this. |
@tehkillerbee: Any updates on this? |
fn_print(url_login) | ||
|
||
# Get redirect URL from user input. | ||
url_redirect: str = input("Paste 'Ooops' page URL here and press <ENTER>:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to get this url programmatically rather than require the copy paste?
If not, can we then make other called functions non-private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't realise that my pending comment needed to actually be set as a review, also, did not necessarily mean to review, rather just leave a comment.
I actually mean to submit this days ago. Apologies for taking so long to realise my mistake
Sorry, I have been occupied by other responsibilities. I will take a look at it no later than this weekend. |
@exislow I got a chance testing this today with a HiFi enabled account. However, I get |
Just double checked this. I cannot re-produce it. Whether with a PKCE nor with a OAuth authorized token. Can you please more verbose? I need the full error log and the TIDAL link to the track you are working with. In my case |
In the response I get "401 Asset is not ready for playback" This issue only occurs with PKCE not with normal OAuth. Which track/album have you tested with? I did try a few, at least one of them should have HiRes import tidalapi
from tidalapi import Quality
session = tidalapi.Session()
# Will run until you visit the printed url and link your account
session.login_pkce()
# Override the required playback quality, if necessary
# Note: Set the quality according to your subscription.
# Normal: Quality.low_320k
# HiFi: Quality.high_lossless
# HiFi+ Quality.hi_res_lossless
session.audio_quality = Quality.hi_res_lossless.value
album = session.album(110827651) # Let's Rock // The Black Keys
tracks = album.tracks()
# list album tracks
for track in tracks:
print(track.name)
print(track.get_url())
for artist in track.artists:
print(' by: ', artist.name) |
Sorry for closing this PR, that was an accident and I have reopened it. Anyways see my notes below after doing some testing:
When a streaming link is requested, no Auth error is given so that part works fine. But it does NOT work with direct URL on my side, unless I switch back to standard Oauth. I think this just confirms what we already know from the tidal2 documentation lists, i.e. MPEG-Dash is allowed but normal https is not: https://github.com/arnesongit/plugin.audio.tidal2 |
Ohh sorry, I thought this was already clear: HiRes ONLY works with MPEG-DASH. This is why have built an MPEG-DASH parses for https://github.com/exislow/tidal-dl-ng/blob/a348909897b8a4453ec0b8764f564a4970b6e238/tidal_dl_ng/download.py#L390-L451 By the way, this works:
|
Ok, you mentioned that you managed to download a HiRes FLAC file in an earlier comment so I assumed get_url() was used. And you also mentioned using get_url() successfully in the comments in this PR so I thought I was doing something wrong. But good to know it works as expected with MPEG-DASH. I noticed tidal2 uses a similar approach to parse the MPEG-DASH stream so it can be passed on to ffmpeg. I'll probably look into implementing something similar, either directly in tidalapi or in the projects relying on it for streaming (mopidy-tidal) |
Oh I actually never mentioned |
How can I help to get this PR accepted and create a new release of |
@exislow You'd need to rebase to master, then we can merge it. Regarding next release, I was planning to add MPEG-DASH parsing as well, similar to how it is done with tidal2. EDIT: On second thought, I think we'll skip that for now so v0.7.4 can be released immediately after merging this PR, in case you need it right away. |
Bumps [pillow](https://github.com/python-pillow/Pillow) from 10.0.1 to 10.2.0. - [Release notes](https://github.com/python-pillow/Pillow/releases) - [Changelog](https://github.com/python-pillow/Pillow/blob/main/CHANGES.rst) - [Commits](python-pillow/Pillow@10.0.1...10.2.0) --- updated-dependencies: - dependency-name: pillow dependency-type: direct:development ... Signed-off-by: dependabot[bot] <[email protected]>
@tehkillerbee: Rebase is done. Regarding the release: When are you going to implement the MPEG-DASH parser? And why it is not possible to have two releases: One now and another after the parser is implemented? Release don't cost anything, right? Would be happy if you could release this ASAP. A lot of user waiting for HiRes support for a long time already. Thanks :-) |
See my comment above. :-) I think we'll stick to PKCE login for now and worry about MPEG-DASH later and/or leave it up to the users to implement in their own libraries so there is nothing stopping us to release v0.7.4 |
Ohh sorry, haven't seen the edited part, since I was only checking the mail notification. Thank you for your effort! |
self.process_auth_token(json) | ||
|
||
# Swap the client_id and secret | ||
#self.client_enable_hires() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@exislow Any reason this was not enabled in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works with and without this. To avoid a possible break of existing functions I have decided to comment it out. But if you are sure, that all tests are running correct with an OAuth2 token you could also enable line 494. If you like I can also get you a pull request for this. Just let me know, how you decide.
This is a straight forward user interactions based PKCE authorization implementation, which fixes #188 and #217.
You simply use
login_pkce()
and follow the instructions.Feel free to comment and / or merge.
Due to lack of time no tests have been implemented but comments / doc strings are in place.