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.

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

Merged
merged 3 commits into from
Jul 22, 2021

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Jul 20, 2021

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

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 in PlayQueueItem that only compared the url, and since duplicate items have the same url they were considered the exact same PlayQueueItems. Using Java's default equals() 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. Removing equals() should be completely fine since PlayQueueItems are never clone()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

@triallax triallax added the bug Issue is related to a bug label Jul 20, 2021
@RickyM7
Copy link
Contributor

RickyM7 commented Jul 20, 2021

I just tested it and the bug was fixed, thank you very much!

  • Device: Redmi Note 4 (ArrowOS v11 - Android 11)

@@ -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) {
Copy link
Member

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.

Copy link
Member Author

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.

@ekiniko12
Copy link

It's finally working, thank you!

@Redirion
Copy link
Member

Redirion commented Jul 21, 2021

"for some reason" :D you have added this yourself Stypox:
2a2c82e#diff-6dcb0e241835a3f32ba00835e94c64b48b6faf84c0252769617507c1c2fd8371

PR #4562

@Stypox
Copy link
Member Author

Stypox commented Jul 21, 2021

@Redirion 🙈🙈

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.

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.
@Stypox Stypox force-pushed the fix-duplicate-items-queue branch from 2306519 to 736cefe Compare July 21, 2021 16:22
@Stypox
Copy link
Member Author

Stypox commented Jul 21, 2021

@Redirion take a look at the last 2 commits, and thank you for taking a look ;-)
I also added some tests

Copy link
Member

@Redirion Redirion left a comment

Choose a reason for hiding this comment

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

lgtm!

@RickyM7
Copy link
Contributor

RickyM7 commented Aug 3, 2021

@Stypox Was this included in the last update (v0.21.8)? I just tested this version but the bug still exists.

@Stypox
Copy link
Member Author

Stypox commented Aug 4, 2021

@RickyM7 no, 0.21.8 was a hotfix, just read the changelog

@RickyM7
Copy link
Contributor

RickyM7 commented Aug 4, 2021

no, 0.21.8 was a hotfix, just read the changelog

Got it, thank you!

This was referenced Aug 4, 2021
@randacco
Copy link

randacco commented Aug 7, 2021

Many thanks for this so needed fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Endless buffering on duplicate videos in queue
7 participants