-
-
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
Fix duplicate items in queue causing endless buffering #6712
Conversation
I just tested it and the bug was fixed, thank you very much!
|
@@ -64,20 +64,6 @@ private PlayQueueItem(@Nullable final String name, @Nullable final String url, | |||
this.recoveryPosition = RECOVERY_UNSET; | |||
} | |||
|
|||
@Override | |||
public boolean equals(final Object o) { |
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.
Oh my! what a bug xD
Did you check the usages of the method? Maybe someone implemented this on purpose.
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.
Android Studio's "Show usages" yields all of the usages of the equals()
method on any object of any class, so that's not much useful. I tried to manually look for usages by checking out the methods in PlayQueue
and in Player
that use PlayQueueItem
comparison and could not find any issue.
The only downside I can think of is that if the same video is queued multiple times in a row, the loading strategy may start to load the current and the next stream contemporarily, but since those streams point to the same url but do not compare equal, they will be loaded twice. But this only happens if two play queue items with the same url start loading contemporarily, because otherwise the caching method prevents reloading something that already was loaded. I don't think this is really a downside.
It's finally working, thank you! |
"for some reason" :D you have added this yourself Stypox: PR #4562 |
@Redirion 🙈🙈
Now I have to understand what I meant 😅😅 |
From TeamNewPipe#4562: Disable player stream preloading only if the current stream is going to be replaced for sure (see this). equals() was implemented for PlayQueueItems, so that (only) the url is compared when checking them.
2306519
to
736cefe
Compare
@Redirion take a look at the last 2 commits, and thank you for taking a look ;-) |
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.
lgtm!
@Stypox Was this included in the last update (v0.21.8)? I just tested this version but the bug still exists. |
@RickyM7 no, 0.21.8 was a hotfix, just read the changelog |
Got it, thank you! |
Many thanks for this so needed fix! |
What is it?
Description of the changes in your PR
This is a simple bugfix that allows having play queues with duplicate items. For some reason an
equals()
method was implemented inPlayQueueItem
that only compared the url, and since duplicate items have the same url they were considered the exact samePlayQueueItem
s. Using Java's defaultequals()
method, that just checks if two objects are the same with==
, solves the issue, since duplicate items were anyway created one by one and thus are never identical. Removingequals()
should be completely fine sincePlayQueueItem
s are neverclone()
d, and therefore we never end up with two objects that should be considered the same but down in memory aren't because they were cloned.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.
I am not completely sure that this doesn't introduce regressions, so it needs some testing. @ekiniko12 @RickyM7 @sqtvrnqliq could you check if the endless buffering on duplicate videos in queue is solved?
Due diligence