Skip to content
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

Is HiRes and HiRes_LOSSLESS the same? #255

Open
exislow opened this issue Apr 20, 2024 · 3 comments
Open

Is HiRes and HiRes_LOSSLESS the same? #255

exislow opened this issue Apr 20, 2024 · 3 comments

Comments

@exislow
Copy link
Contributor

exislow commented Apr 20, 2024

As far as I remember TIDAL offer HIRES and HIRES_LOSSLESS as audio options. Has something changed? Are they considered to be the same nowadays? I guess not:

class Quality:
low_96k: str = "LOW"
low_320k: str = "HIGH"
high_lossless: str = "LOSSLESS"
hi_res: str = "HI_RES"
hi_res_lossless: str = "HI_RES_LOSSLESS"

Why I am asking?

def is_HiRes(self) -> bool:
try:
if (
self.media_metadata_tags
and MediaMetadataTags.hires_lossless in self.media_metadata_tags
):
return True

I got confused by this. There is now a property, which is called is_HiRes but it checks for hires_lossless.

Also wouldn't it be more pythonic to call the property is_hi_res / is_hires? Same applies for is_dolby_atmos etc.

@tehkillerbee
Copy link
Collaborator

FYI Hires and hires_lossless is not the same quality. Usually hires refers to mqa (non lossless) while hires_lossless refers to lossless FLAC.

IIRC, The track metadata does not usually contain enough information to determine what kind of hires mode is available. Only ATMOS/LOSSLESS/HIRES_LOSSLESS will be listed here.

It would probably be better if the property was named differently, mainly to avoid confusion. At least we should make sure both is_hires and is_hires_lossless exists as a property to avoid confusion. Perhaps it is enough to set is_hires true, if is_hires_lossless is not in the list, as I believe hires will be used as a fallback in this case.

We could consider changing the naming. Some of the names carry over from when the functionally was added from the tidal2 plugin and were never changed. I'd stick to hires, however, as this is the naming used by tidal.

@exislow
Copy link
Contributor Author

exislow commented Apr 21, 2024

It would probably be better if the property was named differently, mainly to avoid confusion. At least we should make sure both is_hires and is_hires_lossless exists as a property to avoid confusion. Perhaps it is enough to set is_hires true, if is_hires_lossless is not in the list, as I believe hires will be used as a fallback in this case.

But there are also a lot of songs available, which only have HIGH_LOSSLESS as max. quality instead of HI_RES. I don't know... This somehow feels strange / not as a perfect solution. If I understand you correctly, you are not 100% sure, what the metadata tags really mean, are you?

This is how I currently determine the available quality: https://github.com/exislow/tidal-dl-ng/blob/039ecf97e55ea655c18aacf696c38681920c4744/tidal_dl_ng/helper/tidal.py#L157-L169

I don't know, if it's the best solution.

We could consider changing the naming. Some of the names carry over from when the functionally was added from the tidal2 plugin and were never changed. I'd stick to hires, however, as this is the naming used by tidal.

Are you sure, that TIDAL uses HIRES instead of HI_RES as also already implemented here?

class Quality:
low_96k: str = "LOW"
low_320k: str = "HIGH"
high_lossless: str = "LOSSLESS"
hi_res: str = "HI_RES"
hi_res_lossless: str = "HI_RES_LOSSLESS"

@tehkillerbee
Copy link
Collaborator

tehkillerbee commented Apr 21, 2024

But there are also a lot of songs available, which only have HIGH_LOSSLESS as max. quality instead of HI_RES.

Yes, if HI_RES is not available, HIGH_LOSSLESS will be used instead.

Only ATMOS/LOSSLESS/HIRES_LOSSLESS will be listed here.

I forgot that MQA is also added to this list. Anyways, after looking closer at the code, it looks like I already added all the necessary properties so we don't need to add anything new. However, the naming might be a little confusing. Currently we have is_hires(), is_mqa(). So to avoid confusion, we should rename it is_hires_lossless() and is_mqa() OR is_hires(). I think understanding the media metadata should be pretty straight forward.

Next problem is that the media metadata shows the available quality for a given track (album) and not the one currently selected by the user.

Are you sure, that TIDAL uses HIRES instead of HI_RES as also already implemented here?

Looks like I remembered incorrectly, as tidal indeed uses HI_RES as the naming scheme. We could consider changing these property helper functions to is_hi_res .. so it corresponds to the naming used by tidal, and to be more pythonic. Of course this is another breaking change.

https://github.com/exislow/tidal-dl-ng/blob/039ecf97e55ea655c18aacf696c38681920c4744/tidal_dl_ng/helper/tidal.py#L157-L169

This piece of code can be useful in determining the best possible quality for a track, eg. in case the user does not want to use MQA (lossy) over HIGH_LOSSLESS. I have a related issue here: tehkillerbee/mopidy-tidal#161

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

No branches or pull requests

2 participants