-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Added response to error code for bad http status #9167
Conversation
Clears metadata cache and reloads playback when player runs into bad http status error code
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.
Hmm, that's not a good behavior in my opinion to clear the cache and retry each time this issue occurs.
For YouTube, this is an A/B test, they could roll out this change globally in UK or in the entire world, making your workaround useless.
This error affects other apps, even reVanced if I am not wrong, and must be fixed by extractor changes (and that's pretty easy in fact, by changing the primary InnerTube client used for playback (ANDROID
) or removing it, unless a real fix with it is found).
However, I think we could retry to get stream info only one time when this type of error occurs, without clearing cache. The expiration time is managed by NewPipe's cache system for each service. This would solve 403 HTTP errors when stream URLs have expired (for instance on YouTube and SoundCloud).
I don't mind changing it so it only retries to get stream info 1 time without clearing cache but I don't see how I would be able to do that without introducing a new private variable for Player.java to keep track of if it encounters the same error twice in a row. So, am I allowed to do that or no? |
I can also keep the original clear cache but it retries it up to x times before giving an error message? Like maybe 3 times? |
When running into a bad http status error code, metadata cache will be wiped and playback will be reloaded up to 3 times before giving an error.
So I've limited the amount of times the playback could be reloaded before it gave an error message. I've currently set the limit to 3 as with 1 attempt, the problem is a lot less common but still happens occasionally, with 2 attempts the problem was basically gone so I just went with 3 attempts to be safe. If you still want to go with your original suggestion where it doesn't wipe cache and only reloads once, then all is needed to do is set the limit to 1 and remove the InfoCache.getInstance().clearCache(); line. However, I feel like it would be fine to keep it as this is supposed to be a more general response to the error code rather than a specific fix for the 403 response code. |
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.
Would it be possible to clear the cache only of the currently playing video? That way we prevent the whole cache from being deleted (cache is there for a reason, to make things faster, so deleting it completely does not seem like a good idea).
Also, I think the badHttpStatusRetry
variable should be reset to 0 whenever the currently playing stream changes. Otherwise, with the current implementation, only the first 3 videos with failures would benefit from retrying.
if (badHttpStatusRetry < 3) { | ||
badHttpStatusRetry += 1; | ||
isCatchableException = true; | ||
// Clears metadata cache and then reloads playback |
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.
Add more comments here, with what you have in the PR description.
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.
I'm not sure if you can only clear the cache of the currently playing video.
However, I have tested the badHttpStatusRetry variable thing and it does reset back to 0 automatically whenever the player changes. For example, if you watch a different video. I think it's probably because the Player.java class gets created every time you watch a new video.
Sorry, I didn't think my workaround was necessary anymore, I also don't have the time to work on this right now. |
What is it?
Description of the changes in your PR
A lot of people (including me) have been getting a network error 403 response code randomly when starting a new video. 403 response code is when the client is forbidden from accessing a URL. A common fix for this was to wipe your cached metadata in settings and then reloading the video.
This PR works by wiping your metadata cache and then reloading the video playback if the player runs into any bad http status error.
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.
Due diligence