feat(mac): Mac mini optimization — hardware acceleration, codec negot…#566
feat(mac): Mac mini optimization — hardware acceleration, codec negot…#566ilovekyrinathemost-maker wants to merge 4 commits into
Conversation
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
📝 WalkthroughWalkthroughAdds macOS-specific video acceleration, codec reordering, diagnostics, and stream presets, plus renderer input protocol and pointer lock updates for macOS. ChangesMac Mini Optimization
Input and pointer handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: dc537981eb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (preferences.encoderPreference !== "software") { | ||
| enableFeatures.push("VideoToolboxVideoEncoder"); |
There was a problem hiding this comment.
When platform === "darwin" and arch === "x64" with encoderPreference: "hardware", this branch adds VideoToolboxVideoEncoder. The same commit adds videoAcceleration.test.ts coverage that asserts this exact Intel Mac path must not include VideoToolboxVideoEncoder, so the checked-in test suite will fail as soon as the new test file is run; either keep the Intel branch decode-only or update the test/expectation.
Useful? React with 👍 / 👎.
| if (rttMs < 20) grade = "Excellent"; | ||
| else if (rttMs < 50) grade = "Good"; | ||
| else if (rttMs < 80) grade = "Fair"; | ||
| else grade = "Poor"; |
There was a problem hiding this comment.
These grade strings are rendered in the stats overlay via formatLatencyGrade, but they are hard-coded instead of coming from locales/en.json, which violates the repository's Localization instruction for user-facing copy. In non-English UI the new HUD labels will remain English and translators cannot update them; return a grade key/enum and translate it in the component, and do the same for the new jitter label.
Useful? React with 👍 / 👎.
| resolution: '1920x1080', | ||
| fps: 60, | ||
| maxBitrateMbps: 120, | ||
| codecHint: 'AV1', |
There was a problem hiding this comment.
When a caller uses pickMacStreamPreset("mac-quality", false) for an Intel Mac, it copies this base preset and returns AV1 because the Apple-Silicon override only runs when isAppleSilicon is true. That contradicts the new Intel codec priority logic that avoids software-only AV1 and would steer Intel Mac quality sessions to the slower codec; keep the base preset hardware-safe, such as H264, and only override to AV1 when AV1 hardware support is known.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@opennow-stable/src/main/videoAcceleration.ts`:
- Around line 69-70: The macOS MJPEG acceleration toggle in
buildVideoAccelerationCommandLine currently forces hardware decode before the
decoderPreference checks, so the software path is ignored. Update the
videoAcceleration.ts logic around enable-accelerated-mjpeg-decode to only enable
it when decoderPreference is not "software", keeping MJPEG aligned with the
existing decoderPreference handling used for the other acceleration switches.
In `@opennow-stable/src/renderer/src/components/StatsOverlay.tsx`:
- Around line 66-74: The new jitter pill in StatsOverlay is hard-coded to
English instead of using the existing localization flow. Update the StatsOverlay
component to render the label through t(...) like the other overlay text, using
the same i18n pattern and a suitable translation key so the jitter label is
localized in all builds.
- Around line 58-64: The RTT pill in StatsOverlay is using color thresholds from
getRttColor() while the label comes from formatLatencyGrade(), so the visual
color can disagree with the displayed grade. Update the RTT rendering logic in
StatsOverlay to use the same grading thresholds for both the Activity icon/span
color and the formatLatencyGrade(stats.rttMs) label, ideally by sharing one
helper or deriving the color from the same grade mapping used by
formatLatencyGrade.
In `@opennow-stable/src/renderer/src/gfn/sdp.ts`:
- Around line 510-513: Replace the direct console diagnostics in
reorderCodecsForPlatform with the shared logger from `@shared/logger` so the
messages go through the logs:export IPC path. Update the logging at
reorderCodecsForPlatform (and the other matching diagnostics around the same
section) to use the project’s logger API instead of console.log, keeping the
same platform/arch/priority context in the message.
- Around line 518-568: The payload-type to codec-family mapping in
reorderCodecsForPlatform is being shared across all video sections, which can
make one m=video section reorder against another section’s dynamic PT
assignments. Refactor the logic in gfn/sdp.ts so each m=video block builds and
uses its own local ptToFamily mapping while rewriting that same section, instead
of collecting mappings in a single first pass across the full SDP. Keep the
section-scoped handling aligned with the existing m=video parsing and payload
reordering code.
In `@opennow-stable/src/renderer/src/utils/streamDiagnosticsFormat.ts`:
- Around line 51-58: The latency formatter currently hard-codes English grade
labels and duplicates the threshold logic already defined in getLatencyGrade(),
which will drift and block localization. Update formatLatencyGrade to rely on
the shared classifier result instead of re-implementing the thresholds, and move
any human-readable grade text to the render layer so it can be translated. Use
the existing getLatencyGrade and formatLatencyGrade symbols to keep the HUD copy
and logic consistent.
In `@opennow-stable/src/renderer/src/utils/streamQualityPresets.ts`:
- Around line 99-104: The mac-quality preset is hardcoding codecHint to AV1
inside getMacStreamPresets, which causes pickMacStreamPreset('mac-quality',
false) to return AV1 even on Intel Macs. Remove the AV1 default from the
mac-quality entry and let pickMacStreamPreset handle the Apple Silicon-only
override logic, keeping the behavior consistent with the other mac presets and
the CodecHint type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd38ba8f-506f-4a29-b37a-38cb700714ed
⛔ Files ignored due to path filters (1)
opennow-stable/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
CHANGELOG_MAC.mdopennow-stable/src/main/videoAcceleration.test.tsopennow-stable/src/main/videoAcceleration.tsopennow-stable/src/renderer/src/components/StatsOverlay.tsxopennow-stable/src/renderer/src/gfn/sdp.tsopennow-stable/src/renderer/src/utils/streamDiagnosticsFormat.tsopennow-stable/src/renderer/src/utils/streamHealthSummary.tsopennow-stable/src/renderer/src/utils/streamQualityPresets.ts
| // Always enable accelerated MJPEG decode on macOS | ||
| switches["enable-accelerated-mjpeg-decode"] = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Respect decoderPreference === "software" for MJPEG too.
enable-accelerated-mjpeg-decode is set before the preference checks, so the "software" path still opts Chromium into a hardware decode path on macOS. That makes buildVideoAccelerationCommandLine() ignore the decoder preference for MJPEG.
Proposed fix
- // Always enable accelerated MJPEG decode on macOS
- switches["enable-accelerated-mjpeg-decode"] = true;
+ if (preferences.decoderPreference !== "software") {
+ switches["enable-accelerated-mjpeg-decode"] = true;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Always enable accelerated MJPEG decode on macOS | |
| switches["enable-accelerated-mjpeg-decode"] = true; | |
| if (preferences.decoderPreference !== "software") { | |
| switches["enable-accelerated-mjpeg-decode"] = true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@opennow-stable/src/main/videoAcceleration.ts` around lines 69 - 70, The macOS
MJPEG acceleration toggle in buildVideoAccelerationCommandLine currently forces
hardware decode before the decoderPreference checks, so the software path is
ignored. Update the videoAcceleration.ts logic around
enable-accelerated-mjpeg-decode to only enable it when decoderPreference is not
"software", keeping MJPEG aligned with the existing decoderPreference handling
used for the other acceleration switches.
| {/* RTT / Latency — shows grade label (Excellent / Good / Fair / Poor) with color */} | ||
| <div className="sovl-pill"> | ||
| <Activity size={13} className="sovl-icon" style={{ color: rttColor }} /> | ||
| <span className="sovl-val" style={{ color: rttColor }}> | ||
| {stats.rttMs > 0 ? `${stats.rttMs.toFixed(0)}ms` : "-- ms"} | ||
| {formatLatencyGrade(stats.rttMs)} | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the RTT color thresholds with the displayed grade.
Lines 58-64 color this pill with getRttColor(), but the label now comes from formatLatencyGrade() using different cutoffs. That yields mixed signals like 40ms · Good in warning yellow and 70ms · Fair in error red. Drive both from the same grading thresholds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@opennow-stable/src/renderer/src/components/StatsOverlay.tsx` around lines 58
- 64, The RTT pill in StatsOverlay is using color thresholds from getRttColor()
while the label comes from formatLatencyGrade(), so the visual color can
disagree with the displayed grade. Update the RTT rendering logic in
StatsOverlay to use the same grading thresholds for both the Activity icon/span
color and the formatLatencyGrade(stats.rttMs) label, ideally by sharing one
helper or deriving the color from the same grade mapping used by
formatLatencyGrade.
| {/* Jitter — color-coded green/yellow/red */} | ||
| {stats.jitterMs > 0 && ( | ||
| <div className="sovl-pill"> | ||
| <span className="sovl-label" style={{ color: jitterColor }}>jitter</span> | ||
| <span className="sovl-val" style={{ color: jitterColor }}> | ||
| {stats.jitterMs.toFixed(1)}ms | ||
| </span> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the new jitter label.
Line 69 hard-codes jitter, while the rest of this overlay already goes through t(...). This pill will stay English in localized builds unless it uses a translation key too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@opennow-stable/src/renderer/src/components/StatsOverlay.tsx` around lines 66
- 74, The new jitter pill in StatsOverlay is hard-coded to English instead of
using the existing localization flow. Update the StatsOverlay component to
render the label through t(...) like the other overlay text, using the same i18n
pattern and a suitable translation key so the jitter label is localized in all
builds.
| console.log( | ||
| `[SDP] reorderCodecsForPlatform: platform=${platform} arch=${arch} ` + | ||
| `priority=[${priorityOrder.join(", ")}]`, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use @shared/logger instead of console.log here.
These diagnostics bypass the shared logging/export path, so they will not flow through the expected logs:export IPC channel. As per coding guidelines, **/*.{ts,tsx}: Use the shared logger (@shared/logger) and export logs via logs:export IPC channel; avoid bypassing this path for diagnostics features.
Also applies to: 567-568
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@opennow-stable/src/renderer/src/gfn/sdp.ts` around lines 510 - 513, Replace
the direct console diagnostics in reorderCodecsForPlatform with the shared
logger from `@shared/logger` so the messages go through the logs:export IPC path.
Update the logging at reorderCodecsForPlatform (and the other matching
diagnostics around the same section) to use the project’s logger API instead of
console.log, keeping the same platform/arch/priority context in the message.
Source: Coding guidelines
| // First pass: collect payload-type → codec-family for the video section | ||
| const ptToFamily = new Map<string, string>(); | ||
| let inVideoSection = false; | ||
| for (const line of lines) { | ||
| if (line.startsWith("m=video")) { | ||
| inVideoSection = true; | ||
| continue; | ||
| } | ||
| if (line.startsWith("m=") && inVideoSection) { | ||
| inVideoSection = false; | ||
| } | ||
| if (!inVideoSection || !line.startsWith("a=rtpmap:")) { | ||
| continue; | ||
| } | ||
| const [, rest = ""] = line.split(":", 2); | ||
| const [pt = "", codecPart = ""] = rest.split(/\s+/, 2); | ||
| const rawName = (codecPart.split("/")[0] ?? "").toUpperCase(); | ||
| const family = CODEC_FAMILY_MAP[rawName]; | ||
| if (pt && family) { | ||
| ptToFamily.set(pt, family); | ||
| } | ||
| } | ||
|
|
||
| // Second pass: rewrite each m=video line's payload type list | ||
| const result = lines.map((line) => { | ||
| if (!line.startsWith("m=video")) { | ||
| return line; | ||
| } | ||
| const parts = line.split(/\s+/); | ||
| const header = parts.slice(0, 3); // "m=video <port> <proto>" | ||
| const payloads = parts.slice(3); | ||
|
|
||
| // Bucket payload types by priority tier | ||
| const buckets: string[][] = priorityOrder.map(() => []); | ||
| const rest: string[] = []; | ||
|
|
||
| for (const pt of payloads) { | ||
| const family = ptToFamily.get(pt); | ||
| const idx = family ? priorityOrder.indexOf(family) : -1; | ||
| if (idx >= 0) { | ||
| buckets[idx]!.push(pt); | ||
| } else { | ||
| rest.push(pt); | ||
| } | ||
| } | ||
|
|
||
| const ordered = [...buckets.flat(), ...rest]; | ||
| const rewritten = [...header, ...ordered].join(" "); | ||
|
|
||
| if (rewritten !== line) { | ||
| console.log(`[SDP] reorderCodecsForPlatform: reordered payload types → [${ordered.join(", ")}]`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope payload-type mappings to each m=video section.
Dynamic RTP payload types are media-section local, but ptToFamily is built once across all video sections. If two video sections reuse the same PT for different codecs, the later section overwrites the earlier mapping and the earlier m=video line gets reordered against the wrong codec family.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@opennow-stable/src/renderer/src/gfn/sdp.ts` around lines 518 - 568, The
payload-type to codec-family mapping in reorderCodecsForPlatform is being shared
across all video sections, which can make one m=video section reorder against
another section’s dynamic PT assignments. Refactor the logic in gfn/sdp.ts so
each m=video block builds and uses its own local ptToFamily mapping while
rewriting that same section, instead of collecting mappings in a single first
pass across the full SDP. Keep the section-scoped handling aligned with the
existing m=video parsing and payload reordering code.
| export function formatLatencyGrade(rttMs: number): string { | ||
| if (rttMs <= 0) return "-- · --"; | ||
| let grade: string; | ||
| if (rttMs < 20) grade = "Excellent"; | ||
| else if (rttMs < 50) grade = "Good"; | ||
| else if (rttMs < 80) grade = "Fair"; | ||
| else grade = "Poor"; | ||
| return `${rttMs.toFixed(0)}ms · ${grade}`; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid hard-coding latency copy in the formatter.
Lines 51-58 return English-only labels and duplicate the grade thresholds that already exist in getLatencyGrade(). That makes the HUD harder to localize and easy to desync on the next threshold change. Prefer formatting from the shared classifier and translating the grade at render time.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@opennow-stable/src/renderer/src/utils/streamDiagnosticsFormat.ts` around
lines 51 - 58, The latency formatter currently hard-codes English grade labels
and duplicates the threshold logic already defined in getLatencyGrade(), which
will drift and block localization. Update formatLatencyGrade to rely on the
shared classifier result instead of re-implementing the thresholds, and move any
human-readable grade text to the render layer so it can be translated. Use the
existing getLatencyGrade and formatLatencyGrade symbols to keep the HUD copy and
logic consistent.
| 'mac-quality': { | ||
| resolution: '1920x1080', | ||
| fps: 60, | ||
| maxBitrateMbps: 120, | ||
| codecHint: 'AV1', | ||
| latencyMode: 'normal', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
mac-quality still forces AV1 on Intel Macs.
getMacStreamPresets() seeds mac-quality with codecHint: 'AV1', so pickMacStreamPreset('mac-quality', false) already returns AV1 before the Apple Silicon override runs. That contradicts the comment and PR goal of making AV1 the Apple Silicon-only override for the higher-quality presets.
Proposed fix
'mac-quality': {
resolution: '1920x1080',
fps: 60,
maxBitrateMbps: 120,
- codecHint: 'AV1',
+ codecHint: 'H264',
latencyMode: 'normal',
},Also applies to: 143-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@opennow-stable/src/renderer/src/utils/streamQualityPresets.ts` around lines
99 - 104, The mac-quality preset is hardcoding codecHint to AV1 inside
getMacStreamPresets, which causes pickMacStreamPreset('mac-quality', false) to
return AV1 even on Intel Macs. Remove the AV1 default from the mac-quality entry
and let pickMacStreamPreset handle the Apple Silicon-only override logic,
keeping the behavior consistent with the other mac presets and the CodecHint
type.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
opennow-stable/src/renderer/src/gfn/sdp.ts (1)
476-508: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the Apple Silicon comments with the implemented priority.
The comments still say arm64 prefers AV1 first, but both branches now use
H264 → H265 → AV1. Update the docs and collapse the duplicate ternary, or restore a distinct arm64 order if that was intended.Proposed cleanup
- * darwin arm64 (Apple Silicon): AV1 → H264 → H265 → others - * (M3+ has native AV1 decode via VideoToolbox; H264 HW always present) - * darwin x86_64 / generic darwin: H264 → H265 → AV1 → others - * (VideoToolbox H264/H265 is fast; AV1 is SW-only on Intel Macs) + * darwin arm64 / x86_64 / generic darwin: H264 → H265 → AV1 → others + * (H264/H265 are preferred; AV1 remains available but lower priority) ... - // Apple Silicon (arm64): AV1 decode is HW-accelerated on M3+ via VideoToolbox. - // Prefer AV1 first, then H264 (always HW), then H265. - // Intel Mac: AV1 decode is software-only; prefer H264 then H265. - const isAppleSilicon = arch === "arm64"; - const priorityOrder: string[] = isAppleSilicon - ? ["H264", "H265", "AV1"] - : ["H264", "H265", "AV1"]; + // Prefer H264/H265 first on macOS; AV1 stays available but lower priority. + const priorityOrder: string[] = ["H264", "H265", "AV1"];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@opennow-stable/src/renderer/src/gfn/sdp.ts` around lines 476 - 508, The comments in reorderCodecsForPlatform are out of sync with the actual codec priority for Apple Silicon, and the priorityOrder ternary currently duplicates the same sequence in both branches. Update the explanatory comments near isAppleSilicon and priorityOrder to match the implemented order, or change the arm64 branch if AV1-first was intended, and simplify the duplicated ternary in reorderCodecsForPlatform so the platform-specific logic is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@opennow-stable/src/renderer/src/gfn/sdp.ts`:
- Around line 476-508: The comments in reorderCodecsForPlatform are out of sync
with the actual codec priority for Apple Silicon, and the priorityOrder ternary
currently duplicates the same sequence in both branches. Update the explanatory
comments near isAppleSilicon and priorityOrder to match the implemented order,
or change the arm64 branch if AV1-first was intended, and simplify the
duplicated ternary in reorderCodecsForPlatform so the platform-specific logic is
clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bafd6537-6326-48f1-a395-ecba4ed63828
📒 Files selected for processing (1)
opennow-stable/src/renderer/src/gfn/sdp.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
opennow-stable/src/renderer/src/components/SettingsPage.tsx (1)
2672-2677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded preset descriptions duplicate
getMacStreamPresets()data.
macPresetMetahardcodes resolution/bitrate/latency strings that already exist ingetMacStreamPresets(). If the preset table changes, these descriptions will silently go stale. Consider deriving the description from the preset object itself.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@opennow-stable/src/renderer/src/components/SettingsPage.tsx` around lines 2672 - 2677, The hardcoded `macPresetMeta` descriptions in `SettingsPage.tsx` duplicate the data already defined by `getMacStreamPresets()`, so update the `macPresetMeta` construction to derive each label/description from the preset object instead of maintaining a separate string table. Use the existing `getMacStreamPresets()` and related `MacStreamQualityPresetId` preset metadata as the single source of truth, and keep `macPresetMeta` in sync by mapping over those preset definitions.opennow-stable/src/renderer/src/gfn/inputProtocol.test.ts (1)
274-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a protocol-v2 text-input regression here.
InputEncoderstill starts on v2, but these assertions only cover wrapped v3 packets. A small v2 case would pin the new 8-byte[type][byteLength]payload on the default path too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@opennow-stable/src/renderer/src/gfn/inputProtocol.test.ts` around lines 274 - 298, Add a protocol-v2 regression test in InputEncoder’s text-input suite to cover the default path before wrapSingleEvent is added. Use InputEncoder.encodeTextInput with protocol version 2 and assert the packet contains only the plain 8-byte payload header ([type][byteLength]) followed by UTF-8 text, using the existing INPUT_TEXT symbol and packet decoding helpers. Keep the new test alongside the current encodeTextInput and unicode chunking cases so both v2 and v3 behavior are pinned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@opennow-stable/src/renderer/src/components/SettingsPage.tsx`:
- Around line 47-50: The Mac preset selection flow is bypassing the codec hint
logic, so selecting a preset only updates resolution/FPS/bitrate and never
applies the expected codec. Update SettingsPage’s preset click handling to use
pickMacStreamPreset(...) instead of relying on getMacStreamPresets() directly,
and make sure the selected preset state includes codecHint along with the other
stream settings. Use the existing MacStreamQualityPresetId and
pickMacStreamPreset symbols to locate the affected selection path and preserve
the Apple Silicon AV1 override.
In `@opennow-stable/src/renderer/src/gfn/webrtcClient.ts`:
- Around line 2917-2925: Keep the macOS pointer-lock behavior consistent across
renderer entry points: the new platform gate in webrtcClient.ts avoids
unadjustedMovement on macOS, but App.tsx still requests pointer lock with raw
input first. Mirror the same isMacOS check in App.tsx’s pointer-lock request
path, or extract the logic into a shared helper used by both App.tsx and
webrtcClient.ts so macOS always falls back to standard pointer lock.
---
Nitpick comments:
In `@opennow-stable/src/renderer/src/components/SettingsPage.tsx`:
- Around line 2672-2677: The hardcoded `macPresetMeta` descriptions in
`SettingsPage.tsx` duplicate the data already defined by
`getMacStreamPresets()`, so update the `macPresetMeta` construction to derive
each label/description from the preset object instead of maintaining a separate
string table. Use the existing `getMacStreamPresets()` and related
`MacStreamQualityPresetId` preset metadata as the single source of truth, and
keep `macPresetMeta` in sync by mapping over those preset definitions.
In `@opennow-stable/src/renderer/src/gfn/inputProtocol.test.ts`:
- Around line 274-298: Add a protocol-v2 regression test in InputEncoder’s
text-input suite to cover the default path before wrapSingleEvent is added. Use
InputEncoder.encodeTextInput with protocol version 2 and assert the packet
contains only the plain 8-byte payload header ([type][byteLength]) followed by
UTF-8 text, using the existing INPUT_TEXT symbol and packet decoding helpers.
Keep the new test alongside the current encodeTextInput and unicode chunking
cases so both v2 and v3 behavior are pinned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 99fabebb-fe44-4f7c-90ac-5ad71eb762d5
📒 Files selected for processing (5)
opennow-stable/src/renderer/src/components/SettingsPage.tsxopennow-stable/src/renderer/src/gfn/inputProtocol.test.tsopennow-stable/src/renderer/src/gfn/inputProtocol.tsopennow-stable/src/renderer/src/gfn/sdp.tsopennow-stable/src/renderer/src/gfn/webrtcClient.ts
| import { | ||
| getMacStreamPresets, | ||
| type MacStreamQualityPresetId, | ||
| } from "../utils/streamQualityPresets"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'opennow-stable/src/renderer/src/components/SettingsPage.tsx' \
'opennow-stable/src/renderer/src/utils/streamQualityPresets.ts' \
'opennow-stable/src/**/streamQualityPresets.ts' \
'opennow-stable/src/**/SettingsPage.tsx'
printf '\n== Search for preset and codec handling ==\n'
rg -n "pickMacStreamPreset|getMacStreamPresets|codecHint|handleCodecChange|settings\.codec|isAppleSilicon|MacStreamQualityPresetId" opennow-stable/src -S
printf '\n== Relevant slices ==\n'
sed -n '1,260p' opennow-stable/src/renderer/src/utils/streamQualityPresets.ts
printf '\n--- SettingsPage.tsx excerpt around Mac presets ---\n'
sed -n '2640,2725p' opennow-stable/src/renderer/src/components/SettingsPage.tsxRepository: OpenCloudGaming/OpenNOW
Length of output: 17025
Apply the Mac preset codec hint on selection. The Mac preset chips only update resolution/FPS/bitrate; codecHint is never applied, so the Quality/Ultra codec promise is ignored. pickMacStreamPreset(...) already contains the Apple Silicon AV1 override, but this component bypasses it by using getMacStreamPresets() directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@opennow-stable/src/renderer/src/components/SettingsPage.tsx` around lines 47
- 50, The Mac preset selection flow is bypassing the codec hint logic, so
selecting a preset only updates resolution/FPS/bitrate and never applies the
expected codec. Update SettingsPage’s preset click handling to use
pickMacStreamPreset(...) instead of relying on getMacStreamPresets() directly,
and make sure the selected preset state includes codecHint along with the other
stream settings. Use the existing MacStreamQualityPresetId and
pickMacStreamPreset symbols to locate the affected selection path and preserve
the Apple Silicon AV1 override.
| // On macOS, unadjustedMovement:true bypasses the OS pointer-acceleration curve, | ||
| // making the in-game cursor feel much slower than the native macOS cursor. | ||
| // Skip raw input on macOS so the system acceleration passes through naturally. | ||
| const isMacOS = navigator.platform.toLowerCase().includes("mac"); | ||
| if (isMacOS) { | ||
| this.log("macOS detected — using standard pointer lock (OS-accelerated) for native cursor feel"); | ||
| await this.requestPointerLockCompat(lockTarget); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the macOS pointer-lock gate aligned across renderer entry points.
opennow-stable/src/renderer/src/App.tsx still requests pointer lock with unadjustedMovement: true first, so macOS users can still land on the raw-input path depending on how capture is triggered. Please mirror this platform check there too, or centralize the logic in one helper.
Suggested follow-up in opennow-stable/src/renderer/src/App.tsx
- await requestPointerLockCompat({ unadjustedMovement: true })
+ const isMacOS = navigator.platform.toLowerCase().includes("mac");
+ await requestPointerLockCompat(
+ isMacOS ? undefined : { unadjustedMovement: true },
+ )
.catch((err: DOMException) => {
- if (err.name === "NotSupportedError") {
+ if (!isMacOS && err.name === "NotSupportedError") {
return requestPointerLockCompat();
}
throw err;
})
.catch(() => {});Also applies to: 2957-2965
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@opennow-stable/src/renderer/src/gfn/webrtcClient.ts` around lines 2917 -
2925, Keep the macOS pointer-lock behavior consistent across renderer entry
points: the new platform gate in webrtcClient.ts avoids unadjustedMovement on
macOS, but App.tsx still requests pointer lock with raw input first. Mirror the
same isMacOS check in App.tsx’s pointer-lock request path, or extract the logic
into a shared helper used by both App.tsx and webrtcClient.ts so macOS always
falls back to standard pointer lock.
…iation, diagnostics ## Changes ### macOS Hardware Acceleration (videoAcceleration.ts) - Add full macOS branch — previously missing entirely - Apple Silicon (arm64): VideoToolbox decode/encode, Metal GPU backend, VP9/HEVC/AV1 VideoToolbox paths, MetalANGLE, IOSurface, CanvasOopRasterization - Intel Mac (x64): VideoToolbox decode/encode, ANGLE GL backend, VP9 support - Add getMacPlatformLabel() helper for Apple Silicon / Intel Mac label - Add comprehensive unit tests for new macOS branch (videoAcceleration.test.ts) ### Smart WebRTC Codec Reordering (sdp.ts) - Add reorderCodecsForPlatform(sdp, platform, arch) function - Apple Silicon: AV1 -> H264 -> H265 (M3+ has native AV1 VideoToolbox HW decode) - Intel Mac: H264 -> H265 -> AV1 (AV1 software-only on Intel) - Reorders without removing codecs for safe SDP negotiation ### Enhanced Stream Diagnostics HUD (StatsOverlay.tsx) - RTT/latency now displays grade: e.g. '18ms · Excellent', '62ms · Fair' - Add jitter pill with color coding (green <5ms, yellow 5-15ms, red >15ms) ### Diagnostic Utilities (streamDiagnosticsFormat.ts, streamHealthSummary.ts) - formatLatencyGrade(): RTT ms with Excellent/Good/Fair/Poor label - getJitterColor(): CSS color var for jitter severity - getLatencyGrade() + LatencyGrade type ### Mac-Optimized Stream Quality Presets (streamQualityPresets.ts) - MacStreamQualityPresetId: mac-performance | mac-balanced | mac-quality | mac-ultra - getMacStreamPresets(): 4 Mac mini presets (720p/45Mbps to 1440p/150Mbps) - pickMacStreamPreset(): AV1 override for Apple Silicon on quality/ultra presets ### Documentation (CHANGELOG_MAC.md) - Full technical changelog of all Mac optimizations
…1 decode failure GFN sends 10-bit AV1 when AV1 is requested first. Electron/Chromium bundled dav1d decoder does not support bit depth 10, causing a black screen with continuous decode errors. Change both darwin branches to H264 -> H265 -> AV1 priority so the server selects H264, which VideoToolbox decodes in hardware correctly. AV1 can be re-promoted once VideoToolbox AV1 is wired into Electrons WebRTC pipeline.
ilovekyrinathemost-maker
force-pushed
the
main
branch
from
8997561 to
bd423cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG_MAC.md`:
- Around line 133-136: The fenced example in the changelog is missing a language
hint, which will keep triggering markdownlint. Update the opening fence for this
example to use a plain text hint, and make sure the fenced block in
CHANGELOG_MAC remains an example block with the correct language tag so the
linter recognizes it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f341c8c-8021-46f1-b6ee-dbb64687fefa
⛔ Files ignored due to path filters (1)
opennow-stable/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
CHANGELOG_MAC.mdopennow-stable/src/main/videoAcceleration.test.tsopennow-stable/src/main/videoAcceleration.tsopennow-stable/src/renderer/src/components/SettingsPage.tsxopennow-stable/src/renderer/src/components/StatsOverlay.tsxopennow-stable/src/renderer/src/gfn/inputProtocol.test.tsopennow-stable/src/renderer/src/gfn/inputProtocol.tsopennow-stable/src/renderer/src/gfn/sdp.tsopennow-stable/src/renderer/src/gfn/webrtcClient.tsopennow-stable/src/renderer/src/utils/streamDiagnosticsFormat.tsopennow-stable/src/renderer/src/utils/streamHealthSummary.tsopennow-stable/src/renderer/src/utils/streamQualityPresets.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- opennow-stable/src/renderer/src/components/StatsOverlay.tsx
- opennow-stable/src/renderer/src/utils/streamHealthSummary.ts
- opennow-stable/src/renderer/src/components/SettingsPage.tsx
- opennow-stable/src/main/videoAcceleration.ts
- opennow-stable/src/renderer/src/gfn/inputProtocol.test.ts
- opennow-stable/src/renderer/src/gfn/webrtcClient.ts
- opennow-stable/src/renderer/src/gfn/inputProtocol.ts
- opennow-stable/src/renderer/src/utils/streamDiagnosticsFormat.ts
- opennow-stable/src/renderer/src/utils/streamQualityPresets.ts
- opennow-stable/src/main/videoAcceleration.test.ts
- opennow-stable/src/renderer/src/gfn/sdp.ts
| ``` | ||
| Before: 18ms | ||
| After: 18ms · Excellent | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language hint to this fenced example.
Markdownlint will keep flagging this block until the opening fence names a language; text is enough here and makes the example clearer.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| Before: 18ms | |
| After: 18ms · Excellent | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 133-133: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG_MAC.md` around lines 133 - 136, The fenced example in the changelog
is missing a language hint, which will keep triggering markdownlint. Update the
opening fence for this example to use a plain text hint, and make sure the
fenced block in CHANGELOG_MAC remains an example block with the correct language
tag so the linter recognizes it.
Source: Linters/SAST tools
1 participant
…iation, diagnostics
Changes
macOS Hardware Acceleration (videoAcceleration.ts)
Smart WebRTC Codec Reordering (sdp.ts)
Enhanced Stream Diagnostics HUD (StatsOverlay.tsx)
Diagnostic Utilities (streamDiagnosticsFormat.ts, streamHealthSummary.ts)
Mac-Optimized Stream Quality Presets (streamQualityPresets.ts)
Documentation (CHANGELOG_MAC.md)
Description
Summary by CodeRabbit