Tyrrrz / YoutubeDownloader

Downloads videos and playlists from YouTube
MIT License
9.08k stars 1.23k forks source link

Big playlists of over 500 videos cause crashes #2

Closed pheonixfirewingz closed 5 years ago

pheonixfirewingz commented 5 years ago

I did some testing and the program for some reason is running only on one thread my CPU can use four threads at once being a 4 core CPU (4 being the common CPU core count) @Tyrrrz I think multithreading is needed to stop or at least counter this bug (would try fixing it myself for you but I can't get the project to work and also don't have time to work on something else due to the project I'm working on right now)

rcitaliano commented 5 years ago

I've tested this on windows with a playlist of 160+ videos, It finished the whole process and downloaded all the videos plus the conversion to MP3, I've seen that too many processing threads were created (40+) on an 8 thread processor.

anyway very good job on the parallel processing =)

I see that this might be a real issue when trying to download 500+ videos because, at leat the UI showed this, it started to download the videos all at once, then when it finished downloading some videos it started calling ffmpeg, I've seen like 20+ ffmpeg instances open.

With all this being said I guess that, for security but also for a performance improve, you should limit the amount of parallel actions.

I gues that, at least on my case which is an 8 thread processor, it's ok to have a maximum of 10 or 20 concurrent downloads (depending on the connection speed) and then have the amount of FFMPEG instances limited to the amount of processing threads (which is 8 in my case), this because the "download threads" don't require much CPU power as the FFMPEG so they can actually work well together.

To completely avoid this issue it would be nice to have an option to execute the whole operation in "a single thread", or to be able to set the maximum amount of concurrent operations via UI.

another way would be to "intelligently" set the amount of concurrent operations based on the download and conversion speeds, it would require to download and convert some videos of the playlist, creating some "performance parameters" and then go at full capacity limiting the amount of concurrent operations using the calculated parameters, pretty much this would mean to maximize the network usage while also maximizing the CPU "power" by avoiding deadlocks on the threads and pushing as far as the CPU and network can go =)

I would like to discuss this with you if you want.

pheonixfirewingz commented 5 years ago

also, there's not a way to limit the program's CPU usage it can take up to 98% of my CPU at a time almost crashing my tower and killing my laptop. Quote "another way would be to "intelligently" set the number of concurrent operations based on the download and conversion speeds, it would require to download and convert some videos of the playlist, creating some "performance parameters" and then go at full capacity limiting the number of concurrent operations using the calculated parameters, pretty much this would mean to maximize the network usage while also maximizing the CPU "power" by avoiding deadlocks on the threads and pushing as far as the CPU and network can go =)" this can be countered but having the program store it on the ram until can be processed that's what the ram's job is :100: also doing it this way would save stress on the CPU trying to download the song and convert it at the same time the program could download it store it on the ram and then convert it when finished if got discord 🎄❄ Pheonixfirewingz ❄ 🎄#2116 to let's try live discussion

Tyrrrz commented 5 years ago

High CPU utilization comes from ffmepg, even one process is enough to put it to 100% usage when it's transcoding (which happens when you save to mp3, or anything other than mp4 in YouTube's case). I agree that the degree of parallelism should be configured via UI though. 👍 Downloading and converting happens separately and there's no benefit in storing it on RAM as opposed to saving to a temporary file, which is what it's doing now. Passing multiple asynchronous streams to ffmpeg is actually not trivial and the only way to achieve that I know of is named pipelines.

rcitaliano commented 5 years ago

I guess the issue here isn't getting to 100% CPU, it's creating too many processing threads/processes that would overwhelm the CPU by increasing the amount of context switching required.

it's ok to go to 100%CPU, it's not OK to spend too much time jumping from thread to thread, in these kind of scenarios we need to find the right balance

Tyrrrz commented 5 years ago

Good point!

xamarin-enthusiast commented 5 years ago

What's the largest number of videos in a playlist that it can handle at any one time?

Tyrrrz commented 5 years ago

You can try and find out ;)