TeamNewPipe / NewPipe

A libre lightweight streaming front-end for Android.
https://newpipe.net
GNU General Public License v3.0
31.32k stars 3.05k forks source link

[Feature Request] YouTube's fast forward/rewind behavior #4264

Closed vkay94 closed 2 years ago

vkay94 commented 4 years ago

Describe the feature you want

It's a replacement of the current fast forward/rewind functionality. For a demonstration watch this video (https://streamable.com/e/qhuca0) or see the images below.

image image image

Is your feature request related to a problem? Please describe it

No.

Additional context

When you try the debug (app-debug-seek-20200915-02.zip) then you should keep this in mind:

The current dev branch has problems with seeking for many videos. You can verify it by seeking the seekbar and check if there is a delay (visually). I recommend these two videos for testing: https://www.youtube.com/watch?v=xTJcASRxbfI and https://www.youtube.com/watch?v=pXRviuL6vMY They haven't got that great delay and therefore the UI should response better (less jerky) than on most over videos with this build.

How will you/everyone benefit from this feature?

I think it feels more natural when

I'm looking forward to your opinions ;)

opusforlife2 commented 4 years ago

Nah, that's a hard video. Could you confirm something for me? As I've told often, many videos have a noticeable seek delay in the dev branch. Can you install the latest unified player test apk and test this video? Just press somewhere on the seekbar and check if there is a delay. If yes, it does many work on the main thread => jerky.

Actually, for me, seeking to a non-preloaded part takes 3-4 seconds, whether it is this video, another 60 Hz video, or even a 30 Hz video.

When seeking to a preloaded part, the delay is roughly the same for me between 60 and 30 Hz videos.

vkay94 commented 4 years ago

Same applies to me except of very few videos. That's why I recommended the two videos I've shared because they haven't this heavy delay (for me), neither on my debug build nor on the unified player test apk. I really can't tell why.

opusforlife2 commented 4 years ago

Let's test again after avently's performance PR is merged.

vkay94 commented 4 years ago

Update: app-debug-seek-20200924_01.zip

Added:

Changed:

Improved

Fixed:

Note: I disabled all notification updates for now due to performance issues for videos with high resolution thumbnail (wait for notification/performance PRs).

opusforlife2 commented 4 years ago

Stop animation when video start/end is reached (no endless animation)

I didn't see this issue with the previous build. How did it occur?

Switch from forward to rewind (vice versa) requires double tap, so no "fast switching" possible.

Works as expected! Thanks!

If the video is stopped, in every case the shadow stays and the arc background (if enabled) is light grey (with higher transparency).

Looks better now. πŸ‘

Trigger seek-Callback on Action_Down (previously Action_Up like YouTube). It may feel a little bit "faster" because the animation won't wait until you lift your finger.

It does indeed feel more fluid. Good thinking.

Added Checkbox to switch between light grey and dark arc shape color.

Dark is better! But now we have dark for when video is playing and light for when video is paused. Shouldn't both be dark for consistency?

Call seekBy-method only during fade-out animation instead of each tap (UI becomes less jerky). During the animation the playback stops - similar to dragging the seekBar.

Yup. The stutter is gone. The animation is perfectly smooth now. Even in the previous build, if the video was buffering, the animation would be smooth. It was only when you were seeking and the video would immediately play after that the animation would stutter.

avently commented 4 years ago

I like the latest version. Arc radius is consistent in non/fullscreen mode, no delays, fast show/hide, overlay always appears when you end previous sequence of taps and start a new one. Darker is more universal: it's ok on dark and light videos, but white is not ok on light videos. So dark is preferred "theme". It looks and works great

vkay94 commented 4 years ago

Thanks for the feedback ;)

I didn't see this issue with the previous build. How did it occur?

Basically it happened every time because there was no check for the player position (UI/UX was/is my prior in this phase). For example if the video was at 00:25 und you had fast rewinded, the animation wouldn't stop at 30 seconds but keep going until you stop tapping.

Works as expected! Thanks!

Glad to hear that .D

Dark is better! But now we have dark for when video is playing and light for when video is paused. Shouldn't both be dark for consistency?

Darker is more universal: it's ok on dark and light videos, but white is not ok on light videos. So dark is preferred "theme".

I thought of this dark option at the very end while searching for a satisfying light (grey) color for light and dark videos (but there is none which fits both cases if shadow is disabled), so I relied on your reactions. Thanks for that πŸ‘

