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

Video playback speed control #222

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Natolu24
Copy link

@Natolu24 Natolu24 commented May 9, 2024

What is it?

  • Bugfix
  • Feature
  • Codebase improvement

Description of the changes in your PR

  • Add control to the video playback speed (longpress, then swip up/down to control)
    • It increments by 0.25x, with minimum speed of 0.25x and maximum of 5.0x
  • Add setting to define the default playback speed
    • Possible values : x0.25 / x0.5 / x0.75 / x1.0 / x1.25 / x1.5 / x1.75 / x2.0 / x3.0 / x4.0 / x5.0
  • Add visual indicator to the current playback speed, which appears when the playback speed is changed from the default value
    • And can be clicked to reset to the default speed value

Before/After Screenshots/Screen Record

  • Before:
    Screenshot_Gallery_debug_Before

  • After:
    Screenshot_Gallery_debug_After

Fixes the following issue(s)

Acknowledgement

@Aga-C
Copy link
Member

Aga-C commented May 9, 2024

Add control to the video playback speed (longpress, then swip up/down to control)

It's counterintuitive. How somebody will know that it's done this way? Also, there's no visual guidance - I don't know when I can start moving my finger to change the speed. I think it should rather be done similarly as it's in Fossify Music Player (and similar to numerous other players like NewPipe or VLC), where we have an icon/button to change the playback speed, and it shows a normal slider.

It increments by 0.25x, with minimum speed of 0.25x and maximum of 5.0x

Why this range? I doubt anybody will need speed higher than 3x. In Fossify Music Player we have range between 0.25x to 3x without limiting to multiplies of 0.25. Same is in NewPipe.

Add visual indicator to the current playback speed, which appears when the playback speed is changed from the default value

Why it displays on left if it's slower and on right if it's faster? It should stay in one place, moving elements are annoying for users.

Add setting to define the default playback speed

I think more intuitive would be to remember last playback speed instead of a separate option in Settings. I'd assume that if I chose 2x in one video, it would be kept until I change it. That's how YouTube works, so that's what people are used to.

Comment on lines 185 to 195
<string name="default_playback_speed_1">x0.25</string>
<string name="default_playback_speed_2">x0.5</string>
<string name="default_playback_speed_3">x0.75</string>
<string name="default_playback_speed_4">x1.0</string>
<string name="default_playback_speed_5">x1.25</string>
<string name="default_playback_speed_6">x1.5</string>
<string name="default_playback_speed_7">x1.75</string>
<string name="default_playback_speed_8">x2.0</string>
<string name="default_playback_speed_9">x3.0</string>
<string name="default_playback_speed_10">x4.0</string>
<string name="default_playback_speed_11">x5.0</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It shouldn't be in strings. Numeric value should be provided and formatted by the code according to current user region, not hard-coded. Check how it's done in Fossify Music Player.
  2. It should be 0.25x, 0.5x etc.

@Natolu24
Copy link
Author

Natolu24 commented May 9, 2024

Thanks for the review, it all does indeed makes sense, we'll fix all that !

@Natolu24
Copy link
Author

Natolu24 commented May 15, 2024

Ok so we somewhat copy pasted the implementation from the Fossify Music Player to here :

Screenshot_20240515_201011_Gallery_debug(1)

Does the location look fine, or the UI itself need some adjustment ? (I guess adding a background to the icon could be nice)

Also, there's still a problem that we have issue finding a solution to :
When changing the playback speed, and then changing to a video that has already been started/opened, the playback speed won't be taken into account in the video nor the UI text.
I'm thinking that changing the playback speed should then affect all the videos currently started, but I do not really know how exactly it works, and how to implement it right, would someone have an idea / hint where to change things ?

@Aga-C
Copy link
Member

Aga-C commented May 16, 2024

  • Background should be added, so it would look similarly to play button. Now the button is invisible on the light theme with horizontal videos.
    image

  • After checking in settings Always open videos on a separate screen with new horizontal gestures, there's no button for changing speed. Also, it always plays videos with 1x speed in this mode.

  • When you change speed to lower than 1x, the icon changes, what's the same as in Music Player. But when I open a video, there's always an icon with a running man, no matter the speed. Icon should always be adjusted to the speed.

@Natolu24
Copy link
Author

Background added, and fixed the rest !
Screenshot_20240516_161948_Gallery(1)

@Aga-C
Copy link
Member

Aga-C commented May 16, 2024

Seems to work fine, thanks! Now please wait for the final review by Naveen 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Video Playback Speed Control
2 participants