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

Add Quality Selection and Audio Output Controls to Playback Overlay #1406

Closed

Conversation

zkhcohen
Copy link
Contributor

@zkhcohen zkhcohen commented Feb 1, 2022

Changes

  1. Added quality selection (max bitrate) menu to playback overlay.
  2. Added audio output (downmixing/transcoding) selection menu to playback overlay.
  3. Added PlaybackController.refreshSteam() function to restart a stream when transcoding options are changed.
  4. Fixed Mbit/s to Kbit/s conversion factor (100 -> 1000).
  5. 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 and AudioOutputController / SelectAudioOutputAction.

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

  • Try changing quality / audio output settings through the playback menu. You should be able to verify that the media is being transcoded / downmixed 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 / audio output settings 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 / downmixed through the logs / dashboard.

@zkhcohen zkhcohen changed the base branch from release-0.13.z to master February 1, 2022 08:20
@mueslimak3r
Copy link
Member

mueslimak3r commented Feb 2, 2022

please rebase this branch (onto a base up to date with upstream main) so it has the fix for the debug build provider name conflicting with the release version's name.

@zkhcohen zkhcohen force-pushed the playback_options_integration branch from 4073a3c to 31331ac Compare February 2, 2022 23:03
@zkhcohen
Copy link
Contributor Author

zkhcohen commented Feb 2, 2022

please rebase this branch (onto a base up to date with upstream main) so it has the fix for the debug build provider name conflicting with the release version's name.

Done. Let me know if there are any other issues.

@jellyfin-bot jellyfin-bot added merge conflict Conflicts prevent merging and removed merge conflict Conflicts prevent merging labels Feb 3, 2022
@zkhcohen
Copy link
Contributor Author

zkhcohen commented Feb 7, 2022

Having some issues with ExoPlayer. I'll perform further tests with LibVLC.

  1. Changing the output from Direct Play to Downmix to Stereo works, but changing it back just starts a new transcode. Exiting out of the stream and then restarting it resolves the issue.
  2. Exiting out of the stream after setting the output to Downmix to Stereo, then RESUMING playback causes Jellyfin to create a swarm of new transcodes, which all fail. Starting the stream over from the beginning seems to avoid the issue, but seeking causes it to fail again. This issue may be related to my setup, as all of the failed transcodes contain set_mempolicy: Operation not permitted spam.
  3. When switching audio outputs I receive the following (benign) error in the debug logs which I haven't been able to pinpoint the cause of (besides being related to the playback UI):

E/PlaybackController: Unable to get internal stream info E/PlaybackController: java.lang.IndexOutOfBoundsException: Index: 0, Size: 0 E/PlaybackController: at java.util.ArrayList.get(ArrayList.java:437) E/PlaybackController: at androidx.leanback.widget.ArrayObjectAdapter.get(ArrayObjectAdapter.java:72) E/PlaybackController: at androidx.leanback.widget.ControlBarPresenter$ViewHolder$1.onChildFocusedListener(ControlBarPresenter.java:97) E/PlaybackController: at androidx.leanback.widget.ControlBar.requestChildFocus(ControlBar.java:90)

@mueslimak3r
Copy link
Member

Having some issues with ExoPlayer. I'll perform further tests with LibVLC.

  1. Changing the output from Direct Play to Downmix to Stereo works, but changing it back just starts a new transcode. Exiting out of the stream and then restarting it resolves the issue.
  2. Exiting out of the stream after setting the output to Downmix to Stereo, then RESUMING playback causes Jellyfin to create a swarm of new transcodes, which all fail. Starting the stream over from the beginning seems to avoid the issue, but seeking causes it to fail again. This issue may be related to my setup, as all of the failed transcodes contain set_mempolicy: Operation not permitted spam.
  3. When switching audio outputs I receive the following (benign) error in the debug logs which I haven't been able to pinpoint the cause of (besides being related to the playback UI):

