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.

Sign up for GitHub

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

Jump to bottom

Add Playback speed controls #1302

Merged
merged 14 commits into from
Jan 12, 2022
Merged

Add Playback speed controls #1302

merged 14 commits into from
Jan 12, 2022

Conversation

DavidFair
Copy link
Contributor

@DavidFair DavidFair commented Dec 20, 2021 •
edited
Loading

Changes
Adds Playback speed controls:
Use the same icon / general UX style as Jellyfin-Android's playback button
A playback speed will apply to the current and future videos for the current application launch. I.e. setting speed to 2.00x then playing the next episode will also play at 2.00x

This PR Contains the following:

  • Add some unit tests for functionality I've touched where it's possible to automatically test
  • Add a VideoSpeedController to keep logic / state nice and encapsulated with the associated PlaybackSpeedAction. This should hopefully make it easy to incorporate into The Playback Rewrite #1057

See each commit for more detailed descriptions.

This is my first foray into Kotlin, and I haven't done Android dev in 4-5 years. So I'm happy to take any/all criticisms, feedback and change ideas.

Issues
Fixes: #1189

Testing

  • Try playing a video with speed set to various levels
  • Try seeking around in the video, the speed should be preserved
  • Play a different video. It should continue at the same speed as the last video
  • Ensure unit tests run / pass

@DavidFair DavidFair mentioned this pull request Dec 22, 2021
Closed
@nielsvanvelzen
Copy link
Member

Thanks for your pull request(s). Before I review it I'd like to ask you to split all the unit testing changes to a separate PR as it's not simply adding a few tests but adding dependencies etc. with it. We try to focus pull requests to one particular change to make it easier to review, and with this PR I feel like it might take some time to get merged purely because of a secondary change (testing changes).

@DavidFair DavidFair mentioned this pull request Dec 23, 2021
Merged
@DavidFair

This comment has been minimized.

Sign in to view
@DavidFair DavidFair force-pushed the Playback_speed_controls branch from 584e925 to b3b0a39 Compare December 23, 2021 13:57
@DavidFair DavidFair marked this pull request as draft December 23, 2021 13:57
@DavidFair
Copy link
Contributor Author

DavidFair commented Dec 23, 2021 •
edited
Loading

Thanks for your pull request(s). Before I review it I'd like to ask you to split all the unit testing changes to a separate PR as it's not simply adding a few tests but adding dependencies etc. with it.

Done, I've rejigged the history of both branches to drop any non-essential commits into their own PRs.

with this PR I feel like it might take some time to get merged purely because of a secondary change

No worries, I'm also aware of the amount of maintainers time required especially with the time commitments from the festive time of the year coming up too. In this case I just got on a bit of a roll and ended up with a much larger PR than originally envisioned....

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Dec 25, 2021
@DavidFair DavidFair force-pushed the Playback_speed_controls branch from b3b0a39 to f1e72e0 Compare December 26, 2021 16:13
@jellyfin-bot jellyfin-bot added merge conflict Conflicts prevent merging and removed merge conflict Conflicts prevent merging labels Dec 26, 2021
@DavidFair DavidFair force-pushed the Playback_speed_controls branch from f1e72e0 to a8ad0c9 Compare December 31, 2021 21:41
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Dec 31, 2021
@DavidFair DavidFair marked this pull request as ready for review December 31, 2021 21:42
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jan 1, 2022
@DavidFair DavidFair force-pushed the Playback_speed_controls branch from a8ad0c9 to dce56ce Compare January 2, 2022 03:21
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jan 2, 2022
@DavidFair DavidFair force-pushed the Playback_speed_controls branch from dce56ce to f7e9999 Compare January 2, 2022 03:25
@DavidFair DavidFair mentioned this pull request Jan 2, 2022
Merged
@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jan 2, 2022
@DavidFair
Add basic mocks to unit test PlaybackController
4ebb25e 
Adds the various basic mocks required to unit test Playback Controller.

At a high level the steps we perform are:
- Prep mocks for deps injected by constructor
- Prep mocks for Koin, then pass these as singletons
- Patch up various calls out to Android SharedPrefs ...etc.

This allows us to construct a PlaybackController in a JVM context now.
These mechanisms will be useful for later in the patch series too....
@DavidFair
Add setPlayback impl and tests to controller/view
456fd50 
Adds setPlaybackSpeed to the view and controller.
The controller has associated tests which built on the earlier ones.

Unfortunately the view also has logic in it and I've tried to (ab)use
mocks to factor out the ExoPlayer factory. However because it's a static
final the mock will always defer to the real impl.

I could introduce some sort of interface and abstract out the calls to
the underlying players using an adapter pattern so we can test this. But
alas, the player rewrite will obselete this and I'm 8+ hours into
getting tests to work...
@DavidFair
Add VideoSpeedController and tests
0423b8b 
Adds a new controller for VideoSpeed, to help encapsulate items
following SRP.
This also adds a mechanism to track the users most recent speed
selection, so switching from videos / to other media and back will
restore the user's selection on the next video.
@DavidFair
Switch to using Enums to representing speed
5788167 
This makes it much easier to compare for equality and has built in
indexing making the menu much easier to work with.

