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

Update to ExoPlayer 2.12.3 #5457

Merged
merged 2 commits into from
Mar 18, 2021
Merged

Update to ExoPlayer 2.12.3 #5457

merged 2 commits into from
Mar 18, 2021

Conversation

Redirion
Copy link
Member

@Redirion Redirion commented Jan 19, 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

Updates ExoPlayer from 2.11 to 2.13

Note that we will have to do another PR to migrate from deprecated mediasession to media2.
However this will most likely require to also rework everything to the new MediaItems instead of working with MediaSources.
For now however this can stay and if ExoPlayer stays being compatible with mediasession we aren't in a rush.

Due diligence

@TobiGr TobiGr added the player Issues related to any player (main, popup and background) label Jan 19, 2021
@TacoTheDank
Copy link
Member

TacoTheDank commented Jan 21, 2021

@Redirion Also, I think media2 only supports API 21 and newer, so that would require dropping KitKat support, which will definitely cut off a small chunk of users 🤔

@Stypox
Copy link
Member

Stypox commented Jan 21, 2021

@TacoTheDank we will be forced to drop KitKat support sooner or later anyway due to OkHttp 3.12.x being discontinued on December 31, 2021

@TacoTheDank
Copy link
Member

@Stypox All okay with me lol, though I do feel kinda bad for those older phone users. The only option they really have left now is SkyTube or using a mobile browser. (Also, you should edit your comment to specify the 3.12.x LTS branch, because you scared me with "OkHttp being discontinued" 😰)

@Redirion
Copy link
Member Author

@TacoTheDank do you have a source for the API 21 requirement of media2 for me? I wasn't able to find that info.

@TacoTheDank
Copy link
Member

TacoTheDank commented Jan 22, 2021

@Redirion
Copy link
Member Author

found something. According to this website media2 in the current version that is used by ExoPlayer requires API 16: https://androidx.tech/artifacts/media2/media2-session/1.1.0

while the media2 extension of ExoPlayer declares API 19: https://github.com/google/ExoPlayer/blob/release-v2/extensions/media2/build.gradle (this could however be overwritten by our gradle config, if ExoPlayer raises this to 21, as long as media2 stays below 21).

@TacoTheDank
Copy link
Member

TacoTheDank commented Jan 22, 2021

@Redirion I see... 🤔

Oh btw, ExoPlayer 2.12.0 introduced two convenience methods play() and pause(), which are equivalent to setPlayWhenReady(true) and setPlayWhenReady(false), respectively. There are two usages of setPlayWhenReady in https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/player/helper/AudioReactor.java that could be replaced with the convenience methods. (Here's the link in ExoPlayer's release notes)

@Stypox
Copy link
Member

Stypox commented Jan 22, 2021

The only option they really have left now is SkyTube or using a mobile browser

@TacoTheDank they would still be able to use NewPipe legacy

@opusforlife2
Copy link
Collaborator

@Stypox Does Newpipe Legacy not use OkHttp?

@Stypox
Copy link
Member

Stypox commented Jan 22, 2021

It does, but I think it can be kept or replaced with something else specific to old android versions

@TacoTheDank
Copy link
Member

TacoTheDank commented Jan 25, 2021

The only option they really have left now is SkyTube or using a mobile browser

@TacoTheDank they would still be able to use NewPipe legacy

Oh yeah, and that :)

@TacoTheDank
Copy link
Member

Is this good to merge?

@Redirion
Copy link
Member Author

@Stypox I am a bit hesitant to merge my own PR. From my opinion I would say this is good to merge. Or would you prefer if we skip ExoPlayer 2.12 and move directly to 2.13?

Robin and others added 2 commits February 16, 2021 16:42
Removed a textTrack null check on a now- NonNull method
Added a error type switch case (TIMEOUT)
@Stypox
Copy link
Member

Stypox commented Feb 16, 2021

I added a commit with a few style improvements and fixes of warnings. I tested playing some videos and enabling skip silence and everything worked as expected

@Redirion
Copy link
Member Author

Now we seem to have the situation, that we require a third dev to approve this PR as we both contribute to it

@opusforlife2
Copy link
Collaborator

You could always approve each other's changes. :P

@opusforlife2
Copy link
Collaborator

@Redirion Were you ever invited to join the Newpipe channel on IRC? If not, could you please make a Matrix account and join it?

@TobiGr TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24
@Redirion
Copy link
Member Author

You could always approve each other's changes. :P

I just tried but I may not approve my own pull request even though the last commit was not by me

@Redirion Redirion requested a review from Stypox March 18, 2021 14:57
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 tested again and everything works fine. Thanks :-)

@Stypox Stypox merged commit 8638169 into TeamNewPipe:dev Mar 18, 2021
@Redirion Redirion deleted the exo123 branch March 19, 2021 14:50
@opusforlife2
Copy link
Collaborator

Were you ever invited to join the Newpipe channel on IRC? If not, could you please make a Matrix account and join it?

@Redirion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants