FossifyOrg / Gallery

Browse your memories without any interruptions with this photo and video gallery
https://www.fossify.org
GNU General Public License v3.0
1.59k stars 52 forks source link

Video playback speed control #222

Closed Natolu24 closed 3 weeks ago

Natolu24 commented 4 months ago

What is it?

Description of the changes in your PR

Before/After Screenshots/Screen Record

Fixes the following issue(s)

Acknowledgement

Aga-C commented 4 months ago

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.

Natolu24 commented 4 months ago

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

Natolu24 commented 4 months ago

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 commented 4 months ago
Natolu24 commented 4 months ago

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

Aga-C commented 4 months ago

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

naveensingh commented 3 weeks ago

Just made some XML layout changes. Not too happy with the playback speed button placement but I don't have any ideas at the moment either so I guess I'll merge this now and improve it later. Thanks!