feat(rldb): detect JPEG vs H264 image encoding instead of trusting dtype

[ElmoPA]

ZarrDataset dispatched purely on features[key]["dtype"]. That is a claim, and a
stale claim routes real mp4 payloads into simplejpeg -- which fails deep in the
decoder, far from the actual cause. Episodes converted before the codec switch,
or whose metadata was copied from a sibling, could not be loaded at all.

_classify_image_keys now uses three signals, cheapest first, escalating only on
disagreement:

1. the declared dtype;
2. the element count -- per-frame JPEG stores one element per frame, chunked
   video one mp4 per frames_per_chunk, so a full-length array is JPEG and a
   much shorter one is video. Compared with ">=" because writers pad 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.

Also fixes two gaps this exposed:

• video keys with no "video" metadata block now recover frames_per_chunk by
  decoding chunk 0. It is not derivable arithmetically -- 1000 frames over 4
  chunks admits any fpc in (250, 333].
• ZarrDataset.getitem had NO video branch, only _read_span did, so a
  detected video key hit a frame-indexed read of a chunk-indexed array and
  returned the wrong elements. It now mirrors _read_span, matching
  decode_jpeg_single for horizon=None and _pad_sequences at the episode tail.

Mismatch warnings are deduped per (key, declared, detected) so a systematically
mislabelled dataset logs once, not once per episode.

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

[ElmoPA]

• feat(rldb): detect JPEG vs H264 image encoding instead of trusting dtype #543 👈 (View in Graphite)
• main

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

[github-actions]

Claude Code Review

Review

Summary

Replaces trust-the-dtype dispatch in ZarrDataset with a three-signal detector (declared dtype → element count → magic bytes), and closes two follow-on gaps: frames_per_chunk recovery when the video block is missing, and a missing video branch in __getitem__. Sensible and well-motivated fix for a real load failure mode.

Key concerns

1. Class-level mutable state for mismatch dedup. _ENCODING_MISMATCHES: set = set() is a class attribute shared across all ZarrDataset instances and across DDP workers. Under Lightning DDP with multiple dataloader workers, each worker process gets its own copy (fine for dedup within a process), but the set will persist and grow for the process lifetime. Not a real leak (bounded by |keys| × |declared| × |detected|), but worth a comment. More importantly: no lock. If dataset construction ever moves to threads, the if sig not in seen: seen.add(sig) is racy. Low risk given current usage, but flag it.

2. by_count disagreement with declared, but sniff returns None → verdict silently flips. In the branch:

   elif not sniffed and by_count and by_count != verdict:
       verdict = by_count

   This overrides declared dtype based purely on element count when bytes couldn't be sniffed (e.g. arr[0] returned something _as_bytes couldn't unwrap). Count is a heuristic — a truncated or malformed array could easily satisfy n < total_frames. Consider logging when this fallback fires; right now it silently reclassifies with no diagnostic.

3. self.total_frames assumed populated. _classify_image_keys is called from init_episode and reads self.total_frames. Confirm this is set on self before this call (the diff doesn't show it). If it's only in metadata, use self.metadata.get("total_frames") explicitly to avoid an AttributeError on some episodes.

4. arr[0] on a video array reads and decompresses an entire mp4 chunk just to sniff 8 bytes. The docstring claims "ZERO payload reads on the common path" — true when count and declared agree, but any disagreement (including legitimately mislabeled episodes, which is the whole point of this PR) triggers a full chunk fetch. Consider reading a bounded byte slice if the zarr codec allows, or at minimum note this cost in the docstring. Not blocking.

5. __getitem__ video branch — end_idx bound not clamped. _read_span presumably clamps against total_frames before calling decode_video_span; the new branch computes end_idx = self._chunk_end_idx(idx, horizon, key_type) and hands it straight to decode_video_span. If _chunk_end_idx returns something past the episode, does decode_video_span handle it? Please verify parity with _read_span — the comment says "mirrors _read_span" but I'd like to see them side by side in a test.

6. _pad_sequences behavior parity. Inline padding via np.repeat(span[-1:], ...) — confirm _pad_sequences does exactly this (repeat last frame, not zero-pad, not edge-reflect). If _pad_sequences ever changes, these two paths silently diverge. Prefer calling _pad_sequences directly.

Suggestions

• Call self._pad_sequences(span, horizon) (or whatever the existing helper is named) instead of duplicating the pad logic in __getitem__.
• Add a unit test with three fixtures: (a) episode declaring jpeg with jpeg bytes, (b) declaring jpeg with mp4 bytes (the bug this PR fixes), (c) declaring h264 with no video metadata block. Assert classification and that __getitem__ returns correctly-shaped frames for horizon=None and horizon>1.
• Add a test for the 1-frame-episode ambiguous case — the code correctly punts to sniffing, but that path should be pinned.
• Log at INFO when frames_per_chunk is recovered via frames_per_chunk_from_data (the inferred=True flag is set but nothing surfaces it). Silent recovery is exactly the pattern this PR is trying to move away from.
• The _as_bytes unwrap loop terminates on len(blob) for ndarrays but doesn't guard against a 0-d ndarray whose .item() returns another ndarray (unlikely but zarr VLenBytes has surprised us before). Add a max-depth guard.
• Docstring on _classify_image_keys says "keeps its declared classification" for unreadable payloads — verify: in the code, if arr is None or n is None, we fall through to if verdict == "h264" using the declared value, which is correct. Good.

Verdict: Request Changes

The core detection logic is sound and the bug is real, but I want (1) test coverage for the mp4-declared-as-jpeg case and the __getitem__ video branch, (2) _pad_sequences reuse rather than inline duplication, and (3) a log line when count-based reclassification fires without sniff confirmation. After those, happy to approve.

---

Reviewed by Claude · Review workflow