Tzahi12345 / YoutubeDL-Material

Self-hosted YouTube downloader built on Material Design
MIT License
2.64k stars 273 forks source link

Problems and suggestions (subscriptions) #317

Open controlol opened 3 years ago

controlol commented 3 years ago

I have this app for one purpose and that's downloading youtube videos automatically but it's not working like I would expect it to work.

The application does not show downloaded videos from subscriptions untill I restart the docker container. Not so nice but this can be lived with.

It would make sense to use the same "default file output" and "global custom args" settings as in the settings menu by default for subscriptions. And keep the option to change them per subscription like it is now. This would lead to simpler file management in my opinion.

Currently if I set the "custom file output" in a subscription it will fail with this error Command failed with ETXTBSY: node_modules/youtube-dl/bin/youtube-dl -i -o downloads/channels/Q-Dance/%(title)s (%(upload_date)s) %(height)sp %(fps)sfps.%(ext)s -ciw --write-info-json --print-json -f bestvideo[height<=1080]+bestaudio/best[height<=1080] --merge-output-format mp4 --restrict-filenames -r 10M --dateafter now-1week --write-thumbnail https://www.youtube.com/user/Qdancedotnl I set the option to: %(title)s (%(upload_date)s) %(height)sp %(fps)sfps which works just fine in the general settings.

And my last most important point: WHY would you send the full json object of the subscription when you want to remove a subscription. I tried to remove a channel that was just under 20k lines in your database. Sending all this data would result in a 413 error of course. Just send the ID of the subscription, let it remove all the files in the base folder of that subscription and remove the subscription object from the database... Please change this, you can also add the option to remove files when removing the subscription (or a video for that matter) from the database.

My default.json:

{
  "YoutubeDLMaterial": {
    "Host": {
      "url": "http://example.com",
      "port": "17442"
    },
    "Downloader": {
      "path-audio": "downloads/audio/",
      "path-video": "downloads/video/",
      "default_file_output": "%(uploader)s/%(title)s (%(upload_date)s) %(height)sp %(fps)sfps",
      "use_youtubedl_archive": false,
      "custom_args": "--restrict-filenames,,-r,,10M",
      "safe_download_override": false,
      "include_thumbnail": true,
      "include_metadata": false
    },
    "Extra": {
      "title_top": "Youtube Downloader",
      "file_manager_enabled": true,
      "allow_quality_select": true,
      "download_only_mode": false,
      "allow_multi_download_mode": true,
      "enable_downloads_manager": true,
      "allow_playlist_categorization": true
    },
    "API": {
      "use_API_key": true,
      "API_key": "xxx",
      "use_youtube_API": true,
      "youtube_API_key": "xxx",
      "use_twitch_API": false,
      "twitch_API_key": "",
      "twitch_auto_download_chat": false
    },
    "Themes": {
      "default_theme": "dark",
      "allow_theme_change": true
    },
    "Subscriptions": {
      "allow_subscriptions": true,
      "subscriptions_base_path": "downloads/",
      "subscriptions_check_interval": "60",
      "redownload_fresh_uploads": true
    },
    "Users": {
      "base_path": "users/",
      "allow_registration": true,
      "auth_method": "internal",
      "ldap_config": {
        "url": "ldap://localhost:389",
        "bindDN": "cn=root",
        "bindCredentials": "secret",
        "searchBase": "ou=passport-ldapauth",
        "searchFilter": "(uid={{username}})"
      }
    },
    "Advanced": {
      "default_downloader": "youtube-dl",
      "use_default_downloading_agent": true,
      "custom_downloading_agent": "",
      "multi_user_mode": false,
      "allow_advanced_download": false,
      "use_cookies": false,
      "jwt_expiration": 86400,
      "logger_level": "info"
    }
  }
}

My db.json:

{
  "playlists": [],
  "files": [],
  "configWriteFlag": false,
  "downloads": {},
  "subscriptions": [
    {
      "name": "Q-Dance",
      "url": "https://www.youtube.com/user/Qdancedotnl",
      "maxQuality": "1080",
      "id": "a6cab6b2-7d25-414e-91ee-516d2c84576b",
      "streamingOnly": false,
      "user_uid": null,
      "type": "video",
      "timerange": "now-1week",
      "custom_args": "--restrict-filenames,,-r,,10M",
      "custom_output": "%(title)s (%(upload_date)s) %(height)sp %(fps)sfps",
      "isPlaylist": false,
      "videos": [],
      "downloading": false
    }
  ],
  "files_to_db_migration_complete": false,
  "simplified_db_migration_complete": true,
  "categories": []
}
Tzahi12345 commented 3 years ago

The application does not show downloaded videos from subscriptions untill I restart the docker container.

Is the subscription still retrieving videos when this happens? If you go to the subscription page and there is a loading bar, it means it's still downloading videos. And the simple answer to "why doesn't it populate over time rather than waiting for the check to finish" is that youtube-dl doesn't give us the metadata until it's all finished.

Now, there are ways around this: we could get the URL for the individual videos we need to download and do them piecemeal rather than in one go, but that may take significantly longer (haven't tested this).

If it finished downloading and it didn't populate, there may be a bug there.

It would make sense to use the same "default file output" and "global custom args" settings as in the settings menu by default for subscriptions. And keep the option to change them per subscription like it is now. This would lead to simpler file management in my opinion.

Not a bad idea! Regarding the global custom args, would you want the per-subscription ones to override them or simply add on to it?

Currently if I set the "custom file output" in a subscription it will fail with this error

I think it's related to this issue, can you try enclosing the custom file output arg in single quotes like so?:

'%(title)s (%(upload_date)s) %(height)sp %(fps)sfps'

That may fix it.

WHY would you send the full json object of the subscription when you want to remove a subscription. I tried to remove a channel that was just under 20k lines in your database. Sending all this data would result in a 413 error of course. Just send the ID of the subscription, let it remove all the files in the base folder of that subscription and remove the subscription object from the database...

You're 100% right on this one, it's bad design but there actually is a reason we send the whole object and not just the ID -- we save a call to the DB as we need to know whether the subscription is a playlist or not. Bad reason, I know, but rather than changing a bunch of code I decided to just strip away the videos array so that only the necessary metadata is passed (see commit here).

295 is somewhat related. Basically I wrote this about 4 years ago when I was a much worse programmer, and a lot of the tech debt related to the DB is catching up quick. Simple stuff like why were the videos stored within the subscription object in the DB instead of in a separate table? Anyways hopefully I'll get to fixing all that soon.

you can also add the option to remove files when removing the subscription (or a video for that matter) from the database.

I actually added in the option to "pause" a subscription for this reason, but I gather you'd rather not see it in the UI at all? In that case I could add another button that says "Unsubscribe but keep files" or something like that.

controlol commented 3 years ago

I have been playing around a bit with the youtube command generated by your app and the youtube-dl binary located in the node_modules. I succeeded with the following command: ./youtube-dl -i -o 'downloads/channels/Q-Dance/%(title)s %(upload_date)s %(height)sp %(fps)sfps.%(ext)s' -ciw --write-info-json --print-json -f 'bestvideo[height<=1080]+bestaudio/best[height<=1080]' --merge-output-format mp4 --restrict-filenames -r 10M --dateafter now-1week --write-thumbnail https://www.youtube.com/user/Qdancedotnl

Notice the quotation marks around the download string and -f argument. These were at least missing in the logs. After adding that it worked fine. Adding the quation marks in the UI did not help

You're 100% right on this one, it's bad design but there actually is a reason we send the whole object and not just the ID -- we save a call to the DB as we need to know whether the subscription is a playlist or not. Bad reason, I know, but rather than changing a bunch of code I decided to just strip away the videos array so that only the necessary metadata is passed (see commit here).

Yay that's a step in the right direction!

Not a bad idea! Regarding the global custom args, would you want the per-subscription ones to override them or simply add on to it?

I would say overwrite as it's more flexible.

I actually added in the option to "pause" a subscription for this reason, but I gather you'd rather not see it in the UI at all? In that case I could add another button that says "Unsubscribe but keep files" or something like that.

Does remove subscription already remove all the downloads? I did not see it happen. If it should already do that I think the pause option is a nice solution. Keeping all the downloaded video files in the UI under that subscription :)

controlol commented 3 years ago

Also something that would be really nice: edit the metadata of the file to have the created and modified date the same as the upload date :) That would be great for general sorting on windows and plex would use the right date (I am quite sure) without needing the date in the title

Tzahi12345 commented 3 years ago

Sorry for the late reply, I've looked into the spaces issue and it seems to be a problem with the node-youtube-dl package we're using where if quotes are added there it's considered part of the path which is not what we want. Interestingly enough, this is only an issue on Linux/Docker. On Windows, the spaces are considered part of the path and it works just fine.

And so I thought, perhaps the path needs to be escaped with a backslash. No luck there either :/

The best bet here is just to use an underscore instead of space. Definitely not ideal, but I wrote on a related issue and we'll see if they back to me.

I would say overwrite as it's more flexible.

Yeah that makes the most sense to me. I added this as a card to the v4.3 project so it's lined up for the next update (but it's a fairly simple one so I may just do it relatively soon)

Does remove subscription already remove all the downloads? I did not see it happen. If it should already do that I think the pause option is a nice solution. Keeping all the downloaded video files in the UI under that subscription :)

It does remove the videos, I'll stick with the pause button solution for now since it seems to do the job.

Also something that would be really nice: edit the metadata of the file to have the created and modified date the same as the upload date :)

That's a really good idea! I'll probably have to leverage something like ffmetadata to do this. Also added this to the v4.3 project :)

joca879 commented 3 years ago

Hi @Tzahi12345

I've installed youtubedl-material through docker in a Synology NAS and have the same ETXTBSY error with subscriptions.

Is this issue already fixed?

I'm trying to do what @controlol did:

I have been playing around a bit with the youtube command generated by your app and the youtube-dl binary located in the node_modules. I succeeded with the following command:

./youtube-dl -i -o 'downloads/channels/Q-Dance/%(title)s %(upload_date)s %(height)sp %(fps)sfps.%(ext)s' -ciw --write-info-json --print-json -f 'bestvideo[height<=1080]+bestaudio/best[height<=1080]' --merge-output-format mp4 --restrict-filenames -r 10M --dateafter now-1week --write-thumbnail https://www.youtube.com/user/Qdancedotnl

Notice the quotation marks around the download string and -f argument. These were at least missing in the logs. After adding that it worked fine. Adding the quation marks in the UI did not help

but i can't find the youtube-dl binary or the node_modules... I´ve got only the /app/appdata mounted.

Can someone tell me where is located the file so that I can try to solve this issue?

controlol commented 3 years ago

It is a node module in this case. The binary is located in there. Of course normally when you install youtube-dl on linux you would be able to execute the command anywhere from the system because it is include in the "path". In that case you would not need the "./" at the front off the path.

If you however want to test this with the included binary you would have to go to the node_modules folder. This folder is normally not part of the persistant storage so you would have to exec into the docker container to test this.

To be clear this does not resolve any issues in yt-dl-material, it was just a way for me to test how the youtube-dl CLI works because I had never worked with it before. Trying to understand why the command fails in yt-dl-material and how it should be resolved. I hope @Tzahi12345 has been able to solve this by now. I have not used the application since because I never got a update about this issue and storage was running out a bit anyways.

joca879 commented 3 years ago

I'm a newbie in linux and docker... I've installed the youtubdl-material docker in the Synology NAS accordingly with the instructions that I've found, mapping the /appdata, /users, /video, /audio, /subscriptions folders... In none of them I 've found node_modules folder... I'm lost in this thing. Thanks anyway for your explanation.

joca879 commented 3 years ago

Hello @controlol Sorry to bother you, I think I've finally managed to found the folder in the container: image It's this one, correct?

Nonetheless, when opening the youtube-dl file to modify, the following appears to me: image

I can´t see the text you mentioned in previous post... Is this the correct file to change? I'm kind of lost, like I said I'm a newbie

controlol commented 3 years ago

You should be changing any of the files' content. It's a binary and should be executed. It's like a application like you would have on windows or mac but instead of opening a window it takes arguments from the command (just like your buttons and inputs you would normally have with a application). After the program is finished with what you asked it to do it will just simply close and you could ask it to do something else.

So you can not change how the program works.

We will have to wait for a update from the developer for this to work properly, until then I can maybe create a pull request with changes to yt-dl-material, but NOT to youtube-dl. There is no problem with youtube-dl, which is a separate from this application (and thus not controlled by the developer of yt-dl-material). yt-dl-material uses youtube-dl to download videos on a schedule/automatically.

I would like to give the developer a chance to react to this problem before continuing.

Before I close this message I would like to point out a last point to @Tzahi12345 I was just looking to create a pull request to solve this issue. What I noticed is that you are using a deprecated npm package for youtube-dl, instead you should be using youtube-dl-exec according to the npm page. Take this as a heads up :)

Tzahi12345 commented 3 years ago

@joca879 controlol is correct, you can't modify or view the binary using a text editor, nor would you really ever want to do this. The only time you'd want to deal with it is for debugging purposes or manually running a youtube-dl command (which you can do from the Advanced download dropdown anyways.

@controlol I'll go ahead and look at the PR, thanks for the heads up! Hopefully it will fix this issue.

kreditor0815 commented 3 years ago

Is there any progress on this matter? Im currently facing the same problem and the PR 343 (https://github.com/Tzahi12345/YoutubeDL-Material/pull/343) looks promising. I also believe that the Issue 285 (https://github.com/Tzahi12345/YoutubeDL-Material/issues/285) is also about the problem diskussed here.

Tzahi12345 commented 3 years ago

The last time I tried the fix proposed in #343 I had an issue on Linux where the quotes were included in the path, I'll try again but I'm sure there's a solution somewhere. Not sure why --restrict-filenames doesn't work.