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 preferences to configure audio codecs and 4k transcoding/direct play #3110

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

t0mas
Copy link

@t0mas t0mas commented Oct 9, 2023

Allow users to select which audio codecs to direct play, and in case of transcoding select the preferred codec to transcode into. Main use-case is for systems where the connected audio equipment fails to play some codecs. For example transcode Dolby TrueHD and DTS into AC3 or EAC3 if those are supported by the audio receiver or select AAC if more advanced formats are not supported.

Allow users to select whether 4k video streams can be accepted or not. Main example is the "Chromecast with Google TV" device, which in the old method will request 4k streams but is unable to play those (there exists a 4K version of the device). The new preference allows users to disable 4k if their player is not capable of playing it. The setting defaults to the same as the original behaviour (device detection).

I saw PR #3109 by @MichaelRUSF overlaps with this but only does AAC to DolbyDigital and not the other options.

Changes

  • Add user preferences to enable audio codecs
  • Add user preference to select transcode audio target
  • Add user preference to enable/disable 4k video
  • Changed ExoPlayerProfile to be based on the selected preferences

Issues
Fixes #2867, #2602, #2991 for audio and #2971, #2516 for video

@t0mas
Copy link
Author

t0mas commented Oct 9, 2023

Looking at older issues, this also fixes #2432, #1765, #618

Could be useful to keep all those issue numbers together when either this PR or another solution is merged to close a lot of older ones.

@nielsvanvelzen
Copy link
Member

I'm currently focusing on getting 0.16 out and bugfixes can still be merged but larger changes (especially those with string changes) are unfortunately going to wait for 0.17. This also means it will take some time before I'll review the pull request.

Additionally, my focus with 0.17 is to implement the new playback code for all video playback and completely removing the old implementation which will basically remove all changes from this PR again. The profiles will work quite differently in the rewrite (with fancy auto detection and such) so it won't be easy to port these changes over. Individual toggles for all codecs/formats are planned though.

@SplitMilky
Copy link

I think #520 would be fixed as well.

Copy link
Member

@nielsvanvelzen nielsvanvelzen left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait. I've done an initial review on the changes and got some small comments.

/**
* Enable 4k codecs for direct play and transcode target
*/
var enable4kSupport = booleanPreference("pref_enable_4k_support", DeviceUtils.has4kVideoSupport())
Copy link
Member

Choose a reason for hiding this comment

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

Preference keys should not start with pref_. This is only used on old keys for compatibility reasons (creating a migration just for a rename of the key is kinda pointless)

/**
* Enable TrueUD
*/
var thdEnabled = booleanPreference("pref_bitstream_thd", false)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should shorten this to thd, doesn't seem like an "official" abbreviation.

Comment on lines +31 to +33
audioDirectPlayCodecs : Array<String>,
audioTranscodeTarget : Array<String>,
enable4KSupport : Boolean
Copy link
Member

Choose a reason for hiding this comment

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

This is not correctly formatted.

Suggested change
audioDirectPlayCodecs : Array<String>,
audioTranscodeTarget : Array<String>,
enable4KSupport : Boolean
audioDirectPlayCodecs: Array<String>,
audioTranscodeTarget: Array<String>,
enable4KSupport: Boolean

@@ -175,8 +177,18 @@
<string name="msg_item_added">" item added"</string>
<string name="desc_automatic_fav_songs">Automatic playlist of favorite songs</string>
<string name="lbl_new_premieres">New premieres</string>
<string name="lbl_bitstream_ac3">Bitstream Dolby Digital audio</string>
<string name="desc_bitstream_ac3">Requires capable hardware</string>
<string name="pref_audio_directplay">Direct Play Audio Codecs</string>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<string name="pref_audio_directplay">Direct Play Audio Codecs</string>
<string name="pref_audio_directplay">Direct play audio codecs</string>

case AUTO:
String[] directPlayCodecs = getDirectPlayPreferences();
// If any codecs are enabled, return those
if(directPlayCodecs.length > 0) return directPlayCodecs;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(directPlayCodecs.length > 0) return directPlayCodecs;
if (directPlayCodecs.length > 0) return directPlayCodecs;

Comment on lines +206 to +207
category {
setTitle(R.string.pref_audio_directplay)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better if add a new preference screen for this category. You can look at UserPreferencesScreen to see how to link to one.

@nielsvanvelzen nielsvanvelzen added the enhancement New feature or request label Jan 20, 2024
@zkhcohen
Copy link
Contributor

This is a great addition to Jellyfin's config.

It would also serve as a workaround for: #3227

@jellyfin-bot jellyfin-bot added the merge conflict Conflicts prevent merging label Jan 21, 2024
@PriceChild
Copy link

This is awesome. I have a 1st gen fire TV and this makes it very easy to disable aac. Audio is being transcoded to my preferred format using the debug build I built myself from this branch.

Unfortunately (and identical to one of the other branches solution) although I get perfect 5.1 audio, the video takes seconds per frame, skipping.

Using the debug build, with aac disabled, If I check the server dashboard when playing a specific file, I see under reasons for transcode is that both audio and video are not supported by the player.

Using the debug build, with aac enabled, I get perfect video and stereo audio. The server dashboard explaining it is transcoding for video.

If I use the fire TV store's release build, I see under reasons for transcode only video being the reason.

So the server is transcoding in both cases, about 80fps reported by the server.

Is there any more troubleshooting I can provide to understand why taking advantage of this change helps, but also regresses my device's performance?

@anym001
Copy link

anym001 commented Feb 2, 2024

Where can I find these settings for audio codec directplay?

@PriceChild
Copy link

@anym001 This isn't in a released build. Have you installed a build based on this branch?

@anym001
Copy link

anym001 commented Feb 2, 2024

@PriceChild Oh okay, I wasn't aware of that. I am currently on the stable release.

Is it already foreseeable when this build will be released?

@nielsvanvelzen
Copy link
Member

Is it already foreseeable when this build will be released?

The pull request is currently waiting on @t0mas to update it so I can re-review and potentially merge. It will then be included in 0.17 (no ETA). If no activity happens the pull request will eventually be closed as stale.

@t0mas
Copy link
Author

t0mas commented Feb 4, 2024

@nielsvanvelzen Ok, thanks for reviewing! I'll look into the separate screen option. Quite busy at the moment so it may take a while.

@t0mas
Copy link
Author

t0mas commented Feb 4, 2024

@PriceChild:

Is there any more troubleshooting I can provide to understand why taking advantage of this change helps, but also regresses my device's performance?

This branch also adds a setting to enable and disable 4k video playback. The Fire stick is one of the devices that is explicitly listed in the original code to disable 4k playback, because it doesn't have enough performance to play it. Did you test with a 1080p video or with a 4k one?

}

if(userPreferences.getValue().get(UserPreferences.Companion.getDtsEnabled())) {
codecs.add(Codec.Audio.DTS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
codecs.add(Codec.Audio.DTS);
codecs.add(Codec.Audio.DCA);
codecs.add(Codec.Audio.DTS);

case AAC:
return new String[] { Codec.Audio.AAC, Codec.Audio.AAC_LATM };
case DTS:
return new String[]{ Codec.Audio.DTS };
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return new String[]{ Codec.Audio.DTS };
return new String[]{ Codec.Audio.DCA, Codec.Audio.DTS};

case DTS:
return new String[]{ Codec.Audio.DTS };
case PCM:
return new String[]{ Codec.Audio.PCM_ALAW, Codec.Audio.PCM_MULAW };
Copy link
Contributor

Choose a reason for hiding this comment

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

When trying to transcode to DTS or PCM (original files in an MKV container), the server sends an incompatible file container message and remuxes the file with the original audio and video. I would consider removing these as transcoding options.

Comment on lines +623 to +624
codecs.add(Codec.Audio.OPUS);
codecs.add(Codec.Audio.FLAC);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you’re adding a new panel and there are only two other codecs, I would add them as separate options.

checkbox {
setTitle(R.string.lbl_codec_other)
setContent(R.string.desc_bitstream_generic)
bind(userPreferences, UserPreferences.otherAudioEnabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

As commented above, I think it makes sense to split out OPUS and FLAC as separate options.

}

if(userPreferences.getValue().get(UserPreferences.Companion.getThdEnabled())) {
codecs.add(Codec.Audio.TRUEHD);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
codecs.add(Codec.Audio.TRUEHD);
codecs.add(Codec.Audio.MLP);
codecs.add(Codec.Audio.TRUEHD);

Comment on lines +621 to +622
codecs.add(Codec.Audio.DCA);
codecs.add(Codec.Audio.MLP);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
codecs.add(Codec.Audio.DCA);
codecs.add(Codec.Audio.MLP);

@cgernert
Copy link

cgernert commented Feb 20, 2024

I have a Sony (Google) TV and am also trying to solve "AAC Plays as Stereo" #2602

I have built
https://github.com/t0mas/jellyfin-androidtv/tree/transcode-preferences
and it does not work despite trying different configurations: Transcoding to PCM, DD, DTS. AAC checked and unchecked as supported format. I got 2.0 or 2.1 PCM (depending on TV audio settings) from the 5.1 AAC stream. No transcoding.

On the other hand, I built
https://github.com/MichaelRUSF/jellyfin-androidtv/tree/master
and it works. I get 5.1 PCM from the 5.1 AAC stream.

I do not know if this behavior is intended or not. I hope an effective fix will be included in the official release soon.

@nielsvanvelzen
Copy link
Member

I have built t0mas/jellyfin-androidtv@transcode-preferences and it does not work despite trying different configurations: Transcoding to PCM, DD, DTS. AAC checked and unchecked as supported format. I got 2.0 or 2.1 PCM (depending on TV audio settings) from the 5.1 AAC stream. No transcoding.

On the other hand, I built MichaelRUSF/jellyfin-androidtv@master and it works. I get 5.1 PCM from the 5.1 AAC stream.

You're comparing a pull request that is almost 300 commits behind with a fork that is only 11 behind. Not sure what you're getting at.

@SplitMilky
Copy link

So what are we waiting on here? For nielsvanvelzen comments to be incorporated in? Can i do this directly or do I have to make a fork of this fork to add suggested comment changes?

@cgernert
Copy link

You're comparing a pull request that is almost 300 commits behind with a fork that is only 11 behind. Not sure what you're getting at.

So the code that was recently (2 weeks ago) reviewed by MichaelRUSF on https://github.com/jellyfin/jellyfin-androidtv/pull/3110/files/7e36ca6af03d11c4f1dc93bf4e97ccf3c435d2b5 is just the settings in the UI?
I just wonder why this code was recently reviewed, resulting in an incomplete(?) fix for the issue.

@cgernert
Copy link

So what are we waiting on here? For nielsvanvelzen comments to be incorporated in? Can i do this directly or do I have to make a fork of this fork to add suggested comment changes?

I just built MichaelRUSF's master branch.

@nielsvanvelzen
Copy link
Member

@t0mas have you been able to look at this PR again?

@dandud100
Copy link

@t0mas have you been able to look at this PR again?

It's been a week without a reply from @t0mas, and I'd like to inquire about the status of this PR. Could someone else possibly take over to finalize it, or could you provide an update on how we'll proceed from here?

@nielsvanvelzen
Copy link
Member

If the pull request becomes stale (e.g. author is unresponsive/unwilling to continue with the PR) it will eventually be closed. Someone else can of course take over (with permission from the author).

@t0mas
Copy link
Author

t0mas commented Apr 23, 2024

@dandud100 I unfortunately don't have time at the moment, but of course no objections if someone else wants to make the required changes.

@dandud100
Copy link

dandud100 commented Apr 27, 2024

Someone else can of course take over (with permission from the author).

t0mas don't have time at this moment but replied here to give the permission for someone to take over.

@MichaelRUSF are you able to continue this pr?

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

Successfully merging this pull request may close these issues.

Add a fallback to AC3/EAC3 option