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

Sleeping only for missing songs, edit error for Album not found #1020

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bloedboemmel
Copy link

Closes #1018

@strikeout
Copy link

I tested it out because I was going to suggest the same feature when I saw your PR! :)

It works really well, there is only one issue with it:
When you are syncing a large playlist you are running into the Tidal 429 (too many connections) error and have to wait the 20 sec cooldown. I guess that comes from the much quicker execution of TIDAL_API.getStreamUrl for each track. Is there a way to even cut short execution before that call gets made?

@bloedboemmel
Copy link
Author

bloedboemmel commented Nov 24, 2022

Is there a way to even cut short execution before that call gets made?

Hey @strikeout , thanks for your review. I think there should be a way, because you can get all playlist-tracks at once. I was a little lazy tbh, but I will have a look into it again.

@bloedboemmel
Copy link
Author

Now it avoids also the 429 Errors. @strikeout, please confirm :)

@strikeout
Copy link

Niiice!
Confirmed working as intended.

@strikeout
Copy link

Ok I found a little bug. I'm not sure if it is directly related to your PR but here it goes:
if a track is unavailable for streaming it's going to be downloaded as a video by default, and obviously fails. I traced this to the faulty logic in tidal.py, line 266+:

// tidal.py
for item in data:
    if item['type'] == 'track' and item['item']['streamReady']:
        tracks.append(aigpy.model.dictToModel(item['item'], Track()))
    else:
        videos.append(aigpy.model.dictToModel(item['item'], Video()))

should be along the lines of:

for item in data:
            if item['type'] == 'track' and item['item']['streamReady']:
                tracks.append(aigpy.model.dictToModel(item['item'], Track()))
            elif item['type'] == 'video' and item['item']['streamReady']:
                videos.append(aigpy.model.dictToModel(item['item'], Video()))
            else:
                <some cool error handling here>

@bloedboemmel
Copy link
Author

Niiice!
Confirmed working as intended.

Thanks for checking :)

if a track is unavailable for streaming it's going to be downloaded as a video by default, and obviously fails

I don't think that has something to do with my pull request. You should open another issue for that.

@donbernhardo
Copy link

May I suggest moving the "skip if file exists" logic from the "downloadTrack" function to the "downloadTracks" function of download.py.

Problem:
TIDAL_API.getAlbum() gets called for every track in playlist before the "skip if file exists"-logic of the current implementation resulting in "429 too many API calls"-Error for large playlists. It is also slow because of that.

Proposed solution:
Implement "skip if file exists" -logic in the "downloadTracks"-function when looping through the tracklist.

Here is my working implementation of the suggested change:

download.py

def downloadTracks(...

   ...
   if not SETTINGS.multiThread:
        for index, item in enumerate(tracks):
           path = getTrackPath(item, album, playlist)
           if os.path.exists(path):
               Printf.success(aigpy.path.getFileName(path) + " (skip super early:already exists!)")
               continue

           itemAlbum = album
           if itemAlbum is None:
              itemAlbum = __getAlbum__(item)
              item.trackNumberOnPlaylist = index + 1

           downloadTrack(item, playlist=playlist, album=itemAlbum)
        ...

@j-alvarez-moreno
Copy link

May I suggest moving the "skip if file exists" logic from the "downloadTrack" function to the "downloadTracks" function of download.py.

Problem: TIDAL_API.getAlbum() gets called for every track in playlist before the "skip if file exists"-logic of the current implementation resulting in "429 too many API calls"-Error for large playlists. It is also slow because of that.

Proposed solution: Implement "skip if file exists" -logic in the "downloadTracks"-function when looping through the tracklist.

Here is my working implementation of the suggested change:

download.py

def downloadTracks(...

   ...
   if not SETTINGS.multiThread:
        for index, item in enumerate(tracks):
           path = getTrackPath(item, album, playlist)
           if os.path.exists(path):
               Printf.success(aigpy.path.getFileName(path) + " (skip super early:already exists!)")
               continue

           itemAlbum = album
           if itemAlbum is None:
              itemAlbum = __getAlbum__(item)
              item.trackNumberOnPlaylist = index + 1

           downloadTrack(item, playlist=playlist, album=itemAlbum)
        ...

For what is worth, I tried your fork and ran into the following error:

[ERR] 'Playlist' object has no attribute 'artists'

What are your specific settings to make this work?

@donbernhardo
Copy link

donbernhardo commented Mar 7, 2023

For what is worth, I tried your fork and ran into the following error:

[ERR] 'Playlist' object has no attribute 'artists'

What are your specific settings to make this work?

I checked out from here:

git fetch origin pull/1020/head:lobra
git checkout lobra

I attached the download.py with the changes that are working for me. The zip also includes all the other python-files changes by this pull request.

tidal-dl.zip

@j-alvarez-moreno
Copy link

Thank you @donbernhardo, that did the trick. To any person from the future who may be running into this thread: multithreaded downloads must be off for this fork.

If you want to make additional changes to the too many requests, waiting for x seconds backoff logic, take a look a tidal.py:44 and tweak the number of seconds to wait there.

In my case (multiple playlists with almost a thousand songs on each) 300 seconds is a safe value to avoid going over the threshold.

@noname77
Copy link

hey, thanks for work on this, it will be a great speed up in my workflow when I manage to make it work :)

I didn't get to look into the root cause yet, just reporting for now
it seems to me that the file format is hardcoded to .flac in this PR

+----------------+------------+
| TRACK-PROPERTY | VALUE      |
+----------------+------------+
| Title          | Some title |
| ID             | 12345678   |
| Album          | Some album |
| Version        | None       |
| Explicit       | False      |
| Max-Q          | LOSSLESS   |
| Get-Q          | HIGH       |
| Get-Codec      | mp4a.40.2  |
+----------------+------------+
100%|▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓| 11.57/11.57 MB
[ERR] DL Track[Some title] failed.'./download//Playlist/a playlist [playlist id]/01 - Artist - Some title.flac' is not a valid FLAC file

when using master branch the files are correctly saved as .m4a and there's no error message

any ideas?

thanks,
wiktor

@noname77
Copy link

I poked around and it turned out that event though the requested stream had correct quality set, the track api response was set to LOSLESS regardless

here's my quick and dirty fix

TIDALDL-PY % git status
On branch lobra
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   tidal_dl/tidal.py
% git diff TIDALDL-PY/tidal_dl/tidal.py
diff --git a/TIDALDL-PY/tidal_dl/tidal.py b/TIDALDL-PY/tidal_dl/tidal.py
index b00e06b..2d872f3 100644
--- a/TIDALDL-PY/tidal_dl/tidal.py
+++ b/TIDALDL-PY/tidal_dl/tidal.py
@@ -265,7 +265,12 @@ class TidalAPI(object):
         videos = []
         for item in data:
             if item['type'] == 'track' and item['item']['streamReady']:
-                tracks.append(aigpy.model.dictToModel(item['item'], Track()))
+                # print(f"track model: {item['item']}")
+                # fix audio quality
+                track = aigpy.model.dictToModel(item['item'], Track())
+                track.audioQuality = self.getQualityStr(SETTINGS.audioQuality)
+                tracks.append(track)
+
             else:
                 videos.append(aigpy.model.dictToModel(item['item'], Video()))
         return tracks, videos
@@ -280,7 +285,7 @@ class TidalAPI(object):
         albums += list(aigpy.model.dictToModel(item, Album()) for item in data)
         return albums
 
-    def getStreamUrl(self, id, quality: AudioQuality):
+    def getQualityStr(self, quality: AudioQuality) -> str:
         squality = "HI_RES"
         if quality == AudioQuality.Normal:
             squality = "LOW"
@@ -289,6 +294,11 @@ class TidalAPI(object):
         elif quality == AudioQuality.HiFi:
             squality = "LOSSLESS"
 
+        return squality
+
+    def getStreamUrl(self, id, quality: AudioQuality):
+        squality = self.getQualityStr(quality)
+
         paras = {"audioquality": squality, "playbackmode": "STREAM", "assetpresentation": "FULL"}
         data = self.__get__(f'tracks/{str(id)}/playbackinfopostpaywall', paras)
         resp = aigpy.model.dictToModel(data, StreamRespond())

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

Successfully merging this pull request may close these issues.

[FEATURE]: Check if file exists BEFORE sleeping
5 participants