fix(onvif): xaddr fallback - #68
Merged
Merged
Conversation
A Hikvision dome failed every probe with "GetCapabilities: empty SOAP body" — HTTP 200, zero bytes, no fault to explain itself. Three separate causes, each of which produces that same unhelpful result. The client carried no credentials on its handler, so a 401 challenge was never answered. All it sent was a preemptive Basic header, and Hikvision wants Digest for ONVIF; the request was simply unauthorized and the camera declined to say so. Each (host, user) now gets an HttpClient whose handler holds the credentials, which is what lets HttpClient satisfy Basic or Digest as the camera asks. PreAuthenticate stays off so the camera states its terms first, and the preemptive Basic header stays for onvif_simple_server, which enforces Basic at the transport and never challenges. Every request went out as SOAP 1.2 only. Several firmwares are built for 1.1 and answer 1.2 with nothing at all. A response with no usable envelope is now retried once as SOAP 1.1 — different content type, action moved into its own SOAPAction header. A fault counts as an answer, so a camera that says why it refused is not asked twice. And GetStreamUri read Uri as a direct child of the response, which only matches the flatter shape onvif_simple_server sends. The spec nests it as MediaUri/Uri, so a compliant camera looked like it had no stream at all; SetPreset's token is nested the same way on some firmwares. Both now search the response instead of assuming its depth. When both versions come back empty the error names what to check — ONVIF switched off on the camera, or an account without ONVIF rights, which is what an empty body from a working camera almost always means — and the response is logged at debug level. Covered by a stub camera over a real socket, so the client's own HTTP stack does the work: the 401 handshake, the content types and the SOAPAction header are all exercised rather than mocked.
…-sent, authed clients recycled Two holes the review caught, both real. The SOAP 1.1 fallback retried every call whose response was unusable, including SetPreset and RemovePreset. An unusable response does not prove the request was not executed — a camera that ran SetPreset and answered garbage would get a duplicate preset from the resend, and a resend after a successful but unreadable remove would fault on the now-missing preset and report failure for a removal that worked. The dialect a host speaks is now learned once and remembered: the first time a host answers 1.2 with nothing usable and 1.1 with something, the flip is cached and later calls lead with 1.1. Since every authed operation is preceded by the unauthenticated clock probe on first contact, the dialect is already known by the time any mutation goes out. Mutations never cross-dialect retry — they use what the host's reads taught and fail honestly otherwise. The clock-skew retry stays, for mutations too: a fault means the camera refused the request, not that it ran it. And the per-credential HttpClient cache was keyed by host, user and password together with no eviction: every password a camera has ever had kept a live handler — old secret included — for the rest of the process, and two threads missing the cache at once could each construct a client only one of which was ever stored. Keyed by host:port now, since a camera has one credential at a time; a lookup that finds a different credential swaps the entry and disposes the superseded client (a request in flight on it was sent with the old password and failing anyway), and a plain lock replaces GetOrAdd so the losing constructor of a concurrent miss never exists. Growth is bounded by the camera addresses spoken to. Two tests pin the retry behaviour: the dialect is remembered (exactly one 1.2 request ever reaches a 1.1-only host), and a SetPreset whose response is empty is reported as failed after exactly one attempt.
…orts The PTZ surface was a joystick: hold a direction, the camera sweeps, release and it stops. That is right for scanning a scene and wrong for framing one — at full zoom a doorway is a fraction of a degree away, and no amount of care on a hold-to-move control lands on it. This adds the other half. A 3x3 step keypad and two zoom keys send one RelativeMove per press: a single request the camera runs to completion, where a continuous move has to be started and stopped and leaves the camera drifting if the stop is lost. Cameras without RelativeMove fall back to a 350ms self-stopping continuous move, so the button does something everywhere. Home sits in the middle of the pad, where every NVR keypad puts it, with an explicit "set home here" behind a confirmation since overwriting it cannot be undone from the app. None of it is offered blind. GetConfigurationOptions says which move spaces the node implements and what ranges it declares, and GetServiceCapabilities says whether MoveStatus is maintained; the buttons that depend on an operation are hidden on cameras that lack it. A camera that will not describe itself gets the continuous-only profile, which is what this app assumed of every camera until now — so nothing regresses. The ranges matter more than they look. The UI thinks in normalized [-1, 1]; cameras declare degrees, [0, 360], or something asymmetric, and a value outside the declared range is quietly clamped or refused. On an asymmetric range normalized zero maps to the midpoint rather than zero, so a step that touches only pan would also tilt — every press creeping the camera further off. PtzRange does the mapping in one place and PtzRangeTests pins it, that case included. Preset names get a repair on the way in: cameras routinely store UTF-8 and label the response Latin-1 (or Windows-1252, which differs only in 0x80-0x9F and is what a good number of firmwares actually send), so a non-ASCII name arrives as mojibake. The bytes are intact, so recovering them and decoding as UTF-8 gives the name back; anything that is not valid UTF-8 underneath is left alone. The browser gets the same controls. /ptz/step and /ptz/home are stateless like the existing /ptz/move, and /ptz/capabilities lets the pad hide what the camera cannot do. Tapping an arrow there now nudges rather than sweeping — a press under 220ms was never going to move anything useful, so it finishes as the step the user meant. OnvifCoreClient, the superseded WCF path, throws NotSupportedException for the new operations rather than returning a silent no-op: DI resolves SoapOnvifClient, and a camera that ignored its buttons would be worse than an error.
…ns, home from the node, raceless web tap All six review findings were real; five are behaviour fixes pinned by tests, the sixth is a contract fix. Asymmetric ranges. Mapping a step through FromNormalized and then subtracting the midpoint turned a declared [0, 100] into [-50, 50], so negative steps went out below the camera's own minimum — defeating the range handling this feature exists for. PtzRange.ScaleTranslation now scales by the half-span and clamps into the declared bounds: zero stays zero (an untouched axis must not creep) and nothing leaves the range — [0, 100] simply refuses to go negative. FromNormalized is gone. Conflated axes. SupportsRelative came from the pan/tilt space alone, so a pan/tilt-only camera was sent relative zoom translations and a zoom-only camera never used the RelativeMove it supports. The capabilities now carry the four flags the spaces actually declare — relative and continuous, per axis pair, ContinuousZoomVelocitySpace included — and StepAsync decides each axis on its own. Unconditional fallback. A step on an axis with no relative space fell back to ContinuousMove without asking whether a continuous space was declared either. The fallback now runs only where it is, and an axis the camera can serve neither way is dropped rather than sent an operation that must fault. The desktop keypad and the web pad hide keys for such axes; a camera that will not describe itself still gets the continuous-only profile, so nothing regresses. Fabricated home. SupportsHome was hard-coded true for any camera that answered GetConfigurationOptions, with a comment admitting the guess. The node knows: GetConfiguration names it, GetNode reports HomeSupported and FixedHomePosition. Home appears only when the node says so, Set Home additionally requires the position not to be hardware-fixed, and a camera that answers nothing about its node gets no home controls — the web pad keeps its Stop button instead. Web tap race. The pad started the sweep on pointer-down and, on a quick release, fired Stop and the step concurrently — so one press was not reliably one step. On step-capable axes the sweep now starts only after the 220 ms tap window: release inside it sends exactly one step and nothing to stop; a longer press swept, and release sends exactly one stop. Axes without step support keep the immediate hold-to-sweep. Status positions. PtzStatus promised normalized positions while the client returned raw device units. Nothing consumes the positions yet, and normalizing inside the client would mean re-fetching the declared ranges on every poll — so the contract now states the truth: positions are in the camera's own units, and a consumer normalizes through the ranges the capability probe already read (PtzRange.ToUnit). Also from the same pass: RelativeMove is marked non-retryable in the SOAP dialect fallback (a re-send the camera already executed is a double step), and the web /step endpoint seeds PtzController from a per-camera capabilities cache so a step is one SOAP call, not a discovery per press — /capabilities refreshes the entry on every pad mount. New tests: controller steps against a recording fake (asymmetric ranges never leave bounds, untouched axes stay zero, zoom falls back only where continuous zoom is declared, unservable axes send nothing, seeded capabilities skip discovery) and the capability probe against the stub camera (flags come from the declared spaces, fixed home allows goto but not set, a camera that will not describe its node gets no home).
The mojibake repair rewrote any name whose recovered bytes formed valid
UTF-8 — including names that were already right: "©" became "©",
because valid-UTF-8-underneath cannot by itself distinguish damage from
intent.
The discriminator that can: whether the decoded result leaves Latin-1.
A repair that stays inside it ("©" → "©", "Entrée" → "Entrée") proves
nothing, since the original is equally plausible as intentional text —
so the original stands. A result outside it (Cyrillic, Vietnamese,
Greek) could not have been intended as the Latin-1 characters it arrived
as, so those — the names that arrive as pure noise — are still repaired.
The cost is that mojibake whose true text is itself Latin-1 stays as
sent, which is at least legible; rewriting a correct name is not.
…ly ones that leave Latin-1 Field feedback on the leave-Latin-1 rule: it abandons every name whose true text is itself Latin-1. "Entrée" stays mangled, and so does the untoned half of Vietnamese — "Sân" decodes to "Sân", which never leaves Latin-1 either. Three tiers now. A decode that leaves Latin-1 is proof outright. A decode that stays inside but contains a Latin-1 *letter* is proof too: its mangled form is "Ã" chased by a currency sign, a pair nobody types on purpose. Only a decode made purely of Latin-1 symbols — "©" to "©", "°C" to "°C" — is ambiguous, and there the original still stands, which keeps the review's false-positive class protected.
- Probe each candidate with a read-only call (GetProfiles / GetNodes) and cache the first that answers per device endpoint - Candidates: advertised XAddr, same path on the reached host, other advertised XAddrs naming the service, conventional paths, device URI - Fixes YooSee firmwares whose GetCapabilities shifts every XAddr (#67)
# Conflicts: # src/OpenIPC.Viewer.Devices/Onvif/SoapOnvifClient.cs
- Retry a VersionMismatch fault as SOAP 1.1; name a 401/403 as a bad login - Key the learned SOAP dialect by host:port, like the client cache - Candidate probes never flip the dialect; MoveStatus="1" reads as true - Prefer the FOV relative space when a camera declares both
- Explicit Grid cells so a hidden key no longer shuffles the arrows - Stop in the middle on desktop and web; home moves next to "set home" - Continuous fallback step sends an explicit Stop after the step - Capability probe no longer blocks presets and Majestic setup
PR Summary by QodoHarden ONVIF discovery and add capability-aware PTZ controls
AI Description
Diagram
High-Level Assessment
Files changed (25)
|
This was referenced Sep 22, 2026
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
- Cancellation no longer caches a continuous-only capability profile - Web capability cache keyed by endpoint + profile; candidate probes learn the SOAP dialect - Web pad: no sweep on relative-only cameras, one pointer per press, steps serialized, caps reset per camera - Mojibake repair ignores the × ÷ symbols among Latin-1 letters
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
2 participants
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.
Summary
Fixes #67 and supersedes #65 and #66, whose commits are merged here with authorship kept.
VersionMismatchfault is retried as SOAP 1.1, a 401/403 is reported as a bad login, and the learned SOAP version is keyed by host:port.MoveStatus="1"is read as true, and the FOV relative space is preferred.Test plan
Related
Type
Checklist
TreatWarningsAsErrors=true).dotnet test); new Core logic has unit tests.AppreferencesCoreonly (Infrastructure / Video / Devices wired via DI in a head).Platforms tested
Screenshots / notes