E/PlaybackController: Unable to get internal stream info E/PlaybackController: java.lang.IndexOutOfBoundsException: Index: 0, Size: 0 E/PlaybackController: at java.util.ArrayList.get(ArrayList.java:437) E/PlaybackController: at androidx.leanback.widget.ArrayObjectAdapter.get(ArrayObjectAdapter.java:72) E/PlaybackController: at androidx.leanback.widget.ControlBarPresenter$ViewHolder$1.onChildFocusedListener(ControlBarPresenter.java:97) E/PlaybackController: at androidx.leanback.widget.ControlBar.requestChildFocus(ControlBar.java:90)

I think what's going on here is related to calling play() instead of playInternal(). When I was working on the audio track switching, I tried using play() and it resulted in some really bizarre behavior. I think this is due to play() reinitializing the player surfaces and even re-instancing the libVLC player.

You could try mimicking the behavior of switchAudioStream:

playInternal(getCurrentlyPlayingItem(), mCurrentPosition, mCurrentOptions, mCurrentOptions);

@mueslimak3r
Copy link
Member

mueslimak3r commented Feb 7, 2022

If the above approach doesn't work you could also try using changeVideoStream similar to how seek() uses it. Perhaps conditionally if currently transcoding. This sends an apiClient.StopTranscodingProcesses() for that sessionId and then provides a new getVideoStreamInfo response via StopTranscodingResponse.

tbh there's a lot of weirdness around restarting or switching transcoded streams, and I don't fully understand it.

