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

YouTube's Fast Forward/Rewind behavior #4833

Merged
merged 21 commits into from
Jan 30, 2022

Conversation

vkay94
Copy link
Contributor

@vkay94 vkay94 commented Nov 7, 2020


Video Preview

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

This PR adds multi-double-tap seeking with YouTube's progress indicator style and a small change to alpha animation handling (dynamic duration depending on the view's alpha value).

I've decided to open this PR to discuss it here even if it isn't merge-ready instead of in the issue since the code base is basically done when #4587 was merged and I've reduced the added code to the minimum.

Features

  • more polished design with fluid transitions
  • indicator which shows the current progress
  • multi double-tap-to-seek design that allows seeking faster in one flow

To-do

  • Apply specific handling for popup-videos due to its variable sizes (triangle icon size and text size)

Fixes the following issue(s)

Relies on the following changes

In the moment there's a design "bug" which results in heavy UI lagging during fast double tapping in some fragments. The reason is that with each seeking the history and playlist fragment is reloaded. Since this feature increases the frequency such high reloading is noticeable.

It could be fixed if the lists fragments apply changes to single items instead of reloading the whole list. But that should be done in another PR, probably by changing to the Groupie library.

APK testing (2020-12-01)

When you try it, please make sure you haven't opened the history or playlist detail fragments opened (also in tabs).
Releases page

Here are some visuals for the 2020-12-01-version with its states:


Normal video frame for reference


Playing + dark arc shadow


Paused + controls were visible (shadow + dark transparent arc)

So let's continue discussing it here with the view of the code implementation ;)

Due diligence

@opusforlife2
Copy link
Collaborator

Nice, your PR has a very professional feel to it. :P

@opusforlife2
Copy link
Collaborator

Yup. Everything is smooth and awesome and absolutely amazeballs, as expected. (I tested the release version).

The only thing that could make it better would be removing that background shadow. :*

@TobiGr TobiGr added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background) labels Nov 8, 2020
@vkay94
Copy link
Contributor Author

vkay94 commented Nov 8, 2020

@opusforlife2 Preferably I'd like to add a setting to give a opportunity to adjust it individually - why doesn't Github provide a poll functionality xD
I'll go through the issue and assess again (it's a while ago).

Besides of that I've increased the time for the triangle animation cycle a little bit. What do you think?

@opusforlife2
Copy link
Collaborator

I don't remember how long it took in previous builds. The current one looks fine, though.

why doesn't Github provide a poll functionality xD

You could say:

  • want dark shadow - use hooray emoji on this comment
  • want it gone - use eyes emoji on this comment.

or something like that.

@vkay94
Copy link
Contributor Author

vkay94 commented Nov 11, 2020

@opusforlife2 This poll option is not really "optimal" :) It's a little bit difficult to decide when there are only a few people voting (3 votes aren't very meaningful - just remember the complaint when the unified player was introduced o.0). The shadow definitely ensures readability under all circumstances.

In the issue back then I made a small "config" button but stopped it due to niche. Now I thought I could implement it for testing - why not ^^:

app-release-setting-test_20201111.zip

Note: The setting is not saved (changes won't be applied to the video player itself).

@opusforlife2
Copy link
Collaborator

No shadow still seems better to me. Why doesn't the universe like no shadow? ( ;´༎ຶД༎ຶ`)

@Stypox
Copy link
Member

Stypox commented Nov 20, 2020

Oh, if you are talking the dark shadow covering all of the screen (and not only the arc background), then I agree with you @opusforlife2 that the shadow is unneeded. @vkay94 could you rebase? I will then review ;-)

@vkay94
Copy link
Contributor Author

vkay94 commented Nov 20, 2020

Okay, fine. I'll set it like the following (where shadow is the screen covering shadow, arc the rounded shadow with text and ,transparent arc the one with less transparency shadow):

State before double tapping => what happens visually when beginning: (arc is always animating in)

  • Video is playing + shadow/controls are hidden => ---
  • Video is playing + shadow/controls are visible => animate out shadow/controls
  • Video is paused + shadow/controls are hidden => --- **
  • Video is paused + shadow/controls are visible => animate out controls (+arc is transparent arc here)

State during double tapping => what happens visually when it's finished: (arc is always animating out)

  • Video is playing + arc is visible => ---
  • Video is paused + arc is visible => animate in shadow + animate in controls
  • Video is paused + transparent arc and shadow are visible => animate in controls

**: In the previous versions I animated in the shadow for all cases when the video is paused even if the shadow wasn't visible then. Since the shadow is unwanted that much ( :( ) I'll leave it out in this specific case I guess but I need to check how the animation looks when the arc is animating out while the shadow and controls are animating in simultaneously though).

@Stypox Yeah I'll rebase it and commit the above change. There was no need to do it since the debug app works (and I don't like to force-push when it's unnecessary + the feature isn't merge-ready anyway due to the update problem).

@vkay94
Copy link
Contributor Author

vkay94 commented Nov 20, 2020

@opusforlife2 @Stypox I uploaded a version with the configs above and updated the OP with the testing apk. Let me know whether the dark arc color (second screenshot) is fine (for me it's too light for reading on some frames which aren't "monotonous", but darker make it unsuitable for other videos xD)

@opusforlife2
Copy link
Collaborator

YAY NO SHADOW

Bugs found:

  • When paused, the seekbar doesn't update after double tapping. It only updates upon tapping Play.

  • Pause. Rewind to the beginning. Now double tapping doesn't work at all, either forwards or backwards. It starts working again after you tap Play.

@Stypox
Copy link
Member

Stypox commented Nov 21, 2020

Shapes and colors look good and polished to me now :-D
I found a bug: while playing, tap on the screen to show controls, then double tap on one side of the screen to fast-forward/rewind. The arc appears on the double-tapped side of the screen, as expected, but the X seconds text and the triangles animation appear on the opposite side.

@vkay94
Copy link
Contributor Author

vkay94 commented Nov 21, 2020

When paused, the seekbar doesn't update after double tapping. It only updates upon tapping Play.

I can reproduce it rarely indeed. I may look into it but I can't find a reason why it happens with the changes made (I call the onFastForward/onFastForward the same way as before and it calls triggerProgressUpdate which updates the progress bar and text).
The only circumstances when this call does "nothing" (= is not prepared) are when 1. no media found or unavailable or 2. the video is finished/ended. @opusforlife2 Can you do a favor and check whether you can reproduce it in 0.20.3 too?

Pause. Rewind to the beginning. Now double tapping doesn't work at all, either forwards or backwards. It starts working again after you tap Play.

That was (partially) designed to work like this, so that you can't rewind endlessly. I added a small puffer that you can rewind only when the stream progress is greater or equal of a half second. PArtially because it should fast forward in this situation. I'll add a separate check for the forwarding in this case ;)

I found a bug: while playing, tap on the screen to show controls, then double tap on one side of the screen to fast-forward/rewind. The arc appears on the double-tapped side of the screen, as expected, but the X seconds text and the triangles animation appear on the opposite side.

Unfortunately, I can't reproduce it, works as expected for me. Could you give me some more information please? (E.g. Android Version or whether it happens for all streams or only rarely). Thank you.

@Stypox @opusforlife2 Thanks for your bug reports :)

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Nov 21, 2020

Can you do a favor and check whether you can reproduce it in 0.20.3 too?

Good catch! Yeah, this is a 0.20.3 issue. I'll open a new issue for it.

@Stypox
Copy link
Member

Stypox commented Nov 21, 2020

Here is a video of the bug:
SVID-20201121-142813-1

@vkay94
Copy link
Contributor Author

vkay94 commented Nov 21, 2020

Hmm, I tried to reproduce it under the same conditions as you show in the video (locked orientation, navigation bar enabled and same video). I tried it on two devices with Android 8.0 and Android 10.0, but it doesn't happen.

I've looked into the code and there might be a race condition between changing the position and animating. @Stypox Short question since it's hard to see in the gif: When it appears and you keep seeking, does it change the position or does it keep on the same side for this series of taps?

