-
-
Notifications
You must be signed in to change notification settings - Fork 522
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
Conversation
97f206d
to
7e36ca6
Compare
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. |
I think #520 would be fixed as well. |
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.
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()) |
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.
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) |
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 don't think we should shorten this to thd
, doesn't seem like an "official" abbreviation.
audioDirectPlayCodecs : Array<String>, | ||
audioTranscodeTarget : Array<String>, | ||
enable4KSupport : Boolean |
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.
This is not correctly formatted.
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> |
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.
<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; |
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.
if(directPlayCodecs.length > 0) return directPlayCodecs; | |
if (directPlayCodecs.length > 0) return directPlayCodecs; |
category { | ||
setTitle(R.string.pref_audio_directplay) |
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 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.
This is a great addition to Jellyfin's config. It would also serve as a workaround for: #3227 |
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? |
Where can I find these settings for audio codec directplay? |
@anym001 This isn't in a released build. Have you installed a build based on this branch? |
@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? |
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. |
@nielsvanvelzen Ok, thanks for reviewing! I'll look into the separate screen option. Quite busy at the moment so it may take a while. |
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); |
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.
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 }; |
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.
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 }; |
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.
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.
codecs.add(Codec.Audio.OPUS); | ||
codecs.add(Codec.Audio.FLAC); |
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.
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) |
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.
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); |
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.
codecs.add(Codec.Audio.TRUEHD); | |
codecs.add(Codec.Audio.MLP); | |
codecs.add(Codec.Audio.TRUEHD); |
codecs.add(Codec.Audio.DCA); | ||
codecs.add(Codec.Audio.MLP); |
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.
codecs.add(Codec.Audio.DCA); | |
codecs.add(Codec.Audio.MLP); |
I have a Sony (Google) TV and am also trying to solve "AAC Plays as Stereo" #2602 I have built On the other hand, I built I do not know if this behavior is intended or not. I hope an effective fix will be included in the official release soon. |
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 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? |
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 built MichaelRUSF's master branch. |
@t0mas have you been able to look at this PR again? |
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). |
@dandud100 I unfortunately don't have time at the moment, but of course no objections if someone else wants to make the required changes. |
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? |
If all this request needs is the changes specified in previous comments, I can continue some work on this PR. |
There would definitely be additional review feedback, so it's not just rebasing + applying the current feedback comments. |
Closing as stale. Please re-open or create a new pull request if interested in finishing. |
@majmongoose are you able to work on it or not? Btw. Is there an option to add an bounty to this pr? |
I did further review of the previous committers code and as Neils stated it would be a significant amount of work to bring it up to date; which is unfortunately beyond my skill set and availability. I did make a PR (since accepted) to add the Chromecast HD (CCHD) to the list of devices which is forced to a max of 1080p, but it's not in any of the app releases yet. However the server appears to be ignoring the limit and sends the CCHD 4k anyway, this may be a bug with the server but again, it's beyond my skill set. In the end I ditched the CCHD and spent $20 of my hard earned cash at a local Walmart for the Onn 4K streaming box. No extra ads, no extra tracking that I can see, and runs a stock Android TV experience. And of course, it can actually handle 4K downscaling in-device. Regarding the bounty theory, generally that's poor taste for open source projects like this, but I don't know the feelings of the team here. |
The pull request (#3820) is released in 0.17.1. The 1080p conditions require Jellyfin 10.9.10 to work properly as there was also an server-side issue (since 10.8.0) we discovered. |
Aha! Thanks for the update Neils. |
Is this known to still be broken with server 10.9.11? Mine is still direct streaming 4k content, unfortunately. I'm on a Chromecast HD, which supposedly is already hardcoded to require 1080p max, as of Jellyfin TV 0.17.5. Also I'm not sure if it's relevant, but for some reason my Chromecast is only reporting its model as "Google Chromecast", verified via Jellyfin TV's About entry, and AIDA64 (for the latter, it's specifically "Google" for manufacturer, and only "Chromecast" for model string). Pointing this out only because the hardcode appears to be for "Chromecast HD" model string specifically. |
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
Issues
Fixes #2867, #2602, #2991 for audio and #2971, #2516 for video