playbackManager.getValue().changeVideoStream(mCurrentStreamInfo, api.getValue().getDeviceInfo(), mCurrentOptions, pos * 10000, apiClient.getValue(), new Response<StreamInfo>() {

@zkhcohen
Copy link
Contributor Author

zkhcohen commented Feb 9, 2022

If the above approach doesn't work you could also try using changeVideoStream similar to how seek() uses it. Perhaps conditionally if currently transcoding. This sends an apiClient.StopTranscodingProcesses() for that sessionId and then provides a new getVideoStreamInfo response via StopTranscodingResponse.

tbh there's a lot of weirdness around restarting or switching transcoded streams, and I don't fully understand it.

playbackManager.getValue().changeVideoStream(mCurrentStreamInfo, api.getValue().getDeviceInfo(), mCurrentOptions, pos * 10000, apiClient.getValue(), new Response<StreamInfo>() {

Before I attempt to resolve the issues you pointed out in your (very helpful and detailed) comments, do you think it would be wise to wait for #1432 to be merged? I ran a diff and didn't see any conflicts, but I'm curious if some of the track selection functionality you're developing might assist in the resolution of the issues in my PR, as you did with #1430

@mueslimak3r
Copy link
Member

Before I attempt to resolve the issues you pointed out in your (very helpful and detailed) comments, do you think it would be wise to wait for #1432 to be merged? I ran a diff and didn't see any conflicts, but I'm curious if some of the track selection functionality you're developing might assist in the resolution of the issues in my PR, as you did with #1430

You could try checking out #1432 and locally merge your branch (this PR) into it, and see if anything improves or breaks more.
Exoplayer infinitely restarting does sound like something related to track selection, and is something I've had happen a lot while testing changes related to that.

It's hard to say if it will though from my end since for some reason I didn't see the issues you're having with this PR. But only did some basic testing.

@nielsvanvelzen
Copy link
Member

We're getting quite a lot of options in the toolbar. I think it would make sense to add a "more options" menu and move the less commonly used actions to it.

This is how it looks now, with this PR:

image
(with libvlc you also have an action to adjust audio delay)

@zkhcohen
Copy link
Contributor Author

We're getting quite a lot of options in the toolbar. I think it would make sense to add a "more options" menu and move the less commonly used actions to it.

I fully agree with this, but with my work schedule at the moment I'm not sure when I'll have the time to figure out the GUI framework well enough to implement the menu.

I'm going to try to focus on fixing the remaining issues with the functionality in this PR over the weekend, then I can try to work on nesting them under a menu.

@zkhcohen zkhcohen force-pushed the playback_options_integration branch 2 times, most recently from 33099ab to 5bfcd0b Compare February 12, 2022 19:24
@zkhcohen zkhcohen force-pushed the playback_options_integration branch from 5bfcd0b to 83e7239 Compare February 12, 2022 19:26
@zkhcohen
Copy link
Contributor Author

I rebased the current changes on the latest commit in master, then performed further tests. I also resolved all of the server-specific errors I was having, so I would expect other people to be able to replicate most if not all of this behavior. I plan on re-testing just using the latest master commit, and not with my changes.

When I have some time this weekend, I'll try to implement some of the suggestions in this thread.


ExoPlayer:

  • All video transcoding options work as expected.
  • Switching back and forth between Direct Play and Downmix to Stereo work as expected, but only if the video hasn't been seeked / is at the beginning. Enabling Downmix to Stereo (successfully) and then seeking the video, breaks the stream.
  • Resuming a seeked video with Downmix to Stereo enabled fails, while if Direct Stream was enabled, it would work.
  • If you enable video transcoding and then the audio output selection, then back out and restart, you'll get errors until you perform the reverse order of the action - change audio output back, then disable video transcoding. I need to test more, but it appears that the order is essential with this bug. This issue applies even when there are no transcodes in the folder on your server...

LibVLC:

  • All video transcoding options work as expected.
  • Switching to Downmix to Stereo works. Seeking works with this option selected.
  • Switching to Downmix to Stereo then back to Direct Play only works if you either disable video transcoding afterwards, or if you restart the stream by backing out and clicking play again.
  • If you start the stream while Downmix to Stereo is selected, it resumes playback as expected, but switching back to Direct Play doesn't work. This means that the issue isn't simply that the first audio-switch action you perform is the only one that applies, but that the issue is that transcoding isn't stopped appropriately.
  • Unlike with ExoPlayer, downmixing to stereo doesn't cause transcoding.

Other issues:

  • In the logs, the profile alternates on every transcode playback:
    [INF] [16] Jellyfin.Api.Helpers.MediaInfoHelper: Profile: "AndroidTV-ExoPlayer"
    then....
    [INF] [10] Jellyfin.Api.Helpers.MediaInfoHelper: Profile: "AndroidTV-libVLC"
    ...but the player is specifically set to either LibVLC or ExoPlayer.

@mueslimak3r
Copy link
Member

What device are you testing this on?
For some reason, I'm seeing none of these issues on my end on my CCwGTV. #itworksonmymachine

For downmix to stereo, my sound system is stereo so I can't be sure about whether surround sound is on or not. But I don't notice any issues or anything weird in the log.

@zkhcohen
Copy link
Contributor Author

What device are you testing this on? For some reason, I'm seeing none of these issues on my end on my CCwGTV. #itworksonmymachine

For downmix to stereo, my sound system is stereo so I can't be sure about whether surround sound is on or not. But I don't notice any issues or anything weird in the log.

Hmmm.. CCwGTV as well. I'm going to stand up a different Jellyfin server and test again. I'm passing the audio through an AVR so I've been able to validate when it is and isn't 5.1.

@mueslimak3r
Copy link
Member

What device are you testing this on? For some reason, I'm seeing none of these issues on my end on my CCwGTV. #itworksonmymachine
For downmix to stereo, my sound system is stereo so I can't be sure about whether surround sound is on or not. But I don't notice any issues or anything weird in the log.

Hmmm.. CCwGTV as well. I'm going to stand up a different Jellyfin server and test again. I'm passing the audio through an AVR so I've been able to validate when it is and isn't 5.1.

My server is 10.8-alpha5 (official jellyfin docker image) if that helps. If for some reason you're experiencing these issues due to being on 10.7.7 that's still a problem since we don't want to have compatibility broken.

@zkhcohen
Copy link
Contributor Author

zkhcohen commented Feb 13, 2022

My server is 10.8-alpha5 (official jellyfin docker image) if that helps. If for some reason you're experiencing these issues due to being on 10.7.7 that's still a problem since we don't want to have compatibility broken.

Yep... that seems to be the case. Upgrading from 10.7.7 to 10.8-alpha5 immediately resolved all of the weird transcoding issues. The only remaining issues are related to refreshing the audio stream appropriately so that you're able to switch back to Direct Play.

Since I had similar issues with the v0.13.0 beta without the content of this PR, I think we might want to do more testing against 10.7.7. It might be helpful to note that I'm using the lscr.io (Linuxserver) container image, and since they don't tag the alpha releases, I moved to their Nightly branch (which is currently building 10.8.0).

Since my changes are changing the same user setting in the backend, my guess is that downmixing is largely broken for CCwGTV and ExoPlayer with this release.

I'll work on resolving the audio output switching issue...

@mueslimak3r
Copy link
Member

mueslimak3r commented Feb 13, 2022

Testing based using nightly server builds may produce behavior that's the server's fault and wouldn't be present otherwise.
I do plan to spin up a nightly build to see if it fixes an issue I'm having with HLS timestamps.

If you ever want to migrate from the linuxserver image -> official (for 10.8-alpha builds) you can map the linuxserver /config layout to the official image using env variables:


      - JELLYFIN_DATA_DIR=/config/data
      - JELLYFIN_CONFIG_DIR=/config
      - JELLYFIN_LOG_DIR=/config/log
      - JELLYFIN_CACHE_DIR=/config/cache

@zkhcohen
Copy link
Contributor Author

zkhcohen commented Feb 13, 2022

Testing based using nightly server builds may produce behavior that's the server's fault and wouldn't be present otherwise. I do plan to spin up a nightly build to see if it fixes an issue I'm having with HLS timestamps.

Sorry, to clarify, I was running their stable build of 10.7.7 when I encountered the issues. I only moved to the nightly build to test 10.8.0.

It might be worth testing the official 10.7.7 image to see if the same transcoding issues persist.

If you ever want to migrate from the linuxserver image -> official (for 10.8-alpha builds) you can map the linuxserver /config layout to the official image using env variables:

The only thing preventing me from moving to the official image is the user mapping. I haven't managed to find a way to get the official container to bind to the appropriate UID/GID. No matter which combination of mapping ("User: UID:GID", "UID=XX, GID=XX" "PUID=XX, PGID=XX", "GUID=XX") I've tried in my compose file, it still ends up creating the config files as root. I haven't invested the time to find a workaround for this. The linuxserver image works OOTB when using PUID and PGID as env vars.

@DavidFair
Copy link
Contributor

DavidFair commented Feb 13, 2022

The only thing preventing me from moving to the official image is the user mapping. I haven't managed to find a way to get the official container to bind to the appropriate UID/GID. No matter which combination of mapping ("User: UID:GID", "UID=XX, GID=XX" "PUID=XX, PGID=XX", "GUID=XX") I've tried in my compose file, it still ends up creating the config files as root. I haven't invested the time to find a workaround for this. The linuxserver image works OOTB when using PUID and PGID as env vars.

You can adapt this technique to modify the official docker image with your UID/GID:
(I'm on a phone so this example might not be perfect)

cat Dockerfile

FROM jellyfin/jellyfin
ARG UID=1000
ARG GID=1000
ARG UNAME=user
RUN groupadd -g $GID -o $UNAME
RUN useradd -m -u $UID -g $GID -o -s /bin/bash $UNAME
USER $UNAME

docker build . -t CustomImageName && docker run CustomImageName

This should run everything in the image with the specific UID and GID, but there might be some stuff you need to also chown <path> in your Dockerfile if they have root ownership.

@zkhcohen
Copy link
Contributor Author

zkhcohen commented Feb 13, 2022

You can adapt this technique to modify the official docker image with your UID/GID:

Thank you for the feedback! Unfortunately I'm trying to avoid managing the dockerfile myself (enough of that at my day job). I appreciate that Linuxserver has made rootless builds and user bindings paradigmatic; I might make the suggestion to the maintainers of the official image.

EDIT: From their Focal base image: https://github.com/linuxserver/docker-baseimage-ubuntu/blob/bionic/Dockerfile

echo "**** create abc user and make our folders ****" && \
 useradd -u 911 -U -d /config -s /bin/false abc && \
 usermod -G users abc && \
 mkdir -p \
	/app \
	/config \
	/defaults

@zkhcohen
Copy link
Contributor Author

zkhcohen commented Feb 17, 2022

If the above approach doesn't work you could also try using changeVideoStream similar to how seek() uses it. Perhaps conditionally if currently transcoding. This sends an apiClient.StopTranscodingProcesses() for that sessionId and then provides a new getVideoStreamInfo response via StopTranscodingResponse.

tbh there's a lot of weirdness around restarting or switching transcoded streams, and I don't fully understand it.

playbackManager.getValue().changeVideoStream(mCurrentStreamInfo, api.getValue().getDeviceInfo(), mCurrentOptions, pos * 10000, apiClient.getValue(), new Response<StreamInfo>() {

Unfortunately neither of these solutions have worked for me. Furthermore, it doesn't appear that transcoding is necessarily the issue, as I had expected, because I'm still unable to switch back to Direct Play audio (5.1) when LibVLC is not transcoding. In other words, I have a movie where both Direct Play and Downmixing don't result in transcoding with LibVLC, but I still can't switch back to Direct Play.

This issue also applies to ExoPlayer, so I need to figure out what the difference between starting a brand new stream, and stopping/refreshing/starting a pre-existing stream is. The PlaybackController class is quite difficult to follow...

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Feb 23, 2022
@mueslimak3r
Copy link
Member

mueslimak3r commented Mar 1, 2022

you could try giving max audio channels a value like 6 if down-mixing isn't enabled and see if that changes anything.
I found in order to toggle off burnt in subtitles it wasn't enough to just set setSubtitleStreamIndex(null) on the line below. I had to actually set it to -1 for the server to treat it as a preference instead of a default value.

internalOptions.setMaxAudioChannels(Utils.downMixAudio() ? 2 : null); //have to downmix at server

@nielsvanvelzen nielsvanvelzen added the next-release Pull request related to a future release which is not being worked on yet label Mar 4, 2022
@nielsvanvelzen nielsvanvelzen removed the next-release Pull request related to a future release which is not being worked on yet label Mar 17, 2022
@mueslimak3r mueslimak3r marked this pull request as draft March 20, 2022 01:08
@nielsvanvelzen
Copy link
Member

@mueslimak3r @zkhcohen what's the state of this PR?

@mueslimak3r
Copy link
Member

When I tested it a while ago it worked fine.
The issue that's been a roadblock is with switching between audio modes (downmix <--> direct) when it doesn't go from transcoding back to direct play when the audio mode is changed. I didn't experience this issue when I tested it. 🤷

If a solution for the audio issue isn't practical, the quality selection part might be ready as-is or with minor changes.

@zkhcohen
Copy link
Contributor Author

@mueslimak3r @zkhcohen what's the state of this PR?

Sorry about the delay. Things really picked up at work and I haven't had a chance to work on this at all.

When I tested it a while ago it worked fine. The issue that's been a roadblock is with switching between audio modes (downmix <--> direct) when it doesn't go from transcoding back to direct play when the audio mode is changed. I didn't experience this issue when I tested it. 🤷

Correct. If you're using Jellyfin Server 10.8.0 then the only remaining issue with this PR is the ability to switch back to direct play. I have confirmed this behavior on my AVR and in the logs.

If a solution for the audio issue isn't practical, the quality selection part might be ready as-is or with minor changes.

Agreed. I think we should release the quality selection feature now and delay the audio output selection feature for when the playback rewrite makes it more practical to address the direct play issue.

@crotoc
Copy link

crotoc commented Apr 18, 2022

When will these functionalities be merged to main commits? Want quality control badly! Thanks for adding this feature. Great job! @zkhcohen

@nielsvanvelzen
Copy link
Member

When will these functionalities be merged to main commits? Want quality control badly! Thanks for adding this feature. Great job! @zkhcohen

As stated above, there are still some issues with this PR that need to be resolved before we can merge it. It also needs to be rebased to solve the merge conflicts.

@zkhcohen
Copy link
Contributor Author

Jellyfin 10.8.0 finally released, so I think it's safe to begin investigating these issues again: https://jellyfin.org/posts/jellyfin-10-8-0/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge conflict Conflicts prevent merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants