-
-
Notifications
You must be signed in to change notification settings - Fork 514
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
Add Quality Selection to Playback Overlay #1924
Add Quality Selection to Playback Overlay #1924
Conversation
@nielsvanvelzen @mueslimak3r I removed the transcoding settings from #1406 and built this off of the new v0.14.0 release for Jellyfin 10.8.1. Everything appears to be working as expected, but the audio transcoding options still fail, so they will be added to a separate branch / PR in the future. |
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoQualityController.kt
Fixed
Show fixed
Hide fixed
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoQualityController.kt
Fixed
Show fixed
Hide fixed
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/SelectQualityAction.kt
Fixed
Show fixed
Hide fixed
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/SelectQualityAction.kt
Fixed
Show fixed
Hide fixed
I'll test and review tonight or tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
The UI button looks nice.
Quality switching during playback works well.
The addition of refreshStream()
and its use is good, and it mirrors how a stream is reloaded in a couple other places.
I'm not really qualified to review the kotlin code.
Other than some minor changes it looks good. 🙂
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/SelectQualityAction.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoQualityController.kt
Outdated
Show resolved
Hide resolved
…ction/SelectQualityAction.kt Co-authored-by: Cameron <[email protected]>
…Moved Auto quality to top of list.
Suggestions implemented and tested. Clearly I don't understand GitHub PR diff behavior because my rebase has integrated changes into the diff which are already merged with master. It seems like it's using the common ancestor in the diff instead of resetting to the tip of the target branch rebased with. If this is an issue, let me know how you're handling this. |
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoQualityController.kt
Fixed
Show fixed
Hide fixed
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/SelectQualityAction.kt
Fixed
Show fixed
Hide fixed
It's a little hard to tell exactly how it happened but it looks like you merged two copies of your branch together but they have different bases. Assuming the most recent merge commit is responsible, and that the branches are the same (so discarding the "merged-from" one won't lose any code, you could try checking out the commit prior to that and force-pushing. Definitely back up first though since that's just a guess. If I were to guess what happened, it may be due to not syncing your remote and local branches at some point and then merging them. 🤷 I've attached screenshots from gitkraken that made it a little easier to visualize what's going on. |
58821f7
to
5bbd58b
Compare
Yes, you're correct. I fixed this issue and implemented some fixes suggested by the code scanner. Re-tested after #1943 was merged and everything is working as expected. This should be good to go, pending final approval. |
app/src/main/java/org/jellyfin/androidtv/constant/QualityProfiles.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoQualityController.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoQualityController.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoQualityController.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Niels van Velzen <[email protected]>
Removed unused members. Updated format of vector file.
app/src/main/java/org/jellyfin/androidtv/ui/playback/VideoQualityController.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/androidtv/ui/playback/overlay/action/SelectQualityAction.kt
Outdated
Show resolved
Hide resolved
@nielsvanvelzen I can also consolidate the mapping transformation into the following class, but I would need some guidance on where to store it since it would no longer be a constant: Let me know if you think this is a better solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found 18 potential problems in the proposed changes. Check the Files changed tab for more details.
Right, if the same mapping is needed in two places it makes sense to keep it in a class. Another option would be to make it a function that receives the context and returns the mapping, that might make more sense actually. |
Okay, I updated it to be a top-level function (Kotlin). I still don't really think that storing it in org.jellyfin.androidtv.constant is necessarily appropriate, but without converting back to my original model of using an enum class and then moving the mapping function to a companion object, I'm not really sure where to put it. Let me know if you want any further changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've formatted the code to be more in line with our other Kotlin code and pushed that in the last commit. Everything looks fine now, thanks for the contribution!
@nielsvanvelzen for bitrates < 5mbit i always get a player error. |
Please open an issue so we can track it. |
@nielsvanvelzen but the logs do not trow any errors :( |
Changes
This PR builds off of #1302 using my very limited understanding of Kotlin, implementing
VideoQualityController
/SelectQualityAction
.As a result of this change, it was necessary to implement the function
PlaybackController.refreshSteam()
in order to restart the stream when the setting is changed, applying the new setting.During this change, it was also noted that the Mbit/s to Kbit/s conversion factor was incorrect (x100), and an unusual quality increment had been added to the settings menu (21 Mbit/s). These have been fixed.
Issues
May satisfy these vague enhancements:
Testing