- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 497
Attempt to parallelize YouTube Stream extractor #751
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
base: dev
Are you sure you want to change the base?
Conversation
         FireMasterK
  
      
      
      commented
      
            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