In comparison using doubles directly would have required us to add a
comparison lambda in the view to correctly detect what was selected /
determine what is selected.
@DavidFair DavidFair force-pushed the Playback_speed_controls branch from f7e9999 to a25c7e2 Compare January 2, 2022 15:15
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Jan 2, 2022
nielsvanvelzen
nielsvanvelzen requested changes Jan 2, 2022
View reviewed changes
app/src/main/res/values-en-rGB/strings.xml Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/preference/UserPreferences.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/PlaybackController.java Show resolved Hide resolved
app/src/main/res/values/strings.xml Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/PlaybackSpeedAction.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/PlaybackSpeedAction.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/PlaybackSpeedAction.kt Outdated Show resolved Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/PlaybackSpeedAction.kt Outdated Show resolved Hide resolved
@DavidFair
Wire up view for Playback speed controls
9806b05 
Wires up the view component for playback speed controls, based on the
pattern used in various other action items.
@DavidFair DavidFair force-pushed the Playback_speed_controls branch from a25c7e2 to f35ba7d Compare January 2, 2022 18:48
@DavidFair
Add check that setPlaybackSpeed is sane
13a289d 
This will ignore any values that are clearly incorrect i.e. <0.25.

This means the playback controller can still have a sentinel value of
-1.0 to ensure the model controlling speed is in-place correctly. Whilst
not breaking user playback by default if something goes wrong.

As this file directly interfaces with the video manager (which holds
surfaces) we can't easily make a test for it, so leave this block
untested
@DavidFair DavidFair force-pushed the Playback_speed_controls branch from d3d0459 to 71047c4 Compare January 6, 2022 14:02
@DavidFair
DI interface instead of concrete type
20415d2 
Inject the interface for shared preferences rather than the concrete
type.
This avoids Mockk trying to grab the abstract class underneath, which
would run the real impl instead. It also brings our DI in this file more
in-line with the commonly used pattern
@DavidFair DavidFair force-pushed the Playback_speed_controls branch from 71047c4 to 20415d2 Compare January 8, 2022 01:07
@DavidFair
Address PR comments
bd68c12 
- Revert some noisy automatic formatting
- Move speed steps out of companion object
- Drop supuerfluous interface
@DavidFair
Tidy up VideoSpeedController from PR review
1630a53 
- Use Kotlin a kotlin setter directly
- Make the variable public
- (Small amount of rewiring to make everything work)
@DavidFair
Drop PlaybackController tests and revert DI
0b95cf7 
Drop PlaybackController test. We can't simply inject by interface as
this isn't type safe with how it's currently implemented.

Since it's likely PlaybackController is going to undergo a major rework
with pending future work, let's simply drop the tests and revert the
associated changes.
@nielsvanvelzen nielsvanvelzen dismissed mueslimak3r’s stale review January 12, 2022 07:49

discussions already resolved

@nielsvanvelzen nielsvanvelzen merged commit 17ba482 into jellyfin:master Jan 12, 2022
@nielsvanvelzen nielsvanvelzen added the release-highlight Pull request might be suitable for mentioning in the release blog post label Jan 12, 2022
@mueslimak3r
Copy link
Member

mueslimak3r commented Jan 17, 2022 •
edited
Loading

The speed controls are available for live tv playback. Setting the speed to > 1 breaks playback by constantly running out of buffer.

Can you patch this?
Maybe just have a flag to constrain the speed adjustment to values <= 1, and that would be used for live tv. Also, the speed would also need to be reset at playback start for live tv since otherwise it persists across sessions.

I'm not too familiar with live-tv but it seems likely that even for values < 1 the stream could become unavailable if the client falls too far behind the live edge. If that's likely then it may be better to just disable playback speed adjustment for live tv

@Dnkhatri
Copy link

@DavidFair When playing faster speed pass through audio should be disabled as otherwise we get the high pitch audio,

@DavidFair
Copy link
Contributor Author

The speed controls are available for live tv playback. Setting the speed to > 1 breaks playback by constantly running out of buffer.
Maybe just have a flag to constrain the speed adjustment to values <= 1, and that would be used for live tv. Also, the speed would also need to be reset at playback start for live tv since otherwise it persists across sessions.

I'll put a patch in tonight to check if we're playing back live tv and simply reset the speed from the speed controller.
I'm sure there is a way to have faster playback up until we hit the end of the buffer, then reset back to 1. But I can't really test it here so I'm not in a position to have a crack at it.

When playing faster speed pass through audio should be disabled as otherwise we get the high pitch audio
I'm not sure disabling pass through audio will fix the pitch, as we need some sort of audio scale filter. If it does I'm happy to apply that as a short term fix.

If it doesn't we'll have to leave it as a limitation, where users can choose to play faster with the current caveat that the pitch is wrong. I'll have a look what's available too, but AFAIK Chromium, Firefox and MPV all get around this by implementing a scale filter. So it depends on if ExoPlayer and / or libVLC have the appropriate audio filters available

@DavidFair DavidFair deleted the Playback_speed_controls branch January 18, 2022 18:15
@DavidFair DavidFair mentioned this pull request Jan 18, 2022
Merged
@DavidFair
Copy link
Contributor Author

@Dnkhatri I can't reproduce this with Exoplayer locally, shifting 1x to 2x maintains the same sang note on my content
Could you check you're using Exoplayer locally? If you're still seeing this please feel free to open an issue and I'll be happy to look at this.

@mueslimak3r
Copy link
Member

@Dnkhatri It should be there. The only place it is disabled is with live tv

@zkhcohen zkhcohen mentioned this pull request Feb 1, 2022
Closed
@mueslimak3r mueslimak3r mentioned this pull request Mar 18, 2022
Closed
@zkhcohen zkhcohen mentioned this pull request Aug 2, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsvanvelzen nielsvanvelzen nielsvanvelzen approved these changes

@mueslimak3r mueslimak3r Awaiting requested review from mueslimak3r

Assignees
No one assigned
Labels
release-highlight Pull request might be suitable for mentioning in the release blog post
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DavidFair @nielsvanvelzen @mueslimak3r @Dnkhatri @jellyfin-bot