Repository navigation
Conversation
|
Update: I retrieved the raw log for the I added the missing comments in c9ad363. The new Per your guidance, I have not started the GitHub Actions workflows. I have also left the separate |
We only get 20 runners from GitHub and these are basically running all day long, so I'll run it when I'm ready to merge the code. Docs failures are here: https://app.readthedocs.org/projects/sunshinestream/builds/35131897/ Don't worry about the other already reported failure. Your agent can lint and build the docs if it follows the AGENTS.md file. |
ReenigneArcher
left a comment
There was a problem hiding this comment.
Requesting changes on c9ad3637b48907316f3957f72ac46cb9c7155023: the settings page cannot see the advertised encoder capabilities, and the new backend/negotiation behavior has no added regression tests.
On FFmpeg packaging, I recommend using Sunshine's existing linked FFmpeg libraries for this feature. The pinned build-deps configuration enables the encoders, and its published macOS archive already contains both native encoders. I linked the PR's extracted encoder class to that archive and successfully encoded all eight combinations of AC-3/E-AC-3, stereo/5.1, and normal/high quality. This establishes that a second runtime FFmpeg distribution is unnecessary for those encoding operations; it is not a Windows DLL-loader test.
The native AC-3 and E-AC-3 implementations are LGPL-2.1-or-later, compatible with Sunshine's GPL-3.0 licensing. FFmpeg's legal page distinguishes copyright licensing from codec patents. I found no identified licensing requirement in this PR that mandates user-supplied DLLs. Dolby's 2007 filing listed original Dolby Digital patent expirations through 2017 and Dolby Digital Plus through 2022; that historical disclosure is not comprehensive present-day patent or trademark clearance.
Validation: 28 existing focused Vue tests passed, along with two temporary review reproductions showing the settings bug and verifying the proposed endpoint correction; all eight isolated encoding cases passed; whitespace checks passed for all 15 changed files. The reproductions and probes live outside the working checkout and are not tests contributed by this PR.
GitHub reports the head as mergeable and behind master (current branch tip a178f465e42f8e77102002e9156b8becd7b13628). The Sunshine documentation check passed. Full Windows builds, Windows DLL loading, client passthrough, and protocol interoperability were not verified locally; GitHub Actions await maintainer approval. The reported Sonar issue points to the unchanged permission-status test override, so it is not a new-code finding.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
|
Thanks for the detailed review. I pushed a revision to address the requested changes:
The code and tests are committed, but I have not run the revised Windows build or tests locally. I have updated the PR description to reflect that CI and playback re-validation of the linked-FFmpeg change are still pending. I have not attempted to start the Actions workflows, per your note about the runner pool. |
|
I extended the revision beyond the review fixes to remove the remaining Windows/runtime-DLL assumptions:
The current branch head contains these changes. I have not run the revised build/tests or verified Dolby playback on other platforms; per the earlier request, I am leaving Actions for maintainer scheduling. |
|
@ReenigneArcher I've addressed the four SonarCloud findings in the latest revision ( |
|
Hi @ReenigneArcher, just following up on this PR. I've addressed the requested changes and tested the Windows build locally; it's working correctly with Opus, AC-3, and E-AC-3. When you have a chance, could you please take another look and approve it if everything looks good? Thanks! |
|
There is still something I'm missing? Do I need to make any other change here @ReenigneArcher ? |
ReenigneArcher
left a comment
There was a problem hiding this comment.
Could you fix the sonar issues before I review this again?
Thanks for the feedback. I've gone through all 30 SonarCloud issues listed in the report and compared their locations against the actual PR diff. None of the reported issues appears to be located in a line modified by this PR. Although some findings are in files touched by the PR, they refer to pre-existing code outside the changed sections. Several are also reported as having been present for years. For example, the numerous I agree that genuine Sonar issues introduced by this PR should be addressed. However, I don't think it would be appropriate to modify unrelated upstream code as part of this feature, particularly when none of these 30 findings appears to originate from my changes. Could you please check why SonarCloud is classifying these findings as new issues for this PR? If there are specific findings located in code actually modified by this PR that I've overlooked, please point me to them and I'll address them. |
Okay, thank you for double checking. Sonar will report issues that are close to changed code, so those are likely acceptable. |
You can merge the PR then. All is ok :) |
|
@ReenigneArcher I can also open a new PR from a clean |
|
I'm going as fast as I can |
|



Description
This PR adds optional Dolby Digital (AC-3) and Dolby Digital Plus (E-AC-3) encoded audio output to Sunshine on platforms whose linked FFmpeg build contains the corresponding encoders. Compatible Moonlight clients can negotiate these formats as an alternative to Opus when they need a compressed Dolby bitstream for native decoding or passthrough.
The main use case is Moonlight running directly on a Smart TV connected to a receiver or sound system through optical S/PDIF. AC-3 can provide Dolby Digital stereo or 5.1 output over compatible TV audio paths. Actual output depends on the TV model, its audio settings, the receiver, and client support; E-AC-3 passthrough over optical output is model-dependent.
Related client implementation
The companion Moonlight Tizen with AC-3 support (see details in its website) implements the client-side Dolby offer/accept negotiation and passthrough through Tizen's native EMSS audio renderer. It is an independently maintained client and is linked here as an example of a client that uses this new host feature. This PR changes Sunshine only; other clients need their own support for the optional negotiation and playback/passthrough path.
Implementation
High quality audioswitch, disabled by default, for higher Dolby bitrates at the cost of increased network bandwidth. It has no effect on Opus and is disabled in the UI while Opus is selected.The existing Opus encoder and its normal negotiation path are intended to remain behaviorally unchanged. This is a host-side feature, not a Tizen-only or Windows-only server feature.
Testing
sunshinestreamRead the Docs build passed (build 35163518).masterinto the feature branch in commite7e57df; GitHub reports no merge conflicts.Type of Change
Checklist
lb-update-clang-formathelper has been run and a second pass produced no changes; other lint checks have not been run on the latest revision).AI Usage
The contributor remains responsible for reviewing the implementation, verifying test results, and addressing review feedback.