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

feat(audio): custom surround-params #2424

Merged

Conversation

mariotaku
Copy link
Contributor

@mariotaku mariotaku commented Apr 15, 2024

Description

This PR is made for improving webOS client support. On webOS 5.0 and above, if surround channel configuration is done properly, it can re-encode OPUS stream on-the-fly & send it to surround sound system through eARC/Optical.

Currently, webOS only handles 6-channels, 4 streams & 2 coupled streams configuration, and the channel must be ordered as [0, 1, 4, 5, 2, 3] which Sunshine doesn't have. It won't accept stream header either, which makes this PR necessary.

With this PR, if surroundParams is specified (formatted like surround-params= in RTSP handshake, e.g. 642014523) in HTTP launch request, it will be applied, and during RTSP handshake, our custom surround-params a=fmtp:97 surround-params=642014523 will be advertised twice first, followed by the original ones.

Screenshot

Not applicable

Issues Fixed or Closed

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

@mariotaku mariotaku force-pushed the feature/custom-surround-params branch from fd72e35 to ce8b027 Compare April 16, 2024 01:54
@mariotaku mariotaku marked this pull request as ready for review April 16, 2024 02:33
@mariotaku mariotaku changed the title Custom Surround Params feat(audio): custom surround params Apr 16, 2024
@mariotaku mariotaku changed the title feat(audio): custom surround params feat(audio): custom surround-params Apr 16, 2024
@kentyman23
Copy link

Do we want this to work with 11+ channels eventually? If so, using 0-9 won't work. Perhaps using a , delimiter and then parsing as an int would scale better?

@mariotaku
Copy link
Contributor Author

@kentyman23 In that case, Moonlight core lib needs to changed. And of course webOS needs that support too.

@mariotaku mariotaku force-pushed the feature/custom-surround-params branch from ce8b027 to 9fe580b Compare April 24, 2024 10:37
if (!session.surround_params.empty()) {
// If we have our own surround parameters, advertise them twice first
ss << "a=fmtp:97 surround-params="sv << session.surround_params << std::endl;
ss << "a=fmtp:97 surround-params="sv << session.surround_params << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of advertising them twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first surround-params will be applied to NormalQualityOpusConfig, and the second one goes to HighQualityOpusConfig. I think advertising them twice first will make sure both configs having the desired mapping.

@moi952
Copy link

moi952 commented May 1, 2024

Hi, will ac3 encoding be functional on moonlight-android and moonlight-qt? (provided that development is done I suppose)

@mariotaku
Copy link
Contributor Author

@moi952 Hi, this PR won't enable AC3 codec, it only rearranges sound channel mapping. May I ask why you are interested in AC3 codec? As far as I know, all existing clients handles OPUS better.

@moi952
Copy link

moi952 commented May 1, 2024

@moi952 Hi, this PR won't enable AC3 codec, it only rearranges sound channel mapping. May I ask why you are interested in AC3 codec? As far as I know, all existing clients handles OPUS better.

Thank you for your answer. Too bad, I don't know OPUS but I guess it's PCM?
In fact with my current TV if I connect my Nvidia Shield or my PC to the TV and I put it in 5.1, my amp (Denon AVC-6500x) remains in stereo, I don't know if the TV doesn't allow it to accept uncompressed 5.1.
On the other hand, if I connect my Nvidia Shield directly to my amp I have good sound in 5.1

I tried different sound input/output settings on my TV but it doesn't work.

For example for Windows if I connect my PC to my TV, it does not offer me more than 2 channels in the sound output configuration.

@mariotaku
Copy link
Contributor Author

@moi952 That may have something to do with the TV, and yes while Sunshine transfers audio to Moonlight using OPUS, it will eventually become PCM. Let's discuss in the channel you're in on discord.

@ReenigneArcher ReenigneArcher changed the base branch from master_legacy to master May 29, 2024 00:59
@ReenigneArcher
Copy link
Member

@mariotaku could you rebase your PR?

Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 20.83333% with 38 lines in your changes missing coverage. Please review.

Project coverage is 7.98%. Comparing base (7371a20) to head (4e2d4ff).
Report is 155 commits behind head on master.

Files with missing lines Patch % Lines
src/rtsp.cpp 0.00% 19 Missing ⚠️
src/audio.cpp 37.03% 17 Missing ⚠️
src/nvhttp.cpp 0.00% 1 Missing ⚠️
src/process.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           master   #2424      +/-   ##
=========================================
+ Coverage    7.01%   7.98%   +0.97%     
=========================================
  Files          88      88              
  Lines       17987   18036      +49     
  Branches     8579    8596      +17     
=========================================
+ Hits         1261    1441     +180     
- Misses      14019   15827    +1808     
+ Partials     2707     768    -1939     
Flag Coverage Δ
Linux 6.05% <17.50%> (+0.73%) ⬆️
Windows 3.76% <21.95%> (+1.15%) ⬆️
macOS-12 9.03% <12.76%> (+1.00%) ⬆️
macOS-13 8.92% <12.76%> (+1.00%) ⬆️
macOS-14 9.22% <12.76%> (+1.00%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/audio.h 0.00% <ø> (ø)
src/rtsp.h 0.00% <ø> (ø)
src/nvhttp.cpp 1.34% <0.00%> (-0.15%) ⬇️
src/process.cpp 1.62% <0.00%> (-0.28%) ⬇️
src/audio.cpp 28.57% <37.03%> (+27.02%) ⬆️
src/rtsp.cpp 1.54% <0.00%> (-0.06%) ⬇️

... and 30 files with indirect coverage changes

@mariotaku mariotaku force-pushed the feature/custom-surround-params branch from 0511af3 to ca24752 Compare May 29, 2024 02:41
@ReenigneArcher ReenigneArcher force-pushed the feature/custom-surround-params branch from da82fa2 to 4e2d4ff Compare June 1, 2024 01:38
@ReenigneArcher
Copy link
Member

@mariotaku are you happy with this PR and tests?

@mariotaku
Copy link
Contributor Author

@mariotaku are you happy with this PR and tests?

Yes, I'd like to get it merged :)

@ReenigneArcher ReenigneArcher merged commit 17e0f1a into LizardByte:master Jun 1, 2024
47 checks passed
@mariotaku mariotaku deleted the feature/custom-surround-params branch June 2, 2024 13:48
KuleRucket pushed a commit to KuleRucket/Sunshine that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants