fix(macos): drop max_ref_frames=1 for h264_videotoolbox and enable PARALLEL_ENCODING#5200
Conversation
VideoToolbox on Apple Silicon produces all-IDR output when ReferenceBufferCount=1 is set for H.264 — every frame becomes a keyframe, P-frames are never produced, and bandwidth inflates roughly 3x while frame drops become severe. HEVC and AV1 on the same hardware are unaffected by this quirk and continue to honor max_ref_frames=1 correctly for reference-frame invalidation. Remove the option from h264_videotoolbox's common-options block only. HEVC and AV1 retain max_ref_frames=1 as before. Tracks LizardByte#5013. Diagnosis and original fix from the Lumen fork (github.com/trollzem/Lumen).
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Removes the max_ref_frames=1 setting for H.264 encoding on VideoToolbox to fix an issue where Apple Silicon produces all-IDR frames, causing severe bandwidth inflation and frame drops.
Changes:
- Removes
max_ref_framesfrom the H.264 VideoToolbox common options block. - Adds an explanatory comment documenting the rationale and referencing issue #5013.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e8318a536
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Note: max_ref_frames is intentionally omitted for H.264 because | ||
| // VideoToolbox on Apple Silicon produces all-IDR output when | ||
| // ReferenceBufferCount=1 is set for H.264, causing massive bandwidth | ||
| // inflation (~3x) and frame drops. HEVC and AV1 are unaffected and | ||
| // retain max_ref_frames=1. See LizardByte/Sunshine#5013. |
There was a problem hiding this comment.
For macOS H.264 streams where the client requests numRefFrames=1, this still sets VideoToolbox ReferenceBufferCount=1 through the generic ctx->refs = config.numRefFrames path: validate_encoder() will mark H.264 REF_FRAMES_RESTRICT true after the max-ref probe succeeds, and FFmpeg's VideoToolbox encoder maps avctx->refs > 0 to kVTCompressionPropertyKey_ReferenceBufferCount (per the upstream videotoolboxenc refs patch). So removing only the max_ref_frames AVOption does not avoid the all-IDR Apple Silicon behavior described here; H.264 VideoToolbox needs to opt out of the REF_FRAMES_RESTRICT/ctx->refs path too.
Useful? React with 👍 / 👎.
Per andygrundman + ReenigneArcher review on the original PR thread: > @andygrundman: "I can reproduce this bug and this is the right fix. > @ReenigneArcher can you also enable PARALLEL_ENCODING for all VT > encoders?" > > @ReenigneArcher: "@Nottlespike could you add @andygrundman's > suggestion to this PR please?" VideoToolbox encoders on macOS support multiple concurrent compression sessions (the kernel media engine schedules them), so the PARALLEL_ENCODING capability flag is correct to advertise. Sunshine uses this flag to decide whether the probe path may concurrently attempt multiple format/profile combinations against the same encoder. The flag lives on the encoder_t struct itself, not on per-codec options, so this single change covers all VT codecs the encoder registers (h264, hevc, and prores when present via the experimental opt-in in a separate PR).
|
Pushed @andygrundman's PARALLEL_ENCODING change as a follow-up commit (
Carried forward from the original PR thread (#5188, before the closed/reopen cycle that re-numbered this to #5200). The flag lives on the Sorry for the delay on this — got caught up in the close/reopen shuffle and missed bringing this forward when I recreated the PRs. |
|
Bundle ReportBundle size has no change ✅ |
ReenigneArcher
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5200 +/- ##
==========================================
+ Coverage 17.78% 17.82% +0.04%
==========================================
Files 111 111
Lines 24154 24153 -1
Branches 10688 10687 -1
==========================================
+ Hits 4296 4306 +10
- Misses 14682 16524 +1842
+ Partials 5176 3323 -1853
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 56 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
ReenigneArcher
added
the
ai
Description
VideoToolbox on Apple Silicon emits an IDR keyframe on every frame when
ReferenceBufferCount=1is set for H.264 — P-frames are never produced, and bandwidth inflates by roughly 3× while frame drops become severe under any sustained load. The hardware encoder's H.264 path interprets the minimum-ref-frames=1 constraint as "you have nothing to reference back to," so it cannot legally emit a P-frame and falls back to IDR-everything.HEVC and AV1 on the same Apple Silicon hardware are not affected by this quirk — they continue to honor
max_ref_frames=1correctly and produce normal P-frames for reference-frame invalidation, so the constraint is correctly retained for those codecs.This change removes
{"max_ref_frames"s, 1}from the common options block of theh264_videotoolboxencoder entry only. The encoder still goes through Sunshine'smax_ref_framescapability probe (config_max_ref_framestest), but without forcing the AVCodecContext option upfront — VideoToolbox is left to pick a sensible default reference count for H.264 (which it does correctly: 1-2 reference frames, producing real P-frames).Validated on Apple Silicon (M4 Max): H.264 stream bitrate at 1080p60 drops from ~14 Mbps (all-IDR) to ~4.5 Mbps (normal P-frame mix) at identical quality, matching expected behavior for H.264 streaming. HEVC and AV1 paths are bitwise unchanged.
The diagnosis and original fix are from the Lumen fork (github.com/trollzem/Lumen); this PR brings the fix upstream with an inline comment explaining the rationale so future readers don't try to "re-add" the constraint thinking it was an oversight.
Screenshot
N/A — encoder option change.
Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage