fix(venc): stop requesting an IDR on every bitrate write

[vertexodessa]

Summary

apply_bitrate() on Star6E and Maruko requested an IDR after every rate change "so the decoder resyncs against the new rate-control state". The decoder needs no resync: the rate controller absorbs a mid-GOP rate change, and the frame-shm throttle clamp has relied on exactly that since it shipped, re-programming the encoder as often as every 200 ms through the want_idr=0 path.

The forced IDR was pure cost, and on the bench link the dominant latency-spike source. An IDR is the largest frame the encoder can emit (~42 KB at 10 Mbps, measured 2026-08-22), so every adaptive-ladder or congestion-control bitrate write that cleared the 100 ms IDR gate turned rate control into an IDR train. The E0 capture's long-standing "IDR burst every ~19 pictures" is two gate windows at 90 fps.

Behaviour

• video0.bitrate writes via /set and /live/set no longer request an IDR on Star6E and Maruko. apply_bitrate_ex(kbps, want_idr) collapses into apply_bitrate(kbps); the throttle path calls the same function.
• A controller that wants a resync point calls /api/v1/idr (or /request/idr) explicitly, as before.
• CV610 never IDR'd on bitrate; /api/v1/dual/set keeps its own dual_apply_bitrate() behaviour.
• Contract 0.18.2 -> 0.18.3 (behavioral), changelog entry in HTTP_API_CONTRACT.md; VERSION 0.65.2 -> 0.65.3, HISTORY.md entry.

Verification

• make test: 2479 passed, 0 failed
• make build SOC_BUILD=star6e and SOC_BUILD=maruko: clean
• Running on the bench drone (Star6E, imx415 1472x816@120, H.265 CBR 10 Mbps) since 2026-08-22 08:56 in a local 0.66.0 build, alongside maxIpProp=2: quiet-link glass-to-glass p50 62-72 ms, no IDR trains on ladder writes.
• Maruko: same change, compile-tested only.

Independent of #110 and #111 (all three bump VERSION/HISTORY.md/contract from the same base); whichever lands later needs a trivial rebase of those three files.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Stop forcing IDR frames on Star6E/Maruko bitrate writes

🐞 Bug fix 📝 Documentation ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Remove implicit IDR requests triggered by video0.bitrate writes on Star6E/Maruko.
• Keep IDR generation explicit via /request/idr (or /api/v1/idr) when callers need resync
 points.
• Bump app + contract versions and document the behavioral contract change.

Diagram

graph TD
  C(("Controller")) --> S["/api/v1/set & /api/v1/live/set"] --> L["venc_api: LIVE_GROUP_BITRATE"] --> B["Star6E/Maruko apply_bitrate()"] --> E[("Encoder channel")]
  C --> I["/request/idr (/api/v1/idr)"] --> R["request_idr() callback"] --> E
  C --> D["/api/v1/dual/set (Star6E)"] --> E
  subgraph Legend
    direction LR
    _act(("Caller")) ~~~ _api["HTTP endpoint"] ~~~ _cb["Backend callback"] ~~~ _enc[("Encoder")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep implicit IDR but only on large bitrate deltas

• ➕ Maintains a single-call "bitrate+resync" behavior for some clients
• ➕ Could reduce perceived artifacts if any decoders actually require resync
• ➖ Still produces latency spikes under adaptive ladders during congestion
• ➖ Delta threshold is heuristic and risks regressions across codecs/modes

2. Add a config knob: `idr_on_bitrate_write` (default off)

• ➕ Allows opting back in for edge deployments/decoders without code changes
• ➕ Makes the behavioral change reversible per system
• ➖ Adds API surface and testing matrix for a behavior that is expected to be unnecessary
• ➖ Risk of clients enabling it and reintroducing IDR-train latency issues

3. Debounce/coalesce bitrate writes and emit at most one IDR per window

• ➕ Mitigates IDR trains while preserving auto-resync semantics
• ➕ Reduces encoder reprogramming churn under very chatty controllers
• ➖ Still couples RC changes to IDR emission (cost remains)
• ➖ Adds timing/queueing complexity and complicates controller behavior expectations

Recommendation: Prefer the PR’s approach: make bitrate writes purely RC updates and keep IDR as an explicit operation via /request/idr//api/v1/idr. It removes a high-cost side effect from a frequently-written control, avoids latency spikes/IDR trains under adaptive bitrate activity, and preserves resync capability for callers that actually need it.

Files changed (6) +49 / -43

Bug fix (2) +17 / -40

maruko_controls.cMaruko: remove want_idr path from bitrate apply +6/-16

Maruko: remove want_idr path from bitrate apply

• Collapses 'maruko_apply_bitrate_ex(kbps, want_idr)' into 'maruko_apply_bitrate(kbps)' and removes the post-bitrate-change IDR request. Updates the output-throttle path to call the same bitrate apply function, ensuring throttle-driven rewrites also avoid IDRs.

src/maruko_controls.c

star6e_controls.cStar6E: stop requesting IDR on bitrate changes +11/-24

Star6E: stop requesting IDR on bitrate changes

• Replaces 'apply_bitrate_ex(kbps, want_idr)' with a single 'apply_bitrate(kbps)' implementation and deletes the forced-IDR logic after 'MI_VENC_SetChnAttr'. Updates the output-throttle clamp to reprogram bitrate via the same no-IDR path.

src/star6e_controls.c

Documentation (2) +30 / -1

HISTORY.mdDocument 0.65.3: no forced IDR on bitrate writes +17/-0

Document 0.65.3: no forced IDR on bitrate writes

• Adds a 0.65.3 release entry describing removal of forced IDRs on bitrate changes and the resulting latency improvement. Notes the contract version bump from 0.18.2 to 0.18.3 and scopes the behavior to Star6E/Maruko.

HISTORY.md

HTTP_API_CONTRACT.mdBump contract to 0.18.3 and record behavioral change +13/-1

Bump contract to 0.18.3 and record behavioral change

• Increments 'contract_version' to 0.18.3 and adds a changelog entry stating that 'video0.bitrate' writes no longer request an IDR on Star6E/Maruko. Clarifies that callers must explicitly hit the IDR endpoint when a resync point is desired; CV610 and '/api/v1/dual/set' remain unchanged.

documentation/HTTP_API_CONTRACT.md

Other (2) +2 / -2

VERSIONBump application version to 0.65.3 +1/-1

Bump application version to 0.65.3

• Updates the top-level VERSION file from 0.65.2 to 0.65.3 to match the documented release entry.

VERSION

venc_api.cReport contract_version 0.18.3 from /api/v1/version +1/-1

Report contract_version 0.18.3 from /api/v1/version

• Updates the '/api/v1/version' JSON payload to return 'contract_version' 0.18.3, aligning runtime reporting with the contract/documentation change.

src/venc_api.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[snokvist]

The bitrate-control code change is correct in scope, but this needs factual/documentation corrections before merge.

Required corrections:

1. /api/v1/idr does not exist. The registered ch0 endpoint is /request/idr; /api/v1/dual/idr is ch1-specific and /api/v1/idr/stats is read-only. Please remove /api/v1/idr from HISTORY.md, the contract change log, and the PR text (or add and test an alias, though that is not needed for this fix). This was also confirmed on the exact PR binary: GET /api/v1/idr returned 404 and GET /request/idr succeeded.
2. The contract's /api/v1/dual/set table says a ch1 bitrate write issues an IDR, but dual_apply_bitrate() only updates MI_VENC_ChnAttr; it does not request one. Since this PR explicitly describes dual behavior, please correct that row to say no implicit IDR and point to /api/v1/dual/idr for an explicit request.
3. The /api/v1/idr/stats example still reports min_spacing_us: 250000, while IDR_RATE_LIMIT_MIN_SPACING_US is 100000. Please correct this directly related contract drift.
4. Please narrow the measurement claims. ~42 KB at 10 Mbps is one measured frame in the stated bench setup, not “the largest frame the encoder can produce.” Likewise, the latency run used a local 0.66.0 build alongside maxIpProp=2, so it does not isolate this patch as the dominant cause or prove that it explains the E0 anomaly. It is fair to say the code previously converted sufficiently spaced bitrate writes into forced IDRs, that this patch removes those requests, and that the mixed bench run was consistent with the expected improvement.

Independent exact-head device check performed here:

• Commit: 11dcabbf2abcda5987f3b48502f6eeb80768d3b9
• Binary SHA-256: 88f60e8b160c21e768e466ce2e9166e81f6ee8a7788fadb6a49902fc200841d6
• Device: Star6E SSC338Q, IMX335, 1920x1080@60, H.265 CBR, frame-shm://
• Ten volatile /api/v1/live/set?video0.bitrate=... writes, spaced 150 ms apart: all returned 200; ch0 IDR counters stayed exactly honored=8, dropped=9; framesSent advanced 1804 -> 1911; transport drops stayed at 12.
• Control: a later /request/idr advanced honored=8 -> 9.
• No visual/decoder capture was performed. Maruko was unreachable, so it remains compile-tested only here. CV610 is unchanged by the patch.

The existing make test result is valid for the covered suite, but the host test binary does not link star6e_controls.c or maruko_controls.c, so 2479/0 does not itself test the removed IDR calls. The device counter test above directly covers Star6E; please describe the unit result accordingly.

Related IDR audit (not a blocker to keeping this PR bitrate-focused):

• video0.qpDelta and video0.maxIBytes/maxPBytes still force IDRs on both Star6E and Maruko after changing RC parameters. They are the same decoder-neutral class as bitrate/QP bounds and should be removed in a focused follow-up unless a separate device requirement is demonstrated.
• Keep IDRs for live FPS rebinds, output re-enable/new destination, reference-chain-breaking output drops, recording start, explicit IDR endpoints, and the opt-in scene detector; those have stream-recovery or explicit-policy reasons.
• Star6E output disable currently calls apply_fps(idle), which requests an IDR after output has already been disabled; output enable then calls apply_fps(restored) and requests another IDR. That nested behavior deserves a separate targeted fix so disable emits none and enable emits exactly one reliable recovery IDR.

After the factual corrections, I consider the bitrate code merge-ready. It applies cleanly to the current fork's Star6E/Maruko control files; version/history/contract integration will require manual conflict resolution because the fork is already at 0.67.0.

[snokvist]

Additional code-audit correction: the output-toggle issue exists on both backends and is more severe on Maruko.

At this PR head, maruko_request_idr() calls maruko_mi_venc_request_idr() directly, without idr_rate_limit_allow(). Therefore Maruko's /request/idr callback is neither rate-limited nor represented in /api/v1/idr/stats, unlike Star6E and CV610. maruko_apply_fps(), output enable/server change, and Maruko recorder-start paths also issue raw requests. In particular:

• output disable calls maruko_apply_fps(idle), which requests an IDR after output has been disabled;
• output enable calls maruko_apply_fps(restored) and then requests another IDR directly, so two back-to-back requests are possible;
• the contract's statement that IDR sources are coalesced and counted equivalently on both SigmaStar backends is not true for these Maruko paths.

Star6E has the same nested disable/enable structure, but its shared gate normally coalesces the duplicate; that also means a mandatory output-enable/new-destination recovery IDR can theoretically be swallowed by an unrelated request inside the 100 ms window.

This is pre-existing and need not expand the bitrate patch, but it should become a focused follow-up. The clean policy split is:

• RC-only writes (bitrate, QP delta/bounds, frame-size caps): no implicit IDR;
• advisory/storm-prone requests (scene detection, ring recovery, HTTP): paced and counted;
• recovery-required transitions (output re-enable/new receiver, recording start after missing references): exactly one reliable IDR, without the nested FPS request.

Until that follow-up lands, please also avoid/repair contract language claiming that all listed Maruko IDR sources go through the shared limiter and appear in its stats.

[snokvist]

Heads-up: #114 (parity sync v0.65.2 → v0.67.1, 54 commits) is open and will conflict with this one on bookkeeping.

This PR claims VERSION 0.65.3, which #114 already uses. Once it lands the next free version is 0.67.2, so this will need its VERSION and HISTORY.md entry renumbered, and its contract bump re-cut from 0.18.3 to 0.18.7 (#114 takes the contract to 0.18.6).

The code itself doesn't overlap much — #114 leaves star6e_controls.c/maruko_controls.c untouched, so the substantive change should rebase cleanly.

Sorry for the churn — this bundle had been accumulating on the fork for a while. Happy to help with the rebase if useful.

[snokvist]

#114 is merged, so this now shows as conflicting. Overlap is four files: VERSION, HISTORY.md, documentation/HTTP_API_CONTRACT.md and src/venc_api.c.

src/star6e_controls.c and src/maruko_controls.c — where the actual change lives — are untouched by the sync, so the substance rebases clean. The bookkeeping to re-cut: VERSION → 0.67.2, HISTORY.md entry to the top as ## [0.67.2], and the contract bump from 0.18.3 → 0.18.7 (the sync took it to 0.18.6, and there are four sites: the header near line 21, the example body, the divergence note, and the string in venc_api.c).

Worth flagging: the sync added a 1 s holdoff on the ring-full recovery IDR with honored-aware anchor rollback (venc_frame_drop_idr_due()). It is a different IDR source from the bitrate-write one you are removing, so the two are complementary rather than conflicting — but you may want to re-measure your IDR-train numbers on top of it.

[vertexodessa]

Note: this comment was written by Claude (Anthropic's Claude Code), driving a debugging session on a bench rig. Numbers below are measured on hardware; the mechanism is my analysis and I'd welcome a sanity check from someone who knows the Star6E SDK's SetChnAttr semantics firsthand.

TL;DR

This PR removes the explicit MI_VENC_RequestIdr(), but bitrate writes on Star6E still IDR the link — because apply_bitrate() programs the rate via MI_VENC_SetChnAttr, and on this SoC re-setting the channel attribute itself emits an IDR. The throttle path was already want_idr=0 before this PR, yet it still produces a keyframe on every clamp change. So the "IDR burst every ~19 pictures" this PR mentions survives the change.

Setup

• Running a local 0.66.0 build that contains this PR (/api/v1/version → app_version: 0.66.0, contract_version: 0.18.3).
• Star6E, H.265 CBR, 1280x720@120, ~11 Mbps commanded.
• The frame-shm ring is drained by a downstream consumer that reads in bursts (its read is gated by a radio TX queue), so fillPct square-waves between 0 and 100% — which keeps the shm-throttle continuously active.

Observation

At the receiver, real IDRs (HEVC NAL 19/20) arrive at 6.5/sec, spaced ~200 ms, each 46–70 KB. This rate is independent of GOP (tested 2 s and 4 s), resilience preset (racing / patrol / off), scene detection (off), intra-refresh (off), and receiver-side IDR requests (≈0). So it is neither GOP, nor intra, nor request-driven.

/api/v1/transport/status sampled every ~250 ms shows the throttle never settling:

fillPct:  75   0 100   0 100  12   0 100   0   0 100   0
permille:257 204 304 282 199 299 349 289 273 213 313 287   ← changes every window, never repeats

VENC_SHM_THROTTLE_WINDOW_US = 200000 (200 ms) — which matches the ~200 ms IDR spacing exactly.

Mechanism

star6e_controls_set_output_throttle() → apply_bitrate(), and apply_bitrate() ends in MI_VENC_SetChnAttr(). The runtime calls set_output_throttle() every window the permille changes (star6e_runtime.c: if (want != g_applied_permille) set_output_throttle(want)). With the ring oscillating, permille changes every window → SetChnAttr every ~200 ms → one IDR each. This path is want_idr=0 and never called the explicit MI_VENC_RequestIdr() that this PR removes, so the removal can't affect it.

I suspect the "E0 capture's IDR burst every ~19 pictures ... two gate windows at 90 fps" noted in the PR description is this same effect: 19 pictures @ 90 fps ≈ 211 ms ≈ one 200 ms throttle window, i.e. one SetChnAttr per window, not two 100 ms IDR-gate windows.

Confirming test

Toggling outgoing.shm_throttle=false live pins throttlePermille at 1000, so set_output_throttle() (and thus SetChnAttr) stops being called:

	throttle ON	throttle OFF
IDR rate (receiver)	6.5 /sec	0.2 /sec (= the 4 s GOP)
IDR spacing	~200 ms	4.04 s
glass-to-glass	~100–143 ms	~15–37 ms

The keyframes collapse to exactly the GOP, confirming the throttle's bitrate write is the sole extra source. (The large latency drop is a side effect: the forced 55–70 KB IDRs were themselves backing up the ring, which drove the throttle to clamp harder — a self-reinforcing loop.)

Why the PR's bench didn't catch it

The verification was on a quiet link ("no IDR trains on ladder writes"). On a quiet link the ring doesn't back up, the throttle stays at 1000, and set_output_throttle() is never called — so the SetChnAttr path is never exercised. The bug only appears once the throttle actively engages.

Suggested follow-up

Change the CBR bitrate without a full SetChnAttr channel re-init — e.g. via a rate-only MI_VENC_SetRcParam path if the SDK carries the target bitrate there, or whatever the SoC's IDR-free rate update is. Failing that, deadband the throttle so it doesn't reprogram every window. Either lets the throttle do its job without keyframing the link.

How the loop works (intuitive)

It's a control loop whose actuator has a self-defeating side effect:

1. The consumer drains the ring in bursts. At 120 fps the downstream reader is gated by a radio TX queue — it empties the ring, stalls on TX, empties it again. fillPct square-waves 0↔100%.
2. The throttle reacts to that oscillation by changing its bitrate clamp (permille) every 200 ms window. It never settles, because a square-wave fill signal is not something an AIMD loop can converge on.
3. Each clamp change applies the new bitrate via SetChnAttr, which emits an IDR. On this SoC, the only way the throttle can slow the encoder also injects a keyframe.
4. That IDR is the largest frame the encoder makes (~10× a P-frame), so it deepens the ring backup — which makes the throttle clamp again → another SetChnAttr → another IDR.

The sting: the throttle exists to relieve ring pressure, but its actuator (bitrate-via-SetChnAttr) injects a giant frame that makes the pressure worse. That's positive feedback where negative was intended — so it never converges, and ~20 % of the bitrate goes to keyframes nobody requested. Disabling the throttle breaks the loop entirely (frames stay small → ring stops oscillating → nothing to throttle); making the bitrate write IDR-free turns it back into a proper negative-feedback loop with the throttle still doing its job.

Happy to run more measurements on the rig if useful.

[snokvist]

Thanks for the follow-up analysis — you were right, and I was wrong in my earlier review.

I took this to a Star6E bench (SSC338Q, IMX335 1920x1080@60, H.265 CBR, GDR via the racing preset) and counted IRAP access units in the encoder's own bitstream rather than trusting /api/v1/idr/stats. Ten live video0.bitrate writes spaced 300 ms:

binary	idr/stats honored Δ	IRAP AUs
stock	+11	11
this PR	+1	11

MI_VENC_SetChnAttr emits an IDR by itself. So this PR's removal is a no-op on the wire, and the IDR trains survive it — exactly your point. My earlier "merge-ready on the code" was based on counters alone, and I explicitly noted I'd done no decoder capture; counters only see requests, and an SDK-implicit IDR is invisible to them. That was the wrong instrument and I should have caught it.

Two further data points that shape the fix:

• The IDR follows the value change, not the call: ten writes of the same bitrate produced 1 IRAP, not 11. So a "skip redundant write" guard buys nothing — the lever is fewer distinct programmed rates.
• MI_VENC_RcParam_t carries no bitrate field on either SigmaStar backend, so the rate-only SetRcParam path you suggested isn't available. SetChnAttr is the only rate actuator. SetRcParam/SetRcPriority are measured IDR-free, though — qpDelta and maxIBytes writes went 11 → 1.

Your deadband suggestion was the right call, and it's in. I've opened #116, which rebuilds this on the current tip (your commit cherry-picked, authorship intact) and adds: the same removal for qpDelta/maxIBytes, the throttle deadband, the ring-full recovery IDR made opt-in, the nested output-toggle double IDR fixed, and Maruko's ungated /request/idr routed through the shared limiter.

On your ~200 ms observation: note the ring-full recovery IDR was un-paced on the 0.65.2 base this PR was cut from. A 1 s holdoff landed in 0.66.0, so on current master that source is bounded to ~1/s — which is probably why your rate sat above one-per-throttle-window. #116 turns it off by default entirely.

This PR is superseded by #116, but the substantive commit there is yours. Happy to have you review it — particularly the deadband constant, since you have a rig that actually drives the throttle into oscillation and I had to induce ring pressure by stopping the consumer outright.