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

Use ExoPlayer default values for buffers #6515

Merged
merged 1 commit into from
Aug 29, 2021

Conversation

Redirion
Copy link
Member

@Redirion Redirion commented Jun 18, 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

ExoPlayer 2.12 noted this in the changelog:
Set the default minimum buffer duration in DefaultLoadControl to 50 seconds (equal to the default maximum buffer), and treat audio and video the same.

Maybe there was a reason they decided to have both minimum and maximum buffer identical since 2.12. NewPipe used years old constants which are now removed in favor of ExoPlayer constants so that future versions will keep up with what changes at ExoPlayer.

Fixes the following issue(s)

Hopefully fixes some of the buffer issues we have lately like #6510

APK testing

On the website the APK can be found by going to the "Checks" tab below the title and then on "artifacts" on the right.

Due diligence

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Jun 18, 2021

It does seem like this solves the issue.

In fact, it seems to have taken Newpipe back to the old behaviour, where it keeps buffering the video endlessly, unless you rewind or fast forward (may not work), or change the resolution (usually works).

Edit: Ah. Got a couple of "Could not play this stream" as well. This, too, was the old behaviour.

Regardless of these old issues with buffering, I would consider the "repeated rebuffering" bug solved.

@triallax triallax added bug Issue is related to a bug player Issues related to any player (main, popup and background) labels Jun 19, 2021
@tsiflimagas
Copy link
Contributor

@opusforlife2 I got the same error too, without this commit, so maybe it's not related to it.

@Redirion Redirion mentioned this pull request Jun 19, 2021
5 tasks
@litetex
Copy link
Member

litetex commented Jul 16, 2021

Is this pull request anymore required?

The exact problem was found and we got #6673 that hopefully fixes it 😄

@XiangRongLin
Copy link
Collaborator

Unless there are good reasons to deviate from the default values, I would still merge this. The guys from exoplayer probably put more thought into those values than we.

@tsiflimagas
Copy link
Contributor

Is this good to merge? I've used it for some days now and seems fine (not that I actually noticed some difference).

@Redirion Redirion requested a review from TobiGr August 29, 2021 08:34
Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Looks good to me. I did not ran into any issues while testing.

@Redirion Redirion merged commit 8bfd380 into TeamNewPipe:dev Aug 29, 2021
@Redirion Redirion deleted the buffersharmonization branch August 29, 2021 15:44
This was referenced Sep 5, 2021
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 player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants