-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fixed insane wait times #51
base: master
Are you sure you want to change the base?
Conversation
Does this fix it locally for you? I'm having trouble getting it working. |
I'm the author of this fix and it does work! I use it in my app called: YTerMusic (https://github.com/ccgauche/ytermusic) which works well. |
@DzenanJupic Does it work (I fixed the CI issue that happened for no reason) ? |
I'll try it out in a second, I'll first try to get #49 in, to see what's going on. |
Just tested it, it's absolutely amazing! Thanks a lot, @ccgauche. |
@DzenanJupic May you merge? |
Using both your pull request (Discursif@6b676ca) and the "fixed" branch on this repo (f118c6b), this minimum example from the readme downloads an empty video file almost instantly: #[tokio::main]
async fn main() {
let url = "https://www.youtube.com/watch?v=Edx9D2yaOGs&ab_channel=CollegeHumor";
println!("downloaded video to {:?}", rustube::download_best_quality(&url).await.unwrap());
} Here's how I added the dependency as a sanity check: [dependencies]
rustube = { git = "https://github.com/DzenanJupic/rustube", branch = "fix-warnings-and-rebase-master" }
tokio = { version = "1.12.0", features = ["rt-multi-thread"] } |
I'll check that, thanks for the notice. |
I'll also look around possibly tonight but I don't really understand how it works lol 🙃 |
The fix has been tested on a very very large amount of musics (since it was made for my music app) but wasn't tested at all on videos. Maybe using the old system as a fallback could be a good idea while we find out what causes the issue (probably related to the file size of videos since music is a lot smaller). The pytube fix is a little big longer with more things and checks so maybe they are here to fix this issue. Didn't have the time to test. In my tests on music they changed nothing so I removed them to better fit my use case and reduce the fix size. |
Ok( | ||
let k = self.client.get(url.as_str()); | ||
Ok(if let Some(content_length) = content_length { | ||
let k = k.header("Range", format!("bytes=0-{content_length}")); |
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.
Unfortunately some streams have content length set as zero. One of them being the highest quality stream of the example video. This leads to zero bytes being returned by the server.
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.
I'll check on that but I don't know if there is a possible fix at least better than defaulting to old slow system...
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.
I'm pretty sure this is not idiomatic code (this basically being the first lines of Rust I've written outside of tutorials) - but here we go - we should probably fall back to not using the Range
if the content length is zero.
let k = k.header("Range", format!("bytes=0-{content_length}")); | |
let k = if content_length > 0 { | |
k.header("Range", format!("bytes=0-{content_length}")) | |
} else { | |
k | |
}; |
Closes #37