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.
PR Summary by QodoAdd capability-driven PTZ steps, home, and ONVIF interoperability
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
iBinh
force-pushed
the
feat/ptz-steps-and-home
branch
from
August 25, 2026 08:50
89bdb92 to
792c032
Compare
…-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.
iBinh
force-pushed
the
feat/ptz-steps-and-home
branch
from
August 25, 2026 09:07
792c032 to
afb124b
Compare
…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).
iBinh
force-pushed
the
feat/ptz-steps-and-home
branch
from
August 25, 2026 09:08
afb124b to
5d0dcc3
Compare
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.
keyldev
self-requested a review
August 27, 2026 16:46
Collaborator
|
Stacked on #65, so #65 lands first. Verified locally: The design is right. Capabilities are read per axis pair rather than as one flag, 1. Blocker — hiding the Home button scrambles the keypad — verified
Nine buttons sit in a
This fires on the default path: 2. The continuous step fallback never sends a stop
The fallback sends one 3. The web pad trades away its Stop button
Home is rendered instead of Stop when Worth a look
|
keyldev
added a commit
that referenced
this pull request
Sep 22, 2026
keyldev
added a commit
that referenced
this pull request
Sep 22, 2026
- 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
11 of 20 tasks
Collaborator
|
Thanks a lot for this — the work is merged, with your commits and authorship kept, into #68 . The review items are fixed on top there, so I'm closing this one in favour of it. |
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
The PTZ surface is 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, one
RelativeMoveper press. RelativeMove is the right operation for a step — asingle 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 it fall back to a 350 ms self-stopping continuous move, so the button
does something everywhere. Home sits in the middle of the pad, where every NVR
keypad puts it; "set home here" is behind a confirmation, since overwriting it
cannot be undone from the app.
Nothing is offered blind.
GetConfigurationOptionsreports which movespaces the node implements and what ranges it declares;
GetServiceCapabilitiesreports whether
MoveStatusis maintained. Buttons that depend on an operationare hidden on cameras that lack it, and 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 avalue outside the declared range is quietly clamped or refused. On an asymmetric
range, normalized zero maps to the range midpoint rather than zero — so a step
touching only pan would also tilt, creeping the camera further off on every
press.
PtzRangedoes the mapping in one place andPtzRangeTestspins it,that case included.
Preset names are repaired on the way in. Cameras routinely store UTF-8 and
label the response Latin-1 — or Windows-1252, which differs only in
0x80-0x9Fand 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 not valid UTF-8 underneath is left alone.
The browser gets the same controls.
/ptz/stepand/ptz/homearestateless like the existing
/ptz/move, and/ptz/capabilitieslets the padhide what the camera cannot do. Tapping an arrow now nudges rather than sweeps:
a press under 220 ms was never going to move anything useful, so it finishes as
the step the user meant.
OnvifCoreClient, the superseded WCF path, throwsNotSupportedExceptionforthe new operations rather than returning a silent no-op — DI resolves
SoapOnvifClient, and a camera that ignored its buttons would be worse than anerror. Deleting that class felt out of scope for this PR; happy to do it in a
follow-up if you want it gone.
Related
Phase 4 — ONVIF + PTZ (the web endpoints mirror the PTZ surface added in
Phase 21). No issue.
Type
Checklist
TreatWarningsAsErrors=true). Desktopsolution and the Android head both clean; the iOS head was not built here
(workload not installed on this machine) — left to CI. The React console
builds too (
npm run build).dotnet test); new Core logic has unit tests. 16 new testsover
PtzRangeandOnvifText; suites green at Core 331 / Devices 11 /Video 11.
AppusesPtzController,PtzCapabilitiesandPtzVelocity, all of which live inCore. No new references.keypad and says the controls are read from the camera.
Platforms tested
Built and tested on macOS; the Android head builds clean. No head was actually
run — see the note below.
Screenshots / notes
No screenshots, and that is a real gap in this PR. I have no display to run
the desktop head on, so the keypad has never been seen rendered. Its layout is a
3x3
UniformGridinside the existing PTZ panel, above the presets list, withthe zoom pair, a speed slider and the "set home" button stacked beneath it.
Bindings are compile-checked (
AvaloniaUseCompiledBindingsByDefault), so thecommands and properties resolve, but spacing and fit are unverified. If the
layout is wrong I would rather hear it than have it merged.
Also unverified: a real PTZ camera. The SOAP bodies follow the spec and the
capability parsing and range arithmetic are covered by unit tests, but no move
in this PR has been sent to actual hardware. The fallback path is deliberately
conservative — a camera that answers nothing keeps exactly the behaviour it has
on
main.