Skip to content

Comments

fix duration can not be paresd#177

Merged
theScrabi merged 1 commit intodevfrom
duration_fix
Jul 31, 2019
Merged

fix duration can not be paresd#177
theScrabi merged 1 commit intodevfrom
duration_fix

Conversation

@theScrabi
Copy link
Member

@theScrabi theScrabi commented Jul 30, 2019

this will fix the duration error that came up recently:
#176

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Before reading GitHub notifications I delved into playerArgs too, and I found out how to fix this bug by myself. Only then I saw you had fixed it already :-).

By the way playerArgs["player_response"] contains many interesting things (among which "videoDetails object, formats...). What about replacing all suitable functions in the YoutubeStreamExtractor with player_response-based functions? As you said it is simple and fast to parse json, and player_response seems to have the same format in "youtube.com/get_video_info" so we could remove some duplicate code for age restricted videos. But this is for another pr. :-D

@Stypox
Copy link
Member

Stypox commented Jul 30, 2019

Oh, by skimming through YoutubeStreamExtractor I also found out why NewPipe always fetches location-dependent strings instead of the chosen ones. It is because downloader.download(...) is called only with the url and not with the localization parameter. But this is also for another pr.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Sorry if I do another review, but I didn't see you had fixed sts too. ;-)
Btw if you think there is something wrong in the way I do reviews please tell me, since I haven't done many of them.

@theScrabi
Copy link
Member Author

theScrabi commented Jul 30, 2019

This:
grafik

The F****** extractor is literally untestable at the moment. Everytime you run it other tests fail. We need to fix that otherwise our whole CI is BullS****.

@theScrabi
Copy link
Member Author

Or there is a S***load of changes on the youtube webpage coming around, and sometimes the extractor hits those servers with the new site.

13 failed. This is a catastrophe.

@Stypox
Copy link
Member

Stypox commented Jul 30, 2019

🤔 Many of the errors are caused by ReCaptcha...

@theScrabi
Copy link
Member Author

theScrabi commented Jul 30, 2019

thinking Many of the errors are caused by ReCaptcha...

uff. So how are we going to tell the the CI?
Maybe we "bruteforced" youtube from our CI so often that they made Travis use ReCaptcha.

@Stypox
Copy link
Member

Stypox commented Jul 31, 2019

What about creating a custom test runner that, for example, ignores ReCaptcha exceptions and only generates warnings for classes where more than 60% of test functions fail?
Screenshot_20190731_103406
This happened to me once, and I wasn't able to reproduce it afterwards. No ReCaptcha exception was thrown, but the downloaded page lacked content anyway. With the custom runner you would just be warned that YoutubePlaylistExtractorRest$HugePlaylist couldn't be run.

Another option would be to wait some time before every network request: this way youtube should not create robot-verification problems.

@theScrabi
Copy link
Member Author

Yes :D, the idea with a custom test runner sounds good. Was it possible to "re run" the test if it failed? For that we could check in the setup function of the test class weather the request went through well.

@theScrabi theScrabi requested a review from Stypox July 31, 2019 11:25
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Now all seems to be ok. I ran all the YouTube tests on my pc until I got a random error, and they passed 10 times in a row (probably my pc is slower than the build one, so it overloads Youtube servers much less). This means that the code is all good, we just need to add a custom test runner in another pr just after this one.

The things I pointed out in this review are just small ones, then the pr is ready :-)

update gradle to version 5.1

fix sts issue for agegated videos

GOD DAMN FUCKING BULLSHIT

add duratin for controversal/age gated videos

bring back sts

remove ignores

fix ogg test
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

I think we're good now. Thank you :-)

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.

2 participants