Regarding themes: I've already planned predefined themes which could be selected/adjusted in preferences. I will implement such »picker« in the next version (due to your feedback I know which combinations would fit and which not).

opusforlife2 commented 4 years ago

Glad to hear that .D

Oh no, you lost an eye. :O

Stypox commented 4 years ago

@vkay94 thank you for your efforts! The latest behaviour with the default settings feels really polished in my opinion. I think this is ready for review, so feel free to open a PR whenever you want ;-) (The settings menu is not going to be there in the actual release, is it? Did I understand correctly?)

vkay94 commented 4 years ago

@Stypox Thanks!

I think this is ready for review, so feel free to open a PR whenever you want ;-)

Nice to hear that :). I decided to wait until #3178 and #4288 are merged into dev, so I could put it on top of the changes because this feature request depends on them (I disabled the notification to make it even usable).

(The settings menu is not going to be there in the actual release, is it? Did I understand correctly?)

It isn't but I'm working on it this weekend. Here is the current status: https://streamable.com/e/owvdnr I think this way it isn't too intrusive, is it?

Stypox commented 4 years ago

@vkay94

I decided to wait until #3178 and #4288 are merged into dev, so I could put it on top of the changes because this feature request depends on them

Yeah, good idea

I think this way it isn't too intrusive, is it?

It definitely isn't, though I am concerned about who would benefit from having these so-specific and niche settings. Isn't the default behaviour acceptable for anyone? Adding more settings is always a problem because things need to be tested in more configurations and users could enable them by accident and not find a way to restore them. So in NewPipe we usually add settings only when they do really provide a value, and in my humble opinion UI adjustments for really small things do not fit. @B0pol @opusforlife2 what do you think?

opusforlife2 commented 4 years ago

Right now the Seek settings menu is fluctuating because new things are being tried every build. Once we settle on a set of good defaults, we can see if we need any settings at all, and if so, where to put them.

vkay94 commented 4 years ago

@Stypox When I opened this issue and shared a first video everyone hated the ripple and arc shape - now the dark arc is good to go ^^ I agree with you that a solid default should be enough for NewPipe. We just have to find it (that's why there are so many configurations). For now, I'll stop on the setting.

To clarify: Arc shape is fine in any case?

I'll make three "themes" to choose from and add them to the next build:

What do you think?

vkay94 commented 4 years ago

Update: app-debug-seek_20200929_01.zip

Improved

Changed

In all themes- except No Arc - an arc is visible. In case of Dark theme and paused the additional arc color is more transparent. Background of this addition is thinking of an indicator as "arc shape + text".

Let me know which theme suits you most ;)

opusforlife2 commented 4 years ago

This could be rolled back when the notification and player improvements are merged and the general UI performance is satisfying with the changes.

Did you roll this back? I think it's still there.

Let me know which theme suits you most ;)

DARK THEME FOR LIFE.

Stypox commented 4 years ago

The default theme is the one I prefer, since the arc is dark while playing (thus not disturbing), but is white while paused (otherwise it can barely be seen in combination with the darkened background)

TobiGr commented 4 years ago

I suggest to use the dark background, because otherwise one cannot read the white text on white background.

Btw. I do not suggest to put a theme chooser into the player itself. The player UI is already cluttered, overloaded and missing simplicity.

vkay94 commented 4 years ago

Did you roll this back? I think it's still there.

@opusforlife2 I hadn't changed it, my focus lied on merging and adjusting my changes according to them. Here is a build which has a checkBox to enable seek-callback on each tap: app-seek_20200929_02.zip

I've also included a signed/optimized version which runs perfectly smooth I guess (well, the debug does too), thanks to @avently and @Stypox ;)

DARK THEME FOR LIFE.

The default theme is the one I prefer

I suggest to use the dark background

