-
Notifications
You must be signed in to change notification settings - Fork 422
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
Attempt to parallelize YouTube Stream extractor #751
base: dev
Are you sure you want to change the base?
Conversation
FireMasterK
commented
Nov 17, 2021
- I carefully read the contribution guidelines and agree to them.
- I have tested the API against NewPipe.
- I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.
...in/java/org/schabi/newpipe/extractor/services/youtube/extractors/YoutubeStreamExtractor.java
Outdated
Show resolved
Hide resolved
ps: I did NOT take a real look at the code |
Why use Java futures manually? I'd rely on RxJava instead (like we already do in the app) |
I think there is no RXJava inside this project. |
Yeah, I know. I'd propose to use it, what do you think? |
I don't know.
I'm also fine with the current way of using the java native way. |
Was there any conclusion to what library to use? Should we go with the default Java API? I'd like to rework on this, one last time. |
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.
@FireMasterK yes, let's use the default Java API. The current code structure looks good :-)
extractor/src/main/java/org/schabi/newpipe/extractor/utils/ExceptionUtils.java
Outdated
Show resolved
Hide resolved
40c5124
to
3b632db
Compare
Yes, currently in NewPipe's case, a minimum of 2 requests are made regardless. We parallelize them, to reduce the time taken by half. Another additional request may be made - ios or android client, depending if it is a live stream or not. In Piped's case, a minimum of 4 requests are made since I force the android and ios player fetch.
Piped's response times have dropped by ~400-500ms on average (it depends on how fast youtube's server responds), this should benefit NewPipe too. |
ce9f3cb
to
be69e39
Compare
I tried updating the mocks, but it appears to be unable to find some requests, anyone have any idea why this might be happening? |
b0a0049
to
e579898
Compare
6e9f4cc
to
c5bb081
Compare
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.
Some time has passed, sorry... On a second thought this implementation is fine. @AudricV do you have anything against merging this, after it has been rebased? ;-)
It's still a draft since it would have issues with mocks. What I'm going to do to fix them is remove the |
c5bb081
to
1dd3faa
Compare
1dd3faa
to
35f40b7
Compare
35f40b7
to
7fd580f
Compare