Auto-downgrade to MJPEG when VideoToolbox can't encode H.264#105
Open
EvanBacon wants to merge 3 commits into
Open
Auto-downgrade to MJPEG when VideoToolbox can't encode H.264#105EvanBacon wants to merge 3 commits into
EvanBacon wants to merge 3 commits into
Conversation
…e H.264 Virtualized macOS hosts (cloud Macs, CI VMs) accept VTCompressionSessionCreate but never produce H.264 output, so the AVCC stream would sit on a frozen seed until the client's no-frame timeout (~4s) fired. The helper now probes its own encoder at startup and tells viewers to use MJPEG up front — so `serve-sim` on a cloud Mac "just works" without needing `--codec mjpeg`. - Helper: feed a few captured frames through VideoToolbox at startup, independent of any AVCC viewer; first encoded output ⇒ H.264 OK, no output within 3s ⇒ unsupported. Result surfaced via /config (`h264Supported`). - New AVCC `0x05` downgrade envelope: sent to an AVCC viewer on connect (and broadcast to any already-connected viewers) when the encoder is known dead. - Client demuxer decodes the downgrade chunk and routes it through the existing onDecoderError → MJPEG fallback, so the switch is immediate instead of a 4s stall. The no-frame timeout remains as a backstop. `--codec` stays an explicit override. Verified on bare metal: probe reports h264Supported:true and the stream stays on H.264. VM (false) path still needs on-device verification. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new ChangesAVCC H.264 downgrade signal
Sequence Diagram(s)sequenceDiagram
rect rgba(99, 102, 241, 0.5)
Note over main.swift,h264Encoder: Startup H.264 probe
end
participant main.swift
participant h264Encoder
participant ClientManager
participant AVCCClient as AVCC Client (TypeScript)
main.swift->>h264Encoder: encode first frame (forced keyframe)
alt H.264 output received within timeout
h264Encoder-->>main.swift: onEncoded callback
main.swift->>ClientManager: setH264Supported(true)
else Timeout, no output
main.swift->>ClientManager: setH264Supported(false)
ClientManager->>AVCCClient: broadcast downgrade envelope (tag 0x05)
end
AVCCClient->>AVCCClient: AvccDemuxer parses tag 0x05 → "downgrade"
AVCCClient->>AVCCClient: useAvccStream reportDecodeFailure("server: H.264 encode unavailable")
AVCCClient->>AVCCClient: fall back to MJPEG
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@packages/serve-sim/Sources/SimStreamHelper/main.swift`:
- Around line 65-69: The h264Decided variable and related probe state (h264OK,
h264ProbeStart) are accessed both inside and outside the h264Queue, creating a
data race where line 193 reads h264Decided outside the queue while probe
operations write to it inside the queue. Move all reads of h264Decided, h264OK,
and h264ProbeStart to occur within the h264Queue synchronization context to
ensure thread-safe access. Additionally, relocate the timeout check (comparing
current time against h264ProbeStart and h264ProbeTimeout) to execute before the
h264Encoding backpressure condition is evaluated, so the downgrade decision is
not blocked by encoding backpressure.
🪄 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: f4cffe8d-c7f3-47d2-aa35-6cb42cc0d188
📒 Files selected for processing (7)
packages/serve-sim-client/src/__tests__/avcc-codec.test.tspackages/serve-sim-client/src/avcc-codec.tspackages/serve-sim-client/src/simulator/use-avcc-stream.tspackages/serve-sim/Sources/SimStreamHelper/ClientManager.swiftpackages/serve-sim/Sources/SimStreamHelper/StreamFormat.swiftpackages/serve-sim/Sources/SimStreamHelper/main.swiftpackages/serve-sim/bin/serve-sim-bin
On a virtualized runner the helper's startup probe finds no VideoToolbox H.264 and emits the new 0x05 downgrade envelope, so the AVCC endpoint test now sees tags [seed, downgrade]. Recognize that as the designed H.264-unavailable outcome (assert valid framing + no keyframe, break early) instead of failing on an unknown tag or waiting out the 30s soft-pass budget. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 `@packages/serve-sim/src/__tests__/avcc-stream-endpoint.test.ts`:
- Around line 113-116: The test prematurely exits when TAG_DOWNGRADE is observed
by breaking immediately, which means it doesn't verify that keyframes don't
appear after the downgrade signal is received. Instead of breaking immediately
upon seeing TAG_DOWNGRADE, modify the logic to continue reading from the stream
to check that no keyframes appear post-downgrade, ensuring the "downgrade stream
does not contain keyframes" guarantee is actually verified rather than assumed.
🪄 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: b457675c-2e67-4b6a-af19-582320e64b78
📒 Files selected for processing (1)
packages/serve-sim/src/__tests__/avcc-stream-endpoint.test.ts
- main.swift: confine all H.264 probe/backpressure state (h264Decided, h264OK, h264ProbeStart, h264Encoding, forceKeyframe) to h264Queue. The frame handler previously read h264Decided from the capture thread while the queue wrote it — a data race. The gating decision now runs inside the queue, and the probe timeout is evaluated before the backpressure early-return so a wedged encode can't suppress the downgrade decision. - avcc-stream-endpoint.test.ts: don't break the read loop immediately on the downgrade tag; keep reading for a 300ms grace window so a keyframe emitted after downgrade (a protocol violation) is actually observed before asserting none exist. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Virtualized macOS hosts (cloud Macs like Namespace, CI VMs) accept
VTCompressionSessionCreatebut never actually produce H.264 output. Today the AVCC stream paints its JPEG seed and then sits frozen until the client's no-frame timeout (~4s) downgrades to MJPEG — a poor first-load experience, and the reason cloud users have to know to pass--codec mjpeg.This makes
serve-simon a cloud Mac just work: the helper detects its own encoder's capability and tells viewers to use MJPEG up front.How
/configash264Supported.0x05downgrade envelope: sent to an AVCC viewer on connect (and broadcast to any already-connected viewers) when the encoder is known dead. Empty payload; tag kept in sync between SwiftAVCCEnvelopeand the TSavcc-codec.onDecoderError → MJPEGfallback, so the switch is immediate. The 4s no-frame timeout stays as a backstop.--codecremains an explicit override; this only changes the default behavior on hosts that can't encode H.264.Verification
/config→{"h264Supported":true,...}, log[h264] probe: VideoToolbox H.264 encode OK, stream stays on H.264. ✅falsepath (cloud Mac → MJPEG) still needs on-device verification; the existing client fallback de-risks it.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
0x05) to detect when H.264 encoding is unavailable and promptly signal a fallback to MJPEG.Tests