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 a new option to select preferred audio codec for transcodes #1702

Merged
merged 3 commits into from
Mar 17, 2024

Conversation

chrisforte
Copy link
Contributor

Changes

Add a new option that allows the user to select a preferred audio codec to use during transcoding.

The user-selected preferred codec is added to the front of the codec list for video transcoding profiles. If a transcode is required, it will be selected first to use (assuming the server and stream type support it). If it is not supported (example: DTS is selected as preferred, but the device does not support it, like the Roku Streambar), the next codec in the list is used.

Additionally, the wording of the existing DTS option has been updated to reflect that it applies to audio transcoding profiles only.

Issues

Addresses #958 and #741, related to #1466

Additional Context

While watching different movies on my Roku Ultra with image-based subtitles, I noticed that my AVR was only receiving PCM audio. Upon investigation, I realized that all transcodes were using AAC instead of EAC3. Because Roku doesn’t support multichannel AAC, it was being downmixed to stereo before being passed to the AVR.

By changing the preferred codec to AC3 in my setup, I was able to retain the channel layout of a transcoded file (DTS 5.1 -> AC3 5.1 instead of DTS 5.1 -> AAC 5.1 -> AAC 2.0)

@chrisforte chrisforte requested a review from a team as a code owner February 12, 2024 20:01
@1hitsong 1hitsong added the new-setting A new user setting. label Feb 13, 2024
Copy link
Member

@cewert cewert left a comment

Choose a reason for hiding this comment

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

This is very similar to the DTS option but isn't tied to multichannel support. If that's something you or other users want then we should just get rid of the old DTS option completely. Especially because you are modifying the audio codec string right after the DTS option which effectively makes the DTS option useless when they are both turned on.

@chrisforte
Copy link
Contributor Author

This is very similar to the DTS option but isn't tied to multichannel support. If that's something you or other users want then we should just get rid of the old DTS option completely. Especially because you are modifying the audio codec string right after the DTS option which effectively makes the DTS option useless when they are both turned on.

That was my initial change (before the PR), but I realized that the current DTS option only changes the codec list for audio profiles, and this change adjusts the video profiles. I didn't want to remove the current setting in case other users were expecting the current functionality to persist. If a combined setting would have the same result, that works for me.

@cewert
Copy link
Member

cewert commented Feb 14, 2024

... I realized that the current DTS option only changes the codec list for audio profiles, and this change adjusts the video profiles.

I don't follow. Here's my understanding of things. Let me know if this makes sense to you.

By default we use AAC as the preferred audio codec because it is compatible with all stereo devices which is why it is hardcoded here to always be the first item in the audio transcode codec list.

A few lines below that is the only place that uses the DTS setting. All it does is change the order in which we test for multi channel support. Then down just above your current changes here we put the preferred audio codec in the front of the tsArray.AudioCodec and tsArray.AudioCodec strings just like you are doing in this PR.

The only difference between the two would be the transcoding profiles that get added for the preferred surround sound codec here but I forget what that does exactly 🤔 .

I'm suggesting that we remove the whole if statement here, rework the audioCodecs array to include all the multichannel codecs first instead of being in a separate array here, and then keep your changes to also copy the preferred codec to the front of the string.

Copy link
Contributor Author

@chrisforte chrisforte left a comment

Choose a reason for hiding this comment

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

Changes made. I retained the settings menu structure, but this option could be moved up one level.

@cewert
Copy link
Member

cewert commented Feb 23, 2024

@chrisforte looks great!

I agree, we should move the setting up. Can you move it up a level but keep the current Preferred Audio Codec title?

@jellyfin-bot
Copy link
Contributor

This pull request has been inactive for 21 days and will be automatically closed in 7 days if there is no further activity.

@jellyfin-bot jellyfin-bot added the stale This issue/PR has gone stale. label Mar 16, 2024
@cewert cewert removed the stale This issue/PR has gone stale. label Mar 16, 2024
@1hitsong 1hitsong merged commit 5f18ac9 into jellyfin:master Mar 17, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-setting A new user setting.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants