Repository navigation
feat(test/ts): grade the full T-STD buffer model - #4640
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
I cross-checked the T-STD check from #4643 against an independent implementation of the same
And one convention. A capture's last access unit is usually cut mid-frame. The check sizes it The first two fail clean streams, which matters once the check gates CI; the third under-reports STD |
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts: - test/ts: keep the branch's split (compliance.py grades the T-STD model, pcr-timing.py owns PCR spacing), taking main's 100 ms TR 101 290 V1.4.1 repetition default and main's description of the exporter's padding. - test/justfile: keep both ts-tstd and drain recipes. - quest/m1/tstd/README.md: keep the branch's Required list and main's Related section. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Merged Conflict resolutions:
Checks: The earlier Interop failure was Child PR #4645 still targets this branch and was left untouched. (Written by Claude Opus 5.5) |
|
Summary: This is the questline parent for T-STD compliant TS export. So far it contains the full ISO 13818-1 T-STD buffer model in Findings
Nits
Verdict: ITERATE (reviewed head 1c2bec1). The harness work is good, but four Required children are open, and #2 should be closed before This is an automated review, not the maintainer's decision |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 25f6a47
Direction: the PCR/T-STD split and positive/negative controls are useful, but iterate before making this model a compliance gate. This head merges main; compliance.py, pcr-timing.py and tstd-controls.py are unchanged from 1c2bec1. I reviewed the current PR diff and checked the existing reports against this head.
Existing findings independently confirmed (referencing the discussions rather than adding duplicate inline threads):
- [P2] Byte-granular audio delivery: test/ts/compliance.py:946–955,1033–1038 delivers an entire packet only when its last byte leaves TB. A focused 576-byte-frame case completes byte-exactly before PTS but is reported 0.352 ms late. Transfer payload progressively, with transport/PES headers accounted for, and add an audio frame ending partway through a packet as a passing control.
- [P2] Grade complete trailing units: test/ts/compliance.py:957–974 stops at the capture horizon. In a two-unit fixture, a fully received unit held for 1.299 s is silently omitted; extending only the horizon exposes the missing delay violation. Drain complete units through their decode deadlines at final EOF, separately handling truncated units and clock discontinuities. Assert STD-delay failures in the fast-restamp controls.
- [P2] Bound numerical error: test/ts/compliance.py:932,1001–1014,1049 compares cumulative floating-point bytes with a fixed 1e-9-byte epsilon. The reported 286,429,515.9999984 versus 286,429,516 still evaluates as underflow. Use stable/exact accumulation or a justified sub-byte error bound and add a long-stream regression. These three confirm the existing independent-grader report; I did not rerun its capture set.
- [P2] Require PCR on each declared program clock: test/ts/compliance.py:284–288 checks only that both lists are nonempty. Declaring PID 256 while only PID 999 carries PCR returns PASS; the affected program then has no modelled time base (1088–1091). Validate declared PCR PIDs individually and add a mismatched-PID negative control. This confirms the existing review discussion.
The end-to-end quest remains incomplete: #4645 is open (now ready, not draft), and this head still leaves tstd report-only. Verification: Python compilation and focused in-memory model/check reproductions passed as described; TSDuck is unavailable here, so the nine-control suite, real captures and loss/netem proof were not run. Head CI was queued when checked.
Audio bytes enter B as they leave TB, so a frame ending partway through a packet is no longer flagged late. Complete units are graded through their decode times after the stream's last packet, and a trailing unit cut off by the capture is counted, not graded. EB snaps to the exact offset whenever MB empties and completeness tolerates under a bit, so float drift cannot fail a long stream. pcr-presence checks each declared PCR PID. Controls cover the straddling audio frame, the STD delay on the fast restamps, and a mismatched PCR PID. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A unit decoded partway through a packet advanced the removal pointer before that packet's bytes were counted, hiding the peak. Count what of the next packet has already left TB, and add a mid-packet overflow control. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Adversarial Codex review (peer-review), branch diff against main, final head dae164a. AI review, suggestions only.
Round 1 (head d11b534), needs-attention:
- [high] Audio B overflow was checked only once a whole packet left TB, so a unit decoded mid-packet advanced
outfirst and hid the peak. Fixed in 862d877: B now also counts what of the next packet has left TB just before each removal, with amid-packet overflownegative control (fails on the previous model, x20 B overflow now).
Round 2 (862d877, same test code as the final head), needs-attention:
- [medium] A time base with fewer than two PCRs after a signalled discontinuity is dropped silently, so its units go ungraded and the check can PASS. Not fixed here: pre-existing, needs a capture tail cut right after a discontinuity, and
tstdis still report-only. Listed as a follow-up for before the gate flips in #4645, since warning on it could also fail clean captures that end just after an export restart.
Earlier findings on this PR, all addressed on this head:
- Byte-granular audio delivery (t0ms 1, OpenAI review): audio bytes enter B as they leave TB;
straddling audiocontrol (576 B frames ending at byte 41 of a packet, decoded 0.3 ms after) passes, fails before. - Trailing complete units (t0ms 3): units are graded through their decoding times after the stream's last packet. The 4x and 15x restamps now report
held over 1 sx132 and x142 per audio PID, matching t0ms's independent counts, and the controls require it. - Float drift (t0ms 2): EB snaps to the exact ES offset whenever MB empties, and completeness tolerates under one bit. No discriminating control: neither the Kyrion capture nor the synthetic produce a partial leak at a unit boundary even with offsets moved past 2^46 B. t0ms's offered long-AVC control would close that.
- Truncated last unit (t0ms convention): counted as
truncated_units, not graded. - Per-PID PCR presence (Grok 2, OpenAI review):
pcr-presencefails any declared PCR PID (other than 0x1FFF) with no PCR;PCR PID mismatchcontrol. - Perf (Grok 3): FIFOs are deques and each segment bisects its packet range.
- Justfile orphaned comment (Grok nit): merged.
read_pesstride vs resync (Grok nit): not changed; sync loss already hard-fails.- Open children (Grok 1): questlines are flat now, so the line lands on
mainand #4645 retargets tomain.
Checks on this head: just check, just test ts-tstd (12/12), just test interop --all (all rows pass), quest check.
(Written by Claude Opus 5.5)
|
Follow-up review after the push to Earlier findings
New, non-blocking
Verdict: MERGE once CI is green (reviewed head This is an automated review, not the maintainer's decision |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: dae164a
Incremental review since 25f6a47, separating the merged main changes.
The four previous findings are addressed: per-declared-PID PCR validation (test/ts/compliance.py:283–296), byte-granular audio completion and pre-removal B peaks (:974–988,1109–1122), complete trailing-unit removal (:990–1015), and exact-offset resets plus sub-bit completion tolerance (:1047–1068,1105). The new controls cover straddling audio, mid-packet overflow, PCR mismatch, and held-over-1-s failures (test/ts/tstd-controls.py:245–256). No new actionable defect found in this delta.
Direction: these changes make the report-only model substantially more credible. Retain the documented follow-up for ungraded short PCR time bases (:1160–1162) before promoting it to a hard gate; this is an existing limitation, not a new finding.
Verification: Python compilation passed. Directly ran the current synthetic audio builder/parser/model: straddling audio passed with 79 graded/1 truncated unit; the overflow control recorded 20 B overflows; a 1.3-s delay recorded 79 held-over-1-s violations. The mismatched-PID check failed as expected. TSDuck and real-capture/long-stream/netem validation were not run here. Current-head Interop fails earlier at max_age_relay_javascript (max_age.rs:145, JS publisher not ready), so T-STD controls were skipped; CI has not validated this model yet.
|
Re-graded the 35-file corpus from my earlier comment against |
The T-STD questline (#4640) landed on main, so this branch now targets main. - Harness files merged against the questline head the branch started from. - Quests: delay.md and byte-schedule.md are done and deleted here; references repointed, and the T-STD README records t0ms's CNN grading. - #4577's per-stream export stats ported onto the slot schedule: a unit counts once the slot carrying its first packet is returned. Export::stats returns ts::stats::Export (rows plus the release clock's dropped, drift and out_of_tolerance), with From<stats::Export> for ts::Stats for stats::Log; ts::export is private again. moq export ts logs the release counters. - #4652 skips a stalled group once the next group's start falls the budget behind, so the export's sources skip at half the delay: with the whole delay the group after a skip arrived at its deadline. - The jitter buffer waits only for audio, video and PES tracks, not sparse sections. - At end of stream a past-due unit ignores its decoder buffer, so an oversized last unit cannot hold the stream open. - Tests: a timeline restarting at zero fails the export (#4725); main's new export tests moved to with_delay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
The TS harness graded only the T-STD transport buffer, so
moq export tsoutput could overflow or underflow the decoder buffers behind it and still pass. The T-STD questline needs a check that can tell compliant delivery from broken delivery before the export is fixed against it.Approach
test/ts/compliance.pyruns every audio and video stream through the full ISO 13818-1 T-STD (TB, MB, EB, B) on the stream's own PCR clock, with buffers from the SPS HRD or the level (AVC, HEVC) and from H.222.0 / ATSC for ADTS, MPEG audio, AC-3, E-AC-3 and Opus. It stays report-only (tstdis a shape check) until fixed-delay release flips the gate.truncated_units, not graded.pcr-presencechecks each declared PCR PID individually.pcr-timing.pyowns PCR spacing and the byte schedule; sync and continuity are graded only under--live.just test ts-tstd(tstd-controls.py, wired into Interop) proves the model: a Kyrion broadcast capture passes, its PCR restamps at 0.7x/4x/15x fail as named (including audio held over 1 s), and synthetic layouts cover refusal, adaptation bursts, duplicates, a mismatched PCR PID, a straddling audio frame (pass) and a mid-packet B overflow (fail).check.mdis done and deleted;send-ahead.mdandmux-rate-hold.mdare added under the line, which now stays flat onmain.Impact
just test ts-tstdis new,run.shnow needs TSDuck'ststables, andpcr-presencefails a declared PCR PID that carries no PCR.Alternatives
tstdnow: today's export fails it on delivery timing, so the gate flips in feat(moq-mux)!: fixed-delay jitter buffer and per-PID T-STD admission for TS export #4645.Follow-ups
tstdto hard.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)