Add Quality Selection to Playback Overlay

[zkhcohen]

Changes

1. Added quality selection (max bitrate) menu to playback overlay.
2. Added PlaybackController.refreshSteam() function to restart a stream when transcoding options are changed.
3. Fixed Mbit/s to Kbit/s conversion factor (100 -> 1000).
4. Fixed unusual Mbit/s increment (21Mbit/s -> 20Mbit/s).

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:

• Inapp Quality control  #1388
• Jellyfin on Fire TV has no quality selection #455
• Quality selector - transcode option #43

Testing

• Try changing quality settings through the playback menu. You should be able to verify that the media is being transcoded through the logs / dashboard.
• Try playing a different video - the settings should persist.
• Open the settings -> playback menu through the main menu - the settings configured through the overlay should persist there.
• Change the quality in the settings -> playback menu through the main menu, then open both menus in the playback overlay - the settings should persist and you should be able to verify that the media is being transcoded through the logs / dashboard.

[zkhcohen]

@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.

[mueslimak3r]

I'll test and review tonight or tomorrow

[mueslimak3r]

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. 🙂

[zkhcohen]

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.

[mueslimak3r]

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.

[zkhcohen]

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. 🤷

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.

[zkhcohen]

@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.

[github-advanced-security]

Found 18 potential problems in the proposed changes. Check the Files changed tab for more details.

[nielsvanvelzen]

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.

[zkhcohen]

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.

[nielsvanvelzen]

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!

[Y0ngg4n]

@nielsvanvelzen for bitrates < 5mbit i always get a player error.

[nielsvanvelzen]

@nielsvanvelzen for bitrates < 5mbit i always get a player error.

Please open an issue so we can track it.

[Y0ngg4n]

@nielsvanvelzen but the logs do not trow any errors :(