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

Refactor play logic in PlaybackController #1574

Merged
merged 3 commits into from Apr 5, 2022

Conversation

DavidFair
Copy link
Contributor

@DavidFair DavidFair commented Mar 27, 2022

Changes

  • Refactors the video option building from PlaybackController:play into private methods. This first step was done as-is so no modifications were made to the actual methods
  • Removes the three call sites to Utils.getMaxBitrate to a single location

This is a precursor to #1569 and should allow us to:

  • Make sense of the diff in the next PR
  • Allow us to easily refactor away deprecated VideoOptions when the replacement
  • Makes the code more readable

Testing

  • Try to play a video with libVLC and ExoPlayer

Copy link
Member

@mueslimak3r mueslimak3r left a comment

Choose a reason for hiding this comment

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

looks good. Please rebase/fix the conflicts

Since we will be touching this code in a later commit it makes sense to
extract these into private methods as-is.
This commit only extracts the options and does not perform any
additional refactoring steps
Since we are doing this three times we can simply move it out of these
methods. This makes it easier for us to calculate a dynamic bitrate
later.
This one was noticed during refactoring and it's fairly free. Now we use
a float (rather than a Float object) the nullable annotation is
incorrect.
@jellyfin-bot jellyfin-bot removed the merge conflict Conflicts prevent merging label Apr 4, 2022
@nielsvanvelzen nielsvanvelzen added this to In progress in v0.14.0 via automation Apr 5, 2022
@nielsvanvelzen nielsvanvelzen merged commit 8dec566 into jellyfin:master Apr 5, 2022
v0.14.0 automation moved this from In progress to Done Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v0.14.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants