Skip to content

fix(video): validate pass-through fixed GOP cadence - #380

Open
victorwon2001 wants to merge 1 commit into
Hebbian-Robotics:mainfrom
victorwon2001:fix/issue-376
Open

fix(video): validate pass-through fixed GOP cadence#380
victorwon2001 wants to merge 1 commit into
Hebbian-Robotics:mainfrom
victorwon2001:fix/issue-376

Conversation

@victorwon2001

Copy link
Copy Markdown
Contributor

Summary

  • Validate pass-through H.264 keyframes against the fixed-GOP message grid derived from configured gop_seconds and each channel's measured frame rate before provenance is written.
  • Report off-grid canonical video through the new video-keyframe-cadence doctor error while preserving existing malformed-payload and stream-start diagnostics.
  • Add regression coverage for the reported 100-message off-grid stream, exact-grid byte preservation, and independent channels sharing one topic, and document the pass-through requirement and finding code.

Fixes #376

Why

Pass-through episodes previously stamped gop_seconds as the interval actually used without verifying that their H.264 keyframes followed that cadence. Refusing off-grid pass-through streams and checking existing files in hflow doctor makes that provenance assertion verifiable without changing the provenance schema or re-encode behavior.

Validation

Focused and full test suites passed, and lint, formatting, type checking, documentation links, and diff whitespace checks are clean.

uv run pytest -q tests/test_processing_regressions.py tests/test_doctor.py tests/test_transform.py tests/test_video.py
uv run ruff check --fix
uv run ruff format
uv run ty check
lychee --no-progress --include-fragments --exclude '^https://github\.com/Hebbian-Robotics/hflow/(issues|security/advisories/new)$' --exclude-path references/mcap-spec.md --exclude-path references/foxglove-CompressedVideo.proto .
uv run pytest -q
git diff --cached --check

Checklist

  • I added or updated outcome-focused tests for changed business logic.
  • I updated documentation for changed behavior, flags, formats, or requirements.
  • I ran uv run ruff check --fix, uv run ruff format, and uv run ty check.
  • I ran the relevant pytest suite.
  • I did not add recordings, generated media, credentials, private URLs, or runtime artifacts.
  • I preserved stored-data compatibility or documented an explicit version change.

@kstonekuan kstonekuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding this rather than merging it, and the reason is about #376 rather than about your code. The work is sound: gate is clean on the merged result, 1451 passed / 6 skipped, and the cadence derivation from gop_seconds and each channel's measured frame rate is the right way to compute the grid.

#376 is deliberately open on what to do, not just how. It names three options: refuse off-grid pass-through, measure and stamp the real interval, or report it as a doctor finding and leave refusal to callers. This PR implements the first and the third. Merging it would settle that choice by landing it, and the first option is the one with the largest blast radius, so it is the one I asked not to be decided by default.

Timing was against you here. The issue's author posted a measurement 36 minutes after you opened this, and it speaks directly to the refusal half. Scanning nominal-io/xplane-mcap (the 24,689-message corpus src/hflow/video.py:513 records this repo's lossless-suffix guarantee as validated on):

video messages scanned:      24689
messages WITHOUT an AUD:     24689
keyframe-bearing (IDR):      103   (agrees with the corpus's own video/keyframes channel)
keyframe spacing:            nominal 250, holds for 93 of 102 intervals
off-grid intervals:          9, tightest at 39 messages

That source needs AUD repair to conform, repair cannot move keyframes, and so it is a conforming pass-through stream with an irregular grid. Your _validate_passthrough_video_payload change would refuse it. Refusing a recording this repository used to validate a shipped code path is a real cost, and it is not one anybody had measured when you started.

None of that is wasted. Whichever direction #376 lands on, something has to measure cadence from the access units, and your derivation plus the video-keyframe-cadence finding are the parts most likely to survive intact: a doctor finding is wanted under all three options. The piece that is genuinely contingent is the SourceNotConforming raise in the transform.

So: leaving this open rather than closing it, and no changes to push yet. When #376 names a direction I will say so here, and if it is anything other than "refuse", the reshape is smaller than rewriting.

One thing worth doing regardless, if you want to keep moving on it: _validate_passthrough_video_payload returning bool for the keyframe flag makes a validation function carry a second job in its return type. A separate accessor, or collecting the flag where the access unit is already in hand, keeps the validator's contract "raises or does not". That is a comment on the code and it holds whichever way the design goes.

@kstonekuan

Copy link
Copy Markdown
Contributor

#376 now has a direction, so here is where this stands.

The refusal comes out. The SourceNotConforming raise in _validate_passthrough_video_payload is the one piece the measurement ruled against: it would reject nominal-io/xplane-mcap, a conforming pass-through source this repo used to validate its AUD repair. Pass-through exists to avoid re-encoding, and a refusal only satisfiable by re-encoding takes that back.

Your doctor half stays. video-keyframe-cadence is wanted, and the cadence derivation from gop_seconds and each channel's measured frame rate is the part that was always going to be needed. Keep both.

What is new work: the provenance record stops copying the configured interval on the pass-through path and reports the stream instead. The rule is that it reports the measurement, or reports that there is not one; a stream with spacing 7, 1, 82, 1 has no single interval and the record must not invent one. The encoding is your call, with a note in the PR saying which and why. The re-encode path does not change.

Two things to plan for. content_episode_id hashes the canonical file and provenance is written into it, so every episode's content ID moves and the behaviour version has to move with the change rather than after it. And the re-encode path's output should be byte-identical afterwards; if it is not, something is wrong.

If you would rather not carry the provenance half, say so and land the doctor finding and the cadence derivation on their own. That is a clean, useful PR by itself and the record change can follow separately. Either way this is not starting over.

Also still worth doing whichever shape you pick: _validate_passthrough_video_payload returning bool gives a function whose contract is "raises or does not" a second job. Collect the keyframe flag where the access unit is already in hand instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The fixed-GOP promise is stamped into provenance as "actually used" but is never measured on pass-through video

2 participants