I did a small change which should fix the race condition (I apply the correct position before animating in). Could you test it please? app-debug-frff-test-change-position.zip

Thanks

@Stypox
Copy link
Member

Stypox commented Nov 22, 2020

@vkay94 if I keep tapping it keeps happening (the side is not correctly set even after skipping multiple tens of seconds), and the apk you provided is no different unfortunately

@vkay94
Copy link
Contributor Author

vkay94 commented Nov 22, 2020

@Stypox That's weird. I've converted your GIF to a video and analyzed the taps. The arc side is always correct as I see and also the direction of the triangles. The only thing which is incorrect is the position of the view itself. I'm changing it via constraints programmatically.

May I ask you which Android version you're running? @opusforlife2 Could you check whether you encounter Stypox issue as well, please?

Edit:
I've did a rebase with the latest changes. I hope the upgrade of ConstraintLayout from 1.1.3 to '2.0.4' might fix it somehow:
app-debug-frff-changes-and-rebase-2020-11-22.zip

@opusforlife2
Copy link
Collaborator

@vkay94 Already tried it yesterday. I couldn't reproduce it. Could be Huawei specific.

@vkay94
Copy link
Contributor Author

vkay94 commented Nov 22, 2020

If it's a manufactor-specific issue then I can't do much because I haven't got such device here. But if the above change doesn't fix it, I have one option at least to try :)

@Stypox
Copy link
Member

Stypox commented Nov 22, 2020

@vkay94 with your last apk I can't reproduce it anymore. It was a bug with constraint layout, it seems like

@vkay94
Copy link
Contributor Author

vkay94 commented Nov 22, 2020

Great to hear that 👍 (that means I've to update my library in the next time too :')).

TobiGr and others added 12 commits January 21, 2022 22:49
* Deduplicated and simplified a lot of code
* Fixed ``invalidSeekConditions`` so that it's possible to seek while the player is loading (like currently the case)
* Removed alphaRelativeDuration as there is no use for it
* When rewinding: Check if <0,5s
* When fast-forwarding: Check if player has completed or the current playback has ended

This allows rewinding on the endscreen
Stypox
Stypox previously approved these changes Jan 24, 2022
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 fullscreen, landscape, vertical videos, specific player states (e.g. just after switching player, or soundcloud music videos, etc.) on API 19 emulated, API 31 emulated and API 29 on-device and couldn't find any issue. @litetex if we want to include this in this release, merge!

@Stypox
Copy link
Member

Stypox commented Jan 25, 2022

Wait, I forgot to test changing the settings... and found an issue. When a video is already playing and the fast-forward duration setting is changed, the player still fast-forwards by the old amount until the app is restarted. This does not happen on current dev.

@opusforlife2
Copy link
Collaborator

and found an issue

@litetex
Copy link
Member

litetex commented Jan 25, 2022

Wait, I forgot to test changing the settings... and found an issue. When a video is already playing and the fast-forward duration setting is changed, the player still fast-forwards by the old amount until the app is restarted.

Before this PR it didn't show any seek amount so I think this works as expected.
Let's simply restart the app when the user changes the duration.
...

Just kidding. I will fix it.

Already found the bad line:
https://github.com/TeamNewPipe/NewPipe/pull/4833/files#diff-68cd2f051cf461f1cee8f97ab4a793ab90276943eed26f8f1baf5105948b2b5eR589

Update: Should be fixed now

@sonarcloud
Copy link

sonarcloud bot commented Jan 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

Works, and the fast-inexact-seek also works ;-)

@avently
Copy link
Contributor

avently commented Jan 29, 2022

Guys, let me know if you need help in clicking on merge button. I have skills

@litetex litetex merged commit 2886bc3 into TeamNewPipe:dev Jan 30, 2022
@avently
Copy link
Contributor

avently commented Jan 30, 2022

Thank you to all contributors!

@opusforlife2
Copy link
Collaborator

Is this real or am I dreaming?

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 GUI Issue is related to the graphical user interface player Issues related to any player (main, popup and background)
Projects
None yet
9 participants