Pick the camera player from the camera's reported capabilities - #5686
Pick the camera player from the camera's reported capabilities#5686bgoncal wants to merge 27 commits into
Conversation
The in-app camera player opened a WebRTC offer on every camera and only learned the right stream type by failing its way down a cascade, so cameras the webview plays fine ended up on the still-image proxy. Ask camera/capabilities for frontend_stream_types and build the fallback order from it, the way ha-camera-stream does: no stream support goes straight to MJPEG, an HLS-only camera never sends a doomed offer, a native-WebRTC camera (Nest) knows it has no HLS behind it, and a go2rtc camera prefers WebRTC with HLS as the fallback. The HLS player also read its stream request before the promise resolved, so it always threw "no stream available" and dropped to MJPEG. It now awaits the request, and joins the playlist path without a leading slash so servers on a subpath resolve. The WebRTC path picks up the rest of the frontend player's behaviour: the offer carries the ICE candidates gathered so far, remote tracks are adopted from the peer connection instead of assumed, a failed ICE connection is rebuilt once before cascading, returning to the foreground restarts a stream that did not survive suspension, and an unsupported camera is recognised from the client-config rejection and the signaling error event as well as the offer rejection. getCandidatesUpfront is gone, since core no longer sends it.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core camera streaming selection and WebRTC/HLS connection behavior, and the remaining concurrency/thread-safety issues should be addressed before merge.
Pull request overview
This PR updates the native camera player to choose streaming methods based on the camera’s reported capabilities (camera/capabilities.frontend_stream_types), aligning the app’s behavior with the Home Assistant frontend’s ha-camera-stream selection logic and fixing HLS/WebRTC flow issues that previously caused unnecessary fallbacks to MJPEG.
Changes:
- Introduces
CameraCapabilities,CameraStreamType, andCameraStreamPlanto build an ordered stream fallback plan fromfrontend_stream_types. - Fixes HLS stream URL resolution by properly awaiting the stream request and correctly appending server-relative playlist paths (including subpath installs).
- Improves WebRTC robustness (offer handling, track adoption, ICE failure retry, and lifecycle restart on foregrounding) and adds targeted unit tests for capability decoding and stream planning.
File summaries
| File | Description |
|---|---|
| Tests/App/Cameras/CameraStreamPlanTests.swift | Adds tests validating player ordering derived from reported capabilities (and nil/unknown behavior). |
| Tests/App/Cameras/CameraCapabilitiesTests.swift | Adds tests for decoding camera/capabilities responses and soft-failing fetch behavior. |
| Sources/App/Cameras/CameraPlayer/WebRTC/WebRTCViewPlayerViewModel.swift | Refactors WebRTC setup to use callback-based send, improves unsupported detection, retries, and app lifecycle restart behavior. |
| Sources/App/Cameras/CameraPlayer/WebRTC/WebRTCVideoPlayerView.swift | Hooks into scenePhase to suspend/resume WebRTC intent and restart dead streams. |
| Sources/App/Cameras/CameraPlayer/WebRTC/WebRTCClientConfiguration.swift | Removes getCandidatesUpfront handling and adds data-channel label support. |
| Sources/App/Cameras/CameraPlayer/WebRTC/WebRTCClient.swift | Updates offer semantics, adopts remote tracks dynamically, adds connection liveness helper, and handles receiving callbacks. |
| Sources/App/Cameras/CameraPlayer/CameraStreamType.swift | Adds stream-type enum mirroring core/frontend frontend_stream_types. |
| Sources/App/Cameras/CameraPlayer/CameraStreamPlan.swift | Adds stream-plan builder that selects preferred fallback order ending in MJPEG. |
| Sources/App/Cameras/CameraPlayer/CameraStreamHLSView.swift | Fixes HLS stream request timing and baseURL/subpath-safe playlist URL construction. |
| Sources/App/Cameras/CameraPlayer/CameraPlayerView.swift | Switches from a fixed WebRTC-first cascade to a capabilities-driven player list with guarded fallback advancement. |
| Sources/App/Cameras/CameraPlayer/CameraPlayerType.swift | Adds internal player-type enum used by the capabilities-driven player selection. |
| Sources/App/Cameras/CameraPlayer/CameraCapabilities.swift | Adds capability model + websocket fetch for camera/capabilities. |
| HomeAssistant.xcodeproj/project.pbxproj | Adds a synchronized “Cameras” group to the Xcode project structure. |
Review details
Suppressed comments (2)
Sources/App/Cameras/CameraPlayer/CameraPlayerView.swift:75
- Launching an unscoped
Task { ... }in.onAppearisn't automatically cancelled when the view disappears (e.g., dismiss) and can still update@Stateafterward. Prefer using SwiftUI's.task(id:)modifier so the work is cancelled with the view lifecycle and automatically reruns whencameraEntityIdchanges.
This issue also appears on line 337 of the same file.
.onAppear {
loadMetadata()
loadCameras()
Task { await loadCapabilities() }
}
.statusBarHidden(true)
.persistentSystemOverlays(.hidden)
.preferredColorScheme(.dark)
}
Sources/App/Cameras/CameraPlayer/CameraPlayerView.swift:340
- With
.task(id: cameraEntityId)driving capability loading, this explicitTask { await loadCapabilities() }becomes redundant and can race with the.task-initiated fetch. Removing it avoids duplicate requests and keeps capability loading tied to the view lifecycle.
cameraEntityId = entityId
loadMetadata()
Task { await loadCapabilities() }
}
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Found 2 unused localization strings in the codebase. Click to see detailsTo remove them, run the |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (65.74%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #5686 +/- ##
==========================================
+ Coverage 41.59% 42.25% +0.65%
==========================================
Files 1108 1117 +9
Lines 77099 77825 +726
==========================================
+ Hits 32073 32887 +814
+ Misses 45026 44938 -88
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Test coverage of this pull request is below 90%
The unit tests run 35.13% of the lines this pull request
adds or changes (98 of 279 coverable lines).
The per-file breakdown, and the changed lines no test runs, are in the
job summary. Adding tests for those lines and pushing dismisses this
review automatically.
Lines that carry no executable code, and files the unit test targets do not
build, are not counted. If the new code genuinely cannot be unit tested, a
maintainer can dismiss this review.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
A stream that dropped to ICE disconnected was left frozen: only failed was handled, and moving between cellular and Wi-Fi lands on disconnected and never recovers on its own. Device logs show a camera connecting over 5G in about nine seconds, playing for twenty-odd, then sitting disconnected until the player was closed and reopened. Give a dropped connection a few seconds to mend itself — WebRTC re-checks its candidates and often does — and rebuild the stream when it does not. Reaching a connected state again resets the attempts, so an interruption it recovered from does not count against a later one. The rebuild also re-arms the connection timeout, which the first rendered frame had cancelled; without it a retry that never connects leaves the loader spinning with nothing to cascade it onwards. The timeout itself goes to 25s, since gathering and checking relay candidates over cellular takes the better part of ten seconds on a healthy link.
On cellular the peer connection gathered on every interface the phone has — several private pdp_ip addresses, the 464XLAT address, link-local and unique-local IPv6, each as UDP and TCP — and offered close to fifty candidates for a camera reachable over one of them. Connectivity checks then worked through the unreachable pairs first: device logs show nine seconds of that before the relay pair won, on top of the round trip for the answer. Drop the ones that could never carry the stream (TCP host candidates on private addresses, link-local interfaces), prune redundant TURN ports, and pre-gather one candidate so the offer carries it instead of the backend waiting on the first trickled one. TURN over TCP is unaffected — it comes from the ICE server list, not from host candidate gathering.
Twelve releases on from the pinned 140, tracking the same upstream libwebrtc the frontend's browser is built from.
WebRTCClientTests hung the unit test bundle: the suite started and produced no result before the job hit its sixty minute timeout, twice. Constructing an RTCPeerConnection opens sockets and starts gathering, which does not complete on a CI runner, and nothing else in the suite had ever done it. Keep the tests that drive the view model through its mock connection and drop the ones that stand up real WebRTC objects. The peer connection wrapper goes back to being covered by the app rather than by tests.
There was a problem hiding this comment.
Test coverage of this pull request is below 90%
The unit tests run 37.25% of the lines this pull request
adds or changes (130 of 349 coverable lines).
The per-file breakdown, and the changed lines no test runs, are in the
job summary. Adding tests for those lines and pushing dismisses this
review automatically.
Lines that carry no executable code, and files the unit test targets do not
build, are not counted. If the new code genuinely cannot be unit tested, a
maintainer can dismiss this review.
Pruning redundant TURN ports cut the relay candidates offered on cellular from three to one. Behind carrier-grade NAT that is the only kind of candidate that can carry the stream — the host addresses are private and the reflexive one is not reachable inbound — so each relay candidate is a separate chance for the stream to come up rather than redundancy worth removing. Device logs show two consecutive attempts with an identical single-relay candidate set, one connecting in nine seconds and the other never connecting at all. Dropping TCP host candidates and link-local interfaces stays: those can never carry the stream, and they are what the candidate set needed trimming of.
AVFoundation does its own networking outside URLSession delegates, so it never presents the client certificate or answers the trust challenge a security exception covers. On a server using either, the HLS fallback could not load at all. Route the asset's requests through the app's session, the same way the notification extension's player already does.
WebRTC signaling runs over the server's WebSocket. When the network changes under it, that socket is dead but HAKit needs the better part of a minute to notice, and commands sent meanwhile get no reply and no error. The player rebuilt its stream five seconds after ICE dropped, sent camera/webrtc/get_client_config into that socket, and then sat until its own 25s timeout gave up on WebRTC entirely. Hold the attempt until the connection can carry it, reconnecting an idle socket rather than waiting on one nothing will bring back, and start the stream's timeout when the server is reachable instead of at open. A stream now survives moving between Wi-Fi and cellular. The wait is bounded so an unreachable server still reports a failure.
Patch coverage could not be measured on this run, so the gate no longer applies.
There was a problem hiding this comment.
Test coverage of this pull request is below 90%
The unit tests run 47.64% of the lines this pull request
adds or changes (242 of 508 coverable lines).
The per-file breakdown, and the changed lines no test runs, are in the
job summary. Adding tests for those lines and pushing dismisses this
review automatically.
Lines that carry no executable code, and files the unit test targets do not
build, are not counted. If the new code genuinely cannot be unit tested, a
maintainer can dismiss this review.
A socket whose network was taken away still reports itself ready: nothing has tried to use it since, and HAKit needs some forty-five seconds to find out. Device logs caught the race exactly — Wi-Fi went five milliseconds after the connection reported ready, and the rebuilt stream's get_client_config went out over it and was never answered, where the same command takes 43ms on a working connection. Watch a signaling command for that silence and start over when it happens, waiting for a connection that has actually been re-established rather than believing the stale one. Cascading instead only hands the next player the same dead socket.
Patch coverage could not be measured on this run, so the gate no longer applies.
Every other test that builds an API swaps in a mock connection. These did not, so the API kept its real one and went looking for a server that is not there, leaving retries running behind the tests that follow.
There was a problem hiding this comment.
Test coverage of this pull request is below 90%
The unit tests run 51.08% of the lines this pull request
adds or changes (283 of 554 coverable lines).
The per-file breakdown, and the changed lines no test runs, are in the
job summary. Adding tests for those lines and pushing dismisses this
review automatically.
Lines that carry no executable code, and files the unit test targets do not
build, are not counted. If the new code genuinely cannot be unit tested, a
maintainer can dismiss this review.
… uses On cellular the peer connection still gathered on every pdp_ip interface the phone has, because libwebrtc's iOS network monitor is behind the WebRTC-Network-UseNWPathMonitor field trial and without it the network manager takes whatever getifaddrs lists. Only one of those interfaces can reach anything; the rest produce host candidates that never connect, and the connectivity checks work through every one of those pairs before they reach the relay pair that can. That is the stretch of seconds a 5G stream spent before connecting, when it connected at all, while the frontend in the webview connected at once: WKWebView only gathers on the default route. Turn the trial on before the peer connection factory is created. With the path monitor installed, libwebrtc reports interfaces outside the current network path as unavailable and ignores them, leaving the same interface set the webview's player negotiates from. Also record the WebRTC 152.0.0 pin in Package.resolved, which the project file already required.
Patch coverage could not be measured on this run, so the gate no longer applies.
…scription per reconnect The camera picker fetched a still for every camera the moment the player opened, and a camera the server cannot image answered each of those with a 500 on every open. Snapshots are now fetched when the picker menu is shown, kept in a process-wide cache, and a failure is remembered for five minutes so the same camera is not asked again on the next open. A snapshot response that is not an image now rejects instead of leaving the promise pending forever. Every reconnect made the model manager tear down and rebuild its zone and person subscription, because HAKit reports the server changed when the auth handshake updates its version. Device logs show the resulting pair of subscribe_entities plus an unsubscribe of the first on each reconnect, and HAKit complaining about the result and event arriving for the request it had already dropped. The subscription is only rebuilt when the set of subscribable servers changes. Bundle audio and video onto one transport from the start (max-bundle), so the peer connection gathers and checks one candidate set instead of one per m-line; every WebRTC answerer bundles, so nothing is lost.
There was a problem hiding this comment.
Test coverage of this pull request is below 90%
The unit tests run 49.61% of the lines this pull request
adds or changes (315 of 635 coverable lines).
The per-file breakdown, and the changed lines no test runs, are in the
job summary. Adding tests for those lines and pushing dismisses this
review automatically.
Lines that carry no executable code, and files the unit test targets do not
build, are not counted. If the new code genuinely cannot be unit tested, a
maintainer can dismiss this review.
Pull request was converted to draft
The paths after the client configuration — sending the offer, flushing candidates once the session arrives, applying the answer and remote candidates, the error event, rebuilding on a failed or stalled connection, the first-frame and connection-wait timeouts, and restarting after a dead background — only ran against a real RTCPeerConnection, which the test bundle cannot stand up on CI. The view model now takes the client through a small protocol and its timings through a struct, with the production values as the defaults, and a debug-only fake client drives those paths in tests. Also covers unsubscribe on the model manager.
Patch coverage could not be measured on this run, so the gate no longer applies.
A PR run that misses its DerivedData key restores the newest cache main warmed, and with it the Clang modules precompiled for main's binary dependencies. This PR moves the WebRTC pin, so the checked-out headers are newer than the module built from the old ones, and Xcode fails the build with "has been modified since the module file was built" rather than rebuilding it. Drop the explicit precompiled modules after a fallback restore so they are built against the headers actually present.
HAKit posts state transitions asynchronously and without the state they announce, so a socket that drops and is back within one turn of the main queue delivers two notifications that both read ready on arrival. The gate waited for a reading that was not ready before trusting ready again, and so never opened. It was armed while the state read ready, which makes any transition since the evidence it needs.
There was a problem hiding this comment.
Test coverage of this pull request is below 90%
The unit tests run 66.33% of the lines this pull request
adds or changes (461 of 695 coverable lines).
The per-file breakdown, and the changed lines no test runs, are in the
job summary. Adding tests for those lines and pushing dismisses this
review automatically.
Lines that carry no executable code, and files the unit test targets do not
build, are not counted. If the new code genuinely cannot be unit tested, a
maintainer can dismiss this review.
Patch coverage could not be measured on this run, so the gate no longer applies.
There was a problem hiding this comment.
Test coverage of this pull request is below 90%
The unit tests run 67.05% of the lines this pull request
adds or changes (466 of 695 coverable lines).
The per-file breakdown, and the changed lines no test runs, are in the
job summary. Adding tests for those lines and pushing dismisses this
review automatically.
Lines that carry no executable code, and files the unit test targets do not
build, are not counted. If the new code genuinely cannot be unit tested, a
maintainer can dismiss this review.
AI Policy
Select exactly one option that describes AI usage in this contribution:
Summary
The camera player could not play every camera the webview can. It opened a WebRTC offer on every camera and only learned the right stream type by failing its way down a cascade, and the HLS step of that cascade never worked, so cameras WebRTC could not carry ended up on the still-image proxy.
It now asks
camera/capabilitiesforfrontend_stream_typesand builds the fallback order from it, the same way the frontend'sha-camera-streamdoes:hlsonly → HLS, then MJPEGweb_rtconly (Nest and friends, which have no HLS) → WebRTC, then MJPEGThe HLS player read its stream request before the promise resolved, so it always threw "no stream available". It now awaits the request, and joins the playlist path without a leading slash so servers on a subpath resolve.
The WebRTC path picks up the rest of the frontend player's behaviour: the offer carries the ICE candidates gathered so far, remote tracks are adopted from the peer connection instead of assumed to be on the transceiver the offer created, a failed ICE connection is rebuilt once before cascading, returning to the foreground restarts a stream that did not survive suspension, and an unsupported camera is recognised from the client-config rejection and the signaling error event as well as the offer rejection.
getCandidatesUpfrontis removed, since core no longer sends it.On cellular the native player also gathered ICE candidates on every
pdp_ipinterface the phone has, because libwebrtc's iOS network monitor sits behind theWebRTC-Network-UseNWPathMonitorfield trial and without it the network manager takes whatevergetifaddrslists. Only one of those interfaces can reach anything; the others produce host candidates that never connect, and the connectivity checks work through all of those pairs before reaching the relay pair that can — the seconds a 5G stream spent before connecting, when it connected at all. The frontend never had this problem because WKWebView only gathers on the default route. The trial is now enabled before the peer connection factory is created, so libwebrtc ignores interfaces outside the current network path and negotiates from the same interface set the webview does.Smaller things the device logs turned up along the way: the picker fetched a still for every camera on each open and a camera the server cannot image failed each time, so snapshots are now fetched when the picker is shown and a failure is remembered for five minutes; a snapshot response that is not an image rejected nothing and left its promise pending; the model manager rebuilt its zone/person subscription on every reconnect because the auth handshake's version update counts as a server change, which cost a duplicate
subscribe_entitiesand an unsubscribe per reconnect; and the peer connection now bundles audio and video from the start (maxBundle) so it gathers one candidate set instead of one per m-line.Screenshots
No visual change — same player UI, chosen differently.
Link to pull request in Documentation repository
Documentation: not needed, no user-facing behaviour is added.
Any other notes
One gap this cannot close: the bundled WebRTC build ships no HEVC decoder for Apple platforms, while WKWebView gets H.265 from WebKit. An H.265 camera still plays in the webview and not in the native player — but it now times out and falls to HLS, where AVPlayer decodes it natively.
Generated by Claude Code