[WIP] New embeded in fragment video player

[avently]

What was done in this PR:

• all players implemented in one service.
• video player located at the same window as video title, description, related videos, etc. You don't have to switch to another window. Everything in one place.
• popup player supports playlists. Yes, next and previous videos can be selected via popup window.
• easy switching from one player to another. Playing position will be the same. Even current video from playlist will be the same in other player after switching.
• swipe controls improved. Now you can rewind/forward a video via swipe to the flexible number of seconds/minutes. Also previous brightness value will be saved and used after restart.
• fullscreen and half screen support. If you want to read description while watching a video it is possible now.
• option in settings which will allow to play an audio while you are hiding the app and using an other app. Video will be disabled and your internet traffic can relax with only audio. Then after returning back to the app video will start working again.
• autoplaying all videos option. You don't have to click on a play button if you don't want it. This can be done automatically.
• autoplaying while your phone uses landscape orientation.
• you will see next/previous video buttons only when its needed.
• if you want to contribute to NewPipe you'll notice how much code was removed and simplified because now all players located in one file. That's insane, yes.

• [ + ] I carefully read the contribution guidelines and agree to them.

[karyogamy]

Good job, I've briefly went over the changes and tested it, and as you've said, it does feel pretty smooth.

However, here are some major things I saw:

1. The player is being built right into the MainActivity. Please don't do this, the rotation on player does not work the same way as it should, since MainActivity is destroyed on configuration change and this will cause the player teardown and rebuild.
2. To continue the above point, have you considered using a separate video player service instead of a fragment? This way, no major changes needs to be made to the player views (quality selector, etc.) as all commands and views can be sent via binder and listeners.
3. The UI is awkward when trying to use the "Play All" feature of playlists, I almost thought this was broken since there is no indication that videos are queued on the player. We might need to rethink how play queue is represented in this case.
4. The orientation button on main player is gone. User may wish to keep a certain rotation all the time: Switch to landscape automatically #714.
5. The title and uploader name is gone on main player is gone as well. This should at least be optional.
6. Please follow Java variable naming convention: no underscore before local variables.

Sorry for this quick review, I will do a more thorough review again soon. In the mean time, keep up the good work. I should be able to answer questions regarding the players and play queue if you need any help.

[avently]

@karyogamy
1,2: how to do this? Maybe link to example? I hate that player rebuilds every time.
3: yes, it looks weird. I could add playlist under video but I don't think it's useful when you're watching video in full screen. I like the ability to select video from playlist while watching. Maybe just to show playlist in the same place automatically when a user presses Play all? It can be done easily without big changes.
4: should I return that button and leave my new way of handling orientation change? Ok.
5: you think it's cool to see title and uploader name twice? In video player and under video. I don't think so. Maybe I can show this info only in full screen. That's a good alternative.
6: ok.

[avently]

4: this button shouldn't exist. If a user will press on it new autorotation mechanism will not work as expected. Author of the issue #714 wants video rotated only when video recorded in landscape mode. So if he wants to see the video in the landscape orientation he will rotate a device, right? Then new autorotation will rotate screen too as the user wants. What's wrong?
@flipflop97 add your comment, please.
5: i will show title and channel in all situations. Temporary hiding it looks bad.

[theScrabi]

@avently a famous german computer magazing wrote they liked NewPipe especially the rotation button was something they mentioned. Now calculate the chances I'm going to let that button be killed :P

[ghost]

@theScrabi: I like that button too. Btw you're talking about c't, right? It wasn't in the Dutch version though.

[theScrabi]

Yes the c't. Didn't know that there is a dutch version of it.

[karyogamy]

You can take a look into PopupVideoPlayer, which is a service binding (PlayerServiceBinder) with PopupVideoPlayerActivity(extending ServicePlayerActivity). This is how popup and background players interact with their respective UI. You can then start a player service in the details fragment and create a different binder which allows you to pass the video view to the player in the service.

For example, once the player view is changed (e.g. turn to full screen), you can call the service to changed the video surface like this simpleExoPlayer.setVideoSurfaceView(newSurfaceView), without having destroying the player. You might still need to keep this service in the foreground, even though this will create a notification (background service might get killed at anytime).

[avently]

I done this pull request. Everything I and karyogamy and theScrabi wanted is working. So you can merge it and use as a base of the next commits.

[avently]

@theScrabi what's your thoughts about this PR? Do you plan to review it? Maybe the code looks bad or something else

[theScrabi]

There are to much conflicts atm could you please resolve them.

[avently]

@theScrabi done. Also I readded fullscreenButton to popup because without it we can't close popup window.

[avently]

@theScrabi could this be merged? My new PR will be based on this code.

[theScrabi]

All right. I'm sorry for that long delay. I finally tested this now.
Great work so far :), but I noticed several things that require a change:

Fullscreen and rotation

• Instead of having a fullscreen button you should make the video automatically fullscreen when the rotation button is pressed (like when rotating the original youtube app).
  If NewPipe is already in landscape mode, then directly show the Video fullscreened when pressing play.

Stop the video when NewPipe is offscreen

• Currently the video Keeps playing when NewPipe is offscreen. Please don't do that, since the whole video keeps still being streamed. Maybe it was better to automatically switch to the background player if NewPipe gets to be offscreen. Also you should consider to add an option in the settings so people can decide the behavior of NewPipe themselves.

Remove search Icon

• The search Icon should not be prescient in the video screen. You should go back to the video overview for searching.

Switching back from Background player

• When switching back from background/popup player the normal video should continue to play. Atm it stops until I press play again.

[avently]

@theScrabi, please, review.

[theScrabi]

Play video in fullscreen mode when device in a landscape orientation

It would be nice if full screen is disabled again if switching back to portrait mode.

Disable video while in background

I see that you are switching to a player with your own notificatoin inteface. Please use the original background player instead.

Fix for situations when autoPlay disabled in settings

Well you removed the rotation button again. I ment that the rotation button should remain, and that the video starts in fullscreen when in landscape and non fullscreen when in portrait. The way of changing the rotation should not be altered.
Please take the fullscreen button away, not the rotation button.

[avently]

@theScrabi tell something about the last commit related to screen rotation. Is it what you are asking about?

[avently]

Anyway with the last commit i made performance much better. Video in main player and in popup opens so fast. When phone is in screen rotating process views redrawing faster. So, maybe performance issues, you, @mauriciocolli, mentioned is fixed.

[theScrabi]

Perfection. The behaviour of the screen rotation is ok now :)

[avently]

@karyogamy I added you method but it is unused because I can't check how it works on old Android versions with new player interface.
ae6dd19#diff-57eb59a31e38fed8f965fb4a544e3b9fR1261

Maybe without it will be ok. Or not.

Some parts of your commits were not copied to this code (code for old player's activity lifecycle, for example). So be aware

[avently]

I'm done with this PR since now.

I will not add any single line of code to that feature until the PR will be merged. So now it is your problem to implement embed in fragment player. You may use my code or you can make your personal implementation for that feature, I do not really care about that. You can even close this PR. Happy codding!

[karyogamy]

@avently Sorry for the late reply.

As stated from my previous comment, I have started working on a separate player library to, hopefully, integrate with newpipe in the future. This is being done because our player is steadily becoming more complex and less modular with respect to the app. And without any unit test, which is mainly my fault, refactoring and code changes to the player (or the app itself) is almost always going to introduce bugs, not mentioning the additional work needed for reviewing (and manual testing is simply not enough at this point). So this library aims to address these problems with the player, while also provide some support extensions for exoplayer in other apps.

I can't give a strict deadline on when this will be done, though the initial commit will probably come this weekend or the next. We'll be glad if you are around to help with embedding the new player to the detail fragments (which won't be part of the library), as you are knowledgeable with their subtleties. But if not, I completely understand and wish you all the best.

[GKid94]

So when will this be implemented in the app?

[justanidea]

Its apparently on ice, waiting for merging, sadly

[theScrabi]

As @karyogamy points out, and as we said multiple times in this thread. It wasn't a good time to introduce this changes. They deeply impact NewPipe, which makes it complicated. I promis this player will come, but please stay patient. So that in the end you don't get it with a good, but best quality 😉

[avently]

Here is a translation of the previous comment:
I can't do anything better than this PR. I will wait until profi like karyogamy and mauriciocolli will make it for me. Now I don't really know how my application works since I don't do anything important with codebase for months (or years?). I don't care about users who asking about this feature (and comments support based on this feature) here and there for as long as a year.

So, that's reality I see in NewPipe world. This app is alive, thanks for it's contributors. But I think it is important to make decisions that do not limit app success. Because now with your decisions app will die in a moment when karyogamy will lose his interest or time that he can give for PRs. I don't want it. @theScrabi I don't want to read your answer. I just wanted to say all this. If you are responsible for the app now create something your users ask for such long period. Or you are not the guy who created the great app. Maybe you are not ready for changes.

[avently]

And for the record, you are not allowed to reopen this PR. Create your own. This will be the history of my fail.

[theScrabi]

I am quite open for contributions, but sometimes some decisions have to be made. NewPipe is quite a big project, and its quite hard to maintain. I sometimes have to decide which contribution is good to be added and which is not in order to maintain a good quality. The bigger a project becomes the more important it gets to keep track of the quality. Our contribution node clearly states one who wants to contribute has to be patient when we denial something because of changes, and sometimes one has to accept if we just say no.