I saw it coming xD I prefer the dark background too (=> Light or probably Shadow + dark transparent during playback :')),

Btw. I do not suggest to put a theme chooser into the player itself.

@TobiGr This dialog is just for debugging and giving the opportunity to test different combinations. In case of theming is wanted sometime I though about putting it into the settings (Video & Audio -> Skip duration) - the modified ListPreference should be still in there.

opusforlife2 commented 4 years ago

Wait, what? The function you were talking about earlier wasn't seek-callback.

Call seekBy-method only during fade-out animation instead of each tap (UI becomes less jerky). During the animation the playback stops - similar to dragging the seekBar. This could be rolled back when the notification and player improvements are merged and the general UI performance is satisfying with the changes.

vkay94 commented 4 years ago

@opusforlife2 Whoops, my bad - whenever I read performance I think about seek-callbacks xD

Here is a version where you can toggle whether the playback should stop or not: app-release-signed-seek_20200930_01.zip

The performance on debug-level is not so good and there may occur the problem that the video already plays while the animation is still fully visible. avently has criticized it back then which is indeed not good. One solution would be to reduce the durations. Check it for yourself.

Sorry again for the misunderstanding.

opusforlife2 commented 4 years ago

Looks perfect now! The stutter in the animation is gone, from my testing, so if others agree, you could go ahead and make it the default again. The pause in playback takes away from the buttery smoothness of the whole thing.

vkay94 commented 4 years ago

@opusforlife2 Okay, I'll set it to the default (well, you and Stypox are the only ones who reacted :')). Out of curiosity I've checked YouTube and they keep playing, too. And it has its benefits: the video buffers instantly.

vkay94 commented 4 years ago

Small update: app-seek_20201006_01.zip

Changed

  • Added new predefined theme: Dark2. Basically it's the same as Light but with the dark transparent color for the arc shape (it's my favorite alongside the Light, give it a try ;)).
  • Reduced the delay for the first seekBy-Call. **

** In other words: Instead of waiting for the complete animation (fade in to start of fade out) while stopping the playback, now the first seek-Call occurs after the fade-in.

The background of this is to ensure fluidity of the UI. Multiple seek-Calls have an impact which excels in a jerky fade-in animation - most noticeable when tapping on a white background video. To counteract this I've included a Runnable which delays until the fade-in animation is finished (400ms to be exact), then ongoing it checks periodically every 200ms if it should call the seek-Call or not until the animation is completed.

On high-end devices this wouldn't do any change, but on less performant devices it is better. But you won't notice this delay while seeking, it should be as fast as without a delay (but the small exceptions do the most work :))

Starting from here, despite of the choice of a default configuration, I start on inserting it into NewPipe's coding styles/code => enabling checkStyles - hooray, thousands of "Use a final for xyz".errors, luckily in Kotlin there is no such style check :D

opusforlife2 commented 4 years ago

The delay after double tapping is back again. It was absent in the previous build, which was perfect. :/

vkay94 commented 4 years ago

@opusforlife2 Is it so noticeable? (There might be also a delay because of the buffering, but it should be definitively faster than the first approach (waiting for the animation to fade out)). Compared to the current official way it is, I admire that, but the scaling animation of the circle is an other one (not AnimationUtils#animateView which I have to use (I guess) since it's used for other views like the controls, too).

The reason has to lie in the combination of "animateView + seekBy" because a) while debugging I disabled every UI update calls (Notifications, onProgressUpdate, ...) except them and b) I don't encounter this UI jerky stuff on my other projects when using LayoutTransition/animateLayoutChanges

I might try to write a "special animation" for this, but don't expect any result in the next time. This requires more debugging and currently I need a break from this fine-tuning - that's why I tried myself on an other issue ;)

I'm sorry that it doesn't satisfy your expections :(

opusforlife2 commented 4 years ago

Wait wait wait wait wait. To be clear, I'm referring to this,

Here is a version where you can toggle whether the playback should stop or not:

and not to any lag or jitter. There is none. I just miss the zero delay between the double tap and the video playing.

vkay94 commented 4 years ago

Okay, that's totally a pain in the a**. It looks like I've messed something up somewhere after merging the performance changes from the upstream back then ... So just ignore the previous comments. When it's fixed with the upstream I start to create the Pull Request.

opusforlife2 commented 4 years ago

It's good I took a backup of the perfect build, then. :P

vkay94 commented 4 years ago

Thanks for calling it a perfect build :D

Aside from the Video-PlayerType there is the Popup which should be handled, too, so some stuff needs to be adjusted. For example scaling text/icon sizes - currently it's always the same which results in linebreaks for small windows. Or falling back to the current official circle scaling animation (visual) when the popup is very small (should be done easily and doesn't sound absurd, right?).

But firstly I want to make a solid version (PR draft) which I can easily revert to (= no huge upstream merges in between).

avently commented 4 years ago

XML changes were merged. Happy merging :)