fix(amf): enable H.264 profile and coder selection#4927
Conversation
|
Thank you for the PR submission, but I noticed you didn't use our PR template. Please update the PR to use the correct template. You can find it at https://github.com/LizardByte/.github/blob/master/.github/pull_request_template.md?plain=1 |
ReenigneArcher
added
the
ai
46ed0ac to
3b8a19f
Compare
4fe233d to
3b8a19f
Compare
|
3b8a19f to
7fd0fc5
Compare
|
Updated this PR on top of current master and addressed the review feedback about \cfg.profile. Changes in this update:
Reference used: AMF encoder parameter listings for FFmpeg. |
|
ReenigneArcher
left a comment
ReenigneArcher
left a comment
There was a problem hiding this comment.
This PR claims to change the UI in the PR description, but no ui files are modified? Is any of this meant to be user configurable? Or only configured from the client itself?
For example, the PSP-1000's Media Engine only supports
Didn't realize there's a Moonlight client for PSP. You should add https://github.com/k4idyn/Moonlight-PSP to https://github.com/LizardByte/app-directory whenever you feel it's stable enough.
|
In the Configuration -> Encoder tabs, Sunshine/Apollo allows selecting cavlc/cabac in the UI, this setting is ignored by the encoder because it isn't passed to the FFmpeg dictionary; Forcefully streaming cabac regardless of what the user chooses. |
|
Thanks for confirming. There are conflicts on this PR, can you resolve them? |
7fd0fc5 to
96bec1c
Compare
|
Rebased on latest master and resolved merge conflicts. |
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4927 +/- ##
==========================================
- Coverage 26.70% 26.68% -0.02%
==========================================
Files 111 111
Lines 25406 25416 +10
Branches 11232 11236 +4
==========================================
- Hits 6785 6783 -2
- Misses 15624 16231 +607
+ Partials 2997 2402 -595
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 44 files with indirect coverage changes Continue to review full report in Codecov by Harness.
|
|
Failures are probably all from docs. You can easily see the errors here: https://app.readthedocs.org/projects/sunshinestream/builds/33718953/ and search for |
96bec1c to
d497913
Compare
|
Added Doxygen docstrings (/** @brief ... */) to |
d497913 to
0bb97be
Compare
|
| // SDR-specific options | ||
| {"profile"s, [](const config_t &) { | ||
| switch (config::video.amd.amd_coder) { | ||
| case (int) amf::coder_e::cavlc: | ||
| return "constrained_baseline"s; | ||
| case (int) amf::coder_e::cabac: | ||
| return "high"s; | ||
| default: | ||
| return "main"s; | ||
| } | ||
| }}, | ||
| }, |
There was a problem hiding this comment.
According to GPT 5.6,
Baseline profile is never applied. This new profile option is an AMF-private option, but Sunshine already sets AVCodecContext::profile to High for H.264 at line 1994. FFmpeg gives that public profile precedence and only consults the private option when no recognized public profile was supplied, as shown in FFmpeg’s AMF initializer. Consequently, CAVLC is enabled but the stream remains High Profile; the claimed PSP/legacy-decoder fix does not work. The AMF CAVLC path needs to set AVCodecContext::profile to constrained baseline instead.
So, I put up an alternate approach to this. Can you check if #5442 solves your issues? I also added some tests and resolved some existing sonar issues that were highlighted in this PR.
2 participants
Description
h264_amf) insrc/video.cppdoes not have theprofileandcoderoptions wired up in itsencoder_tdefinition. This prevents clients that strictly require H.264 Baseline/CAVLC (like the Sony PSP or other legacy hardware decoders) from successfully streaming, as the encoder defaults to High Profile and CABAC regardless of the client's request or the global configuration.Older hardware decoders often have hardwired limitations. For example, the PSP-1000's Media Engine only supports:
While Sunshine/Apollo allows selecting
cavlcin the UI, this setting is ignored by theh264_amfencoder because it isn't passed to the FFmpegavcodec_open2dictionary.Update the
amdvcedefinition insrc/video.cppto include the missing options:{"coder"s, &config::video.amd.amd_coder}, {"profile"s, [](const config_t &cfg) { if (cfg.profile == 66) return "baseline"s; if (cfg.profile == 77) return "main"s; return "high"s; }},This brings the AMD implementation into parity with NVENC and QuickSync.
Screenshot
Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage