CV610 backend: sidecar, orientation, ROI, live retarget, AE ceilings, and PQTools .bin import/export - #120
Conversation
src/rtp_sidecar.c sits in HELPER_SRC and in the host test suite but was never in CV610_SRC, so a CV610 craft emitted no per-frame metadata at all while outgoing.sidecarPort was parsed, range-validated and persisted like any other field. Nothing errored; the port accepted a value and did nothing. The cost was downstream and silent. The ground station's attitude consumer takes the sidecar ATTITUDE trailer as its only input and, on a board with no flight controller, has no MSP fallback either, so it is compiled out of the CV610 build entirely -- as is the link-log probe that binds through MSG_SUBSCRIBE. The ground pipeline-stats consumer had no clock-sync or end-to-end latency source for such a craft, and tools/rtp_timing_probe had nothing to read, which is why CV610's VPSS latency has stayed unquantified: the instrument was missing, not the bench time. No wire change. CV610 emits the same FRAME the SigmaStar backends do, so every existing consumer and probe works against it unchanged. - ENC_INFO carries frame_size_bytes, frame_type and idr_inserted. qp, complexity and scene_change stay 0: no per-frame QP readback and no scene detector on this backend, and a fabricated value a consumer would trend is worse than a zero that means "not produced". - Under frame-shm:// the RTP identifiers are 0. rtp_session_init() is skipped for a non-RTP transport, which is pre-existing Star6E behaviour (star6e_video.c gates it on star6e_output_is_rtp()), not new here. No consumer reads those fields on that path; the trailer carries the ring state instead. - cv610_collect_transport() now backs both /api/v1/transport/status and the per-frame trailer so the two cannot drift. The drain loop takes the ring reading once and hands it to both the low-water window and the trailer, and only when a probe is subscribed -- an unsubscribed craft pays no clock read, no ring load and no SIOCOUTQ ioctl. - rtp_sidecar_sender_init() runs before the outgoing.enabled bail, on every path, because it is what writes -1 into fd. The context arrives memset to zero and fd 0 is a valid descriptor, so an init after an early return would leave the per-frame gate polling stdin. Also fixes a Makefile defect this change surfaced: include/venc_api.h was missing from the test runner's prerequisites, so bumping VENC_CONTRACT_VERSION did not rebuild the binary the contract-drift test runs from -- a macro-only bump would have passed on a stale artifact. Mutation-verified: with the header listed, a deliberate 0.99.0 now fails three checks; without it, nothing rebuilt. VERSION 0.73.3 -> 0.74.0, contract_version 0.22.0 -> 0.23.0 (additive; one per-backend supported flag changes). Host suite 2810 passed / 0 failed. Cross-builds clean on cv610, star6e and maruko. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
…pture_us Device verification on .181 caught this: capture_us landed ~4.63 s AFTER frame_ready_us on every frame, so the probe's encode-duration derivation went negative and printed "-" on all 665 sampled rows. The field looked populated and could not be trended -- exactly the failure the ENC_INFO zeros in the previous commit exist to avoid. The sidecar contract says capture_us is CLOCK_MONOTONIC microseconds. The encoder's PTS is on the MPP timebase, which on this SoC sits seconds away from it; Star6E gets away with passing its packet timestamp straight through because the MI SDK's stamps are already on that base. cv610_capture_us_from_pts() samples ss_mpi_sys_get_cur_pts() against wb_monotonic_us() once, lazily, on the first frame that needs it -- MI_SYS need not be up when the output starts, so a sample taken at cv610_output_start() would be unreliable. Both clocks are read microseconds apart, which is noise against a seconds-scale offset. While the epoch is unknown it returns 0, the contract's "not available", rather than a number on the wrong base. Device-verified on .181, 720p100 H.265 CBR: - ordering correct on 604/604 sampled frames (was 0/665) - capture-to-encode-complete resolves to a stable ~24.3 ms mean (23.9-26.5 ms), which is the measurement the CV610 VPSS-latency question has been waiting on - confirmed on both egress paths; under frame-shm:// the wire bytes show capture_us and frame_ready_us 25.6 ms apart and correctly ordered Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
… claim The previous commit documented "no per-frame QP readback" on CV610 and left ENC_INFO.qp at 0. That was wrong: ot_venc_h265_stream_info exposes start_qp and mean_qp, and the struct was already in hand -- the SVC-T enhance check two lines earlier reads h265_info.ref_type from it. The contract asks for "start QP / closest available per-frame QP", which start_qp is exactly. Shipping a promised field as a zero justified by a false statement about the hardware is worse than not shipping it, so both the field and the comment are fixed. frames_since_idr is likewise trackable and now tracked. The counter advances outside the sidecar's subscribed-only gate on purpose: one that only ran while a probe was attached would report frames since the subscription, not since the IDR. Still deliberately 0, with the reason corrected: complexity and scene_change come from the shared scene detector and gop_state from the shared GOP controller, neither of which this backend compiles in. That is a real absence, not a readback CV610 lacks. Device-verified on .181, 720p100 H.265 CBR: - qp 31-38, mean 32.6, across 783 frames - frames_since_idr climbs under resilience=racing (which emits no periodic IDR) and resets 4922 -> 0 exactly on /request/idr, with that frame typed IDR at 22321 B against ~2000 B for its neighbours Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
Three defects an adversarial review found in the first cut, all of them divergences from what the other two backends put on the same wire. 1. RTP identifiers under frame-shm://. The first cut seeded the RTP session only on the socket path, justified by the claim that Star6E skips it for non-RTP transports. That claim was false: star6e_output_is_rtp() tests the STREAM MODE (rtp vs compact, default rtp), not the transport, so a Star6E craft on frame-shm:// does seed and does put a non-zero ssrc/rtp_timestamp/seq_first on the sidecar wire. CV610 was the only backend sending a zero ssrc there, which a consumer keying on "ssrc != 0 means session present" reads as a different craft state. Now seeded for every transport; nothing but the sidecar reads ctx->rtp off the socket path. seq_count stays 0 on the ring transports everywhere -- the packetizer never runs. 2. in_pressure and pressure_drops. rtp_sidecar.h defines in_pressure as the 75/50 HYSTERESIS flag, and pressure_drops -- despite the name, kept for ABI stability across the v0.9.2 frame-skip rollback -- as "frames observed in pressure", not a drop count on any backend. The first cut published a bare fill>=75 threshold and a hardcoded 0, so a consumer trending "fraction of frames in pressure" got a different answer per backend for identical ring behaviour. Both now come from the shared venc_observe_pressure() helper, maintained on the producer thread every frame so the hysteresis sees every sample. /api/v1/transport/status reports the same latched values, which is what the shared collector was for -- it was still printing a literal 0. 3. The socket was only serviced alongside a frame. rtp_sidecar_poll() sat inside the "a frame was produced" branch, so every continue in the drain loop skipped it. A probe attaching while the encoder is stalled saw a silent port -- indistinguishable from a build with no sidecar, the exact condition this work exists to remove. It now runs right after select(), ahead of every continue, with errno saved across it so the EINTR test still sees select's value. Also: widen the sendto error filter in the shared sender. errno != EAGAIN was too narrow -- a link drop yields ENETUNREACH/ENETDOWN synchronously every frame for the whole 5 s subscriber TTL, and that fprintf runs between get_stream and release_stream on the encode drain loop. cv610_output_write already treats the same errnos as routine on the video path. Affects all three backends: strictly fewer log lines, no delivery change. And correct two doc claims: the frame-shm identifier claim above, and gop_state -- it is 0 on all three backends because nothing in the tree writes it, not because CV610 lacks a controller the others run. Device-verified on .181 (720p100): - frame-shm: ssrc 0x5409479f, rtp_ts and seq_first non-zero, seq_count 0 - pressure flag latched, counter advanced 824 over 825 frames on a deliberately unconsumed ring, HTTP and wire agreeing - normal UDP path unchanged: 402 rows, qp populated, sync converged, TTL refresh across a 14 s run Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
…lves The adversarial pass that produced the previous commit also reviewed the in-flight delta and found three problems with it. All three are real. 1. cv610_collect_transport() ran twice per frame when subscribed, so the trailer mixed two independent observations: in_pressure came from the first sample (via the latched state) while fill_pct came from the second. On udp:// and unix:// each call is its own SIOCOUTQ, so a trailer carrying fill_pct=10 next to in_pressure=1 was emittable -- and double-sampling defeated the entire reason the `fill` parameter exists. One sample per frame now, observed once, and this frame's observation is folded back into the sample the trailer reads. 2. ctx->pressure_state / pressure_frames were plain ints read-modify- written by the encode thread every frame while the httpd thread read them in the HTTP route. Now relaxed atomics, which is how Star6E publishes the same state. Single writer, so load/modify/store is sufficient. 3. rtp_session_init() ran twice on the socket path -- once in the new unconditional seed, then again in the socket branch -- re-randomising ssrc/seq/timestamp. Harmless today because nothing reads ctx->rtp between the two, which is exactly what makes it a trap for the next edit. The socket-branch copy is gone. Also documents a sample-point divergence the same review surfaced: this backend reads ring fill AFTER writing the current frame, Star6E observes before its send, so fill_pct carries a systematic one slot (12.5 pct pts on an 8-slot ring) more occupancy. Moving it would also move the low-water window, which is pre-existing behaviour outside this change's scope, so it is recorded rather than changed. Device-verified on .181 after the fixes: sidecar delivery 100.000% (frame_id 1707->3613, 1907 of 1907, no gaps, no duplicates, every payload 80 B), qp populated, encode duration 24.3 ms, and the udp path reporting inPressure=false / pressureDrops=0 as it should. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
feat(cv610): compile and wire the RTP sidecar into the CV610 backend
image.mirror and image.flip were hardcoded TD_FALSE on the CV610 VI channel attribute and absent from the backend's advertised field list. An inverted lens mount had no software fix on that board: the fields were accepted, validated and persisted, and the picture never moved. They are now applied at the SENSOR, through the plugin vtable's pfn_mirror_flip, which is where both SigmaStar backends already apply orientation (MI_SNR_SetOrien). Same place, same meaning, so a shared craft config reads the same on all three boards and the encoder is never asked to do geometry. The VI channel attribute stays TD_FALSE and now says why. image.rotate becomes supported at the same time. It is not a third control: venc_config's image parser turns rotate=180 into mirror+flip and anything else back into 0, before any backend sees the struct. It was already advertised on the SigmaStar backends, so leaving it false here would have meant a craft that honours rotate while reporting it unsupported. VERSION 0.74.0 -> 0.75.0, contract_version 0.23.0 -> 0.24.0 (additive; three per-backend supported flags change). Mutability is unchanged -- restart_required on all three, because the sensor is programmed once at bring-up -- so there is no cv610_field_is_restart_only() entry and no widening. ## The Bayer phase does NOT move, and that is measured The sensor driver carries a "flipping changes the Bayer start phase -- VERIFY on hardware" note, and the textbook answer is one XOR per axis into ot_isp_bayer_format. On IMX662 that is wrong: the reverse shifts the readout window with the direction, so the phase the ISP sees does not move. Bench A/B, same scene, same binary but for that one line. With the XOR, mean RGB went 106/98/141 -> 163/33/195 -- green collapsing because the ISP was demosaicing green sites as red and blue. Without it, 105/96/145, which is the unmirrored frame's own colour. Geometry was correct in both arms, so the failure presents as a white balance fault rather than an orientation one, which is exactly the trap the driver comment warns about. That VERIFY note can now be closed. Orientation is programmed after the ISP thread is up. The sensor's power-on and per-mode register blocks run out of the plugin's own init and neither touches 0x3020/0x3021, so writing the reverse registers once the sensor is streaming is both safe and the point at which the write is known to survive. The call is non-fatal and logs when the vtable entry is missing -- an orientation that silently does nothing is indistinguishable from a mis-mounted lens. ## Device verification, bench 192.168.2.181 (CV610 + IMX662) Shipping binary md5 dfe47e5f76e2d2c66d1f7a5a6afb1b80, byte-identical to the built artifact. Snapshots compared against the baseline under four hypotheses (identity / hflip / vflip / 180) by pixel correlation, so the verdict is a measurement rather than an eye check. control (no change) identity 0.9963 hflip 0.039 vflip 0.087 mirror=true hflip 0.6879 identity 0.034 flip=true vflip 0.6486 identity 0.069 rotate=180 180 0.5539 identity -0.211 restored identity 0.9963 Each axis was exercised on its own, so a single combined check cannot hide one of them being a no-op. Colour held across every arm: mean RGB within ~1 of baseline on green, against 33 in the wrong-phase arm. Host suite 2821 passed / 0 failed, including four new allowlist assertions across all three backends and a live-set rejection proving mirror stays restart-required rather than advertising a control with no apply callback. make lint clean on cv610, star6e and maruko. Bench restored: image block back to mirror/flip false, rotate 0, config byte-identical to the pre-test backup, snapshot back to identity 0.9963. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
…nally, move the call site
Six defects from the adversarial review, three of them mine and material.
## image.rotate is dropped from the CV610 allowlist
The first draft advertised it, arguing that leaving it false would mean a
craft honouring rotate while reporting it unsupported. That argument was
wrong in the direction that matters. The decomposition it rests on --
venc_config's load_image() turning rotate=180 into mirror+flip -- runs on
a FILE parse and nowhere else, so a value arriving through /api/v1/set is
never decomposed and never read by any backend.
Measured on .181 with the field advertised:
set?image.rotate=90 -> 200 {"value":90,"reinit_pending":true}
get?image.rotate -> 90 (not coerced at set time)
...encoder reinits...
get?image.rotate -> 0 (load_image() coerces on file read)
/etc/waybeam.json -> "rotate": 90 (file keeps it, permanently)
So advertising it turned a clean 501 into: accept an invalid value,
persist it, restart the encoder for nothing, and leave the file and the
running config disagreeing forever. That is precisely the failure the
allowlist exists to prevent, and its stated invariant -- "every entry is
a field the CV610 backend genuinely reads" -- would have become false for
one entry. A config file carrying rotate:180 still works.
contract_version stays 0.24.0; the change is now two supported flags, not
three, which is what HISTORY.md already said.
## The struct comment told the next person to re-break it
include/cv610_pipeline.h said "reversing the readout also moves the Bayer
start phase, which the ISP has to be told about" -- the pre-measurement
belief, left on the field the next person reads first. Following it leads
straight back to the arm measured broken (green 98 -> 33). Inverted, with
an explicit do-not-add-this-back.
## Orientation is now written unconditionally, and earlier
Both SigmaStar backends call MI_SNR_SetOrien unconditionally at bring-up;
this skipped the write entirely when both flags were false, so
"mirror=false, flip=false" cleared the sensor on SigmaStar and left it
alone on CV610 -- not the "one thing across the fleet" the change claims.
It only worked because the SoC's MIPI reset happens to clear 0x3020/0x3021
on this board, which is board wiring, not a sensor property. The same file
already states this rule for the VPSS crop.
The call moved from after the ISP thread to immediately after isp_setup().
The real gate is the plugin's i2c fd, opened by imx662_init() as
pfn_cmos_sns_init inside ss_mpi_isp_init() -- synchronously, on this
thread -- not "once streaming". Applied after the thread starts, the first
frames and the first AE/AWB statistics were gathered in the old
orientation and the readout then flipped under a converging 3A loop; and
enable_sensor_ccm() sat in between and can abort with the ISP running and
orientation never applied.
## The log claimed a success it cannot observe
pfn_mirror_flip returns td_void, the driver casts both register writes to
(td_void), and imx662_write_register() returns TD_SUCCESS even against a
closed i2c fd. "ok ... orientation" was an assertion with nothing behind
it, which would reproduce the exact failure this function exists to
abolish. Now "requested", with the three layers spelled out.
## Docs
HISTORY overstated the old behaviour ("accepted and persisted" -- the API
returned 501); it now separates the file path from the API path. The
Bayer paragraph said the A/B covered orientation generally when it covered
the mirror arm; it now also records the colour check on all three
reachable states (mirror 106/97/144, flip 106/97/145, rotate-180
106/97/144 against a 106/98/141 baseline), so the result is not
generalised from one arm. README's image.rotate row still documented a
0/90/180/270 domain and now says what actually happens.
## Device re-verification, .181, shipping binary 76568677f884c236194ab8d5a312d081
The call site moved, so all three states were re-run rather than assumed:
mirror -> hflip 0.6874 (identity 0.03)
flip -> vflip 0.6496
rotate=180 -> 180 0.5576
restored -> identity 0.9954
Colour held in every arm (green 97-100 against a 97.9 baseline). And
set?image.rotate=90 answers 501 again. Config byte-identical to the
pre-test backup.
Host suite 2820 passed / 0 failed. make lint clean on cv610, star6e and
maruko.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
cv610: honour image.mirror / image.flip at the sensor
`fpv.roi*` had no CV610 implementation: no `apply_roi_qp` in the callback table and no cold-boot apply, so every CV610 craft carried an `fpv` block in `/etc/waybeam.json` that nothing on the board read. The write path was honest -- `supported:false`, `/api/v1/set` answered 501 -- but `/api/v1/get` returned the stored value and the config kept round-tripping it, so a craft could sit for months with `roiQp:-15` configured and no region programmed. Separately, Maruko applied ROI only on a live write. Star6E has always applied it in `star6e_runtime_apply_startup_controls()`; Maruko applied `qp_delta` and `qp_bounds` there and not ROI, so a value already in the config file never reached the encoder at boot. - `cv610_apply_roi_qp()` programs the bands through `ss_mpi_venc_set_roi_attr()`. `ot_venc_roi_attr` is field-for-field `MI_VENC_RoiCfg_t` and the part takes a wider delta than the shared +/-30 clamp, so no per-backend range was needed. Geometry comes from the channel attr, not a cached width: it is what the encoder is running, and the get doubles as the "channel exists" guard. - `compute_horizontal_roi()` was byte-identical in `star6e_controls.c` and `maruko_controls.c`; CV610 would have made a third copy. The rectangle and the QP taper move to `pipeline_common_roi_band()`. - Maruko gains the cold-boot apply, placed where Star6E has it. contract_version 0.24.0 -> 0.25.0 (four per-backend `supported` flags). Verified in the bitstream, not by return code -- issue #259 is a QP control on these same parts that returns success, logs as applied, reads back clean and never moves the picture, so an SDK result is not evidence. Median Laplacian variance per 64-px column, `roiSteps:1 roiCenter:0.3` (a 384-px band at x=448), decoded from a live capture: Maruko 1280x720@30 CBR 5754k qp=-30 qp=+30 384-448 (outside) 54.5 640.1 448-512 (inside) 755.7 46.9 768-832 (inside) 854.0 6.3 832-896 (outside) 34.7 792.7 CV610 1280x720@100 CBR 2829k qp=-30 qp=+30 384-448 (outside) 88.5 286.1 448-512 (inside) 110.2 11.6 768-832 (inside) 42.7 10.5 The step lands on the programmed rect edges and inverts with the sign of the delta. A repeat of the -30 arm held the in-band figure to 6.6% on Maruko and ~13% on CV610, against 18x and 7.4x swings between arms. Re-running the same fixture on the refactored Maruko build reproduces the pre-refactor profile inside the run-to-run spread, so the shared geometry is behaviour-preserving. Also documented rather than changed: `roi_enabled` is not the switch. The apply clears every region when `roi_qp == 0`, which is the shipped default, so a craft boots logging `> ROI disabled (all regions cleared)` with `roiEnabled:true`. Host suite 2866 passed / 0 failed. Clean `-Werror` builds for cv610, star6e and maruko. Device-verified on a CV610 bench and a Maruko craft; no Star6E craft was reachable, but Maruko exercises the same shared geometry.
The shipped pair was `roiEnabled:true, roiQp:0`, which reads as "ROI is on" while apply_roi_qp() clears every region -- a zero delta is not a region worth programming. Nothing was ignored, but the flag and the log disagreed, and that is the first thing an operator meets. Defaults are now `roiEnabled:false, roiQp:-25`: off, but carrying a delta that bites the moment it is switched on, so the flag means what it says in both positions. Compiled defaults, both `config/waybeam.default*.json`, and therefore `/api/v1/defaults`. Existing crafts are unaffected -- their own /etc/waybeam.json values win. The disabled log line covered three states behind one string. It now names the cause on all three backends: > ROI disabled (roiEnabled=false), all regions cleared > ROI disabled (roiQp=0, no delta to apply), all regions cleared Also corrects a pre-existing drift in the contract doc's config example, which showed roiCenter 0.25 against the compiled 0.4. Host suite 2866 passed / 0 failed. Clean -Werror builds for cv610, star6e and maruko.
logging success that isn't, correct the measurement provenance Seven findings from an independent review of the first two commits. The category that mattered most came back clean: the geometry extraction was mutation-proven behaviour-identical to the two originals it replaced. Code: - `pipeline_common_roi_band()` clamps center_frac to [0.1, 0.9] instead of trusting three callers. It stopped being a static with two clamped call sites and became a cross-TU primitive with a documented domain it did not enforce. Measured before the fix: frac 1.5 returned SUCCESS with x = 2147483328 (unsigned underflow in (width - rw)/2), frac -2.0 returned SUCCESS with a 4294964736-wide rect -- and the negative case is an out-of-range float-to-unsigned conversion, i.e. undefined behaviour, so a target that saturates instead loses the band silently. Unreachable today from config; wrong failure mode for a shared primitive. - The CV610 readback now compares rect.x/y. It covered enable, qp, width and height while the comment claimed it caught a clamped rectangle -- a re-aligned ORIGIN is exactly what it missed, and a horizontally displaced band is the least visible failure in the picture. - The clear loop checks its return code on all three backends. Four disables with an all-zero rect were issued with the result discarded; if a clear is refused the stale band stays enabled, so shrinking roiSteps 4->2 would leave indices 2 and 3 holding the old rects, overlapping the new pair with the wrong taper, while the function logged success. Now reported, and the disabled path returns -1 rather than claiming "all regions cleared". Device-checked on CV610: zero clear failures, so the SDK does accept a zero-rect disable -- the guard is insurance, not a live bug. - No backend logs "> ROI horizontal" when every band was skipped as degenerate. Needed pic_height < 32 to reach, so unreachable through video0.size validation, but asserting an ROI that does not exist is the exact class of lie this change set is about. - Maruko's cold-boot apply reports failure, matching the qpBounds call ten lines below it and the CV610 twin. maruko_apply_roi_qp() returns -1 with no output at all on zero frame geometry -- the same silent-boot symptom this call site was added to fix. Docs: - The allowlist comment and the contract change log cited a MARUKO bitstream measurement to justify a CV610 entry. Different SoC, different SDK call (ss_mpi_venc_set_roi_attr vs MI_VENC_SetRoiCfg), so it did not qualify the claim -- and #259 is precisely a control that behaves differently per part. Both now cite the CV610 measurement taken on that bench: in-band detail drops 7.4x between roiQp -30 and +30 against an 8.8% repeat-arm control. - HISTORY named "14-25x" for a step that is 13.9x and 24.6x in one arm and 126x in the other, and quoted "1739 -> 1854" without saying what it was a median of. Both now state the aggregation and the arm. - Swept ROI docs the first commit missed: the encoder-capabilities matrix still called CV610 ROI "a future parity opportunity" (and claimed eight bands where PIPELINE_ROI_MAX_STEPS is 4), CV610_BACKEND.md still listed ROI as unexposed, ROI_INVESTIGATION.md's JSON block still showed the old shipped pair, and the contract's per-backend table gained no row for the 0.25.0 flip. - pipeline_common.h claimed "full-height, centred" bands. Down-alignment makes that the intent, not the arithmetic: 1080 rows yield 1056, and an origin can sit up to 31 px left of true centre. Host suite 2876 passed / 0 failed; the clamp guard is mutation-proven (6 FAILs without it). Clean -Werror builds for cv610, star6e, maruko. Device-verified on the CV610 bench: cold-boot apply, both new disabled-log paths, and zero clear/readback/no-band errors. The Maruko bench went off the network before its redeploy, so the SigmaStar half of THIS commit is built and tested but not yet device-run.
feat(cv610): centre-priority ROI, and stop skipping it at boot on Maruko
…orts outgoing.server and outgoing.enabled were restart-only on CV610 -- honestly so, because the URI was parsed once in cv610_prepare() and cv610_output_start() ran once from cv610_init(). Retargeting a craft meant a respawn. The two callbacks are the small part. The real work is thread safety: cv610_output_write() read socket_handle / destination / destination_len / connected_udp straight off the context on the producer thread, while a live apply rewrites them from the httpd thread -- a torn sockaddr, or a send on a closed fd. Both SigmaStar backends already guard this with an even/odd transport_gen seqlock; CV610 now does the same, snapshotting once per frame so every datagram of one access unit goes to the same destination. Splitting an access unit across two receivers leaves neither able to decode it. Ordering is a requirement, not a preference: live_group_supported_for_cfg() gates LIVE_GROUP_OUTGOING on the callbacks existing, so widening cv610_field_is_restart_only() first would have advertised `live` while every write was refused. Callbacks first, list second. RING TRANSPORTS STAY RESTART-CLASS RATHER THAN BEING REFUSED. Both SigmaStar backends refuse a live switch to or from shm:// / frame-shm://. Copying that would have REGRESSED CV610: outgoing.server was restart-required here, so writing frame-shm://... persisted and took effect on the next boot, and a hard refusal would have made the fleet's normal production transport unreachable through the API -- hand-editing /etc/waybeam.json as the only way to provision a craft. Instead the value is committed, venc_api_request_reinit() is called, and the response carries reinit_pending. That last part needed a shared fix: a live apply that requested a respawn answered a flat "ok". It now reports reinit_pending, scoped to a false->true transition across that single apply -- g_reinit is a latch the runtime consumes on its own schedule, so reading it absolutely made an unrelated live set claim a respawn belonging to an earlier request. One deliberate divergence from Star6E: it also idles the encoder to 5 fps while output is disabled. CV610 cannot -- video0.fps is restart-only there -- so a disable gates the send on both transports and the encoder keeps running. contract_version 0.25.0 -> 0.26.0 (two per-backend mutability flags). Device-verified on the CV610 bench, 720p100: - live udp->udp retarget: PID UNCHANGED across the switch, no reinit_pending, and the handover is exact -- the old port drops to 0 packets in the same second the new one reaches ~2000/s. A respawn that happens to produce video is the failure this test exists to catch. - outgoing.enabled false/true: sending stops and resumes, PID unchanged. - frame-shm:// through the API: 200 + reinit_pending, respawn, craft comes up on the ring. The bench was restored to production config by that path. - video0.fps still reports restart_required, so the widening is not a blanket. Host suite 2895 passed / 0 failed; the mutability widening and the response shape are both mutation-proven. Clean -Werror for cv610, star6e and maruko.
…e of them
regressions it introduced
An independent review of the first commit found three HIGH defects. None were
caught by the host suite, and two only exist because this change made the field
live.
- **An identical re-POST of the running ring URI respawned the craft.** The
ring branch was evaluated BEFORE the applied-destination no-op guard, so
writing frame-shm://venc_frame to a craft already running it latched a reinit
and restarted the pipeline. Every idempotent config re-apply from the ground
cost a multi-second video outage. Star6E compares before it refuses, for
exactly this reason. Guard moved first; the ring branch now also records the
URI as applied so a second write inside the respawn window is a no-op too.
Fixing that exposed a second half on the device: applied_server was seeded
only on the socket path, so a frame-shm:// craft started with it empty and the
guard could never match. It is now seeded for every transport.
- **/api/v1/live/set accepted a restart-class write and then discarded it.**
That endpoint passes persist=0 by design, which is safe for a change that took
effect in the running process -- but not once an apply has asked for a
respawn, because the respawn re-execs and reloads /etc/waybeam.json. The
write was answered 200 + reinit_pending and then silently lost. The config is
now written to disk whenever an apply requested a reinit, regardless of
persist. The restart-class path already refuses this shape ("restart-class
field requires persistence; use /api/v1/set"); a backend-level restart class
reached the same hazard through the live path.
- **Live-enabling output on a boot-disabled craft sent RTP with ssrc 0 and a
frozen timestamp.** cv610_output_start() bailed on !outgoing.enabled BEFORE
rtp_session_init(), and the context is calloc'd, so ssrc, seq, timestamp AND
frame_ticks were all 0 -- every packet carried ssrc 0 and a timestamp that
never advanced, which a depacketizer keying AU boundaries on the timestamp
cannot follow. Only reachable because outgoing.enabled became live. The
session is now seeded before the bail, matching Star6E.
- **A batched live set reported no respawn.** make_multi_live_set_success_json()
never took the flag the single-field path had just been given, so
`?outgoing.server=frame-shm://...&video0.bitrate=9000` answered a plain ok
while the craft was about to restart -- contradicting HISTORY.md and the
contract doc in the same commit.
- **A rejected destination killed the live socket and the rollback was
swallowed.** Nothing validates outgoing.server, and output_socket_configure()
closes the fd when it fails partway -- including on the fd-REUSE path -- so an
unresolvable udp:// host closed a working socket. applied_server still held
the old URI, so the rollback venc_api runs next hit the no-op guard and
returned 0 without reopening: HTTP 500, config correctly rolled back, and no
video until the process restarted. applied_server is now cleared on failure.
- **GDR position and the ring low-water gauge froze while output was disabled.**
Both describe the ENCODER and the ring, not the transmission. A disable ->
N frames -> enable left meta.gdr_pos desynced from the real intra-refresh
wavefront until the next IDR, and the link consumer assigns slice importance
from that position. Both now advance every frame; only the write is gated.
Docs: README claimed the ring write "errors" on every backend, contradicting the
contract doc in the same diff. The audio side-channel not following a live
retarget (cv610_audio.c derives it once at start; Star6E has the same gap) is
called out in HISTORY rather than half-fixed.
Tests: the batch path had NO coverage, which is how that defect shipped. Added,
and mutation-proven. Host suite 2902 passed / 0 failed.
Device-verified on the CV610 bench, on the binary being merged (md5
cf2614c2f72ca24c330ff13716010c65, byte-identical to out/cv610/waybeam):
- three identical re-POSTs of the running frame-shm:// URI: 200, no
reinit_pending, PID 2554 UNCHANGED (previously respawned on the first).
- boot disabled on udp://, enable over HTTP: ssrc 0x9507aa3f, 105 distinct
advancing timestamps, PID 2612 unchanged. Previously ssrc 0, timestamp 0.
- the bench was returned to frame-shm://venc_frame + audio through the API, and
the value survived the respawn -- which is the persist fix working.
feat(cv610): live output retarget, without regressing the ring transports
0.77.0 shipped with this called out as a known gap. The Opus destination is
derived from the video URI -- peer host for udp://, loopback for the local
transports -- and that derivation ran once, at cv610_audio_start(). So a live
outgoing.server change moved video to the new receiver and left audio going to
the old one, silently, because nothing in the audio path knows the video URI
changed.
- cv610_apply_server() now calls cv610_audio_apply_server(), which re-derives
and repoints the audio socket.
- The derivation moves into cv610_audio_derive_output(), shared by the start
path and the retarget. A rule applied in two places drifts; one copy cannot.
- Same seqlock as the video path: the audio transport fields are read by the
capture thread in cv610_audio_write() and rewritten by the httpd thread, so
the capture thread snapshots them once per encoded frame and every packet of
one Opus access unit reaches the same destination.
- Not fatal on failure. The video socket has already moved by then, and
returning -1 would send venc_api into a rollback that re-enters this
function; a logged audio failure beats a half-applied rollback.
No contract change -- no field, endpoint or payload moves, so contract_version
stays 0.26.0. VERSION 0.77.0 -> 0.77.1.
Star6E and Maruko have the same gap and are deliberately NOT changed: their
retarget path is separate code and no SigmaStar craft was reachable to verify
against. Changing it blind is how the last round shipped three regressions.
Device-verified on the CV610 bench with audio_port=0, so the audio port follows
the video port and both streams are observable on one socket, separated by
payload type (video PT=97, audio PT=98). Live retarget udp://:5700 ->
udp://:5800, packets per second:
5700:video 5700:audio | 5800:video 5800:audio
t=3 2169 50 | 0 0
t=4 0 0 | 2382 50 <- retarget
t=5 0 0 | 2129 50
Both streams move together in the same second; PID 2793 unchanged across the
switch. The bench was then returned to frame-shm://venc_frame + audio_port
5611, and the audio line correctly reads udp://127.0.0.1:5611 again for a local
video transport.
Host suite 2902 passed / 0 failed; clean -Werror for cv610, star6e and maruko.
Note the standing structural gap: src/cv610_audio.c, like src/cv610_runtime.c,
is not in TEST_SRCS, so this path has device evidence and no unit coverage.
fix(cv610): the audio side channel follows a live output retarget
… claim
0.77.0 and 0.77.1 both state that Star6E and Maruko share CV610's audio-retarget
gap. They do not, and the claim was never verified before it was written -- it
came from a review finding that I repeated.
Both SigmaStar backends store a POINTER to the live video output rather than a
copy, and resolve the audio destination at send time through a cache keyed on
the video output's transport_gen (star6e_output.c resolve_cached_audio_target(),
maruko_audio.c:61-85). A retarget bumps that generation, the cache invalidates,
and audio re-resolves to the new destination by itself.
CV610 was different because cv610_audio.c COPIED the resolved destination into
its own state at start. That was the whole bug, and it was CV610-only.
Device-confirmed on a Star6E craft running the RELEASED 0.76.0 binary, with no
fix applied and nothing from 0.77.x deployed -- so this measures the shipped
behaviour, not a patched one. Live udp://:5700 -> udp://:5800 with
audio_port=0, so the audio port follows the video port and both streams are
separable by payload type:
5700:video 5700:audio | 5800:video 5800:audio
t=3 1754 50 | 0 0
t=4 0 0 | 1717 49 <- retarget
t=5 0 0 | 1789 49
Both move in the same second; PID 2146 unchanged. The bench was returned to
frame-shm://venc_frame with audio_port 5601.
No code change: 0.77.1 is correct as shipped, and the SigmaStar backends needed
nothing. VERSION is deliberately not bumped -- this corrects the text of two
released entries rather than changing behaviour.
…llows docs: the SigmaStar backends already follow a retarget — correct the claim
isp.gain_max and isp.shutter_max_us were in the config schema, returned by
/api/v1/get and reported supported:false on CV610 -- accepted into the file
and never read. They now map onto the exposure group cv610_iq.c already
owns, with NO unit conversion, which is the only reason they are wired
rather than approximated:
isp.shutter_max_us -> exposure.auto.exp_time_max ("unit: us")
isp.gain_max -> exposure.auto.a_gain_max (Format:22.10, 1024 == 1x)
Analog gain, not sys_gain: the portable name means the sensor's ceiling on
every other backend, and sys_gain_range caps the product of analog,
sensor-digital and ISP-digital gain.
Measured on a CV610 bench (sc4336p, ave_lum 43) through /api/v1/awb, which
reports the AE's own output rather than the value written: capping
shutterMaxUs to 4000 pulled the applied exp_time 16560 -> exactly 4000 us
and the AE raised a_gain 1497 -> 6611 to hold the same luma; capping gainMax
to 2048 clamped the applied a_gain to 2043 while the ISP took up the slack
in digital gain (isp_d_gain 1024 -> 1948), which is also what shows the
field is the analog ceiling. Clearing each to 0 restored 873800 / 407654
and the original applied values.
0 means "use the sensor plugin's default" on every backend, so the default
is snapshotted before the first write lands -- otherwise setting a ceiling
and clearing it would leave the ISP on the last non-zero value forever while
the config read 0. Both also apply at cold boot, since the ISP is seeded by
the sensor plugin, which never sees the config file; verified with 6000/8192
in the file.
isp.gain_min, isp.shutter_min_us, isp.awb_mode and isp.awb_ct stay
unsupported and still 501. The floors were not measured in this slice. The
AWB pair cannot be honoured at all: ct_manual means "pin white balance to a
colour temperature" and this ISP has no Kelvin input -- ot_isp_mwb_attr is
r/gr/gb/b gains in Format:4.8 and ss_mpi_awb.h exports only
register/unregister. Manual white balance is reachable exactly, under its
own name, through /api/v1/iq's wb group.
contract_version 0.26.0 -> 0.27.0 (a per-backend supported flag changes).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
Two review findings on #270. The claim that this ISP has 'no Kelvin input' and that ss_mpi_awb.h exports 'only register/unregister' was FALSE, and it reached three places -- the allowlist comment, HISTORY and the contract doc. That header exports 14 functions including ss_mpi_isp_cal_gain_by_temp(pipe, wb_attr, color_temp, shift, awb_gain), which is present in libss_mpi_awb.so. A grep pattern anchored on 'ss_mpi_awb' hid every ss_mpi_isp_* symbol in the file. The DECISION to leave isp.awb_mode/isp.awb_ct unsupported stands -- it is a two-call, calibration-dependent flow this slice neither implemented nor measured -- but it is now stated as unmeasured rather than impossible. ae_defaults() called iq_get_exposure() without the cv610_pipeline_isp_ready() check every other MPI entry point in the file makes. Unreachable today (both callers run after cv610_pipeline_start() and before venc_httpd_start()), but it is the guard that lets the snapshot trust the value it latches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
roiQp is a RELATIVE delta and H.265 caps QP at 51, so past +-20 it stops being honoured at both ends -- and the negative end is expensive rather than merely truncated. CBR pays for the ROI discount by raising the frame's base QP roughly 1:1 with |roiQp|. Once base_qp + |roiQp| passes 51 the rate controller saturates -- every frame at qp 51/51/51, zero variance, no authority left -- while the ROI band still encodes at 51 - |roiQp|. Measured on a CV610 bench at 720p60 via the sidecar's per-frame frame_size_bytes and qp: target 2829 kbps, roiQp 0 -> base qp 25.4, delivered 2849 (1.01x) target 2829 kbps, roiQp -20 -> base qp 45.1, delivered 2802 (0.99x) target 2829 kbps, roiQp -25 -> base qp 49.9, delivered 2991 (1.06x) target 2829 kbps, roiQp -30 -> base qp 51/51/51, 16976 (6.0x) target 9000 kbps, roiQp -30 -> base qp 51/51/51, 17300 (1.9x) The rule base_qp + |roiQp| > 51 predicts three separate bitrate targets. The QP column is the instrument: below the cliff CBR absorbs the whole delta and the delivered rate does not move, so rate alone cannot see this. Band width does NOT move the cliff, only its severity -- at roiQp -30, roiCenter 0.6 / 0.4 / 0.2 all pinned at 51 and delivered 17661 / 12520 / 6977 kbps -- so there is nothing to bound on that axis instead. Positive roiQp is the benign end but truncates just as quietly: at +30 the base sat at 21.5, so the region wanted 51.5 and got 51. A config file is CLAMPED, not rejected: load_fpv() runs on the file parse, and a craft already carrying -30 must still boot. It lands on -20 instead, which is the fix applying itself. Only /api/v1/set rejects, so an operator typing -30 is told rather than reading back a value they did not write. Device-verified on a CV610 craft: a -30 config file booted and programmed qp=-20. Shipped default roiQp -25 -> -20, so the default sits inside the range it documents. NOT reproduced on Star6E: 1080p60 held its target at roiQp 0/-15/-25/-30, both at 19092 kbps and forced down to 1500. Its sidecar reports qp=0, so its rate controller could not be watched directly -- unreproduced, not immune. The bound applies on all three backends anyway, because the QP ceiling it derives from is an H.265 limit, not a vendor one. contract_version 0.27.0 -> 0.28.0. This is a NARROWING: a client sending +-21..+-30 now gets 400 where it previously got 200. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
Four review findings on #271, all sites that still assumed +-30. scripts/api_test_suite.sh swept roiQp -30..30 and asserted ok:true on every step, so this PR broke the device suite. The sweep now runs in range, and +-21 and +-30 are asserted REJECTED -- both sides of the edge, because a validator that had drifted to +-25 would still pass an in-range-only sweep. web/dashboard.html's tooltip still read "-30 to +30". It is the only client-facing description of the field (the field table carries no FieldUi metadata for fpv.roi_qp, so /api/v1/capabilities publishes no range), and it compiles into the served blob -- src/venc_webui.c regenerated with `make webui`, `make webui-check` clean. Three stale docs: README's field table row (range AND the -25 default), ROI_INVESTIGATION.md's range line, and the struct comment in venc_config.h. The shipped JSON defaults had no test at all -- config/waybeam.default.json was covered only incidentally by layout_byte_equal, and the Maruko file was loaded by no test whatever. The first attempt at that assertion was itself broken: it compared the LOADED value, and load_fpv() clamps, so a stale -25 loads as -20 and the check passes while the shipped file is still wrong. Mutation-checked -- seeding -25 into the Maruko file produced zero failures. It now parses the raw JSON, and both files fail when mutated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
feat(cv610): the portable AE ceilings reach the ISP
fix: bound fpv.roiQp to ±20, where the delta still reaches the encoder
ENC_INFO.qp reads 0 on both SigmaStar backends. The obvious fix --
enc_info.qp = stream.h265Info.startQual, mirroring what CV610 does with
h265_info.start_qp -- was implemented, cross-built and deployed, and it
ships zeros.
A raw dump of i6_venc_strminfo_h265 on a 1080p60 H.265 stream says why:
[qpdbg] h265Info: 0 0 0 0 0 0 0 0 0 2 0 0
size=0 refType=2 updAttrCnt=0 startQual=0
Word 9 is refType and it IS populated, which proves the struct offset in
sigmastar_types.h is correct -- this is not a layout bug. The driver fills
refType and nothing else: size, all eight CU counts, updAttrCnt and
startQual are 0 every frame.
So the backends leave qp at 0 rather than advertise a field that is always
0, and the contract now says so instead of leaving the reader to infer from
the CV610 row that all three provide it. Recovering a per-frame QP on
SigmaStar means parsing slice_qp_delta out of the slice header; h26x_util.c
and h26x_param_sets.c already parse NALs and parameter sets, so the pieces
exist, but that is a feature rather than a field fill.
No functional change: docs and one header comment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
…p trap
0.79.0 bounded fpv.roiQp to +-20 but calibrated that bound against the
DEFAULT QP ceiling. video0.maxQp moves the ceiling, so an operator who
lowers it to control bitrate silently re-enters the failure 0.79.0 closed.
Measured on a CV610 bench at a fixed roiQp -20 against a 9264 kbps target:
maxQp default -> frame qp 44.9 (44-47), 9502 kbps 1.0x
maxQp 45 -> frame qp 45.0 (36-45), 12528 kbps 1.4x
maxQp 40 -> frame qp 40.0 (40-40), 53493 kbps 5.8x
maxQp 35 -> frame qp 35.0 (35-35), 111085 kbps 12.0x
Also recorded: Star6E's default ceiling is 48, not 51 (MaxQp 48 MinQp 12
from /proc/mi_modules/mi_venc/mi_venc0), so it has three fewer QP of margin
than CV610 at the same roiQp.
pipeline_common_rate_watch() now reports it. Two consecutive 2 s windows at
>=150% of the configured target log once, naming roiQp and the ceiling when
ROI is on; it clears below 120% and can arm again.
It watches the CONSEQUENCE, not the cause, deliberately: the frame QP that
explains the failure is CV610-only, because SigmaStar fills only refType in
its stream info. Frame size exists on every backend, and a collapse is far
easier to see in the delivered rate (1.9x-39x, sustained) than in a QP
sitting near its ceiling.
Thresholds are compiled in, not configurable -- the measurement left no room
to tune. Normal operation ran 0.96-1.06x across every arm, the worst benign
scene transient hit 1.43x for ONE window, and a real collapse ran 1.9x-39x
and did not decay. Two windows is what separates the two.
Device-verified on CV610 with NO sidecar probe attached: 16 s at the default
ceiling logged nothing, forcing maxQp 35 logged exactly one warning ("3695%
of the 2829 kbps target"), and restoring the ceiling logged the clear. That
run also caught a placement bug -- the first version sat inside the sidecar
block, which is gated on a live subscriber, so it observed only while a
probe happened to be attached.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
…cope Two review notes on #273, neither a defect. cfg->video0.bitrate is read on the encode thread while the httpd thread may be committing a config under g_cfg_mutex. That stays unlocked: it is a per-frame path, the field is a naturally aligned uint32_t, and the two-window rule already absorbs the only damage a mid-commit read could do -- one window computed against the wrong target cannot raise a report by itself. Taking the config mutex once per encoded frame to protect a diagnostic is the worse trade. Said so in the code rather than leaving the next reader to re-derive it. Maruko's second 'dual' VENC channel runs its own thread and never reaches the drain loop this is called from, so that stream is not watched. Noted in the header so it reads as scope rather than oversight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDRCithJcHpQ1YA3Q8u2nS
…ilable docs: the sidecar qp is CV610-only, and SigmaStar cannot do better
feat: report a sustained bitrate overrun, and document the roiQp/maxQp trap
cv610_iq.c moves ISP parameters one knob at a time. This moves the whole
image, through the vendor libbin.so, which is what PQTools writes.
The call contract came out of the stripped blob -- the package we have
carries the library but not ot_pq_bin.h, and the older HI_PQ_BIN API on
Hi3516CV100 has a different shape whose error codes do not apply here:
* OT_PQ_BIN_ImportBinData (@0xd9c) reads two enables at +0 and +4;
OT_PQ_BIN_ImportNRXData (@0x14e4) passes +8 to PQ_BIN_SetPipeNRXParam
as the VI pipe. Those three fields are all we mirror, behind a zeroed
pad so an unidentified tail reads as zero rather than off the end.
* g_aeHandle is a void ** that libbin dereferences TWICE, so it gets a
pointer to a static that holds the AE handle. Storing the handle
itself resolves nothing on musl -- silently, while we print success.
* Both transfers refuse to run unless ss_mpi_sys_get_tuning_connect()
reports a connection. Nothing in venc set it before, so this enables
it around the transfer and puts it back.
* A file is [ISP image of OT_PQ_GetISPDataTotalLen() bytes][OTPQNRX
section]. Asking the library for that length IS the version check.
* Export is sized from the library too, and nr_enable is decided by the
ANSWER rather than asserted before the question: GetStructParamLen
returns 0 when the 3DNR query fails, ExportBinData recomputes the same
sum and demands EXACT equality, so a hardcoded nr_enable=1 would pass
that check and then write the 37-byte NRX header one past the end.
* The 3DNR half of an import is gated on that same length. A header-size
check is not enough: ImportNRXData does not forward our length to
PQ_BIN_SetNRDataV2, which memcpy's a fixed ~1298-byte payload and
validates the declared size afterwards -- so a validly-headed but
truncated file (an interrupted scp, a full tmpfs) would read past the
buffer and push uninitialised heap into the 3DNR registers.
The lock on a .bin is the chip register map and the SDK ISP version, not
the sensor: import validates an address/size walk against what this chip's
ISP reports live, and the format carries no sensor identity at all.
isp.sensorBin was already MUT_LIVE with a readability validator, so
apply_isp_bin is the whole live surface. It also needed an entry in
venc_api_field_supported_for_backend()'s CV610 allowlist -- without it
every write 501s "field not supported on this backend" while the callback
sits wired and unreachable, and only the device found that.
The cold-boot apply lands BEFORE isp.gainMax / isp.shutterMaxUs: a .bin
carries an AE ext-register record of its own, so the reverse order
overwrote the config's ceilings on every boot while /api/v1/get kept
reporting them. A successful import also drops cv610_iq.c's cached
cold-boot AE ceilings, which would otherwise write pre-import values back
on the next isp.gainMax=0.
Export has no SigmaStar analogue (those round-trip IQ as JSON), so it gets
a callback, GET /api/v1/iq/export_bin and a routes.iq_export_bin
capability flag. It writes a fixed /tmp path -- the endpoint is
unauthenticated and a caller-supplied path would make it a write-anywhere
primitive -- and answers with the byte count so a caller can confirm the
write. The route sits before /api/v1/iq because venc_httpd matches by
first-match prefix and accepts '/' as a boundary; the new test's middle
arm asserts on the body, which is what would catch an inversion.
Device-verified on .181 (Hi3516CV610 + IMX662):
* Export writes 144774 bytes whose record walk -- 3 records, 131072 +
8192 + 4096 behind 20-byte headers -- lands exactly on 143424 and
matches the vendor tunes in layout. Its header reports the live mode.
* A tune built for an os02h10 imports cleanly, moving 39 of the 102
fields /api/v1/iq reads back across ten ISP groups; the operator saw
the picture go red, which is the expected result for a foreign tune.
* Round trip is exact: importing our own exported file restored an
identical read-back, confirmed by eye as well.
* A live import and a cold-boot import of the same file differ in 0 of
102 fields, so the running ISP thread does not re-assert over it.
* Refusals fire with the right reasons: a file short of the ISP image is
rejected by our gate naming both numbers, a headed-but-truncated 3DNR
section imports the ISP half only with a warning naming both numbers,
and one flipped byte is refused by the library with 0xcb000005 -- so a
.bin is integrity-checked and cannot be spliced in place.
* Video never stalled through any of it, cold boot from /etc/sensors
included.
iq-profiles/cv610-bin/imx662.bin vendors an export of that bench's
cold-boot state as a restore point -- explicitly NOT a measured tune,
since nobody has run PQTools against this sensor. make stage ships it in
isp-bins/, mirroring the maruko path.
Requires libbin.so on the craft; without it the import warns and no-ops.
Every failure is a warning and the craft still boots, matching
isp_runtime.c.
Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
) isp.sensorBin and /api/v1/iq/export_bin both dlopen libbin.so, and nothing put it on the craft -- every deployment needed a manual scp or the two features silently no-opped. The blob is not committed here. Its licensing is the vendor's, and this repo is public; `make stage` copies it from CV610_PQ_LIB instead, and prints a NOTE naming the path it looked at when the file is absent, so a build without it is obvious rather than a craft that quietly cannot import. Both arms verified: the copy lands byte-identical, and a bogus path prints the note and stages nothing. Default points at the vendor directory that sits beside the firmware tree, matching how CV610_SDK_LIB is defaulted. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR Summary by QodoExpand CV610 parity with sidecar, live controls, ROI, and PQTools bins
AI Description
Diagram
High-Level Assessment
Files changed (42)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| __atomic_fetch_add(&g_cv610_runner->transport_gen, 1, __ATOMIC_RELEASE); | ||
| if (output_socket_configure(&g_cv610_runner->socket_handle, | ||
| &g_cv610_runner->destination, &g_cv610_runner->destination_len, | ||
| &g_cv610_runner->transport, &parsed, |
There was a problem hiding this comment.
6. Retarget races active sends 🐞 Bug ☼ Reliability
The video and audio retarget paths rewrite and potentially close their live socket state while producer threads copy and use that state without synchronization, making the purported seqlock a C data race. A retarget can consequently send through a closed or reused descriptor, or route an access unit using inconsistent connection state.
Agent Prompt
## Issue description
Live video and audio retargeting mutates ordinary socket fields concurrently with producer reads, and the generation check does not keep a snapshotted descriptor alive through `sendmsg()`. Introduce synchronization or a reference-counted transport snapshot that prevents closure or reuse until all sends using the old transport have completed.
## Issue Context
A generation counter cannot make non-atomic concurrent struct access valid C, nor can it protect work performed after the generation was checked. Account for same-socket UDP `connect()` changes as well as UDP/Unix descriptor replacement.
## Fix Focus Areas
- src/cv610_runtime.c[1227-1251]
- src/cv610_runtime.c[1378-1415]
- src/cv610_audio.c[321-328]
- src/cv610_audio.c[336-369]
- src/output_socket.c[180-225]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /* cfg->video0.bitrate is read on the encode thread while the httpd | ||
| * thread may be committing a new config under g_cfg_mutex. Deliberately | ||
| * unlocked: this is a per-frame path, the field is a naturally aligned | ||
| * uint32_t, and the two-window rule already absorbs the only damage a |
There was a problem hiding this comment.
8. Bitrate monitor races config 🐞 Bug ◔ Observability
pipeline_common_rate_watch() deliberately reads VencConfig fields without the mutex while the HTTP thread can update the same object. This is undefined behavior in C and can combine bitrate and ROI values from different configuration generations, producing missing or misleading overrun diagnostics.
Agent Prompt
## Issue description
Remove unsynchronized reads of the shared `VencConfig` from the per-frame bitrate monitor. Pass a coherent immutable snapshot of the few required fields, publish those fields atomically, or refresh a monitoring snapshot under the existing configuration mutex when configuration changes.
## Issue Context
Natural alignment prevents some hardware tearing but does not make concurrent non-atomic C accesses valid. The monitor reads the bitrate more than once and separately reads several ROI fields.
## Fix Focus Areas
- src/pipeline_common.c[312-373]
- include/pipeline_common.h[105-117]
- src/venc_api.c[63-80]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
… gap (#276) Three of the eight findings on OpenIPC#120 were real. The other five are answered on the PR rather than patched; see there for the reasoning, which is not the same as agreeing with them. Security: cv610_pq_bin_export() opened a predictable path in a world-writable /tmp with fopen(), which follows symlinks -- and the endpoint that calls it is unauthenticated, so a local user could plant a link and redirect the daemon's write to anything it may write. Now O_NOFOLLOW, plus an fstat that rejects anything which is not a plain single-linked regular file, which also covers a planted hard link. Deliberately NOT unlink-then-create: that reopens the window between the two calls. Honesty: a craft flashed without libbin.so advertised isp.sensorBin and routes.iq_export_bin and then failed every use of them. cv610_init now probes once and drops the two callbacks when the blob is absent, so capabilities describes the control surface the craft can actually serve -- which is what routes.iq_export_bin means, since it tracks the callback pointer. The probe is narrower than pq_lib_open(): one dlsym, no writes into the library's globals. Docs: the export response example omitted the `bytes` member the endpoint always returns. Documented, along with why it exists (a constant path is no evidence the write happened) and the new symlink refusal. NOT device-verified: the bench went off the network before these three could be exercised on hardware. Builds are clean on all three backends and the host suites pass, but the symlink refusal and the capability-gating path have been reasoned about, not run. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#277) persist, and two comments that were simply wrong The symlink hardening shipped in the previous commit had a hole in the half it claimed to cover, and the review reproduced it on the host. O_TRUNC executes INSIDE open(), before the fstat guard can refuse. So a planted HARD link was detected and then rejected -- after the victim had already been zeroed. /tmp/waybeam.log sits on the same tmpfs as the export path, so an unauthenticated GET could empty the daemon's own log, and the 500 body then invited the operator to go read it. Truncation now happens with ftruncate() once the fd is known to be a private regular file. Reproduced before and after: hard link, victim 40 -> 0 bytes before, 40 -> 40 and REFUSED after. O_NONBLOCK, for the same open: a planted FIFO parked open() forever. That runs on the single httpd dispatch thread under its mutex, so it wedged the whole API and deadlocked teardown's pause. Reproduced: 5 s timeout before, ENXIO refusal after. Regular files ignore the flag. The availability probe resolved one symbol where pq_lib_open() requires four, so a partial blob answered "available" and then failed every export. Capability honesty was half-applied: routes.iq_export_bin went honest while fields[isp.sensorBin].supported stayed name-based, so the dashboard still drew an enabled input whose every write 501s. The allowlist entry is now conditional on the callback, which is what the flag beside it already means. Persistence: the guard at the end of the set path used a false->true transition on the reinit latch to mean "this apply asked for a respawn". It cannot mean that -- the latch is consumed by the main loop, whose select() carries a one-second timeout, so a second live/set of a restart-class field while a respawn was already pending saw no transition, skipped the write, and was discarded by the re-exec that reloaded the FIRST value. Silently, in exactly the case the guard was written to cover. Replaced with a monotonic reinit sequence number, which also makes the reported reinit_pending exact rather than a proxy. Two comments asserted protection that does not exist, which is worse than the gap itself: * The transport seqlock does not cover the destination under connectedUdp -- the shipped default -- because it lives in the kernel socket and is retargeted with connect() on the live fd. And "only a udp <-> unix switch closes and reopens" is wrong: a destination fill failure closes the fd with the type unchanged, reachable by setting a hostname URI. Both are inherited from the SigmaStar backends, which have the same gaps; the note now says what is and is not covered. * The rate watch's unlocked read was justified by natural alignment, which is not a validity argument. The real reason it is safe is that no concurrently committable value changes control flow: bitrate == 0 is the only candidate and validation forbids it. Left deliberately unfixed, for a decision rather than a patch: making a live transport-TYPE change restart-class would diverge CV610 from the two SigmaStar backends running the identical pattern, and the bench is off the network, so a transport change cannot be verified on hardware tonight. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tive, (#278) a rotate that resurrected, and a sidecar field that meant two things The rate watch's "two CONSECUTIVE windows" rule was not consecutive. >=150% counted up and <=120% reset, and nothing at all ran for 121-149% -- so a benign window PRESERVED the streak. The worst benign transient measured on the bench is 1.43x, which lands inside that dead band, so this was the common case rather than a corner: two spikes an unbounded time apart reported as one episode, with a duration computed from the window count, so a 42-second gap still claimed "for 4 s". Both halves of the message were false. The streak now breaks in the dead band while !reported; once latched the hysteresis still holds, which is what keeps an oscillating episode from flapping. Tested both ways -- the new armed-streak case fails without the fix, and the existing latched case still passes. image.rotate:180 made image.mirror and image.flip permanently unsettable. The decomposition never cleared rotate, and save() writes it back, so it re-ran on every parse and always won. Since image.mirror is MUT_RESTART the sequence was: write accepted, persisted, reinit_pending reported, /api/v1/get reads false -- and then it silently comes back true across the restart it had just requested. rotate is write-only and is now cleared in both branches. Pre-existing in load_image(), but this PR is what makes it observable: mirror/flip were hardcoded TD_FALSE on CV610 before, so the reversion had nothing to revert. The sidecar's idr_inserted meant two different things by backend. It is defined as "the controller REQUESTED an IDR after this frame", and on SigmaStar the only writer is the scene detector, so there it is disjoint from frame_type == IDR. CV610 set it on every IDR, making it a duplicate of frame_type and FLAG_KEYFRAME -- a ground consumer that sums it as "IDR insertions" would read a once-per-GOP static scene as one per second while an identically configured SigmaStar craft reported zero. Left 0, matching what complexity/scene_change/gop_state already do two lines away to mean "not produced", and the two docs that listed it are corrected. A failed SIOCOUTQ was published as a measured fillPct 0, with active 1, and fed to the pressure observer as a real sample -- so a full queue could be reported as empty and not in pressure. star6e_output.c keys on have_fill and declines to observe for exactly this reason; CV610 now matches. And one release note contradicted the bullet twenty lines above it about whether the transport observation is gated on a subscriber. It is not, since the sidecar work later in the same release moved it. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…279) output_socket_configure() closes the fd when fill_destination fails -- including on the fd-REUSE path, where the transport type is unchanged and the socket was working. Nothing resolves names (the fill is inet_pton only), so `outgoing.server=udp://somehost:5600` destroyed a live socket and left socket_handle -1 across the error return, the JSON build and the rollback re-entry. The recovery already in that branch survives it; it does not prevent it, and the window it leaves is wider than the udp<->unix switch the header comment compares itself to. Filling a throwaway sockaddr with the same input the real call will use is the whole fix: a URI that would have failed inside configure() is now refused with the live transport untouched and the generation counter never bumped. One inet_pton on a stack buffer, on an operator-rate path. Demonstrated on the host against the real output_socket.c, since the CV610 call site is not host-compilable and the bench is off the network: old path: configure(udp://somehost:5600) -> rc=-1, handle -1, the working fd CLOSED fixed: fill_destination refuses it, configure never runs, fd still alive, handle unchanged Deliberately does NOT touch the other half of this area. Routing a live transport-TYPE change to the restart class would diverge CV610 from the two SigmaStar backends running the identical inherited seqlock, which is a fleet decision rather than a bug fix; the gap is documented in the transport_gen note instead. This change has no API-visible effect at all -- a URI that was accepted before is still accepted, and one that failed still fails, only without taking the transport down on its way out. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…it (#280) Device testing #279 on the bench found the placement was wrong, and that the branch it did not cover fails harder than the one it did. The craft was running frame-shm://venc_frame, so `outgoing.server= udp://somehost:5600` took the RING branch -- which commits the value and respawns without ever looking at it. The re-exec reloaded the bad URI, failed to bring the output up, tore down, and the daemon STAYED DOWN. The bench needed its config repaired over ssh to come back. That is worse than the socket-close #279 fixed: it survives the restart and takes the video with it, on a craft nobody can ssh into. The check moves above both branches and is gated on the URI type, since ring URIs carry no sockaddr and are validated where they are created. Device-verified on the CV610 bench, same input both times: before: ok:true reinit_pending:true -> persisted udp://somehost:5600, respawned, failed to start, daemon down until manual repair after: ok:false, nothing persisted (still frame-shm://venc_frame), same PID, framesSent 792 -> 1198, log names the reason "outgoing.server udp://somehost:5600 has no usable destination; transport left unchanged" Also verified on the bench in the same session, both from #276/#277: - a planted SYMLINK at the export path is refused (ELOOP) and the victim stays 40 bytes; a planted HARD LINK is refused ("not a private regular file (mode 0600, links 2)") and the victim stays 40 bytes, where before the fix it was truncated to 0; the clean path still exports 144774 B. - with libbin.so moved aside, routes.iq_export_bin reads false, fields[isp.sensor_bin].supported reads false, the export endpoint 501s, and the craft boots and streams normally. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…as right (#281) I declined this twice on the grounds that cv610_iq.c is built on file-scope statics throughout and matching the module was the consistent thing to do. That was wrong, and checking the repo's own conventions would have shown it: AGENTS.md:551 reads "No global mutable state beyond what already exists" -- which prohibits ADDING, and is not satisfied by resembling what is already there. Three of the four additions were genuinely new. * cv610_iq.c's three AE-defaults variables move into a caller-owned Cv610IqAeDefaults, kept in Cv610RunnerContext -- a context that already exists, so nothing new is introduced. The vtable takes bare values, so three thin adapters in cv610_runtime.c bridge it. * cv610_pq_bin.c's g_ae_handle moves into PqBinLib. It only ever had to outlive pq_lib_open(), not the process: that struct is a local in import/export and lives across the whole transfer, which is the last moment libbin reads through it. pq_lib_close() now also clears libbin's g_aeHandle, so the library is never left pointing at a returned frame -- the static was hiding a dangling pointer, not preventing one. * cv610_pq_bin_available()'s memoization static is gone. cv610_init() is the only caller and calls it once, so the cache bought nothing. The AE-defaults invalidation after a .bin import moves up to the runtime adapter as a consequence, which is where it belonged anyway -- cv610_pq_bin no longer reaches into a sibling module's state, and drops the include. Device-verified on the CV610 bench, since this refactors the AE apply path: cold boot bin imported, THEN ceilings latched (a_gain_max=407654, exp_time_max=524280 us) -- order preserved gainMax2048 applied a_gain clamped 2043 gainMax=0 applied a_gain rose to 7333 -- the ceiling RELEASED back to the plugin default rather than sticking, which is the one-way door this cache exists to prevent import still succeeds through the new adapter; video uninterrupted Not taken: the same reviewer's third rule-violation, that the feature depends on a proprietary libbin.so. That dependency is deliberate and optional -- dlopen'd, every failure warns and no-ops, the blob is not committed, and a craft without it now advertises neither the field nor the route (device-verified). Removing the feature is not a defect fix. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
WAYBEAM_VERSION moves to 0c660fc5 (OpenIPC/waybeam#120), which brings the CV610 backend to parity: RTP sidecar, sensor orientation, ROI, live output retarget, portable AE ceilings, and PQTools .bin import/export. The .bin features dlopen libbin.so, and nothing put it on the craft -- so every image needed a manual scp or isp.sensorBin and /api/v1/iq/export_bin silently no-opped. The blob is carried in files/ and fed to waybeam's own stage step via CV610_PQ_LIB, so there is one source of truth for the file and the install comes from the staged output like every other artifact. WAYBEAM_LICENSE names it. The waybeam sources are MIT; libbin.so is a third-party HiSilicon PQ library redistributed unmodified, and is neither ours nor MIT. Declaring plain MIT would have quietly extended that licence over a proprietary blob. Also installs iq-profiles/cv610-bin/imx662.bin to /etc/sensors/ as an IQ restore point. Deliberately NOT named by the default config: importing a tune at boot is opt-in, and this one only reproduces the sensor plugin's own cold-boot state. A craft without the blob still boots -- the import warns and no-ops, and since 0.81.0 it also advertises neither isp.sensorBin nor routes.iq_export_bin, so the control surface matches what it can serve. Verified on a CV610 bench by moving the library aside. Claude-Session: https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e craft, down (#122) Follow-up to #120. Closes the shared half of #121 plus a worse failure found while testing it. - output_socket_configure() no longer closes a working socket when the destination fill fails; the call is now all-or-nothing. - A bad outgoing.server at boot no longer brings the craft down: the output comes up inert, transport/status reports active:false, and the operator recovers it live over the API. - The inert start re-seeds the transport policy the teardown clears, so a recovery cannot silently apply unconnected UDP or drop the Unix stall allowance. Device-verified on CV610, Star6E and Maruko. Merge commit rather than squash so the fork stays in sync. Gap A and Gap B in #121 remain open by design: both are bounded to one dropped or split access unit during an operator-initiated retarget, and Gap B's fix changes observable API behaviour on one backend only.
1 participant
Summary
Brings the fork to parity with upstream: 33 commits, no divergence.
upstream/masteris a direct ancestor of this branch, so there is nothing to reconcile —git rev-list --count master..upstream/masteris 0.The work is almost entirely the CV610 (Hi3516CV610 + IMX662) backend, which upstream already carries in skeleton form, plus a few shared fixes that apply to all three backends.
VERSION0.74.0 → 0.81.0,contract_version0.24.0 → 0.29.0.CV610 gains
CLOCK_MONOTONICforcapture_us;qpandframes_since_idrpopulatedimage.mirror/image.fliphonoured at the sensor, matching where both SigmaStar backends apply itfpv.roiQpbounded to ±20, where the delta still reaches the encoderoutgoing.server/outgoing.enabledretarget without regressing the ring transports; the audio side channel follows a live retargetisp.gainMax/isp.shutterMaxUsreach the ISP, in the fleet-wide units.binimport viaisp.sensorBin, and a newGET /api/v1/iq/export_binShared
roiQp/maxQpinteraction documented.qpfield is documented as CV610-only: the SigmaStar encoder populates onlyrefTypein its H.265 stream-info struct, sostartQualreads 0 on device. Reading it into the trailer was tried and shipped zeros.The
.binwork, since it is the largest pieceThe vendor
libbin.somoves the whole ISP parameter image at once, which is what PQTools writes. The package available to us ships the library but notot_pq_bin.h, so the call contract was recovered from the stripped blob; every offset cited in the comments is a disassembly finding rather than a guess from a related chip's header.A
.binis locked to the chip register map and the SDK ISP version, not to the sensor. Import validates an address/size walk against what the running chip's ISP reports and carries no sensor identity at all.Device evidence, on a Hi3516CV610 + IMX662 bench:
OT_PQ_GetISPDataTotalLen()= 143424, matching vendor tunes in layout. Its header reports the live mode./api/v1/iqreads back across ten ISP groups. The picture goes red, which is the expected result for a foreign tune's CCM and AWB, not a defect..binis integrity-checked: one flipped byte returns0xcb000005, so files cannot be edited or spliced in place.Two memory-safety defects were found in review and fixed before merge, both from trusting the vendor's size answers — a 37-byte heap overflow on export when the 3DNR size query fails, and a ~1310-byte out-of-bounds read on import from a validly-headed but truncated 3DNR section. Details are in the commit message.
Packaging note for image builders
isp.sensorBinand/api/v1/iq/export_bindlopen("libbin.so"). That blob is not committed here — its licensing is the vendor's — somake stage SOC_BUILD=cv610copies it fromCV610_PQ_LIBand prints a NOTE naming the path when it is absent. A craft without it boots normally; both features warn and no-op.iq-profiles/cv610-bin/imx662.binis an export of a reference bench's cold-boot state, kept as a restore point. It is explicitly not a measured tune — nobody has run PQTools against this sensor yet — and it follows the existingiq-profiles/maruko-bin/precedent.Validation
make buildclean on all three backends (star6e,maruko,cv610),make verifypasses.make test-werror: 3077 pass, 0 fail.qpclarification.🤖 Generated with Claude Code
https://claude.ai/code/session_01GwG8Jmv3daUQgZhMYyz3wB