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

Rewrite the player (and all its components) #7939

Closed
5 tasks done
AudricV opened this issue Feb 23, 2022 · 9 comments
Closed
5 tasks done

Rewrite the player (and all its components) #7939

AudricV opened this issue Feb 23, 2022 · 9 comments
Labels
feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background)

Comments

@AudricV
Copy link
Member

AudricV commented Feb 23, 2022

Checklist

  • I made sure that there are no existing issues - open or closed - which I could contribute my information to.
  • I'm aware that this is a request for NewPipe itself and that requests for adding a new service need to be made at NewPipeExtractor.
  • I have taken the time to fill in all the required details. I understand that the feature request will be dismissed otherwise.
  • This issue contains only one feature request.
  • I have read and understood the contribution guidelines.

Feature description

The player codebase is pretty hard to understand, is very old, and uses several deprecated methods (which may have been already removed in the latest ExoPlayer versions)/old ways to do some things in ExoPlayer.

The current implementation also does not really allow the implementation of the Android Picture and Picture API (or it is really hard to do), required for Android 12+ devices to prevent untrusted touches outside the popup window, as an activity or a fragment is required to enter the PiP mode. The player seems to be added manually with a view within the VideoDetailFragment (that's probably not a good idea at all to enter this fragment into PiP mode).

We currently have some player bugs for which we don't really have a way to detect them and/or fix them, like the playback position reset when switching at least manually player type (that we fixed with a workaround).

We may split the player into two parts such as a part in a fragment and the ExoPlayer part in a separate class and file.

Why do you want this feature?

I want this "feature" mainly to fix weird player bugs, update to new ExoPlayer APIs, fix popup mode on Android 12+ devices but also to improve code readability and maintenability and make comments about the player code such as the player is a hot pile of 💩 or the code is a spaghetti monster of doom gone.

@AudricV AudricV added feature request Issue is related to a feature in the app player Issues related to any player (main, popup and background) labels Feb 23, 2022
@SC1040-TS2
Copy link

This is also necessary now more than ever due to the way both the player and NewPipe's Extractor are currently failing to reliably parse and play WebM videos. Particularly WebM VP9 videos are causing Extractor errors.

@Stypox
Copy link
Member

Stypox commented Feb 24, 2022

Something I was thinking about the other day is the following. We should differentiate between the player logic and the UI.

  • I'd create a PlayerLogic class that handles the exoplayer, for example by handling "next", "pause", "seek", ... actions, player errors, playback listeners, playback parameters, initializing and taking care of the queue, choosing whether to play only audio or not, and possibly other things.
  • Then I'd create multiple PlayerUI classes that take care of the different types of UI we have. The PlayerLogic can interact with multiple UIs at once, thanks to listeners, intents or the UIs calling methods on the PlayerLogic instance (when applicable). The UIs I was thinking of are:
    • NotificationPlayerUI handles creating and maintaining the notification, and reacts to notification actions by sending them to the PlayerLogic
    • MainPlayerUI handles the main player inside the video detail fragment, and provides the PlayerLogic with a surface on which to draw videos
    • PopupPlayerUI handles the popup player fragment in the PiP, also provides a surface like ^
    • QueuePlayerUI shows the play queue (be it for the background or popup player)
    • VideoDetailFragmentPlayerUI (maybe with a better name) handles the bottom mini player and shows the progress of background/popup players in the video detail fragment if needed

@karyogamy
Copy link
Contributor

* I'd create a `PlayerLogic` class that handles the exoplayer, for example by handling "next", "pause", "seek", ... actions, player > errors, playback listeners, playback parameters, initializing and taking care of the queue, choosing whether to play only audio or > not, and possibly other things.

Separating the player logic is definitely a good start. Now that the player feature set is mature enough, is it possible to design a model that captures the full player state so it can reliable reconstruct, for example, after a crash?

Following from that, what about the size of the player state. Do we want to keep an entire play queue in memory with thousands of items? Or continue passing that via intent? Should the player state be persistent in database and constantly updated perhaps?

And if the player can be reliably reconstructed, would it make sense to drop some complex features, such as the on-the-fly video quality selector?

@opusforlife2
Copy link
Collaborator

Is it possible to refactor the player in small chunks instead of one huge PR? I'm thinking if it is possible, the effort could be spread out over several releases (since it's not a user-facing change), so that no one dev is burdened by a huge TODO.

@karyogamy
Copy link
Contributor

Stumbled upon this blogpost from ExoPlayer, which will be merged into AndroidX Media3 in the future. Maybe the player rewrite should target Media3 instead of building from scratch again, since it has an interesting set of dependencies, including ui and cast.

@AudricV
Copy link
Member Author

AudricV commented Mar 22, 2022

Note that the cast extension cannot be included in NewPipe as it uses Play Services components.

@Stypox
Copy link
Member

Stypox commented Jul 18, 2022

I am closing this because the main point has been solved by #8170. There are obviosly more things to do, but they are rather small ones, so I'd track them in #8616.

@Stypox Stypox closed this as completed Jul 18, 2022
@AudricV
Copy link
Member Author

AudricV commented Jul 18, 2022

Indeed, but doesn't a partial rewrite is still needed for the Android Picture in Picture implementation in the app?

@Stypox
Copy link
Member

Stypox commented Jul 18, 2022

Yes, definitely. I added that to the TODO list in #8616

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

No branches or pull requests

5 participants