-
Notifications
You must be signed in to change notification settings - Fork 113
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
Feature/Add ability to set audio/video quality for session + bugfixes #174
Feature/Add ability to set audio/video quality for session + bugfixes #174
Conversation
a52fc66
to
8a08c97
Compare
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.
Mostly looks good. I've rebased against master. Personally I'd use properties for the audio/video quality: more pythonic, saves digging into the config object (which I think we're going to refactor pretty heavily at some point), and more discoverable.
… functions returning image URL..
42618fe
to
e2c121f
Compare
I think that as part of this PR you should also update the Quality Enum in media to properly reflect the new quality possibilities https://github.com/tamland/python-tidal/blob/master/tidalapi/media.py#L39 |
@jozefKruszynski As far as I know, there are only these four quality options available from tidal, with master (hi_res) providing the highest quality possible in lossless FLAC. Are there more options available? I have noticed that tidal does not always refer to the highest quality as 'master'. Is that what you mean? |
Well what I meant was that previously master was essentially the highest possible quality setting and this actually refers to lossless. So actually high now has a different meaning than before. EDIT: |
Ok, now I know what you mean. Yes, I noticed that Tidal changed their naming slightly in the web client. Now we have Low (96kbps, 320kbps), High and Max). The the actual name of the quality used by tidalapi corresponds to low, high, lossless and hi_res. In the android app, we have normal, high, hifi and master. For consistency (with the web/PC client), we could use two "low" settings (eg low_96k, low_320k) and then "high" and "max".
ffprobe output with low (mp4), high (mp4), lossless (flac) and master (flac)
|
Selecting HIGH shouldn't give you HIFI version, that should instead give you 320k mp4. Yes, currently you need to select master to get the best possible quality. |
Now it all makes sense, sorry for the confusion. These changes are really cool. I think combined with mediaMetadata array that was merged last week it should be much easier to ensure that I'm only streaming flac files and never try to grab mqa stuff. |
@tehkillerbee pushed a few fixups: apologies for pushing to your branch. Should we default to high (master) or lossless? Added a test to assert the current behaviour. |
@jozefKruszynski I just tested this. It appears that HI_RES and HI_RES_LOSSLESS returns the same FLAC file. Perhaps for backwards compatibility? Only TIDAL would know... :)
Should we change to HI_RES_LOSSLESS for consistency? What about renaming the quality presets as I suggested earlier?
Thoughts @2e0byo ? |
Great, thanks for adding tests. I still need to get into the habit of writing tests for everything I add :) I think we should default to the quality type that is most likely used by the end user, if a standard subscription is available. So I would say we should use |
Re. the type: why not use Enum aliasing and support both? |
Have you been able to get real HI_RES audio so far, rather than the 24bit 48000Hz MQA files? Are you seeing the same behaviour? |
The above HI_RES files are all 24bit 48kHz FLAC (i.e. not MQA). Of course these files are not 192kHz as TIDAL has previously advertised. Maybe this is dependent on the availability. What albums have you tested with? I can try testing with the same albums. |
I think I would prefer that we do not have too many quality options to avoid confusion and to mimic the options provided in the web players. So I propose something like.
|
Hmm, that makes sense actually, I was somehow assuming that ffmpeg is not able to display if something is actually encoded with MQA as from what I understand this would still show as a flac file, albeit an mqa encoded flac file. EDIT: |
Now I'm more awake aliasing is the other way round: one value (string) mapping onto multiple entries. These look good to me. |
Odd, same thing happens to me with that specific album. Maybe it doesn't actually have the 24bit quality? Try this album, it definitely works for me: https://listen.tidal.com/album/110827651 |
I definitely get the 24bit version with this album too, although limited to 48000Hz, which may of course be the only thing that is available. EDIT: |
Yes, I have yet to find one. Please let me know if you find one as I would like to have one to test with
No problem at all. It took a bit longer to get this PR processed and right now I am working on a small feature related to the "explore" and "for you" pages, then I will make a new release this weekend. 👍 |
The tests that rely on contest.py line 118 no longer work after this merge unfortunately. |
Thanks for letting me know. Yes, I must've overlooked it when doing a search through the sources. I will fix it later today. |
I've fixed it in a PR that I just pushed. If you would like me to split that PR into a fix for that, and the new feature that I am proposing I can do this also. |
Thanks I see you have already made a PR for it. That's fine, no need to split it into a separate PR :) |
@jozefKruszynski This album is 24bit 192kHz: https://tidal.com/browse/album/539203 |
Not sure either , so far I have not been able to get any tracks with a higher bitrate than 48kHz/24bit, so I thought it was dependent on the album. Can you make a new issue for this so we do not use the PR for further discussions? |
Done |
This PR adds the following features: