Rotate hevc recordings, and stop forcing a keyframe to do it - #125
Conversation
A 32-bit glibc build leaves off_t 32-bit and open() without O_LARGEFILE, so
the kernel refuses any write crossing 2^31-1 with EFBIG no matter what the
filesystem allows. A recording on exFAT died at exactly 2147483647 bytes,
twice, and reported "write_error" -- which reads as bad media and sent the
diagnosis to the SD card. `record.maxMB` above ~2047 was therefore a knob
that validated, applied over MUT_RESTART, and then silently could not work.
Build with -D_FILE_OFFSET_BITS=64. That is the whole fix: it removes the
ceiling from open(), and equally from the stat()/fstat() calls in
venc_recordings.c and venc_httpd.c that return EOVERFLOW for a >2 GB file
and would otherwise hide such a recording from the listing and the download
path. The flag has to be on every translation unit or the link silently
mixes two off_t layouts, so it goes in COMMON_CFLAGS and is mirrored into
HOST_CFLAGS to keep the tested ABI equal to the shipped one. Note that a
CFLAGS change does not invalidate objects: this needs a clean build.
The rest is so the knob cannot lie again if that flag is ever lost:
- check_rotation() rotates on whichever binds first, the operator's limit or
the one off_t can reach. A configured max_mb above the ceiling now yields
more segments instead of a dead recorder. Inert at 64-bit off_t.
- A segment can only be cut on an IRAP, and the IDR request is bounded, so a
GDR stream can carry one past any threshold with no cut coming. Rather
than walk into an EFBIG that truncates mid-AU, stop on the frame boundary
with the file intact.
- EFBIG gets its own arm in all three recorders and its own stop reason,
RECORDER_STOP_SIZE_LIMIT ("size_limit"), because nothing failed -- a
ceiling is not an I/O error and should not read as one.
- The inactive branch of the record status filled in only stop_reason, so a
recorder that stopped by itself answered {path:"", frames:0, bytes:0}. It
now carries the path, bytes, frames and segments from the snapshot the
reason came from -- for a non-manual stop that is the entire diagnosis.
contract_version 0.29.0 -> 0.30.0: the stop_reason enum gains a value and the
inactive status payload changes meaning. Both are additive under the
contract's own governance rules. VERSION 0.82.0 -> 0.83.0.
Docs: maxSeconds/maxMB of 0 were documented as "no limit" but the runtime
only overrides its compiled-in default when the value is > 0, so 0 means
300 s / 500 MB. Also state that rotation is TS-only; format "hevc" ignores
both thresholds, which is filed separately.
Device-verified on SSC338Q (.232) against the real recorder core -- three
arms of one harness, same 58 GB FAT32 card, same deterministic input, only
the build differing:
stock master, no flag EFBIG "File too large" at 34084 frames
file on disk exactly 2147483647 -- the report,
reproduced byte for byte
patched, no flag segment cut at 2147212672, second segment opened,
2306867536 bytes over 2 segments, exit 0
patched + flag 2306867536 bytes in ONE segment, exit 0
The last two wrote identical totals from identical input, so segmentation is
the only variable between them. Note the stock arm's counter stopped at
2147464968 while the file is 2147483647: the failing write was partial, which
is the mid-AU truncation the new frame-boundary stop exists to prevent.
Separately verified on .232 that a 32-bit off_t build cannot stat() or fstat()
a 3 GB file at all (EOVERFLOW), so fixing only open() would have produced
recordings the API then hides.
CV610 was checked and is not affected: measured on .181, musl is 64-bit off_t
and forces O_LARGEFILE regardless of the flag. That negative comes from a
probe whose raw-openat control DID fail with EFBIG on the same kernel, so the
instrument was shown able to detect the positive before its negative was
trusted.
Fixes the root cause reported in OpenIPC#118.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1w7gRc6ASpa7P7HRCBQEw
PR Summary by QodoRotate raw HEVC recordings by configured limits
AI Description
Diagram
High-Level Assessment
Files changed (18)
|
Code Review by Qodo
1. Merging also ships another feature
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| # already 64-bit off_t and ignore this; it is here for all of them because | ||
| # the flag must be identical across every translation unit -- a partial | ||
| # application silently mixes two struct stat/off_t layouts across a link. | ||
| COMMON_CFLAGS := -Os -Iinclude -Ilib -DVENC_VERSION=\"$(VENC_VERSION)\" -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -MMD -MP |
There was a problem hiding this comment.
1. Merging also ships another feature 📘 Rule violation ⚙ Maintainability
COMMON_CFLAGS adds 64-bit file offsets and the same diff adds size-limit status semantics from stacked change #124, although the PR description says only the HEVC rotation commit belongs here. Merging this branch before the required rebase would deploy large-file and API behavior beyond issue #123 alongside rotation.
Agent Prompt
## Issue description
The PR includes the unrelated large-file support and status-contract changes from stacked change #124.
## Issue Context
The PR description states that only the HEVC rotation commit belongs to this PR and requires rebasing after #124 is squashed into the target branch.
## Fix Focus Areas
- Makefile[62-75]
- documentation/HTTP_API_CONTRACT.md[1277-1299]
- HISTORY.md[30-83]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| fprintf(stderr, "[recorder] segment %u: %s\n", | ||
| state->segments, state->path); |
There was a problem hiding this comment.
3. Segment progress is sent as an error 📘 Rule violation ◔ Observability
open_next_segment() writes its successful segment announcement with fprintf(stderr, ...) instead of sending ordinary progress to stdout. Every successful raw recording rotation reaches this statement, so scripts and operators monitoring the error stream receive normal progress mixed with failures.
Agent Prompt
## Issue description
A successful segment-opening message is emitted on stderr even though it is informational output.
## Issue Context
Error streams must remain distinguishable from normal recorder progress for callers and monitoring scripts.
## Fix Focus Areas
- src/star6e_recorder.c[350-351]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
5cf5213 to
949cd64
Compare
snokvist
changed the title
949cd64 to
faa3bb1
Compare
Two defects, one of them pre-existing and the more interesting.
`record.format="hevc"` ignored maxSeconds and maxMB completely -- the raw
recorder had no rotation code at all, no segment counter, no threshold check,
no second open(). A raw recording was one file that grew until the card
filled, and it did so silently: the config validated and /api/v1/get echoed it
back. It is also why the "lower maxMB" workaround for the 2 GB ceiling did
not generalise; on that path there was no threshold to lower.
The second is that rotation used to ASK the encoder for a keyframe when a
threshold was crossed and none was coming (TS recorder, since 0.70.0). That
works, but an IDR is a large frame and one per segment raises the bitrate the
link has to carry. On an intra-refresh craft it undoes exactly what the mode
exists for, and under record.mode=mirror the recorder taps the LIVE channel,
so the spike went out over the air for the benefit of a file. The entire ask
-- grace period, 1 Hz pacing, bound, and the take/requeue hand-off at six
sites across three backends -- is deleted.
A segment now opens on a point the stream already produces:
- an IRAP (19/20) where one exists. Normal GOP recording is unchanged and
still cuts on its keyframe. Measured at resilience=off, 601 frames: 4
parameter-set groups, 4 IRAP access units, 0/4 groups detached from an IDR
-- so accepting parameter sets cannot move a normal-GOP cut off its IRAP.
- a parameter-set boundary (32/33/34), the head of a refresh wave, which is
what an intra-refresh stream emits once per GOP. A raw elementary stream
has no container to hold codec config, so this is also the only place a
.hevc segment can begin and still decode; the picture converges over one
wave, as the ground already does on every tune-in. Measured at
resilience=racing, 800 frames: ONE IRAP at startup and never another, so
this is the only boundary that ever arrives there.
When to cut is shared rather than copied: RecorderRotation in
star6e_recorder.h holds it once and both recorders embed one, each supplying
only its own open/close.
Also fixed, all found in review of the above:
- Maruko's own SDK-typed writer bypassed rotation entirely, so format="hevc"
on its dual and synchronous-fallback paths would still have grown one
unbounded file. It now takes the same shared cut, and Maruko's TS adapter
accepts parameter-set boundaries too -- without which rotation stayed inert
on an intra-refresh craft there.
- Segments opened O_TRUNC on a name carrying only uptime seconds plus 16 bits
of nanosecond clock. After a reboot the uptime restarts, so a name can
repeat and the open destroyed whatever was there. Now O_EXCL with a retry,
at start as well as on rotation; rotation multiplied the exposure because it
is one name per segment rather than one per recording.
- fdatasync() and close() results were discarded when finalising the old
segment, so a delayed write surfacing there was reported as a clean
rotation. Both are checked; a failure stops the recorder like a failed
reopen.
- `segments` is reported on the raw path. Two backends left it 0 while
segments were on disk and Maruko hardcoded 1.
Device-verified on Star6E (SSC338Q, resilience=racing, gopSize 2.0,
sliceCount 6, 100 fps) with maxMB=2, run from the SD card so the stock service
and config were restored untouched:
10 segments in 20 s; status segments:10 against 10 files, matching
every segment opens VPS, SPS, PPS
every segment holds exactly 1206 slice NALs = 201 frames x 6 = one wave
IRAP NALs across the sampled segments: 0
Zero IRAPs is the point: rotation runs entirely on boundaries the encoder was
producing anyway. Before this change the same craft produced one file.
NOT device-verified: the Maruko writer. That backend's bench has no storage
meeting the recorder's 50 MB free-space precondition (1 MB free, no SD card),
so its fix rests on calling the same shared cut that is verified on Star6E,
plus a pack scan mirroring the accessor Maruko's TS adapter already uses in
production.
Operator note: on an intra-refresh craft the cut point arrives once per GOP,
so segment granularity IS one GOP -- a maxMB far below one GOP of data still
yields one-GOP segments. If a stream produces neither kind of point, rotation
waits rather than forcing anything, and logs that it is waiting.
contract_version 0.30.0 -> 0.31.0, VERSION 0.83.0 -> 0.84.0.
Closes OpenIPC#123.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1w7gRc6ASpa7P7HRCBQEw
faa3bb1 to
aba48e7
Compare
|
Qodo review addressed. Seven findings: three were already resolved by the Note on ordering: the review ran against 2. Maruko recordings still grow unbounded — real, and the most important one
6. A rotation can erase an older segment — real, fixed. The name carries 7. Rotation hides segment write failures — real, fixed. 3. Segment progress is sent as an error — not taking this one. The rule is 1. Merging also ships another feature — correct, and already disclosed at One thing I want to be explicit about: I added a test for finding 6 and it
Not device-verified: the Maruko writer. That bench has no storage meeting |
Review of the two commits below found a regression introduced by extracting
the TS write path into ts_write_muxed(): the `segment_bytes += written` that
lived beside the write was not carried across, so the counter stayed pinned at
the PAT/PMT bytes a segment opens with.
Two consequences, both silent:
- record.maxMB never fired for format="ts" -- the default recording path on
every backend -- because the threshold is tested against that counter;
- the new write-failure rollback truncates to that counter, so an ENOSPC or
EFBIG would have cut the whole segment back to its ~376-byte header
instead of to the last complete frame. Strictly worse than the bug the
rollback was added to fix.
It shipped green because every rotation test set max_bytes to 1, which the
header alone already satisfies, and the rollback assertion compared the file
size against the same frozen counter ftruncate had just used -- tautological.
The new test crosses a 32 KB threshold by accumulation instead, and fails on
both counts if the increment is removed again.
Device-verified on Star6E against the SD card, max_mb=20 over 100 MB: five
segments of 20980988 bytes each. Before the fix, one file of 100 MB.
Also from the same review:
- runtime_rotate_idr_on() lost its last caller when the IDR ask was deleted;
a static function with no callers fails `make lint`, which is -Wall
-Werror. make lint now runs clean on all three backends -- it is a gate I
had not been running.
- a rotation that cannot reopen because the card filled reports disk_full
rather than a write error; the space check only runs every 300 frames, so
ENOSPC genuinely arrives at the segment open().
- maruko_recorder.c stopped itself without clearing `recording`, so the
status reported a stopped recording as active indefinitely.
- star6e_recorder_write_frame() still said "this format does not rotate" on
EFBIG, which stopped being true when this branch gave it rotation.
- the raw recorder now clears rotation_due_since/warned_no_cut_point on
start, as the TS one already did, and takes the status lock around
segment_bytes like the other two writers.
- two comments describing the deleted IDR-ask machinery.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1w7gRc6ASpa7P7HRCBQEw
Verification pass over the three commits below. The fixes that landed in cdfa1f2 were correct but each covered only one of two symmetric cases, and several new assertions could not fail. Code: - A TS rotation that could not reopen still reported write_error when the card filled -- the defect fixed on the raw path in cdfa1f2, left in place on the one that matters more, since format="ts" is the default. Both now classify ENOSPC as disk_full, and both preserve errno across the diagnostic. - maruko_recorder.c check_disk_space() stopped without clearing `recording`. cdfa1f2 fixed the stop_with_error() half of the same file; this is the other exit, reachable in dual mode when free space drops mid-recording. Left set, the status reports a closed recording as active for the rest of the process. - cv610_rotate_idr() lost its last caller when the IDR ask was deleted, exactly as runtime_rotate_idr_on() did. Worth noting `make lint` cannot see either: it is -fsyntax-only, which suppresses -Wunused-function. `make test-werror` does compile these files with -Wall -Wextra -Werror and is the gate that catches it. - The cut-point byte scan looked only at the FIRST NAL of an access unit while the SDK-pack scanners looked at all of them. An access unit that leads with an AUD or a prefix SEI would have left rotation inert on the mirror path while the synchronous path rotated normally. Now scans the whole unit via h26x_util_annexb_next(), which also removes the hand-rolled NAL parsing this had duplicated in two files. - write_stream() went back to setting is_idr for IRAP only. That value also becomes the TS random_access_indicator, so widening it to parameter-set boundaries was an unintended wire-format change; rotation never needed it, because write_video() scans the access unit itself. - A start refused for low space published the PREVIOUS recording's path and counters next to stop_reason "disk_full", reading as though that file had filled the card. Now cleared with the reason. - maruko_runtime.c never named the format on the inactive status branch, so it answered a fully populated record with "format":"" where the other two backends answer ts/hevc. - Four comments describing the deleted IDR-ask machinery, and the SD_CARD_RECORDING rotation steps still opening with "waits for the next IDR". Tests -- four assertions that could not fail, all confirmed by mutation: - Two "did not cut on a non-cut-point" checks passed because the threshold had not been crossed yet, so the cut-point gate was never consulted: forcing the gate open failed zero raw assertions. Primed, they now fail 4. - "noerase rotated" compared a loop-controlled counter with its own bound. It now asserts the recorder cut once per captured path; disabling rotation entirely now fails 18 assertions where it used to fail 17. - "efbig rolled back to the frame boundary" compared the file against the same counter ftruncate had just used as its target, so any wrong target was self-consistent -- it stayed green through the exact regression cdfa1f2 fixed. It now compares against a byte total accumulated from write_video() return values, and fails when that increment is removed. - The restart test asserted a warning latch it never raised. It raises it now, and removing the reset fails 2 assertions instead of 0. make test-werror, lint and build clean on all three backends; 3044 passed, 0 failed under test, test-asan and test-tsan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U1w7gRc6ASpa7P7HRCBQEw
1 participant
Summary
Closes #123. Two defects, and the second is the more interesting one.
record.format="hevc"ignoredmaxSecondsandmaxMBcompletely. The rawrecorder had no rotation code at all — no segment counter, no threshold check,
no second
open(). A raw recording was one file that grew until the cardfilled, silently: the config validated and
/api/v1/getechoed it back. It isalso why the "lower
maxMB" workaround in #118 did not generalise — on thatpath there was no threshold to lower.
Rotation used to force a keyframe, and no longer does. Since 0.70.0 the TS
recorder asked the encoder for an IDR when a threshold was crossed and none was
coming. It works, but an IDR is a large frame and one per segment raises the
bitrate the link has to carry. On an intra-refresh craft that undoes exactly
what the mode exists for, and under
record.mode=mirrorthe recorder taps thelive channel — so the spike went out over the air for the benefit of a
file. The whole ask (grace period, 1 Hz pacing, bound, and the
take/requeuehand-off at six sites across three backends) is deleted.
Where a segment opens now
A crossed threshold makes rotation due; the next point a decoder can start
from is where it lands. The encoder produces one or the other without being
asked:
19/20) where one exists. Normal GOP recording isunchanged and still cuts on its keyframe — the encoder emits VPS/SPS/PPS
immediately before each IDR (measured 1/1), so it is the same boundary
either way.
32/33/34), the head of a refresh wave,which an intra-refresh stream emits once per GOP. A raw elementary stream has
no container to hold codec config, so this is also the only place a
.hevcsegment can begin and still decode; the picture converges over one wave —
what the ground already does on every tune-in.
Both cases are measured on Star6E, not assumed — the same craft, demuxed at
each
resiliencesetting:racing(800 frames)off(601 frames)The right-hand column is what makes accepting parameter sets safe for normal
GOP: they never appear without an IDR there, so the cut still lands on the
IRAP — which is where it has to land. The left-hand column is what makes it
necessary: waiting for an IRAP alone would leave rotation permanently inert on
the shipped FPV config, which is the defect being fixed, not a hypothetical.
Also fixed, all found reviewing the above
maruko_recorder_write_frame()is the dual and synchronous-fallback path onthat backend, and it never reached the rotation policy — so a change whose
whole point is that
hevcrotates left one of three backends exactly asbroken as before. It now takes the same shared cut. Maruko's TS adapter also
only accepted
19/20, which would have left TS rotation inert on anintra-refresh craft there.
O_TRUNC. The name carries only uptime seconds plus 16bits of nanosecond clock, and after a reboot the uptime restarts — so a
repeat is reachable, and the open destroyed whatever was there. Now
O_EXCLwith a retry, at
start()as well as on rotation. This PR multiplies theexposure (one name per segment rather than one per recording), which makes
it this PR's to fix.
fdatasync()/close()results were discarded when finalising the oldsegment, so a delayed write surfacing there read as a clean rotation. Both
are checked; a failure stops the recorder like a failed reopen.
Evidence
Star6E (SSC338Q,
resilience=racing,gopSize 2.0,sliceCount 6, 100 fps),maxMB=2, patched binary run from the SD card so the stock service and configwere restored untouched:
status.segmentsvs files on diskVPS, SPS, PPSZero IRAPs is the whole point: rotation is driven entirely by boundaries the
encoder was producing anyway, so recording no longer perturbs the live stream.
Control:
record.formatisrestart_required, so no other recorder pathchanged; the craft was verified back on
format: "ts"with venc runningafterwards. Host suite 3034 passed / 0 failed.
Not device-verified: the Maruko writer. That bench has no storage meeting
the recorder's 50 MB free-space precondition (1 MB free, no SD card), so its
fix rests on calling the same shared cut that is verified on Star6E, plus a
pack scan mirroring the accessor Maruko's TS adapter already uses in
production. Saying so rather than letting the table imply all three.
Operator note
On an intra-refresh craft the cut point arrives once per GOP, so segment
granularity is one GOP. A
maxMBfar below one GOP of data still yieldsone-GOP segments — the threshold decides whether to rotate, the wave head
decides when. Documented in
SD_CARD_RECORDING.md.If a stream produces neither kind of point, rotation waits rather than forcing
anything, and logs that it is waiting once, so the case is visible rather than
silent.
Verification
make SOC_BUILD=star6e|maruko|cv610 build, all clean frommake clean. Warnings introduced: none.make test— 3034 passed, 0 failed. Seven ask-specific TS tests weredeleted as obsolete and replaced with cut-point coverage on both recorders,
including the GDR case (no IRAP ever, rotation still fires).
Mutation-checked: restricting the cut point back to IRAP-only — the old
behaviour — fails exactly the 3 GDR assertions.
AGENTS.md:553(~80-line functions):write_au()72,recorder_rotation_due()78.
Blast radius
RecorderRotationis new and shared; the TS recorder's threshold behaviouris otherwise unchanged.
with the two selector helpers they needed. Net for the runtimes is a
simplification.
star6e_recorder_write_au()gains anis_idrparameter. All three callsites already had that value in scope — they were passing it to the TS
recorder in the same
if/else.contract_version0.30.0→0.31.0,VERSION0.83.0→0.84.0.Review
Qodo raised seven findings. Three were already resolved by the redesign that
landed after the review ran. Three were real and are fixed above — the Maruko
one was reported as resolved but was not, and is the most consequential of the
set. One (rotation progress on stderr) I declined: every line this recorder
emits goes to stderr including
started:, so moving one would split arecording's progress across two streams; the worthwhile fix is a consistency
pass over the whole module, not part of this issue.
A test I added for the
O_TRUNCfinding turned out not to discriminate —mutation-checking it by reverting to
O_TRUNCleft it green, because a testcannot provoke a nanosecond-clock name collision. It is kept for what it does
prove and the gap is written into its own comment;
O_EXCLrests onopen(2)semantics, not on that test.
Not in this change
The cut-point test reads
packType.h265Naluonly — deliberately the sameh265-only reading the TS recorder already used, so the two recorders cannot
disagree about where a segment may start. If H.264 recording matters, that is
one change for both rather than a divergence introduced here.
🤖 Generated with Claude Code
https://claude.ai/code/session_01U1w7gRc6ASpa7P7HRCBQEw