feat(rldb): h264 image storage end-to-end, default on, with format detection

[ElmoPA]

Per-frame JPEG codes every frame independently and cannot exploit the temporal
redundancy of 30fps video. Measured on real fold episodes:

JPEG (q75, as shipped)   44.8 KB/frame   8.33 ms/frame decode
h264 crf15               20.2 KB/frame   1.68 ms/frame decode
                         2.2x smaller    5.0x faster

At equal bytes h264 also wins on PSNR (+2.4 to +2.9 dB in the 4-8 KB range), so
this is not a quality-for-size trade. It is now the DEFAULT; set
EGOVERSE_IMAGE_CODEC=jpeg to opt a run back out.

WRITER (new): one self-contained mp4 per frames_per_chunk frames, stored as the
elements of a VariableLengthBytes array -- not one blob per episode, which would
force a span read to pull the whole episode (~68 MB) to decode any window. Wired
into BOTH write paths: the incremental handle (buffers frames, flushes on chunk
boundaries and at close) and the bulk write() converters use.

READER (new): frame -> chunk resolution before any slice, since a frame-range
read of a chunk-indexed array returns the wrong elements entirely.

Because both encodings now exist across a dataset, the reader DETECTS the format
rather than trusting features[key]["dtype"], escalating only on disagreement:

1. the declared dtype;
2. the element count -- a full-length array is JPEG, a much shorter one is
   video. Compared with ">=" because the writer pads past total_frames (a
   290-frame episode occupies 300 slots);
3. the magic bytes of element 0, read only to break a tie, and authoritative
   when read.

Verified on 72 real episodes across 6 datasets: identical classification and
ZERO payload reads, so the common path costs nothing. Round trip at 450 frames
(a partial tail chunk) over all four write paths: frames come back at the right
indices across chunk seams, worst mean abs error 0.0048.

Also fixes a pre-existing bug this exercised: ZarrWriter.add_frame assigned raw
bytes to a VLenBytes element, which zarr rejects with "Expected bytes, got
numpy.ndarray". Confirmed against stock main with these changes reverted.
add_frames already used the object-array holder; add_frame now does too.

Known gap: egomimic/test_zarr.py validates only dtype=="jpeg" keys, so it
silently skips images on video episodes rather than validating them.

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_012V58H37tmcvgDthELMd5Xk

[ElmoPA]

• feat(rldb): h264 image storage end-to-end, default on, with format detection #545 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Claude Code Review

Review: PR #545 — h264 image storage

Summary

Introduces chunked h.264 storage for image arrays as the new default codec, with format auto-detection on read so JPEG and video episodes coexist. Also fixes a latent VLenBytes scalar-assignment bug in add_frame.

Key concerns

1. Default-flip is a large, silent behavior change across the whole dataset ecosystem. Every new episode written after this merge will be h264. This ships without:

   • A migration/rollout note or feature flag defaulting to jpeg for one release.
   • Confirmation that all active training runs (ACT, HPT, Pi 0.5) exercise the new read path — the description lists a round-trip test, not a training smoke test. Given the reader path in getitem has new logic (_video_frame, decode_video_span, chunk cache), I'd want at least one training config validated end-to-end before this becomes default.
   • Norm stats implications: h.264 is lossy differently than JPEG q75. If any cached norm stats were computed from JPEG-decoded pixels, models retrained on h264 data will see a mild distribution shift. Worth flagging even if tolerable.

2. test_zarr.py explicitly skips video episodes (per the description's own "known gap"). This means CI silently loses image validation coverage for the default codec. This should not merge as-is — either extend test_zarr.py to accept dtype in VIDEO_DTYPES, or gate the default flip until it does.

3. Ray env-var forwarding is fragile. run_conversion.py was patched, but any other Ray/distributed entrypoint (training DDP workers don't need it, but any other conversion or eval script that spawns workers does) will silently diverge from the driver. Consider centralizing the "EgoVerse env passthrough" list, or logging the resolved codec on the worker so drift is visible.

4. _ENCODING_MISMATCHES is a class-level mutable set. In a multi-worker DataLoader (fork) this is fine, but under spawn each worker gets its own copy and you'll get one warning per worker per mismatch. Minor, but the dedup guarantee in the docstring is overstated.

5. VIDEO_CHUNK_CACHE_SIZE=4 default with frames_per_chunk=300 at 640×480×3 ≈ ~1.1 GB per dataset instance worst case. With N DataLoader workers × M datasets open concurrently this is not trivial. The docstring notes the tension but the default is aggressive. Consider defaulting to 2, or sizing in MB rather than chunk count.

6. resolve_encoding tie-break on 1-frame episodes falls through to verdict (the declared dtype) without sniffing, even when read_first is available. If an operator manually wrote a 1-frame test episode with wrong metadata, it won't self-correct. Edge case, but the docstring claims payload is authoritative — it isn't here.

7. decode_chunk unwrapping loop: while isinstance(blob, np.ndarray) with blob[0] on non-scalar arrays could infinite-loop on a 0-length array (blob[0] raises IndexError, which is fine) or on an array of arrays. Defensive, but worth a bounded loop or an explicit shape check.

Suggestions

• Blocker: Update test_zarr.py to validate VIDEO_DTYPES keys (decode chunk 0, check shape matches features[key]["shape"]). Otherwise the default codec is untested in CI.
• Blocker or strong request: Default EGOVERSE_IMAGE_CODEC=jpeg for one release cycle; flip to h264 default in a follow-up after training runs on real hardware confirm no regression. Alternatively, document in the PR which training runs have been validated end-to-end (not just round-trip).
• Log the resolved codec + settings once at writer init and once at reader init per dataset, so post-hoc it's obvious which encoding a run used.
• Add a smoke test that writes 450 frames via add_frame AND via write(), reads them back through ZarrDataset.__getitem__ with a horizon crossing a chunk boundary, and asserts pixel MAE < threshold. Round-trip via the codec module alone doesn't cover the _video_frame / decode_video_span dispatch.
• Consider making frames_per_chunk adaptive: 300 is right for span reads, but ZarrActionExpertDataset mostly does single-frame reads (BOS/t/EOS) — the docstring even calls this out. Datasets consumed frame-at-a-time should probably write with fpc=30 (1s/GOP).
• The _video_meta fallback path (getattr(self, "_video_meta_cache", None)) suggests worry about __init__ ordering. If it's genuinely needed, add a comment; if not, drop the getattr.
• Rename _ENCODING_MISMATCHES → _encoding_mismatches_logged and make it instance-level, or use logging.warning once-per-key with functools.lru_cache — the current dedup only works within a single process's dataset instances that happen to share the class.

Verdict: Request Changes

The engineering is solid and the writeup is excellent, but flipping the default codec while test_zarr.py silently skips validating the default is not something I want landing on main. Fix the test coverage gap (blocker), and either delay the default flip or attach evidence of an end-to-end training run on h264 data. Everything else is polish.

---

Reviewed by Claude · Review workflow