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

Fixed insane wait times #51

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

Conversation

DzenanJupic
Copy link
Owner

@DzenanJupic DzenanJupic commented Dec 12, 2022

Closes #37

@cjburkey01
Copy link

Does this fix it locally for you? I'm having trouble getting it working.

@ccgauche
Copy link

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.

@ccgauche
Copy link

@DzenanJupic Does it work (I fixed the CI issue that happened for no reason) ?

@DzenanJupic
Copy link
Owner Author

I'll try it out in a second, I'll first try to get #49 in, to see what's going on.
The reason the CI fails is that it forbids any clippy warnings. Don't worry about the remaining failures, I'll fix them.

@DzenanJupic
Copy link
Owner Author

DzenanJupic commented Dec 16, 2022

Just tested it, it's absolutely amazing! Thanks a lot, @ccgauche.
With these changes, a 45 min video is downloaded in a few seconds for me.

@ccgauche
Copy link

@DzenanJupic May you merge?

@cjburkey01
Copy link

cjburkey01 commented Dec 22, 2022

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"] }

@DzenanJupic
Copy link
Owner Author

I'll check that, thanks for the notice.

@cjburkey01
Copy link

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 🙃

@ccgauche
Copy link

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}"));

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.

Copy link

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...

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.

Suggested change
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
};

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.

Is download speed throttled by YouTube?
4 participants