Performance increase #4288
Performance increase #4288
Conversation
|
Some feedback regarding the orientation flow:
|
|
@avently Wow, dude! PR after PR. You're on fire!
Quite a difference from the unified debug!
Much better and more intuitive behaviour! Since this PR (along with the notification PR, as you said) basically fixes #4285, shall I link it?
Yes. The back button pauses, the fullscreen button doesn't.
Yeah, after the user lifts their finger, the app should go back to portrait.
Fixed in blackbox's rotation fix PR. |
Yep. So you have to options: you can press on fullscreen button and it will rotate a screen but not pause; you can press back button and it will rotate the screen and pause the playback. Something that I use often.
If I made like you're saying it will completelly broke the use-case with swiping the player down and selecting another video without changing the orientation. So now you can browse other videos while current video is playing. If you want to switch to portrait you'll click on fullscreen button. So I just don't see any profits from making like you said.
#4254 will make rotation possible while it's locked in landscape-only. Maybe it's better than hiding button, don't you think so? Since you can in portrait to swich to landscape via button than you should be able to do it from landscape to portrait. So I'm fine with the linked PR. Thanks for the feedback!
You could if you think that the PR in it's current state fixes your issue. |
Alright ;)
I understand the use-case and it makes sense. I wanted to say that the whole app may be locked in this orientation when the collapsed player is dismissed until a new video is opened and something like that could prevent it :)
Nice ;) It just felt odd without functionality but nice to know it's already done :D |
Yeah. If you lock the rotation while watching a video then the button should remain on screen, but when you press the button it should revert you to your devices default orientation. That's what my PR does, although the behaviour between the back button and fullscreen/rotation button is different as one pauses the video and the other doesn't. I often disable auto-rotate, so with the current code if I have a playlist that contains 5 vertical videos and 5 horizontal videos then I'll be flipped between portrait and landscape, which is annoying as they should all be played with my devices current locked orientation unless I specifically click on a button within NewPipe to change the orientation. And has anyone checked how an Android TV box handles this? Personally, I prefer the way v0.19.8 handles rotation as it gives you greater control of the videos that you're watching. Like with these dev versions if you lock your devices orientation to portrait and try to watch a vertical video in landscape then you can't do it anymore due to the buttons being merged. So in my opinion it would be better to have 1 button that toggles fullscreen and a separate button that toggles the orientation. |
Once vertical video starts after landscape in fullscreen the player exists from fullscreen but orientation is still landscape. I fixed it locally, so the player will stay in fullscreen on vertical video too. One line of code, will send with other changes if any of them be needed.
I think it's the matter of taste. For me it's ok to see rotation because without that rotation I will need to do many different steps to make the vertical video in portrait & fullscreen since I don't want to see the vertical video in landscape. I always like to see how my code doing something for me automatically:) If you have another use-case, you can make this behavior switchable from settings maybe it will be useful for others too.
Having two buttons when you can have just one is kind of unoptimized behavior and not good for UI and usabilty. Why do you want to look at vertical videos in landscape? |
Do you see improvements in terms of performance of your device? Does the rotation and prev/next videos switching works better and how much better (on % maybe)? |
I think that if someone locks their device to a specific orientation then they expect everything to use that orientation, unless they specifically press on a button within an app to change it. And then it should automatically restore your original locked orientation when you close the video.
If I started a playlist, locked my device to landscape and then put my device down then I'd expect every video to honor my orientation choice by playing in landscape. If it doesn't do that then it might eventually get to a vertical video and suddenly the image on my screen is sideways, which requires me to alter how I'm holding the device. So that doesn't respect my orientation choice and it's going to annoy people. Use NewPipe v0.19.8 with your orientation locked to portrait, start a mixed playlist and then press on NewPipe's button to change the orientation to landscape. It'll then respect your orientation choice and play every video in landscape until you exit.
I disagree. With auto-rotate enabled it might not be necessary, but with auto-rotate disabled there should be another button so that you have full control like you do with NewPipe v0.19.8. |
|
@blackbox87 when I read your posts I see myself in your posts. I mean, you talk like I would talk - plain, straightforwardly, unceremoniously. You know what and why you need, you have a use-case for every idea. It's interesting. Where are you from? I think we cannot be in agreement in multiple places because we have our use-case which will not work when we hear other's use-case. So that's why I often saying about a PR from you because there is no universal solution of how we can handle some situations. It's completely different vision on some thing, and you can choose only one or another, but not both. Fortunately we have settings in the app which allows to select one or another way for you and me. What if you make your ideas configurable from settings? |
I like to get to the point instead of beating around the bush. Sometimes it's appreciated and sometimes people don't like it, but at the end of the day I'm only trying to help. I'm currently living in the US, but I'm not originally from here.
I just prefer the way NewPipe v0.19.8 does it since that already respects your orientation choice, and you know what they say about people not liking change? So I really do believe that if the current method is pushed to the master then there's going to be some complaints.
I agree that there could be a setting so that NewPipe will either respect your orientation choice until you press a button or automatically changes the orientation depending on if it's playing a vertical or horizontal video. It's not something that I can setup though, since I'm not familiar enough with the existing code and I honestly don't have a lot of free time right now to keep contributing. |
Single video loads are roughly 2-3x faster. Now the unified player feels just as fast as 0.19.8! |
@blackbox87 That is what happens with this PR. So isn't your issue solved? It's only when you've locked to portrait that the player rotates when a vertical video plays. |
|
@opusforlife2 For landscape yes, but not for portrait. Both should honor your locked orientation IMO, otherwise you're forced to rotate your device depending on if it's a horizontal or vertical video. There's also a minor issue if you've got auto-rotate enabled.
It's missing a button to take you out of fullscreen, so you need to use your navbar instead and that pauses the video. |
Actually it's not rotating automatically when going from non-vertical to vertical for the same reason as why the player is not responding to click on fullscreen button with locked landscape orientation. So after #4254 the player will probably rotate the screen anyway (based on how this mechanism is implemented in #4254, I didn't check the code) |
|
I thought of something just now. I was thinking of how some media players have a 'lock rotation' button. Having yet another button would be unoptimal, though. Could we instead allow the full screen button to be long pressed to lock orientation? (There is no forced rotation when global orientation is unlocked, so that case doesn't need to be handled.) Maybe with a toast saying "Locked to portrait/landscape". |
Good idea, I like it. The info about such possibility should be shown somehow. But why it's needed? Why not to just make an option in settings? I don't think that someone will need to switch between these two scenarios often |
As much as I like the idea there might be no "simple way" to explain this feature to the user (the user wouldn't think about long pressing it, it's a toggle button in the end if you understand what I mean) |
|
When a user taps the full screen button for the first time, a toast could be shown telling them about the long press functionality. Regarding an option in settings, in general, the developers don't want to add a setting for every single use case. A setting would generally be added when it would be used by a large number of people. There have often been suggestions to have a Miscellaneous menu in Settings, but that hasn't been done yet, for whatever reason. |
|
I agree with having an option in the settings, since it's easier to understand. I'd label it something like "Orientation changes to suit aspect ratio", which is what this dev branch currently does. Disabling it should restore the behaviour that I'm talking about, as shown in v0.19.8. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
i have tried the apk performance.zip on android 7.0 and i didn't notice any difference the loading improved slightly, but when i rotate the screen it takes 2-3 seconds to show the video |
Is it better or worse than before? Is this result constant or happened once? |
the rotation always remains the same as before, only loads the video first |
|
Didn't understand your answer. You're saying:
How much time (in seconds) it takes on non-performance apk? For example, on any other unified player apk |
in version 0.19.8 when I rotate the screen is fast to show the video and there is no black screen left with the audio of the video that continues. In the unified player when I press play it takes a few seconds to show the video (i.e. to load the video source) and when I rotate the screen has the same delay shown in the previously loaded video.
I wanted to say that I have the same situation of the unified player that is when I rotate the screen the video continues to "move" feeling the audio "walk" but the screen remains black for 2-3 seconds |
|
Merged dev. Didn't tested after merge, there were no merge conflicts. @vkay94 now you can test performance with all optimizations (from this PR and notifications PR) |
Awesome! It's noticeably faster now! Thanks! |
Yep, I like the result. We do here many things that Youtube or other apps don't do. For example, we have an ability to view video description while in landscape fullsreen, we have a notification updates. Without it (like in Youtube) the rotation will be almost instant but I don't want to lose the ability to view description in landscape as well as the features provided by notifications (like background playback, controls, etc). There is a place for optimizaions (like always) so maybe the performance will be even better after someone's PR. |
That seems to work, but while testing this I noticed something that happens with your test version and the release candidate. Watch any video and then select "enqueue in the background" on another video. The audio will pause for a brief second, but then it starts playing the audio again without the video. The player displays a play icon and the notification shows a pause icon. Shouldn't the video remain playing? |
|
@blackbox87 why do you expect the video to be played when you choose to enqueue it in background? So why this enqueue exists from your point of view if the video will continue to play in main player? |
|
He means if you're playing video 1, and you enqueue video 2 in background player, video 1 will switch to playing in the background as well. Video 1 shouldn't be affected at all, right? Edit: if you try enqueue to popup instead, video 1 switches to popup. Again, unexpected and probably unwanted. |
Because I'm selecting to enqueue the other video in the background and not the video that's already playing. @opusforlife2 Exactly. |
|
@blackbox87 you talk about per-video player's type. Now there are three types of the player: audio (everyone except me call it background), video, popup. Changing the type happens in I don't think it's useful because it's really unexpected to see a video inside main player and then (after video ends) to see player change without any user interaction. Or even worse to see like player's type changes when you click next/previous video in the queue. For me it looks super unintuitive. All those buttons (like start playing in background, enqueue in popup and so) should be replaced with a better alternative but no one knows what alternative is. |
|
@avently Well it's not just me who seems to think that it should behave differently. But it's probably something you'd need more input on from others like @TobiGr and @Stypox. To me it seems obvious that if I'm watching a video and I click on "enqueue in the background" then I want to watch that video next and not stop the existing video 🤷♂️
I think "in the background" should be removed and it should instead work as a temporary session queue. So for example, while watching a video you might see a few other videos that you'd like to watch next, so you'd click on "enqueue" to add them to the session queue. And if you then wanted to start playing them all in the background all you'd need to do is press on the existing "background" button. |
|
Wait. I had written a comment, where did it go? Or did I just close the tab without posting it? -.- Anyway, I had written something similar to what blackbox has written above, that since the unified player is meant to have only one queue, enqueuing on popup or audio (there you go, avently!) should just add to the common queue and not affect which player it is. For example, if video 1 is playing in audio and you enqueue video 2 in popup, it should ignore the popup part and keep playing in audio mode. Same for any other player and enqueuing combinations. This is a band-aid solution until we can rethink queuing UI and behaviour for the unified player as a whole. |
|
@blackbox87 this is a possible solution, yes. But it limits current possibilities. So now you can drop the whole queue by starting to play one stream ('start in background') or you can switch the player by choosing to enqueue in popup. So the solution you are talking about limits the possibilities. I don't know is this acceptable or not. It's not hard to do so someone could make a PR and everyone can say their opinion. Since closing of performance PR I'll not be an active contributor anymore. My life changed so much so I'm not interested in doing something large for youtube app. If you expect that I'll start making your ideas alive, don't. |
|
You've done more than enough for Newpipe and its users. Thank you so much! 🥳 |
|
@opusforlife2 you're welcome and thank you too:) You're doing so much as well as other maintainers. This is hard job but looks like you like it |
|
@avently Honestly, my suggestion was just a quick idea. I figured I'd put it out there since @opusforlife2 seems to agree that the existing behaviour is most likely unexpected and unwanted. But to come up with a true solution is going to require input from a few people. I don't expect you to fix every issue, if you even see this as an issue. But regardless we've all got other things going on in our lives and we can only contribute so much at a time. It's not like we're getting paid to do this. Good luck to you. |
|
i think we should replace "enqueue in backgorund / popup" with one option: "enqueue". Switching to other players is very unexpected behaviour. |
Thank you for your contributions! Good luck in real life :) |
|
At least 3 + 1 (me) people agree to remove different player's types from menu. This is a good start.
I'm not dying, lol. |
Looks great. The delay is gone 👍 |
There was a problem hiding this comment.
Thank you again for your contributions, and good luck with life :-D
@opusforlife2 you can merge ;-)
|
First steps of @opusforlife2 as a maintainer :) |
|
٩( ᐛ )و |
TobiGr
added
the
player
What is it?
Description of the changes in your PR
This PR for devs and for users.
For devs:
I made optimizations for managing the player service. Now the player service is bound to static class and there is static listener for all events from the player service. VideoDetailFragment can subscribe/unsubscribe from events dynamically without the need to bind/unbind to the service. Fragment only need to subscribe/unsubscribe to/from events via static PlayerHolder. It gives possibility to skip binding process on configuration change and to speed up orientation change.
Also I discovered that
prepareAndHandleInfois terribly slow, something around 1 second on slow device. Whenever currently playing item in playlist changes this method is called and freezes main thread for a long time. Ideal choice would be to rewrite some methods that populates view groups but it's hard to do and time consuming task. Instead I call this method (prepareAndHandleInfo) after timeout which makes main thread free for some time which allows to start playback on next/prev item in playlists fast.Also I found that SerializedCache clones every item in it which takes some time (100-200 ms). So I removed that part from fragment and saving playQueue, streamInfo and stack inside static variables.
For users:
Faster loading time of streams, faster orientation change.
Also when you have horizontal + vertical + horizontal videos in queue this is what happens:
In locked global orientation (portrait):
next arrowbutton and orientation will be the samenext arrowbutton from landscape video to vertical will change orientation to portrait, then after vertical video will change to landscape again for landscape video.In locked global orientation (landscape):
In unlocked global orientation:
If you read correctly you noticed that the player will rotate the video from landscape to portrait and back if you selecting vertical video after/before landscape with locked global orientation. It's useful thing but with a cost. And the cost is wrong size of the video for about 0.1-1 second based on performance of the device. This happens because the app still doing many things in main thread. Mostly because of notifications. Yes, they are terrible slow and nothing we can do in this area except to wait until this PR #3178 will be merged. I hope it fixes the notifications' performance but if not the delay is still acceptable. But even if the notifications will be fast, it doesn't help to reduce the delay to 0 because when orientation changes from one to another there is a delay related to reconstruction of the whole fragment. I made this process faster but the delay is still visible on slow devices.
Here we have two APKs: the first one is what this PR gives. The second one shows what it means to have all notifications disabled. So I just tested with disabled notifications and the app is soooooo fast when orientation changes and between videos in queue. Of course, we still need the notifications but it's just for educational purposes:)
Relies on the following changes
Based on #4272
Testing apk
This PR:
performance.zip
This PR + disabled notifications (just for experiment)
performance-without-notifications.zip
Agreement
This PR starts at 5b8eda4