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.

Sign up for GitHub

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

Jump to bottom

fix(client): Randomly slow youtube download speed #229

Closed
wants to merge 6 commits into from
Closed

fix(client): Randomly slow youtube download speed #229

wants to merge 6 commits into from

Conversation

jnelle
Copy link

@jnelle jnelle commented Jan 7, 2022 •
edited
Loading

Description

This PR fixes the randomly slow youtube download speed that caused from web client.

Issues to fix

Please link issues this PR will fix:
#[230]

Reminding

Something you can do before PR to reduce time to merge

  • run "make build" to build the code
  • run "make format" to reformat the code
  • run "make lint" if you are using unix system
  • run "make test-integration" to pass all tests

Billaids and others added 3 commits January 8, 2022 09:25
@corny
fix(client): drop web and switch to android client
f207517 
Signed-off-by: Billaids <jimmy.nelle@hsw-stud.de>
@corny
feat(client): improve getStreamSize
28eb60d 
Signed-off-by: Billaids <jimmy.nelle@hsw-stud.de>
@corny
Fix one test
4a911d9
@corny
Copy link
Collaborator

corny commented Jan 8, 2022

Thanks a lot for you contribution! Unfortunately one test fails because the changed user agent leads to a missing creation timestamp. I could not find a solution yet.

@jnelle
Copy link
Author

jnelle commented Jan 8, 2022

Thanks a lot for you contribution! Unfortunately one test fails because the changed user agent leads to a missing creation timestamp. I could not find a solution yet.

Thanks for your help! I'll have a closer look this evening

Billaids added 2 commits January 24, 2022 09:25
fix: adjustments for using an android client [WIP]
27cee14 
* we can't get the publishdate from the android response, have to add maybe another http request for it

Signed-off-by: Billaids <jimmy.nelle@hsw-stud.de>
refactor: clean up response_data
0440d43 
Signed-off-by: Billaids <jimmy.nelle@hsw-stud.de>
jnelle
jnelle commented Jan 25, 2022
View reviewed changes
video.go
if str := prData.Microformat.PlayerMicroformatRenderer.PublishDate; str != "" {
v.PublishDate, _ = time.Parse(dateFormat, str)
}
// couldn't find any publishdate for now, have to add an another request
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@corny could you check this please?

cybre
cybre reviewed Jan 25, 2022
View reviewed changes
video.go

// Assign Streams
v.Formats = append(prData.StreamingData.Formats, prData.StreamingData.AdaptiveFormats...)
v.Formats = append(prData.StreamingData.Formats, prData.StreamingData.Formats...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you duplicating the StreamingData.Formats slice?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cybre@4a0c7d6

I forgot to change it from AdaptiveFormats to Format like you did in your solution

@cybre
Copy link
Contributor

cybre commented Jan 25, 2022 •
edited
Loading

I ran into the issue with slow downloads and tried this patch out and noticed some issues.
This PR introduces a new type for adaptive formats: AdaptiveFormat. The newly separated adaptive formats are thus not included in the Video type so we completely lose access to them. Additionally, even if they were exposed, Client in its current state only works with the Format type. Would it be possible switch back to a single Format type or create a format interface?

@jnelle
Copy link
Author

jnelle commented Jan 25, 2022

I ran into the issue with slow downloads and tried this patch out and noticed some issues. This PR introduces a new type for adaptive formats: AdaptiveFormat. The newly separated adaptive formats are thus not included in the Video type so we completely lose access to them. Additionally, even if they were exposed, Client in its current state only works with the Format type. Would it be possible switch back to a single Format type or create a format interface?

cybre@4a0c7d6

I think your solution in this commit is the best for now!

@jnelle
Copy link
Author

jnelle commented Feb 15, 2022

@corny @kkdai, should I fix the conflict and we merge this change?

@corny
Copy link
Collaborator

corny commented Feb 15, 2022

Please rebase on top of master and then let's check the integration tests.

@corny
Copy link
Collaborator

corny commented Mar 11, 2022

I tried to rebase your branch on top of master. Unfortuntely test TestYoutube_GetItagInfo shows that the number of itags drops from 26 to 6 for one video and there are more issues.

@corny corny force-pushed the master branch 2 times, most recently from 6ef3171 to bc29310 Compare March 12, 2022 10:05
@Clashkid155
Copy link

Any update on this?

@corny
Copy link
Collaborator

corny commented Apr 25, 2022

should be solved by #244

@corny corny closed this Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cybre cybre cybre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jnelle @corny @cybre @Clashkid155